From 4aa672b23e518bb2e628f4190a1385d64e4b0ca3 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 20:57:04 -0500 Subject: [PATCH] =?UTF-8?q?fix(desktop):=20stop=20cold-start=20resume=20ho?= =?UTF-8?q?ming=20focus=20to=20the=20workspace=20tab=20(=E2=8C=98R=20tab?= =?UTF-8?q?=20persistence)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The layout tree persists the active tab, but every reload landed on main anyway: boot's route resume sets $selectedStoredSessionId, and the selection listener in store/session-states.ts treats every selection change as a navigation — noteActiveTreeGroup(null) + revealTreePane('workspace') — fronting the workspace tab over the persisted active tile and then persisting that clobber. A cold-start restore is a re-attachment, not a navigation, so use-route-resume now arms a one-shot (markSelectionRestore) before dispatching the window's FIRST resume; the selection listener consumes it and skips homing exactly once. Every later resume — sidebar click, route change, reconnect — homes as before, and starting on /new consumes boot status too so a subsequent session open still homes. --- .../session/hooks/use-route-resume.test.tsx | 45 +++++++++++++++++++ .../src/app/session/hooks/use-route-resume.ts | 19 ++++++++ apps/desktop/src/store/session-states.test.ts | 33 ++++++++++++++ apps/desktop/src/store/session-states.ts | 18 +++++++- 4 files changed, 114 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/app/session/hooks/use-route-resume.test.tsx b/apps/desktop/src/app/session/hooks/use-route-resume.test.tsx index ae7055ff22a..750e20e8aa2 100644 --- a/apps/desktop/src/app/session/hooks/use-route-resume.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-route-resume.test.tsx @@ -3,9 +3,15 @@ import type { MutableRefObject } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' import { $resumeExhaustedSessionId, setResumeExhaustedSessionId } from '@/store/session' +import { markSelectionRestore } from '@/store/session-states' import { useRouteResume } from './use-route-resume' +// The hook only arms the boot-restore one-shot; the listener consuming it lives +// in the real store (covered by session-states.test.ts). Mock the module so the +// store's side effects (persistence listeners) stay out of this harness. +vi.mock('@/store/session-states', () => ({ markSelectionRestore: vi.fn() })) + interface HarnessProps { activeSessionId: null | string activeSessionIdRef: MutableRefObject @@ -192,6 +198,45 @@ describe('useRouteResume', () => { expect(resumeSession).toHaveBeenCalledWith('session-2', true) }) + it('arms the boot-restore one-shot for the FIRST resume only (⌘R tab persistence)', () => { + // Factory mocks survive restoreAllMocks — drop calls earlier tests made. + vi.mocked(markSelectionRestore).mockClear() + const resumeSession = vi.fn(async () => undefined) + const startFreshSessionDraft = vi.fn() + const activeSessionIdRef: MutableRefObject = { current: null } + const creatingSessionRef = { current: false } + const runtimeIdByStoredSessionIdRef = { current: new Map() } + const selectedStoredSessionIdRef: MutableRefObject = { current: null } + + const props = { + activeSessionId: null, + activeSessionIdRef, + creatingSessionRef, + currentView: 'chat', + freshDraftReady: false, + gatewayState: 'open', + resumeSession, + runtimeIdByStoredSessionIdRef, + selectedStoredSessionId: null, + selectedStoredSessionIdRef, + startFreshSessionDraft + } + + // Cold start: the window mounts already routed at /session-1 (the reload). + const { rerender } = render( + + ) + + expect(resumeSession).toHaveBeenCalledWith('session-1', true) + expect(markSelectionRestore).toHaveBeenCalledTimes(1) + + // A later route change is a real navigation: resume fires, one-shot doesn't. + rerender() + + expect(resumeSession).toHaveBeenCalledWith('session-2', true) + expect(markSelectionRestore).toHaveBeenCalledTimes(1) + }) + it('resumes the selected route again when the gateway reconnects', () => { const resumeSession = vi.fn(async () => undefined) const startFreshSessionDraft = vi.fn() diff --git a/apps/desktop/src/app/session/hooks/use-route-resume.ts b/apps/desktop/src/app/session/hooks/use-route-resume.ts index ed12a91da42..3dedced3ca8 100644 --- a/apps/desktop/src/app/session/hooks/use-route-resume.ts +++ b/apps/desktop/src/app/session/hooks/use-route-resume.ts @@ -2,6 +2,7 @@ import { type MutableRefObject, useEffect, useRef } from 'react' import { isNewChatRoute } from '@/app/routes' import { setResumeExhaustedSessionId } from '@/store/session' +import { markSelectionRestore } from '@/store/session-states' interface RouteResumeOptions { activeSessionId: string | null @@ -85,6 +86,12 @@ export function useRouteResume({ const lastPathnameRef = useRef(null) const seenGatewayStateRef = useRef(false) const wasGatewayOpenRef = useRef(false) + // True until the FIRST resume this window dispatches. That resume is the + // cold-start restore of a route that was already loaded before the reload — + // a re-attachment, not a navigation — so it must not home focus/tabs to the + // workspace (which would clobber the persisted active tab; ⌘R always landed + // on main). Every later resume is a real navigation and homes as usual. + const bootResumeRef = useRef(true) // Per-session retry bookkeeping for the bounded auto-retry effect below. Keyed // by the session id we're retrying so switching chats resets the counter. const retrySessionIdRef = useRef(null) @@ -148,6 +155,16 @@ export function useRouteResume({ // rebinds/reaps the session on its side, and trusting it strands Desktop on // a dead id ("session not found"). Otherwise keep skipping when already active. if ((gatewayBecameOpen || !alreadyActive) && shouldResume && !creatingSessionRef.current) { + // The window's FIRST resume re-attaches the pre-reload route rather + // than navigating anywhere, so the selection listener must not home + // focus/tabs to the workspace over the persisted layout (see + // markSelectionRestore). One-shot: consumed by the selection change + // resumeSession makes synchronously at entry. + if (bootResumeRef.current) { + markSelectionRestore() + } + + bootResumeRef.current = false void resumeSession(routedSessionId, true) } @@ -160,6 +177,8 @@ export function useRouteResume({ (selectedStoredSessionId || activeSessionId || !freshDraftReady) && !rawHashLooksLikeSession() ) { + // A fresh draft is a real navigation — any later resume homes normally. + bootResumeRef.current = false startFreshSessionDraft(true) } }, [ diff --git a/apps/desktop/src/store/session-states.test.ts b/apps/desktop/src/store/session-states.test.ts index f0b4d4c4faf..4133babb927 100644 --- a/apps/desktop/src/store/session-states.test.ts +++ b/apps/desktop/src/store/session-states.test.ts @@ -2,10 +2,13 @@ import { describe, expect, it } from 'vitest' import type { ClientSessionState } from '@/app/types' import { group, split } from '@/components/pane-shell/tree/model' +import { $layoutTree } from '@/components/pane-shell/tree/store' +import { $selectedStoredSessionId } from '@/store/session' import type { SessionTile } from '@/store/session-states' import { blankDraftTile, focusedSessionNeedsRoute, + markSelectionRestore, orderTilesByTree, selectionHomesToWorkspace } from '@/store/session-states' @@ -51,6 +54,36 @@ describe('selectionHomesToWorkspace', () => { }) }) +describe('boot-restore selection homing (⌘R tab persistence)', () => { + const mainGroup = () => group(['workspace', tilePane('t')], { active: tilePane('t'), id: 'main' }) + + const activePane = () => { + const tree = $layoutTree.get() + + return tree?.type === 'group' ? tree.active : null + } + + it('a normal selection change fronts the workspace tab over an active tile', () => { + $layoutTree.set(mainGroup()) + $selectedStoredSessionId.set('nav-1') + + expect(activePane()).toBe('workspace') + }) + + it('markSelectionRestore skips homing exactly once, so the persisted active tab survives a reload', () => { + $layoutTree.set(mainGroup()) + markSelectionRestore() + $selectedStoredSessionId.set('boot-1') + + // Boot restore: the tile tab the user reloaded on stays fronted. + expect(activePane()).toBe(tilePane('t')) + + // One-shot consumed: the next selection change is a real navigation. + $selectedStoredSessionId.set('nav-2') + expect(activePane()).toBe('workspace') + }) +}) + describe('focusedSessionNeedsRoute', () => { it('routes when the session is not on screen', () => { expect(focusedSessionNeedsRoute(null, false)).toBe(true) diff --git a/apps/desktop/src/store/session-states.ts b/apps/desktop/src/store/session-states.ts index a44329f10d3..5d3f41c4fc6 100644 --- a/apps/desktop/src/store/session-states.ts +++ b/apps/desktop/src/store/session-states.ts @@ -768,10 +768,26 @@ export const $focusedSessionState = computed([$focusedRuntimeId, $sessionStates] export const selectionHomesToWorkspace = (selected: null | string, tiles: readonly SessionTile[]): boolean => !(selected && tiles.some(t => t.storedSessionId === selected)) +// Cold-start restore is the one selection change that is NOT a navigation: the +// route already pointed at the primary session before the window loaded, and +// homing on it would front the workspace tab over the PERSISTED active tab — +// then persist that clobber, so the tab you reloaded on never comes back +// (⌘R always landing on main). use-route-resume arms this one-shot right +// before dispatching the boot resume; the very next selection change skips +// homing and the restored layout tree keeps its say. +let selectionRestoreInFlight = false + +export function markSelectionRestore() { + selectionRestoreInFlight = true +} + // Homing also FRONTS the workspace tab: the resumed chat loads in the workspace // pane, so a zone parked on a tile tab must switch back or the click looks dead. $selectedStoredSessionId.listen(selected => { - if (!selectionHomesToWorkspace(selected, $sessionTiles.get())) { + const restoring = selectionRestoreInFlight + selectionRestoreInFlight = false + + if (restoring || !selectionHomesToWorkspace(selected, $sessionTiles.get())) { return }