fix(scroll): ignore browser tail jitter that silently unpinned readers - #7268
ruizanthony wants to merge 11 commits into
Conversation
|
dca28f0 to
b1d2bd1
Compare
Fixed the CI failures from the first pushThe initial revision wrote the guard as an inlined IIFE inside the messages Root cause. Several harnesses slice the listener with Fix. The guard now lives at module scope as A regression test was added so this cannot come back: Re-verified after the refactor
|
b1d2bd1 to
638a42e
Compare
Second CI failure fixed — root cause was mine againMoving the guard to module scope fixed the brace-slicing harnesses, but broke a Shard 1 passed on all three Python versions simply because that shard does not Fix — apply the same pattern the surrounding code already uses for exactly const _tailJitter=typeof _isMessageTailJitter==='function'
&&_isMessageTailJitter(top,bottomDistance);Production behaviour is unchanged — the helper is always defined there. In the Verified on a clean
Apologies for the two rounds of CI noise: both failures were caused by this PR |
|
Addressed in |
6dfacc4 to
ea6adab
Compare
nesquena-hermes
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ea6adabc08f93315eb0ecd47e7a4d2fe7c16f002: the new tail-jitter guard can still swallow a real scrollbar drag.
Reproduced code schedule
- A scrollbar
pointerdownsets_scrollbarDragActive = trueinstatic/ui.js. - The resulting native
scrollevent does not classify the movement immediately. It schedules the state transition inrequestAnimationFrame(). - If the reader releases the scrollbar before that frame runs, the
pointeruphandler clears_scrollbarDragActiveimmediately. - The deferred frame then calls
_isMessageTailJitter(), which consults the live flag. For a real 3–16 px upward drag that still ends within 16 px of the tail, the flag is now false,_tailJitterbecomes true, andmovedUpbecomes false. The reader stays pinned despite explicit input intent.
This contradicts the PR's contract that scrollbar intent always bypasses jitter suppression. The submitted scrollbar parameter case is false-green for this ordering because it injects _scrollbarDragActive = true at frame execution time; it never models pointerdown → scroll queues frame → pointerup → frame flush.
Required fix
Preserve scrollbar intent across the deferred classification boundary. For example, latch an owner/generation or a bounded scrollbar-intent timestamp when the drag begins (or when the scroll callback queues the frame), consume that snapshot in the frame, and reset it on the existing session/stream ownership resets. Do not rely only on the live _scrollbarDragActive flag after pointerup can clear it.
Add a production-composed controllable-rAF regression with this exact sequence:
- pointer down on the scrollbar,
- move upward by a small amount within the new geometric thresholds,
- dispatch
scrollto queue the frame, - dispatch
pointerupbefore flushing the frame, - flush the frame and assert
_messageUserUnpinned === trueand_scrollPinned === false.
The mandatory sandbox test gate did not execute test bodies in this pass because its Layer-1 GitHub diff fetch hit the account's REST rate limit and failed closed. This request is based on the deterministic event-order/code trace above, not on a claimed green or red test run. No PR code was executed outside the gate.
Manny7717
left a comment
There was a problem hiding this comment.
Verified locally with the repo's Node scroll-listener runtime harness.
Bug is real. The transcript opens pinned at the tail; a browser layout-settle artifact (measured 8px on desktop Chromium, no JS scroll write — the PR's instrumentation evidence is solid) shifts scrollTop up. The listener's top<_lastScrollTop-2 direction test reads that as an upward user scroll and latches _messageUserUnpinned=true; auto-follow stays off for the session and later renders restore the semantic anchor → reader stranded mid-transcript.
Fix is correct.
_isMessageTailJitter(top, bottomDistance)gatesmovedUp: it only fires for a 1-16px upward delta while still ≤16px from the true bottom, and every real-intent signal bypasses it — scrollbar drag (_scrollbarDragActive), recent wheel/touch/key message-pane intent, and recent non-message scroll. A deliberate scroll-away keeps unpinning through the existing branches.- The guard sits BEFORE the #4970 post-render artifact window, so it also covers settles that arrive outside the 1400ms artifact window (the reported open-time case) without disturbing that mechanism — the
movedUpsuppression logic below is untouched. - At the tail (≤16px) every branch is
nearBottom(250px band), so jitter frames fall through to the re-pin counter path — net effect is "stay pinned", exactly the intent. The_lastScrollTop===nullfirst-event guard keeps initial placement handling intact. - Scope is tight: 2 commits (fix + runtime tests). The #4702 test was UPGRADED from a source-string assertion to a real runtime harness run (same guard, now executed) — an improvement, not a regression-weakening.
Regression proven: new test file applied to base (e168b67) → all 6 new runtime tests FAIL (pin stability + all 5 parametrized genuine-input bypass cases); on head 14/14 pass (test_tail_jitter_unpins_pinned_reader + upgraded test_issue4702 + test_issue4295 neighbor). node --check clean; repo ruff gate: 0 new violations.
Non-blocking notes:
- The 16px thresholds are calibrated to the measured 8px Chromium settle. Other engines, zoom levels, or a taller settle (e.g. font-load reflow >16px) could still falsely unpin; consider scaling the window by
devicePixelRatioor the measured max settle if reports recur — a tuning follow-up, not a blocker. - Any layout artifact >16px or occurring >16px from the bottom remains unpinning (documented bound of the heuristic) — acceptable, since "still visually at the bottom" is the only safe case to swallow.
|
Addressed in The scroll listener now latches scrollbar-drag intent when the native Validated locally with the targeted runtime suite and 294 adjacent scroll/pin tests, plus |
Re-gate at exact head
|
7b0b3f2 to
75e2a68
Compare
|
Pushed 1. Async 2. Overlay scrollbars ( Guard not weakened: a press well inside the client box ( Tests: |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward — the tail-jitter diagnosis is right, and the rebase onto current master was clean (I did it myself; 104 commits behind, no conflicts, and your three-dot diff came out byte-identical, so nothing of yours was lost).
I took the rebased state through the full authoritative gate (regression review + mutation testing of your tests + the full suite). The core direction holds: ignoring browser tail jitter is the correct fix, and the deferred-rAF drag-intent latch you added in round 2 genuinely closes the original "small drag gets swallowed" hole. Two blockers remain, and one of them re-creates the exact bug class this PR exists to fix.
Blocker 1 (CORE) — release unconditionally re-arms drag intent, which can unpin a re-pinned reader
static/ui.js:6437 (and the pointercancel twin at :6447):
window.addEventListener('pointerup',()=>{
if(!_scrollbarDragActive) return;
_scrollbarDragActive=false;
// `scroll` is async: the drag's own scroll event may only be dispatched
// AFTER this release. Re-stamp so that first classification still owns it.
if(typeof _markScrollbarDragIntent==='function') _markScrollbarDragIntent();
_scheduleMessageVirtualizedRender(true);
},{passive:true});_markScrollbarDragIntent() (ui.js:6073) sets _scrollbarDragIntentUntil = performance.now() + 250 with no condition. The re-stamp is correct when the drag's scroll event is still undelivered — that's the case you were fixing. But it also fires when the drag's scroll was already dispatched and classified. In that case the release opens a fresh 250 ms window in which the next scroll consumes the stale intent, and a render-generated scroll is enough to satisfy it. That bypasses both the new tail-jitter classification and the existing post-render suppression at ui.js:6546, flipping a reader who is legitimately re-pinned to the tail into _scrollPinned=false; _messageUserUnpinned=false→true.
Reproduced against the extracted production listeners: origin/master stays pinned, and the branch stays pinned when the stale intent is removed — only the current head unpins. So this is a regression introduced by the re-stamp, not a pre-existing master behavior.
Fix: record the last scroll position observed during the drag, and on pointerup/pointercancel re-stamp only when release detects a position change that has not yet been delivered to a classification pass. Otherwise clear the timestamp rather than extending it. That preserves the async-scroll case you're protecting while closing the stale-intent window.
Blocker 2 (SILENT) — two tests are false-green; one is weaker than the assertion it replaced
Both verified by mutation (I removed the production guard and the test still passed):
-
tests/test_issue4702_portrait_open_scroll_bottom.py:46— the modified assertion is weaker than the one it replaced. Deleting the!grewclient-height growth guard from production still passes, because the scenario's zero bottom-distance geometry is independently protected by thebottomDistance>1check. Use a small nonzero bottom distance (2 px) so that removing the growth guard actually reds the test. -
tests/test_tail_jitter_unpins_pinned_reader.py:252— passes even when thepointerupre-stamp is removed entirely, so it does not pin the behavior it names. Advance the clock past the 250 ms intent window beforepointerup.
For blocker 1, please also add a regression at tests/test_tail_jitter_unpins_pinned_reader.py:348 that returns to the tail before release and asserts a later render nudge leaves the reader pinned — that's the case that currently regresses.
For reference, the mutation results on the current head: disabling the jitter delta correctly reds its test (good — that one is real coverage), while removing the pointer-up stamp passes (false-green).
Happy to re-gate as soon as you push. The rebase is already done on my side, so you can branch from current master without redoing it.
75e2a68 to
79a48a7
Compare
|
Pushed Blocker 1: release re-armed stale drag intent.
Both ownership resets clear the new tracker. As before, the stamp is consumed by the first scroll only. New runtime regressions drive the real extracted
RED on the previous behaviour: with the unconditional re-stamp restored, 7 tests fail (the 6 above + the reset-tracker assertions). GREEN on this head. Blocker 2: false-green tests.
Validation (local, this head):
|
79a48a7 to
38c35b9
Compare
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate at abb568cdd798: both 09-22 blockers are fixed; one drag-intent path still unpins a reader back at the tail
Thanks @ruizanthony. Both blockers from my 09-22 review are resolved.
- Release now re-arms drag intent only when the drag moved past the last delivered position, and otherwise clears it (
_releaseScrollbarDragIntent,static/ui.js~6133). - Both mutations from that review now turn red: removing the
!grewgrowth guard, and the other guard.
Codex's sandboxed re-gate, rebuilt on current master 0c3550f4a (exp-v0.52.369), ran 31/31 focused tests green.
A process note: our pre-execution scanner rates this diff SUSPICIOUS because of the four eval(payload.*) lines in the node harness (tests/test_tail_jitter_unpins_pinned_reader.py ~202). I think that's the repo's extract-and-eval idiom and not hostile, but the gate is mechanical, so I didn't run the tests locally this round; Codex executed them in its own sandbox. If the harness can load the listeners with vm.runInContext on the sliced source, the next re-gate can run here too.
[CORE] A quick drag up and back to the tail leaves drag intent queued
static/ui.js ~6571 (scroll listener) together with ~6133 (release). Sequence: pointerdown → drag up → drag back to the true tail → pointerup → an 8px render shift before the rAF classification. The scroll listener has already set _scrollbarDragIntentQueued=true from the drag's scrolls. The release logic clears _scrollbarDragIntentUntil but not the queued flag. So the stale intent reaches the rAF pass, which treats the render nudge as a user drag, bypasses both jitter guards, and turns auto-follow off for a reader sitting at the tail.
Codex verified it with the extracted production listeners under a controlled event order: master stays pinned and this head unpins. The timing is simulated rather than a live browser capture, but it's the same bug class this PR exists to fix (a pinned reader silently unpinned).
Fix: when the live or last-delivered drag position is back at the true bottom, discard both pending signals (_scrollbarDragIntentQueued and _scrollbarDragIntentUntil) at release. A released drag that stays above the tail keeps its intent. Test: a listener regression for pointerdown → drag up → return to tail → pointerup → render nudge → rAF that asserts the reader stays pinned, plus the counterpart where a genuine late upward drag still unpins.
That's the only finding this round.
|
Addressed the 2026-09-24 re-gate finding in At scrollbar release, The production-composed listener regression drives the exact Verification on this exact SHA: 28 focused tests and 200 targeted/adjacent scroll tests passed; |
Opening a long conversation with auto-follow on landed the reader in the middle of the transcript instead of at the tail. Root cause is not the compaction card. On opening a long conversation the reader is correctly placed AT the tail, then the browser nudges scrollTop up by a few px (observed: 8) with NO scrollHeight/clientHeight change. A CDP probe instrumenting scrollTop, scrollIntoView, scrollTo, scrollBy and focus recorded zero JS writes for that move: it is a layout-settle artifact of the freshly rebuilt transcript. The scroll listener's direction test (`top < _lastScrollTop - 2`) read that artifact as an upward user scroll and latched _messageUserUnpinned = true on a reader who never touched anything. Auto-follow stayed off for the whole session, and subsequent renders restored the SEMANTIC viewport anchor instead of the tail — landing the reader mid-conversation. The plan widget rides the same anchor path, which is why it drifted too. The compaction card is an aggravating factor, not the cause: it adds height and rerenders that make the jitter more likely on long sessions. Removing the compaction commits would have masked the symptom while leaving the real bug in place. Fix: treat a sub-scroll upward drift as jitter, not intent, but ONLY when all of these hold — drift is <= 16px, the reader is still within 16px of the bottom, and no wheel / touch / key / scrollbar intent was recorded. Any real scroll-up carries one of those intents and keeps unpinning exactly as before, so deliberate reading in history is untouched. Evidence (headless Chromium, real sessions, 2190-message transcript): before: 8/8 opens ended unpinned, ~mid-transcript after: 12/12 opens ended pinned at the tail Two of the 12 still show the 8px browser drift and stay correctly pinned, which is the artifact being absorbed rather than hidden. Tests: 636 ui.js static tests pass; the 2 failures in that set (test_anchor_fallback_ownership, test_api_timeout) reproduce identically with this change stashed and are pre-existing/unrelated. test_issue4702's assertion is updated to target the `!grew` gate behaviorally instead of matching the old literal line, preserving its original intent.
…verlay scrollbar hits Maintainer gate on 7b0b3f2 found two silent scroll-ownership defects: 1. The _scrollbarDragIntentQueued latch was armed only when the SYNC scroll handler saw _scrollbarDragActive===true, but pointerup clears that flag before the asynchronously-dispatched scroll arrives (quick 3-16px thumb drag: pointerdown -> scrollTop change -> pointerup -> scroll -> rAF), so the drag was classified as tail jitter and the reader re-pinned. 2. Drag ownership was claimed only for gutter scrollbars (offsetX >= clientWidth); an overlay scrollbar (Firefox macOS thin, Mozilla bug 1568939) sits INSIDE the client box and was never detected. Fix: stamp a bounded drag intent (_scrollbarDragIntentUntil = now + window) on pointerdown AND pointerup/pointercancel; the first scroll event after it consumes the stamp (folded into the queued latch) whatever branch it takes, so it never leaks into a later frame, and an unconsumed stamp expires. pointerdown now accepts gutter hits OR overlay right-edge hits (offsetX within the edge band of clientWidth, or clientX within the band of getBoundingClientRect().right) while still requiring target === scroller. Wheel/touch/keyboard jitter bypasses are untouched.
pointerup/pointercancel unconditionally re-stamped the scrollbar-drag intent. When the drag's own scroll had already been delivered and classified, that opened a fresh 250ms window which the next scroll -- a render/layout nudge -- consumed, unpinning a reader who had dragged back to the tail and been re-pinned. Track the scrollTop last delivered to the scroll listener during the drag (seeded at pointerdown). Release re-stamps only when the live scrollTop differs (the drag's scroll event is still pending); otherwise it clears the intent. Both ownership resets clear the tracker. Tests: - release after a delivered drag scroll does not re-arm (pointerup and pointercancel); drag back to tail + release + render nudge stays pinned; scrollbar click without movement leaves no intent; drag scroll delivered after release owns the re-armed intent, a later nudge cannot. - async-ordering test now holds the thumb past the pointerdown window so removing the release re-stamp reds it. - nesquena#4702 growth test settles 2px short of the bottom so removing the `!grew` guard reds it.
90517be to
96fc55b
Compare
|
@nesquena-hermes ready for re-review at exact head
The review-request API was unavailable to this fork author, so this comment is the explicit re-review request. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Static re-gate at exact head 780fd6e598c4649aafb5f6ad5e27147502067f0e (mandatory NO-RUN: Layer-1 scan is SUSPICIOUS on the submitted eval(...) harness). The true-bottom, queued-drag, and reset logic improved, but two objective pointer-ownership gaps remain.
- An ambiguous press in the rightmost 20px of the scrollable messages element becomes an overlay candidate. After only 2px of vertical movement, the next
scrollTopchange is itself used to prove a thumb drag before tail-jitter classification. Empty transcript margin + pointer movement + the same browser tail nudge under review can therefore manufacture drag ownership. The submitted negative margin case omits pointer movement and does not exercise this collision. pointerup/pointercancelignore the candidate's storedpointerId; a different pointer can promote, release, or clear another pointer's drag.
Please replace the circular overlay inference with independent thumb/gutter evidence, preserve one pointer owner through move/up/cancel, and add behavioral rows for margin+movement+jitter, both real-drag event orderings, wrong-pointer release/cancel, true-bottom clearing, and full reset. This review is static because the scan refused execution; the bounce is based on the source-level schedules above, not on the scan finding.
Thinking Path
scrolldelivery without leaking into a later render nudge after the reader returns to the true bottom.What Changed
pointerup → scroll → rAFordering and overlay scrollbar hits.TESTING.md.Release-note wording: Prevent small browser layout shifts at the bottom of long conversations from silently disabling auto-follow, while preserving deliberate wheel, touch, keyboard, and scrollbar scrolling.
Why It Matters
Without this guard, opening a long conversation can leave the reader in the middle of the transcript even though they never scrolled. The fix keeps browser layout settle from stealing scroll ownership while preserving explicit reader intent and the sticky-unpin model.
Contract Routing
Task type: transcript scroll/pinning runtime bug fix.
Touched areas:
static/ui.jsscroll ownership and pin stateTESTING.mdinteraction guidanceRelevant public docs:
AGENTS.mdCONTRIBUTING.mddocs/CONTRACTS.mddocs/GUIDELINES.mddocs/UIUX-GUIDE.mdDESIGN.mdTESTING.mdScope boundaries: transcript tail-follow ownership only; no layout, API, persistence, dependency, or build-system change. This restores the existing follow/unpin contract rather than intentionally redefining it, so no
Contract Changesection is required.Evidence needed before claiming done: observable listener-state regressions, adjacent scroll suites, JavaScript syntax/runtime lint, diff-scoped Python lint, clean rebase, and desktop/narrow/mobile evidence.
Verification
Rebased without conflicts onto
masterc296673ebfaf98750fe38438bc71f0cbb1f75777;git range-diffreports all nine commits patch-identical and the aggregate stable patch ID is unchanged.Local verification on exact head
96fc55ba3dd4b05f850f4efb2fe47fafef291b38:./scripts/test.sh tests/test_tail_jitter_unpins_pinned_reader.py tests/test_issue4702_portrait_open_scroll_bottom.py— 33 passed../scripts/test.sh— 176 passed across issue 1731/3250/3319/3470/4295/4346/4793/4856/6414, TARS reset, jump-to-answer settle, pinned-tail jitter, and collapse/clamp follow suites../scripts/test.sh— 6 passed.node --check static/ui.js— clean (Nodev24.16.0).scripts/ruff_lint.py --diff github-upstream/master— 0 findings in changed Python files and 0 on changed lines.git diff --check github-upstream/master...HEAD— clean.Responsive UI evidence
The captures exercise the real application page in Chromium with isolated state and a deterministic 8 px browser tail shift. The diagnostic card is capture-only so the otherwise invisible pin-state transition is readable.
Before — base behavior
After — fixed behavior
Verified viewports and interactions:
On the base revision, the same 8 px no-input movement produced
_scrollPinned=falseand_messageUserUnpinned=trueat all three widths. On the fixed revision, all three remain pinned (true/false). Genuine wheel and touch input still transfers ownership at every relevant width (false/true). No provider credentials or live state were used.Risks / Follow-ups
Model Used
OpenAI Codex
gpt-5.6-solthrough Hermes Agent for the final rebase, review, and verification, using Git, Node, Python, and GitHub CLI tooling.