From dd48d9a8164ba843a44afeef2f7db7720de6c96f Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Fri, 31 Jul 2026 12:54:41 -0500 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20tool=20panel=20tab=20=E2=9C=95?= =?UTF-8?q?=20can=20close=20and=20re-open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logs (and terminal) tab ✕ dismissed the pane from the layout but never synced the owning store — so the ⌘K toggle was stale and its open listener called setPaneCollapsed, a no-op when the pane isn't in the tree. The tab was gone with no way back short of a layout reset. Route the tab ✕ through closeCollapsePane (dismiss + store sync) so the toggle stays truthful, and make bindPaneCollapse's open listener call revealTreePane (un-dismiss + re-adopt) instead of setPaneCollapsed. --- apps/desktop/src/app/contrib/controller.tsx | 13 +- .../tree/collapse-pane-close.test.ts | 117 ++++++++++++++++++ .../pane-shell/tree/renderer/tree-group.tsx | 13 +- .../src/components/pane-shell/tree/store.ts | 26 +++- 4 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 apps/desktop/src/components/pane-shell/tree/collapse-pane-close.test.ts diff --git a/apps/desktop/src/app/contrib/controller.tsx b/apps/desktop/src/app/contrib/controller.tsx index 78304432c83..d48c283b393 100644 --- a/apps/desktop/src/app/contrib/controller.tsx +++ b/apps/desktop/src/app/contrib/controller.tsx @@ -492,6 +492,11 @@ function bindPaneVisibility( // 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; the tab's ✕ removes it. +// +// OPEN uses revealTreePane (not setPaneCollapsed) so a pane dismissed via its +// tab ✕ (closeCollapsePane) is un-dismissed + re-adopted — setPaneCollapsed +// is a no-op when the pane isn't in the tree, so the toggle couldn't bring it +// back after ✕ closed it. function bindPaneCollapse( paneId: string, $open: { get(): boolean; listen(fn: (open: boolean) => void): void }, @@ -500,7 +505,13 @@ function bindPaneCollapse( ) { markCollapsePane(paneId) setPaneCollapsed(paneId, !$open.get()) - $open.listen(isOpen => setPaneCollapsed(paneId, !isOpen)) + $open.listen(isOpen => { + if (isOpen) { + revealTreePane(paneId) + } else { + setPaneCollapsed(paneId, true) + } + }) registerPaneCloser(paneId, close) registerPaneOpener(paneId, open) } diff --git a/apps/desktop/src/components/pane-shell/tree/collapse-pane-close.test.ts b/apps/desktop/src/components/pane-shell/tree/collapse-pane-close.test.ts new file mode 100644 index 00000000000..9aaf645e197 --- /dev/null +++ b/apps/desktop/src/components/pane-shell/tree/collapse-pane-close.test.ts @@ -0,0 +1,117 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { atom } from 'nanostores' + +import { allPaneIds } from './model' + +// Repro for "the logs tab ✕ kills the pane and the toggle can't bring it +// back": a tool panel (terminal/logs) bound via bindPaneCollapse had its tab ✕ +// routed through dismissTreePane, which removed the pane from the layout but +// never synced the owning store — so the ⌘K "Toggle logs" toggle was stale +// (nanostores don't fire on a same-value .set), and its open listener called +// setPaneCollapsed, which is a no-op when the pane isn't in the tree. The tab +// was gone with no way back short of a layout reset. +// +// The fix routes the tab ✕ through closeCollapsePane (dismiss + store sync), +// and bindPaneCollapse's open listener calls revealTreePane (un-dismiss + +// re-adopt) instead of setPaneCollapsed. + +describe('collapse pane tab close + toggle recovery', () => { + beforeEach(() => { + window.localStorage.clear() + vi.resetModules() + }) + + afterEach(() => { + vi.resetModules() + }) + + async function setup() { + const tree = await import('@/components/pane-shell/tree/store') + const model = await import('@/components/pane-shell/tree/model') + const { registry } = await import('@/contrib/registry') + + // A tool panel (logs): placement 'bottom', not uncloseable. + registry.register({ + id: 'logs', + area: 'panes', + title: 'logs', + data: { placement: 'bottom' }, + render: () => null + }) + // The main workspace, so adoption has an anchor. + registry.register({ + id: 'workspace', + area: 'panes', + title: 'workspace', + data: { placement: 'main', uncloseable: true }, + render: () => null + }) + + tree.declareDefaultTree( + model.split('column', [ + model.group(['workspace'], { active: 'workspace', id: 'grp-main' }), + model.group(['logs'], { active: 'logs', id: 'grp-logs' }) + ]) + ) + + // Mirror controller.tsx bindPaneCollapse: the owning store drives + // visibility, the closer syncs it on ✕, the opener reveals. + const $logsOpen = atom(true) + + // bindPaneCollapse inline (can't import the controller's private fn). + tree.markCollapsePane('logs') + tree.setPaneCollapsed('logs', !$logsOpen.get()) + $logsOpen.subscribe(isOpen => { + if (isOpen) { + tree.revealTreePane('logs') + } else { + tree.setPaneCollapsed('logs', true) + } + }) + tree.registerPaneCloser('logs', () => $logsOpen.set(false)) + tree.registerPaneOpener('logs', () => $logsOpen.set(true)) + + return { tree, $logsOpen } + } + + it('tab ✕ dismisses the pane AND syncs the owning store', async () => { + const { tree, $logsOpen } = await setup() + + expect($logsOpen.get()).toBe(true) + expect(allPaneIds(tree.$layoutTree.get()!)).toContain('logs') + + // The tab ✕ on a collapse pane. + tree.closeCollapsePane('logs') + + // Pane is gone from the layout… + expect(allPaneIds(tree.$layoutTree.get()!)).not.toContain('logs') + // …and the store was synced to false. + expect($logsOpen.get()).toBe(false) + }) + + it('the toggle brings the pane back after the tab ✕ closed it', async () => { + const { tree, $logsOpen } = await setup() + + // Close via tab ✕. + tree.closeCollapsePane('logs') + expect($logsOpen.get()).toBe(false) + expect(allPaneIds(tree.$layoutTree.get()!)).not.toContain('logs') + + // Re-open via the toggle (the ⌘K row / opener). + $logsOpen.set(true) + + // The pane is back in the layout. + expect(allPaneIds(tree.$layoutTree.get()!)).toContain('logs') + expect($logsOpen.get()).toBe(true) + }) + + it('closeTabPane routes collapse panes through closeCollapsePane', async () => { + const { tree, $logsOpen } = await setup() + + tree.closeTabPane('logs') + + expect(allPaneIds(tree.$layoutTree.get()!)).not.toContain('logs') + expect($logsOpen.get()).toBe(false) + }) +}) diff --git a/apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx b/apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx index e81bc1afd26..a47e0eda672 100644 --- a/apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx +++ b/apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx @@ -36,10 +36,10 @@ import { activateTreePane, closeAllTreeTabs, closeOtherTreeTabs, + closeTabPane, closeTreePane, closeTreeTabsToRight, collapseTreePane, - dismissTreePane, isCollapsePane, isSessionStripPane, noteActiveTreeGroup, @@ -101,7 +101,7 @@ function ZoneMenu({ renderActionItem(kit, { icon: 'close', label: t.common.close, - onSelect: () => closeTreePane(paneId) + onSelect: () => closeTabPane(paneId) })} {renderActionItem(kit, { disabled: !targets.others, @@ -287,10 +287,11 @@ export function TreeGroup({ // MAIN strands the whole app behind a strip. const minimizable = !shown.some(id => paneChrome(paneFor(id)).uncloseable) - // Tab ✕: a tool panel (terminal/logs) is REMOVED from the layout (comes back - // via its toggle); everything else routes through its Close (a session tile - // closes the session, a store-bound pane collapses). - const closeTab = (paneId: string) => (isCollapsePane(paneId) ? dismissTreePane(paneId) : closeTreePane(paneId)) + // Tab ✕: a tool panel (terminal/logs) is removed from the layout AND its + // owning store is synced (closeTabPane → closeCollapsePane), so the toggle + // stays truthful and can bring it back; everything else routes through its + // Close (a session tile closes the session, a store-bound pane collapses). + const closeTab = (paneId: string) => closeTabPane(paneId) // A pane whose store owns Close keeps the gesture even when the pane itself // is uncloseable — the workspace tab empties to a fresh draft rather than diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index 4bb24460ddf..0c25946d02d 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -420,12 +420,22 @@ export function treeTabCloseTargets(paneId: string): { all: number; others: numb return { all: others.length + (isUncloseablePane(paneId) ? 0 : 1), others: others.length, right: right.length } } +/** Close a pane, routing collapse panes (terminal/logs) through dismiss + + * store sync so their toggle stays truthful. */ +export function closeTabPane(paneId: string) { + if (isCollapsePane(paneId)) { + closeCollapsePane(paneId) + } else { + closeTreePane(paneId) + } +} + export function closeOtherTreeTabs(paneId: string): void { - closeableTreeSiblings(paneId).others.forEach(closeTreePane) + closeableTreeSiblings(paneId).others.forEach(closeTabPane) } export function closeTreeTabsToRight(paneId: string): void { - closeableTreeSiblings(paneId).right.forEach(closeTreePane) + closeableTreeSiblings(paneId).right.forEach(closeTabPane) } /** Close every closeable tab in `paneId`'s group (the uncloseable workspace stays). */ @@ -433,7 +443,7 @@ export function closeAllTreeTabs(paneId: string): void { const tree = $layoutTree.get() const panes = (tree ? findGroupOfPane(tree, paneId) : null)?.panes ?? [] - panes.filter(id => !isUncloseablePane(id)).forEach(closeTreePane) + panes.filter(id => !isUncloseablePane(id)).forEach(closeTabPane) } /** Pane ids in the tree under a `${prefix}:` namespace — lets a mirror prune @@ -616,6 +626,16 @@ export function dismissTreePane(paneId: string) { } } +/** Tab ✕ on a tool panel (terminal/logs): dismiss from the layout AND sync + * the owning store so the toggle stays truthful. The pane is removed first + * so the store listener's setPaneCollapsed is a no-op (pane already gone), + * not a minimize that strands a shared-zone sibling. The toggle's open + * path (revealTreePane in bindPaneCollapse) un-dismisses + re-adopts. */ +export function closeCollapsePane(paneId: string) { + dismissTreePane(paneId) + paneClosers[paneId]?.() +} + export function closeTreePane(paneId: string) { const closer = paneClosers[paneId]