fix(chat): prevent first response jump from snapping back - #6621
Conversation
Gate certification — RED ⛔Exact contributor head: Threat scan is CLEAN (score 0). The focused JS/scope/ruff slice passed 6/6 and The exact head is nevertheless not ship-ready because it introduces a silent active-session refresh dead state:
I reproduced this independently three ways:
Full serial sandbox suite: candidate 13,829 passed / 8 failed / 2 errors; exact frozen-master control 13,828 passed / the identical 8 failed / 2 errors. The candidate adds its one passing regression and has zero suite-failure delta, but the suite remains honestly non-green; the independently reproduced product blocker above is PR-owned. Required fix
A substantive re-push needs a fresh exact-head full gate. No merge, tag, deploy, close, or contributor-branch write was performed. |
|
Implemented the requested destination-aware ownership fix in What changed
Verification on exact head
Full local serial suite result was 13,863 passed / 123 skipped / 1 xfailed / 2 xpassed / 1 failed. The one failure was |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate — changes requested
Exact contributor head: 5c2b9e212b7fa5c4eb46af3aaf92be02980e4430
The re-push fixes the synchronous destination predicate from the prior gate, but the same stale-refresh state is still reachable through the real smooth-scroll lifecycle.
Blocking finding
jumpToTurnQuestion() now correctly avoids setting sticky reader ownership when its predicted, clamped destination is within 80px of the tail. It then calls scrollIntoView({behavior:'smooth'}) without marking that animation as programmatic or otherwise owning its scroll events.
The production #messages scroll listener subsequently sees an upward animation frame as movedUp and sets:
_scrollPinned = false;
_messageUserUnpinned = true;
_nearBottomCount = 0;That recreates the prior dead state at a near-tail destination: the bottom control remains hidden because the distance is <=80, while refreshActiveSessionIfExternallyUpdated('idle-reconcile') defers because _messageUserUnpinned is true.
I reproduced this with a reviewer-owned composed harness using the exact production jumpToTurnQuestion, scroll-listener RAF body, and active-session refresh guard. At a 79px scroll range, the mandatory sandbox run failed with:
{"bottomDistance":79,"cancelCalls":2,"cueShown":false,"deferredReason":"idle-reconcile","messageUserUnpinned":true,"nearBottomCount":0,"refreshResult":"skipped","scrollPinned":false,"scrollTop":0}Threat scan: CLEAN. The contributed exact-head slices remain green (5 passed and 7 passed), but their scrollIntoView mock assigns scrollTop synchronously and dispatches no production scroll-listener lifecycle, so they cannot detect this regression.
Required fix
- Keep
_cancelBottomSettle()unconditional. - Give the response jump a generation/programmatic-scroll owner that covers the full native smooth-scroll lifecycle, so its animation frames cannot enter the listener's manual
movedUpbranch. - Reconcile the actual final clamped destination after scrolling/layout. Preserve the pre-jump sticky state at
<=80px; claim reader ownership only when the resolved destination is>80pxfrom the tail. - Do the same reconciliation for the virtualized estimate-to-render path rather than permanently claiming from the pre-render estimate.
- Add a regression that drives visible assistant, visible user-row, and virtualized jumps through the production scroll listener before calling the production refresh guard. Cover
0/79/80/81/>80px,_nearBottomCount, and bottom-control visibility.
No merge, tag, deploy, close, or contributor-branch write was performed.
|
Addressed the latest smooth-scroll lifecycle review in
Verification on the exact head:
Please re-gate this exact head when ready. |
🎬 Cutter preview — PR #6621
|
a2069be to
0e528c0
Compare
|
| Filename | Overview |
|---|---|
| static/ui.js | Coordinates bottom-settle cancellation and temporary scroll ownership so response jumps retain their destination across rendering and streaming updates. |
| tests/test_jump_to_answer_scroll_settle.py | Adds focused Node-backed coverage for stale settle callbacks, smooth-scroll reconciliation, user takeover, session changes, and streaming interactions. |
| tests/test_issue2246_question_jump.py | Updates source-level assertions for the virtualized response-jump path. |
| tests/test_issue4856_android_scroll_regression.py | Expands the inspected source range to accommodate the updated message-scroll intent logic. |
Reviews (2): Last reviewed commit: "fix(scroll): widen jump-owner takeover t..." | Re-trigger Greptile
| const virtualTarget=clampTargetScrollTop(_messageVirtualScrollTopForVisibleIdx(visWithIdx, visibleIdx, container)); | ||
| container.scrollTop=virtualTarget; |
There was a problem hiding this comment.
Virtualized middle targets stay unmounted
When a response lies in the unrendered middle of a long virtualized transcript, clamping its full-transcript offset to the current DOM range makes the preserve-scroll renders continue selecting windows that exclude the requested row, leaving the viewport at the bottom instead of jumping to that response.
Cancel pending load-time bottom settling before an explicit response jump takes scroll ownership. Add a regression test covering the first-click race.
Native smooth-scroll frames from response jumps could reach the manual scroll listener and claim reader ownership before the final destination was known. - own response-jump scrolling with a generation- and session-scoped lifecycle - reconcile sticky ownership from final 80px tail geometry - preserve current low-delta wheel takeover semantics when integrating with master - cover visible assistant, user-row, and virtualized paths through the production scroll listener
…n explicit End (nesquena#6621 gate fixes) Codex gate found two defects in the jump-scroll ownership mechanism: - Jump during an active stream snapped back to bottom on the next token: the owner suppressed the scroll listener but left the pre-jump pinned state, so scrollIfPinned() reclaimed the bottom. Now _beginMessageJumpScroll unpins for the ownership window and scrollIfPinned() no-ops while an owner exists. - An explicit End click could be undone by the pending jump reconcile restoring the stale unpinned snapshot. scrollToBottom() now cancels the active jump owner first; _cancelMessageJumpScroll restores the preserved snapshot so a non-reconcile cancel doesn't leak the transient unpinned state.
…ionId (nesquena#6621 brick-class scope-undef gate) The PR's _messageJumpSessionId() referenced a bare 'currentSid' global that does not exist in ui.js (everywhere else it's a local const from S.session.session_id). test_static_js_scope_undef flagged it brick-class (nesquena#3696). The S.session.session_id fallback already IS the canonical accessor; removed the dead first line and updated the harness to drive the session-change case via S.session.session_id.
…quena#6621) Re-gate found three more state-transition edge cases from the temp-unpin window: - wheel-up interrupting an active jump owner after the programmatic latch expires now explicitly establishes the unpinned reader-owned state (was cancelling the jump but leaving pinned -> next token snapped to bottom). - _resetStreamScrollFollow() now cancels the jump owner FIRST, before its pinned-state assignments, so a stream starting mid-jump can't have its pin undone by the snapshot restore -> auto-follow stays enabled. - _finishMessageJumpScroll() flushes a deferred external-session refresh after reconciliation when the terminal state is pinned to the tail, so a refresh deferred during the temp-unpin window isn't stranded.
…indow (nesquena#6621) The two _messageJumpScrollOwner guards (scrollIfPinned, scrollToBottom) are pulled into other scroll test harnesses (test_issue6414, test_issue4856) that stub the scroll env without declaring the new global; bare references threw ReferenceError. Guard with typeof (matches the PR's own jumpScrollOwned check). Also widen test_low_delta_wheel_intent_is_tracked_separately's source-slice window 1400->1800 to still contain the (unchanged) _lastMessageWheelIntentMs line after the +9-line wheel-up-during-jump block was inserted.
… tests (nesquena#6621 Fable finding ii) Fable UX gate flagged that the PR's tests model only the stale load-time settle callback, not the streaming case. Add two node-harness tests: - a streaming render frame (scrollIfPinned) fired inside the jump-owner window must not snap the reader to the bottom (holds at target, 0 bottom-writes). - a gentle wheel-up during the owner window after the programmatic latch stales hands ownership to the reader UNPINNED at their position, never pinned mid-transcript.
…quena#6621 Fable S3) Fable's re-review (on the fixed code) confirmed the streaming blocker + wheel-up concern resolved, and probe-proved one remaining narrow regression: a gentle wheel-DOWN or touch scroll during the owner window cancelled the owner and restored the pinned snapshot but did NOT re-unpin (the takeover was gated on wheelUp only), so the next streaming token yanked the reader to the bottom. Widen the takeover to any message-pane scroll input during the owner window. Add a wheel-down regression test; widen the nesquena#4970 static-slice window to 2000 to still contain the (unchanged) _lastMessageWheelIntentMs line.
0e528c0 to
3e9460c
Compare
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gated on the rebased head (3e9460c, rebased onto current master — the stale-base #6626-revert + CHANGELOG drift are gone; net diff is now static/ui.js +156 plus the scroll-settle tests only). Codex semantic-rebase pass: SAFE TO SHIP (jump ownership session-scoped, no currentSid/global conflict, all release/cancel transitions correct, 224 focused assertions). Full suite 14536 passed / 0 failed. This is on top of the prior 3 Codex rounds + Fable UX SHIP + maintainer screenshot verification. Superseding the earlier CHANGES_REQUESTED — all findings resolved. Shipping to the experimental channel.
|
Shipped in exp-v0.52.218, thanks @pxxD1998 🎉 The first-response jump now takes ownership of the scroll position and holds it where you put it, releasing only on an explicit downward scroll, End, or session switch — no more snap-back to the bottom while the answer streams in. Normal bottom-following (when you haven't jumped) is unchanged. Gate: I rebased onto current master, then re-gated — Codex SAFE TO SHIP (jump ownership session-scoped, all release/cancel transitions correct, 224 focused assertions), full suite 14536/0. This was on top of the earlier deep-review rounds (Codex + Fable UX + maintainer screenshot verification across wide/desktop/tablet/mobile). Appreciate your patience through the iterations. |
) * fix(chat): keep response jump position after session load Cancel pending load-time bottom settling before an explicit response jump takes scroll ownership. Add a regression test covering the first-click race. * fix: gate response jump ownership by destination * fix: keep response jumps programmatic through smooth scroll Native smooth-scroll frames from response jumps could reach the manual scroll listener and claim reader ownership before the final destination was known. - own response-jump scrolling with a generation- and session-scoped lifecycle - reconcile sticky ownership from final 80px tail geometry - preserve current low-delta wheel takeover semantics when integrating with master - cover visible assistant, user-row, and virtualized paths through the production scroll listener * fix(scroll): hold reader off-bottom during jump owner + cancel jump on explicit End (nesquena#6621 gate fixes) Codex gate found two defects in the jump-scroll ownership mechanism: - Jump during an active stream snapped back to bottom on the next token: the owner suppressed the scroll listener but left the pre-jump pinned state, so scrollIfPinned() reclaimed the bottom. Now _beginMessageJumpScroll unpins for the ownership window and scrollIfPinned() no-ops while an owner exists. - An explicit End click could be undone by the pending jump reconcile restoring the stale unpinned snapshot. scrollToBottom() now cancels the active jump owner first; _cancelMessageJumpScroll restores the preserved snapshot so a non-reconcile cancel doesn't leak the transient unpinned state. * fix(scroll): drop undefined currentSid global ref in _messageJumpSessionId (nesquena#6621 brick-class scope-undef gate) The PR's _messageJumpSessionId() referenced a bare 'currentSid' global that does not exist in ui.js (everywhere else it's a local const from S.session.session_id). test_static_js_scope_undef flagged it brick-class (nesquena#3696). The S.session.session_id fallback already IS the canonical accessor; removed the dead first line and updated the harness to drive the session-change case via S.session.session_id. * fix(scroll): round-2 gate fixes for jump-owner state transitions (nesquena#6621) Re-gate found three more state-transition edge cases from the temp-unpin window: - wheel-up interrupting an active jump owner after the programmatic latch expires now explicitly establishes the unpinned reader-owned state (was cancelling the jump but leaving pinned -> next token snapped to bottom). - _resetStreamScrollFollow() now cancels the jump owner FIRST, before its pinned-state assignments, so a stream starting mid-jump can't have its pin undone by the snapshot restore -> auto-follow stays enabled. - _finishMessageJumpScroll() flushes a deferred external-session refresh after reconciliation when the terminal state is pinned to the tail, so a refresh deferred during the temp-unpin window isn't stranded. * fix(scroll): make jump-owner guards typeof-safe + widen static test window (nesquena#6621) The two _messageJumpScrollOwner guards (scrollIfPinned, scrollToBottom) are pulled into other scroll test harnesses (test_issue6414, test_issue4856) that stub the scroll env without declaring the new global; bare references threw ReferenceError. Guard with typeof (matches the PR's own jumpScrollOwned check). Also widen test_low_delta_wheel_intent_is_tracked_separately's source-slice window 1400->1800 to still contain the (unchanged) _lastMessageWheelIntentMs line after the +9-line wheel-up-during-jump block was inserted. * test(scroll): add streaming-frame-hold + wheel-during-jump regression tests (nesquena#6621 Fable finding ii) Fable UX gate flagged that the PR's tests model only the stale load-time settle callback, not the streaming case. Add two node-harness tests: - a streaming render frame (scrollIfPinned) fired inside the jump-owner window must not snap the reader to the bottom (holds at target, 0 bottom-writes). - a gentle wheel-up during the owner window after the programmatic latch stales hands ownership to the reader UNPINNED at their position, never pinned mid-transcript. * fix(scroll): widen jump-owner takeover to downward + touch input (nesquena#6621 Fable S3) Fable's re-review (on the fixed code) confirmed the streaming blocker + wheel-up concern resolved, and probe-proved one remaining narrow regression: a gentle wheel-DOWN or touch scroll during the owner window cancelled the owner and restored the pinned snapshot but did NOT re-unpin (the takeover was gated on wheelUp only), so the next streaming token yanked the reader to the bottom. Widen the takeover to any message-pane scroll input during the owner window. Add a wheel-down regression test; widen the nesquena#4970 static-slice window to 2000 to still contain the (unchanged) _lastMessageWheelIntentMs line. --------- Co-authored-by: pxxD1998 <214340659+pxxD1998@users.noreply.github.com> Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
nesquena#7011) Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
) * fix(chat): keep response jump position after session load Cancel pending load-time bottom settling before an explicit response jump takes scroll ownership. Add a regression test covering the first-click race. * fix: gate response jump ownership by destination * fix: keep response jumps programmatic through smooth scroll Native smooth-scroll frames from response jumps could reach the manual scroll listener and claim reader ownership before the final destination was known. - own response-jump scrolling with a generation- and session-scoped lifecycle - reconcile sticky ownership from final 80px tail geometry - preserve current low-delta wheel takeover semantics when integrating with master - cover visible assistant, user-row, and virtualized paths through the production scroll listener * fix(scroll): hold reader off-bottom during jump owner + cancel jump on explicit End (nesquena#6621 gate fixes) Codex gate found two defects in the jump-scroll ownership mechanism: - Jump during an active stream snapped back to bottom on the next token: the owner suppressed the scroll listener but left the pre-jump pinned state, so scrollIfPinned() reclaimed the bottom. Now _beginMessageJumpScroll unpins for the ownership window and scrollIfPinned() no-ops while an owner exists. - An explicit End click could be undone by the pending jump reconcile restoring the stale unpinned snapshot. scrollToBottom() now cancels the active jump owner first; _cancelMessageJumpScroll restores the preserved snapshot so a non-reconcile cancel doesn't leak the transient unpinned state. * fix(scroll): drop undefined currentSid global ref in _messageJumpSessionId (nesquena#6621 brick-class scope-undef gate) The PR's _messageJumpSessionId() referenced a bare 'currentSid' global that does not exist in ui.js (everywhere else it's a local const from S.session.session_id). test_static_js_scope_undef flagged it brick-class (nesquena#3696). The S.session.session_id fallback already IS the canonical accessor; removed the dead first line and updated the harness to drive the session-change case via S.session.session_id. * fix(scroll): round-2 gate fixes for jump-owner state transitions (nesquena#6621) Re-gate found three more state-transition edge cases from the temp-unpin window: - wheel-up interrupting an active jump owner after the programmatic latch expires now explicitly establishes the unpinned reader-owned state (was cancelling the jump but leaving pinned -> next token snapped to bottom). - _resetStreamScrollFollow() now cancels the jump owner FIRST, before its pinned-state assignments, so a stream starting mid-jump can't have its pin undone by the snapshot restore -> auto-follow stays enabled. - _finishMessageJumpScroll() flushes a deferred external-session refresh after reconciliation when the terminal state is pinned to the tail, so a refresh deferred during the temp-unpin window isn't stranded. * fix(scroll): make jump-owner guards typeof-safe + widen static test window (nesquena#6621) The two _messageJumpScrollOwner guards (scrollIfPinned, scrollToBottom) are pulled into other scroll test harnesses (test_issue6414, test_issue4856) that stub the scroll env without declaring the new global; bare references threw ReferenceError. Guard with typeof (matches the PR's own jumpScrollOwned check). Also widen test_low_delta_wheel_intent_is_tracked_separately's source-slice window 1400->1800 to still contain the (unchanged) _lastMessageWheelIntentMs line after the +9-line wheel-up-during-jump block was inserted. * test(scroll): add streaming-frame-hold + wheel-during-jump regression tests (nesquena#6621 Fable finding ii) Fable UX gate flagged that the PR's tests model only the stale load-time settle callback, not the streaming case. Add two node-harness tests: - a streaming render frame (scrollIfPinned) fired inside the jump-owner window must not snap the reader to the bottom (holds at target, 0 bottom-writes). - a gentle wheel-up during the owner window after the programmatic latch stales hands ownership to the reader UNPINNED at their position, never pinned mid-transcript. * fix(scroll): widen jump-owner takeover to downward + touch input (nesquena#6621 Fable S3) Fable's re-review (on the fixed code) confirmed the streaming blocker + wheel-up concern resolved, and probe-proved one remaining narrow regression: a gentle wheel-DOWN or touch scroll during the owner window cancelled the owner and restored the pinned snapshot but did NOT re-unpin (the takeover was gated on wheelUp only), so the next streaming token yanked the reader to the bottom. Widen the takeover to any message-pane scroll input during the owner window. Add a wheel-down regression test; widen the nesquena#4970 static-slice window to 2000 to still contain the (unchanged) _lastMessageWheelIntentMs line. --------- Co-authored-by: pxxD1998 <214340659+pxxD1998@users.noreply.github.com> Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
nesquena#7011) Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>

Thinking Path
What Changed
jumpToTurnQuestion().Why It Matters
The first click on Jump to response should be reliable. Previously, readers could see the correct response start briefly and then be returned to the bottom, while later clicks appeared to work because the stale load-time settle had already expired.
Contract Routing
Verification
Regression proof
On current
origin/master, before the fix:After the fix:
Command:
Additional checks:
Real browser check
The browser-loaded
jumpToTurnQuestion()was compared byte-for-byte with this candidate before the check.Before / after interaction evidence
Each recording is a synchronized side-by-side comparison: left = before (
0a401597594575d5650a755d1228b7de5a87544e), right = this PR (2015ab9c2b15a1cfdba956ec386a79dedece1952). Both sides start at the bottom and perform the same first Jump to response action. The before side returns to the bottom after the load-time settle runs; the fixed side retains the response start.The harness clicked 375 ms after the real control became visible, then sampled the DOM at 590 ms and 2,690 ms after the click. It used eight synthetic messages, separate state roots and ports for each revision, and no provider or LLM call. The red/green status labels are evidence-only overlays injected by the recording harness; they are not product UI.
Desktop — 1440 × 900
Narrow — 900 × 900
Mobile — 430 × 860, touch enabled
All six source recordings passed the behavioral verdict, full media decode, sampled-frame review, and privacy review; no meaningful browser console errors were recorded.
The full test suite was not run locally; the focused and neighboring 53-test set above was run, and the complete Python-version matrix is left to repository CI.
Risks / Follow-ups
Documentation
No documentation change is needed. This restores the existing response-jump contract without changing controls, setup, configuration, public APIs, or the intended interaction flow.
Release Note
Fixed the first Jump to response click after loading a session so it no longer snaps back to the bottom.
Model Used
AI-assisted implementation and verification:
gpt-5.6-solhigh