From 4cdfcf568d49106e49ba79904bcf6295d8db7b52 Mon Sep 17 00:00:00 2001 From: David Metcalfe Date: Sat, 18 Jul 2026 20:49:37 -0700 Subject: [PATCH] fix(desktop): don't auto-expand user-collapsed side on reactive unhide (#65375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(desktop): don't auto-expand user-collapsed side on reactive unhide When `setTreePaneHidden(paneId, false)` was called for a workspace-gated pane like `files` (bound to $hasWorkspace), it auto-called `revealTreePane(paneId)` — which expanded the parent column even when the user had explicitly collapsed it via Cmd+J. That meant every session create / resume — anything that flipped $currentCwd from empty to non-empty — silently re-opened the right sidebar and persisted that open state to localStorage, so the original session also showed the sidebar as open on return. `setTreePaneHidden` is a state primitive; user-intent semantics (open the side, front the tab) belong to `revealTreePane`. Drop the auto-call and replace it with a narrower `frontPaneInGroup` helper that only makes the pane the active tab in its group — visible the next time the column is opened, without forcing it open now. * fix(desktop): preserve explicit review reveal --------- Co-authored-by: David Metcalfe <80915+DavidMetcalfe@users.noreply.github.com> --- .../pane-shell/tree/reactive-unhide.test.ts | 147 ++++++++++++++++++ .../src/components/pane-shell/tree/store.ts | 28 +++- apps/desktop/src/store/review.ts | 2 + 3 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src/components/pane-shell/tree/reactive-unhide.test.ts diff --git a/apps/desktop/src/components/pane-shell/tree/reactive-unhide.test.ts b/apps/desktop/src/components/pane-shell/tree/reactive-unhide.test.ts new file mode 100644 index 000000000000..1699fa034db3 --- /dev/null +++ b/apps/desktop/src/components/pane-shell/tree/reactive-unhide.test.ts @@ -0,0 +1,147 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Ground-truth repro for "reactive unhides silently re-open a user-collapsed +// side": `setTreePaneHidden` is the primitive that backs `bindPaneVisibility` +// (e.g. `bindPaneVisibility('files', $hasWorkspace)`). The original +// implementation auto-called `revealTreePane` on every unhide, which expanded +// the parent column even when the user had explicitly collapsed it — so a +// workspace transition (`$currentCwd` flipping on session create / resume) +// forced the right sidebar open and persisted that open state to localStorage. +// +// We mirror controller.tsx exactly: register the `files` pane with +// `placement: 'right'`, declare the default tree, bind the right side to +// the file-browser open store, then collapse the right sidebar and trigger +// the reactive unhide via the same primitive `bindPaneVisibility` invokes. + +describe('reactive pane unhide', () => { + beforeEach(() => { + window.localStorage.clear() + vi.resetModules() + }) + + afterEach(() => { + vi.resetModules() + }) + + async function setupWithFiles() { + const tree = await import('@/components/pane-shell/tree/store') + const layout = await import('@/store/layout') + const model = await import('@/components/pane-shell/tree/model') + const { registry } = await import('@/contrib/registry') + + // Register `files` like controller.tsx does — placement 'right' is what + // makes `treeSideOfPane('files')` return 'right' (and therefore what + // makes the buggy `revealTreePane` auto-expand the right column). + registry.register({ + id: 'files', + area: 'panes', + title: 'files', + data: { placement: 'right' }, + render: () => null + }) + + // Declare a minimal default tree mirroring the production DEFAULT_TREE's + // row shape (sessions | workspace | right-column-with-files). + tree.declareDefaultTree( + model.split('row', [ + model.group(['sessions'], { id: 'grp-sessions' }), + model.group(['workspace'], { id: 'grp-main' }), + model.split('column', [ + model.split('row', [ + model.group(['files'], { id: 'grp-files' }) + ], [1], 'spl-rail'), + model.group(['terminal'], { id: 'grp-terminal' }) + ], [1.6, 1], 'spl-right') + ], [0.85, 3, 1.6]) + ) + + // Mirror controller.tsx:512. + tree.bindTreeSideVisibility('right', layout.$fileBrowserOpen, layout.setFileBrowserOpen) + + return { tree, layout } + } + + it('reactive unhide does NOT expand a user-collapsed side', async () => { + const { tree, layout } = await setupWithFiles() + + // Simulate the production scenario: a detached chat has no cwd, so + // `bindPaneVisibility('files', $hasWorkspace)` calls `setTreePaneHidden( + // 'files', true)` (hidden). Then the user collapses the right sidebar. + // Then a new session acquires a cwd and the workspace wiring unhides + // `files`. That last step is what the bug corrupts. + tree.setTreePaneHidden('files', true) + expect(tree.$hiddenTreePanes.get().has('files')).toBe(true) + + layout.setFileBrowserOpen(false) + expect(layout.$fileBrowserOpen.get()).toBe(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + + // Workspace flips: `bindPaneVisibility('files', $hasWorkspace)` calls + // `setTreePaneHidden('files', false)` — model it directly. + tree.setTreePaneHidden('files', false) + + // The pane unhides… + expect(tree.$hiddenTreePanes.get().has('files')).toBe(false) + + // …but the user-collapsed side MUST stay collapsed. Before the fix, the + // unhide called `revealTreePane`, which saw the side was collapsed and + // called `setFileBrowserOpen(true)` — silently re-opening the right + // sidebar on every session create / resume. + expect(layout.$fileBrowserOpen.get()).toBe(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + }) + + it('reactive HIDE (hidden=true) leaves the side flag alone', async () => { + const { tree, layout } = await setupWithFiles() + + layout.setFileBrowserOpen(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + + // A reactive HIDE (workspace goes away) must not flip the side either — + // collapsing the side is the user's domain; hiding individual panes + // inside it is the workspace domain. + tree.setTreePaneHidden('files', true) + + expect(tree.$hiddenTreePanes.get().has('files')).toBe(true) + expect(layout.$fileBrowserOpen.get()).toBe(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + }) + + it('explicit setFileBrowserOpen still expands the side (user toggle)', async () => { + const { tree, layout } = await setupWithFiles() + + layout.setFileBrowserOpen(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + + // The user toggles the sidebar back open — this MUST still work; the + // binding mirrors `$fileBrowserOpen` to `$collapsedTreeSides`. + layout.setFileBrowserOpen(true) + expect(layout.$fileBrowserOpen.get()).toBe(true) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(false) + }) + + it('reactive unhide does not invoke the right side opener directly', async () => { + const { tree, layout } = await setupWithFiles() + + // Spy on the opener that `revealTreePane` would call when expanding a + // collapsed side — the bug is exactly this call firing on reactive unhide. + const openerSpy = vi.fn() + tree.bindTreeSideVisibility('right', layout.$fileBrowserOpen, openerSpy) + + layout.setFileBrowserOpen(false) + expect(tree.$collapsedTreeSides.get().has('right')).toBe(true) + + tree.setTreePaneHidden('files', true) + expect(tree.$hiddenTreePanes.get().has('files')).toBe(true) + + openerSpy.mockClear() + + // Reactive unhide fires via the same primitive the workspace wiring uses. + tree.setTreePaneHidden('files', false) + + // The opener MUST NOT be called — before the fix, the auto-reveal would + // call `setFileBrowserOpen(true)` via this opener. + expect(tree.$hiddenTreePanes.get().has('files')).toBe(false) + expect(openerSpy).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index 12bab46bf010..f9289ed212b6 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -126,9 +126,33 @@ export function setTreePaneHidden(paneId: string, hidden: boolean) { $hiddenTreePanes.set(next) - // Unhiding is an intent to SEE the pane — front it in its group. + // Reactive unhides (e.g. `bindPaneVisibility('files', $hasWorkspace)`) are + // state-driven, not user intent — opening the side or fronting the tab in + // response to an environmental flag change would clobber an explicit user + // collapse (Cmd+J) and silently re-open the rail after every session create. + // Callers that want user-intent semantics (open the side, front the tab) + // must call `revealTreePane` explicitly. We still front the pane in its + // group so it's visible the next time the column is shown. if (!hidden) { - revealTreePane(paneId) + frontPaneInGroup(paneId) + } +} + +/** Make `paneId` the active tab in its group without touching side collapse + * or zone-minimized state — the safe "make it visible next time the column + * is shown" primitive that reactive unhides need. */ +function frontPaneInGroup(paneId: string) { + const tree = $layoutTree.get() + const group = tree ? findGroupOfPane(tree, paneId) : null + + if (!tree || !group || group.active === paneId) { + return + } + + const next = setActivePaneOp(tree, group.id, paneId) + + if (next !== tree) { + commit(next) } } diff --git a/apps/desktop/src/store/review.ts b/apps/desktop/src/store/review.ts index 8c3a9946d819..79a70947af3b 100644 --- a/apps/desktop/src/store/review.ts +++ b/apps/desktop/src/store/review.ts @@ -2,6 +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 type { HermesReviewFile, HermesReviewShipInfo } from '@/global' import { matchesQuery } from '@/hooks/use-media-query' import { desktopGit } from '@/lib/desktop-git' @@ -274,6 +275,7 @@ export function toggleReview(): void { closeReview() } else { openReview() + revealTreePane(REVIEW_PANE_ID) } }