fix(web/chat): debounce load-older during scroll-to-top burst#32386
Merged
Conversation
When the user scrolls to the top of the transcript, the browser fires many scroll events (~60/sec). Each one independently called `onLoadOlder()` because the `isLoadingOlder` prop took a React commit + useEffect to propagate into the hook's `latestRef` mirror — by the time the lock landed, 5–20 events had already fired. React Query deduped the underlying fetch, but the rapid re-fires overwrote `savedAnchorRef` with progressively different scrollTop values mid-gesture, producing jittery scroll restoration after the prepend. Fix: introduce a `loadOlderInFlightRef` that flips to `true` SYNCHRONOUSLY at the moment we call `onLoadOlder`. Subsequent scroll events within the same burst see `true` and skip. The lock is released the next time the parent transitions `isLoadingOlder` true→false (success OR failure both transition the same way). Release happens inside the items `useLayoutEffect` (which already depends on `isLoadingOlder`) so the chain-load case on underfilled viewports can re-fire on the same commit that just released the lock. Adds 3 regression tests in a new `.burst.test.tsx` file that mount the real hook against a fake scroll element and dispatch real DOM scroll events — the existing pure-function test suite never exercised the latestRef commit timing, which is why this bug shipped.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ed85d2c03
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+295
to
+296
| const loadOlderInFlightRef = useRef(false); | ||
| const prevIsLoadingOlderRef = useRef(isLoadingOlder); |
There was a problem hiding this comment.
If the user switches conversations immediately after a scroll-to-top trigger, before the parent has rendered isLoadingOlder=true, this new ref stays true with prevIsLoadingOlderRef still false, so the true→false release below never runs. ChatRouteContent reuses this hook across activeConversationId changes, so the next conversation's underfilled/near-top checks are blocked from calling onLoadOlder until a remount or unrelated loading transition; clear the synchronous lock in the existing conversation-switch reset path.
Useful? React with 👍 / 👎.
dvargasfuertes
approved these changes
May 28, 2026
vellum-apollo-bot Bot
added a commit
that referenced
this pull request
May 29, 2026
Bug: scrolling to the top of the transcript loaded older pages two-at-a-time despite PR #32386's synchronous in-flight lock. Root cause: the items-`useLayoutEffect` listed `isLoadingOlder` (and five other mutable values) in its dep array. When the parent's `isLoadingOlder` mirror useEffect (urgent priority in `use-conversation-history.ts`) commits BEFORE the prepend's `setMessages` (which runs inside `startTransition`), the items-effect re-fires on the same `items` reference. In that intermediate commit the body: - releases the in-flight lock (true→false transition), then - has `decideItemsChangeAction` return "anchor-correct" because the saved anchor key is still present in the unchanged items, consuming `savedAnchorRef` on a heightDelta=0 no-op (defeating scroll preservation when the actual prepend arrives), then - re-classifies the scroll position, sees the user still near the top + the just-released lock, and fires `onLoadOlder()` again. The fix is structural, not defensive: 1. Lock release moves to its own `useLayoutEffect` keyed on `[isLoadingOlder]`, declared BEFORE the items-effect. When both change in the same commit (the underfilled-viewport chain-load case) it runs first and the items-effect sees the released lock. When only `isLoadingOlder` transitions, only this effect fires. 2. `latestRef` sync moves from `useEffect` (post-paint) to `useLayoutEffect` (commit phase), declared before the items-effect so all mutable values it mirrors are fresh by the time the items-effect reads them. 3. Items-effect deps trim to `[items, conversationId, transcriptRef, engageAutoPin]` — only what semantically determines "this is an items-change event". `hasMore`, `isLoadingOlder`, `onLoadOlder`, `isPinnedToLatest`, `showScrollToLatest` move to `latestRef` reads. `isPinnedToLatest` / `showScrollToLatest` self-trigger loop (state this effect itself sets via guard-compare) is eliminated as a side benefit. Regression test asserts: rerender with `isLoadingOlder=true` then `false` WITHOUT changing items does NOT auto-fire `onLoadOlder`. All 45 scroll-hook tests pass including the underfilled-viewport `chain-load: false→true→false sequence kicks again when still underfilled` case.
dvargasfuertes
pushed a commit
that referenced
this pull request
May 29, 2026
#32399) Bug: scrolling to the top of the transcript loaded older pages two-at-a-time despite PR #32386's synchronous in-flight lock. Root cause: the items-`useLayoutEffect` listed `isLoadingOlder` (and five other mutable values) in its dep array. When the parent's `isLoadingOlder` mirror useEffect (urgent priority in `use-conversation-history.ts`) commits BEFORE the prepend's `setMessages` (which runs inside `startTransition`), the items-effect re-fires on the same `items` reference. In that intermediate commit the body: - releases the in-flight lock (true→false transition), then - has `decideItemsChangeAction` return "anchor-correct" because the saved anchor key is still present in the unchanged items, consuming `savedAnchorRef` on a heightDelta=0 no-op (defeating scroll preservation when the actual prepend arrives), then - re-classifies the scroll position, sees the user still near the top + the just-released lock, and fires `onLoadOlder()` again. The fix is structural, not defensive: 1. Lock release moves to its own `useLayoutEffect` keyed on `[isLoadingOlder]`, declared BEFORE the items-effect. When both change in the same commit (the underfilled-viewport chain-load case) it runs first and the items-effect sees the released lock. When only `isLoadingOlder` transitions, only this effect fires. 2. `latestRef` sync moves from `useEffect` (post-paint) to `useLayoutEffect` (commit phase), declared before the items-effect so all mutable values it mirrors are fresh by the time the items-effect reads them. 3. Items-effect deps trim to `[items, conversationId, transcriptRef, engageAutoPin]` — only what semantically determines "this is an items-change event". `hasMore`, `isLoadingOlder`, `onLoadOlder`, `isPinnedToLatest`, `showScrollToLatest` move to `latestRef` reads. `isPinnedToLatest` / `showScrollToLatest` self-trigger loop (state this effect itself sets via guard-compare) is eliminated as a side benefit. Regression test asserts: rerender with `isLoadingOlder=true` then `false` WITHOUT changing items does NOT auto-fire `onLoadOlder`. All 45 scroll-hook tests pass including the underfilled-viewport `chain-load: false→true→false sequence kicks again when still underfilled` case. Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
When the user scrolls to the top of the transcript,
loadOlderfires many times in rapid succession, producing a jittery scroll-restoration experience after the older-page prepend lands.Root cause
The browser fires ~60 scroll events/sec during a scroll-to-top gesture. The hook's scroll listener (
handleScroll, ~line 614) readsisLoadingOlderfromlatestRef.current— a mirror that's updated inside auseEffect. That mirror only refreshes after React commits the parent'ssetIsLoadingOlder(true), which takes a full render cycle.Between the firing scroll event and the latestRef refresh, 5–20 more scroll events fire. Every one sees
isLoadingOlder=false(stale) and re-firesonLoadOlder(). React Query dedupes the underlying fetch, but the rapid re-fires overwritesavedAnchorRefwith progressively different scrollTop values mid-gesture — and on prepend, the restore uses the last captured value, which doesn't match the user's intent. That's the jitter.The items-effect path (
useLayoutEffect, ~line 432) has the same issue when chain-loading on underfilled viewports.Fix
A synchronous
loadOlderInFlightRefthat flips totruethe moment we callonLoadOlder— before React has a chance to commit anything. Subsequent triggers within the same burst seetrueand skip. Both firing sites (scroll listener + items-effect) gate on the ref and set it synchronously.The lock is released via a directional check at the top of the items
useLayoutEffect: whenisLoadingOldertransitions true→false, clear the lock. That handles:useLayoutEffectinvocation that re-classifies the viewport, so the second/third page kick in on the same commit that just released the lock.Nothing is mirrored via a passive
useEffect— that ordering would clobber the synchronous lock on mount when the prop starts atfalse.Tests
New
use-transcript-scroll.burst.test.tsxmounts the real hook against a fake scroll element and dispatches real DOM scroll events. Three regression cases:onLoadOlderexactly once. Confirmed FAILS onorigin/main(fires 10 times) before the fix.The pre-existing pure-function test suite (
use-transcript-scroll.test.ts) never exercised the latestRef commit timing — which is why this class of bug shipped. The burst test is the missing test class.Verification
bun test src/domains/chat/transcript/use-transcript-scroll→ 44 pass (41 existing + 3 new).bun run typecheck→ clean.bun run lint→ clean.Diff stats
use-transcript-scroll.ts: +50/-2 (the ref, the directional release, two single-line gate changes).use-transcript-scroll.burst.test.tsx: +320/-0 (new file).Context: this is the first of a series of bug-fixes / progressive in-place simplifications to the canonical
useTranscriptScrollhook, after #32374 removed the parallel-module experiment.