From 3f1fd2cf8a65c43540364c8dfe304070f96f6f47 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:12:00 -0700 Subject: [PATCH] refactor(terminal): drop the scrollback flag; the wipe was never justified here The previous commit added a host flag so the client could tell whether a recovery payload carried scrollback. It cannot: every payload reaching that branch is screen-only by construction. The ack-recovery emit serializes with scrollbackRows: 0 literally, and the untagged resync request carries no scrollback either. A payload that does carry scrollback arrives tagged, and the request branch never clears. So the flag was always false where it mattered. Delete it -- the wire field, the host helper, the parse, and the conditional -- and simply stop emitting the scrollback erase on this path. No wire change at all now. --- src/main/runtime/rpc/methods/terminal.ts | 16 -------------- ...runtime-terminal-frame-drop-resync.test.ts | 4 +--- .../remote-runtime-terminal-multiplexer.ts | 22 +++++++------------ .../runtime/runtime-terminal-stream.test.ts | 17 -------------- 4 files changed, 9 insertions(+), 50 deletions(-) diff --git a/src/main/runtime/rpc/methods/terminal.ts b/src/main/runtime/rpc/methods/terminal.ts index 349557c9245..69f4eb72190 100644 --- a/src/main/runtime/rpc/methods/terminal.ts +++ b/src/main/runtime/rpc/methods/terminal.ts @@ -101,17 +101,6 @@ type SnapshotFrameOptions = { pendingEscapeTailAnsi?: string /** Effective kitty flags proven at this frame's own `seq`. */ kittyKeyboardFlags?: number - /** - * Whether `data` carries scrollback as well as the visible screen. Desktop - * snapshots are serialized with `scrollbackRows: 0`, so a client that erases - * its own scrollback to apply one destroys history the frame cannot replace. - */ - scrollbackIncluded?: boolean -} - -/** Whether a serialized snapshot's payload carries scrollback, not just the visible screen. */ -function snapshotCarriesScrollback(snapshot: SerializedSnapshot): boolean { - return Boolean(snapshot && (snapshot.scrollbackRows > 0 || snapshot.scrollbackAnsi)) } type SerializedSnapshot = { @@ -671,9 +660,6 @@ function sendSnapshotFrames( ...(typeof options.seq === 'number' && options.kittyKeyboardFlags !== undefined ? { kittyKeyboardFlags: options.kittyKeyboardFlags } : {}), - // Why additive on the same rule: absence means "unknown", and a client - // that cannot read it keeps its existing behavior. - ...(options.scrollbackIncluded === true ? { scrollbackIncluded: true } : {}), truncated: options.truncated === true, truncatedByByteBudget: options.truncatedByByteBudget === true }) @@ -1868,7 +1854,6 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [ source: serialized.source, kittyKeyboardFlags: serialized.kittyKeyboardFlags, truncatedByByteBudget: serialized.truncatedByByteBudget, - scrollbackIncluded: snapshotCarriesScrollback(serialized), data: serialized.data } ) @@ -2362,7 +2347,6 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [ truncatedByByteBudget: serialized?.truncatedByByteBudget, // Why: no serializer answered, which is not proof the pane is empty — say so instead of passing off '' as the buffer. unavailable: serialized ? undefined : 'no-serializable-buffer', - scrollbackIncluded: snapshotCarriesScrollback(serialized), data: serialized?.data ?? '' }) } catch (error) { diff --git a/src/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.ts b/src/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.ts index 14a7a38eaec..84e9f180ca6 100644 --- a/src/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.ts +++ b/src/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.ts @@ -16,9 +16,7 @@ import { replaceRuntimeEnvironmentRevisions } from './runtime-environment-revisi // Why screen-only: a resync reply is serialized with `scrollbackRows: 0`, so it // carries the visible screen and nothing else. Erasing the client's scrollback -// (`\x1b[3J`) to apply it would destroy history the frame cannot put back. The -// host sets `scrollbackIncluded` when a payload does carry scrollback, and only -// then is the full clear correct. +// (`\x1b[3J`) to apply it would destroy history the frame cannot put back. const RECOVERY_SCREEN_CLEAR = '\x1b[2J\x1b[H' // Why: reproduces the silent frame-drop corruption. The server multiplex path diff --git a/src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts b/src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts index e1e78ef7bec..7394af885af 100644 --- a/src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts +++ b/src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts @@ -217,10 +217,6 @@ type RemoteRuntimeSnapshotInfo = { // must write it AFTER the replay reset so the next live chunk completes it // instead of rendering literally (#7329). pendingEscapeTailAnsi?: string - // Why: whether the payload carries scrollback as well as the screen. Absent - // from older hosts, which is why the recovery path treats false as "do not - // erase scrollback" rather than "there is none". - scrollbackIncluded?: boolean } type RemoteRuntimeSnapshotRequest = { @@ -908,19 +904,19 @@ class RemoteRuntimeTerminalMultiplexer { kittyKeyboardFlags: info?.kittyKeyboardFlags }) } else if (target === 'recovery') { - // Why: a recovery repaint must never destroy more than it replaces. - // `\x1b[3J` erases xterm's scrollback, but desktop snapshots are - // serialized with `scrollbackRows: 0`, so the frame cannot put that - // history back — clear the screen alone unless the host says this - // payload carries scrollback. An absent flag means an older host, - // which never sends scrollback on this path either. - const clearAnsi = info?.scrollbackIncluded ? '\x1b[2J\x1b[3J\x1b[H' : '\x1b[2J\x1b[H' + // Why the screen only, never `\x1b[3J`: that erases xterm's scrollback, + // and every payload reaching this branch is screen-only by + // construction — the ack-recovery emit serializes with + // `scrollbackRows: 0`, and the untagged resync request carries no + // scrollback either. Erasing history the frame cannot put back is + // silent data loss. A payload that DOES carry scrollback arrives + // tagged, and the request branch above never clears. // Why not an empty repaint: with no payload there is nothing to put // on screen, so applying one only blanks a pane the host could not // describe. A mid-escape tail still has to replay so the next live // chunk completes it instead of rendering literally (#7329). if (data) { - stream.callbacks.onSnapshot(`${clearAnsi}${data}`, { + stream.callbacks.onSnapshot(`\x1b[2J\x1b[H${data}`, { pendingEscapeTailAnsi: info?.pendingEscapeTailAnsi, seq: info?.seq, kittyKeyboardFlags: info?.kittyKeyboardFlags @@ -1565,7 +1561,6 @@ function decodeSnapshotInfo( requestId?: unknown truncated?: unknown unavailable?: unknown - scrollbackIncluded?: unknown pendingEscapeTailAnsi?: unknown kittyKeyboardFlags?: unknown }>(payload) @@ -1582,7 +1577,6 @@ function decodeSnapshotInfo( requestId: typeof raw.requestId === 'number' ? raw.requestId : undefined, truncated: raw.truncated === true, unavailable: parseTerminalSnapshotUnavailableReason(raw.unavailable), - scrollbackIncluded: raw.scrollbackIncluded === true, pendingEscapeTailAnsi: typeof raw.pendingEscapeTailAnsi === 'string' ? raw.pendingEscapeTailAnsi : undefined } diff --git a/src/renderer/src/runtime/runtime-terminal-stream.test.ts b/src/renderer/src/runtime/runtime-terminal-stream.test.ts index 51c1e7d2ca1..e113d8d693b 100644 --- a/src/renderer/src/runtime/runtime-terminal-stream.test.ts +++ b/src/renderer/src/runtime/runtime-terminal-stream.test.ts @@ -595,23 +595,6 @@ describe('remote runtime terminal multiplex ACK gate', () => { }) expect(onSubscribed).toHaveBeenCalledTimes(1) - // Why scrollback may be erased here: the host states the payload carries - // it, so the frame replaces the history it removes. - injectSnapshot( - { - kind: 'scrollback', - cols: 120, - rows: 40, - reason: 'ack-pending-overflow', - truncated: false, - scrollbackIncluded: true - }, - 'restored with history' - ) - expect(onSnapshot).toHaveBeenCalledWith(`\x1b[2J\x1b[3J\x1b[H${'restored with history'}`, { - pendingEscapeTailAnsi: undefined - }) - // Why nothing at all: this replaced the previous rule, which read an empty // payload as an authoritative blank and cleared on it. An empty payload is // the host failing to describe the pane, not proof the pane is empty, and