fix(terminal): keep the viewport pinned across resize/redraw reflows (#403)#412
Conversation
…403) The residual intermittent divergence on relay 10.6.3 (codex resize-mid-stream) is NOT byte loss and NOT predictive echo: the rendered cells match the broker oracle exactly (differingCells=0; replaying the delivered bytes through a clean emulator equals the broker). The failure is the viewport being stranded in scrollback — at quiet it sits at [viewportY=0, baseY=100] with byte-correct content, so the fidelity quiet gate (viewportAtBottom) never closes. (The "[39,2] vs [40,3]" cursor signature is a 0-indexed xterm vs 1-indexed broker reporting artifact — the stream's last CUP is literally ESC[40;3H — not corruption.) Root cause: "am I following the tail?" was inferred from the INSTANTANEOUS viewportY === baseY. Two paths transiently scroll the viewport off the bottom: a TUI's SIGWINCH full-screen redraw (a server write, off-bottom DURING xterm's async parse) and a width/height reflow (fitAddon.fit -> term.resize). Once the viewport is off by even one line, the instantaneous check reads false and every later write AND fit stops re-pinning — the grid freezes in scrollback. Fix: track a sticky `followBottom` intent, flipped ONLY by a real user wheel scroll. Re-pin against that intent (a) in the echo-router's direct-route write COMPLETION callback, so a chunk that scrolls during its own parse is corrected after the parse, and (b) centrally in tryFit, so every reflow site (ResizeObserver, reconciler onPersistentDimsMismatch, init, refreshOnShow) keeps a following viewport at the tail. A deliberate wheel-up into scrollback clears the intent and is left alone (scrollback workload safe). No bytes are dropped and the reconciler confirm-twice / rate-limit / dims gates are untouched. Verified: 12/12 clean codex resize-mid-stream matrix runs (from a ~1-in-2 baseline); 71 focused renderer tests green including new regression tests that lock the post-parse write re-pin and the reflow/wheel intent behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
khaliqgant
added
the
no-agent-relay-review
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: d7648518aa
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| host.addEventListener( | ||
| 'wheel', | ||
| () => { |
There was a problem hiding this comment.
Listening only for wheel misses deliberate scrolling performed by dragging xterm's scrollbar. In that case followBottom remains true, so the next direct write completion or tryFit() calls scrollToBottom() and immediately yanks the user out of scrollback; dragging the scrollbar back to the bottom likewise cannot re-arm a previously cleared intent. Track the terminal viewport's actual user-driven scroll changes rather than only wheel events.
Useful? React with 👍 / 👎.
…criminator Two diagnostic improvements to the term-fidelity matrix, from the #403 fixer investigation (lead-endorsed evidence-first method). 1. Cursor-convention normalization (bundle meta). The renderer cursor is xterm's 0-indexed [row,col]; the broker plain-snapshot cursor is 1-indexed (it mirrors the raw CUP coords — a stream ending in ESC[40;3H reports broker [40,3] while xterm reports [39,2] for the SAME cell). Comparing the raw values makes a faithful render look like an off-by-one bug — this misread #403's headline "[39,2] vs [40,3]". Meta now records the raw values WITH their conventions, both normalized to a common 0-indexed base, and a computed `match` bool: a false there on an otherwise content-matching bundle is a REAL cursor divergence, not the convention. 2. Raw-stream divergence discriminator. Tap the raw broker->renderer byte stream (onPtyChunk, post main dedup), and on any divergence — including a telemetry-only reconciler repair — replay those exact bytes through a clean headless xterm at broker dims and diff vs broker and vs the live grid: rawVsBroker==0 -> delivery byte-clean; divergence is RENDERER-INTERNAL rawVsBroker >0 -> bytes lost/reordered before the renderer (DELIVERY-LOSS) Plus a scrollToBottom probe (stranded-viewport vs real content corruption), PEAR_DIAG_PTY per-chunk logging, and renderer-console capture. Writes raw.bin / raw-replay.txt / discriminator.txt / renderer-console.log into the bundle (and the reconciler-telemetry dir). This is what classified #403 as a viewport-pin regression (not byte loss) and #415 as delivery-clean. Test-only; no production/renderer code changed. Refs #403, #412, #415. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Fixes the residual intermittent terminal divergence on relay 10.6.3 tracked in #403 (codex
resize-mid-stream, ~1-in-2). Authored by fixer-residual (term-fidelity program); coordinated byleadin #term-fidelity.What the bytes proved (root cause)
This vector is not byte loss and not predictive echo:
differingCells=0— rendered cells match the broker oracle exactly. Replaying the exact delivered PTY bytes (tapped atonPtyChunk, post main-dedup) through a clean headless xterm equals the broker snapshot (rawVsBroker=0). Delivery is byte-clean; the live grid is a faithful render of the delivered bytes.viewportAtBottom): at quiet the viewport is stranded in scrollback —viewportY=0, baseY=100— with byte-correct content, so the gate never closes. A singlescrollToBottom()fully repairs it (probe:[0,100] → [100,100]).[39,2]vs[40,3]cursor is a reporting-convention artifact, not corruption: the stream's final cursor op is literallyESC[40;3H; xterm's 0-indexed buffer reports[39,2], the broker snapshot reports the 1-indexed CUP coords[40,3].hasPredictions=falsethroughout — theerasePredictionRegion/ relay#1326 path is not involved here.A per-frame
viewportY/baseYtrace across the resize storm showed two independent unpin sources, both defeated by the same flaw:fitAddon.fit → term.resize; unpin observed exactly at rows45→44,43→42) scrolls the viewport off-bottom; the ResizeObserver re-pin reads the instantaneouswasPinned, which a prior reflow already flipped false.Common flaw: "am I following the tail?" was inferred from the instantaneous
viewportY === baseY. Once any transient reflow/redraw unpins by even one line, that reads false and every later write and fit stops re-pinning → the grid freezes in scrollback.Fix
Track a sticky
followBottomintent, flipped only by a real user wheel scroll, and re-pin against that intent (not the instantaneous position):echo-router.ts— direct-route re-pin now fires from the write-completion callback (after xterm parses the chunk), so a chunk that scrolls during its own parse is corrected post-parse.terminal-runtime-registry.ts—followBottom(wheel-driven) + a re-pin centralized intryFit, so every reflow site (ResizeObserver fit, reconcileronPersistentDimsMismatch, init,refreshOnShow) keeps a following viewport at the tail.A deliberate wheel-up into scrollback clears the intent and is left alone (scrollback workload safe). Invariants intact: no bytes are dropped, and the reconciler confirm-twice / rate-limit / dims gates are untouched.
Verification
npm run test:term-fidelity -- --cli=codex(resize-mid-stream), 12/12 clean with the fix, from a ~1-in-2 pre-fix baseline (p≈2e-4 by chance). An earlier write-path-only fix still failed 1/4 (the final fit unpins after streaming ends), which is why the fit path is also covered.echo-router,terminal-runtime-registry.dom,use-terminal.dom,terminal-reconciler,pty-size-sync,pty-buffer-store), including 3 new regression tests locking (a) the post-parse write re-pin and (b) the reflow re-pin gated on wheel-driven follow intent.npm run typecheckclean; scoped ESLint clean;npm run buildclean.pearbroker / port 3889 were never touched; all work on a git worktree.Note: the
opencodetyping-during-stream case (reconciler fired+repaired) is a different vector (genuine content divergence) and is not addressed here — captured separately.🤖 Generated with Claude Code