diff --git a/.changeset/pi-tui-above-viewport-inplace.md b/.changeset/pi-tui-above-viewport-inplace.md new file mode 100644 index 0000000000..011962fc47 --- /dev/null +++ b/.changeset/pi-tui-above-viewport-inplace.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/pi-tui": patch +--- + +Skip the destructive full redraw when lines above the viewport change in place, so scrollback and scroll position survive above-viewport status ticks. diff --git a/.changeset/tui-usage-flicker.md b/.changeset/tui-usage-flicker.md new file mode 100644 index 0000000000..8b9433a388 --- /dev/null +++ b/.changeset/tui-usage-flicker.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix the TUI flickering and losing scroll position when live agent status updates continue above the visible viewport, such as after opening /usage during a running task. diff --git a/packages/pi-tui/AGENTS.md b/packages/pi-tui/AGENTS.md index c25bd9efe3..fe2e704c95 100644 --- a/packages/pi-tui/AGENTS.md +++ b/packages/pi-tui/AGENTS.md @@ -1,6 +1,6 @@ # pi-tui Agent Guide -`packages/pi-tui` is a vendored copy of pi-tui from the upstream pi-mono project (baseline: upstream 0.80.2, see commit `7859b0af`). It is no longer patched via pnpm patches — all local fixes are applied directly to the source. The differential-rendering behavior in `src/tui.ts` matches upstream: the fork's viewport/scrollback rendering patches were reverted; the only remaining divergences are listed below. +`packages/pi-tui` is a vendored copy of pi-tui from the upstream pi-mono project (baseline: upstream 0.80.2, see commit `7859b0af`). It is no longer patched via pnpm patches — all local fixes are applied directly to the source. The fork's broad viewport/scrollback rendering patches were reverted back to upstream's destructive-redraw baseline; the only remaining divergences are listed below. ## Local divergences from upstream (must be preserved on every re-vendor) @@ -12,6 +12,8 @@ Never overwrite this directory wholesale when syncing from upstream. Each of the 4. **`src/components/text.ts` / `markdown.ts` / `truncated-text.ts` / `editor.ts` — negative-width `repeat` guards**: the `repeat` counts for blank lines, horizontal rules, and the editor's top/bottom borders are clamped to ≥ 0 (two editor border sites; markdown's emptyLine and hr — the hr site is currently unreachable from the render entry and is purely defensive). Guarding tests: the "negative width safety" cases — Text's lives in `test/tui-render.test.ts` (Text has no dedicated test file), Markdown's and TruncatedText's live in their own test files; the editor's is "does not throw at zero or negative widths" inside the "Editor narrow width rendering" group in `test/editor.test.ts`. 5. **`src/tui.ts` — per-frame processed-line reuse**: `doRender` keeps the previous frame's raw lines (`previousRawLines`), their processed output (`previousLines`), and per-line kitty image ids (`previousLineImageIds`). A line whose raw string is reference-identical to the previous frame's reuses its processed output (truncation + `normalizeTerminalOutput` + trailing `SEGMENT_RESET`) verbatim, so a steady-state frame costs O(total lines) pointer comparisons plus O(changed lines) real work instead of re-normalizing every line. Image-id consumers (`expandChangedRangeForKittyImages`, `deleteChangedKittyImages`, the frame-end id union) read the cached per-line ids rather than re-scanning line text; upstream has no such cache and re-processes every line every frame (upstream also has `applyLineResets`, which this divergence inlines into the processed-line build). Reuse is only valid when the terminal width is unchanged; width changes re-render everything at the new width so references never match. Guarding tests: "TUI steady-frame processed-line reuse" in `test/tui-render.test.ts`. +6. **`src/tui.ts` — skip destructive redraw for in-place changes above the viewport**: when every changed line sits at or above the previous viewport top, the line count is unchanged, and neither frame has kitty image ids in the above-viewport part of the changed range, `doRender` must NOT fall back to `fullRender(true)` (upstream clears screen + scrollback via ESC[2J/ESC[3J for any above-viewport change, which flickers and yanks scroll position on every above-viewport status tick). Entirely-above changes commit the line caches without painting (scrollback keeps a stale frame); changes spanning the boundary clamp the repaint range to the viewport top and continue on the differential path. Any layout shift or image involvement keeps upstream's destructive redraw. The lowest skipped row is tracked in `aboveViewportStaleTop`; when a Termux height increase re-exposes skipped rows (the one height-change path that avoids `fullRender`), the exposed cache entries are invalidated so the differential path repaints them. Guarding tests: "TUI above-viewport in-place changes" in `test/tui-render.test.ts`. + ## Acceptance after syncing from upstream - `pnpm --filter @moonshot-ai/pi-tui test` must pass in full; any failure among the guarding tests above means a local divergence was overwritten and lost. diff --git a/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index e4556a8f47..ae6dc8c93f 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -336,6 +336,7 @@ export class TUI extends Container { private clearOnShrink = process.env['PI_CLEAR_ON_SHRINK'] === "1"; // Clear empty rows when content shrinks (default: off) private maxLinesRendered = 0; // Track terminal's working area (max lines ever rendered) private previousViewportTop = 0; // Track previous viewport top for resize-aware cursor moves + private aboveViewportStaleTop: number | null = null; // Lowest row whose repaint was skipped above the viewport private fullRedrawCount = 0; private stopped = false; private pendingOsc11BackgroundReplies = 0; @@ -1171,6 +1172,19 @@ export class TUI extends Container { return { firstChanged: expandedFirstChanged, lastChanged: expandedLastChanged }; } + /** Whether either frame carries kitty image ids inside [start, end]. */ + private rangeHasKittyImages( + start: number, + end: number, + newLineImageIds: ReadonlyArray>, + ): boolean { + for (let i = start; i <= end; i++) { + if ((newLineImageIds[i] ?? EMPTY_IMAGE_IDS).length > 0) return true; + if ((this.previousLineImageIds[i] ?? EMPTY_IMAGE_IDS).length > 0) return true; + } + return false; + } + private deleteChangedKittyImages(firstChanged: number, lastChanged: number): string { if (firstChanged < 0 || lastChanged < firstChanged) return ""; @@ -1274,6 +1288,24 @@ export class TUI extends Container { let prevViewportTop = heightChanged ? Math.max(0, previousBufferLength - height) : this.previousViewportTop; let viewportTop = prevViewportTop; let hardwareCursorRow = this.hardwareCursorRow; + + // A height increase can pull rows whose repaint was skipped (the + // above-viewport in-place path below) back into the viewport. On the + // Termux path, which deliberately avoids the destructive height-change + // redraw, invalidate the exposed rows in the caches so the diff + // repaints them; every other height change full-renders anyway. + if (heightChanged && this.aboveViewportStaleTop !== null && prevViewportTop < this.previousViewportTop) { + const exposedEnd = Math.min(this.previousViewportTop, this.previousLines.length); + for (let i = prevViewportTop; i < exposedEnd; i++) { + // NUL can never equal a rendered raw line or processed output, so + // both the reuse fast path and the diff see these rows as changed. + this.previousLines[i] = "\u0000"; + this.previousRawLines[i] = "\u0000"; + } + if (prevViewportTop <= this.aboveViewportStaleTop) { + this.aboveViewportStaleTop = null; + } + } const computeLineDiff = (targetRow: number): number => { const currentScreenRow = hardwareCursorRow - prevViewportTop; const targetScreenRow = targetRow - viewportTop; @@ -1333,6 +1365,7 @@ export class TUI extends Container { // Helper to clear scrollback and viewport and render all new lines const fullRender = (clear: boolean): void => { this.fullRedrawCount += 1; + this.aboveViewportStaleTop = null; let buffer = "\x1b[?2026h"; // Begin synchronized output if (clear) { buffer += this.deleteKittyImages(this.previousKittyImageIds); @@ -1515,11 +1548,49 @@ export class TUI extends Container { } // Differential rendering can only touch what was actually visible. - // If the first changed line is above the previous viewport, we need a full redraw. + // If the first changed line is above the previous viewport, a full + // redraw is normally needed — but an in-place mutation (same line + // count, no kitty images in the above-viewport range) of rows already + // committed to scrollback cannot change what the terminal shows, so + // repainting the whole buffer would only produce a destructive + // ESC[2J/ESC[3J clear per tick (e.g. an agent-status spinner pushed + // above the viewport, #2039). Skip the paint, or clamp the range to + // the viewport when it spans the boundary; any layout shift or image + // involvement keeps the full redraw (the post-#1367 baseline). if (firstChanged < prevViewportTop) { - logRedraw(`firstChanged < viewportTop (${firstChanged} < ${prevViewportTop})`); - fullRender(true); - return; + const aboveEnd = Math.min(lastChanged, prevViewportTop - 1); + const inPlaceAboveViewport = + newLines.length === this.previousLines.length && + !this.rangeHasKittyImages(firstChanged, aboveEnd, lineImageIds); + if (!inPlaceAboveViewport) { + logRedraw(`firstChanged < viewportTop (${firstChanged} < ${prevViewportTop})`); + fullRender(true); + return; + } + // Remember the lowest skipped row: a later Termux height increase + // re-exposes such rows and must invalidate them (see doRender top). + this.aboveViewportStaleTop = + this.aboveViewportStaleTop === null + ? firstChanged + : Math.min(this.aboveViewportStaleTop, firstChanged); + if (lastChanged < prevViewportTop) { + // Entirely above the viewport: scrollback keeps a stale frame + // of those rows (invisible from the live viewport), so only + // commit the caches and reposition the hardware cursor, + // mirroring the no-change path above. + this.positionHardwareCursor(cursorPos, newLines.length); + this.previousViewportTop = prevViewportTop; + this.previousLines = newLines; + this.previousRawLines = rawLines; + this.previousLineImageIds = lineImageIds; + this.previousKittyImageIds = this.unionKittyImageIds(lineImageIds); + this.previousWidth = width; + this.previousHeight = height; + return; + } + // The change spans the viewport boundary: repaint only the + // visible part on the regular differential path. + firstChanged = prevViewportTop; } // Render from first changed line to end diff --git a/packages/pi-tui/test/tui-render.test.ts b/packages/pi-tui/test/tui-render.test.ts index d40811d0e8..e80b4e8436 100644 --- a/packages/pi-tui/test/tui-render.test.ts +++ b/packages/pi-tui/test/tui-render.test.ts @@ -990,3 +990,134 @@ describe("TUI steady-frame processed-line reuse", () => { } }); }); + +describe("TUI above-viewport in-place changes", () => { + // Divergence 6 in AGENTS.md: in-place mutations of rows already committed + // to scrollback must not trigger the destructive ESC[2J/ESC[3J full + // redraw (#2039). Layout shifts keep upstream's destructive baseline. + function makeLines(count: number): string[] { + return Array.from({ length: count }, (_, i) => `line ${i}`); + } + + it("skips the destructive full redraw when a line above the viewport changes in place", async () => { + const terminal = new LoggingVirtualTerminal(40, 10); + const tui = new TUI(terminal); + const component = new TestComponent(); + tui.addChild(component); + // 30 lines on a 10-row terminal -> viewport top sits at row 20. + component.lines = makeLines(30); + tui.start(); + await terminal.waitForRender(); + const initialRedraws = tui.fullRedraws; + const viewportBefore = await terminal.flushAndGetViewport(); + + // Repeated ticks mirror a spinner line pushed above the viewport: + // same line count, one changed row per tick. + for (let tick = 0; tick < 4; tick++) { + const lines = makeLines(30); + lines[3] = `spinner frame ${tick}`; + component.lines = lines; + terminal.clearWrites(); + tui.requestRender(); + await terminal.waitForRender(); + + assert.strictEqual(tui.fullRedraws, initialRedraws, "in-place above-viewport change must not full-redraw"); + const writes = terminal.getWrites(); + assert.ok(!writes.includes("\x1b[2J"), "must not clear the screen"); + assert.ok(!writes.includes("\x1b[3J"), "must not clear scrollback"); + } + + assert.deepStrictEqual(await terminal.flushAndGetViewport(), viewportBefore, "viewport must stay untouched"); + tui.stop(); + }); + + it("repaints only the visible part when an in-place change spans the viewport boundary", async () => { + const terminal = new LoggingVirtualTerminal(40, 10); + const tui = new TUI(terminal); + const component = new TestComponent(); + tui.addChild(component); + component.lines = makeLines(30); + tui.start(); + await terminal.waitForRender(); + const initialRedraws = tui.fullRedraws; + + const lines = makeLines(30); + for (let i = 18; i <= 22; i++) { + lines[i] = `changed ${i}`; + } + component.lines = lines; + terminal.clearWrites(); + tui.requestRender(); + await terminal.waitForRender(); + + assert.strictEqual(tui.fullRedraws, initialRedraws, "boundary-spanning in-place change must not full-redraw"); + const writes = terminal.getWrites(); + assert.ok(!writes.includes("\x1b[2J"), "must not clear the screen"); + assert.ok(!writes.includes("\x1b[3J"), "must not clear scrollback"); + + // The visible rows (20-22 -> screen rows 0-2) show the new content. + const viewport = await terminal.flushAndGetViewport(); + assert.ok(viewport[0]?.includes("changed 20"), "row 20 repainted"); + assert.ok(viewport[1]?.includes("changed 21"), "row 21 repainted"); + assert.ok(viewport[2]?.includes("changed 22"), "row 22 repainted"); + assert.ok(viewport[3]?.includes("line 23"), "unchanged rows preserved"); + tui.stop(); + }); + + it("repaints skipped rows that a Termux height increase re-exposes", async () => { + await withEnv({ TERMUX_VERSION: "1" }, async () => { + const terminal = new LoggingVirtualTerminal(40, 10); + const tui = new TUI(terminal); + const component = new TestComponent(); + tui.addChild(component); + component.lines = makeLines(30); + tui.start(); + await terminal.waitForRender(); + const initialRedraws = tui.fullRedraws; + + // An in-place tick above the viewport takes the skip path, leaving + // the on-screen scrollback row stale. + const lines = makeLines(30); + lines[15] = "spinner frame 0"; + component.lines = lines; + tui.requestRender(); + await terminal.waitForRender(); + terminal.clearWrites(); + + // Growing the terminal re-exposes rows 6-19. Termux height changes + // skip the destructive redraw, so the stale row must be repainted + // by the differential path instead. + terminal.resize(40, 24); + await terminal.waitForRender(); + + assert.strictEqual(tui.fullRedraws, initialRedraws, "Termux height change must not full-redraw"); + assert.ok(!terminal.getWrites().includes("\x1b[2J"), "must not clear the screen"); + assert.ok(!terminal.getWrites().includes("\x1b[3J"), "must not clear scrollback"); + const viewport = terminal.getViewport(); + assert.ok(viewport.join("\n").includes("spinner frame 0"), "re-exposed row shows the current content"); + tui.stop(); + }); + }); + + it("keeps the full redraw when the line count changes above the viewport", async () => { + const terminal = new LoggingVirtualTerminal(40, 10); + const tui = new TUI(terminal); + const component = new TestComponent(); + tui.addChild(component); + component.lines = makeLines(30); + tui.start(); + await terminal.waitForRender(); + const initialRedraws = tui.fullRedraws; + + // Inserting a row above the viewport shifts every following line — + // that layout change must keep the destructive redraw baseline. + const lines = makeLines(30); + lines.splice(3, 0, "inserted above viewport"); + component.lines = lines; + tui.requestRender(); + await terminal.waitForRender(); + + assert.ok(tui.fullRedraws > initialRedraws, "layout shift above the viewport must full-redraw"); + tui.stop(); + }); +});