From 018e38269ac0edcd48de149c8c40d83c71940ffd Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 17 Sep 2026 13:59:52 -0700 Subject: [PATCH] fix(terminal): keep a parked pane's scrollback across a reconnect merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The direct-SSH pull replaces a replaced tab's layout wholesale, and a park capture does not bump tab.generation — so a just-parked tab is not in locallyPreservedTabIds and the only client-side copy of its remote scrollback went with the layout it replaced. That is the same data loss this branch already fixes, one layer down, and it is the layer that decides whether the fix survives the app update the user actually performed. Carry the client's leaf-keyed scrollback into the host's layout, filtered to the host's own root leaves. Structure stays the host's verbatim, so a split it added while we were away still wins and a leaf it retired still drops its bytes. Local wins a conflict: neither copy is then the only one, but remote-wins would overwrite the tail captured since the last upload and propagate that backwards on the next replace-session patch. Not a generation bump: the pane key is `${tab.id}-${tab.generation}`, so bumping would remount the pane and destroy the very buffer the capture just serialized, lift the recovery-storm ledger ceiling, and let a stale local ptyId win through preserveNewerLocalTerminalFields. --- ...remote-layout-scrollback-retention.test.ts | 86 ++++++++++++ .../remote-layout-scrollback-retention.ts | 56 ++++++++ ...ce-session-merge-parked-scrollback.test.ts | 122 ++++++++++++++++++ .../hooks/remote-workspace-session-merge.ts | 13 +- 4 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.test.ts create mode 100644 src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.ts create mode 100644 src/renderer/src/hooks/remote-workspace-session-merge-parked-scrollback.test.ts diff --git a/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.test.ts b/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.test.ts new file mode 100644 index 00000000000..228c3f9b01a --- /dev/null +++ b/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest' +import type { TerminalLayoutSnapshot } from '../../../../shared/terminal-tab-types' +import { retainLocalScrollbackInRemoteLayout } from './remote-layout-scrollback-retention' + +const LEAF_A = 'leaf-a' +const LEAF_B = 'leaf-b' + +function layout(overrides: Partial = {}): TerminalLayoutSnapshot { + return { + root: { type: 'leaf', leafId: LEAF_A }, + activeLeafId: LEAF_A, + expandedLeafId: null, + ...overrides + } +} + +const SPLIT_ROOT: TerminalLayoutSnapshot['root'] = { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF_A }, + second: { type: 'leaf', leafId: LEAF_B } +} + +describe('retainLocalScrollbackInRemoteLayout', () => { + it('keeps a parked tab scrollback the host copy never had', () => { + const local = layout({ buffersByLeafId: { [LEAF_A]: 'parked-scrollback' } }) + + const merged = retainLocalScrollbackInRemoteLayout(local, layout()) + + expect(merged.buffersByLeafId).toEqual({ [LEAF_A]: 'parked-scrollback' }) + }) + + it('takes the host structure verbatim when the host added a split while we were away', () => { + const local = layout({ buffersByLeafId: { [LEAF_A]: 'parked-scrollback' } }) + const remote = layout({ root: SPLIT_ROOT, ptyIdsByLeafId: { [LEAF_B]: 'remote:env/pty-b' } }) + + const merged = retainLocalScrollbackInRemoteLayout(local, remote) + + expect(merged.root).toBe(SPLIT_ROOT) + expect(merged.ptyIdsByLeafId).toEqual({ [LEAF_B]: 'remote:env/pty-b' }) + // The shared leaf keeps its bytes; nothing is invented for the leaf the host just added. + expect(merged.buffersByLeafId).toEqual({ [LEAF_A]: 'parked-scrollback' }) + }) + + it('drops scrollback for a leaf the host retired', () => { + const local = layout({ + root: SPLIT_ROOT, + buffersByLeafId: { [LEAF_A]: 'kept', [LEAF_B]: 'retired-pane' } + }) + + const merged = retainLocalScrollbackInRemoteLayout(local, layout()) + + expect(merged.buffersByLeafId).toEqual({ [LEAF_A]: 'kept' }) + }) + + it('prefers the local copy when both sides hold bytes for one leaf', () => { + const local = layout({ buffersByLeafId: { [LEAF_A]: 'captured-since-last-upload' } }) + const remote = layout({ buffersByLeafId: { [LEAF_A]: 'older-upload' } }) + + const merged = retainLocalScrollbackInRemoteLayout(local, remote) + + expect(merged.buffersByLeafId).toEqual({ [LEAF_A]: 'captured-since-last-upload' }) + }) + + it('carries scrollback refs, which name client-local snapshot files only this client can read', () => { + const local = layout({ scrollbackRefsByLeafId: { [LEAF_A]: 'v1-abc' } }) + + const merged = retainLocalScrollbackInRemoteLayout(local, layout()) + + expect(merged.scrollbackRefsByLeafId).toEqual({ [LEAF_A]: 'v1-abc' }) + }) + + it('returns the host layout untouched when this client holds no scrollback', () => { + const remote = layout({ buffersByLeafId: { [LEAF_A]: 'from-another-client' } }) + + expect(retainLocalScrollbackInRemoteLayout(layout(), remote)).toBe(remote) + expect(retainLocalScrollbackInRemoteLayout(undefined, remote)).toBe(remote) + }) + + it('leaves a rootless host layout alone rather than guessing which leaves are live', () => { + const local = layout({ buffersByLeafId: { [LEAF_A]: 'parked-scrollback' } }) + const remote = layout({ root: null }) + + expect(retainLocalScrollbackInRemoteLayout(local, remote)).toBe(remote) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.ts b/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.ts new file mode 100644 index 00000000000..5146cc85be3 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/remote-layout-scrollback-retention.ts @@ -0,0 +1,56 @@ +import type { TerminalLayoutSnapshot } from '../../../../shared/terminal-tab-types' +import { collectLeafIdsInOrder } from './terminal-layout-leaf-ids' +import { mergeCapturedLeafState } from './merge-captured-leaf-state' + +/** + * Carry this client's captured scrollback into the host's layout for a replaced tab. + * + * Why: the pull replaces a replaced tab's layout wholesale, and a park capture does not bump + * `tab.generation`, so a just-parked tab is not in `locallyPreservedTabIds` and its + * `buffersByLeafId` — the only client-side copy of a remote pane's scrollback — goes with it. + * The host is authoritative for structure and never mints scrollback of its own: its copy is + * only ever some client's earlier upload. So take `root`, `ptyIdsByLeafId`, titles and the + * active/expanded leaves from the host verbatim, and let the client fill in content the host's + * copy lacks. Structure from the host, bytes from whoever still has them. + * + * Why local wins a conflict: neither copy is then the only one, so neither choice destroys + * evidence — but remote-wins would overwrite the tail captured since this client's last upload + * and then propagate that regression back on the next replace-session patch. + * + * Why filtered to the host's leaves: it keeps the client from resurrecting a leaf the host + * retired, and from contributing anything for a split the host added while we were away. + */ +export function retainLocalScrollbackInRemoteLayout( + local: TerminalLayoutSnapshot | undefined, + remote: TerminalLayoutSnapshot +): TerminalLayoutSnapshot { + if (!local?.buffersByLeafId && !local?.scrollbackRefsByLeafId) { + return remote + } + const currentLeafIds = new Set(collectLeafIdsInOrder(remote.root)) + if (currentLeafIds.size === 0) { + return remote + } + const buffersByLeafId = mergeCapturedLeafState({ + prior: remote.buffersByLeafId, + fresh: local.buffersByLeafId ?? {}, + currentLeafIds + }) + const scrollbackRefsByLeafId = mergeCapturedLeafState({ + prior: remote.scrollbackRefsByLeafId, + fresh: local.scrollbackRefsByLeafId ?? {}, + currentLeafIds + }) + const retained = { ...remote } + if (Object.keys(buffersByLeafId).length > 0) { + retained.buffersByLeafId = buffersByLeafId + } else { + delete retained.buffersByLeafId + } + if (Object.keys(scrollbackRefsByLeafId).length > 0) { + retained.scrollbackRefsByLeafId = scrollbackRefsByLeafId + } else { + delete retained.scrollbackRefsByLeafId + } + return retained +} diff --git a/src/renderer/src/hooks/remote-workspace-session-merge-parked-scrollback.test.ts b/src/renderer/src/hooks/remote-workspace-session-merge-parked-scrollback.test.ts new file mode 100644 index 00000000000..c1dd4f7579d --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-session-merge-parked-scrollback.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it } from 'vitest' + +import { mergeDirectSshRemoteWorkspaceSession } from './remote-workspace-session-merge' +import { getDefaultWorkspaceSession } from '../../../shared/constants' +import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { TerminalTab, TerminalLayoutSnapshot } from '../../../shared/terminal-tab-types' +import type { AppState } from '../store/types' + +/** + * A reconnect must not delete the only copy of a parked remote pane's scrollback. + * + * A remote-runtime pty's bytes never transit main, so the park capture in + * `terminalLayoutsByTabId[tabId].buffersByLeafId` is the client's only copy. The reconnect replaces + * a replaced tab's layout wholesale, and a park capture does not bump `tab.generation` — so the tab + * is not in `locallyPreservedTabIds` and, before this was fixed, its scrollback went with the + * layout. The host never mints scrollback of its own; its copy is only ever an earlier upload. + */ +const WORKTREE = 'repo-1::/home/user/remote-checkout' +const TAB = 'tab-parked' +const LEAF = 'leaf-1' + +// Why a complete tab rather than a cast partial: the merge reads `generation` to decide local +// preservation, so a fixture missing it would make the control pass for the wrong reason. +function terminalTab(overrides: Partial = {}): TerminalTab { + return { + id: TAB, + ptyId: null, + worktreeId: WORKTREE, + title: TAB, + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 0, + generation: 1, + ...overrides + } +} + +function layout(overrides: Partial = {}): TerminalLayoutSnapshot { + return { + root: { type: 'leaf', leafId: LEAF }, + activeLeafId: LEAF, + expandedLeafId: null, + ...overrides + } +} + +function sessionState(overrides: Partial = {}): WorkspaceSessionState { + return { + ...getDefaultWorkspaceSession(), + activeRepoId: 'repo-1', + activeWorktreeId: WORKTREE, + activeWorkspaceKey: worktreeWorkspaceKey(WORKTREE), + activeTabId: TAB, + ...overrides + } +} + +function merge( + current: WorkspaceSessionState, + remote: WorkspaceSessionState, + liveTabs: AppState['tabsByWorktree'] = {} +): WorkspaceSessionState { + return mergeDirectSshRemoteWorkspaceSession( + current, + remote, + new Set([WORKTREE]), + liveTabs, + new Set() + ) +} + +describe('direct-SSH reconnect merge: parked scrollback', () => { + it('keeps the park capture when the host copy predates it and no generation changed', () => { + const tab = terminalTab() + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [tab] }, + terminalLayoutsByTabId: { + [TAB]: layout({ buffersByLeafId: { [LEAF]: 'parked-scrollback' } }) + } + }) + // Same generation on both sides is the whole point: nothing marks this tab as locally newer. + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [tab] }, + terminalLayoutsByTabId: { [TAB]: layout() } + }) + + const merged = merge(current, remote, { [WORKTREE]: [tab] }) + + expect(merged.terminalLayoutsByTabId[TAB]?.buffersByLeafId).toEqual({ + [LEAF]: 'parked-scrollback' + }) + }) + + it('still lets the host own the layout structure it changed while we were away', () => { + const tab = terminalTab() + const splitRoot: TerminalLayoutSnapshot['root'] = { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF }, + second: { type: 'leaf', leafId: 'leaf-2' } + } + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [tab] }, + terminalLayoutsByTabId: { + [TAB]: layout({ buffersByLeafId: { [LEAF]: 'parked-scrollback' } }) + } + }) + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [tab] }, + terminalLayoutsByTabId: { [TAB]: layout({ root: splitRoot }) } + }) + + const merged = merge(current, remote, { [WORKTREE]: [tab] }) + + expect(merged.terminalLayoutsByTabId[TAB]?.root).toEqual(splitRoot) + expect(merged.terminalLayoutsByTabId[TAB]?.buffersByLeafId).toEqual({ + [LEAF]: 'parked-scrollback' + }) + }) +}) diff --git a/src/renderer/src/hooks/remote-workspace-session-merge.ts b/src/renderer/src/hooks/remote-workspace-session-merge.ts index f8c2c9dabf2..0baabb93031 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge.ts @@ -4,6 +4,7 @@ import { reconcileClosedTerminalTabTombstones } from '../../../shared/closed-ter import type { ExecutionHostId } from '../../../shared/execution-host' import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' import { splitWorktreeId } from '../../../shared/worktree/id' +import { retainLocalScrollbackInRemoteLayout } from '@/components/terminal-pane/remote-layout-scrollback-retention' import { getWorktreeIdFromHostIdentity, isWorktreeHostIdentity @@ -249,9 +250,15 @@ export function mergeDirectSshRemoteWorkspaceSession( ) ), ...Object.fromEntries( - Object.entries(remote.terminalLayoutsByTabId).filter( - ([tabId]) => !locallyPreservedTabIds.has(tabId) && !suppressedTabIds.has(tabId) - ) + Object.entries(remote.terminalLayoutsByTabId) + .filter(([tabId]) => !locallyPreservedTabIds.has(tabId) && !suppressedTabIds.has(tabId)) + // Why: this replace is wholesale, and a park capture does not bump tab.generation, so a + // just-parked tab is not locally preserved and the only client-side copy of its remote + // scrollback would go with its layout. Structure stays the host's. + .map(([tabId, layout]) => [ + tabId, + retainLocalScrollbackInRemoteLayout(current.terminalLayoutsByTabId[tabId], layout) + ]) ) } const activeOutsideTarget =