From 28258c0ba9466587d2374792db863f2bbd22613a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:08:08 -0700 Subject: [PATCH] fix(terminal): keep host-retired panes out of every layout a paired client writes (#33083) Exiting a paired host tab's original pane moved the host's active leaf to the surviving split. The client's reconciliation effect followed it with setActivePane, and that persisted every still-mounted pane, writing the retired leaf (whose remote PTY had not ended yet) back into the layout. The leaf was then no longer retired, so its pane stayed forever. Any other persist in that window (insertions, clicks, title edits, divider drags) could do the same. persistLayoutSnapshot now drops the leaves the host retired, using the retireLeavesFromTerminalLayout main already uses, moved to shared. The retired set it reads is the one the reconciliation effect tracks. --- .../terminal-topology/terminal-leaf-move.ts | 2 +- ...session-terminal-persistence-retirement.ts | 2 +- .../mobile-session-terminal-retirement.ts | 74 +------------------ .../terminal-layout-title-persist.test.tsx | 17 ++++- .../use-terminal-pane-layout-persistence.ts | 12 ++- .../use-terminal-pane-reconciliation.ts | 13 ++-- src/shared/terminal-layout-leaf-retirement.ts | 63 ++++++++++++++++ ...mote-split-pane-host-retired-ghost.spec.ts | 33 +++++++-- 8 files changed, 125 insertions(+), 91 deletions(-) create mode 100644 src/shared/terminal-layout-leaf-retirement.ts diff --git a/src/main/persistence/terminal-topology/terminal-leaf-move.ts b/src/main/persistence/terminal-topology/terminal-leaf-move.ts index a18f721fb18..54f6b4abbfd 100644 --- a/src/main/persistence/terminal-topology/terminal-leaf-move.ts +++ b/src/main/persistence/terminal-topology/terminal-leaf-move.ts @@ -8,7 +8,7 @@ import { } from '../../../shared/terminal-leaf-move' import type { TerminalLayoutSnapshot, TerminalTab } from '../../../shared/terminal-tab-types' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' -import { retireLeavesFromTerminalLayout } from '../../runtime/mobile-session-terminal-retirement' +import { retireLeavesFromTerminalLayout } from '../../../shared/terminal-layout-leaf-retirement' import { createMinimalPersistedTerminalTab } from '../restoring-sessions/session-owner-fields' import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' import { writeRestorable } from './restorable-write' diff --git a/src/main/runtime/mobile-session-terminal-persistence-retirement.ts b/src/main/runtime/mobile-session-terminal-persistence-retirement.ts index 25ed04c2f32..98c781c0510 100644 --- a/src/main/runtime/mobile-session-terminal-persistence-retirement.ts +++ b/src/main/runtime/mobile-session-terminal-persistence-retirement.ts @@ -3,9 +3,9 @@ import type { WorkspaceSessionState } from '../../shared/workspace-session-state import { pruneTabGroupLayoutAfterRetirement, repairMobileSessionTabGroupsAfterRetirement, - retireLeavesFromTerminalLayout, type RetiredTerminalSurface } from './mobile-session-terminal-retirement' +import { retireLeavesFromTerminalLayout } from '../../shared/terminal-layout-leaf-retirement' import { rebaseWorkspaceSessionTerminalMembership } from './workspace-session-terminal-membership-authority' import { advanceTerminalTopologyRevision } from '../persistence/terminal-topology/terminal-topology-membership' diff --git a/src/main/runtime/mobile-session-terminal-retirement.ts b/src/main/runtime/mobile-session-terminal-retirement.ts index cd9f8d2d3cb..f920506eec7 100644 --- a/src/main/runtime/mobile-session-terminal-retirement.ts +++ b/src/main/runtime/mobile-session-terminal-retirement.ts @@ -6,11 +6,8 @@ import type { RuntimeMobileSessionTerminalTab } from '../../shared/runtime-types' import type { TabGroupLayoutNode } from '../../shared/tab-types' -import type { - TerminalLayoutSnapshot, - TerminalPaneLayoutNode -} from '../../shared/terminal-tab-types' import { appendRetiredTerminalSurfaceProofs } from '../../shared/terminal-retirement-proof-ledger' +import { retireLeavesFromTerminalLayout } from '../../shared/terminal-layout-leaf-retirement' export type RetiredTerminalSurface = { worktreeId: string @@ -21,75 +18,6 @@ export type RetiredTerminalSurface = { retiredAt?: number } -function pruneTerminalPane( - node: TerminalPaneLayoutNode | null, - retiredLeafIds: ReadonlySet -): TerminalPaneLayoutNode | null { - if (!node) { - return null - } - if (node.type === 'leaf') { - return retiredLeafIds.has(node.leafId) ? null : node - } - const first = pruneTerminalPane(node.first, retiredLeafIds) - const second = pruneTerminalPane(node.second, retiredLeafIds) - if (first && second) { - return { ...node, first, second } - } - return first ?? second -} - -function collectTerminalLeafIds(node: TerminalPaneLayoutNode | null): string[] { - if (!node) { - return [] - } - return node.type === 'leaf' - ? [node.leafId] - : [...collectTerminalLeafIds(node.first), ...collectTerminalLeafIds(node.second)] -} - -function omitLeafRecords( - values: Record | undefined, - retiredLeafIds: ReadonlySet -): Record | undefined { - if (!values) { - return undefined - } - const retained = Object.fromEntries( - Object.entries(values).filter(([leafId]) => !retiredLeafIds.has(leafId)) - ) - return Object.keys(retained).length > 0 ? retained : undefined -} - -export function retireLeavesFromTerminalLayout( - layout: TerminalLayoutSnapshot, - retiredLeafIds: ReadonlySet -): TerminalLayoutSnapshot | null { - const root = pruneTerminalPane(layout.root, retiredLeafIds) - if (!root) { - return null - } - const retainedLeafIds = collectTerminalLeafIds(root) - const retainedLeafIdSet = new Set(retainedLeafIds) - const activeLeafId = - layout.activeLeafId && retainedLeafIdSet.has(layout.activeLeafId) - ? layout.activeLeafId - : retainedLeafIds[0]! - return { - ...layout, - root, - activeLeafId, - expandedLeafId: - layout.expandedLeafId && retainedLeafIdSet.has(layout.expandedLeafId) - ? layout.expandedLeafId - : null, - ptyIdsByLeafId: omitLeafRecords(layout.ptyIdsByLeafId, retiredLeafIds), - buffersByLeafId: omitLeafRecords(layout.buffersByLeafId, retiredLeafIds), - scrollbackRefsByLeafId: omitLeafRecords(layout.scrollbackRefsByLeafId, retiredLeafIds), - titlesByLeafId: omitLeafRecords(layout.titlesByLeafId, retiredLeafIds) - } -} - export function pruneTabGroupLayoutAfterRetirement( layout: TabGroupLayoutNode | undefined, retainedGroupIds: ReadonlySet diff --git a/src/renderer/src/components/terminal-pane/terminal-layout-title-persist.test.tsx b/src/renderer/src/components/terminal-pane/terminal-layout-title-persist.test.tsx index 85ade79752a..6262b0ce3a9 100644 --- a/src/renderer/src/components/terminal-pane/terminal-layout-title-persist.test.tsx +++ b/src/renderer/src/components/terminal-pane/terminal-layout-title-persist.test.tsx @@ -80,7 +80,7 @@ function renderPersistence() { controller as unknown as Parameters[0] ) ) - return { hook, push } + return { hook, push, setTabLayout } } afterEach(() => { @@ -148,3 +148,18 @@ describe('terminal layout title persist', () => { expect(push).not.toHaveBeenCalled() }) }) + +describe('terminal layout persist with host-retired panes', () => { + it('keeps a host-retired pane out of the layout it writes while that pane is still mounted', () => { + const { hook, push, setTabLayout } = renderPersistence() + hook.result.current.hostRetiredLeafIdsRef.current = new Set([RIGHT]) + + hook.result.current.changePaneTitle(1, 'build') + + const written = setTabLayout.mock.lastCall?.[1] + expect(written?.root).toEqual({ type: 'leaf', leafId: LEFT }) + expect(written?.activeLeafId).toBe(LEFT) + expect(written?.ptyIdsByLeafId).toEqual({ [LEFT]: 'remote:host:1' }) + expect(push.mock.lastCall?.[0].layout.root).toEqual({ type: 'leaf', leafId: LEFT }) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-layout-persistence.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-layout-persistence.ts index bafcc2d46bd..ba8f193f107 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-layout-persistence.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-layout-persistence.ts @@ -15,6 +15,7 @@ import type { TerminalPaneStartupController } from './use-terminal-pane-startup- import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../../shared/layout-owner-switch' import { sendLayoutCommandToOwner } from '@/runtime/layout-owner-command' import { notePendingPaneTitleRename } from '@/runtime/pending-pane-title-renames' +import { retireLeavesFromTerminalLayout } from '../../../../shared/terminal-layout-leaf-retirement' export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartupController) { const { @@ -37,6 +38,9 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup terminalTab, worktreeId } = controller + // Leaves a host-authoritative layout dropped whose panes are still mounted, kept out of every + // write so a persist cannot re-add one while its PTY is still ending. + const hostRetiredLeafIdsRef = useRef>(new Set()) const chatOwnerRef = useRef({ chatLeafId, isChatViewMode }) useLayoutEffect(() => { chatOwnerRef.current = { chatLeafId, isChatViewMode } @@ -50,7 +54,7 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup } const activePaneId = manager.getActivePane()?.id ?? manager.getPanes()[0]?.id ?? null const leafIdByPaneId = manager.getLeafIdMap() - const layout = serializeTerminalLayout( + let layout = serializeTerminalLayout( container, activePaneId, expandedPaneIdRef.current, @@ -125,6 +129,11 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup if (Object.keys(titlesByLeafId).length > 0) { layout.titlesByLeafId = titlesByLeafId } + const hostRetiredLeafIds = hostRetiredLeafIdsRef.current + if (hostRetiredLeafIds.size > 0) { + // Why: never retire the last pane here; the host drops a whole tab by itself. + layout = retireLeavesFromTerminalLayout(layout, hostRetiredLeafIds) ?? layout + } setTabLayout(tabId, layout) const hasRemotePane = Object.values(mergedPtyIds).some( (ptyId) => typeof ptyId === 'string' && isRemoteRuntimePtyId(ptyId) @@ -297,6 +306,7 @@ export function useTerminalPaneLayoutPersistence(controller: TerminalPaneStartup }, [paneCount, paneTitles, persistLayoutSnapshot, terminalTab]) return { + hostRetiredLeafIdsRef, persistLayoutSnapshot, clearPaneScrollback, changePaneTitle, diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-reconciliation.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-reconciliation.ts index e0932eec0a4..1dc48d66cf9 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-reconciliation.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-reconciliation.ts @@ -23,6 +23,7 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr closeTerminalLinkActions, containerRef, executeClosePane, + hostRetiredLeafIdsRef, isActive, isRendererVisible, isolatedPaneKey, @@ -36,11 +37,9 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr restoredLayout, tabId } = controller - // Leaves the last host-authoritative layout named, and the ones it has since - // dropped whose panes are still mounted; a removal needs the host to have - // named the leaf before it dropped it, and may have to wait for the PTY exit. + // Leaves the last host-authoritative layout named; a removal needs the host to + // have named the leaf before it dropped it, and may have to wait for the PTY exit. const hostLayoutLeafIdsRef = useRef>(new Set()) - const retiredLeafIdsRef = useRef>(new Set()) useEffect(() => { closeTerminalLinkActions() @@ -51,7 +50,9 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr if (!manager || !restoredLayout.root) { return } + // Why the retired check: dropping a retired leaf can drop the layout's only remote PTY id. if ( + hostRetiredLeafIdsRef.current.size === 0 && !isHostAuthoritativeLayout({ isWebClient: !!(globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__, ptyIdsByLeafId: restoredLayout.ptyIdsByLeafId @@ -62,13 +63,13 @@ export function useTerminalPaneReconciliation(controller: TerminalPaneCloseContr const layoutLeafIds = new Set(collectLeafIds(restoredLayout.root)) const mountedLeafIds = manager.getPanes().map((pane) => pane.leafId) const retiredLeafIds = trackRetiredLeafIds({ - retiredLeafIds: retiredLeafIdsRef.current, + retiredLeafIds: hostRetiredLeafIdsRef.current, previousLayoutLeafIds: hostLayoutLeafIdsRef.current, layoutLeafIds, mountedLeafIds }) hostLayoutLeafIdsRef.current = layoutLeafIds - retiredLeafIdsRef.current = retiredLeafIds + hostRetiredLeafIdsRef.current = retiredLeafIds const insertions = planTerminalLiveLayoutInsertions(restoredLayout.root, mountedLeafIds) const removals = planTerminalLiveLayoutRemovals( restoredLayout.root, diff --git a/src/shared/terminal-layout-leaf-retirement.ts b/src/shared/terminal-layout-leaf-retirement.ts new file mode 100644 index 00000000000..edef75b76f9 --- /dev/null +++ b/src/shared/terminal-layout-leaf-retirement.ts @@ -0,0 +1,63 @@ +import type { TerminalLayoutSnapshot, TerminalPaneLayoutNode } from './terminal-tab-types' +import { collectLeafIds } from './terminal-pane-layout-tree' + +function pruneTerminalPane( + node: TerminalPaneLayoutNode | null, + retiredLeafIds: ReadonlySet +): TerminalPaneLayoutNode | null { + if (!node) { + return null + } + if (node.type === 'leaf') { + return retiredLeafIds.has(node.leafId) ? null : node + } + const first = pruneTerminalPane(node.first, retiredLeafIds) + const second = pruneTerminalPane(node.second, retiredLeafIds) + if (first && second) { + return { ...node, first, second } + } + return first ?? second +} + +function omitLeafRecords( + values: Record | undefined, + retiredLeafIds: ReadonlySet +): Record | undefined { + if (!values) { + return undefined + } + const retained = Object.fromEntries( + Object.entries(values).filter(([leafId]) => !retiredLeafIds.has(leafId)) + ) + return Object.keys(retained).length > 0 ? retained : undefined +} + +/** Drops leaves and their per-leaf records; null when no leaf remains. */ +export function retireLeavesFromTerminalLayout( + layout: TerminalLayoutSnapshot, + retiredLeafIds: ReadonlySet +): TerminalLayoutSnapshot | null { + const root = pruneTerminalPane(layout.root, retiredLeafIds) + if (!root) { + return null + } + const retainedLeafIds = collectLeafIds(root) + const retainedLeafIdSet = new Set(retainedLeafIds) + const activeLeafId = + layout.activeLeafId && retainedLeafIdSet.has(layout.activeLeafId) + ? layout.activeLeafId + : retainedLeafIds[0]! + return { + ...layout, + root, + activeLeafId, + expandedLeafId: + layout.expandedLeafId && retainedLeafIdSet.has(layout.expandedLeafId) + ? layout.expandedLeafId + : null, + ptyIdsByLeafId: omitLeafRecords(layout.ptyIdsByLeafId, retiredLeafIds), + buffersByLeafId: omitLeafRecords(layout.buffersByLeafId, retiredLeafIds), + scrollbackRefsByLeafId: omitLeafRecords(layout.scrollbackRefsByLeafId, retiredLeafIds), + titlesByLeafId: omitLeafRecords(layout.titlesByLeafId, retiredLeafIds) + } +} diff --git a/tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts b/tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts index c5b4c69d8aa..77db32fb446 100644 --- a/tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts +++ b/tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts @@ -14,13 +14,15 @@ * no exit overlay and no restart control. The refutation-proof shape (verified * here) is that the client's store layout shrinks to one leaf while its DOM * keeps two panes. After the fix the client removes the retired pane and store - * + DOM agree at exactly the surviving leaf. + * + DOM agree at exactly the surviving leaf. Exiting the original pane also + * moves the host's active leaf; the client must not write the retired pane back + * into its layout while that PTY is still ending. * * Run: * pnpm exec playwright test tests/e2e/paired-remote-split-pane-host-retired-ghost.spec.ts \ * --config tests/playwright.config.ts --project electron-headless --workers=1 */ -import type { Page } from '@stablyai/playwright-test' +import type { Page, TestInfo } from '@stablyai/playwright-test' import { toWebTerminalSurfaceTabId } from '../../src/shared/terminal-surface-id' import { expect, test } from './helpers/orca-app' import { launchHeadlessPairedRuntimeHost } from './helpers/headless-paired-runtime-host' @@ -56,9 +58,11 @@ async function hostLayoutLeafIds(page: Page, webTabId: string): Promise { +async function expectHostRetiredPaneRemoved( + exitedPane: 'split' | 'original', + testRepoPath: string, + testInfo: TestInfo +): Promise { test.setTimeout(240_000) const host = await launchHeadlessPairedRuntimeHost() let client: Awaited> | null = null @@ -102,9 +106,13 @@ test('removes the pane a paired remote host retired instead of leaving a dead gh .toBe(true) const beforeExit = await mountedLeafPtyIds(client.page, webTabId) - // The user quits the second shell — the host retires that leaf and + // The user quits one shell — the host retires that leaf and // republishes a one-leaf layout. - await host.client.call('terminal.send', { terminal: secondHandle, text: 'exit', enter: true }) + await host.client.call('terminal.send', { + terminal: exitedPane === 'split' ? secondHandle : firstHandle, + text: 'exit', + enter: true + }) // The host-authoritative layout the client holds shrinks to one leaf // (confirms the retirement). This is the refutation-proof signal: before the @@ -139,4 +147,13 @@ test('removes the pane a paired remote host retired instead of leaving a dead gh await client?.dispose() await host.dispose() } -}) +} + +test('removes the pane a paired remote host retired instead of leaving a dead ghost', async ({ + testRepoPath +}, testInfo) => expectHostRetiredPaneRemoved('split', testRepoPath, testInfo)) + +// Why: the original pane is the host's active leaf, so its retirement also moves focus. +test('removes the original pane a paired remote host retired after it exits', async ({ + testRepoPath +}, testInfo) => expectHostRetiredPaneRemoved('original', testRepoPath, testInfo))