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))