From cdf41df37c6c16acae89b06795f0ee5267182632 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 10 Sep 2026 21:09:04 -0700 Subject: [PATCH] perf(terminal): stop rebuilding per-workspace collections on a worktree switch (#19975) Two allocations scale with the workspace count and are rebuilt on inputs that cannot change their result. `workspaceSurfaces` took `renderedActiveWorktreeId` as a memo dep, but `projectWorkspaceSurfaces` reads that id only behind a truthy `activeWorkspaceResolvedHostId` (the folder-collision tie-break), which is null unless the active workspace is itself a folder workspace. Every git-worktree switch therefore re-projected every surface and re-derived the id array to reach an identical answer. Gate the id on the host so the memo holds. The parked-watcher sync built a fresh empty `Set` for every workspace surface, even though only a mounted workspace can park a tab and the sync only reads the set. Share one empty instance for the rest. Both keep every effect firing on exactly the inputs it fired on before. --- ...minal-parked-watcher-sync-entries.test.tsx | 110 ++++++++++++++++++ .../terminal-workspace-surface-ids.test.tsx | 79 +++++++++++++ .../use-terminal-watcher-effects.ts | 14 ++- .../use-terminal-workspace-foundation.ts | 8 +- .../workspace-surface-projection.test.ts | 18 +++ .../workspace-surface-projection.ts | 1 + 6 files changed, 224 insertions(+), 6 deletions(-) create mode 100644 src/renderer/src/components/terminal-parked-watcher-sync-entries.test.tsx diff --git a/src/renderer/src/components/terminal-parked-watcher-sync-entries.test.tsx b/src/renderer/src/components/terminal-parked-watcher-sync-entries.test.tsx new file mode 100644 index 00000000000..e14faedbc18 --- /dev/null +++ b/src/renderer/src/components/terminal-parked-watcher-sync-entries.test.tsx @@ -0,0 +1,110 @@ +// @vitest-environment happy-dom +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { useTerminalWatcherEffects } from './use-terminal-watcher-effects' +import type { ParkedTerminalTabWatcherSyncEntry } from './terminal-pane/terminal-parked-tab-watchers' +import type { TerminalColdActivationController } from './terminal-cold-activation' + +const mocks = vi.hoisted(() => ({ + sync: vi.fn(), + prune: vi.fn() +})) +vi.mock('@/store', () => ({ + useAppStore: Object.assign(() => 'unverifiable', { + getState: () => ({ activeWorktreeId: null }) + }) +})) +vi.mock('@/lib/workspace-terminal-host-authority', () => ({ + createWorkspaceTerminalHostAuthoritySelector: () => () => 'unverifiable' +})) +vi.mock('./terminal-pane/terminal-parked-tab-watchers', () => ({ + canWatcherCoverParkedTerminalTab: () => true, + disposeAllParkedTerminalWatchers: vi.fn(), + pruneParkedTerminalWatchers: mocks.prune, + syncParkedTerminalTabWatchersForWorkspaces: mocks.sync, + terminalWatcherLiveWorkspaceIds: (ids: Iterable) => new Set(ids) +})) +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +const SURFACE_COUNT = 423 +const PARKED_WORKTREE_ID = 'repo-1::/worktree-0' +const surfaceIds = Array.from({ length: SURFACE_COUNT }, (_, index) => `repo-1::/worktree-${index}`) + +let root: Root | undefined +afterEach(async () => { + await act(async () => root?.unmount()) + vi.clearAllMocks() +}) + +function renderWatcherEffects(): Promise { + function Watcher(): null { + useTerminalWatcherEffects({ + activationDeferredMountTabIdsByWorktreeRef: { current: new Map() }, + activeTabId: null, + activeTabIdByWorktree: {}, + activeView: 'terminal', + activeWorktreeId: null, + activityTerminalPortals: [], + anyMountedWorktreeHasLayout: false, + backgroundMountRevision: 0, + effectiveParkedTerminalWorktreeIds: new Set([PARKED_WORKTREE_ID]), + evictionExemptTerminalTabIds: new Set(['tab-exempt']), + getEffectiveLayoutForWorktree: () => null, + groupsByWorktree: {}, + hydrationSucceeded: false, + measurableBackgroundWorktreeIdsRef: { current: new Set() }, + mountedWorktreeIdsRef: { current: new Set([PARKED_WORKTREE_ID]) }, + pendingStartupByTabId: {}, + // Another workspace is on screen, so the mounted one is hidden and parks. + renderedActiveWorktreeId: 'repo-1::/worktree-9', + tabsByWorktree: { + [PARKED_WORKTREE_ID]: [{ id: 'tab-parked' }, { id: 'tab-exempt' }] + }, + terminalParkingEnabled: true, + terminalStartupRestorationReady: false, + terminalTitleSnapshotAuthorityEnabled: true, + workspaceSessionReady: false, + workspaceSurfaceIds: surfaceIds + } as unknown as TerminalColdActivationController) + return null + } + root = createRoot(document.createElement('div')) + return act(async () => root?.render()) +} + +function lastSyncEntries(): Map { + return mocks.sync.mock.calls.at(-1)?.[0] as Map +} + +describe('parked terminal watcher sync entries', () => { + it('publishes an entry for every surface so closed-tab disposal still sees it', async () => { + await renderWatcherEffects() + + const entries = lastSyncEntries() + expect(entries.size).toBe(SURFACE_COUNT) + expect([...entries.keys()]).toEqual(surfaceIds) + expect(mocks.prune).toHaveBeenCalledWith(new Set(surfaceIds)) + }) + + it('parks the hidden mounted workspace tabs and exempts the eviction-exempt tab', async () => { + await renderWatcherEffects() + + const parkedEntry = lastSyncEntries().get(PARKED_WORKTREE_ID) + expect([...(parkedEntry?.parkedTabIds ?? [])]).toEqual(['tab-parked']) + }) + + it('does not allocate a parked-tab-id set per unmounted surface', async () => { + await renderWatcherEffects() + + const entries = lastSyncEntries() + const unmountedSets = new Set( + [...entries] + .filter(([workspaceId]) => workspaceId !== PARKED_WORKTREE_ID) + .map(([, entry]) => entry.parkedTabIds) + ) + // Pre-fix this was one empty Set per surface (422 of them) on every fire. + expect(unmountedSets.size).toBe(1) + expect([...unmountedSets][0]?.size).toBe(0) + }) +}) diff --git a/src/renderer/src/components/terminal-workspace-surface-ids.test.tsx b/src/renderer/src/components/terminal-workspace-surface-ids.test.tsx index 770b1cfc47b..59984d7fe06 100644 --- a/src/renderer/src/components/terminal-workspace-surface-ids.test.tsx +++ b/src/renderer/src/components/terminal-workspace-surface-ids.test.tsx @@ -8,6 +8,7 @@ import { makeRepo, makeWorktree } from './worktree-jump-palette-test-fixtures' import { useTerminalWorkspaceFoundation } from './use-terminal-workspace-foundation' import { applyTerminalColdActivation } from './terminal-cold-activation' import { collectTerminalParkingPassCandidates } from './terminal-parking-pass-candidates' +import type { FolderWorkspace } from '../../../shared/folder-workspace-types' import type { WorkspaceSurface } from './workspace-surface-projection' import type { TerminalParkingFoundation } from './use-terminal-parking-foundation' @@ -147,6 +148,84 @@ describe('workspace surface ids', () => { ]) expect(hiddenSince.has('repo::/stale')).toBe(false) }) + it('keeps the surface projection stable across a git-worktree switch', () => { + const repo = makeRepo() + const worktrees = Array.from({ length: 423 }, (_, index) => + makeWorktree(`repo-1::/worktree-${index}`, `Workspace ${index}`) + ) + useAppStore.setState({ + worktreesByRepo: { [repo.id]: worktrees }, + activeWorktreeId: worktrees[0].id + }) + + const { result } = renderHook(() => useTerminalWorkspaceFoundation()) + const surfaces = result.current.workspaceSurfaces + const ids = result.current.workspaceSurfaceIds + const idSet = result.current.workspaceSurfaceIdSet + expect(surfaces).toHaveLength(423) + + // Pre-fix every switch re-ran the projection (423 surface objects) and re-derived + // the id array, because the active id was a dep even with no folder host to tie-break. + for (let index = 1; index < 10; index += 1) { + act(() => { + useAppStore.setState({ activeWorktreeId: worktrees[index].id }) + }) + expect(result.current.renderedActiveWorktreeId).toBe(worktrees[index].id) + expect(result.current.workspaceSurfaces).toBe(surfaces) + expect(result.current.workspaceSurfaceIds).toBe(ids) + expect(result.current.workspaceSurfaceIdSet).toBe(idSet) + } + }) + + it('still re-projects when the active folder workspace owns the collision tie-break', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const localFolder: FolderWorkspace = { + id: 'folder-shared', + projectGroupId: 'group-shared', + name: 'orca', + folderPath: '/work/orca-local', + connectionId: null, + executionHostId: 'local', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1 + } + const runtimeFolder: FolderWorkspace = { + ...localFolder, + folderPath: '/remote/orca', + executionHostId: 'runtime:env-1' + } + useAppStore.setState({ + folderWorkspaces: [localFolder, runtimeFolder], + activeWorktreeId: 'folder:folder-shared', + activeWorkspaceExecutionHostId: 'runtime:env-1' + }) + + const { result } = renderHook(() => useTerminalWorkspaceFoundation()) + expect(result.current.workspaceSurfaces).toEqual([ + { id: 'folder:folder-shared', path: '/remote/orca' } + ]) + + // Leaving the folder workspace drops the tie-break, so the projection must re-run + // and fall back to first-wins rather than mount the previous host's path. + act(() => { + useAppStore.setState({ + activeWorktreeId: 'repo-1::/worktree-0', + activeWorkspaceExecutionHostId: null + }) + }) + expect(result.current.workspaceSurfaces).toEqual([ + { id: 'folder:folder-shared', path: '/work/orca-local' } + ]) + warn.mockRestore() + }) + it('stops idle worktree writes from re-firing surface-keyed terminal effects', () => { const repo = makeRepo() const worktrees = Array.from({ length: 20 }, (_, index) => diff --git a/src/renderer/src/components/use-terminal-watcher-effects.ts b/src/renderer/src/components/use-terminal-watcher-effects.ts index 3c6a89fb323..fc44352c942 100644 --- a/src/renderer/src/components/use-terminal-watcher-effects.ts +++ b/src/renderer/src/components/use-terminal-watcher-effects.ts @@ -17,6 +17,10 @@ import { getStructuredAgentLaunchStatus } from '@/lib/structured-agent-session-l import { AGENT_SESSION_PROVIDER_HANDLE_PROVIDERS } from '../../../shared/agent-session-provider-handle' import type { TerminalColdActivationController } from './terminal-cold-activation' +// Why shared: only a mounted workspace can park a tab, and the watcher sync only reads +// this set, so every other surface would otherwise allocate its own empty one per fire. +const NO_PARKED_TAB_IDS: ReadonlySet = new Set() + export function useTerminalWatcherEffects(controller: TerminalColdActivationController): void { const { activationDeferredMountTabIdsByWorktreeRef, @@ -58,9 +62,11 @@ export function useTerminalWatcherEffects(controller: TerminalColdActivationCont continue } const tabs = tabsByWorktree[workspaceId] ?? [] - const parkedTabIds = new Set() + let parkedTabIds: ReadonlySet = NO_PARKED_TAB_IDS let deferredTabIds: ReadonlySet | null = null if (!anyMountedWorktreeHasLayout && mountedWorktreeIdsRef.current.has(workspaceId)) { + const mountedParkedTabIds = new Set() + parkedTabIds = mountedParkedTabIds const isVisible = activeView === 'terminal' && workspaceId === renderedActiveWorktreeId const shouldMeasureHiddenWorktree = !isVisible && measurableBackgroundWorktreeIdsRef.current.has(workspaceId) @@ -75,7 +81,7 @@ export function useTerminalWatcherEffects(controller: TerminalColdActivationCont tabId: tab.id }) if (!activityTerminalPortal && !evictionExemptTerminalTabIds.has(tab.id)) { - parkedTabIds.add(tab.id) + mountedParkedTabIds.add(tab.id) } } } @@ -83,14 +89,14 @@ export function useTerminalWatcherEffects(controller: TerminalColdActivationCont for (const tab of tabs) { if ( deferredTabIds?.has(tab.id) && - !parkedTabIds.has(tab.id) && + !mountedParkedTabIds.has(tab.id) && canWatcherCoverParkedTerminalTab(workspaceId, tab) && !findActivityTerminalPortal(activityTerminalPortals, { worktreeId: workspaceId, tabId: tab.id }) ) { - parkedTabIds.add(tab.id) + mountedParkedTabIds.add(tab.id) } } } diff --git a/src/renderer/src/components/use-terminal-workspace-foundation.ts b/src/renderer/src/components/use-terminal-workspace-foundation.ts index 61726fa9643..d68c5104da2 100644 --- a/src/renderer/src/components/use-terminal-workspace-foundation.ts +++ b/src/renderer/src/components/use-terminal-workspace-foundation.ts @@ -37,15 +37,19 @@ export function useTerminalWorkspaceFoundation() { parseWorkspaceKey(renderedActiveWorktreeId ?? '')?.type === 'folder' ? activeWorktreeDeferralHostId : null + // Why gate the id on the host: the projection reads `activeWorkspaceId` only behind a + // truthy resolved host, so without one every active id yields the same surfaces — and a + // worktree switch must not re-project (and re-identify) every surface to rediscover that. + const activeFolderSurfaceId = activeFolderSurfaceHostId ? renderedActiveWorktreeId : null const workspaceSurfaces = useMemo( () => projectWorkspaceSurfaces({ worktreesById, folderWorkspaces, - activeWorkspaceId: renderedActiveWorktreeId, + activeWorkspaceId: activeFolderSurfaceId, activeWorkspaceResolvedHostId: activeFolderSurfaceHostId }), - [worktreesById, folderWorkspaces, renderedActiveWorktreeId, activeFolderSurfaceHostId] + [worktreesById, folderWorkspaces, activeFolderSurfaceId, activeFolderSurfaceHostId] ) // Why split the ids out: every mount/park/activation pass reads only `.id`, but // the surface array is re-identified on any worktree write. Reusing the previous diff --git a/src/renderer/src/components/workspace-surface-projection.test.ts b/src/renderer/src/components/workspace-surface-projection.test.ts index d5226bc5662..7f7c1709ca9 100644 --- a/src/renderer/src/components/workspace-surface-projection.test.ts +++ b/src/renderer/src/components/workspace-surface-projection.test.ts @@ -118,6 +118,24 @@ describe('projectWorkspaceSurfaces', () => { expect(surfaces).toEqual([{ id: 'folder:folder-shared', path: '/remote/orca' }]) }) + it('ignores the active workspace id entirely when no resolved host disambiguates', () => { + // The terminal foundation memo relies on this: with no resolved folder host it passes + // `null` instead of the active id, so a worktree switch cannot change the projection. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const folderWorkspaces = [localFolder, runtimeFolder] + const worktrees = [localWorktree] + + const withoutActiveId = project({ worktrees, folderWorkspaces, activeWorkspaceId: null }) + for (const activeWorkspaceId of [ + 'folder:folder-shared', + 'folder:other-workspace', + SHARED_WORKTREE_ID + ]) { + expect(project({ worktrees, folderWorkspaces, activeWorkspaceId })).toEqual(withoutActiveId) + } + warn.mockRestore() + }) + it('keeps the first row when no resolved host disambiguates the folder collision', () => { const surfaces = project({ folderWorkspaces: [runtimeFolder, localFolder] diff --git a/src/renderer/src/components/workspace-surface-projection.ts b/src/renderer/src/components/workspace-surface-projection.ts index 4a3d28ea17f..9b0a5731be2 100644 --- a/src/renderer/src/components/workspace-surface-projection.ts +++ b/src/renderer/src/components/workspace-surface-projection.ts @@ -49,6 +49,7 @@ export function projectWorkspaceSurfaces({ }: { worktreesById: ReadonlyMap> folderWorkspaces: readonly FolderWorkspaceSurfaceRow[] + /** Read only when `activeWorkspaceResolvedHostId` is set; inert otherwise. */ activeWorkspaceId: string | null /** Resolved (not user-selected) host of the active workspace; the folder tie-break. */ activeWorkspaceResolvedHostId: ExecutionHostId | null