Release: non-virtualized transcript scroll jump-back (#5751) - #5756
Merged
Merged
Conversation
…ts to stop scroll jump-back When transcript virtualization is disabled (the #4325 opt-out, _virtualizeTranscript===false), renderMessages() renders every row with no windowing and never runs the virtualized measure pass (_updateMessageVirtualMeasurements early-returns when !virtualized). Under @media (pointer: coarse), .msg-row[data-role="user"] carries content-visibility: auto; contain-intrinsic-size: auto 96px. Every rebuild does inner.innerHTML='' then recreates rows as fresh elements, so a fresh off-screen tall user row (a long paste measuring thousands of px) reserves only the flat estimate instead of its real height. scrollHeight shrinks by (realHeight - estimate), the browser force-clamps scrollTop, and the viewport jumps backward (a browser clamp, JS=none, so scrollTop-write compensation cannot catch it). #5638 fixed this for the virtualized wipe-and-rebuild path but left the non-virtualized full-rebuild path uncovered. Fix, three coordinated parts: - _estimateUserRowIntrinsicHeight weights CJK / full-width characters as ~2 columns (they wrap at ~24 chars/line, not 48), so a CJK paste reserves close to its real height even before it is ever measured. - _applyUserRowIntrinsicHeight reserves max(remembered, estimate): a remembered height can be a partial paint (a row taller than the viewport only paints its intersecting slice under content-visibility:auto), so the estimate floors it. - _rememberRenderedUserRowIntrinsicHeights, called pre-wipe inside renderMessages, reads the still-laid-out rows' real heights and persists them keyed by session-relative index, only for rows within the viewport (a fully off-screen never-painted row reports its collapsed reserve and must not poison the map), floored at the estimate. Desktop rests at content-visibility:visible so intrinsic-size is inert there; verified no behavior change with pointer:fine. Adds tests/test_issue5744_nonvirtual_userrow_collapse_jumpback.py (7 mutation-checked node-harness tests). Existing #5637/#5638 suites and the render/virtualization suites pass.
Contributor
|
| Filename | Overview |
|---|---|
| static/ui.js | Adds the non-virtualized user-row height capture and reserve path; stale positional height reuse and pre-guard layout reads need attention. |
| tests/test_issue5744_nonvirtual_userrow_collapse_jumpback.py | Adds focused Node-based regression tests for estimation, capture filtering, max reserve behavior, and render ordering. |
| CHANGELOG.md | Adds the release note for the mobile non-virtualized transcript scroll fix. |
Reviews (1): Last reviewed commit: "docs(changelog): #5751 non-virtualized t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // height can be a PARTIAL paint: a user row taller than the viewport that only ever had its | ||
| // top slice scrolled through content-visibility:auto reports just the painted portion, not | ||
| // its full height — persisting that would under-reserve and let scrollHeight collapse on the | ||
| // next rebuild (the jump-back). Taking the max means a good estimate floors the reserve even |
Contributor
There was a problem hiding this comment.
When a user message is edited shorter, or an earlier message is deleted and later rows shift raw indices, this cached sessionMsgIdx can now describe different content than the row that was measured. Because the rebuild takes Math.max(remembered, estimate), an old tall measurement keeps winning over the new text estimate and can leave extra reserved scroll height, blank space, and incorrect tail or preserved-scroll positioning.
| // clamps scrollTop → the jump-back. Reading pre-wipe (not post-render) is what makes the | ||
| // measurement reliable — the old elements have painted, so their rect height is real even | ||
| // off-screen; a post-render read of a fresh off-screen row returns its collapsed reserve. | ||
| if(typeof _rememberRenderedUserRowIntrinsicHeights==='function') _rememberRenderedUserRowIntrinsicHeights(); |
Contributor
There was a problem hiding this comment.
This pre-wipe capture performs getBoundingClientRect() reads before _programmaticScroll is set for the wipe. If that forced layout flushes a pending browser scroll event from the current streaming/render frame, the scroll listener can classify it as user intent and mark the transcript unpinned, so a reader who was following the tail can stop auto-following after the rebuild.
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
2 participants
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.
Release: non-virtualized transcript scroll jump-back (#5751)
Ships #5751 (@allenliang2022) — the mobile/coarse-pointer sibling of the #5637/#5638/#5742 scroll jump-back cluster.
What
With transcript virtualization off (#4325 opt-out),
renderMessages()rebuilds every row from scratch and never runs the virtualized measure pass. A fresh off-screen tall user row undercontent-visibility: auto(active only on coarse-pointer devices) reserved just its flatcontain-intrinsic-sizeplaceholder →scrollHeightshrank on re-render → browser clampedscrollTop→ viewport jumped toward the top. The fix reserves each user row's real height via three coordinated parts instatic/ui.js: pre-wipe capture of the still-laid-out rows' real heights, a CJK-aware estimate backstop for never-painted rows, and amax(remembered, estimate)floor so a partial paint can't under-reserve.Gate (all legs, on a stage rebased onto current master v0.51.922)
scrollTop,max(remembered, estimate)floor + in-viewport-only trust + tallest-wins verified.scrollTop: desktop (fix(webui): reserve real user-row height in non-virtualized transcripts to stop scroll jump-back #5751 inert —content-visibilityis coarse-pointer-only), Android (fix(webui): compensate stale snapshot.top on desktop live-refresh restore (#5637 follow-up) #5742's realign branch unreachable), iOS (they compose sequentially — fix(webui): reserve real user-row height in non-virtualized transcripts to stop scroll jump-back #5751 sets geometry during rebuild, fix(webui): compensate stale snapshot.top on desktop live-refresh restore (#5637 follow-up) #5742 measures residual after; double-compensation structurally impossible). Defense-in-depth.node --check+ scope-undef gate clean.Note
Ships on the gate per the maintainer's nod — the repro needs a long coarse-pointer transcript, so there's no static before/after; the contributor's mutation-checked harness + before/after measurement table stands in (same trust-the-gate call as #5742).
Attribution
Commit authored by @allenliang2022, cherry-picked onto current master. Credited in CHANGELOG.
Closes #5751.