diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index cf1f063ec8a..8195a1eb170 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -49,6 +49,7 @@ import { import { useGlobalFileDrop } from './hooks/useGlobalFileDrop' import { registerUpdaterBeforeUnloadBypass } from './lib/updater-beforeunload' import { buildWorkspaceSessionPayload } from './lib/workspace-session' +import { createSessionWriteSubscriber } from './lib/session-write-subscriber' import { applyDocumentTheme } from './lib/document-theme' import { isEditableTarget } from './lib/editable-target' import { @@ -416,25 +417,10 @@ function App(): React.JSX.Element { // Using a Zustand subscribe() outside React removes ~15 subscriptions from // App's render cycle, eliminating re-renders on every tab/file/browser change. useEffect(() => { - let timer: number | null = null - const unsub = useAppStore.subscribe((state) => { - if (!state.workspaceSessionReady) { - return - } - if (timer) { - window.clearTimeout(timer) - } - timer = window.setTimeout(() => { - timer = null - void window.api.session.set(buildWorkspaceSessionPayload(state)) - }, 150) + return createSessionWriteSubscriber({ + store: useAppStore, + persist: (payload) => void window.api.session.set(payload) }) - return () => { - unsub() - if (timer) { - window.clearTimeout(timer) - } - } }, []) // On shutdown, capture terminal scrollback buffers and flush to disk. diff --git a/src/renderer/src/lib/session-write-subscriber.test.ts b/src/renderer/src/lib/session-write-subscriber.test.ts new file mode 100644 index 00000000000..19701600e3b --- /dev/null +++ b/src/renderer/src/lib/session-write-subscriber.test.ts @@ -0,0 +1,139 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { WorkspaceSessionState } from '../../../shared/types' +import { useAppStore, type AppState } from '@/store' +import { createSessionWriteSubscriber } from './session-write-subscriber' + +// Why: useAppStore is a module-level singleton — tests must snapshot and +// restore the full state around each case so cross-test pollution can't mask +// a real regression in the gate logic this suite exists to lock down. +let initialState: AppState + +describe('createSessionWriteSubscriber', () => { + beforeEach(() => { + initialState = useAppStore.getState() + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + useAppStore.setState(initialState, true) + }) + + it('does not write while workspaceSessionReady is false', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ tabsByWorktree: { 'wt-1': [] } }) + vi.advanceTimersByTime(200) + + expect(persist).not.toHaveBeenCalled() + cleanup() + }) + + it('writes exactly once after workspaceSessionReady flips to true', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ workspaceSessionReady: true }) + vi.advanceTimersByTime(200) + + expect(persist).toHaveBeenCalledTimes(1) + cleanup() + }) + + it('ignores mutations to fields outside SESSION_RELEVANT_FIELDS', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ workspaceSessionReady: true }) + vi.advanceTimersByTime(200) + expect(persist).toHaveBeenCalledTimes(1) + persist.mockClear() + + // setAgentStatus / setCacheTimerStartedAt mutate fields that are NOT in + // SESSION_RELEVANT_FIELDS — the gate must skip the timer reset entirely. + useAppStore.getState().setAgentStatus('tab-1:1', { + state: 'working', + prompt: 'Fix tests', + agentType: 'codex' + }) + useAppStore.getState().setCacheTimerStartedAt('tab-1:pane-1', Date.now()) + vi.advanceTimersByTime(200) + + expect(persist).not.toHaveBeenCalled() + cleanup() + }) + + it('writes exactly once when a relevant field changes', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ workspaceSessionReady: true }) + vi.advanceTimersByTime(200) + expect(persist).toHaveBeenCalledTimes(1) + persist.mockClear() + + useAppStore.setState({ + tabsByWorktree: { + 'wt-1': [ + { + id: 'tab-1', + ptyId: null, + worktreeId: 'wt-1', + title: 'shell', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + } + }) + vi.advanceTimersByTime(200) + + expect(persist).toHaveBeenCalledTimes(1) + cleanup() + }) + + it('coalesces multiple relevant mutations within a debounce window', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ workspaceSessionReady: true }) + vi.advanceTimersByTime(200) + persist.mockClear() + + useAppStore.setState({ activeRepoId: 'repo-1' }) + vi.advanceTimersByTime(50) + useAppStore.setState({ activeWorktreeId: 'wt-1' }) + vi.advanceTimersByTime(50) + useAppStore.setState({ activeTabId: 'tab-1' }) + vi.advanceTimersByTime(200) + + expect(persist).toHaveBeenCalledTimes(1) + cleanup() + }) + + it('cleanup unsubscribes and cancels a pending timer', () => { + const persist = vi.fn<(payload: WorkspaceSessionState) => void>() + const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) + + useAppStore.setState({ workspaceSessionReady: true }) + vi.advanceTimersByTime(200) + persist.mockClear() + + useAppStore.setState({ activeTabId: 'tab-1' }) + cleanup() + vi.advanceTimersByTime(200) + + expect(persist).not.toHaveBeenCalled() + + // Why: without this second mutation, the assertion above only proves the + // pending timer was cancelled — a regression where cleanup() forgot to + // unsub() would still pass. Mutating after cleanup verifies the listener + // was detached and no new timer is queued. + useAppStore.setState({ activeTabId: 'tab-2' }) + vi.advanceTimersByTime(200) + expect(persist).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/session-write-subscriber.ts b/src/renderer/src/lib/session-write-subscriber.ts new file mode 100644 index 00000000000..b25fa82a1b2 --- /dev/null +++ b/src/renderer/src/lib/session-write-subscriber.ts @@ -0,0 +1,81 @@ +import type { AppState } from '../store' +import type { WorkspaceSessionState } from '../../../shared/types' +import { buildWorkspaceSessionPayload, SESSION_RELEVANT_FIELDS } from './workspace-session' + +export type SessionWriteSubscriberDeps = { + store: { + subscribe: (listener: (state: AppState) => void) => () => void + getState: () => AppState + } + persist: (payload: WorkspaceSessionState) => void + debounceMs?: number +} + +/** + * Why: factored out so a vitest can drive the real Zustand store and assert + * which mutations cause a session write — the gate against unrelated updates + * (agent status, usage, runtime title ticks) is load-bearing for setTimeout + * violation budgets and the failure mode is silent. + */ +export function createSessionWriteSubscriber({ + store, + persist, + debounceMs = 150 +}: SessionWriteSubscriberDeps): () => void { + let timer: ReturnType | null = null + // Why: the subscriber fires on every store update (agent status, usage + // refreshes, runtime title ticks, …). Without this gate each fire reset + // the debounce, and when it finally expired buildWorkspaceSessionPayload + // crossed 70-110ms with many tabs, tripping setTimeout violations. Compare + // each session-feeding field by reference against the prior snapshot and + // skip both the timer reset and the rebuild when none changed. `null` + // sentinel guarantees the very first fire always proceeds. + let prev: Record | null = null + + const unsub = store.subscribe((state) => { + if (!state.workspaceSessionReady) { + return + } + let changed = false + if (prev === null) { + changed = true + } else { + for (const key of SESSION_RELEVANT_FIELDS) { + if (prev[key] !== state[key]) { + changed = true + break + } + } + } + if (!changed) { + return + } + const next: Record = {} + for (const key of SESSION_RELEVANT_FIELDS) { + next[key] = state[key] + } + prev = next + if (timer !== null) { + clearTimeout(timer) + } + timer = setTimeout(() => { + timer = null + // Why: rebuild from the freshest store state rather than the snapshot + // captured when this timer was scheduled. Today this is equivalent + // because buildWorkspaceSessionPayload reads only SESSION_RELEVANT_FIELDS + // (the same fields gating the timer reset), so the captured `state` is + // already current for those fields. Calling getState() guards against a + // future refactor that adds a non-relevant field read to the payload + // builder — without this, such a change would silently start emitting + // stale values for that field. + persist(buildWorkspaceSessionPayload(store.getState())) + }, debounceMs) + }) + + return () => { + unsub() + if (timer !== null) { + clearTimeout(timer) + } + } +} diff --git a/src/renderer/src/lib/workspace-session.test.ts b/src/renderer/src/lib/workspace-session.test.ts index b7643733f24..873b8be6446 100644 --- a/src/renderer/src/lib/workspace-session.test.ts +++ b/src/renderer/src/lib/workspace-session.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from 'vitest' -import { buildWorkspaceSessionPayload } from './workspace-session' +import { + buildWorkspaceSessionPayload, + SESSION_RELEVANT_FIELDS, + type WorkspaceSessionSnapshot +} from './workspace-session' import type { AppState } from '../store' function createSnapshot(overrides: Partial = {}): AppState { @@ -140,3 +144,49 @@ describe('buildWorkspaceSessionPayload', () => { expect(payload.activeTabTypeByWorktree).toEqual({ 'wt-2': 'terminal' }) }) }) + +describe('SESSION_RELEVANT_FIELDS', () => { + // Why: this list gates the App-level session-write debounce subscriber. + // If a future field is added to WorkspaceSessionSnapshot but not to the + // gate, the subscriber would silently stop noticing changes to that field + // and persist stale data. Listing every key here as a fixture and asserting + // the gate covers them catches the drift at test time. The compile-time + // _exhaustive check in workspace-session.ts is the primary line of defense; + // this test is the runtime backstop. + const fixture: Record = { + activeRepoId: true, + activeWorktreeId: true, + activeTabId: true, + tabsByWorktree: true, + terminalLayoutsByTabId: true, + activeTabIdByWorktree: true, + openFiles: true, + activeFileIdByWorktree: true, + activeTabTypeByWorktree: true, + browserTabsByWorktree: true, + browserPagesByWorkspace: true, + activeBrowserTabIdByWorktree: true, + browserUrlHistory: true, + unifiedTabsByWorktree: true, + groupsByWorktree: true, + layoutByWorktree: true, + activeGroupIdByWorktree: true, + sshConnectionStates: true, + repos: true, + worktreesByRepo: true, + lastKnownRelayPtyIdByTabId: true, + lastVisitedAtByWorktreeId: true + } + + it('contains every key of WorkspaceSessionSnapshot', () => { + const fixtureKeys = Object.keys(fixture) + expect( + fixtureKeys.every((k) => (SESSION_RELEVANT_FIELDS as readonly string[]).includes(k)) + ).toBe(true) + expect(SESSION_RELEVANT_FIELDS.length).toBe(fixtureKeys.length) + }) + + it('has no duplicate entries', () => { + expect(new Set(SESSION_RELEVANT_FIELDS).size).toBe(SESSION_RELEVANT_FIELDS.length) + }) +}) diff --git a/src/renderer/src/lib/workspace-session.ts b/src/renderer/src/lib/workspace-session.ts index 446a6df5ec4..38b18724d23 100644 --- a/src/renderer/src/lib/workspace-session.ts +++ b/src/renderer/src/lib/workspace-session.ts @@ -8,7 +8,7 @@ import type { import type { AppState } from '../store' import type { OpenFile } from '../store/slices/editor' -type WorkspaceSessionSnapshot = Pick< +export type WorkspaceSessionSnapshot = Pick< AppState, | 'activeRepoId' | 'activeWorktreeId' @@ -34,6 +34,44 @@ type WorkspaceSessionSnapshot = Pick< | 'lastVisitedAtByWorktreeId' > +// Why: the App-level Zustand subscriber that debounces session writes uses +// this list as a shallow-equality gate so it only resets the timer when a +// field that actually feeds buildWorkspaceSessionPayload changes. Keeping +// the list co-located with WorkspaceSessionSnapshot means a future field +// added to the snapshot type fails the _exhaustive check below at compile +// time, preventing the gate from silently going stale. +export const SESSION_RELEVANT_FIELDS = [ + 'activeRepoId', + 'activeWorktreeId', + 'activeTabId', + 'tabsByWorktree', + 'terminalLayoutsByTabId', + 'activeTabIdByWorktree', + 'openFiles', + 'activeFileIdByWorktree', + 'activeTabTypeByWorktree', + 'browserTabsByWorktree', + 'browserPagesByWorkspace', + 'activeBrowserTabIdByWorktree', + 'browserUrlHistory', + 'unifiedTabsByWorktree', + 'groupsByWorktree', + 'layoutByWorktree', + 'activeGroupIdByWorktree', + 'sshConnectionStates', + 'repos', + 'worktreesByRepo', + 'lastKnownRelayPtyIdByTabId', + 'lastVisitedAtByWorktreeId' +] as const satisfies readonly (keyof WorkspaceSessionSnapshot)[] + +type _MissingSessionField = Exclude< + keyof WorkspaceSessionSnapshot, + (typeof SESSION_RELEVANT_FIELDS)[number] +> +const _exhaustive: [_MissingSessionField] extends [never] ? true : never = true +void _exhaustive + /** Build the editor-file portion of the workspace session for persistence. * Only edit-mode files are saved — diffs and conflict views are transient. */ export function buildEditorSessionData(