diff --git a/apps/desktop/src/app/contrib/controller.tsx b/apps/desktop/src/app/contrib/controller.tsx index 7243f7dc90..b9dc972c15 100644 --- a/apps/desktop/src/app/contrib/controller.tsx +++ b/apps/desktop/src/app/contrib/controller.tsx @@ -13,21 +13,20 @@ import { LayoutTreeRoot } from '@/components/pane-shell/tree/renderer' import type { DoubleTapContext } from '@/components/pane-shell/tree/renderer/drag-session' import { $layoutTree, + bindPaneVisibility, bindToolPaneCollapse, bindTreeSideVisibility, declareDefaultTree, dismissTreePane, dockPaneBeside, - isToolPaneVisible, + isPaneVisible, mirrorLayoutTree, paneRootSide, registerLayoutResetHandler, registerPaneCloser, - registerPaneOpener, resetLayoutTree, revealTreePane, - setTreePaneHidden, - toggleToolPane, + togglePaneVisible, watchContributedPanes } from '@/components/pane-shell/tree/store' import { SidebarProvider } from '@/components/ui/sidebar' @@ -55,7 +54,7 @@ import { SIDEBAR_MAX_WIDTH } from '@/store/layout' import { $previewOpenRequest, $previewTabs, closeRightRail } from '@/store/preview' -import { $reviewOpen, closeReview, REVIEW_PANE_ID } from '@/store/review' +import { $reviewOpen, closeReview, openReview, REVIEW_PANE_ID } from '@/store/review' import { $currentCwd, $selectedStoredSessionId, $sessions, $yoloActive, sessionMatchesStoredId } from '@/store/session' import { watchSessionPins } from '@/store/session-pin-sync' import { $statusbarVisible } from '@/store/statusbar-prefs' @@ -472,27 +471,9 @@ registerLayoutResetHandler(stackSessionTilesIntoMain) // toggle mirrors the root row. // --------------------------------------------------------------------------- -function bindPaneVisibility( - paneId: string, - $open: { get(): boolean; listen(fn: (open: boolean) => void): void }, - close?: () => void, - open?: () => void -) { - setTreePaneHidden(paneId, !$open.get()) - $open.listen(isOpen => setTreePaneHidden(paneId, !isOpen)) - - // The tab menu's Close routes through the owning store (never dismissal), - // so the pane's toggle buttons stay truthful. - if (close) { - registerPaneCloser(paneId, close) - } - - // The opener is the mirror: preset application (revealOnPreset) shows the - // pane through the same store, so the toggle stays truthful. - if (open) { - registerPaneOpener(paneId, open) - } -} +// HIDE-STYLE PANES (files, review, preview): the binding lives in the tree +// store — bindPaneVisibility — alongside bindToolPaneCollapse, so both are +// testable against the real function instead of a copy. // TOOL PANELS (terminal, logs): the binding lives in the tree store — // bindToolPaneCollapse — so the boot rule it encodes is testable against the @@ -549,15 +530,25 @@ const $hasWorkspace = computed($currentCwd, cwd => Boolean(cwd.trim())) // The tree pane's own presence tracks ⌘J directly, not just the column's // collapse — otherwise revealing a preview (which opens that shared column) // would drag the tree along with it. See revealPreview. +// +// Both get a CLOSER and an OPENER. The closer keeps ⌘J/⌘G truthful when the +// pane is closed from the tab menu; the opener is its mirror, so bringing the +// pane back through the tree (the toggle's reveal path, the rail, a preset) +// writes the store too. Without the opener the boolean went stale the moment +// anything but the toggle showed the pane — the divergence this whole change +// is about. bindPaneVisibility( 'files', - computed([$hasWorkspace, $fileBrowserOpen], (workspace, open) => workspace && open) + computed([$hasWorkspace, $fileBrowserOpen], (workspace, open) => workspace && open), + () => setFileBrowserOpen(false), + () => setFileBrowserOpen(true) ) // ⌘G — the review sidebar appears/disappears (and comes to the front). bindPaneVisibility( 'review', computed([$reviewOpen, $hasWorkspace], (open, workspace) => open && workspace), - closeReview + closeReview, + openReview ) // ⌃` / statusbar toggle — the terminal COLLAPSES to a rail (tab stays), not // hides; PTYs stay alive while collapsed (see PersistentTerminal). @@ -593,8 +584,8 @@ registry.register( keywords: ['logs', 'agent log', 'tail', 'debug'], // On-screen, not the store's boolean: logs stacks with the terminal, and // behind its sibling's tab `$logsOpen` stays true while nothing is visible. - get: () => isToolPaneVisible('logs'), - set: () => toggleToolPane('logs') + get: () => isPaneVisible('logs'), + set: () => togglePaneVisible('logs') }) ) diff --git a/apps/desktop/src/app/hooks/use-keybinds.ts b/apps/desktop/src/app/hooks/use-keybinds.ts index 3b3677815c..32d9caaba2 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -7,9 +7,9 @@ import { closeActiveTerminal, createTerminal, cycleTerminal } from '@/app/right- import { activateTreeTabSlot, cycleTreeTabInFocusedZone, - isToolPaneVisible, + isPaneVisible, layoutHasRootSide, - toggleToolPane + togglePaneVisible } from '@/components/pane-shell/tree/store' import { onReleaseTypingFocus } from '@/components/ui/keyboard-first' import { findBarClaimsCombo } from '@/lib/find-in-page' @@ -190,11 +190,11 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { // terminal-on-bottom) would leave it a dead key, so it falls back to the // terminal there. The single "secondary panel" toggle. 'view.toggleRightSidebar': () => - layoutHasRootSide('right') ? toggleFileBrowserOpen() : toggleToolPane('terminal'), + layoutHasRootSide('right') ? toggleFileBrowserOpen() : togglePaneVisible('terminal'), 'view.toggleReview': toggleReview, 'view.toggleStatusbar': toggleStatusbarVisible, 'view.showFiles': showFiles, - 'view.showTerminal': () => toggleToolPane('terminal'), + 'view.showTerminal': () => togglePaneVisible('terminal'), // Create first so the pane's open-effect ensure sees a non-empty set and // doesn't also spawn one — net effect is exactly one fresh terminal. 'view.newTerminal': () => { @@ -204,9 +204,9 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { // Switch / close only act while the terminal is actually ON SCREEN — ask // the tree, not the toggle store (which stays true behind a stacked // sibling tab or a minimized zone). - 'view.nextTerminal': () => isToolPaneVisible('terminal') && cycleTerminal(1), - 'view.prevTerminal': () => isToolPaneVisible('terminal') && cycleTerminal(-1), - 'view.closeTerminal': () => isToolPaneVisible('terminal') && closeActiveTerminal(), + 'view.nextTerminal': () => isPaneVisible('terminal') && cycleTerminal(1), + 'view.prevTerminal': () => isPaneVisible('terminal') && cycleTerminal(-1), + 'view.closeTerminal': () => isPaneVisible('terminal') && closeActiveTerminal(), 'view.flipPanes': togglePanesFlipped, // ⌘W: close the focused tab (terminal / preview target / zone tree tab). // On the main tab with session tabs stacked, it shifts the next one in — diff --git a/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx b/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx index fa81c227ad..adedab3fa5 100644 --- a/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx +++ b/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx @@ -5,7 +5,7 @@ import type { CommandCenterSection } from '@/app/command-center' import { useApprovalModeStatusbarItem } from '@/app/shell/approval-mode-menu' import { ContextUsagePanel } from '@/app/shell/context-usage-panel' import { GatewayMenuPanel } from '@/app/shell/gateway-menu-panel' -import { $toolPaneVisible, toggleToolPane } from '@/components/pane-shell/tree/store' +import { $paneVisible, togglePaneVisible } from '@/components/pane-shell/tree/store' import { Codicon } from '@/components/ui/codicon' import { GlyphSpinner } from '@/components/ui/glyph-spinner' import { useI18n } from '@/i18n' @@ -95,7 +95,7 @@ export function useStatusbarItems({ // What the button paints and flips is whether the terminal is ON SCREEN — // the takeover store alone stays true behind a stacked sibling tab or a // minimized zone, which lit the button for a pane the user couldn't see. - const terminalShowing = useStore($toolPaneVisible('terminal')) + const terminalShowing = useStore($paneVisible('terminal')) const primaryBusy = useStore($busy) const currentCwd = useStore($currentCwd) // Derive the workspace's project name from the already-cached project tree @@ -513,7 +513,7 @@ export function useStatusbarItems({ hidden: !chatOpen, icon: , id: 'terminal', - onSelect: () => toggleToolPane('terminal'), + onSelect: () => togglePaneVisible('terminal'), title: terminalShowing ? copy.hideTerminal : copy.showTerminal, toggleLabel: copy.toggleTerminal, variant: 'action' diff --git a/apps/desktop/src/components/pane-shell/tree/pane-toggle-visibility.test.ts b/apps/desktop/src/components/pane-shell/tree/pane-toggle-visibility.test.ts new file mode 100644 index 0000000000..1f57ed0158 --- /dev/null +++ b/apps/desktop/src/components/pane-shell/tree/pane-toggle-visibility.test.ts @@ -0,0 +1,126 @@ +import { atom } from 'nanostores' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { registry } from '@/contrib/registry' + +import { group, split } from './model' +import { + $dismissedPanes, + $hiddenTreePanes, + $layoutTree, + bindPaneVisibility, + isPaneVisible, + setTreeGroupMinimized, + togglePaneVisible +} from './store' + +// The bug class, across EVERY pane kind — not just the terminal she reported. +// +// A toggle that flips its own boolean diverges from the tree the moment +// anything else moves the pane: stacked behind a sibling tab, folded into a +// minimized zone, closed with ⌘W. The store then says "open" while nothing is +// on screen, the press re-asserts a value it already held, and the key reads +// as dead. Same shape for the COLLAPSE-style tool panels (terminal, logs) and +// the HIDE-style panes (files, review) — proven here with one table. + +const disposers: (() => void)[] = [] + +/** Bind through the REAL production function, with the closer/opener pair the + * controller gives it. */ +function bindVisibility(paneId: string, $open: ReturnType>) { + bindPaneVisibility( + paneId, + $open, + () => $open.set(false), + () => $open.set(true) + ) +} + +beforeEach(() => { + window.localStorage.clear() + $dismissedPanes.set(new Set()) + $hiddenTreePanes.set(new Set()) + + for (const [id, data] of [ + ['workspace', { placement: 'main', uncloseable: true }], + ['files', { placement: 'right' }], + ['review', { placement: 'right' }] + ] as const) { + disposers.push(registry.register({ area: 'panes', data, id, render: () => null, title: id })) + } +}) + +afterEach(() => { + disposers.splice(0).forEach(dispose => dispose()) +}) + +describe('a hide-style pane stacked behind a sibling tab', () => { + it('comes forward on the first press instead of swallowing it', () => { + $layoutTree.set( + split('row', [ + group(['workspace'], { active: 'workspace', id: 'g-main' }), + group(['files', 'review'], { active: 'files', id: 'g-right' }) + ]) + ) + + const $review = atom(true) + bindVisibility('review', $review) + + // The column is open but showing FILES, so the diff is not on screen — + // and $reviewOpen is true, which is what made ⌘G a no-op. + expect(isPaneVisible('review')).toBe(false) + + togglePaneVisible('review') + + expect(isPaneVisible('review')).toBe(true) + }) +}) + +describe('a hide-style pane inside a minimized zone', () => { + it('un-minimizes on the first press', () => { + $layoutTree.set( + split('row', [ + group(['workspace'], { active: 'workspace', id: 'g-main' }), + group(['files'], { active: 'files', id: 'g-files' }) + ]) + ) + + const $files = atom(true) + bindVisibility('files', $files) + setTreeGroupMinimized('g-files', true) + + expect(isPaneVisible('files')).toBe(false) + + togglePaneVisible('files') + + expect(isPaneVisible('files')).toBe(true) + }) +}) + +describe('the toggle round-trip', () => { + it('closes a visible hide-style pane through its own store', () => { + $layoutTree.set( + split('row', [ + group(['workspace'], { active: 'workspace', id: 'g-main' }), + group(['files'], { active: 'files', id: 'g-files' }) + ]) + ) + + const $files = atom(true) + bindVisibility('files', $files) + + expect(isPaneVisible('files')).toBe(true) + + togglePaneVisible('files') + + // Close routes through the registered closer, so the pane's own toggle + // store stays truthful rather than being bypassed by a tree edit. + expect($files.get()).toBe(false) + expect(isPaneVisible('files')).toBe(false) + + togglePaneVisible('files') + + expect($files.get()).toBe(true) + expect(isPaneVisible('files')).toBe(true) + }) +}) diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index 9c9be382ba..bf5be6e17b 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -1340,9 +1340,10 @@ export function restoreTreePane(paneId: string) { revealTreePane(paneId) } -/** Is a tool pane actually ON SCREEN? In the tree, not dismissed or chrome - * hidden, its zone un-minimized, and holding its stack's active slot. */ -export function isToolPaneVisible(paneId: string): boolean { +/** Is a pane actually ON SCREEN? In the tree, not dismissed, not chrome + * hidden, its zone un-minimized, and holding its stack's active slot. + * True for every pane class — tool panels and hide-style panes alike. */ +export function isPaneVisible(paneId: string): boolean { if ($dismissedPanes.get().has(paneId) || $hiddenTreePanes.get().has(paneId)) { return false } @@ -1352,22 +1353,54 @@ export function isToolPaneVisible(paneId: string): boolean { return Boolean(group && !group.minimized && group.active === paneId) } -const toolPaneVisibleCache = new Map>() +const paneVisibleCache = new Map>() -/** Reactive `isToolPaneVisible` for chrome that renders an on/off affordance +/** Reactive `isPaneVisible` for chrome that renders an on/off affordance * (the statusbar's terminal button). Memoized per pane id so `useStore` * subscriptions stay referentially stable across renders. */ -export function $toolPaneVisible(paneId: string): ReadableAtom { - let cached = toolPaneVisibleCache.get(paneId) +export function $paneVisible(paneId: string): ReadableAtom { + let cached = paneVisibleCache.get(paneId) if (!cached) { - cached = computed([$layoutTree, $dismissedPanes, $hiddenTreePanes], () => isToolPaneVisible(paneId)) - toolPaneVisibleCache.set(paneId, cached) + cached = computed([$layoutTree, $dismissedPanes, $hiddenTreePanes], () => isPaneVisible(paneId)) + paneVisibleCache.set(paneId, cached) } return cached } +/** + * HIDE-STYLE PANES (files, review, preview): bind a pane's visibility STORE to + * the tree so its toggle HIDES the pane — its zone collapses while the content + * stays mounted — as opposed to the tool panels, which collapse to a rail and + * keep their tab. + * + * `close` and `open` are a PAIR, and passing only one is the bug this exists to + * prevent. The closer keeps the toggle truthful when the pane is closed from + * the tab menu; the opener is its mirror, so anything that shows the pane + * through the tree — a reveal, a preset, the toggle's own un-hide path — writes + * the store too. With a closer and no opener the boolean goes stale the moment + * something other than the toggle reveals the pane, and the next press spends + * itself re-asserting a value it already held. + */ +export function bindPaneVisibility( + paneId: string, + $open: { get(): boolean; listen(fn: (open: boolean) => void): void }, + close?: () => void, + open?: () => void +) { + setTreePaneHidden(paneId, !$open.get()) + $open.listen(isOpen => setTreePaneHidden(paneId, !isOpen)) + + if (close) { + registerPaneCloser(paneId, close) + } + + if (open) { + registerPaneOpener(paneId, open) + } +} + /** * TOOL PANELS (terminal, logs): bind a pane's visibility STORE to the tree so * its toggle COLLAPSES the zone to a persistent rail (the tab stays) instead of @@ -1407,8 +1440,8 @@ export function bindToolPaneCollapse( } /** - * ⌃` / the statusbar button / ⌘J's terminal fallback: ONE resolver for "flip - * this tool panel", derived from what is on screen rather than from the + * EVERY pane toggle: ⌃`, ⌘G, the statusbar button, the ⌘K rows. ONE resolver + * for "flip this pane", derived from what is on screen rather than from the * toggle's own boolean. * * A free-floating `!$open.get()` diverges from the tree the moment anything @@ -1416,10 +1449,21 @@ export function bindToolPaneCollapse( * menu, closed with ⌘W — and then the toggle spends its press re-asserting a * value the store already held, which reads as a dead key. Asking the tree * instead means the first press always does the visible thing. + * + * This is not a tool-panel quirk. The hide-style panes (files, review) had it + * too: `setTreePaneHidden(id, false)` deliberately does NOT front or + * un-minimize, because reactive unhides (a cwd arriving) must not clobber what + * the user is looking at. Correct for a reactive change, useless for a + * keypress — so user intent routes here and reactive bindings keep the quiet + * path. + * + * Close goes through `closeTreePane` so each pane keeps its own semantics: a + * tool panel collapses to its rail, files/review close through their store, + * anything else is dismissed. */ -export function toggleToolPane(paneId: string) { - if (isToolPaneVisible(paneId)) { - collapseTreePane(paneId) +export function togglePaneVisible(paneId: string) { + if (isPaneVisible(paneId)) { + closeTreePane(paneId) } else { restoreTreePane(paneId) } diff --git a/apps/desktop/src/components/pane-shell/tree/tool-pane-toggle.test.ts b/apps/desktop/src/components/pane-shell/tree/tool-pane-toggle.test.ts index 8031fd6bab..6e24300d6d 100644 --- a/apps/desktop/src/components/pane-shell/tree/tool-pane-toggle.test.ts +++ b/apps/desktop/src/components/pane-shell/tree/tool-pane-toggle.test.ts @@ -10,9 +10,9 @@ import { $layoutTree, bindToolPaneCollapse, closeToolPane, - isToolPaneVisible, + isPaneVisible, setTreeGroupHeaderHidden, - toggleToolPane + togglePaneVisible } from './store' // Ground truth for "toggle terminal broke — ⌘J/⌘B work fine, but once I move @@ -114,7 +114,7 @@ describe('binding a tool panel on boot', () => { $terminal.set(false) expect(toolZone()?.minimized).toBe(true) - expect(isToolPaneVisible('terminal')).toBe(false) + expect(isPaneVisible('terminal')).toBe(false) }) it('still collapses a tool panel whose store says it is off', () => { @@ -132,11 +132,11 @@ describe('toggling the terminal while it is stacked with logs', () => { bindPaneCollapse('terminal', atom(true)) bindPaneCollapse('logs', atom(true)) - toggleToolPane('terminal') - expect(isToolPaneVisible('terminal')).toBe(false) + togglePaneVisible('terminal') + expect(isPaneVisible('terminal')).toBe(false) - toggleToolPane('terminal') - expect(isToolPaneVisible('terminal')).toBe(true) + togglePaneVisible('terminal') + expect(isPaneVisible('terminal')).toBe(true) }) it('brings the terminal forward when logs holds the active tab', () => { @@ -146,11 +146,11 @@ describe('toggling the terminal while it is stacked with logs', () => { // The zone is open but showing LOGS, so the terminal is not on screen — // the first press must reveal it rather than collapse the whole zone. - expect(isToolPaneVisible('terminal')).toBe(false) + expect(isPaneVisible('terminal')).toBe(false) - toggleToolPane('terminal') + togglePaneVisible('terminal') - expect(isToolPaneVisible('terminal')).toBe(true) + expect(isPaneVisible('terminal')).toBe(true) expect(toolZone()?.minimized).toBeFalsy() }) @@ -163,10 +163,10 @@ describe('toggling the terminal while it is stacked with logs', () => { closeToolPane('terminal') expect(allPaneIds($layoutTree.get()!)).not.toContain('terminal') - toggleToolPane('terminal') + togglePaneVisible('terminal') expect(allPaneIds($layoutTree.get()!)).toContain('terminal') - expect(isToolPaneVisible('terminal')).toBe(true) + expect(isPaneVisible('terminal')).toBe(true) }) }) diff --git a/apps/desktop/src/store/layout.ts b/apps/desktop/src/store/layout.ts index ba9a078920..c2debd8a72 100644 --- a/apps/desktop/src/store/layout.ts +++ b/apps/desktop/src/store/layout.ts @@ -2,6 +2,7 @@ import { atom, computed, type ReadableAtom, type WritableAtom } from 'nanostores import { SIDEBAR_COLLAPSE_MEDIA_QUERY } from '@/app/layout-constants' import { PANE_TOGGLE_REVEAL_EVENT } from '@/components/pane-shell' +import { isPaneVisible, revealTreePane } from '@/components/pane-shell/tree/store' import { matchesQuery } from '@/hooks/use-media-query' import { Codecs, persistentAtom } from '@/lib/persisted' import { arraysEqual, insertUniqueId, readKey } from '@/lib/storage' @@ -37,6 +38,9 @@ const RIGHT_RAIL_ACTIVE_TAB_STORAGE_KEY = 'hermes.desktop.rightRailActiveTab' export const CHAT_SIDEBAR_PANE_ID = 'chat-sidebar' export const FILE_BROWSER_PANE_ID = 'file-browser' +/** The file tree's id in the LAYOUT TREE — distinct from the pane-state id + * above, which keys its open/width record. Toggles need both. */ +export const FILES_PANE_ID = 'files' export const PREVIEW_PANE_ID = 'preview' /** Every rail tab is a preview of something, namespaced by what backs it: a @@ -287,9 +291,23 @@ export function toggleSidebarOpen() { } export function toggleFileBrowserOpen() { - if (!revealNarrowPane(FILE_BROWSER_PANE_ID, 'toggle')) { - togglePane(FILE_BROWSER_PANE_ID) + if (revealNarrowPane(FILE_BROWSER_PANE_ID, 'toggle')) { + return } + + // Ask the TREE, not the pane's boolean. `$fileBrowserOpen` stays true while + // the tree pane sits behind a sibling tab in the shared right column (the + // preview rail, the diff) or inside a minimized zone, so ⌘J spent its press + // re-asserting a value it already held and read as a dead key. Only fold the + // side when the tree is genuinely the thing on screen; otherwise bring it + // forward through the reveal path, which fronts and un-minimizes. + if (!isPaneVisible(FILES_PANE_ID) && $fileBrowserOpen.get()) { + revealTreePane(FILES_PANE_ID) + + return + } + + togglePane(FILE_BROWSER_PANE_ID) } export function setFileBrowserOpen(open: boolean) { diff --git a/apps/desktop/src/store/review.ts b/apps/desktop/src/store/review.ts index 91474682f8..c25c044b17 100644 --- a/apps/desktop/src/store/review.ts +++ b/apps/desktop/src/store/review.ts @@ -2,7 +2,7 @@ import { atom, computed } from 'nanostores' import { SIDEBAR_COLLAPSE_MEDIA_QUERY } from '@/app/layout-constants' import { PANE_TOGGLE_REVEAL_EVENT } from '@/components/pane-shell' -import { revealTreePane } from '@/components/pane-shell/tree/store' +import { isPaneVisible, revealTreePane } from '@/components/pane-shell/tree/store' import type { HermesReviewFile, HermesReviewShipInfo } from '@/global' import { matchesQuery } from '@/hooks/use-media-query' import { desktopGit } from '@/lib/desktop-git' @@ -285,7 +285,12 @@ export function toggleReview(scopeCwd: null | string = null): void { return } - if ($reviewOpen.get()) { + // Ask the TREE, not `$reviewOpen`. The store stays true while the pane sits + // behind a sibling tab in the right column or inside a minimized zone, so a + // boolean flip spent the press re-asserting a value it already held and ⌘G + // read as a dead key. `revealReview` fronts and un-minimizes; only close when + // the diff is genuinely the thing on screen. + if (isPaneVisible(REVIEW_PANE_ID)) { closeReview() } else { revealReview(scopeCwd)