fix(chat): suppress post-render scroll artifacts - #4970
allenliang2022 wants to merge 6 commits into
Conversation
|
@nesquena-hermes Follow-up PR for another mobile/streaming scroll artifact after normal render/send paths. Suggested labels: |
|
| Filename | Overview |
|---|---|
| static/ui.js | Adds render-artifact timing and user-intent checks so real transcript scrolling can still unpin live follow. |
| tests/test_issue4856_android_scroll_regression.py | Adds regression coverage for render-artifact suppression, wheel intent, keyboard intent, scrollbar dragging, and reset behavior. |
Reviews (6): Last reviewed commit: "fix(chat): do not treat Space on transcr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks @allenliang2022 — this is a sensible follow-up to your #4934 DOM-wipe clamp fix (suppressing the short-lived post-render upward scroll only when there's no recent wheel/touch intent). One CI blocker, and it's the same harness-injection shape you already solved on #4934: 🔴 CI red —
|
nesquena-hermes
added
the
changes-requested
…rness passes The new movedUp suppression branch called _recentMessageRenderArtifactWindow() and the intent helpers in an unguarded position. The nesquena#4295 unit harness injects the scroll-listener body via new Function() without those helpers, so the branch threw ReferenceError on a movedUp sample and crashed the node subprocess. Reorder the && chain with typeof-function guards first; production evaluates the real helpers, the harness short-circuits before any call. Behavior unchanged.
|
Thanks @allenliang2022 — the harness CI is green now and the suppression idea is right. But the dual gate (Codex, reproduced) found a real behavior bug: the suppression can swallow a genuine trackpad scroll for ~1.4s after a render. 🔴 MUST-FIX — small-delta upward wheel scrolls get swallowed in the post-render window (
|
1 similar comment
|
Thanks @allenliang2022 — the harness CI is green now and the suppression idea is right. But the dual gate (Codex, reproduced) found a real behavior bug: the suppression can swallow a genuine trackpad scroll for ~1.4s after a render. 🔴 MUST-FIX — small-delta upward wheel scrolls get swallowed in the post-render window (
|
…esquena#4970 review) Maintainer dual-gate (Codex) found the suppression could swallow a genuine low-delta trackpad wheel scroll-up for ~1.4s after a render: _recordNonMessageScrollIntent() only recorded message-pane wheel intent at deltaY<-30, so a gentle deltaY:-5 left both intent helpers false and the post-render branch returned before movedUp set _messageUserUnpinned. - Track recent low-delta upward message-pane wheel intent separately (_lastMessageWheelIntentMs / _recentMessageWheelIntent), recorded for any upward wheel (deltaY<0). The decisive deltaY<-30 sticky-unpin is unchanged. - Require !_recentMessageWheelIntent() before suppressing the artifact, so a real gentle scroll-up inside the window still unpins. - Add behavioral node-harness regressions: gentle wheel inside the window unpins; no-intent artifact inside the window stays suppressed; outside the window unpins. Plus a source lock that low-delta intent is tracked.
|
@nesquena-hermes Fixed — thanks for the Codex catch, that's a real swallow and the repro is exactly right. Root cause: Fix (Codex's exact recommendation):
Behavioral regressions added (node-harness, runs the extracted scroll listener):
Local: |
|
Good — the low-delta trackpad intent tracking is the right fix and the original swallow is resolved (the executed behavioral tests confirm a gentle 🔴 MUST-FIX 1 — stale wheel intent crosses session switches (
|
…sion on scrollbar drag (nesquena#4970 review) Maintainer dual-gate found two stale-state leaks in the new _lastMessageWheelIntentMs and one adjacent pre-existing gap; all three fixed: MUST-FIX 1 — _resetScrollDirectionTracker() (session switch) did not clear _lastMessageWheelIntentMs, so a gentle wheel in chat A left _recentMessageWheelIntent() true into chat B's first post-render window, under-suppressing the artifact and falsely unpinning. Now reset to -Infinity. MUST-FIX 2 — _resetStreamScrollFollow() (fresh stream) had the same leak: a gentle upward wheel within 1200ms of a new stream could silently disable live follow. Now reset to -Infinity. SHOULD-FIX 3 — the suppression branch ignored _scrollbarDragActive, so a manual scrollbar-drag upward scroll inside the 1400ms window was swallowed. Gate the branch on (typeof _scrollbarDragActive==='undefined' || !_scrollbarDragActive); typeof guard keeps the nesquena#4295 node harness inert. Tests: scrollbar-drag-inside-window-still-unpins behavioral regression (harness extended with injected _scrollbarDragActive), plus source locks for both resets and the scrollbar-drag gate. test_4856 + test_4295 green (17 passed), node --check clean.
|
@nesquena-hermes All three landed — thanks, both leaks and the scrollbar gap reproduced exactly as described. 🔴 MUST-FIX 1 — stale wheel intent across session switch
🔴 MUST-FIX 2 — stale wheel intent across fresh stream start
🟡 SHOULD-FIX 3 — scrollbar-drag swallowed in the windowThe suppression branch now gates on Regressions added
Local: |
… greptile P1) Keyboard scrolling of the message pane (PageUp/PageDown, Arrow keys, Space, Home/End) fires a native scroll event with no wheel/touch/scrollbar/non-message intent. Inside the 1400ms post-render artifact window the suppression branch then returned before movedUp could unpin, so a keyboard scroll-up was swallowed and live-follow snapped the reader back to the bottom. - Add _lastMessageKeyScrollIntentMs + _recentMessageKeyScrollIntent(), stamped by a capture-phase keydown listener on the scroll keys, gated to when the message pane is the scroll target (focused/contains focus/hovered) and not an editable field (composer/input/contenteditable). - Gate the post-render suppression on !_recentMessageKeyScrollIntent() (typeof guard keeps the nesquena#4295 node harness inert). - Clear the stamp in both _resetScrollDirectionTracker() and _resetStreamScrollFollow() (same stale-state hygiene as the wheel stamp). Tests: keyboard-scroll-inside-window-still-unpins behavioral regression (harness extended with injected _recentMessageKeyScrollIntent), plus a source lock for the helper/keydown-stamp/suppression-gate/both-resets. test_4856 + test_4295 green (19 passed), node --check clean.
|
Addressed the greptile P1 — keyboard scrolls swallowed (and the related scrollbar shape from the prior pass). Root cause: keyboard scrolling of the message pane (PageUp/PageDown, Arrow keys, Space, Home/End) fires a native Fix (positively record the intent, per the maintainer's "mark these real scroll paths as user intent" guidance):
Regressions added:
Local: |
nesquena-hermes
added
size:L
|
The keyboard-intent addition is the right completeness fix, and the composer exclusion is correct (Codex verified 🟡 MUST-FIX — Space on a focused transcript control wrongly stamps scroll intent (
|
…nesquena#4970 review) Maintainer/Codex found one narrow edge in the keyboard intent stamp: because the listener runs capture-phase and accepts any focused descendant of #messages, Space/Spacebar on an in-transcript control (tool-card toggles, copy buttons, role buttons, links/tabs) stamped _lastMessageKeyScrollIntentMs before the control handler could preventDefault/stopPropagation. That made a button activation look like a scroll intent and could mask a legitimate post-render artifact. Fix by excluding Space/Spacebar when the event target or active element is an interactive transcript control: button, a[href], select, summary, role=button, role=tab, role=menuitem, or contenteditable. Keep PageUp/PageDown/arrows/Home/End stamping unchanged, and keep the existing INPUT/TEXTAREA/contenteditable composer exclusion. Add a behavioral node-harness test: Space on a focused transcript button leaves the key-scroll stamp at -Infinity (serialized null), while PageUp on the pane still stamps 1234. test_4856 + test_4295 green (20 passed), node --check clean.
|
@nesquena-hermes Fixed the Space-on-control edge — good catch, the capture-phase point is exactly the issue. Root cause: the keyboard intent listener ran in capture phase and accepted any focused descendant of Fix:
Regression: behavioral node-harness test extracts the real keydown listener region and verifies:
Local: |
|
Shipped in v0.51.678 (Release YH, just deployed) — thanks @allenliang2022, and real appreciation for the persistence through 5 rounds. The post-render scroll-artifact suppression now covers every intent modality (wheel incl low-delta trackpad, touch, scrollbar drag, keyboard) without swallowing a real scroll, clears intent state on session-switch + stream-reset, and excludes Space-on-a-focused-transcript-control. Gate: Codex SAFE, full suite 10689. Verified on prod. |
…esquena#4970 review) Maintainer dual-gate (Codex) found the suppression could swallow a genuine low-delta trackpad wheel scroll-up for ~1.4s after a render: _recordNonMessageScrollIntent() only recorded message-pane wheel intent at deltaY<-30, so a gentle deltaY:-5 left both intent helpers false and the post-render branch returned before movedUp set _messageUserUnpinned. - Track recent low-delta upward message-pane wheel intent separately (_lastMessageWheelIntentMs / _recentMessageWheelIntent), recorded for any upward wheel (deltaY<0). The decisive deltaY<-30 sticky-unpin is unchanged. - Require !_recentMessageWheelIntent() before suppressing the artifact, so a real gentle scroll-up inside the window still unpins. - Add behavioral node-harness regressions: gentle wheel inside the window unpins; no-intent artifact inside the window stays suppressed; outside the window unpins. Plus a source lock that low-delta intent is tracked.
…sion on scrollbar drag (nesquena#4970 review) Maintainer dual-gate found two stale-state leaks in the new _lastMessageWheelIntentMs and one adjacent pre-existing gap; all three fixed: MUST-FIX 1 — _resetScrollDirectionTracker() (session switch) did not clear _lastMessageWheelIntentMs, so a gentle wheel in chat A left _recentMessageWheelIntent() true into chat B's first post-render window, under-suppressing the artifact and falsely unpinning. Now reset to -Infinity. MUST-FIX 2 — _resetStreamScrollFollow() (fresh stream) had the same leak: a gentle upward wheel within 1200ms of a new stream could silently disable live follow. Now reset to -Infinity. SHOULD-FIX 3 — the suppression branch ignored _scrollbarDragActive, so a manual scrollbar-drag upward scroll inside the 1400ms window was swallowed. Gate the branch on (typeof _scrollbarDragActive==='undefined' || !_scrollbarDragActive); typeof guard keeps the nesquena#4295 node harness inert. Tests: scrollbar-drag-inside-window-still-unpins behavioral regression (harness extended with injected _scrollbarDragActive), plus source locks for both resets and the scrollbar-drag gate. test_4856 + test_4295 green (17 passed), node --check clean.
… greptile P1) Keyboard scrolling of the message pane (PageUp/PageDown, Arrow keys, Space, Home/End) fires a native scroll event with no wheel/touch/scrollbar/non-message intent. Inside the 1400ms post-render artifact window the suppression branch then returned before movedUp could unpin, so a keyboard scroll-up was swallowed and live-follow snapped the reader back to the bottom. - Add _lastMessageKeyScrollIntentMs + _recentMessageKeyScrollIntent(), stamped by a capture-phase keydown listener on the scroll keys, gated to when the message pane is the scroll target (focused/contains focus/hovered) and not an editable field (composer/input/contenteditable). - Gate the post-render suppression on !_recentMessageKeyScrollIntent() (typeof guard keeps the nesquena#4295 node harness inert). - Clear the stamp in both _resetScrollDirectionTracker() and _resetStreamScrollFollow() (same stale-state hygiene as the wheel stamp). Tests: keyboard-scroll-inside-window-still-unpins behavioral regression (harness extended with injected _recentMessageKeyScrollIntent), plus a source lock for the helper/keydown-stamp/suppression-gate/both-resets. test_4856 + test_4295 green (19 passed), node --check clean.
…ll intent modalities (nesquena#4970)
|
Shipped in v0.51.678 (Release YH) — thanks @allenliang2022! Your post-render scroll-artifact suppression (across wheel/trackpad/touch/scrollbar-drag/keyboard intent, with the session/stream-reset clears and the Space-on-transcript-control exclusion) is live in master — verified the symbols are deployed. This PR is from your fork so it didn't auto-close; closing as shipped with credit. Appreciate the persistence through the 5 review rounds. |
A new post-nesquena#4970 scroll-jump class surfaced after Release YH: when a streamed assistant turn with a large live worklog/tool trace settles, the settled compact worklog collapses hundreds of pixels at STREAM_DONE. The reader is still pinned and scroll state is correct, but the net scrollHeight shrink makes the browser clamp scrollTop down by the same amount, which is visible as a large backward jump. Fix by keeping the just-settled activity worklog open for pinned followers (_scrollPinned && !_messageUserUnpinned), so the live->settled DOM swap is height-stable. Unpinned readers still get compact settled worklogs and keep their viewport normally. Also make _anchorSceneWorklogGroup respect opts.collapsed; previously it hard-coded collapsed: !live so the caller could not request an open settled group. Regression locks assert the pinned-follow helper, that settled rendering passes collapsed:!keepSettledWorklogOpen, and that _anchorSceneWorklogGroup honors an explicit opts.collapsed. Verified locally with a Playwright repro: before the fix a large tool-worklog answer produced scrollHeight -367/-422px and scrollTop -367/-422px at STREAM_DONE; after the fix nShrinks=0 and nBack=0. Local focused scroll locks pass: 22 passed.
A new post-nesquena#4970 scroll-jump class surfaced after Release YH: when a streamed assistant turn with a large live worklog/tool trace settles, the settled compact worklog collapses hundreds of pixels at STREAM_DONE. The reader is still pinned and scroll state is correct, but the net scrollHeight shrink makes the browser clamp scrollTop down by the same amount, which is visible as a large backward jump. Fix by keeping the just-settled activity worklog open for pinned followers (_scrollPinned && !_messageUserUnpinned), so the live->settled DOM swap is height-stable. Unpinned readers still get compact settled worklogs and keep their viewport normally. Also make _anchorSceneWorklogGroup respect opts.collapsed; previously it hard-coded collapsed: !live so the caller could not request an open settled group. Regression locks assert the pinned-follow helper, that settled rendering passes collapsed:!keepSettledWorklogOpen, and that _anchorSceneWorklogGroup honors an explicit opts.collapsed. Verified locally with a Playwright repro: before the fix a large tool-worklog answer produced scrollHeight -367/-422px and scrollTop -367/-422px at STREAM_DONE; after the fix nShrinks=0 and nBack=0. Local focused scroll locks pass: 22 passed.
A new post-nesquena#4970 scroll-jump class surfaced after Release YH: when a streamed assistant turn with a large live worklog/tool trace settles, the settled compact worklog collapses hundreds of pixels at STREAM_DONE. The reader is still pinned and scroll state is correct, but the net scrollHeight shrink makes the browser clamp scrollTop down by the same amount, which is visible as a large backward jump. Fix by keeping the just-settled activity worklog open for pinned followers (_scrollPinned && !_messageUserUnpinned), so the live->settled DOM swap is height-stable. Unpinned readers still get compact settled worklogs and keep their viewport normally. Also make _anchorSceneWorklogGroup respect opts.collapsed; previously it hard-coded collapsed: !live so the caller could not request an open settled group. Regression locks assert the pinned-follow helper, that settled rendering passes collapsed:!keepSettledWorklogOpen, and that _anchorSceneWorklogGroup honors an explicit opts.collapsed. Verified locally with a Playwright repro: before the fix a large tool-worklog answer produced scrollHeight -367/-422px and scrollTop -367/-422px at STREAM_DONE; after the fix nShrinks=0 and nBack=0. Local focused scroll locks pass: 22 passed.
…obile 往回大跳) The STREAM_DONE keep-open exception (nesquena#4970 round 6/7) was scoped to PINNED followers only, on the assumption that unpinned readers 'preserve their viewport normally'. That holds on desktop (overflow-anchor:none + JS snapshot restore) but NOT on mobile. Repro (isolated debug instance + CDP touch emulation, faithful to a real phone): a reader who scrolls UP to read inside the just-settled turn is UNPINNED. At STREAM_DONE the live worklog/thinking cards collapse into a compact summary, shrinking the transcript by hundreds of px ABOVE their viewport. On mobile the CSS resting value is overflow-anchor:auto, but _fixMobileScrollJank() flips an inline overflow-anchor:none over the settle render — so native scroll-anchoring is suppressed during exactly the frame the unpinned reader needs it to absorb the above-viewport shrink. The content then leaps to the top of the latest turn (the '往回大跳' report). Single-variable measurement: collapse-with-auto shifts the reader 0px; collapse-with-none shifts -248px. Fix: keep the just-settled worklog open for BOTH pin states (still one-shot scoped to the turn that just settled, so historical worklogs still collapse compact). This removes the shrink entirely, fixing the jump for every device/anchor-mode instead of fighting the anchor engine. The nesquena#4856 guard and the mobile overflow-anchor:auto CSS are untouched, so Boev's Android DOM-wipe fix and akrhin's #MOBILESCROLL fix both stay intact. - Rename _shouldKeepSettledWorklogOpenForPinnedFollow -> _shouldKeepSettledWorklogOpenForStreamSettle (drops the _scrollPinned / _messageUserUnpinned gate; keeps the one-shot stream-id token). - Update the nesquena#4970 behavioral regression test to assert keep-open is TRUE for the armed turn under BOTH pin states, FALSE for historical turns and after disarm.
…obile 往回大跳) The STREAM_DONE keep-open exception (nesquena#4970 round 6/7) was scoped to PINNED followers only, on the assumption that unpinned readers 'preserve their viewport normally'. That holds on desktop (overflow-anchor:none + JS snapshot restore) but NOT on mobile. Repro (isolated debug instance + CDP touch emulation, faithful to a real phone): a reader who scrolls UP to read inside the just-settled turn is UNPINNED. At STREAM_DONE the live worklog/thinking cards collapse into a compact summary, shrinking the transcript by hundreds of px ABOVE their viewport. On mobile the CSS resting value is overflow-anchor:auto, but _fixMobileScrollJank() flips an inline overflow-anchor:none over the settle render — so native scroll-anchoring is suppressed during exactly the frame the unpinned reader needs it to absorb the above-viewport shrink. The content then leaps to the top of the latest turn (the '往回大跳' report). Single-variable measurement: collapse-with-auto shifts the reader 0px; collapse-with-none shifts -248px. Fix: keep the just-settled worklog open for BOTH pin states (still one-shot scoped to the turn that just settled, so historical worklogs still collapse compact). This removes the shrink entirely, fixing the jump for every device/anchor-mode instead of fighting the anchor engine. The nesquena#4856 guard and the mobile overflow-anchor:auto CSS are untouched, so Boev's Android DOM-wipe fix and akrhin's #MOBILESCROLL fix both stay intact. - Rename _shouldKeepSettledWorklogOpenForPinnedFollow -> _shouldKeepSettledWorklogOpenForStreamSettle (drops the _scrollPinned / _messageUserUnpinned gate; keeps the one-shot stream-id token). - Update the nesquena#4970 behavioral regression test to assert keep-open is TRUE for the armed turn under BOTH pin states, FALSE for historical turns and after disarm.
test_issue4970_stream_done_shrink_regression asserted the literal `collapsed:!keepSettledWorklogOpen` in _renderSettledAnchorSceneForMessage. #5941 OR'd an errored-turn keep-open term into that expression (`collapsed:!(keepSettledWorklogOpen||erroredWorklogKeepOpen)`), so the substring no longer matched and the test failed in the full suite (the PR updated its own touched tests but missed this pre-existing one). Re-anchor to `collapsed:!(keepSettledWorklogOpen` — still guards #4970's invariant (keepSettledWorklogOpen negates into the settled-render collapsed flag) without pinning the exact term list. #4970 + #5941 suites green.
…y honors model pick (#5924) (#5964) * fix(streaming): keep errored-turn assistant response visible (#5941) An errored turn that produced assistant content (tool calls + reasoning) folded that content into a collapsed worklog above the error card, so the user read a lone error bubble as "nothing came back". The settled-scene renderer collapsed the worklog unconditionally, never consulting the turn terminal_state. Now an errored/failure terminal_state keeps the worklog expanded by default (unless the user explicitly collapsed it), while completed turns and genuinely-empty errored turns are unchanged. Reported by @b3nw. * fix(review): update #4970 brittle collapsed-expr assertion for #5941 test_issue4970_stream_done_shrink_regression asserted the literal `collapsed:!keepSettledWorklogOpen` in _renderSettledAnchorSceneForMessage. #5941 OR'd an errored-turn keep-open term into that expression (`collapsed:!(keepSettledWorklogOpen||erroredWorklogKeepOpen)`), so the substring no longer matched and the test failed in the full suite (the PR updated its own touched tests but missed this pre-existing one). Re-anchor to `collapsed:!(keepSettledWorklogOpen` — still guards #4970's invariant (keepSettledWorklogOpen negates into the settled-render collapsed flag) without pinning the exact term list. #4970 + #5941 suites green. * fix(chat): honor explicit model pick on post-failure recovery send (#5924) The onchange explicit-pick marker is single-shot: send() consumes it once. submitEdit() and cmdRetry() truncated and called send() directly without re-arming it, so the recovery send went out with explicit_model_pick=false and the server's compatible-model resolution re-reverted a freshly-picked cross-family model back to the failed/stale value (Facet 1). Facet 4 is the same loop keeping the persisted model_provider pinned across fork/refresh. Re-arm the pending explicit-pick marker from the current selector state immediately before await send() in both recovery paths. Survives a second consecutive recovery send. Normal send path and the #3737/#5731 server repair guard are unchanged. Reported by @b3nw. * fix(#5924): gate recovery re-arm on genuine pick + session-race guards Codex gate found 4 defects on the recovery send path; fixed all: - /retry + edit-resubmit re-armed the explicit-pick marker UNCONDITIONALLY, forcing explicit_model_pick even with no fresh pick (suppressed server compatible-model resolution). Now gated on _recoveryPick (selector model differs from session's own stored model), captured pre-await. - session-switch races: added active-session guards after the retry GET await and the edit truncate await so session A's recovery intent can't apply to session B. - 3 new regression tests (gated re-arm, pre-await capture, post-await re-guard). * fix(#5924) round-2: derive recovery pick from non-default session model, not inference Codex round-2 CORE: the state-comparison predicate false-negatived an already-applied pick (consumed marker → looks unchanged) and false-positived on provider inference (@removed:mistral-large + null stored provider → inferred 'removed' → fake change). Replace with _deliberateSessionModelPick(sid): reports {model,provider} only when the session's OWN model is genuinely non-default vs window._defaultModel/_activeProvider — inference-free + survives marker consumption. Both recovery paths + tests updated. * fix(#5924) round-3: require known-default+owned-provider evidence + fire-time re-arm guard Codex round-3: (1) _deliberateSessionModelPick still false-positived when the profile default was unknown or the provider was only inferred — now requires a session-OWNED provider AND a known window._defaultModel/_activeProvider, else fails closed. (2) new same-session race: a model change DURING the recovery awaits made the pre-await pick stale and clobbered a newer marker — new _reArmRecoveryPick helper re-arms only if the current session model/provider still equals the captured pick AND no different pending marker exists. Both recovery paths route through it. * Release exp-v0.52.45: errored-turn response stays visible (#5950/#5941) + recovery honors model pick (#5949/#5924) * fix: keep absoluteKeepCount capture before _recoveryPick in submitEdit The #5924 _recoveryPick comment contained the word 'await', tripping the pre-existing test_issue_edit_regenerate_absolute_keep_count regex (first \bawait\b must come after the absoluteKeepCount capture). Reorder both synchronous pre-network captures (absoluteKeepCount first) + reword the comment. No behavior change. --------- Co-authored-by: nesquena-hermes <agent@nesquena-hermes> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
…ertion + add behavioral coverage
- Fix test_stream_done_runs_scroll_preserving_collapse_pass_after_disarm:
assert _doneLiveScrollSnapshot is captured before arm, and the
argument-bearing _renderMessagesWithScrollSnapshot({_prescrollSnapshot:_doneLiveScrollSnapshot})
call is used after disarm (the no-arg literal no longer exists).
- Add test_prescroll_snapshot_bypasses_capture_no_option_fallback_still_captures:
Node.js behavioral harness that supplies a sentinel _prescrollSnapshot,
stubs _captureMessageScrollSnapshot with a counter, and proves:
1) sentinel reaches _restoreMessageScrollSnapshotSameFrame without a capture
2) no-option / empty-{} calls still capture normally (callers at
static/ui.js:9565, 14416, 14424).
…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.
* 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 (#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 (#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 (#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 (#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 (#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 (#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 (#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 #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>
…ertion + add behavioral coverage
- Fix test_stream_done_runs_scroll_preserving_collapse_pass_after_disarm:
assert _doneLiveScrollSnapshot is captured before arm, and the
argument-bearing _renderMessagesWithScrollSnapshot({_prescrollSnapshot:_doneLiveScrollSnapshot})
call is used after disarm (the no-arg literal no longer exists).
- Add test_prescroll_snapshot_bypasses_capture_no_option_fallback_still_captures:
Node.js behavioral harness that supplies a sentinel _prescrollSnapshot,
stubs _captureMessageScrollSnapshot with a counter, and proves:
1) sentinel reaches _restoreMessageScrollSnapshotSameFrame without a capture
2) no-option / empty-{} calls still capture normally (callers at
static/ui.js:9565, 14416, 14424).
) * 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>
) * 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>
2 participants
Summary
Root cause
This is a follow-up to the DOM-wipe clamp fix. That fix handles the scroll event produced during the
innerHTML=''rebuild window.There is a second class of artifacts: after a normal
renderMessages()completes (for example after sending a new message or during late layout settle), browsers can emit a follow-up upwardscrollevent even though the user did not wheel/touch the transcript. The listener seesmovedUpand marks_messageUserUnpinned=true, breaking live follow and making the page appear to jump backward after sending a reply.The fix records the most recent transcript render time and, for a short window, ignores upward scroll events that have no recent touch/wheel intent.
Verification
python -m pytest tests/test_issue4856_android_scroll_regression.py tests/test_issue4856_mobile_transcript_unscrollable.py -q -n 09 passedLocal reproduction evidence
Diagnostics captured several post-render artifacts distinct from the original DOM-wipe clamp:
im=Infinityindicates no recent input intent;rs=renderMessagesidentifies the render path; and these happen after ordinary render/send/settle paths, not just inside the immediate DOM-wipe clamp.After applying this follow-up locally (
scrollfix6artifact), a real WebUI send-path test on a mobile viewport hadjumps: []and keptbottomDistanceat0throughout the send/stream/settle sequence.