diff --git a/apps/desktop/src/app/contrib/controller.tsx b/apps/desktop/src/app/contrib/controller.tsx index 3c4d1d3e98..da4ff828cb 100644 --- a/apps/desktop/src/app/contrib/controller.tsx +++ b/apps/desktop/src/app/contrib/controller.tsx @@ -13,10 +13,13 @@ 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, + isPaneVisible, markCollapsePane, mirrorLayoutTree, paneRootSide, @@ -26,8 +29,7 @@ import { removeTreePane, resetLayoutTree, revealTreePane, - setPaneCollapsed, - setTreePaneHidden, + togglePaneVisible, watchContributedPanes } from '@/components/pane-shell/tree/store' import { SidebarProvider } from '@/components/ui/sidebar' @@ -54,7 +56,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' @@ -175,7 +177,11 @@ registry.registerMany([ // staying collapsed behind the ⌃` toggle. height sizes the fixed track (a // single-pane zone declaring a height is a fixed track — the preset weight // is moot): a short deck, not a third of the window. - data: { placement: 'bottom', height: '20vh', minHeight: '7.5rem', maxHeight: '80vh', revealOnPreset: true }, + // + // NO minHeight: a tool panel drags all the way down to its collapsed + // header (the sash floors it at COLLAPSED_ZONE_PX and folds the zone to + // its rail there). A real floor left a sliver of unusable terminal. + data: { placement: 'bottom', height: '20vh', maxHeight: '80vh', revealOnPreset: true }, render: () => }, { @@ -455,49 +461,13 @@ 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)) +// 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. - // 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) - } -} - -// TOOL PANELS (terminal, logs): like bindPaneVisibility but the toggle COLLAPSES -// the zone to a persistent rail (tab stays) instead of hiding it — the -// IntelliJ/VS-Code tool-window model. Restore routes back through `open` (rail -// click / chevron) so ⌃`/the button stay truthful; Close removes the tab. -// -// OPEN goes through revealTreePane, not setPaneCollapsed: Close DISMISSES the -// pane, and setPaneCollapsed can't act on a pane that has left the tree — the -// toggle would flip its store with nothing coming back. revealTreePane -// un-dismisses and re-adopts. -function bindPaneCollapse( - paneId: string, - $open: { get(): boolean; listen(fn: (open: boolean) => void): void }, - close: () => void, - open: () => void -) { - markCollapsePane(paneId) - setPaneCollapsed(paneId, !$open.get()) - $open.listen(isOpen => (isOpen ? revealTreePane(paneId) : setPaneCollapsed(paneId, true))) - registerPaneCloser(paneId, close) - registerPaneOpener(paneId, open) -} +// TOOL PANELS (terminal, logs): the binding lives in the tree store — +// bindToolPaneCollapse — so the boot rule it encodes is testable against the +// real function instead of a copy. See its docblock for the semantics. // SIDES have one source of truth: the TREE. The legacy $panesFlipped flag is // DERIVED from where the sessions zone actually sits (TitlebarControls maps @@ -550,26 +520,39 @@ 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). -bindPaneCollapse( +bindToolPaneCollapse( 'terminal', $terminalTakeover, () => setTerminalTakeover(false), () => setTerminalTakeover(true) ) -// ⌘K door onto the same store the keybind and statusbar pill flip — was a +// ⌘K door onto the same pane the keybind and statusbar pill flip — was a // one-way "open" row under Go to, so it never showed on/off and couldn't hide. +// Reads the TREE like every other pane toggle: `$terminalTakeover` stays true +// behind a stacked sibling tab or a minimized zone, which would light the row +// "on" for a terminal that isn't on screen. registry.register( paletteToggle({ id: 'view.showTerminal', @@ -577,8 +560,8 @@ registry.register( action: 'view.showTerminal', icon: Terminal, keywords: ['terminal', 'shell', 'console', 'pty'], - get: () => $terminalTakeover.get(), - set: setTerminalTakeover + get: () => isPaneVisible('terminal'), + set: () => togglePaneVisible('terminal') }) ) @@ -606,13 +589,14 @@ const syncLogsPane = (open: boolean) => { id: 'logs', area: 'panes', title: 'logs', - // Same tool-panel sizing rule as the terminal above. dock: its OWN zone - // beside the terminal — never a tab in the terminal's strip. + // Same tool-panel sizing rule as the terminal above — no minHeight, so + // the sash floors it at COLLAPSED_ZONE_PX and folds the zone to its rail + // rather than leaving a sliver. dock: its OWN zone beside the terminal — + // never a tab in the terminal's strip. data: { placement: 'bottom', dock: { pane: 'terminal', pos: 'right' }, height: '20vh', - minHeight: '7.5rem', maxHeight: '80vh' }, render: () => idle() @@ -650,8 +634,13 @@ registry.register( label: 'Toggle logs', icon: FileText, keywords: ['logs', 'agent log', 'tail', 'debug'], - get: () => $logsOpen.get(), - set: enabled => $logsOpen.set(enabled) + // On-screen, not the store's boolean. Summon-only keeps the two in step + // while logs sits in its own zone, but the user can still drag it into the + // terminal's strip or minimize its zone — and then `$logsOpen` reads true + // with nothing visible, so the row would show "on" and its press would + // spend itself re-asserting a value it already held. + 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 dcd97cf29c..32d9caaba2 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -2,9 +2,15 @@ import { useEffect, useRef } from 'react' import { useNavigate } from 'react-router' import { closeActiveTab } from '@/app/chat/close-tab' -import { $terminalTakeover, setTerminalTakeover } from '@/app/right-sidebar/store' +import { setTerminalTakeover } from '@/app/right-sidebar/store' import { closeActiveTerminal, createTerminal, cycleTerminal } from '@/app/right-sidebar/terminal/terminals' -import { activateTreeTabSlot, cycleTreeTabInFocusedZone, layoutHasRootSide } from '@/components/pane-shell/tree/store' +import { + activateTreeTabSlot, + cycleTreeTabInFocusedZone, + isPaneVisible, + layoutHasRootSide, + togglePaneVisible +} from '@/components/pane-shell/tree/store' import { onReleaseTypingFocus } from '@/components/ui/keyboard-first' import { findBarClaimsCombo } from '@/lib/find-in-page' import { contributedKeybindHandler, PROFILE_SLOT_COUNT, SESSION_SLOT_COUNT } from '@/lib/keybinds/actions' @@ -184,22 +190,23 @@ 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() : setTerminalTakeover(!$terminalTakeover.get()), + layoutHasRootSide('right') ? toggleFileBrowserOpen() : togglePaneVisible('terminal'), 'view.toggleReview': toggleReview, 'view.toggleStatusbar': toggleStatusbarVisible, 'view.showFiles': showFiles, - 'view.showTerminal': () => setTerminalTakeover(!$terminalTakeover.get()), + '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': () => { createTerminal() setTerminalTakeover(true) }, - // Switch / close only act while the pane is open (no focus-scoping here, so - // this stands in for "terminal is showing"). - 'view.nextTerminal': () => $terminalTakeover.get() && cycleTerminal(1), - 'view.prevTerminal': () => $terminalTakeover.get() && cycleTerminal(-1), - 'view.closeTerminal': () => $terminalTakeover.get() && closeActiveTerminal(), + // 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': () => 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 0aa3c69fe3..adedab3fa5 100644 --- a/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx +++ b/apps/desktop/src/app/shell/hooks/use-statusbar-items.tsx @@ -2,10 +2,10 @@ import { useStore } from '@nanostores/react' import { useCallback, useMemo } from 'react' import type { CommandCenterSection } from '@/app/command-center' -import { $terminalTakeover, setTerminalTakeover } from '@/app/right-sidebar/store' 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 { $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' @@ -92,7 +92,10 @@ export function useStatusbarItems({ const fileMenu = t.fileMenu const primaryActiveSessionId = useStore($activeSessionId) const activeGatewayProfile = useStore($activeGatewayProfile) - const terminalTakeover = useStore($terminalTakeover) + // 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($paneVisible('terminal')) const primaryBusy = useStore($busy) const currentCwd = useStore($currentCwd) // Derive the workspace's project name from the already-cached project tree @@ -506,12 +509,12 @@ export function useStatusbarItems({ }, { actionId: 'view.showTerminal', - className: `w-7 justify-center px-0${terminalTakeover ? ' bg-accent/55 text-foreground' : ''}`, + className: `w-7 justify-center px-0${terminalShowing ? ' bg-accent/55 text-foreground' : ''}`, hidden: !chatOpen, icon: , id: 'terminal', - onSelect: () => setTerminalTakeover(!$terminalTakeover.get()), - title: terminalTakeover ? copy.hideTerminal : copy.showTerminal, + onSelect: () => togglePaneVisible('terminal'), + title: terminalShowing ? copy.hideTerminal : copy.showTerminal, toggleLabel: copy.toggleTerminal, variant: 'action' }, @@ -533,7 +536,7 @@ export function useStatusbarItems({ requestGateway, sessionStartedAt, gatewayState, - terminalTakeover, + terminalShowing, turnStartedAt ] ) diff --git a/apps/desktop/src/components/pane-shell/tree/model.ts b/apps/desktop/src/components/pane-shell/tree/model.ts index 0834360b0a..7f61143bab 100644 --- a/apps/desktop/src/components/pane-shell/tree/model.ts +++ b/apps/desktop/src/components/pane-shell/tree/model.ts @@ -134,17 +134,18 @@ export function normalize(node: LayoutNode): LayoutNode | null { } const active = node.panes.includes(node.active) ? node.active : node.panes[0] - // A zone down to one pane clears a redundant HIDDEN override (the lone-pane - // default is already headerless) but KEEPS an explicit SHOWN override — - // once a zone has ever had a tab bar, closing back to one tab leaves it - // shown (sticky bar; the off switch is "Hide tab bar"). `false` survives. - const headerHidden = node.panes.length <= 1 && node.headerHidden !== false ? undefined : node.headerHidden - if (active === node.active && headerHidden === node.headerHidden) { + // NOTE: `headerHidden` is deliberately untouched here. A zone down to one + // pane is headerless by default anyway, so a stored `true` is visually + // redundant *while it's alone* — but normalize used to DROP it, which threw + // away the user's standing choice: the bar came back the moment a pane + // rejoined (close a stacked tool panel, toggle it back on). `false` is + // sticky for the mirror reason — once a zone has had a tab bar, it keeps it. + if (active === node.active) { return node } - return { ...node, active, headerHidden } + return { ...node, active } } const children: LayoutNode[] = [] 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/renderer/track-model.ts b/apps/desktop/src/components/pane-shell/tree/renderer/track-model.ts index 7dd10012aa..67dbdac932 100644 --- a/apps/desktop/src/components/pane-shell/tree/renderer/track-model.ts +++ b/apps/desktop/src/components/pane-shell/tree/renderer/track-model.ts @@ -19,6 +19,15 @@ import type { FloatingAnchor } from './floating-rect' export const MIN_PANE_PX = 80 +/** + * The floor for a TOOL PANEL zone (terminal / logs) instead of `MIN_PANE_PX`. + * A tool panel is meant to be draggable down to nothing — the minimized rail + * (its header strip, `h-7`) is the smallest meaningful form, so the sash lets + * it shrink to exactly that and then collapses the zone rather than jamming + * against an 80px floor with a sliver of unusable content still showing. + */ +export const COLLAPSED_ZONE_PX = 28 + /** Optional CSS sizing a pane contributes (`data.width` / `data.minWidth`…). * Applied to the pane's GROUP along the axis of the split that contains it — * the same semantics as the app's `Pane width/minWidth/maxWidth` props: diff --git a/apps/desktop/src/components/pane-shell/tree/renderer/tree-split.tsx b/apps/desktop/src/components/pane-shell/tree/renderer/tree-split.tsx index 378341552b..07a4e18a84 100644 --- a/apps/desktop/src/components/pane-shell/tree/renderer/tree-split.tsx +++ b/apps/desktop/src/components/pane-shell/tree/renderer/tree-split.tsx @@ -22,12 +22,15 @@ import { $collapsedTreeSides, $hiddenTreePanes, $narrowViewport, + isCollapsePane, persistTree, presetSplitWeights, + setTreeGroupMinimized, setTreeSplitWeights } from '../store' import { + COLLAPSED_ZONE_PX, computedPx, cssMax, edgeFixedZone, @@ -43,6 +46,18 @@ import { } from './track-model' import { TreeNode } from './tree-node' +/** The single group id a subtree resolves to, or null when it holds several + * zones — the sash can only collapse a boundary that IS exactly one zone. */ +function groupIdOf(node: LayoutNode): null | string { + if (node.type === 'group') { + return node.id + } + + const ids = new Set(node.children.map(groupIdOf)) + + return ids.size === 1 ? [...ids][0] : null +} + /** * The size overrides for a fixed set of panes, referentially stable until one * of THEM changes. Sash drags churn `$paneStates` every frame; subscribing the @@ -197,6 +212,11 @@ export function TreeSplit({ node, root, rootRow }: { node: SplitNode; root?: boo // Clamps live on the zone's split-child WRAPPER (where we render them). const el = zoneEl?.parentElement ?? wrapper const cs = window.getComputedStyle(el) + // A tool panel (terminal / logs) may be dragged down to its collapsed + // header — the generic 80px floor is not its floor. Below that the + // release minimizes the zone instead of leaving a useless sliver. + const toolZone = allPaneIds(child).length > 0 && allPaneIds(child).every(isCollapsePane) + const floor = toolZone ? COLLAPSED_ZONE_PX : MIN_PANE_PX return { // EVERY shown pane of the zone: the zone's track is the max() of its @@ -206,8 +226,10 @@ export function TreeSplit({ node, root, rootRow }: { node: SplitNode; root?: boo paneIds: zone ? shownPaneIds(zone, trackCtx) : [], fixed: Boolean(zone), size: sizeOf(zoneEl ?? wrapper), - min: Math.max(MIN_PANE_PX, computedPx(horizontal ? cs.minWidth : cs.minHeight, 0)), - max: computedPx(horizontal ? cs.maxWidth : cs.maxHeight, Number.POSITIVE_INFINITY) + min: toolZone ? floor : Math.max(floor, computedPx(horizontal ? cs.minWidth : cs.minHeight, 0)), + max: computedPx(horizontal ? cs.maxWidth : cs.maxHeight, Number.POSITIVE_INFINITY), + collapseId: toolZone ? (zone?.id ?? groupIdOf(child)) : null, + floor } } @@ -310,9 +332,21 @@ export function TreeSplit({ node, root, rootRow }: { node: SplitNode; root?: boo resize.finish() if (lastShift !== null) { - // One store commit; the re-render rewrites `flex` and clears the - // preview overrides. - applyShift(lastShift) + // Dragged a tool panel down to its collapsed header? Fold the zone + // to its rail instead of persisting a sliver — and DON'T write the + // sliver size, so restoring brings back the size it had before. + const collapsedSide = + (a.collapseId && a0px + lastShift <= a.floor && a.collapseId) || + (b.collapseId && b0px - lastShift <= b.floor && b.collapseId) || + null + + if (collapsedSide) { + setTreeGroupMinimized(collapsedSide, true) + } else { + // One store commit; the re-render rewrites `flex` and clears the + // preview overrides. + applyShift(lastShift) + } } else { // Click without movement: nothing will re-render, so put the // wrappers' inline styles back exactly as React last wrote them. diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index a1abe54478..42267459b9 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -4,7 +4,7 @@ * the persisted tree is the user's customization; reset returns to default. */ -import { atom, computed } from 'nanostores' +import { atom, computed, type ReadableAtom } from 'nanostores' import { SIDEBAR_COLLAPSE_MEDIA_QUERY } from '@/app/layout-constants' import { setPluginEnabled } from '@/contrib/plugins-store' @@ -1050,16 +1050,27 @@ function adoptContributedPanes(): void { const target = findGroupOfPane(next, anchor ?? '')?.id if (target) { + // Whether the DESTINATION zone's header was explicitly hidden, read + // BEFORE the insert — `insertAtGroup` pins `headerHidden: false` on a + // center drop (a stack you can't see is a trap), which is right for a + // drag but wrong for adoption into a zone whose bar the user hid. + const hostHeaderHidden = findGroup(next, target)?.headerHidden === true + // Silent adoption: don't front over the zone's active tab — a reveal does. next = insertAtGroup(next, target, pane.id, dock?.pos ?? 'center', dock?.before, false) ?? next // An adopted pane ARRIVES with its chip showing — a surprise zone with // zero chrome has no obvious handle to drag or close. (Explicit reveal; // the next structural op returns lone panes to the auto-hide default.) + // + // EXCEPT into a zone whose header the user explicitly hid: that's a + // standing preference about the zone, not a stale default. Without this + // the bar came back every time a tool panel was closed and toggled on + // again — Close dismisses the pane, the toggle re-adopts it through here. const landed = findGroupOfPane(next, pane.id) if (landed) { - next = setGroupHeaderHiddenOp(next, landed.id, false) + next = setGroupHeaderHiddenOp(next, landed.id, hostHeaderHidden) } } } @@ -1314,34 +1325,146 @@ export function setPaneCollapsed(paneId: string, collapsed: boolean) { } /** Restore a minimized tool pane the truthful way — through its store opener - * when bound (keeps ⌃`/titlebar toggles in sync), else just un-minimize + - * front. Used by the rail (tab / whole-rail click) and the header chevron. */ + * when bound (keeps ⌃`/titlebar toggles in sync), then reveal regardless. + * Used by the rail (tab / whole-rail click), the header chevron, and ⌃`. + * + * The opener is fire-and-forget because it may be a NO-OP: the store can + * already be `true` while the pane is off screen (the zone was minimized from + * the zone menu, the tab was closed with ⌘W, or a stacked sibling holds the + * active slot). nanostores don't fire listeners on a same-value `.set()`, so + * the bindPaneCollapse listener never runs. `revealTreePane` is idempotent and + * does the real work — un-dismiss, un-collapse the side, un-minimize, front. */ export function restoreTreePane(paneId: string) { - const open = paneOpeners[paneId] + paneOpeners[paneId]?.() + revealTreePane(paneId) +} - if (open) { - open() - - // The opener may be a no-op — the store was already true (zone minimized - // via the zone menu, not the toggle). nanostores don't fire listeners on - // a same-value .set(), so the bindPaneCollapse listener never runs and - // the zone stays minimized. Un-minimize directly when that happens. - const group = paneGroup(paneId) - - if (group?.minimized) { - setTreeGroupMinimized(group.id, false) - } - - revealTreePane(paneId) - - return +/** 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 } const group = paneGroup(paneId) - if (group) { - setTreeGroupMinimized(group.id, false) - activateTreePane(group.id, paneId) + return Boolean(group && !group.minimized && group.active === paneId) +} + +const paneVisibleCache = new Map>() + +/** 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 $paneVisible(paneId: string): ReadableAtom { + let cached = paneVisibleCache.get(paneId) + + if (!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 + * hiding it — the IntelliJ/VS-Code tool-window model. Restore routes back + * through `open` (rail click / chevron) so ⌃` and the statusbar button stay + * truthful; Close removes the tab. + * + * OPEN goes through `revealTreePane`, not `setPaneCollapsed`: Close DISMISSES + * the pane, and `setPaneCollapsed` is a no-op on a pane that has left the tree, + * so the toggle would flip its store with nothing coming back. `revealTreePane` + * un-dismisses and re-adopts. + * + * BOOT ONLY COLLAPSES — it must never reveal. `setPaneCollapsed(id, false)` + * fronts the pane in its stack, so binding two tool panels that are both "open" + * let the second one steal the active tab from the persisted tree. With + * terminal+logs stacked (what you get by dragging the terminal to the bottom), + * logs bound last and won the slot; ⌃` then asked to collapse a terminal that + * wasn't the active tab, the shared-zone branch declined, and the key read as + * dead until the stack was broken up. The persisted tree already records which + * tab was active — leave it alone. + */ +export function bindToolPaneCollapse( + paneId: string, + $open: { get(): boolean; listen(fn: (open: boolean) => void): void }, + close: () => void, + open: () => void +) { + markCollapsePane(paneId) + + if (!$open.get()) { + setPaneCollapsed(paneId, true) + } + + $open.listen(isOpen => (isOpen ? revealTreePane(paneId) : setPaneCollapsed(paneId, true))) + registerPaneCloser(paneId, close) + registerPaneOpener(paneId, open) +} + +/** + * 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 + * else moves the pane — stacked behind a sibling tab, minimized from the zone + * 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 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 new file mode 100644 index 0000000000..6e24300d6d --- /dev/null +++ b/apps/desktop/src/components/pane-shell/tree/tool-pane-toggle.test.ts @@ -0,0 +1,210 @@ +import { atom } from 'nanostores' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { registry } from '@/contrib/registry' + +import { allPaneIds, group, split } from './model' +import { + $dismissedPanes, + $hiddenTreePanes, + $layoutTree, + bindToolPaneCollapse, + closeToolPane, + isPaneVisible, + setTreeGroupHeaderHidden, + togglePaneVisible +} from './store' + +// Ground truth for "toggle terminal broke — ⌘J/⌘B work fine, but once I move +// the terminal around it just doesn't open/close anymore", and for "I have the +// header hidden and it keeps coming back". +// +// Both symptoms need the terminal STACKED with logs, which is the layout you +// get by dragging the terminal to the bottom: adoption stacks logs into the +// terminal's zone. A lone terminal never reproduced either bug, which is why +// they read as "randomly broken". + +const disposers: (() => void)[] = [] + +/** Bind through the REAL production function, with the ergonomics the + * controller gives it (closer/opener writing the same store). */ +function bindPaneCollapse(paneId: string, $open: ReturnType>) { + bindToolPaneCollapse( + 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 }], + ['terminal', { placement: 'bottom' }], + ['logs', { placement: 'bottom' }] + ] as const) { + disposers.push(registry.register({ area: 'panes', data, id, render: () => null, title: id })) + } +}) + +afterEach(() => { + disposers.splice(0).forEach(dispose => dispose()) +}) + +/** The bottom tool zone, as the tree currently holds it. */ +const toolZone = () => { + const tree = $layoutTree.get()! + + const found = + tree.type === 'split' + ? tree.children.find(c => c.type === 'group' && (c.panes.includes('terminal') || c.panes.includes('logs'))) + : null + + return found as { active?: string; headerHidden?: boolean; minimized?: boolean; panes: string[] } | null +} + +/** Terminal dragged to the bottom; logs adopted into the same zone. + * + * Set via `$layoutTree.set`, NOT `declareDefaultTree` — that only adopts into + * an existing tree, and `$layoutTree` is module state that survives between + * tests, so the second case would silently assert against the first's shape. */ +const stackTree = (options?: { active?: string; headerHidden?: boolean }) => { + $layoutTree.set( + split('column', [ + group(['workspace'], { active: 'workspace', id: 'grp-main' }), + group(['terminal', 'logs'], { + active: options?.active ?? 'terminal', + headerHidden: options?.headerHidden, + id: 'g-tools' + }) + ]) + ) +} + +describe('binding a tool panel on boot', () => { + it('does not front itself over the tab the persisted tree had active', () => { + stackTree({ active: 'terminal' }) + + const $terminal = atom(true) + const $logs = atom(true) + bindPaneCollapse('terminal', $terminal) + bindPaneCollapse('logs', $logs) + + // Binding logs LAST must not steal the active slot. It used to, because + // boot called setPaneCollapsed(id, false), whose shared-zone branch + // reveals. + expect(toolZone()?.active).toBe('terminal') + }) + + it('leaves ⌃` able to collapse the zone straight after boot', () => { + stackTree({ active: 'terminal' }) + + const $terminal = atom(true) + bindPaneCollapse('terminal', $terminal) + bindPaneCollapse('logs', atom(true)) + + // THE reported symptom, driven the way the keybind drives it. When boot + // fronted logs, this asked setPaneCollapsed to fold a terminal that wasn't + // the active tab; the shared-zone branch declines that, so the first press + // did nothing at all and ⌃` read as a dead key. + $terminal.set(false) + + expect(toolZone()?.minimized).toBe(true) + expect(isPaneVisible('terminal')).toBe(false) + }) + + it('still collapses a tool panel whose store says it is off', () => { + stackTree() + + bindPaneCollapse('terminal', atom(false)) + + expect(toolZone()?.minimized).toBe(true) + }) +}) + +describe('toggling the terminal while it is stacked with logs', () => { + it('closes and reopens on every press', () => { + stackTree({ active: 'terminal' }) + bindPaneCollapse('terminal', atom(true)) + bindPaneCollapse('logs', atom(true)) + + togglePaneVisible('terminal') + expect(isPaneVisible('terminal')).toBe(false) + + togglePaneVisible('terminal') + expect(isPaneVisible('terminal')).toBe(true) + }) + + it('brings the terminal forward when logs holds the active tab', () => { + stackTree({ active: 'logs' }) + bindPaneCollapse('terminal', atom(true)) + bindPaneCollapse('logs', atom(true)) + + // 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(isPaneVisible('terminal')).toBe(false) + + togglePaneVisible('terminal') + + expect(isPaneVisible('terminal')).toBe(true) + expect(toolZone()?.minimized).toBeFalsy() + }) + + it('reopens after the terminal tab was closed outright', () => { + stackTree() + const $terminal = atom(true) + bindPaneCollapse('terminal', $terminal) + bindPaneCollapse('logs', atom(true)) + + closeToolPane('terminal') + expect(allPaneIds($layoutTree.get()!)).not.toContain('terminal') + + togglePaneVisible('terminal') + + expect(allPaneIds($layoutTree.get()!)).toContain('terminal') + expect(isPaneVisible('terminal')).toBe(true) + }) +}) + +describe('a zone whose header the user hid', () => { + it('keeps it hidden after a stacked sibling is closed and toggled back', () => { + stackTree({ headerHidden: true }) + bindPaneCollapse('terminal', atom(true)) + const $logs = atom(true) + bindPaneCollapse('logs', $logs) + + setTreeGroupHeaderHidden('g-tools', true) + + // Close logs: the zone drops to one pane. normalize used to DISCARD the + // hidden flag here ("a lone zone is headerless anyway"), so the bar + // reappeared the moment logs was toggled back in. + closeToolPane('logs') + expect(toolZone()?.headerHidden).toBe(true) + + $logs.set(true) + + expect(toolZone()?.panes).toContain('logs') + expect(toolZone()?.headerHidden).toBe(true) + }) + + it('keeps it hidden when a closed pane is re-adopted into it', () => { + stackTree() + const $terminal = atom(true) + bindPaneCollapse('terminal', $terminal) + bindPaneCollapse('logs', atom(true)) + + setTreeGroupHeaderHidden('g-tools', true) + + // Re-adoption pins headerHidden:false so a surprise pane always has a + // handle — correct for a new pane, wrong for a zone the user hid. + closeToolPane('terminal') + $terminal.set(true) + + expect(toolZone()?.panes).toContain('terminal') + expect(toolZone()?.headerHidden).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)