From bf1e925a1f561705f0836c4bcb611529ceeb8aae Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 12 May 2026 14:31:02 -0700 Subject: [PATCH] perf(session): gate session-write subscriber on relevant field changes (#1720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(session): gate session-write subscriber on relevant field changes The App-level Zustand subscriber that debounces buildWorkspaceSessionPayload fires on every store update (agent status, usage refreshes, runtime title ticks, …). Each fire reset the 150ms timer, and when the timer eventually expired the rebuild crossed 70-110ms with many tabs open, tripping setTimeout violation warnings. Add a shallow reference-equality gate over the fields actually consumed by the payload so the timer only resets when those fields change. The field list is co-located with WorkspaceSessionSnapshot and locked to it via a compile-time exhaustiveness check, so adding a future snapshot field will fail to typecheck rather than silently skip the gate. Co-authored-by: Orca * test(session): regression test for session-write debounce gate Extract the subscriber into createSessionWriteSubscriber so a vitest can drive the real Zustand store and assert which mutations cause a session write. The gate against unrelated updates (agent status, cache timers, runtime title ticks) is load-bearing for setTimeout violation budgets and the failure mode is silent — without this test, future store additions could re-introduce the regression unnoticed. Six cases lock in the contract: no write while not ready, exactly one write when ready flips, no write on unrelated mutations, exactly one write on a relevant mutation, coalescing within a debounce window, and cleanup cancels a pending timer. Co-authored-by: Orca * perf(session): rebuild session payload from latest store state in debounce Replace the closed-over `state` snapshot captured at timer-schedule time with `store.getState()` inside the setTimeout callback. Today this is behaviorally equivalent because `buildWorkspaceSessionPayload` reads only SESSION_RELEVANT_FIELDS (the same fields gating the timer reset), but a future refactor that adds a non-relevant field read to the payload builder would silently start emitting stale values without this guard. Also tighten the cleanup test: mutate the store after `cleanup()` and assert no persist, so a regression where the timer is cancelled but the listener is left subscribed would now fail rather than pass. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/renderer/src/App.tsx | 22 +-- .../src/lib/session-write-subscriber.test.ts | 139 ++++++++++++++++++ .../src/lib/session-write-subscriber.ts | 81 ++++++++++ .../src/lib/workspace-session.test.ts | 52 ++++++- src/renderer/src/lib/workspace-session.ts | 40 ++++- 5 files changed, 314 insertions(+), 20 deletions(-) create mode 100644 src/renderer/src/lib/session-write-subscriber.test.ts create mode 100644 src/renderer/src/lib/session-write-subscriber.ts 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(