From cfae306ab6e2f53c4381201c8cbd0f6f48509b48 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Fri, 31 Jul 2026 13:07:51 -0500 Subject: [PATCH] =?UTF-8?q?Revert=20"fix(desktop):=20tool=20panel=20tab=20?= =?UTF-8?q?=E2=9C=95=20can=20close=20and=20re-open"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit dd48d9a8164ba843a44afeef2f7db7720de6c96f. --- 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, 10 insertions(+), 159 deletions(-) delete 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 d48c283b393..78304432c83 100644 --- a/apps/desktop/src/app/contrib/controller.tsx +++ b/apps/desktop/src/app/contrib/controller.tsx @@ -492,11 +492,6 @@ 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 }, @@ -505,13 +500,7 @@ function bindPaneCollapse( ) { markCollapsePane(paneId) setPaneCollapsed(paneId, !$open.get()) - $open.listen(isOpen => { - if (isOpen) { - revealTreePane(paneId) - } else { - setPaneCollapsed(paneId, true) - } - }) + $open.listen(isOpen => setPaneCollapsed(paneId, !isOpen)) 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 deleted file mode 100644 index 9aaf645e197..00000000000 --- a/apps/desktop/src/components/pane-shell/tree/collapse-pane-close.test.ts +++ /dev/null @@ -1,117 +0,0 @@ -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 a47e0eda672..e81bc1afd26 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: () => closeTabPane(paneId) + onSelect: () => closeTreePane(paneId) })} {renderActionItem(kit, { disabled: !targets.others, @@ -287,11 +287,10 @@ 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 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) + // 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)) // 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 0c25946d02d..4bb24460ddf 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -420,22 +420,12 @@ 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(closeTabPane) + closeableTreeSiblings(paneId).others.forEach(closeTreePane) } export function closeTreeTabsToRight(paneId: string): void { - closeableTreeSiblings(paneId).right.forEach(closeTabPane) + closeableTreeSiblings(paneId).right.forEach(closeTreePane) } /** Close every closeable tab in `paneId`'s group (the uncloseable workspace stays). */ @@ -443,7 +433,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(closeTabPane) + panes.filter(id => !isUncloseablePane(id)).forEach(closeTreePane) } /** Pane ids in the tree under a `${prefix}:` namespace — lets a mirror prune @@ -626,16 +616,6 @@ 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]