fix(terminal): resolve absolute cursor moves against the TUI viewport - #654
Open
RonenMars wants to merge 1 commit into
Open
fix(terminal): resolve absolute cursor moves against the TUI viewport#654RonenMars wants to merge 1 commit into
RonenMars wants to merge 1 commit into
Conversation
`VirtualTerminal` resolved `CSI H`/`f` against the whole append-only scrollback grid, but a TUI paints absolute cursor moves against a fixed 40-row screen — the streamer spawns every PTY at 120x40. Once the grid passed 40 rows every footer repaint landed mid-transcript and overwrote conversation content, and on resume it is immediate rather than gradual, because `terminal_replay` feeds up to 200 rendered rows before the first live frame arrives. Absolute row addressing is now relative to a derived viewport origin and clamped to the screen, and `CSI A`/`B` are clamped to the viewport rather than the grid so that no cursor move can grow it. Refs #652
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
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.
Fixes the shipped transcript corruption described in #652.
The bug
VirtualTerminalresolvedCSI H/CSI fagainst its whole append-only scrollback grid, but a TUI paints absolute cursor moves against a fixed-height screen, and the streamer spawns every PTY at 120x40 (tb-streamersrc/pty-manager.ts:42-43,src/codex-pty-runner.ts:36-37).Once the grid grew past 40 rows,
CSI 40;1Haddressed grid row 39 — deep inside scrollback — instead of the bottom of the screen, so every footer repaint landed mid-transcript and overwrote conversation content.A spinner ticks continuously through a turn, so it repeated for the whole turn.
This is immediate on resume rather than gradual:
terminal_replaycarries up to 200 already-rendered rows whichfeedHistoryfeeds to the emulator before the first live frame arrives, so the grid is ~200 rows deep by the time the first rawCSIlands at absolute rows 0-39.The change
Absolute row addressing is now relative to a viewport origin derived from
grid.length, and clamped to the screen.The origin is derived and never stored — a stored origin would have to be maintained correctly through every grid mutation, and each one becomes a way to desynchronise it.
The clamp is what keeps
viewportTop()safe to read beforeensureRow(): the target is at mostgrid.length - 1whenever the grid is at least a screen tall, so a cursor move can never extend the grid and invalidate the origin it just read.CSI AandCSI Bare clamped to the viewport rather than the grid, for the same reason.CSI Apreviously clamped at grid row 0, so a cursor-up from the footer region walked into scrollback and the next write overwrote transcript.CSI Bpreviously grew the grid from a cursor move, which shifts the derived origin mid-frame so a later absolute move in the same frame lands somewhere else.CSI Sis deliberately left alone, with a comment pointing at its follow-up.It destroys the oldest scrollback rows rather than appending blanks at the viewport bottom, but that is not a cursor defect — the splice's implicit shift compensates exactly — and no capture yet shows Claude's TUI emitting SU at all.
Tests
Five new cases under
VirtualTerminal – viewport-relative cursor positioning.The fixture is built the way production builds it: 200 escape-free rows joined with
\n, which is whatfeedHistoryreceives, then one live frame.Each was verified against the specific code it protects rather than trusting a green run.
Two reproduce the original bug and fail on pre-fix code; two are guards that fail only when their clamp is removed; one is a no-throw contract test for the HTTP fallback path, which serves a byte-level tail slice of the PTY ring buffer and can begin mid-escape.
The comments mark which is which, so a guard is not later misread as a regression test.
Two assertions that look obvious do not work, and the tests avoid both.
Asserting that the footer "lands near the bottom" against
getLines()fails after the fix, becauselib/terminalChrome.ts:34matches a correctly-placed footer exactly and filters it out, while the mangled hybrid the bug produces survives the^anchor.Asserting that an out-of-range
CSI 200;1HleavesgetRawLines().lengthunchanged passes even with the clamp removed, because unclamped growth is blank rows andgetRawLines()drops those — so that test now runs past the eviction cliff, where the grid crossesMAX_ROWSand the trim starts evicting real transcript.Verification
npx jest --ci --runInBand --testPathPattern "useTerminalStream|TerminalView|terminal"— 11 suites, 157 tests, all passing.npx tsc --noEmit --pretty falseandnpx eslinton both changed files are clean.Verification item 4 from the plan — validating the
VIEWPORT_ROWS = 40constant itself against a real captured PTY log — is satisfied.A capture of Claude Code v2.1.228, spawned by the streamer at its real 120x40 geometry, addresses exactly three absolute rows across every form of the sequence: row 1 (1483 occurrences), row 37 (1456) and row 40 (1456), for a maximum addressed row of exactly 40.
The constant is therefore measured against real TUI output rather than resting only on the two streamer constants and on
tb-streamer/src/pty-host/protocol.ts:38-41.A synthetic fixture was deliberately not substituted for this, because a log generated from the same 40-row assumption cannot test that assumption.
The same capture contains zero
CSI Ssequences, against a positive control returning 1 on a file known to contain one, which is the evidence for leaving scroll-up out of scope.CSI Ais likewise never emitted, so its clamp above is correct but defensive rather than reachable, unlikeCSI Bwhich the capture shows in heavy use.Both figures are quoted here rather than only cited, because the capture lives at
~/.threadbase/captures/outside any repository and would otherwise leave these as assertions if it were removed.Two caveats travel with them, and they apply to different parts of the evidence.
The per-row frequencies are inflated, because the capture is assembled from overlapping full-ring-buffer dumps, so they should not be read as traffic.
The maximum addressed row and the distinct set of rows addressed are exact and are what the constant actually rests on: replaying the same bytes cannot introduce a row that was never addressed, nor raise a maximum.
Separately, this is a single turn with no tool use, and a tool-heavy capture is the only artifact that could change any of it.