From a8dabe2cf8dca2696073b8a9a8ce58d58ff53ccd Mon Sep 17 00:00:00 2001 From: Brit Date: Thu, 28 May 2026 15:45:33 +0200 Subject: [PATCH] fix(tui): deferred native scrollback rebuilds --- .../src/modes/interactive-mode.ts | 1 + packages/tui/src/tui.ts | 54 ++++++++------ packages/tui/test/render-regressions.test.ts | 73 +++++++++++++++++-- packages/tui/test/virtual-terminal.ts | 13 ++++ 4 files changed, 111 insertions(+), 30 deletions(-) diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 93eac5099..b40dcb803 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -839,6 +839,7 @@ export class InteractiveMode implements InteractiveModeContext { this.#pendingSubmissionDispose = undefined; } this.editor.setText(""); + this.ui.refreshNativeScrollbackIfDirty(); this.ensureLoadingAnimation(); this.ui.requestRender(); return submission; diff --git a/packages/tui/src/tui.ts b/packages/tui/src/tui.ts index a438a0b14..861fa6556 100644 --- a/packages/tui/src/tui.ts +++ b/packages/tui/src/tui.ts @@ -236,8 +236,6 @@ export class Container implements Component { * - `initial`: first paint after `start()` — clear viewport, emit transcript. * - `sessionReplace`: caller asked for `{ clearScrollback: true }` on a forced * render — clear viewport, clear scrollback (outside multiplexers). - * - `historyRebuild`: width changed and offscreen rows changed — clear viewport - * and scrollback so terminal history rewraps at the new width. * - `viewportRepaint`: rewrite the visible viewport in place. If `appendFrom` * is set, emit those tail rows as scrollback growth first so streaming * output reaches terminal history before the corrected viewport is drawn. @@ -248,7 +246,6 @@ type RenderIntent = | { kind: "noop" } | { kind: "initial" } | { kind: "sessionReplace" } - | { kind: "historyRebuild" } | { kind: "viewportRepaint"; appendFrom?: number } | { kind: "shrink" } | { kind: "diff"; firstChanged: number; lastChanged: number; appendedLines: boolean }; @@ -290,6 +287,7 @@ export class TUI extends Container { // Set after a clear+full replay so the next insert-above-suffix frame does // not scroll replayed live chrome (status/editor) into fresh history. #suppressNextSuffixScroll = false; + #nativeScrollbackDirty = false; #fullRedrawCount = 0; #clearScrollbackOnNextRender = false; #hasEverRendered = false; @@ -627,6 +625,17 @@ export class TUI extends Container { this.terminal.stop(); } + /** + * Rebuild native terminal scrollback if live rendering deferred a history rewrite. + * Callers should only invoke this at checkpoints where the user is expected to be + * at the terminal bottom, such as after submitting a new prompt. + */ + refreshNativeScrollbackIfDirty(): boolean { + if (!this.#nativeScrollbackDirty) return false; + this.requestRender(true, { clearScrollback: true }); + return true; + } + requestRender(force = false, options?: RenderRequestOptions): void { if (force) { this.#clearScrollbackOnNextRender ||= options?.clearScrollback === true; @@ -1111,12 +1120,7 @@ export class TUI extends Container { return; case "sessionReplace": this.#clearScrollbackOnNextRender = false; - this.#emitFullPaint(lines, width, height, cursorPos, { - clearViewport: true, - clearScrollback: !isMultiplexerSession(), - }); - return; - case "historyRebuild": + this.#nativeScrollbackDirty = false; this.#emitFullPaint(lines, width, height, cursorPos, { clearViewport: true, clearScrollback: !isMultiplexerSession(), @@ -1149,9 +1153,10 @@ export class TUI extends Container { /** * Map the current frame onto a single render intent. Order matters: forced - * resets and session replacement short-circuit before any diff work, and - * width-changed-with-offscreen edits route to {@link "historyRebuild"} so - * terminal scrollback receives the new geometry. + * resets and session replacement short-circuit before any diff work. Frames + * that would require rewriting native scrollback mark it dirty and repaint + * the viewport instead; the destructive clear+replay is deferred to an + * explicit checkpoint. */ #planRender( newLines: string[], @@ -1177,9 +1182,9 @@ export class TUI extends Container { // Shrink-across-viewport-boundary: if a shrink would place the new // viewport above rows already committed to terminal scrollback, those - // rows would appear twice when the user scrolls back. A clear+replay - // keeps the current OMP transcript scrollable while dropping stale - // terminal history. + // rows can become stale or duplicated in native history. Preserve native + // scrollback for users reading it now, and defer the destructive + // clear+replay to the next checkpoint. const naturalViewportTop = Math.max(0, newLines.length - height); if ( diff.firstChanged !== -1 && @@ -1187,7 +1192,8 @@ export class TUI extends Container { naturalViewportTop < this.#scrollbackHighWater && !isMultiplexerSession() ) { - return { kind: "historyRebuild" }; + this.#nativeScrollbackDirty = true; + return { kind: "viewportRepaint" }; } const suppressSuffixScroll = this.#suppressNextSuffixScroll; @@ -1210,12 +1216,16 @@ export class TUI extends Container { return { kind: "noop" }; } - // Width changes alter wrapping for the whole transcript. Offscreen - // edits need a history rebuild so terminal scrollback receives the - // new geometry; pure appends fall through to the diff path so the - // append handler scrolls them into scrollback correctly. + // Width changes alter wrapping for the whole transcript. Offscreen edits + // make native history stale at the old geometry; mark it dirty and repaint + // only the viewport so users scrolled into history are not yanked mid-stream. + // Pure appends fall through to the diff path so the append handler scrolls + // them into history correctly. if (widthChanged) { - if (diff.firstChanged < prevViewportTop) return { kind: "historyRebuild" }; + if (diff.firstChanged < prevViewportTop) { + this.#nativeScrollbackDirty = true; + return { kind: "viewportRepaint" }; + } const pureAppend = diff.appendedLines && diff.firstChanged === this.#previousLines.length; if (!pureAppend) return { kind: "viewportRepaint" }; } @@ -1340,7 +1350,7 @@ export class TUI extends Container { /** * Clear the viewport (optionally scrollback) and emit the full transcript. - * Backs `initial`, `sessionReplace`, and `historyRebuild` intents. + * Backs `initial` and `sessionReplace` intents. */ #emitFullPaint( lines: string[], diff --git a/packages/tui/test/render-regressions.test.ts b/packages/tui/test/render-regressions.test.ts index 71e160096..a1f269270 100644 --- a/packages/tui/test/render-regressions.test.ts +++ b/packages/tui/test/render-regressions.test.ts @@ -545,7 +545,7 @@ describe("TUI terminal-state regressions", () => { } finally { tui.stop(); } - }); + }, 15_000); it("forced renders during resize storm stay stable under cursor relocation", async () => { const term = new VirtualTerminal(80, 18); const tui = new TUI(term); @@ -731,7 +731,7 @@ describe("TUI terminal-state regressions", () => { } finally { tui.stop(); } - }); + }, 15_000); it("keeps viewport aligned when offscreen header changes during overflow growth", async () => { const term = new VirtualTerminal(32, 6); @@ -849,12 +849,66 @@ describe("TUI terminal-state regressions", () => { } }); - it("tail-cell mutation after the transcript overflowed does not re-deposit header rows", async () => { - // Repro for the reported scrollback-duplication bug: once a header + it("defers stale-history rebuild while native scrollback is scrolled", async () => { + const term = new VirtualTerminal(32, 5); + const tui = new TUI(term); + const component = new MutableLinesComponent(rows("line-", 12)); + tui.addChild(component); + + try { + tui.start(); + await settle(term); + term.scrollLines(-2); + const before = term.getBufferPosition(); + expect(before.viewportY).toBeGreaterThan(0); + + component.setLines(rows("line-", 8)); + tui.requestRender(); + await settle(term); + + const after = term.getBufferPosition(); + expect(after.viewportY).toBe(before.viewportY); + expect(tui.refreshNativeScrollbackIfDirty()).toBe(true); + } finally { + tui.stop(); + } + }); + + it("refreshes deferred native scrollback at an explicit bottom checkpoint", async () => { + const term = new VirtualTerminal(32, 5); + const tui = new TUI(term); + const component = new MutableLinesComponent(rows("line-", 12)); + tui.addChild(component); + + try { + tui.start(); + await settle(term); + term.scrollLines(-2); + + component.setLines(rows("line-", 8)); + tui.requestRender(); + await settle(term); + + term.scrollLines(999); + expect(tui.refreshNativeScrollbackIfDirty()).toBe(true); + await settle(term); + + const position = term.getBufferPosition(); + expect(position.viewportY).toBe(position.baseY); + expect(visible(term).map(line => line.trim())).toEqual(["line-3", "line-4", "line-5", "line-6", "line-7"]); + expect(tui.refreshNativeScrollbackIfDirty()).toBe(false); + } finally { + tui.stop(); + } + }); + + it("tail-cell mutation is cleaned up at the next native scrollback checkpoint", async () => { + // Repro for the old scrollback-duplication bug: once a header // (e.g. the welcome screen) has scrolled into terminal history, the // last tool cell mutating (grow/shrink cycles, completion collapse) - // must not re-emit those header rows in a way that visibly - // duplicates them when the user scrolls back. + // makes native scrollback stale. Live frames now defer the destructive + // clear+replay until a user-run checkpoint rather than yanking users who + // are reading scrollback mid-stream. const term = new VirtualTerminal(40, 10); const tui = new TUI(term); const header = new MutableLinesComponent(["HEADER-0", "HEADER-1", "HEADER-2", "HEADER-3", "HEADER-4"]); @@ -892,11 +946,14 @@ describe("TUI terminal-state regressions", () => { // Final completion-style collapse: full transcript fits in the // viewport again, even though scrollback already holds an - // earlier copy of HEADER. + // earlier copy of HEADER. Rebuild at the next checkpoint to clean the + // stale native history. tail.setLines(["[completed: many lines]", "[footer]"]); tui.requestRender(); await settle(term); - + term.scrollLines(999); + expect(tui.refreshNativeScrollbackIfDirty()).toBe(true); + await settle(term); const scrollback = term.getScrollBuffer(); for (let i = 0; i < 5; i++) { const pattern = new RegExp(`\\bHEADER-${i}\\b`); diff --git a/packages/tui/test/virtual-terminal.ts b/packages/tui/test/virtual-terminal.ts index 155eea0f8..a76d76cd8 100644 --- a/packages/tui/test/virtual-terminal.ts +++ b/packages/tui/test/virtual-terminal.ts @@ -130,6 +130,19 @@ export class VirtualTerminal implements Terminal { this.inputHandler(data); } } + /** + * Simulate the user scrolling through native terminal scrollback. + * Negative values scroll up; positive values scroll down. + */ + scrollLines(lines: number): void { + this.xterm.scrollLines(lines); + } + + /** Get the terminal buffer's scrollback and viewport offsets. */ + getBufferPosition(): { baseY: number; viewportY: number } { + const buffer = this.xterm.buffer.active; + return { baseY: buffer.baseY, viewportY: buffer.viewportY }; + } /** * Resize the terminal