fix(pi-tui): stop scrollback duplication from viewport rewinds#1353
Merged
Conversation
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 detectedLatest commit: de3fc5a 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: 7b634444b5
ℹ️ 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".
…isibility 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.
Collaborator
Author
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
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.
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; follow-up to #1315, the problem is explained below.
Problem
#1315 introduced viewport re-anchoring: when content shrinks, the renderer rewinds the viewport anchor and repaints so the input area snaps back to the screen bottom. But terminal scrollback is append-only — rewinding the anchor repaints rows that scrollback already holds, and the next scroll commits them again. Every rewind therefore duplicated its span in scrollback.
Streaming makes content length oscillate constantly (thinking finalizing, activity panes collapsing, step summaries merging), so long turns stacked duplicate blocks of transcript content in scrollback — messages and tables visibly appearing twice when scrolling back.
What changed
The viewport anchor is now treated as the scrollback high-water mark and never moves backward, with all shrink/shift handling built on a shared viewport repaint:
Pure appends keep flowing through the screen into scrollback unchanged, and equal-length above-viewport changes keep the bounded clamped diff, so following a long stream by scrolling back still works. No
ESC[2J/ESC[3Jis emitted on any of these paths — scrollback and the user's scroll position stay intact, and the scroll-yank fix from #1315 does not regress.Review fixes
Two correctness findings from review are fixed on top:
hardwareCursorRowon an invisible logical row while the real cursor clamped at the screen top, desyncing every later differential move. The cursor is now hidden when the marker is outside the visible window and the bookkeeping stays on the real cursor row.e2e rendering-bug ledger
packages/pi-tui/e2e/now keeps one xterm-emulated end-to-end repro per production rendering bug (blank screen on collapse, scroll-position yank, full-redraw residue, oscillation duplication, anchor-advance row loss, cursor desync), asserting the renderer invariants documented ine2e/README.md: monotonic anchor, exactly-once scrollback commit, cursor bookkeeping sync. The cases run as part ofpnpm --filter @moonshot-ai/pi-tui test.Reproduced deterministically with the xterm-headless harness (each e2e case fails on the buggy revision and passes with this change) and verified in a live terminal with
PI_DEBUG_REDRAW=1under a long streaming workload.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.