diff --git a/.changeset/cli-scrollback-dup.md b/.changeset/cli-scrollback-dup.md new file mode 100644 index 000000000..55770f63d --- /dev/null +++ b/.changeset/cli-scrollback-dup.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix duplicated transcript content appearing in scrollback during streaming. diff --git a/.changeset/pi-tui-scrollback-dup.md b/.changeset/pi-tui-scrollback-dup.md new file mode 100644 index 000000000..22ab5ae25 --- /dev/null +++ b/.changeset/pi-tui-scrollback-dup.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/pi-tui": patch +--- + +Pin the viewport anchor on partial shrinks and repaint above-viewport shifts in place, so streaming shrink/grow cycles no longer stack duplicate copies of content in scrollback; only a collapse past the viewport top re-anchors the view. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ebbc4897f..4611d7f5f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,24 @@ jobs: - run: pnpm install --frozen-lockfile - run: pnpm run test + # pi-tui's suite runs on node:test (not vitest), so the root `pnpm run test` + # does not execute it; it needs its own job. + test-pi-tui: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - uses: pnpm/action-setup@v6 + + - uses: actions/setup-node@v6 + with: + node-version-file: .nvmrc + cache: pnpm + + - run: pnpm install --frozen-lockfile + - run: pnpm --filter @moonshot-ai/pi-tui test + test-windows: runs-on: windows-latest # Temporarily disabled while Windows tests are being stabilized. diff --git a/packages/pi-tui/AGENTS.md b/packages/pi-tui/AGENTS.md index c5b5fdd8f..924f5f13c 100644 --- a/packages/pi-tui/AGENTS.md +++ b/packages/pi-tui/AGENTS.md @@ -17,5 +17,6 @@ Never overwrite this directory wholesale when syncing from upstream. Each of the ## Testing -- This package's tests run with `node --test` (`pnpm --filter @moonshot-ai/pi-tui test`), not vitest; the root `vitest run` does not execute them. +- This package's tests run with `node --test` (`pnpm --filter @moonshot-ai/pi-tui test`), not vitest; the root `vitest run` does not execute them — CI covers them through the dedicated `test-pi-tui` job in `.github/workflows/ci.yml`. The `test` script runs both `test/*.test.ts` and `e2e/*.test.ts`; `test:e2e` runs the e2e cases alone. - Prefer adding new narrow-width tests to the existing test file of the corresponding component. +- `e2e/` is a rendering-bug ledger: one case file per production rendering bug, driven end to end through the xterm-emulated `VirtualTerminal` (screen, scrollback, scroll position). When a rendering bug is found, add a `caseNN-*.test.ts` repro there and register it in `e2e/README.md`; keep it as a permanent regression guard after the fix. See `e2e/README.md` for the renderer invariants these cases assert. diff --git a/packages/pi-tui/e2e/README.md b/packages/pi-tui/e2e/README.md new file mode 100644 index 000000000..3c07a2440 --- /dev/null +++ b/packages/pi-tui/e2e/README.md @@ -0,0 +1,71 @@ +# pi-tui rendering e2e cases + +This folder is a **bug ledger**: one test file per rendering bug we hit in +production, driving the real `TUI` against an xterm-emulated terminal +(`test/virtual-terminal.ts`, backed by `@xterm/headless`). Each case +reproduces the original failure end to end — screen contents, scrollback, +scroll position — and stays as a permanent regression guard. + +Run with: + +```sh +pnpm --filter @moonshot-ai/pi-tui test:e2e # e2e cases only +pnpm --filter @moonshot-ai/pi-tui test # unit tests + e2e cases +``` + +## Background: the rendering model + +`TUI.doRender()` renders differentially against a logical line buffer. +`previousViewportTop` anchors which logical line sits on the top screen row. +Terminal scrollback is **append-only**: rows enter it only by scrolling off +the top of the screen, and can never be partially rewritten or truncated. +That physical constraint drives three invariants the renderer must hold: + +1. **Monotonic anchor** — `previousViewportTop` never rewinds (except on a + collapse, where the content ends at or above the screen top). Rewinding + repaints rows scrollback already holds; the next scroll commits them + again, duplicating the span. +2. **Exactly-once commit** — every row that crosses the viewport top must be + scrolled into scrollback exactly once: zero times loses the row, twice + duplicates it. +3. **Cursor bookkeeping sync** — the internal `hardwareCursorRow` must equal + the real terminal cursor row at all times; the differential path computes + relative moves from it, so any desync writes rows to the wrong place. + +Accepted trade-offs (by design, not bugs): + +- **Stale bytes**: content that changes above the viewport is not repainted + into scrollback; scrollback keeps the old version. This avoids `ESC[3J`, + which yanks the scroll position on Windows Terminal (microsoft/Terminal#20370). +- **Collapse seam**: a collapse rewind re-commits up to one screen of rows; + the content changed so drastically there that the seam is not recognizable. +- **Pinned gap**: a partial shrink keeps the anchor pinned, leaving a bounded + blank gap below the input box that the next growth refills. + +## Case index + +| Case | Symptom | Root cause | Status | +| --- | --- | --- | --- | +| [case01](./case01-collapse-blank-screen.test.ts) | Screen mostly blank after a large shrink (compaction), input box gone | Clamped differential path desynced the cursor when content collapsed above the viewport | Fixed (#1315) | +| [case02](./case02-collapse-scroll-position.test.ts) | Scroll position yanked to the top while reading scrollback (Windows Terminal) | Destructive full redraw emitted `ESC[3J` | Fixed (#1315) | +| [case03](./case03-full-redraw-residue.test.ts) | Stale text above the welcome banner after `/clear`; duplicated rows after ctrl+o expand | App-level session reset / expansion toggled content wholesale without a forced full redraw | Fixed (#1315, app layer uses `requestRender(true)`) | +| [case04](./case04-oscillation-duplication.test.ts) | Transcript spans duplicated in scrollback during streaming (shrink/grow oscillation) | Shrink re-anchor rewound the viewport anchor; the next grow re-committed rows scrollback already held | Fixed (#1353) | +| [case05](./case05-growth-past-anchor-row-loss.test.ts) | Transcript rows missing from scrollback after streaming | Growth past the anchor with an above-viewport change repainted in place without scrolling, so the skipped rows never got committed (invariant 2) | Fixed (#1353, review) | +| [case06](./case06-cursor-above-pinned-window.test.ts) | Rows written to the wrong screen position after a pinned shrink | `repaintViewport()` positioned the cursor to a logical row above the painted window; `hardwareCursorRow` desynced from the real cursor (invariant 3) | Fixed (#1353, review) | + +A case for a not-yet-fixed bug is marked **Open** in the table and is +expected to fail — it is the executable reproduction. Flip it to Fixed +when it turns green. + +Related unit-level guards (not duplicated here): kitty images straddling the +viewport top during a collapse are covered in `test/tui-shrink.test.ts`. + +## Adding a case + +1. Reproduce the bug with `createHarness()` from [harness.ts](./harness.ts): + drive frames with `frame(lines)`, then assert on `getViewport()`, + `getScrollBuffer()`, `getScrollPosition()`, or captured writes. +2. Name the file `caseNN-short-slug.test.ts` and add a row to the table + above: symptom as the user saw it, root cause, and which invariant broke. +3. Prefer unique per-line markers (`row-42`) so exactly-once assertions + cannot be confused by stale bytes. diff --git a/packages/pi-tui/e2e/case01-collapse-blank-screen.test.ts b/packages/pi-tui/e2e/case01-collapse-blank-screen.test.ts new file mode 100644 index 000000000..602d468c7 --- /dev/null +++ b/packages/pi-tui/e2e/case01-collapse-blank-screen.test.ts @@ -0,0 +1,59 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { createHarness } from "./harness.ts"; + +// Case 01 — blank screen / missing input box after a large shrink. +// +// Symptom: after context compaction (or any event collapsing the transcript +// far above the viewport top), the screen went mostly blank and the input +// box disappeared; typing produced no visible feedback. +// +// Root cause: the differential path clamped the first changed line to the +// viewport but then rendered as if nothing had shifted, desyncing the +// cursor. Fixed in #1315 by re-anchoring the viewport to the tail of the +// collapsed content (repaintViewport). + +describe("e2e case01: collapse must keep the input box on screen", () => { + it("shows the content tail and input box after a 60 -> 8 line collapse", async () => { + const h = await createHarness([...Array.from({ length: 59 }, (_, i) => `old-${i}`), "[INPUT-BOX]"], { + rows: 10, + }); + + // Collapse far above the viewport top (anchor is at 50) with a + // changed line above it — the original failure combination. + await h.frame(["new-0", "new-1", "new-2", "new-3", "new-4", "new-5", "new-6", "[INPUT-BOX]"]); + + const viewport = h.terminal.getViewport(); + assert.ok( + viewport.some((line) => line.includes("[INPUT-BOX]")), + `input box must stay visible, got: ${JSON.stringify(viewport)}`, + ); + assert.ok( + viewport.some((line) => line.includes("new-0")), + "collapsed content must be visible", + ); + + // Self-healing: the next differential frame must land in place. + await h.frame(["new-0", "new-1", "new-2", "new-3", "new-4", "new-5", "new-6", "[INPUT-BOX]x"]); + assert.ok( + h.terminal.getViewport().some((line) => line.includes("[INPUT-BOX]x")), + "frame after the collapse must update the input box in place", + ); + + h.stop(); + }); + + it("re-anchors to the tail when collapsed content is still taller than the screen", async () => { + const h = await createHarness([...Array.from({ length: 99 }, (_, i) => `old-${i}`), "[INPUT-BOX]"], { + rows: 10, + }); + + await h.frame([...Array.from({ length: 29 }, (_, i) => `new-${i}`), "[INPUT-BOX]"]); + + const viewport = h.terminal.getViewport(); + assert.ok(viewport[9]!.includes("[INPUT-BOX]"), `input box must sit on the bottom row, got: ${JSON.stringify(viewport)}`); + assert.ok(viewport[0]!.includes("new-20"), "viewport must show the tail of the new content"); + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/case02-collapse-scroll-position.test.ts b/packages/pi-tui/e2e/case02-collapse-scroll-position.test.ts new file mode 100644 index 000000000..e403818e2 --- /dev/null +++ b/packages/pi-tui/e2e/case02-collapse-scroll-position.test.ts @@ -0,0 +1,53 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { createHarness, type WriteCapturingTerminal } from "./harness.ts"; + +// Case 02 — scroll position yanked while reading scrollback. +// +// Symptom: on Windows Terminal, when the transcript collapsed while the +// user was scrolled up reading history, the view jumped to the absolute +// top of scrollback (microsoft/Terminal#20370: ESC[3J while scrolled +// into scrollback). +// +// Root cause: the fallback was a destructive full redraw that cleared +// scrollback with ESC[3J. Fixed in #1315: the collapse repaints only the +// live screen area, never emits ESC[3J, and leaves the scroll position +// alone. + +describe("e2e case02: collapse must not yank the user's scroll position", () => { + it("preserves scroll position and never emits ESC[3J during a collapse", async () => { + const h = await createHarness([...Array.from({ length: 59 }, (_, i) => `old-${i}`), "[INPUT-BOX]"], { + rows: 10, + capture: true, + }); + const terminal = h.terminal as WriteCapturingTerminal; + + // User scrolls up into scrollback and starts reading. + terminal.scrollViewport(-20); + const scrolledPosition = terminal.getScrollPosition(); + assert.ok(terminal.getViewport().some((line) => line.includes("old-35"))); + + terminal.writes = []; + await h.frame(["new-0", "new-1", "new-2", "new-3", "new-4", "new-5", "new-6", "[INPUT-BOX]"]); + + assert.strictEqual(terminal.getScrollPosition(), scrolledPosition, "scroll position must be preserved"); + assert.ok( + terminal.getViewport().some((line) => line.includes("old-35")), + "the user's scrolled view must still show the history", + ); + assert.ok( + !terminal.writes.join("").includes("\x1b[3J"), + "a collapse must never clear scrollback with ESC[3J", + ); + + // Scrolling back down reveals the fresh content. + terminal.scrollToBottom(); + const viewport = terminal.getViewport(); + assert.ok( + viewport.some((line) => line.includes("[INPUT-BOX]")), + `input box must be at the bottom after scrolling down, got: ${JSON.stringify(viewport)}`, + ); + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/case03-full-redraw-residue.test.ts b/packages/pi-tui/e2e/case03-full-redraw-residue.test.ts new file mode 100644 index 000000000..004933b01 --- /dev/null +++ b/packages/pi-tui/e2e/case03-full-redraw-residue.test.ts @@ -0,0 +1,70 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { countInBuffer, createHarness } from "./harness.ts"; + +// Case 03 — stale text above the welcome banner; duplicated rows after +// expansion toggles. +// +// Symptom (a): after /new, /clear, or a session switch, the new session's +// welcome banner rendered with the previous session's text still visible +// above it. +// Symptom (b): pressing ctrl+o to expand collapsed tool output rendered a +// duplicated copy of earlier content above the transcript. +// +// Root cause: both flows replace the content wholesale, which the +// differential renderer cannot reconcile against an unrelated previous +// frame. Fixed in #1315 at the app layer: kimi-code calls +// `requestRender(true)` on session reset and on expansion toggles. This +// case guards the TUI primitive those fixes rely on: a forced render +// repaints the screen from scratch, leaving no residue and no duplicates +// in the visible viewport. + +describe("e2e case03: forced full redraw leaves no residue on screen", () => { + it("clears previous-session text from the screen on a forced render (/clear)", async () => { + const h = await createHarness([...Array.from({ length: 59 }, (_, i) => `old-${i}`), "[INPUT-BOX]"], { + rows: 10, + }); + + // Session switch: unrelated short content + forced full redraw. + await h.frame(["[WELCOME]", "", "[INPUT-BOX]"], true); + + const viewport = h.terminal.getViewport(); + assert.ok(viewport.some((line) => line.includes("[WELCOME]")), "welcome banner must be visible"); + assert.ok(viewport.some((line) => line.includes("[INPUT-BOX]")), "input box must be visible"); + assert.ok( + !viewport.some((line) => line.includes("old-")), + `no previous-session text may remain on screen, got: ${JSON.stringify(viewport)}`, + ); + + h.stop(); + }); + + it("renders expanded content exactly once on a forced render (ctrl+o)", async () => { + const collapsed = [ + ...Array.from({ length: 40 }, (_, i) => `line-${i}`), + "... (100 more lines, ctrl+o to expand)", + "[INPUT-BOX]", + ]; + const h = await createHarness(collapsed, { rows: 10 }); + + // Expansion regrows the transcript wholesale + forced full redraw. + const expanded = [ + ...Array.from({ length: 40 }, (_, i) => `line-${i}`), + ...Array.from({ length: 20 }, (_, i) => `detail-${i}`), + "[INPUT-BOX]", + ]; + await h.frame(expanded, true); + + const viewport = h.terminal.getViewport(); + assert.ok(viewport[9]!.includes("[INPUT-BOX]"), "input box must sit on the bottom row"); + for (const marker of ["detail-12", "detail-19"]) { + assert.strictEqual( + countInBuffer(viewport, marker), + 1, + `"${marker}" must appear exactly once in the viewport`, + ); + } + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/case04-oscillation-duplication.test.ts b/packages/pi-tui/e2e/case04-oscillation-duplication.test.ts new file mode 100644 index 000000000..8931242f4 --- /dev/null +++ b/packages/pi-tui/e2e/case04-oscillation-duplication.test.ts @@ -0,0 +1,62 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { assertExactlyOnce, createHarness } from "./harness.ts"; + +// Case 04 — transcript spans duplicated in scrollback during streaming. +// +// Symptom: while an agent streamed output, scrolling up showed the same +// span of transcript (activity lines, messages) repeated over and over; +// each shrink/grow oscillation of the live region stacked another copy. +// +// Root cause: a partial shrink re-anchored (rewound) previousViewportTop, +// repainting rows scrollback already held; the next growth scrolled them +// out again. Fixed in #1353: the anchor never rewinds on a partial shrink +// (it stays pinned; growth refills the gap), so every row is committed to +// scrollback exactly once. + +describe("e2e case04: shrink/grow oscillation must not duplicate scrollback", () => { + const transcript = (status: string, activity: string[]): string[] => [ + ...Array.from({ length: 30 }, (_, i) => `msg-${i}`), + status, + ...Array.from({ length: 25 }, (_, i) => `out-${i}`), + ...activity, + "[INPUT-BOX]", + ]; + const fullActivity = ["act-0", "act-1", "act-2", "act-3"]; + + it("keeps every row exactly once across repeated oscillations", async () => { + const h = await createHarness(transcript("status-0", fullActivity), { rows: 10 }); + + // Three oscillations: the activity pane collapses and regrows while + // a status line above the viewport keeps changing (streaming reflow). + for (let cycle = 1; cycle <= 3; cycle++) { + await h.frame(transcript(`status-${cycle}a`, [])); + await h.frame(transcript(`status-${cycle}b`, fullActivity)); + } + + // Stable rows must appear exactly once — no duplicated spans, no + // losses. (Status lines are excluded: they legitimately leave stale + // copies above the viewport.) + assertExactlyOnce(h.terminal, ["msg-29", "out-0", "out-13", "out-24", "act-0", "act-3", "[INPUT-BOX]"]); + + const viewport = h.terminal.getViewport(); + assert.ok(viewport[9]!.includes("[INPUT-BOX]"), "input box must be back on the bottom row"); + + h.stop(); + }); + + it("keeps the pinned gap bounded and refills it on growth", async () => { + const h = await createHarness(transcript("status-0", fullActivity), { rows: 10 }); + + await h.frame(transcript("status-0", [])); + const pinned = h.terminal.getViewport(); + const inputRow = pinned.findIndex((line) => line.includes("[INPUT-BOX]")); + assert.ok(inputRow >= 0 && inputRow < 9, "input box hovers above a bounded blank gap while pinned"); + assert.strictEqual(pinned[9], "", "rows below the pinned content stay blank"); + + await h.frame(transcript("status-0", fullActivity)); + assert.ok(h.terminal.getViewport()[9]!.includes("[INPUT-BOX]"), "growth refills the gap to the bottom row"); + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/case05-growth-past-anchor-row-loss.test.ts b/packages/pi-tui/e2e/case05-growth-past-anchor-row-loss.test.ts new file mode 100644 index 000000000..3890305f3 --- /dev/null +++ b/packages/pi-tui/e2e/case05-growth-past-anchor-row-loss.test.ts @@ -0,0 +1,54 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { createHarness } from "./harness.ts"; + +// Case 05 — transcript rows missing from scrollback after streaming. +// +// Symptom: after long streaming turns, scrolling up showed gaps: spans of +// transcript rows were simply absent from scrollback. +// +// Root cause (invariant 2, exactly-once commit): when a single frame both +// changed a line above the viewport (status/reflow) and grew the content +// past the pinned anchor, the length-change branch advanced the anchor and +// repainted the screen in place without scrolling. The rows between the +// old and new anchor were overwritten on screen and never committed to +// scrollback. Found by review on #1353. +// +// Fixed on #1353: an anchor advance now paints from the old anchor and +// scrolls the skipped rows into scrollback, committing each exactly once. + +describe("e2e case05: growth past the anchor must not lose rows", () => { + it("commits the rows skipped by an anchor advance exactly once", async () => { + // 60 lines at height 10 -> anchor 50; rows 50..59 are on screen and + // not yet committed to scrollback. + const h = await createHarness([...Array.from({ length: 59 }, (_, i) => `row-${i}`), "[INPUT-BOX]"], { + rows: 10, + }); + + // One frame combines an above-viewport change (row-10 completes) + // with growth past the anchor (3 appended rows): anchor 50 -> 53. + const grown = [ + ...Array.from({ length: 59 }, (_, i) => (i === 10 ? "row-10 [done]" : `row-${i}`)), + "tail-0", + "tail-1", + "tail-2", + "[INPUT-BOX]", + ]; + await h.frame(grown); + + // The rows the anchor skipped over must be in the buffer exactly + // once — they crossed the viewport top and have to be committed. + const buffer = h.terminal.getScrollBuffer(); + for (const marker of ["row-50", "row-51", "row-52"]) { + const count = buffer.filter((line) => line.includes(marker)).length; + assert.strictEqual(count, 1, `"${marker}" must survive the anchor advance, got ${count} occurrences`); + } + + // And the fresh tail is visible with the input box at the bottom. + const viewport = h.terminal.getViewport(); + assert.ok(viewport.some((line) => line.includes("tail-2")), "appended rows must be visible"); + assert.ok(viewport[9]!.includes("[INPUT-BOX]"), "input box must sit on the bottom row"); + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/case06-cursor-above-pinned-window.test.ts b/packages/pi-tui/e2e/case06-cursor-above-pinned-window.test.ts new file mode 100644 index 000000000..d154bcea4 --- /dev/null +++ b/packages/pi-tui/e2e/case06-cursor-above-pinned-window.test.ts @@ -0,0 +1,57 @@ +import assert from "node:assert"; +import { describe, it } from "node:test"; +import { CURSOR_MARKER } from "../src/tui.ts"; +import { countInBuffer, createHarness } from "./harness.ts"; + +// Case 06 — rows written to the wrong screen position after a pinned shrink. +// +// Symptom: after the transcript shrank while the viewport stayed pinned, +// subsequent updates landed on the wrong screen rows: lines duplicated or +// misplaced a couple of rows below where they belonged. +// +// Root cause (invariant 3, cursor bookkeeping sync): extractCursorPosition +// scans the bottom `height` rows of the content, but a pinned repaint +// paints `pinnedTop..pinnedTop+height-1` — a higher window. A cursor +// marker in the gap between the two (e.g. a tall input editor poking above +// the pinned window) made positionHardwareCursor record hardwareCursorRow +// on an invisible logical row while the real cursor clamped at the screen +// top. The next differential frame computed its relative move from the +// desynced value and wrote to the wrong rows. Found by review on #1353. +// +// Fixed on #1353: the cursor is only positioned when the marker row is +// inside the visible window; otherwise it is hidden and the bookkeeping +// stays on the real cursor row. + +describe("e2e case06: pinned repaint must keep cursor bookkeeping in sync", () => { + it("places the next differential update on the correct screen row", async () => { + // 60 lines at height 10 -> anchor 50; cursor marker at the bottom. + const h = await createHarness( + [...Array.from({ length: 59 }, (_, i) => `old-${i}`), `[INPUT]${CURSOR_MARKER}`], + { rows: 10 }, + ); + + // Shrink 60 -> 56 with everything shifted (change above the + // viewport): the viewport stays pinned at 50, painting rows 50..55. + // The cursor marker sits at row 48 — inside the bottom-10 scan + // window (46..55) but above the painted window. + const pinned = Array.from({ length: 56 }, (_, i) => (i === 48 ? `new-48${CURSOR_MARKER}` : `new-${i}`)); + await h.frame(pinned); + + // Next frame changes row 55 (screen row 5 of the pinned window). + const next = pinned.map((line, i) => (i === 55 ? "new-55 CHANGED" : line)); + await h.frame(next); + + const viewport = h.terminal.getViewport(); + assert.ok( + viewport[5]!.includes("new-55 CHANGED"), + `row 55 must render on screen row 5, got: ${JSON.stringify(viewport)}`, + ); + assert.strictEqual( + countInBuffer(viewport, "new-55"), + 1, + "the changed row must not be duplicated onto another screen row", + ); + + h.stop(); + }); +}); diff --git a/packages/pi-tui/e2e/harness.ts b/packages/pi-tui/e2e/harness.ts new file mode 100644 index 000000000..ca9f349a5 --- /dev/null +++ b/packages/pi-tui/e2e/harness.ts @@ -0,0 +1,86 @@ +import assert from "node:assert"; +import { type Component, TUI } from "../src/tui.ts"; +import { VirtualTerminal } from "../test/virtual-terminal.ts"; + +/** A component whose rendered lines are fully controlled by the test. */ +export class Lines implements Component { + private lines: string[] = []; + + setLines(lines: string[]): void { + this.lines = lines; + } + + render(): string[] { + return this.lines; + } + + invalidate(): void {} +} + +/** VirtualTerminal that also records every raw write for escape-sequence assertions. */ +export class WriteCapturingTerminal extends VirtualTerminal { + writes: string[] = []; + + override write(data: string): void { + this.writes.push(data); + super.write(data); + } +} + +export interface Harness { + terminal: T; + tui: TUI; + content: Lines; + /** Replace the content lines and wait for the next render to settle. */ + frame(lines: string[], force?: boolean): Promise; + stop(): void; +} + +/** Build a TUI driven by a single Lines component on a virtual terminal. */ +export async function createHarness( + initialLines: string[], + options?: { columns?: number; rows?: number; capture?: boolean }, +): Promise> { + const columns = options?.columns ?? 60; + const rows = options?.rows ?? 10; + const terminal = options?.capture ? new WriteCapturingTerminal(columns, rows) : new VirtualTerminal(columns, rows); + const tui = new TUI(terminal); + const content = new Lines(); + tui.addChild(content); + content.setLines(initialLines); + tui.start(); + await terminal.waitForRender(); + return { + terminal, + tui, + content, + async frame(lines: string[], force = false): Promise { + content.setLines(lines); + tui.requestRender(force); + await terminal.waitForRender(); + }, + stop(): void { + tui.stop(); + }, + }; +} + +/** Count buffer lines (scrollback + screen) containing the marker. */ +export function countInBuffer(buffer: string[], marker: string): number { + return buffer.filter((line) => line.includes(marker)).length; +} + +/** + * Assert each marker appears exactly once across scrollback + screen. + * Catches both duplication (count > 1) and row loss (count === 0). + * Only use markers whose logical line did not change between frames: + * lines rewritten above the viewport legitimately leave a stale copy + * in scrollback (see e2e/README.md, "stale bytes" trade-off). + */ +export function assertExactlyOnce(terminal: VirtualTerminal, markers: string[]): void { + const buffer = terminal.getScrollBuffer(); + for (const marker of markers) { + const count = countInBuffer(buffer, marker); + assert.strictEqual(count, 1, `"${marker}" should appear exactly once in the buffer, got ${count}`); + } +} diff --git a/packages/pi-tui/package.json b/packages/pi-tui/package.json index 2b9e42258..5a6e0f331 100644 --- a/packages/pi-tui/package.json +++ b/packages/pi-tui/package.json @@ -56,7 +56,8 @@ "scripts": { "build": "tsdown", "typecheck": "tsc -p tsconfig.json --noEmit", - "test": "node --test test/*.test.ts", + "test": "node --test test/*.test.ts e2e/*.test.ts", + "test:e2e": "node --test e2e/*.test.ts", "clean": "rm -rf dist" }, "dependencies": { diff --git a/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index c9e7a7b6d..8d6f168df 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -1341,7 +1341,7 @@ export class TUI extends Container { } const bufferLength = Math.max(height, newLines.length); this.previousViewportTop = Math.max(0, bufferLength - height); - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, this.previousViewportTop, height); this.previousLines = newLines; this.previousKittyImageIds = this.collectKittyImageIds(newLines); this.previousWidth = width; @@ -1388,67 +1388,33 @@ export class TUI extends Container { return; } - // Find first and last changed lines - let firstChanged = -1; - let lastChanged = -1; - const maxLines = Math.max(newLines.length, this.previousLines.length); - for (let i = 0; i < maxLines; i++) { - const oldLine = i < this.previousLines.length ? this.previousLines[i] : ""; - const newLine = i < newLines.length ? newLines[i] : ""; - - if (oldLine !== newLine) { - if (firstChanged === -1) { - firstChanged = i; - } - lastChanged = i; - } - } - const appendedLines = newLines.length > this.previousLines.length; - if (appendedLines) { - if (firstChanged === -1) { - firstChanged = this.previousLines.length; - } - lastChanged = newLines.length - 1; - } - if (firstChanged !== -1) { - const expandedRange = this.expandChangedRangeForKittyImages(firstChanged, lastChanged, newLines); - firstChanged = expandedRange.firstChanged; - lastChanged = expandedRange.lastChanged; - } - const appendStart = appendedLines && firstChanged === this.previousLines.length && firstChanged > 0; - - // No changes - but still need to update hardware cursor position if it moved - if (firstChanged === -1) { - this.positionHardwareCursor(cursorPos, newLines.length); - this.previousViewportTop = prevViewportTop; - this.previousHeight = height; - return; - } - - // Re-anchor the viewport when content shrinks. previousViewportTop - // only ever grows during normal rendering, so after a shrink the - // content bottom can sit above the screen bottom, leaving dead rows - // that nothing repaints (upstream masked this by frequently doing - // destructive full redraws, which re-anchored as a side effect). // Repaint the visible viewport in place with the tail of the new - // content: the input area snaps back to the screen bottom (or the - // content top-anchors when shorter than the screen), while scrollback - // and the user's scroll position stay intact (no ESC[3J). - const desiredViewportTop = Math.max(0, newLines.length - height); - if (prevViewportTop > desiredViewportTop) { + // content, re-anchoring previousViewportTop to desiredTop. Used when + // the differential path cannot run safely or would push duplicate + // rows into scrollback. Emits no ESC[3J. When desiredTop advances + // past the current anchor, the skipped rows are painted with fresh + // content and scrolled into scrollback (committed exactly once); + // otherwise nothing scrolls, so scrollback and the user's scroll + // position stay intact. + const repaintViewport = (desiredTop: number, reason: string): void => { + // An advancing anchor must commit the rows it skips: paint from + // the old anchor and let the paint loop scroll `advance` rows out + // of the screen top. A pinned or rewound anchor paints the target + // window in place without scrolling. + const advance = Math.max(0, desiredTop - prevViewportTop); + const paintStart = advance > 0 ? prevViewportTop : desiredTop; + const paintCount = height + advance; // Kitty image lines need multi-row placement; fall back to a // destructive full render for that rare combination. - for (let r = 0; r < height; r++) { - const idx = desiredViewportTop + r; + for (let r = 0; r < paintCount; r++) { + const idx = paintStart + r; if (idx < newLines.length && isImageLine(newLines[idx]!)) { logRedraw(`viewport repaint hit kitty image at line ${idx}`); fullRender(true); return; } } - logRedraw( - `viewport repaint: re-anchor after shrink (viewportTop ${prevViewportTop} -> ${desiredViewportTop}, newLines=${newLines.length})`, - ); + logRedraw(`viewport repaint: ${reason}`); let repaint = "\x1b[?2026h"; // Begin synchronized output // Delete kitty images placed in the old visible viewport; images // above it live in scrollback and are left alone. A multi-row @@ -1474,28 +1440,86 @@ export class TUI extends Container { repaint += `\x1b[${cursorScreenRow}A`; } repaint += "\r"; - // Rewrite every screen row; the last row gets no trailing \r\n, - // so this never scrolls the terminal. - for (let r = 0; r < height; r++) { + // Rewrite paintCount rows from paintStart. Rows written past the + // bottom screen row scroll the terminal: with an advancing anchor + // the first `advance` rows (freshly painted) scroll into + // scrollback, committing them exactly once. With advance = 0 the + // last row gets no trailing \r\n, so nothing scrolls. + for (let r = 0; r < paintCount; r++) { if (r > 0) repaint += "\r\n"; repaint += "\x1b[2K"; - const idx = desiredViewportTop + r; + const idx = paintStart + r; if (idx < newLines.length) { repaint += newLines[idx]!; } } repaint += "\x1b[?2026l"; // End synchronized output this.terminal.write(repaint); - // Re-anchor: screen row r now shows newLines[desiredViewportTop + r]. + // Re-anchor: screen row r now shows newLines[desiredTop + r]. this.cursorRow = Math.max(0, newLines.length - 1); - this.hardwareCursorRow = desiredViewportTop + height - 1; - this.previousViewportTop = desiredViewportTop; + this.hardwareCursorRow = desiredTop + height - 1; + this.previousViewportTop = desiredTop; this.maxLinesRendered = Math.max(this.maxLinesRendered, newLines.length); - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, desiredTop, height); this.previousLines = newLines; this.previousKittyImageIds = this.collectKittyImageIds(newLines); this.previousWidth = width; this.previousHeight = height; + }; + + // Rewinding the viewport anchor below the scrollback high-water mark + // (previousViewportTop) repaints rows that scrollback already holds, + // and the next scroll commits them again — every rewind duplicates + // its span. So a shrink normally keeps the anchor pinned: the content + // bottom hovers above the screen bottom with a bounded blank gap that + // the next growth naturally fills. The single exception is a collapse + // — the content ends at or above the screen top (compaction, clears), + // where nothing sensible could be shown otherwise. There the rewind + // is unavoidable, and the content has changed so drastically that the + // duplicated span is not recognizable as such. + const desiredViewportTop = Math.max(0, newLines.length - height); + if (prevViewportTop > desiredViewportTop && newLines.length <= prevViewportTop) { + repaintViewport( + desiredViewportTop, + `re-anchor after collapse (viewportTop ${prevViewportTop} -> ${desiredViewportTop}, newLines=${newLines.length})`, + ); + return; + } + + // Find first and last changed lines + let firstChanged = -1; + let lastChanged = -1; + const maxLines = Math.max(newLines.length, this.previousLines.length); + for (let i = 0; i < maxLines; i++) { + const oldLine = i < this.previousLines.length ? this.previousLines[i] : ""; + const newLine = i < newLines.length ? newLines[i] : ""; + + if (oldLine !== newLine) { + if (firstChanged === -1) { + firstChanged = i; + } + lastChanged = i; + } + } + const appendedLines = newLines.length > this.previousLines.length; + if (appendedLines) { + if (firstChanged === -1) { + firstChanged = this.previousLines.length; + } + lastChanged = newLines.length - 1; + } + if (firstChanged !== -1) { + const expandedRange = this.expandChangedRangeForKittyImages(firstChanged, lastChanged, newLines); + firstChanged = expandedRange.firstChanged; + lastChanged = expandedRange.lastChanged; + } + const appendStart = appendedLines && firstChanged === this.previousLines.length && firstChanged > 0; + + // No changes - but still need to update hardware cursor position if it moved + if (firstChanged === -1) { + this.positionHardwareCursor(cursorPos, newLines.length, prevViewportTop, height); + this.previousViewportTop = prevViewportTop; + this.previousHeight = height; return; } @@ -1539,7 +1563,7 @@ export class TUI extends Container { this.cursorRow = targetRow; this.hardwareCursorRow = targetRow; } - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, prevViewportTop, height); this.previousLines = newLines; this.previousKittyImageIds = this.collectKittyImageIds(newLines); this.previousWidth = width; @@ -1567,7 +1591,7 @@ export class TUI extends Container { } if (visibleFirstChanged === -1) { logRedraw(`all changes above viewport (firstChanged=${firstChanged} < ${prevViewportTop})`); - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, prevViewportTop, height); this.previousLines = newLines; this.previousKittyImageIds = this.collectKittyImageIds(newLines); this.previousWidth = width; @@ -1575,17 +1599,33 @@ export class TUI extends Container { this.previousViewportTop = prevViewportTop; return; } - // The remaining visible changes would be entirely in the deleted - // tail region. The shrink re-anchor repaint above handles every - // case where the content bottom moves off the screen bottom, so - // this should be unreachable; keep a destructive fallback rather - // than let the differential path below desync on an empty render - // range (empty loop + untracked tail-clear scrolling). + // The remaining visible changes are entirely in the deleted tail + // region. Reachable while a shrink is pinned: repaint the window + // at the pinned anchor, which clears the deleted rows without + // scrolling or rewinding. if (visibleFirstChanged >= newLines.length) { - logRedraw( - `unexpected deleted-tail clamp (visibleFirstChanged=${visibleFirstChanged} >= ${newLines.length})`, + repaintViewport( + prevViewportTop, + `deleted tail within pinned viewport (newLines=${newLines.length}, viewportTop=${prevViewportTop})`, + ); + return; + } + // A length change above the viewport shifts every line below it. + // Painting the shifted lines through the screen would push rows + // that are already in scrollback out again, stacking a duplicate + // copy there on every shrink/grow cycle (e.g. streaming content + // oscillating). Repaint the visible viewport instead: scrollback + // keeps the stale old version and the screen shows the fresh + // tail. The anchor never rewinds here (that would duplicate as + // well): it stays pinned on a shrink, and advances only when the + // content has outgrown the pinned window — repaintViewport then + // scrolls the skipped rows into scrollback so none are lost. + if (newLines.length !== this.previousLines.length) { + const repaintTop = Math.max(desiredViewportTop, prevViewportTop); + repaintViewport( + repaintTop, + `re-anchor after above-viewport length change (${this.previousLines.length} -> ${newLines.length}, viewportTop ${prevViewportTop} -> ${repaintTop})`, ); - fullRender(true); return; } logRedraw(`clamped firstChanged ${firstChanged} -> ${visibleFirstChanged} (viewportTop=${prevViewportTop})`); @@ -1725,7 +1765,7 @@ export class TUI extends Container { this.previousViewportTop = Math.max(prevViewportTop, finalCursorRow - height + 1); // Position hardware cursor for IME - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, this.previousViewportTop, height); this.previousLines = newLines; this.previousKittyImageIds = this.collectKittyImageIds(newLines); @@ -1737,8 +1777,15 @@ export class TUI extends Container { * Position the hardware cursor for IME candidate window. * @param cursorPos The cursor position extracted from rendered output, or null * @param totalLines Total number of rendered lines + * @param viewportTop Logical row shown on the top screen row after this frame + * @param height Terminal height (visible viewport size) */ - private positionHardwareCursor(cursorPos: { row: number; col: number } | null, totalLines: number): void { + private positionHardwareCursor( + cursorPos: { row: number; col: number } | null, + totalLines: number, + viewportTop: number, + height: number, + ): void { if (!cursorPos || totalLines <= 0) { this.terminal.hideCursor(); return; @@ -1748,6 +1795,17 @@ export class TUI extends Container { const targetRow = Math.max(0, Math.min(cursorPos.row, totalLines - 1)); const targetCol = Math.max(0, cursorPos.col); + // The hardware cursor can only sit inside the visible window. A + // marker outside it (e.g. a tall editor poking above a pinned + // viewport) must not move the cursor: the ANSI moves would clamp at + // the screen edge while hardwareCursorRow recorded the unreachable + // row, desyncing every later differential move. Hide the cursor and + // keep the bookkeeping on the real cursor row instead. + if (targetRow < viewportTop || targetRow >= viewportTop + height) { + this.terminal.hideCursor(); + return; + } + // Move cursor from current position to target const rowDelta = targetRow - this.hardwareCursorRow; let buffer = ""; diff --git a/packages/pi-tui/test/tui-render.test.ts b/packages/pi-tui/test/tui-render.test.ts index 4d67dca8f..d33bca4d8 100644 --- a/packages/pi-tui/test/tui-render.test.ts +++ b/packages/pi-tui/test/tui-render.test.ts @@ -744,7 +744,7 @@ describe("TUI differential rendering", () => { assert.strictEqual( tui.fullRedraws, redrawsBeforeSwitch, - "Branch switch should not trigger a full redraw (re-anchored in place)", + "Branch switch should not trigger a full redraw (repainted in place)", ); const viewport = terminal.getViewport(); @@ -755,21 +755,21 @@ describe("TUI differential rendering", () => { assert.ok(!line.includes("Chat 14"), `Stale "Chat 14" at viewport row ${i}`); } - // Each shrink re-anchors the viewport so the content bottom stays on - // the bottom screen row; the tail of the chat plus the editor fill - // the screen with no dead rows. Stale "Chat 12/13/14" rows remain in - // scrollback but are not visible. + // Partial shrinks keep the viewport anchor pinned (rewinding would + // duplicate scrollback rows), so the remaining content sits at the + // top of the window with blank rows below. The stale "Chat 12/13/14" + // rows remain in scrollback but are not visible. assert.deepStrictEqual(viewport, [ - "Chat 5", - "Chat 6", - "Chat 7", - "Chat 8", - "Chat 9", - "Chat 10", - "Chat 11", - "Editor 0", "Editor 1", "Editor 2", + "", + "", + "", + "", + "", + "", + "", + "", ]); tui.stop(); diff --git a/packages/pi-tui/test/tui-shrink.test.ts b/packages/pi-tui/test/tui-shrink.test.ts index 16974f046..c5992b9b4 100644 --- a/packages/pi-tui/test/tui-shrink.test.ts +++ b/packages/pi-tui/test/tui-shrink.test.ts @@ -179,10 +179,10 @@ describe("TUI shrinking content", () => { tui.stop(); }); - it("re-anchors the input box to the screen bottom when content shrinks", async () => { - // Regression: previousViewportTop only ever grows, so after a shrink - // (e.g. collapsing expanded tool output) the content bottom hovered - // mid-screen with dead rows below, and nothing ever re-anchored it. + it("re-anchors the input box to the screen bottom when content collapses past the viewport top", async () => { + // Regression: previousViewportTop only ever grows; when content + // collapses to or above the viewport top (compaction, clears) the + // viewport must rewind so the tail of the new content is shown. const terminal = new VirtualTerminal(40, 10); const tui = new TUI(terminal); const content = new Lines([]); @@ -193,8 +193,8 @@ describe("TUI shrinking content", () => { await terminal.waitForRender(); assert.ok(terminal.getViewport()[9]!.includes("[INPUT-BOX]")); - // Shrink 60 -> 30 lines; content is still taller than the screen, so - // the tail must stay glued to the screen bottom. + // Collapse 60 -> 30 lines (30 <= viewportTop 50); content is still + // taller than the screen, so the tail must land at the screen bottom. content.setLines([...Array.from({ length: 29 }, (_, i) => `new-${i}`), "[INPUT-BOX]"]); tui.requestRender(); await terminal.waitForRender(); @@ -215,6 +215,91 @@ describe("TUI shrinking content", () => { tui.stop(); }); + it("does not duplicate scrollback rows on a shrink/grow cycle with above-viewport changes", async () => { + // Regression: a shrink re-anchor rewinds viewportTop; if the next + // grow with an above-viewport change painted from the rewound top + // through the end, rows already in scrollback were pushed out again, + // stacking a duplicate copy there on every oscillation. + const terminal = new VirtualTerminal(60, 10); + const tui = new TUI(terminal); + const content = new Lines([]); + tui.addChild(content); + + const base = (status: string, tail: string[]): string[] => [ + ...Array.from({ length: 5 }, (_, i) => `L${i}`), + status, + ...Array.from({ length: 48 }, (_, i) => `M${i}`), + ...tail, + "[INPUT]", + ]; + + content.setLines(base("status-A", ["** marker message **"])); + tui.start(); + await terminal.waitForRender(); + + // Shrink by a few rows (activity pane collapsing) -> re-anchor. + content.setLines(base("status-A", ["** marker message **"]).slice(0, -4).concat(["[INPUT]"])); + tui.requestRender(); + await terminal.waitForRender(); + + // Grow back with an above-viewport change (reflow/status update). + content.setLines(base("status-B", ["** marker message **"])); + tui.requestRender(); + await terminal.waitForRender(); + + const buffer = terminal.getScrollBuffer(); + for (const marker of ["M37", "M39", "** marker message **"]) { + const count = buffer.filter((line) => line.includes(marker)).length; + assert.strictEqual(count, 1, `"${marker}" should appear exactly once, got ${count}`); + } + const viewport = terminal.getViewport(); + assert.ok(viewport[9]!.includes("[INPUT]"), "input box should sit on the bottom screen row"); + assert.ok(viewport.some((line) => line.includes("** marker message **"))); + + tui.stop(); + }); + + it("keeps the viewport pinned on partial shrinks and lets growth refill the gap", async () => { + // Rewinding the anchor would repaint rows scrollback already holds + // and duplicate them on the next scroll, so a partial shrink stays + // pinned: the input box hovers above a bounded blank gap that the + // next growth naturally fills. No rewind, no duplication. + const terminal = new VirtualTerminal(40, 10); + const tui = new TUI(terminal); + const content = new Lines([]); + tui.addChild(content); + + content.setLines([...Array.from({ length: 59 }, (_, i) => `old-${i}`), "[INPUT-BOX]"]); + tui.start(); + await terminal.waitForRender(); + assert.ok(terminal.getViewport()[9]!.includes("[INPUT-BOX]")); + + // Shrink by 4 rows; content still spans the pinned viewport (56 > 50). + content.setLines([...Array.from({ length: 55 }, (_, i) => `old-${i}`), "[INPUT-BOX]"]); + tui.requestRender(); + await terminal.waitForRender(); + + // The viewport stays pinned: the input box hovers above blank rows. + const pinned = terminal.getViewport(); + assert.ok(pinned[5]!.includes("[INPUT-BOX]"), `expected pinned gap, got: ${JSON.stringify(pinned)}`); + assert.strictEqual(pinned[9], "", "rows below the content should be blank while pinned"); + + // Growth fills the gap; the input box returns to the bottom row and + // nothing was duplicated in the buffer along the way. + content.setLines([...Array.from({ length: 59 }, (_, i) => `old-${i}`), "[INPUT-BOX]"]); + tui.requestRender(); + await terminal.waitForRender(); + const grown = terminal.getViewport(); + assert.ok(grown[9]!.includes("[INPUT-BOX]"), `input box should be back at the bottom, got: ${JSON.stringify(grown)}`); + const buffer = terminal.getScrollBuffer(); + for (const marker of ["old-40", "old-54", "[INPUT-BOX]"]) { + const count = buffer.filter((line) => line.includes(marker)).length; + assert.strictEqual(count, 1, `"${marker}" should appear exactly once, got ${count}`); + } + + tui.stop(); + }); + it("shows the tail of collapsed content when it is still taller than the screen", async () => { // 100 lines -> 30 lines (still > height 10) with a change above the // viewport: the viewport should show the tail of the new content.