Stop forced layout reads in the bottom-anchored scroll hot path - #2280
Stop forced layout reads in the bottom-anchored scroll hot path#2280vburojevic wants to merge 1 commit into
Conversation
Mobile telemetry shows 1,500+ scroll stalls on div.thread-scrollbar. BottomAnchoredScrollBody read scrollHeight/clientHeight on every scroll and wheel event (syncBottomStateFromScroll, markWheelScrollIntent, and the throttled writeScrollAnchor), forcing a synchronous layout pass per event on an unvirtualized timeline of up to ~5,400 DOM nodes. Mirror useStickyBottomScroll's established pattern: cache scrollHeight - clientHeight in a ref and let per-scroll-event code read only scrollTop plus the cache. The cache refreshes only where layout legitimately changes: the ResizeObserver callback (it already watches both the scroll port and the content wrapper), the bottom-restore loop (which deliberately re-reads fresh geometry after content growth), the programmatic scroll paths (scrollToBottom, clamped reveal, saved-row restore, prepend compensation), and the one-shot unmount anchor flush. Two guards keep the cache honest: - Shrink-edge verification: on a content-shrink frame the scroll event outruns the ResizeObserver refresh, so a still-pinned viewport compares its clamped scrollTop against a stale-high max and reads as a user detach — unrecoverable, since the bottom-restore is suppressed once stick-to-bottom is off (deterministic on iOS: tap-collapsing a long tool output while pinned). On the attach->detach edge only, one fresh read re-tests the predicate before flipping state; the same edge verification guards markWheelScrollIntent and writeScrollAnchor. Growth stays cache-only (stale-low is safe). Zero reads per steady-state scroll event, one per detach edge. - The cache is only authoritative after the first ResizeObserver delivery; before that (or under a polyfill that never fires, as in the shared vitest setup) hot paths fall back to live reads, the pre-cache behavior, instead of trusting a frozen value. jsdom tests now deliver the ResizeObserver notification a real browser fires when scroll geometry changes. New tests pin the contract: getter spies prove one read on the detach edge and zero across a mid-timeline burst and re-attach; a shrink-frame regression test proves a pinned viewport stays pinned and never persists a detached anchor; isAtBottom threshold transitions stay correct against the resize-refreshed cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ttach edge (#2300) Lands five mobile-performance PRs by Vedran Burojevic plus one follow-up fix. * #2277 Keep the thread panel host mounted across thread navigation * #2278 Gate the default focus refetch on lost realtime coverage * #2279 Apply urgent realtime thread changes without flushing the debounce buffer * #2280 Cache max scroll offset outside the timeline scroll hot path * #2281 Make the published plugin composer host stable across keystrokes Follow-up fix: a detached timeline viewport that a content shrink clamps onto the new, smaller maximum stayed detached, because the clamp's scroll event outruns the ResizeObserver refresh and is classified against the stale cache. The ResizeObserver path now detects that case and re-attaches. Regression test fails on the #2280 head and passes here. Verified: typecheck pass; oxlint 0 errors; @bb/app 422 files / 3,288 tests pass. Merge commits are disabled on this repository, so this squash carries the work of all five branches; co-author trailers preserve authorship. Co-Authored-By: Vedran Burojevic <vedran.burojevic@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
|
Shipped in #2300 (squash commit 836e216). This branch's changes are on Closing as superseded rather than merged only because merge commits are disabled on this repo: a squash creates a new commit, so GitHub cannot mark this PR merged even though its changes are in Review notes: sound, with one edge fixed on top — the detached mirror of your attach→detach guard. A detached viewport that a content shrink clamps onto the new, smaller maximum received the clamp's scroll event before the ResizeObserver refresh, so it was classified against the stale cache and stayed detached with nothing to re-test it (jump-to-bottom pill stuck on while already at the bottom; streaming stopped following). The fix detects it in the ResizeObserver path and re-attaches via a shared Thanks — nice work on this batch.
|
2 participants
What was wrong
Every scroll event on the thread timeline read scrollHeight/clientHeight (forced layout in WebKit) via getMaxScrollOffset, plus again in wheel-intent and anchor capture — on an unvirtualized list that reaches thousands of nodes. useStickyBottomScroll already documents and avoids this exact cost; the main scroll body never got the same treatment.
What changed
Cache maxScrollOffset in a ref refreshed only where layout legitimately changes (ResizeObserver on port+content, restores, scrollToBottom, prepend compensation, unmount flush); hot handlers read scrollTop + cache. Two review-driven safeguards: the attach→detach edge does one fresh re-test before flipping state (a shrink frame delivers scroll before the RO refresh — without this, collapsing a tool output while pinned falsely detaches on iOS, which has no scroll anchoring); and until the first real RO delivery the code falls back to live reads (also keeps jsdom's no-op RO polyfill on pre-change semantics).
How verified
Getter-spy test proves 0 reads across a 10-event burst and exactly 1 read pair on the detach edge; shrink-frame regression test; both fail on the pre-fix commit. Full @bb/app suite 3,273 passed. Typecheck + oxlint clean; adversarially reviewed.
🤖 Generated with Claude Code