🤖 fix: stop scroll-up jitter at bottom + harden auto-scroll ownership#3226
🤖 fix: stop scroll-up jitter at bottom + harden auto-scroll ownership#3226
Conversation
|
@codex review Best-of-5 read-only audit was run before opening; this PR rolls up the convergent findings (delta-0 wheel filter + |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Asymmetric thresholds in handleScroll's user-intent branch: - Locked -> release on >1px drift (small wheel notch shouldn't snap back) - Released -> relock only when moving toward bottom AND within 8px Previously a single 8px geometry threshold gated both sides, so a small wheel-up tick (~3-7px) re-engaged the lock and the rAF settle tick wrote scrollTop=max on the next frame, snapping the user back. The direction check uses lastScrollTopRef captured at the top of every handleScroll call.
Audit follow-ups for the bottom-lock state machine after the scroll-up jitter fix: 1) Filter delta-0 wheel events (Cmd-wheel zoom on macOS, Shift-wheel for horizontal-only, Bluetooth-mouse jitter, pinch gestures). Without the filter every phantom wheel cleared programmaticDisableRef and refreshed the 750 ms intent window, weakening downstream gates. 2) Seed lastScrollTopRef inside disableAutoScroll and jumpToBottom. The released-branch user-intent direction check compares scrollTop against this ref, but neither path always fires a scroll event (disable never does; jumpToBottom skips the write when scrollTop is already max). Without the seed, a small wheel-up notch following an explicit programmatic disable could be misread as 'moving toward bottom' and spuriously relock. 3) Add three regression tests covering disable-then-wheel-up, delta-0 wheel ignored, and non-zero wheel still marks intent. ChatPane wires onWheel to the new handleScrollContainerWheel handler from the hook, replacing the direct binding to markUserScrollIntent.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Fixes the small-but-noticeable jitter when the user starts scrolling up from the very bottom while the chat transcript bottom-lock is engaged. Eventually a large enough wheel/touch delta would "win" against the rAF settle tick, but the first 1–3 notches of a slow gesture used to be snapped back to the bottom — felt like the scrollport was fighting the user.
Background
useAutoScroll.handleScroll's user-intent branch decided lock state from a single 8 px geometric threshold. A small wheel notch (typical mousewheel notch is 3–7 px) landed within that window, so the hook re-engaged the lock and the 60-frame rAF settle loop wrotescrollTop = scrollHeight − clientHeighton the next frame, snapping the user back to the bottom. The user could only "win" by accumulating a single tick of more than 8 px before the next rAF.Implementation
Two commits, both behavior-preserving outside the targeted regression and validated against the existing 22 hook unit tests + the
bottomLayoutShiftintegration suite.Commit 1 —
fix: stop scroll-up jitter from the very bottomAsymmetric thresholds in
handleScroll's user-intent branch:BOTTOM_LOCK_EPSILON_PX). The very first wheel-up notch releases without rAF snap-back.currentScrollTop > previousScrollTop) AND within 8 px (USER_BOTTOM_RELOCK_THRESHOLD_PX). Direction is tracked with a single new ref (lastScrollTopRef) updated at the top of everyhandleScrollcall.The no-intent paths (1 px drift correction while locked, geometric relock at 8 px after the intent window expires) are unchanged. The existing "scroll back to bottom and the lock re-engages" UX is preserved.
Commit 2 —
fix: harden auto-scroll user-intent ownershipAudit follow-ups (best-of-5 read-only audit converged on these):
Filter delta-0 wheel events. Cmd-wheel zoom on macOS, Shift-wheel for horizontal-only, Bluetooth-mouse jitter, and pinch gestures all dispatch
wheelevents withdeltaY === 0(and oftendeltaX === 0). Without filtering, every phantom wheel clearedprogrammaticDisableRefand refreshed the 750 ms intent window, weakening every downstream gate that relied on those refs. NewhandleScrollContainerWheelis exposed by the hook; ChatPane wires it in place ofmarkUserScrollIntent.Seed
lastScrollTopRefinsidedisableAutoScrollandjumpToBottom. The released-branch direction check comparesscrollTopagainst this ref, but neither path always emits a scroll event (disableAutoScrollnever does;jumpToBottomskips the write whenscrollTopis already max). Without the seed, a small wheel-up notch following an explicit programmatic disable could be misread as "moving toward bottom" (e.g.895 > 0) and spuriously relock the lock that was just disabled.Validation
bun test src/browser/hooks/useAutoScroll.test.tsx— 25 / 25 pass (19 prior + 6 new regression tests covering the four scenarios above).bun x jest tests/ui/chat/bottomLayoutShift.test.ts— passes (drift-correction / pin-on-resize / send / workspace-switch contracts unchanged).make static-check— passes locally end-to-end.Risks
BOTTOM_LOCK_EPSILON_PXwas already used for the no-intent drift correction. The fix uses the same value for the locked-intent release path, so any pre-existing subpixel sensitivity is consistent across paths.deltaY = 0but expect intent marking are unaffected because such events also do not move the scrollport, and our intent window only matters when scroll motion follows.lastScrollTopRefseeding is purely additive — every code path that writes to it before now still writes to it now; we just close two narrow staleness windows (disableAutoScrollwith no follow-up scroll event,jumpToBottomwhen already at bottom).Pains
The audit phase (5 read-only sub-agents in parallel) was the right call here: 4 of 5 audits independently flagged Findings #1 (
programmaticDisableRefbypass) and #2 (lastScrollTopRefcold-start), which I would have likely missed reasoning forward from the initial fix alone. The audits also agreed that the new direction-aware logic is the right primitive — none recommended walking it back.Deferred to follow-up PRs (out of scope for "scroll-up jitter"):
hasLoadedTranscriptRowsflip mid-read snaps to bottom).autoScrolltoggle.TRANSCRIPT_SCROLL_KEYS(keyboard-nav focus-induced scroll snaps back).OutputTab/BashToolCall/InitMessage(different sub-views, each with its own bottom-lock heuristic).Generated with
mux• Model:anthropic:claude-opus-4-7• Thinking:max• Cost:$11.30