From f65fa01926fee90992df40ad7a56251ff9ef237e Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 28 Jul 2026 01:12:43 -0500 Subject: [PATCH] =?UTF-8?q?desktop:=20=E2=8C=981=E2=80=939=20slots=20map?= =?UTF-8?q?=20to=20visible=20tabs,=20not=20raw=20pane=20indices?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Focus layout keeps `files` in the workspace group's panes while chrome- hidden. Slot keys walked that raw array, so ⌘2 landed on the first session chip and every later number was off-by-one (especially after a ⌘W main-tab shift). Index the same shown filter the strip paints, and cycle the same. --- .../src/components/pane-shell/tree/store.ts | 61 +++++++++++--- .../pane-shell/tree/tab-slot-shown.test.ts | 81 +++++++++++++++++++ 2 files changed, 132 insertions(+), 10 deletions(-) create mode 100644 apps/desktop/src/components/pane-shell/tree/tab-slot-shown.test.ts diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index b07f4eb4424..5f4b494dd64 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -350,14 +350,52 @@ export function treePanesWithPrefix(prefix: string): string[] { * An atom so the strip re-renders when the action becomes available. */ export const $newSessionTabAction = atom<(() => void) | null>(null) -/** ⌘1…⌘9: activate the Nth tab of the FOCUSED zone (the interaction tracker's - * group), but only when it's a real tab strip (≥2 panes). Returns false so the - * caller falls back to its default (profile switch) — the number keys mean - * "switch tab" only while a multi-tab zone holds focus. */ +/** + * Keyboard slots (⌘1…⌘9, ⌃Tab) must index the SAME tabs the strip paints — + * chrome-hidden panes (files in Focus layout), unregistered ones, and + * narrow-collapsed collapsibles stay in `group.panes` but aren't chips. Walking + * the raw array made ⌘2 land on what the strip called tab 1 after a hidden + * pane sat earlier in the list (classic after-⌘W-shift offset). + */ +function shownPanesInGroup(group: { panes: readonly string[] }): string[] { + const hidden = $hiddenTreePanes.get() + const registered = registry.getArea('panes') + const paneFor = (id: string) => registered.find(c => c.id === id) + + return group.panes.filter(id => { + const pane = paneFor(id) + + if (!pane) { + return false + } + + if (hidden.has(id)) { + return false + } + + // Match TreeGroup's paneShown for the narrow breakpoint — collapsible + // panes drop out of the strip when the viewport collapses them. + if ( + typeof window !== 'undefined' && + window.matchMedia?.(SIDEBAR_COLLAPSE_MEDIA_QUERY).matches && + Boolean((pane.data as { collapsible?: boolean } | undefined)?.collapsible) + ) { + return false + } + + return true + }) +} + +/** ⌘1…⌘9: activate the Nth *visible* tab of the FOCUSED zone (the interaction + * tracker's group), but only when it's a real tab strip (≥2 shown panes). + * Returns false so the caller falls back to its default (profile switch) — + * the number keys mean "switch tab" only while a multi-tab zone holds focus. */ export function activateTreeTabSlot(slot: number): boolean { const groupId = $activeTreeGroup.get() const tree = $layoutTree.get() - const panes = (groupId && tree ? findGroup(tree, groupId)?.panes : null) ?? [] + const group = groupId && tree ? findGroup(tree, groupId) : null + const panes = group ? shownPanesInGroup(group) : [] if (panes.length < 2 || slot < 1 || slot > panes.length) { return false @@ -368,20 +406,23 @@ export function activateTreeTabSlot(slot: number): boolean { return true } -/** ⌃Tab / ⌃⇧Tab: cycle the FOCUSED zone's tabs (wrapping) — but only a - * session/main strip with ≥2 tabs. Returns false so the caller falls back to - * the recent-session switcher when the focus isn't a chat tab strip. */ +/** ⌃Tab / ⌃⇧Tab: cycle the FOCUSED zone's *visible* tabs (wrapping) — but only a + * session/main strip with ≥2 shown tabs. Returns false so the caller falls + * back to the recent-session switcher when the focus isn't a chat tab strip. */ export function cycleTreeTabInFocusedZone(direction: 1 | -1): boolean { const groupId = $activeTreeGroup.get() const tree = $layoutTree.get() const group = groupId && tree ? findGroup(tree, groupId) : null - const panes = group?.panes ?? [] + const panes = group ? shownPanesInGroup(group) : [] if (panes.length < 2 || !panes.some(id => id === 'workspace' || id.startsWith('session-tile:'))) { return false } - const idx = Math.max(0, panes.indexOf(group!.active ?? '')) + // Active may itself be hidden (Files collapsed mid-cycle) — treat it as + // missing so the step starts from a real chip rather than landing on a ghost. + const current = Math.max(0, panes.indexOf(group!.active ?? '')) + const idx = panes.includes(group!.active ?? '') ? current : 0 const nextId = panes[(idx + direction + panes.length) % panes.length] activateTreePane(group!.id, nextId) diff --git a/apps/desktop/src/components/pane-shell/tree/tab-slot-shown.test.ts b/apps/desktop/src/components/pane-shell/tree/tab-slot-shown.test.ts new file mode 100644 index 00000000000..5d8921a9192 --- /dev/null +++ b/apps/desktop/src/components/pane-shell/tree/tab-slot-shown.test.ts @@ -0,0 +1,81 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// ⌘1…⌘9 / ⌃Tab must index the same tabs the strip paints. A chrome-hidden +// pane (Focus layout's `files`) stays in `group.panes` but isn't a chip — +// indexing the raw array made ⌘2 land on the strip's first session tab. + +describe('activateTreeTabSlot indexes shown panes only', () => { + 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') + + for (const id of ['workspace', 'files', 'session-tile:a', 'session-tile:b']) { + registry.register({ + area: 'panes', + data: id === 'files' ? { collapsible: true, placement: 'side' } : { placement: 'main' }, + id, + render: () => null, + title: id + }) + } + + // Focus-ish packing: files rides with workspace; two session tiles stacking + // of top of that — pane order in the tree: workspace, files, A, B. + tree.declareDefaultTree( + model.group(['workspace', 'files', 'session-tile:a', 'session-tile:b'], { + active: 'workspace', + id: 'grp-main' + }) + ) + tree.noteActiveTreeGroup('grp-main') + tree.setTreePaneHidden('files', true) + + const activeOf = () => model.findGroup(tree.$layoutTree.get()!, 'grp-main')?.active ?? null + + return { activeOf, tree } + } + + it('⌘1 is workspace and ⌘2 is the first SESSION tab when files is hidden', async () => { + const { activeOf, tree } = await setup() + + expect(tree.activateTreeTabSlot(1)).toBe(true) + expect(activeOf()).toBe('workspace') + + expect(tree.activateTreeTabSlot(2)).toBe(true) + expect(activeOf()).toBe('session-tile:a') + + expect(tree.activateTreeTabSlot(3)).toBe(true) + expect(activeOf()).toBe('session-tile:b') + }) + + it('does not claim a slot past the visible count (falls through to profile switch)', async () => { + const { tree } = await setup() + + // Shown: workspace + A + B → 3. Slot 4 would have been `B` on the raw array + // (workspace, files, A, B) before this fix — now it correctly refuses. + expect(tree.activateTreeTabSlot(4)).toBe(false) + }) + + it('⌃Tab cycles only visible chips', async () => { + const { activeOf, tree } = await setup() + + expect(tree.cycleTreeTabInFocusedZone(1)).toBe(true) + expect(activeOf()).toBe('session-tile:a') + + expect(tree.cycleTreeTabInFocusedZone(1)).toBe(true) + expect(activeOf()).toBe('session-tile:b') + + expect(tree.cycleTreeTabInFocusedZone(1)).toBe(true) + expect(activeOf()).toBe('workspace') + }) +})