fix(web): stop terminal scrollback from jumping to the live prompt - #6104
fix(web): stop terminal scrollback from jumping to the live prompt#6104fuggysense wants to merge 1 commit into
Conversation
Preserve the user's place in integrated-terminal history when output, reflow, or a history-ring head-trim would otherwise re-pin or rewrite the Ghostty surface back to the bottom (fixes pingdotgg#6096). - Capture/restore scroll anchors across write, resetAndWrite, and fit - Prefer append plans after head-trim instead of full resetAndWrite - Regression coverage for anchors, buffer write plans, and Ghostty ABI
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (previous.length === 0) { | ||
| return { kind: "replace", data: current }; | ||
| } | ||
|
|
||
| // Head-trim of the history ring: longest suffix of `previous` that prefixes `current`. | ||
| // Only runs when the cheap startsWith path failed (near the byte cap), so a linear | ||
| // scan over the overlap is acceptable. Require a minimum overlap so unrelated | ||
| // rewrites (clear → new prompt) do not accidentally append on a short shared tail. | ||
| const maxOverlap = Math.min(previous.length, current.length); | ||
| const minOverlap = Math.min(16, maxOverlap); | ||
| for (let len = maxOverlap; len >= minOverlap; len -= 1) { | ||
| if (current.startsWith(previous.slice(previous.length - len))) { | ||
| return { kind: "append", data: current.slice(len) }; | ||
| } | ||
| } | ||
|
|
||
| return { kind: "replace", data: current }; |
There was a problem hiding this comment.
🟠 High state/terminalSession.ts:146
terminalBufferWritePlan misclassifies unrelated shorter replacement buffers as head-trim appends. When current equals a suffix of previous and is under 16 characters (including the empty string), it returns { kind: "append", data: "" } instead of { kind: "replace" }. This causes the terminal surface to retain stale content — for example, a cleared event that should erase all output leaves the old buffer visible because no write is applied.
The fallback loop at lines 156-160 compares the longest suffix of previous against the start of current down to a minimum overlap of Math.min(16, maxOverlap). When current is empty, both maxOverlap and minOverlap are 0, so current.startsWith(previous.slice(previous.length)) matches "" and the function returns append with empty data. The same happens for any short current that coincidentally equals a suffix of previous (e.g. a restart snapshot with just a prompt). Consider treating zero or negligible overlap as a replace rather than an append, and raise the minimum overlap floor so short coincidental matches do not suppress a full rewrite.
if (previous.length === 0) {
return { kind: "replace", data: current };
}
// Head-trim of the history ring: longest suffix of `previous` that prefixes `current`.
// Only runs when the cheap startsWith path failed (near the byte cap), so a linear
// scan over the overlap is acceptable. Require a minimum overlap so unrelated
// rewrites (clear → new prompt) do not accidentally append on a short shared tail.
const maxOverlap = Math.min(previous.length, current.length);
const minOverlap = Math.min(16, maxOverlap);
+ if (current.length === 0) {
+ return { kind: "replace", data: current };
+ }
for (let len = maxOverlap; len >= minOverlap; len -= 1) {
if (current.startsWith(previous.slice(previous.length - len))) {
return { kind: "append", data: current.slice(len) };
}
}
return { kind: "replace", data: current };Also found in 1 other location(s)
apps/web/src/components/ThreadTerminalDrawer.tsx:814
The new
terminalBufferWritePlanpath at line 814 can misclassify a short replacement buffer as a head-trim append. Its overlap threshold isMath.min(16, maxOverlap), so whencurrent.length < 16andcurrentequals the suffix ofprevious(for example a reset/restart snapshot containing only the same short prompt), it returnsappendwith emptydata. The surface then retains all old terminal contents instead of applying the replacement snapshot, leaving stale output visible until a later unrelated rewrite.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/state/terminalSession.ts around lines 146-162:
`terminalBufferWritePlan` misclassifies unrelated shorter replacement buffers as head-trim appends. When `current` equals a suffix of `previous` and is under 16 characters (including the empty string), it returns `{ kind: "append", data: "" }` instead of `{ kind: "replace" }`. This causes the terminal surface to retain stale content — for example, a `cleared` event that should erase all output leaves the old buffer visible because no write is applied.
The fallback loop at lines 156-160 compares the longest suffix of `previous` against the start of `current` down to a minimum overlap of `Math.min(16, maxOverlap)`. When `current` is empty, both `maxOverlap` and `minOverlap` are 0, so `current.startsWith(previous.slice(previous.length))` matches `""` and the function returns `append` with empty data. The same happens for any short `current` that coincidentally equals a suffix of `previous` (e.g. a restart snapshot with just a prompt). Consider treating zero or negligible overlap as a `replace` rather than an `append`, and raise the minimum overlap floor so short coincidental matches do not suppress a full rewrite.
Also found in 1 other location(s):
- apps/web/src/components/ThreadTerminalDrawer.tsx:814 -- The new `terminalBufferWritePlan` path at line 814 can misclassify a short replacement buffer as a head-trim append. Its overlap threshold is `Math.min(16, maxOverlap)`, so when `current.length < 16` and `current` equals the suffix of `previous` (for example a reset/restart snapshot containing only the same short prompt), it returns `append` with empty `data`. The surface then retains all old terminal contents instead of applying the replacement snapshot, leaving stale output visible until a later unrelated rewrite.
There was a problem hiding this comment.
🟡 Medium
fit() restores the scroll position using the old absolute offset after a column-count change, so narrowing the terminal moves the viewport to earlier history instead of keeping the content the user was reading. When columns decrease, lines above the viewport wrap into more rows and total grows; terminalScrollDeltaToRestore then keeps anchor.offset unchanged (because total >= anchor.total and anchor.offset <= maxOffset), pointing the viewport at a row that is now further back in scrollback. Consider computing the restore target from the screen-row position of the current viewport rather than an absolute offset, so reflow keeps the same content in view.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/terminal/ghostty/surface.ts around line 244:
`fit()` restores the scroll position using the old absolute offset after a column-count change, so narrowing the terminal moves the viewport to earlier history instead of keeping the content the user was reading. When columns decrease, lines above the viewport wrap into more rows and `total` grows; `terminalScrollDeltaToRestore` then keeps `anchor.offset` unchanged (because `total >= anchor.total` and `anchor.offset <= maxOffset`), pointing the viewport at a row that is now further back in scrollback. Consider computing the restore target from the screen-row position of the current viewport rather than an absolute offset, so reflow keeps the same content in view.
|
|
||
| let target: number; | ||
| const priorMax = Math.max(0, anchor.total - anchor.len); | ||
| if (priorMax > 0 && (total < anchor.total || anchor.offset > maxOffset)) { |
There was a problem hiding this comment.
🟡 Medium ghostty/surface.ts:256
terminalScrollDeltaToRestore misinterprets a viewport enlargement as scrollback trimming. When a resize increases len so that the old anchor.offset exceeds the new maxOffset, the function proportionally rescales the offset instead of clamping the original absolute offset. For {total: 100, len: 20, offset: 60} becoming {total: 100, len: 60, offset: 40}, it targets offset 30 (scrolling up 10 rows) when offset 40 is the closest valid position to the original anchor — so enlarging the terminal pushes users reading near the bottom farther into history. The proportional branch should only run when the buffer itself shrank (total < anchor.total), not merely because len grew.
| if (priorMax > 0 && (total < anchor.total || anchor.offset > maxOffset)) { | |
| if (priorMax > 0 && total < anchor.total) { |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/terminal/ghostty/surface.ts around line 256:
`terminalScrollDeltaToRestore` misinterprets a viewport enlargement as scrollback trimming. When a resize increases `len` so that the old `anchor.offset` exceeds the new `maxOffset`, the function proportionally rescales the offset instead of clamping the original absolute offset. For `{total: 100, len: 20, offset: 60}` becoming `{total: 100, len: 60, offset: 40}`, it targets offset 30 (scrolling up 10 rows) when offset 40 is the closest valid position to the original anchor — so enlarging the terminal pushes users reading near the bottom farther into history. The proportional branch should only run when the buffer itself shrank (`total < anchor.total`), not merely because `len` grew.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca060ae. Configure here.
| for (let len = maxOverlap; len >= minOverlap; len -= 1) { | ||
| if (current.startsWith(previous.slice(previous.length - len))) { | ||
| return { kind: "append", data: current.slice(len) }; | ||
| } |
There was a problem hiding this comment.
Clear treated as empty append
High Severity
terminalBufferWritePlan sets minOverlap to Math.min(16, maxOverlap), so when current is empty (the cleared attach event) or shorter than 16 code units, overlap length 0 or other short suffix matches are accepted and the plan becomes append with empty data. The drawer then skips the write, so Ghostty never gets resetAndWrite and the screen stays on stale scrollback instead of clearing or replacing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ca060ae. Configure here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This bug fix introduces new scroll anchor capture/restore logic with 246 additions. Multiple HIGH severity findings identify that You can customize Macroscope's approvability policy. Learn more. |
1 participant
Summary
Fixes #6096 — desktop integrated terminal (Cmd+J /
terminal.toggle) could force the viewport back to the live prompt while the user was reading scrollback.What changed
write,resetAndWrite,fit) so mutations that re-pin or rewrite the screen put the user back where they were when they had scrolled into history.resetAndWriteof the whole buffer (which always landed at the bottom).VIEWPORT_ACTIVEfalse.Why this approach
Upstream Ghostty already keeps scroll place across pure VT writes/resizes when unpinned (covered by the ABI test). The jump came from the web layer replaying history via full resets and from fit/write paths not restoring a non-following viewport. Small, focused change — no product-scope expansion.
Test plan
vp run --filter @t3tools/client-runtime test(includes new write-plan cases)vp test run --project unit src/terminal/ghostty/(surface + runtimeAbi)mod+j), generate scrollback (seq 1 200), scroll up, keep a shell producing output / resize drawer — viewport should stay put until you return to the bottomCloses #6096
Note
Medium Risk
Touches terminal rendering and buffer sync paths used on every PTY update; behavior change is localized but user-visible if anchor restore is wrong.
Overview
Fixes #6096 — the integrated terminal could snap back to the live prompt while you were reading scrollback.
Ghostty surface now captures and restores scroll position around
write,resetAndWrite, andfitwhen the viewport is not pinned to the bottom, including proportional restore after scrollback is trimmed.Client buffer sync uses
terminalBufferWritePlanso updates preferterminal.writeappend (including after the history ring head-trim) instead of a fullresetAndWrite, which was re-pinning the viewport.Tests cover write-plan cases, scroll-anchor math, and a WASM ABI check that output and resize keep the viewport unpinned after scrolling into history.
Reviewed by Cursor Bugbot for commit ca060ae. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix terminal scrollback jumping to the live prompt on write, resize, or buffer replay
TerminalScrollAnchorinsurface.tsto snapshot the user's scroll position before terminal operations and restore it after, preventing writes, full buffer replays, and resizes from re-pinning the viewport to the bottom.terminalBufferWritePlaninterminalSession.tsto decide between appending a suffix or doing a full buffer replace when the terminal buffer updates, reducing unnecessary full rewrites.ThreadTerminalDrawerto use the write plan instead of astartsWith-based comparison, so appends are correctly identified even after scrollback ring head-trims.📊 Macroscope summarized ca060ae. 3 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.