From 7b634444b5aa38abebdc78ed0bfc734e218f5526 Mon Sep 17 00:00:00 2001 From: liruifengv Date: Fri, 3 Jul 2026 17:21:55 +0800 Subject: [PATCH 1/3] fix(pi-tui): stop scrollback duplication from viewport rewinds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewinding the viewport anchor repaints rows that the terminal scrollback already holds, and the next scroll commits them again — every rewind duplicated its span. Two paths triggered it during streaming oscillation (content shrinking then growing back): the shrink re-anchor rewound immediately, and the clamped differential path then painted the shifted content through the screen. Rework the shrink/shift handling around a shared in-place viewport repaint that never scrolls, with the anchor treated as the scrollback high-water mark that must never move backward: - Partial shrinks keep the anchor pinned: the content bottom hovers above a bounded blank gap that the next growth naturally fills. No rewind means duplication is impossible by construction. - Only a collapse past the viewport top (compaction, clears) rewinds, as nothing sensible could be shown otherwise; the content has changed so drastically there that the repainted span is not recognizable as a duplicate. - Above-viewport length changes repaint the visible window in place instead of painting through: nothing scrolls, scrollback keeps the stale old version, and the anchor only advances when the content outgrows the pinned window. - Deleted-tail changes within a pinned viewport repaint at the pinned anchor instead of falling back to a destructive full render. Pure appends keep flowing through the screen into scrollback, and equal-length above-viewport changes keep the bounded clamped diff. --- .changeset/cli-scrollback-dup.md | 5 + .changeset/pi-tui-scrollback-dup.md | 5 + packages/pi-tui/src/tui.ts | 155 ++++++++++++++---------- packages/pi-tui/test/tui-render.test.ts | 26 ++-- packages/pi-tui/test/tui-shrink.test.ts | 97 ++++++++++++++- 5 files changed, 205 insertions(+), 83 deletions(-) create mode 100644 .changeset/cli-scrollback-dup.md create mode 100644 .changeset/pi-tui-scrollback-dup.md 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/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index c9e7a7b6d..59c16f002 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -1388,67 +1388,23 @@ 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 and never scrolls, so + // scrollback and the user's scroll position stay intact. + const repaintViewport = (desiredTop: number, reason: string): void => { // 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; + const idx = desiredTop + 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 @@ -1479,23 +1435,78 @@ export class TUI extends Container { for (let r = 0; r < height; r++) { if (r > 0) repaint += "\r\n"; repaint += "\x1b[2K"; - const idx = desiredViewportTop + r; + const idx = desiredTop + 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.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); + this.previousViewportTop = prevViewportTop; + this.previousHeight = height; return; } @@ -1575,17 +1586,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 (debounced re-anchor + // pending): 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 in place instead: + // nothing scrolls, 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 until the + // debounced shrink re-anchor above decides to rewind, and only + // advances when the content has outgrown the pinned window. + 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})`); 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. From 0d66073aec10740db096ded4661705934c782062 Mon Sep 17 00:00:00 2001 From: liruifengv Date: Fri, 3 Jul 2026 23:20:07 +0800 Subject: [PATCH 2/3] fix(pi-tui): commit skipped rows on anchor advance and guard cursor visibility Address two review findings on the pinned-anchor rendering: - An anchor advance (growth past the pinned viewport combined with an above-viewport change) repainted the screen in place without scrolling, so the rows between the old and new anchor were never committed to scrollback and vanished. repaintViewport now paints from the old anchor and lets the paint loop scroll the skipped rows out, committing each exactly once with fresh content. - positionHardwareCursor recorded hardwareCursorRow on a logical row outside the visible window when the cursor marker sat above a pinned viewport (tall editor after a deep shrink), desyncing every later differential move. It now hides the cursor and keeps the bookkeeping on the real cursor row when the marker is not visible. Also add an e2e rendering-bug ledger (packages/pi-tui/e2e): one xterm-emulated repro per production rendering bug, asserting the renderer invariants (monotonic anchor, exactly-once commit, cursor bookkeeping sync). The two findings above are case05/case06. --- packages/pi-tui/AGENTS.md | 3 +- packages/pi-tui/e2e/README.md | 71 +++++++++++++++ .../e2e/case01-collapse-blank-screen.test.ts | 59 +++++++++++++ .../case02-collapse-scroll-position.test.ts | 53 ++++++++++++ .../e2e/case03-full-redraw-residue.test.ts | 70 +++++++++++++++ .../case04-oscillation-duplication.test.ts | 62 +++++++++++++ ...case05-growth-past-anchor-row-loss.test.ts | 54 ++++++++++++ .../case06-cursor-above-pinned-window.test.ts | 57 ++++++++++++ packages/pi-tui/e2e/harness.ts | 86 +++++++++++++++++++ packages/pi-tui/package.json | 3 +- packages/pi-tui/src/tui.ts | 79 +++++++++++------ 11 files changed, 571 insertions(+), 26 deletions(-) create mode 100644 packages/pi-tui/e2e/README.md create mode 100644 packages/pi-tui/e2e/case01-collapse-blank-screen.test.ts create mode 100644 packages/pi-tui/e2e/case02-collapse-scroll-position.test.ts create mode 100644 packages/pi-tui/e2e/case03-full-redraw-residue.test.ts create mode 100644 packages/pi-tui/e2e/case04-oscillation-duplication.test.ts create mode 100644 packages/pi-tui/e2e/case05-growth-past-anchor-row-loss.test.ts create mode 100644 packages/pi-tui/e2e/case06-cursor-above-pinned-window.test.ts create mode 100644 packages/pi-tui/e2e/harness.ts diff --git a/packages/pi-tui/AGENTS.md b/packages/pi-tui/AGENTS.md index c5b5fdd8f..870b4819d 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. 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 59c16f002..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; @@ -1391,13 +1391,23 @@ export class TUI extends Container { // Repaint the visible viewport in place with the tail of the new // 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 and never scrolls, so - // scrollback and the user's scroll position stay intact. + // 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 = desiredTop + 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); @@ -1430,12 +1440,15 @@ 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 = desiredTop + r; + const idx = paintStart + r; if (idx < newLines.length) { repaint += newLines[idx]!; } @@ -1447,7 +1460,7 @@ export class TUI extends Container { 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; @@ -1504,7 +1517,7 @@ export class TUI extends Container { // No changes - but still need to update hardware cursor position if it moved if (firstChanged === -1) { - this.positionHardwareCursor(cursorPos, newLines.length); + this.positionHardwareCursor(cursorPos, newLines.length, prevViewportTop, height); this.previousViewportTop = prevViewportTop; this.previousHeight = height; return; @@ -1550,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; @@ -1578,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; @@ -1587,9 +1600,9 @@ export class TUI extends Container { return; } // The remaining visible changes are entirely in the deleted tail - // region. Reachable while a shrink is pinned (debounced re-anchor - // pending): repaint the window at the pinned anchor, which clears - // the deleted rows without scrolling or rewinding. + // 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) { repaintViewport( prevViewportTop, @@ -1601,12 +1614,12 @@ export class TUI extends Container { // 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 in place instead: - // nothing scrolls, 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 until the - // debounced shrink re-anchor above decides to rewind, and only - // advances when the content has outgrown the pinned window. + // 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( @@ -1752,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); @@ -1764,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; @@ -1775,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 = ""; From d13a2df8147753c76f6856b378599043b1bcfad3 Mon Sep 17 00:00:00 2001 From: liruifengv Date: Fri, 3 Jul 2026 23:40:01 +0800 Subject: [PATCH 3/3] ci: run the pi-tui node:test suite in a dedicated job pi-tui's tests (unit + e2e) run on node:test, which the root vitest run silently skips, so CI never executed them. Add a test-pi-tui job that runs the package's test script. --- .github/workflows/ci.yml | 18 ++++++++++++++++++ packages/pi-tui/AGENTS.md | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) 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 870b4819d..924f5f13c 100644 --- a/packages/pi-tui/AGENTS.md +++ b/packages/pi-tui/AGENTS.md @@ -17,6 +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. The `test` script runs both `test/*.test.ts` and `e2e/*.test.ts`; `test:e2e` runs the e2e cases alone. +- 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.