fix(tui): skip destructive redraw for in-place changes above the viewport#2205
fix(tui): skip destructive redraw for in-place changes above the viewport#2205B143KC47 wants to merge 3 commits into
Conversation
…port Any changed line above the previous viewport top forced fullRender(true), which clears the screen and scrollback (ESC[2J/ESC[3J) and reprints the whole transcript. While subagents run, the agents status header ticks several times per second; once /usage (or any appended content) pushes it above the viewport, every tick becomes a destructive full redraw -- sustained flicker and scroll-position yanks. Rows above the viewport are already committed to scrollback, so an in-place mutation (same line count, no kitty images in the above-viewport range) cannot change what the terminal shows. Commit the caches and skip the paint when the change stays above the viewport, or clamp the repaint range to the visible part when it spans the boundary. Layout shifts, size changes, and image lines keep the full-redraw baseline restored in MoonshotAI#1367, so none of the artifact classes that reverted MoonshotAI#1315/MoonshotAI#1353 can reappear on this path. Fixes MoonshotAI#2039
Move the guarding tests into tui-render.test.ts so the package test gate (node --test) exercises them — a re-vendor that clobbered the fix would otherwise pass acceptance — and record the behavior as divergence 6 in the vendored-fork ledger.
🦋 Changeset detectedLatest commit: 2a6000a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a3eac69fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.previousLines = newLines; | ||
| this.previousRawLines = rawLines; |
There was a problem hiding this comment.
When TERMUX_VERSION is set and the terminal grows after this path skips an above-viewport update, the newly exposed rows can show stale content. This branch records the unpainted lines in previousLines, while the existing Termux height-change path deliberately avoids fullRender; the next render therefore sees no diff and does not repaint rows that have just become visible. Retain the unpainted range or force it to be repainted when a height increase exposes it.
AGENTS.md reference: packages/pi-tui/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses TUI flicker and scroll-position yanks caused by destructive full-screen redraws when high-frequency status updates occur above the visible viewport (notably when /usage is opened during a running task). It adds a narrow, test-guarded rendering fast path in the vendored pi-tui renderer to skip repainting rows that are already committed to scrollback when the change is strictly in-place.
Changes:
- Add an above-viewport “in-place change” fast path that skips destructive
ESC[2J/ESC[3Jfull redraws when line count is unchanged and no kitty images are involved in the above-viewport changed range. - Clamp boundary-spanning changes to repaint only the visible portion (starting at the viewport top) while keeping the existing full-redraw baseline for layout shifts and image involvement.
- Add focused renderer tests and document the new divergence in
packages/pi-tui/AGENTS.md, plus publish patch changesets for both@moonshot-ai/pi-tuiand@moonshot-ai/kimi-code.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pi-tui/src/tui.ts | Introduces the above-viewport in-place mutation fast path and kitty-image guarding helper to avoid destructive redraws. |
| packages/pi-tui/test/tui-render.test.ts | Adds regression tests ensuring no ESC[2J/ESC[3J and no fullRedraws increments for above-viewport in-place ticks, plus boundary and layout-shift coverage. |
| packages/pi-tui/AGENTS.md | Records the new divergence (“Divergence 6”) so future re-vendors preserve the behavior and its guarding tests. |
| .changeset/tui-usage-flicker.md | Patch release note for the CLI-facing flicker/scroll-position fix. |
| .changeset/pi-tui-above-viewport-inplace.md | Patch release note for the pi-tui package change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The above-viewport skip path leaves the painted scrollback row stale by design. Termux height changes bypass the destructive redraw, so growing the terminal could pull such rows back into the viewport without a repaint. Track the lowest skipped row and invalidate the re-exposed cache entries so the differential path repaints them; every other height change full-renders anyway.
|
@codex review — the Termux re-exposure case is addressed in 2a6000a: the lowest skipped row is tracked and re-exposed cache entries are invalidated so the differential path repaints them, with a regression test driving the exact tick-then-grow sequence. The PR description now also states the stale-scrollback trade-off explicitly. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Related Issue
Resolve #2039
Problem
While subagents run, the agents status header re-renders several times per second. Once
/usage(or any appended content) pushes that ticking line above the visible viewport, every tick hits thefirstChanged < prevViewportTopbranch indoRender, which callsfullRender(true)— a destructiveESC[2J ESC[H ESC[3Jclear plus a full transcript reprint. The result is sustained flicker (reported on Alacritty) and scroll-position yanks, several times per second, for as long as the status box keeps updating.What changed
A deliberately narrow fast path in front of that branch, active only when the change is in place: same line count as the previous frame and no Kitty images (in either frame) in the above-viewport part of the changed range. Rows above the viewport are already committed to scrollback, so repainting them cannot change what the terminal shows:
Trade-off to accept explicitly: scrollback keeps a stale frame of the skipped rows, and this is not limited to spinner glyphs — any same-line-count, image-free mutation above the viewport is skipped, so a scrolled-out status row can keep reading "running" in scrollback after the component moved on, until a full redraw repaints it. Nothing is lost relative to the previous behavior (which erased the entire scrollback several times per second), but the staleness is observable when scrolling up mid-task.
Termux is the one path where a height change deliberately avoids the destructive redraw, so a height increase there could re-expose skipped rows without a repaint. The follow-up commit tracks the lowest skipped row and invalidates the re-exposed cache entries so the differential path repaints them, with a regression test driving exactly that sequence (in-place tick above the viewport, then a grow).
Everything else — line-count changes, width/height changes, shrink, image lines — keeps the destructive full-redraw baseline restored in #1367. That revert rolled back #1315/#1353 because general diff-clamping produced blank screens, duplicated scrollback, and lost rows; this path cannot reproduce those artifact classes because it never repaints scrolled-out rows at all and never runs when the layout shifts.
Tests: a new "TUI above-viewport in-place changes" group in
test/tui-render.test.ts(node:test, so the package test gate and thetest-pi-tuiCI job exercise it — a future re-vendor that clobbered the fix would fail acceptance) drives aVirtualTerminaltaller-than-viewport transcript and asserts: repeated above-viewport ticks emit noESC[2J/ESC[3Jand don't incrementfullRedrawswhile the viewport stays byte-identical; a boundary-spanning change repaints exactly the visible rows; an above-viewport line-count change still triggers the full redraw. The first two fail on main; the third pins the preserved baseline. The full suite passes (739 tests), and the behavior is recorded as divergence 6 in the vendored-fork ledger (packages/pi-tui/AGENTS.md).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.