From 0ad06ae9da55b5ce3df3f15f1a34e818b6942ee3 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 23:58:41 -0500 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20keep=20=E2=8C=981-9=20working?= =?UTF-8?q?=20when=20the=20pointer=20is=20off=20the=20panes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hover-first targeting (#74447) made the tab verbs read the hovered zone else the focused one. But ⌘1-9 and ⌃Tab then took that single answer as final: with the pointer over the sidebar, the titlebar, or any non-zone chrome, the resolver returned null and the keys did nothing at all. Turn the resolver into an eligibility ladder — hovered, then focused, then the workspace's zone — where each rung must actually satisfy the verb (a real tab strip for the number keys, a chat strip for ⌃Tab and the ⌘W / ⌘T family) to claim the keystroke. Pointing at nothing now falls through to focus, and a fresh window with no interaction yet falls through to main, so the keys always land somewhere sensible. This also fixes the narrower case the old code shared with ⌘W: hovering a zone that is NOT a tab strip (a lone file tree) used to swallow the key rather than handing it to the next rung. ⌘W / ⌘T already laddered to the workspace, so they keep their behavior and simply share the one resolver again. --- .../pane-shell/tree/hovered-zone-tabs.test.ts | 57 ++++++++++ .../src/components/pane-shell/tree/store.ts | 103 ++++++++++-------- 2 files changed, 115 insertions(+), 45 deletions(-) diff --git a/apps/desktop/src/components/pane-shell/tree/hovered-zone-tabs.test.ts b/apps/desktop/src/components/pane-shell/tree/hovered-zone-tabs.test.ts index 4112e181614..e508cd258ba 100644 --- a/apps/desktop/src/components/pane-shell/tree/hovered-zone-tabs.test.ts +++ b/apps/desktop/src/components/pane-shell/tree/hovered-zone-tabs.test.ts @@ -87,4 +87,61 @@ describe('hovered zone retargets the tab verbs', () => { expect(model.allPaneIds(tree.$layoutTree.get()!)).not.toContain('session-tile:c') expect(model.allPaneIds(tree.$layoutTree.get()!)).toContain('workspace') }) + + // Hovering chrome that is not a zone at all (the sidebar, the titlebar, the + // statusbar) reports no group. The keys must fall through to focus rather + // than dead-ending — pointing away from the panes is not a reason to stop + // switching tabs. + it('hovering non-pane chrome falls through to the focused zone', async () => { + const { activeOf, tree } = await setup() + + tree.noteActiveTreeGroup('grp-side') + tree.noteHoveredTreeGroup(null) + + expect(tree.activateTreeTabSlot(2)).toBe(true) + expect(activeOf('grp-side')).toBe('session-tile:c') + expect(tree.cycleTreeTabInFocusedZone(1)).toBe(true) + expect(activeOf('grp-side')).toBe('session-tile:b') + }) + + // Nothing interacted with yet (fresh boot) or focus parked somewhere that + // can't serve the verb: main is the last rung, so ⌘2 still works. + it('falls through to the workspace zone with neither hover nor focus', async () => { + const { activeOf, tree } = await setup() + + tree.noteActiveTreeGroup(null) + tree.noteHoveredTreeGroup(null) + + expect(tree.activateTreeTabSlot(2)).toBe(true) + expect(activeOf('grp-main')).toBe('session-tile:a') + expect(activeOf('grp-side')).toBe('session-tile:b') + }) + + // The ladder is per-rung eligible, not "first rung wins": a hovered zone that + // ISN'T a tab strip must hand the keys to the next rung, not swallow them. + it('an ineligible hovered zone hands off instead of swallowing the key', async () => { + const { activeOf, model, tree } = await setup() + const { registry } = await import('@/contrib/registry') + + registry.register({ area: 'panes', data: { placement: 'right' }, id: 'files', render: () => null, title: 'files' }) + + // Replace the tree outright — declareDefaultTree only ADOPTS into an + // existing one, so the lone-files zone has to be set directly. + tree.$layoutTree.set( + model.split('row', [ + model.group(['workspace', 'session-tile:a'], { active: 'workspace', id: 'grp-main' }), + model.group(['files'], { active: 'files', id: 'grp-files' }) + ]) + ) + + // Pointer resting on the lone file tree, focus nowhere. + tree.noteActiveTreeGroup(null) + tree.noteHoveredTreeGroup('grp-files') + + expect(tree.activateTreeTabSlot(2)).toBe(true) + expect(activeOf('grp-main')).toBe('session-tile:a') + // ⌘W must not close the file tree from a rung that can't serve it. + expect(activeOf('grp-files')).toBe('files') + expect(model.allPaneIds(tree.$layoutTree.get()!)).toContain('files') + }) }) diff --git a/apps/desktop/src/components/pane-shell/tree/store.ts b/apps/desktop/src/components/pane-shell/tree/store.ts index aed1ccc4b00..cf09dde33e0 100644 --- a/apps/desktop/src/components/pane-shell/tree/store.ts +++ b/apps/desktop/src/components/pane-shell/tree/store.ts @@ -269,13 +269,33 @@ export function noteHoveredTreeGroup(groupId: null | string) { } } -/** The zone every keyboard tab verb acts on: the HOVERED zone, else the focused - * one. Hover-first is what makes ⌘1…⌘9 land in the pane you're pointing at - * without clicking into it first; with the pointer outside the panes it's the - * plain focused-zone behavior. One resolver so the number keys, ⌃Tab, and the - * ⌘W / ⌘T family can never disagree about which zone is "the" zone. */ -function tabTargetGroupId(): null | string { - return $hoveredTreeGroup.get() ?? $activeTreeGroup.get() +/** The zone every keyboard tab verb acts on, as an ELIGIBILITY LADDER: the + * hovered zone, else the focused one, else the workspace's. Each rung must + * satisfy `eligible` to claim the keys, so a pointer parked somewhere that + * can't serve the verb — the sidebar, the titlebar, a single-pane rail — + * hands off to the next rung instead of swallowing the keystroke. Hover-first + * is what makes ⌘1…⌘9 land in the pane you're pointing at without clicking + * into it; the rungs below are why the keys still work when you're pointing + * at nothing. One resolver so the number keys, ⌃Tab, and the ⌘W / ⌘T family + * can never disagree about which zone is "the" zone. */ +function tabTargetGroup(eligible: (group: GroupNode) => boolean): GroupNode | null { + const tree = $layoutTree.get() + + if (!tree) { + return null + } + + for (const groupId of [$hoveredTreeGroup.get(), $activeTreeGroup.get()]) { + const group = groupId ? findGroup(tree, groupId) : null + + if (group && eligible(group)) { + return group + } + } + + const main = findGroupOfPane(tree, 'workspace') + + return main && eligible(main) ? main : null } const treeGroupOfEvent = (event: Event): null | string => { @@ -328,23 +348,13 @@ export const isSessionStripPane = (paneId: string): boolean => paneId === 'workspace' || paneId.startsWith('session-tile:') /** The zone the session-tab verbs (⌘W / ⌘T / ⌘⇧T / the strip's "+") act on: - * the TARGET zone (hovered, else focused) when it hosts a chat strip, else the - * workspace's zone. Same source ⌘1…⌘9 indexes (`tabTargetGroupId`), so the - * number keys and the tab verbs can't disagree about which strip is "the" - * strip. A target parked in the sidebar / terminal / files must NOT retarget - * them — those zones fall back to main rather than letting ⌘W close the file - * tree. */ + * the first of hovered / focused / workspace that hosts a chat strip. Same + * ladder ⌘1…⌘9 indexes, so the number keys and the tab verbs can't disagree + * about which strip is "the" strip. A target parked in the sidebar / terminal + * / files must NOT retarget them — those zones fall through to main rather + * than letting ⌘W close the file tree. */ function focusedSessionGroup(): GroupNode | null { - const tree = $layoutTree.get() - - if (!tree) { - return null - } - - const groupId = tabTargetGroupId() - const focused = groupId ? findGroup(tree, groupId) : null - - return focused?.panes.some(isSessionStripPane) ? focused : findGroupOfPane(tree, 'workspace') + return tabTargetGroup(group => group.panes.some(isSessionStripPane)) } /** The pane a NEW session tab should dock beside (⌘T): the focused chat zone's @@ -467,50 +477,53 @@ function shownPanesInGroup(group: { panes: readonly string[] }): string[] { }) } -/** ⌘1…⌘9: activate the Nth *visible* tab of the target zone (hovered, else - * focused), 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 is targeted. */ +/** ⌘1…⌘9: activate the Nth *visible* tab of the target zone — the first of + * hovered / focused / workspace that is a real tab strip (≥2 shown panes). + * Pointing at the sidebar (or nothing) therefore still switches main's tabs + * instead of dead-ending. Returns false so the caller falls back to its + * default (profile switch) when no zone qualifies. */ export function activateTreeTabSlot(slot: number): boolean { - const groupId = tabTargetGroupId() - const tree = $layoutTree.get() - const group = groupId && tree ? findGroup(tree, groupId) : null + const group = tabTargetGroup(candidate => shownPanesInGroup(candidate).length >= 2) const panes = group ? shownPanesInGroup(group) : [] - if (panes.length < 2 || slot < 1 || slot > panes.length) { + if (!group || slot < 1 || slot > panes.length) { return false } - activateTreePane(groupId!, panes[slot - 1]) + activateTreePane(group.id, panes[slot - 1]) return true } -/** ⌃Tab / ⌃⇧Tab: cycle the target 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 target isn't a chat tab strip. */ +/** ⌃Tab / ⌃⇧Tab: cycle the target zone's *visible* tabs (wrapping) — the first + * of hovered / focused / workspace that is a chat strip with ≥2 shown tabs. + * Returns false so the caller falls back to the recent-session switcher when + * no zone qualifies. */ export function cycleTreeTabInFocusedZone(direction: 1 | -1): boolean { - const groupId = tabTargetGroupId() - const tree = $layoutTree.get() - const group = groupId && tree ? findGroup(tree, groupId) : null - const panes = group ? shownPanesInGroup(group) : [] + const group = tabTargetGroup(candidate => { + const shown = shownPanesInGroup(candidate) - if (panes.length < 2 || !panes.some(isSessionStripPane)) { + return shown.length >= 2 && shown.some(isSessionStripPane) + }) + + if (!group) { return false } + const panes = shownPanesInGroup(group) + // 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 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) + activateTreePane(group.id, nextId) // Cycling onto a session/main tab must surface the name card — a zone that // was double-tap-hidden stays headerless otherwise ("the one that cycles // never gets it"). - if (nextId === 'workspace' || nextId.startsWith('session-tile:')) { - setTreeGroupHeaderHidden(group!.id, false) + if (isSessionStripPane(nextId)) { + setTreeGroupHeaderHidden(group.id, false) } return true