From f210dece83c1379cbc33eb8c3e40c2b74c500b29 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 17 Sep 2026 16:34:13 -0700 Subject: [PATCH] fix(terminal): carry a parked pane's scrollback through the mirrored-layout rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found in review of this PR by rc-ssh-remoting. chooseRemoteTerminalLayout rebuilds a mirrored tab's layout from the host's picture and never carried buffersByLeafId or scrollbackRefsByLeafId forward, though it already receives existingLayout. The host publishes no scrollback of its own, so ANY session-inventory frame landing between park and reveal dropped the only client-side copy: the rebuild is bufferless, terminalLayoutEqual compares buffers so the write is not bailed out, and apply-terminal-records assigns it wholesale. Measured before the fix: 336 bytes captured at park, 0 after one forced frame, blank pane on reveal. After: 411 bytes survive the frame and the reveal repaints. The e2e passed either way because no frame happened to land in its window, so it was not covering the destroying event. It now forces one inside the park -> reveal window and asserts the capture survives it. An identical fix was written and reverted earlier in this branch as 'no measurable effect' — that measurement ran on a harness deleting the client profile between launches, so nothing downstream of persistence could register. It was never actually tested. --- .../terminal-surfaces.ts | 11 +++++- ...erminal-parked-scrollback-survives.spec.ts | 39 +++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/runtime/web-session-tabs-sync/terminal-surfaces.ts b/src/renderer/src/runtime/web-session-tabs-sync/terminal-surfaces.ts index 78169094429..36cea9ad0eb 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/terminal-surfaces.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/terminal-surfaces.ts @@ -6,6 +6,7 @@ import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/ter import { defaultAgentChatLabel } from '../../../../shared/agent-session-chat-label' import { sanitizeTerminalLayoutPaneTitlesForLabels } from '@/lib/terminal-pane-title-sanitization' import { resolveTerminalLayoutRoot } from '../remote-terminal-layout-resolution' +import { retainLocalScrollbackInRemoteLayout } from '@/components/terminal-pane/remote-layout-scrollback-retention' import { getRemoteRuntimePtyEnvironmentId } from '../runtime-terminal-stream' import { HOST_TERMINAL_SURFACE_SEPARATOR, @@ -200,7 +201,13 @@ export function chooseRemoteTerminalLayout( : parentLayout?.expandedLeafId && knownLeafIds.has(parentLayout.expandedLeafId) ? parentLayout.expandedLeafId : null - return { + // Why retained: this rebuilds the layout from the host's picture, and the host publishes no + // scrollback of its own — a parked remote pane's bytes live only in the client's copy. Without + // this, ANY inventory frame landing between park and reveal drops the only copy: the rebuild is + // bufferless, terminalLayoutEqual compares buffers so the write is not bailed out, and + // apply-terminal-records assigns it wholesale. Structure still comes from the host; only bytes + // for leaves the host itself names are carried over. + return retainLocalScrollbackInRemoteLayout(existingLayout, { // Why: host parentLayout is authoritative for split direction; else keep the prior client tree, then degenerate — never re-guess a direction. root: resolveTerminalLayoutRoot({ authoritativeRoot: parentLayout?.root, @@ -216,7 +223,7 @@ export function chooseRemoteTerminalLayout( ptyIdsByLeafId, // Why: surface.title is the tab/PTY label, not a pane title; restoring it as one renders a fake title bar. Only host layout titles are real pane titles. ...(parentLayout?.titlesByLeafId ? { titlesByLeafId: parentLayout.titlesByLeafId } : {}) - } + }) } export function shouldReplaceTerminalTab( diff --git a/tests/e2e/paired-remote-terminal-parked-scrollback-survives.spec.ts b/tests/e2e/paired-remote-terminal-parked-scrollback-survives.spec.ts index 094077f63de..389eddc9b22 100644 --- a/tests/e2e/paired-remote-terminal-parked-scrollback-survives.spec.ts +++ b/tests/e2e/paired-remote-terminal-parked-scrollback-survives.spec.ts @@ -174,6 +174,45 @@ async function runParkRevealScenario( ) console.log(`[parked-scrollback] park-diagnostics ${JSON.stringify(parkDiagnostics)}`) + // Why a forced inventory frame: without one this spec passes whether or not the mirrored-layout + // rebuild carries the capture, because no frame happens to land in its window. A host frame + // rebuilds the tab's layout bufferless; terminalLayoutEqual compares buffers, so the write is + // not bailed out and apply-terminal-records assigns it wholesale. That wipes the only + // client-side copy. This is the destroying event, so it belongs inside the window under test. + const probeTab = await createPairedHostTerminal( + clientPage, + environmentId, + worktreeId, + fixtureCommand() + ) + createdTerminals.push(probeTab.terminal) + await expect + .poll( + () => + clientPage.evaluate( + (id) => (window.__store?.getState().tabsByWorktree[id] ?? []).map((tab) => tab.id), + worktreeId + ), + { + timeout: 60_000, + message: 'client never mirrored the probe tab (no inventory frame landed)' + } + ) + .toContain(probeTab.webTabId) + const afterInventoryFrame = await clientPage.evaluate((webTabId) => { + const layout = window.__store?.getState().terminalLayoutsByTabId?.[webTabId] + return { + leafIds: Object.keys(layout?.buffersByLeafId ?? {}), + length: Object.values(layout?.buffersByLeafId ?? {}).join('').length, + layoutKnown: layout !== undefined + } + }, target.webTabId) + console.log(`[parked-scrollback] after-inventory-frame ${JSON.stringify(afterInventoryFrame)}`) + expect( + { survivedInventoryFrame: afterInventoryFrame.length > 0 }, + 'a host inventory frame wiped the park capture before the reveal' + ).toEqual({ survivedInventoryFrame: true }) + await openPairedClientTab(clientPage, worktreeId, target.webTabId) const tokenAfterReveal = await waitForPairedPaneMarker( clientPage,