From 8d49ffb69e5f74e3ef6ccc8aa95319e14e59a7b7 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:20:27 -0700 Subject: [PATCH] fix: memoize terminal eviction exemptions on layout leaf PTYs Splits add leaf panes to the layout store without changing the tabs array. A memo keyed only on tabs misses this change, leaving new panes unexempted for unmount. Include layout leaf PTYs in the exemption memo key so it recalculates when splits occur or PTYs are re-minted. --- src/renderer/src/components/Terminal.tsx | 3 ++ .../terminal-pane/command-code-done-settle.ts | 5 +- .../ssh-reattach-model-restore.ts | 3 ++ .../terminal-eviction-exempt-tabs.test.ts | 49 +++++++++++++++++++ .../terminal-eviction-exempt-tabs.ts | 25 ++++++++++ .../use-terminal-tab-cold-parking.test.ts | 43 +++++++++++++++- .../use-terminal-tab-cold-parking.ts | 19 +++++-- 7 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.test.ts diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 6632ec63d35..d8b424bc5bf 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -1031,6 +1031,9 @@ function Terminal(): React.JSX.Element | null { capturedForceParked.delete(id) } } + // Why getState and not a dep: this read happens in the same pass that + // unmounts the panes, so it is the current catalog; re-running on a later + // repos change could not re-capture buffers whose panes are already gone. const repos = useAppStore.getState().repos const nextEvictionExemptTabIds = new Set() for (const worktreeId of forceParkedWorktreeIds) { diff --git a/src/renderer/src/components/terminal-pane/command-code-done-settle.ts b/src/renderer/src/components/terminal-pane/command-code-done-settle.ts index f7c207fb7e7..69a0082df2d 100644 --- a/src/renderer/src/components/terminal-pane/command-code-done-settle.ts +++ b/src/renderer/src/components/terminal-pane/command-code-done-settle.ts @@ -17,7 +17,10 @@ type CommandCodeDoneSettleExecutor = (normalizedPrompt: string) => void const executorByPaneKey = new Map() const timerByPaneKey = new Map>() -/** Registers the current writer for this pane; returns its release. */ +/** Registers the current writer for this pane; returns its release. + * Last registrant wins by design: park and reveal each hand the pane to a new + * owner while the predecessor is still registered, so refusing the overwrite + * would leave the row with an owner that can no longer write it. */ export function setCommandCodeDoneSettleExecutor( paneKey: string, execute: CommandCodeDoneSettleExecutor diff --git a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts index 1bcaa1ebf6a..c641332758c 100644 --- a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts +++ b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts @@ -54,6 +54,9 @@ export function decideSshReattachPaintSource(args: { } // Why the escape tail is excluded: a dangling mid-escape is not content — a // model holding only one paints a blank pane, which this gate forbids. + // Accepted cost: a session that wrote only control-sequence preamble before + // parking always falls back to relay; the gate cannot tell it apart from a + // model that never received content, and relay replays that preamble anyway. const contentLength = (args.snapshot.scrollbackAnsi?.length ?? 0) + args.snapshot.data.length return contentLength === 0 ? 'relay-replay' : 'main-model-snapshot' } diff --git a/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.test.ts b/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.test.ts new file mode 100644 index 00000000000..0560057b3dd --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('@/store', () => ({ + useAppStore: { getState: () => ({ terminalLayoutsByTabId: {}, runtimePaneTitlesByTabId: {} }) } +})) + +import { selectEvictionExemptTerminalTabLayoutKey } from './terminal-eviction-exempt-tabs' + +const TAB_ID = 'tab-1' +const LEAF_ID = '11111111-1111-4111-8111-111111111111' +const SECOND_LEAF_ID = '22222222-2222-4222-8222-222222222222' +const PTY_ID = 'repo::/worktree@@session-1' +const TABS = [{ id: TAB_ID, ptyId: PTY_ID }] + +function layoutState(ptyIdsByLeafId: Record) { + return { terminalLayoutsByTabId: { [TAB_ID]: { ptyIdsByLeafId } } } +} + +describe('selectEvictionExemptTerminalTabLayoutKey', () => { + // Why: the exempt memo is keyed on the tabs array, which a split leaves + // untouched — a key blind to layout PTYs would unmount the new live pane. + it('changes when a split adds a leaf pty', () => { + const single = layoutState({ [LEAF_ID]: PTY_ID }) + expect(selectEvictionExemptTerminalTabLayoutKey(single, TABS)).toBe( + selectEvictionExemptTerminalTabLayoutKey(single, TABS) + ) + expect( + selectEvictionExemptTerminalTabLayoutKey( + layoutState({ [LEAF_ID]: PTY_ID, [SECOND_LEAF_ID]: 'pty-local-detached' }), + TABS + ) + ).not.toBe(selectEvictionExemptTerminalTabLayoutKey(single, TABS)) + }) + + it('changes when a leaf pty is re-minted under the same layout', () => { + expect( + selectEvictionExemptTerminalTabLayoutKey( + layoutState({ [LEAF_ID]: 'pty-local-detached' }), + TABS + ) + ).not.toBe(selectEvictionExemptTerminalTabLayoutKey(layoutState({ [LEAF_ID]: PTY_ID }), TABS)) + }) + + it('tolerates tabs with no persisted layout', () => { + expect(selectEvictionExemptTerminalTabLayoutKey({ terminalLayoutsByTabId: {} }, TABS)).toBe( + `${TAB_ID}=` + ) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.ts b/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.ts index 242cab9bf65..3a11fab51fd 100644 --- a/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.ts +++ b/src/renderer/src/components/terminal-pane/terminal-eviction-exempt-tabs.ts @@ -60,3 +60,28 @@ export function selectEvictionExemptTerminalTabIds( } return exemptTabIds } + +type EvictionExemptTabLayoutState = { + terminalLayoutsByTabId: Record } | undefined> +} + +/** + * Memo key over the store half of an exemption verdict: a split add or a + * re-minted leaf pty changes the layout's pane PTYs without changing the tabs + * array, and a memo keyed on tabs alone would keep serving an exempt set that + * misses the new pane — force-park would unmount it and orphan a live shell. + */ +export function selectEvictionExemptTerminalTabLayoutKey( + state: EvictionExemptTabLayoutState, + tabs: readonly ParkableTerminalTabModel[] +): string { + return tabs + .map((tab) => { + const ptyIdsByLeafId = state.terminalLayoutsByTabId[tab.id]?.ptyIdsByLeafId ?? {} + const leafPtys = Object.entries(ptyIdsByLeafId) + .map(([leafId, ptyId]) => `${leafId}:${ptyId}`) + .join(',') + return `${tab.id}=${leafPtys}` + }) + .join('|') +} diff --git a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts index af7168ccb69..24ae41a7725 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts @@ -6,7 +6,8 @@ import type { TerminalTab } from '../../../../shared/types' const mocks = vi.hoisted(() => ({ storeState: { pendingStartupByTabId: {} as Record, - settings: {} as Record + settings: {} as Record, + terminalLayoutsByTabId: {} as Record }> }, exemptTabIds: new Set(), exemptSelectCalls: 0 @@ -20,7 +21,16 @@ vi.mock('./terminal-eviction-exempt-tabs', () => ({ selectEvictionExemptTerminalTabIds: (_worktreeId: string, tabs: readonly { id: string }[]) => { mocks.exemptSelectCalls += 1 return new Set(tabs.filter((tab) => mocks.exemptTabIds.has(tab.id)).map((tab) => tab.id)) - } + }, + selectEvictionExemptTerminalTabLayoutKey: ( + state: typeof mocks.storeState, + tabs: readonly { id: string }[] + ) => + tabs + .map( + (tab) => `${tab.id}=${JSON.stringify(state.terminalLayoutsByTabId[tab.id]?.ptyIdsByLeafId)}` + ) + .join('|') })) vi.mock('./terminal-parked-tab-watchers', () => ({ @@ -64,6 +74,7 @@ describe('useTerminalTabColdParking measure-clock contract', () => { vi.useRealTimers() mocks.exemptTabIds = new Set() mocks.exemptSelectCalls = 0 + mocks.storeState.terminalLayoutsByTabId = {} }) // Why: the worktree layer preserves hiddenSince through a background-measure @@ -154,6 +165,34 @@ describe('useTerminalTabColdParking measure-clock contract', () => { expect(result.current.parkedTerminalTabIds).toEqual(new Set(['tab-1', 'tab-2'])) }) + // Why: a split lands in the layout store, not in terminalTabs — a memo keyed + // on the tabs alone would keep serving a set that misses the new pane and + // unmount the live shell it should have exempted. + it('re-resolves exemptions when only the layout PTYs change', () => { + const stableArgs = { ...hookArgs(false), coldParkTerminalPanes: true, isForceParked: true } + const { result, rerender } = renderHook( + (args: typeof stableArgs) => useTerminalTabColdParking(args), + { initialProps: stableArgs } + ) + expect(result.current.parkedTerminalTabIds).toEqual(new Set(['tab-1', 'tab-2'])) + + // Same tabs, same verdict: nothing the memo can see has moved yet. + mocks.exemptTabIds = new Set(['tab-1']) + act(() => { + rerender(stableArgs) + }) + expect(result.current.parkedTerminalTabIds).toEqual(new Set(['tab-1', 'tab-2'])) + + // The split's leaf pty lands in the layout store and re-keys the memo. + mocks.storeState.terminalLayoutsByTabId = { + 'tab-1': { ptyIdsByLeafId: { 'leaf-2': 'pty-local-detached' } } + } + act(() => { + rerender(stableArgs) + }) + expect(result.current.parkedTerminalTabIds).toEqual(new Set(['tab-2'])) + }) + // Why: resolving an exemption re-reads the store and walks the layout tree per // tab, so an unrelated re-render must not repeat that work. it('resolves eviction exemptions once per force-park input change', () => { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts index 55db274b6ad..cbf128e7877 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts @@ -24,7 +24,10 @@ import { type ParkVerdictFlipRecord } from './terminal-park-verdict-flip-telemetry' import { getTerminalParkingPolicyOverrides } from './terminal-parking-e2e-overrides' -import { selectEvictionExemptTerminalTabIds } from './terminal-eviction-exempt-tabs' +import { + selectEvictionExemptTerminalTabIds, + selectEvictionExemptTerminalTabLayoutKey +} from './terminal-eviction-exempt-tabs' import { canWatcherCoverParkedTerminalTab, disposeParkedTerminalWatchersForWorktree, @@ -246,13 +249,21 @@ export function useTerminalTabColdParking(args: { worktreeId ]) + // Why subscribed: the exemption also reads layout leaf PTYs, which change + // without a terminalTabs change (split added, pty re-minted); gated on + // isForceParked so only force-parked worktrees build the key per store change. + const evictionExemptLayoutKey = useAppStore((state) => + isForceParked ? selectEvictionExemptTerminalTabLayoutKey(state, terminalTabs) : '' + ) // Why memoized: resolving an exemption re-reads the store and walks the - // layout tree per tab, so recompute only when the force-park verdict or the - // tabs change — not on every assignment/park-set change below. + // layout tree per tab, so recompute only when the force-park verdict, the + // tabs, or their layout PTYs change — not on every assignment/park-set change + // below. const evictionExemptTerminalTabIds = useMemo( () => isForceParked ? selectEvictionExemptTerminalTabIds(worktreeId, terminalTabs) : EMPTY_TAB_IDS, - [isForceParked, terminalTabs, worktreeId] + // eslint-disable-next-line react-hooks/exhaustive-deps -- the layout key encodes the store fields the selector re-reads internally. + [evictionExemptLayoutKey, isForceParked, terminalTabs, worktreeId] ) // Why: the rendered park verdict — worktree-level park (prop from