fix(pi-tui): repaint viewport in place when content collapses above it#1315
Merged
Conversation
When content shrinks past the viewport top while a line above the viewport also changes, the clamped differential path left the render loop empty, cleared deleted lines past the screen bottom, and desynced the cursor anchor — leaving the viewport blank with the input box gone until a full redraw. Repaint the visible viewport in place for that case (no ESC[3J, so the scrollback and the user's scroll position are preserved), and clamp deleted-line clearing to the screen bottom so it can never scroll untracked.
🦋 Changeset detectedLatest commit: 5aaecf1 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 |
commit: |
![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: bad60a9213
ℹ️ 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".
The collapse repaint in pi-tui intentionally preserves scrollback, so /new, /clear, and session switches no longer got a clean screen as a side effect of the destructive full redraw — the previous session's text stayed above the welcome banner. Session resets want a pristine screen, so force a destructive full render explicitly instead of relying on the renderer's shrink behavior.
…pse repaint A multi-row kitty image can start above prevViewportTop while its reserved rows are still visible. The collapse repaint's image-delete range started at prevViewportTop and missed the image line carrying the id, leaving a stale overlay that also dropped out of previousKittyImageIds tracking. Widen the range to include such a straddling block.
Collaborator
Author
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
…e screen bottom previousViewportTop only ever grows during normal rendering, so after a shrink the content bottom could hover 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; the fork removed those redraws without replacing the re-anchoring. Generalize the collapse repaint into a re-anchor check at the top of the differential path: whenever prevViewportTop exceeds max(0, newLines - height), repaint the visible viewport in place with the tail of the new content. The input area snaps back to the screen bottom, scrollback and the user's scroll position stay intact (no ESC[3J), and the previous collapse branch becomes a defensive destructive fallback. Update the three renderer tests that encoded the old behavior (destructive redraw on shrink, viewport hover after clamped shrink) to assert the re-anchored behavior instead.
Expanding tool output shifts content above the viewport; the clamped differential render paints the shifted content through the screen, stacking a duplicate copy below the stale one in scrollback on every toggle. The toggle is a deliberate user action (like /clear), so do a destructive full render instead: scrollback holds exactly one copy and the expanded output stays readable by scrolling up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
No linked issue; the problem is explained below.
Problem
The vendored pi-tui fork clamps the differential render to the visible viewport when a change occurs above it, instead of doing a destructive full redraw (which clears scrollback and yanks the user's scroll position — on Windows Terminal it jumps to the absolute top, microsoft/terminal#20370).
Removing those destructive redraws exposed two problems on content shrink:
firstChangedlands in the deleted tail region. The render loop runs empty while the cursor sits at the clamped row, the deleted-line clearing writes\r\npast the screen bottom, and the terminal scrolls without the viewport anchor being updated. Every subsequent render lands on the wrong rows: the screen goes blank, the input box disappears, and the TUI never recovers.previousViewportToponly ever grows during normal rendering. Upstream's frequent destructive redraws re-anchored the content bottom to the screen bottom as a side effect; without them, any shrink (e.g. collapsing expanded tool output) left the UI hovering mid-screen above an ever-growing band of dead rows.What changed
previousViewportTopabovemax(0, newLines - height), repaint the visible viewport in place with the tail of the new content and re-anchor. The input area stays glued to the screen bottom (content shorter than the screen top-anchors, matching a fresh session), noESC[2J/ESC[3Jis emitted, so scrollback and the user's scroll position are preserved — the original scroll-yank fix does not regress. This subsumes the earlier collapse-specific repaint; that branch is now a defensive destructive fallback. Kitty images in the repaint window fall back to a destructive full render, and a multi-row image straddling the viewport top is included in the image-delete range./new,/clear, and session switches previously got a clean screen only as a side effect of destructive redraws on shrink; they now explicitly request a full render (requestRender(true)). The ctrl+o toggle does the same: expanding shifts content above the viewport, and the clamped differential render would stack a duplicate copy of the transcript in scrollback on every toggle. Both are deliberate user actions, while compaction and collapse during streaming keep scrollback intact.VirtualTerminalgainedscrollViewport/getScrollPosition/scrollToBottomtest helpers.Verified in a live terminal with
PI_DEBUG_REDRAW=1: expanding then collapsing long output, streaming shrink cycles, and starting a fresh session all hit the intended paths with no blank screen and no dead rows below the input box.Full pi-tui suite: 709 tests pass. kimi-code suite: 2177 tests pass.
tsc --noEmitclean in both packages.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.