fix(pi-tui): skip destructive redraw for in-place changes above the viewport - #3197
fix(pi-tui): skip destructive redraw for in-place changes above the viewport#3197jiangshibiao wants to merge 2 commits into
Conversation
…iewport Lines already committed to scrollback cannot change what the terminal shows, yet any above-viewport change fell back to fullRender(true), issuing ESC[2J/ESC[H/ESC[3J and reprinting the whole buffer. During streaming (thinking blocks finalizing, status lines ticking) this cleared scrollback and yanked the user's scroll position to the top. Skip the paint for in-place above-viewport changes (same line count, no kitty images in the range), clamp boundary-spanning changes to the viewport, and track the lowest skipped row so a Termux height increase can invalidate re-exposed cache entries. Layout shifts keep the destructive redraw.
🦋 Changeset detectedLatest commit: c97f407 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: 7d6ac69715
ℹ️ 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.hardwareCursorRow = state.hardwareCursorRow; | ||
| this.maxLinesRendered = state.maxLinesRendered; | ||
| this.previousViewportTop = state.previousViewportTop; | ||
| this.aboveViewportStaleTop = null; |
There was a problem hiding this comment.
When captureRenderState() is called after an above-viewport repaint was skipped and that state is restored before a Termux height increase, this assignment discards the only record of the stale scrollback row. The restored line cache already contains the current text, so resizing to expose that row produces no diff and no full render, leaving the terminal showing the old content indefinitely. Include aboveViewportStaleTop in TuiMainScreenRenderState and restore it instead of clearing it.
AGENTS.md reference: packages/pi-tui/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c97f407 — aboveViewportStaleTop is now carried in TuiMainScreenRenderState so it survives captureRenderState/restoreRenderState, with a regression test covering the skip → capture/restore → Termux height-increase sequence (full pi-tui suite: 979 tests pass).
… restore The above-viewport in-place skip path records the lowest stale row in aboveViewportStaleTop so a later Termux height increase can invalidate and repaint re-exposed rows. restoreRenderState discarded that marker, so a capture/restore round-trip between the skipped repaint and the resize left the re-exposed row showing stale content with no diff and no full render to correct it. Carry the marker in TuiMainScreenRenderState instead.
1 participant
Related Issue
Resolve #981
Fixes #1487, fixes #1705, fixes #1805, fixes #2008, fixes #2167, fixes #2168, fixes #2212, fixes #1213 — all reports of the same root cause (an above-viewport change triggering a destructive full redraw that yanks scroll position during streaming), verified against the same code path. Also covers the scroll-position portions of #2743 and #1261. Partially addresses #2098 and #2296 — in-place ticks are covered, frames that insert or collapse rows above the viewport still full-redraw (see "Known limitations"). Earlier reports #1117 #1284 #1180 #2097 were closed without the mechanism being fixed; the problem persists through 0.38.0 (the latest release at the time of writing).
Problem
During streaming, any change to rows above the visible viewport — a thinking block switching from live spinner to finalized text, or a status/elapsed-time line ticking — takes the
firstChanged < prevViewportTopbranch in pi-tui's differential renderer and falls back tofullRender(true). That emitsESC[2J ESC[H ESC[3J, clearing the screen and scrollback and reprinting the whole buffer, which yanks the user's scroll position (widely reported as "jumps to the top") and can fire several times per second while a status line ticks.What changed
In
packages/pi-tui/src/tui-main-screen.ts, an in-place change above the viewport (same line count, no kitty image lines in the changed range) no longer triggers the destructive redraw:aboveViewportStaleTop); the Termux height-increase path (the only height-change path that avoidsfullRender) invalidates and repaints re-exposed rows.This is deliberately narrow. The fork's earlier broad viewport/scrollback patches were reverted in 23daf0f after accumulating blank-screen / duplicated-scrollback / lost-row edge cases; this change never re-anchors the viewport and never scroll-commits, leaving upstream differential behavior untouched everywhere else. The divergence is registered in
packages/pi-tui/AGENTS.md(item 9) with guarding tests.Tests: new "TUI above-viewport in-place changes" suite in
packages/pi-tui/test/tui-render.test.ts(skip-paint, boundary clamp, Termux re-exposure, layout-shift fallback). Full pi-tui suite passes (978 tests);tsc --noEmitis clean. Verified locally against a built CLI: scrolling up during streaming no longer jumps when a thinking block finalizes or a status line ticks.Known limitations / related work:
tui.tsand now conflicting with main), fix(pi-tui): handle above-viewport changes without a destructive redraw #2133 (broader re-anchoring), fix(pi-tui): keep transcript anchored and render scroll input immediately during streaming #3162 (fullscreen ScrollView anchoring), fix(pi-tui): avoid spurious viewport scroll jumps from idle cursor repositioning #1310 (idle cursor repositioning). This PR ports the narrow approach onto the post-refactorTuiMainScreenstructure, with guarding tests and the AGENTS.md divergence entry.Checklist
/approve). — I am aware none of the linked issues carries an/approveyet; linking the canonical reports so maintainers can pick one, and happy to adjust per guidance.gen-changesetsskill, or this PR needs no changeset. (Changeset included: patch for@moonshot-ai/kimi-code.)gen-docsskill, or this PR needs no doc update. (Internal rendering change;packages/pi-tui/AGENTS.mddivergence list updated, no user docs affected.)