From 414e509cda163b32f46a75c288d6f2793511f25b Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 14:28:20 +0000 Subject: [PATCH] fix(web/chat): scope items-effect deps to actual items-change triggers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../use-transcript-scroll.burst.test.tsx | 98 +++++++++++++++++++ .../chat/transcript/use-transcript-scroll.ts | 97 ++++++++++-------- 2 files changed, 155 insertions(+), 40 deletions(-) diff --git a/apps/web/src/domains/chat/transcript/use-transcript-scroll.burst.test.tsx b/apps/web/src/domains/chat/transcript/use-transcript-scroll.burst.test.tsx index 7ce0b01f338..0bf1a898be4 100644 --- a/apps/web/src/domains/chat/transcript/use-transcript-scroll.burst.test.tsx +++ b/apps/web/src/domains/chat/transcript/use-transcript-scroll.burst.test.tsx @@ -316,4 +316,102 @@ describe("useTranscriptScroll — load-older burst regression", () => { }); expect(onLoadOlderCalls).toBe(2); }); + + test("isLoadingOlder transition without items change does not auto-fire onLoadOlder", () => { + // Bug repro: parent's `transcriptPagination.isLoadingOlder` mirror + // useEffect runs at urgent priority while `setMessages` runs inside + // `startTransition` (in `use-conversation-history.ts`). That can + // produce an intermediate commit where `isLoadingOlder` transitions + // true→false BEFORE the older-page items have prepended. The + // previous items-effect implementation listed `isLoadingOlder` in + // its dep array, so this intermediate commit re-fired the effect + // even though items hadn't changed — and the body would: + // - release the in-flight lock, + // - have `decideItemsChangeAction` return "anchor-correct" on a + // key still present in the unchanged list (consuming + // `savedAnchorRef` on a heightDelta=0 no-op), and + // - re-classify + chain-load fire `onLoadOlder()` again because + // the user is still near the top and the lock just released. + // Result: scrolling to the top loads older pages two-at-a-time. + // + // Fix: trim items-effect deps to `[items, conversationId, ...]` + // only, move the lock-release into its own `useLayoutEffect` + // keyed on `isLoadingOlder`, and read other mutable values via + // `latestRef`. An `isLoadingOlder` transition no longer reaches + // the items-effect at all. + let onLoadOlderCalls = 0; + const scrollEl = createScrollElement({ + scrollTop: 30, + scrollHeight: 5000, + clientHeight: 800, + }); + document.body.appendChild(scrollEl); + + const transcriptRef = { + current: { + scrollToLatest: () => {}, + getScrollElement: () => scrollEl, + }, + }; + + const items: TranscriptItem[] = [ + makeMessageItem("m1"), + makeMessageItem("m2"), + ]; + + const onLoadOlder = () => { + onLoadOlderCalls += 1; + }; + + const { rerender } = renderHook( + (args: UseTranscriptScrollArgs) => useTranscriptScroll(args), + { + initialProps: { + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder, + }, + }, + ); + + // User scrolls into the load-older window → fires once. + act(() => { + scrollEl.dispatchEvent(new Event("scroll")); + }); + expect(onLoadOlderCalls).toBe(1); + + // Parent flips loading on (mirror useEffect commit). + rerender({ + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: true, + onLoadOlder, + }); + + // Parent flips loading off BEFORE the items have prepended (the + // urgent vs. transition priority split). scrollTop is still 30 — + // no anchor restoration could possibly have happened because no + // items changed. The hook must NOT auto-fire onLoadOlder here. + rerender({ + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder, + }); + + expect(onLoadOlderCalls).toBe(1); + + // Lock is released → a fresh user-initiated scroll still fires. + act(() => { + scrollEl.dispatchEvent(new Event("scroll")); + }); + expect(onLoadOlderCalls).toBe(2); + }); }); diff --git a/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts b/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts index 99555bb0d84..877b2b8ba91 100644 --- a/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts +++ b/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts @@ -240,6 +240,17 @@ export function useTranscriptScroll( const [showScrollToLatest, setShowScrollToLatest] = useState(false); // ---------- Latest-props ref (ref-backed fresh-closure pattern) --------- + // Synced in useLayoutEffect (declared BEFORE the items-effect below) + // so the items-effect — whose deps are intentionally narrow + // (`items` + `conversationId` + transcriptRef + engageAutoPin) — + // reads fresh values for everything else via latestRef without + // forcing those values into its dep array. Putting them in the + // items-effect deps would cause it to re-run on transitions that + // shouldn't trigger items-change work, with two real consequences: + // (1) the `decideItemsChangeAction` branch consumes `savedAnchorRef` + // on a heightDelta=0 no-op, and (2) the chain-load branch re-fires + // `onLoadOlder()` on a just-released lock while items haven't yet + // prepended. See the dedicated lock-release effect below. // TODO: migrate to useEffectEvent once it's stable in React. const latestRef = useRef({ items, @@ -250,7 +261,7 @@ export function useTranscriptScroll( isPinnedToLatest, showScrollToLatest, }); - useEffect(() => { + useLayoutEffect(() => { latestRef.current = { items, hasMore, @@ -277,8 +288,8 @@ export function useTranscriptScroll( // // The `isLoadingOlder` prop is the source of truth, but it propagates // through React state: parent calls `setIsLoadingOlder(true)` inside - // `onLoadOlder`, React commits, then a `useEffect` mirrors the new - // value into `latestRef`. Between the firing scroll event and the + // `onLoadOlder`, React commits, then a `useLayoutEffect` mirrors the + // new value into `latestRef`. Between the firing scroll event and the // latestRef refresh, ~5–20 more scroll events can fire — every one // sees the stale `isLoadingOlder=false` and re-fires `onLoadOlder`. // Even though React Query dedupes the underlying fetch, the rapid @@ -288,13 +299,30 @@ export function useTranscriptScroll( // // The fix: a ref 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 - // settles `isLoadingOlder` back from true→false (success or failure - // both transition the same way; reaching the items-effect below also - // detects this so chain-load on underfilled viewport keeps working). + // see `true` and skip. The lock is released by the dedicated + // `isLoadingOlder` transition effect below — declared BEFORE the + // items-effect so it fires first in the commit phase when both + // change together (the underfilled-viewport chain-load case). const loadOlderInFlightRef = useRef(false); const prevIsLoadingOlderRef = useRef(isLoadingOlder); + // ---------- Lock release on isLoadingOlder true→false transition ------ + // + // Declared as its own effect (not co-located with the items-effect) + // so transitions of `isLoadingOlder` that do NOT coincide with an + // items change cannot accidentally trigger the items-effect's + // anchor-correct + chain-load work. Declared BEFORE the items-effect + // so in commits where BOTH `isLoadingOlder` and `items` change + // (underfilled-viewport prepend), this effect runs first within the + // commit phase and the items-effect's chain-load branch sees the + // released lock immediately. + useLayoutEffect(() => { + if (prevIsLoadingOlderRef.current && !isLoadingOlder) { + loadOlderInFlightRef.current = false; + } + prevIsLoadingOlderRef.current = isLoadingOlder; + }, [isLoadingOlder]); + // ---------- Previous items ref (for change detection) ----------------- const previousItemsRef = useRef(items); @@ -375,23 +403,23 @@ export function useTranscriptScroll( // ----------------------------------------------------------------------- // Items change handler — runs in useLayoutEffect so the anchor correction // happens before the browser paints. + // + // Deps are intentionally narrow: `items` + `conversationId` (the two + // values that determine WHEN this work should run) plus the two + // stable refs (`transcriptRef`, `engageAutoPin`). Everything else + // that the body reads — `hasMore`, `isLoadingOlder`, `onLoadOlder`, + // `isPinnedToLatest`, `showScrollToLatest` — comes from `latestRef`, + // which is synced in the useLayoutEffect declared above this one. + // Listing those values directly in this effect's deps would cause it + // to re-run for transitions that didn't change items, and the + // anchor-correct + chain-load branches would fire against stale + // assumptions (e.g. the `isLoadingOlder` true→false → premature + // chain-load bug). // ----------------------------------------------------------------------- useLayoutEffect(() => { const prev = previousItemsRef.current; previousItemsRef.current = items; - - // Directional release of the load-older in-flight lock: when the - // parent transitions `isLoadingOlder` true→false, the load has - // settled (success OR failure) and the next near-top check should - // be allowed to re-fire. Done here (in the layout effect that runs - // BEFORE any paint) rather than in a passive useEffect so the - // chain-load case below can re-fire on the same commit that just - // released the lock — needed for underfilled viewports that need - // multiple pages to reach scrollable height. - if (prevIsLoadingOlderRef.current && !isLoadingOlder) { - loadOlderInFlightRef.current = false; - } - prevIsLoadingOlderRef.current = isLoadingOlder; + const latest = latestRef.current; const action = decideItemsChangeAction({ items, @@ -459,10 +487,9 @@ export function useTranscriptScroll( // browser does NOT fire a scroll event when scrollHeight grows under // a stationary scrollTop (the streaming case), so without this the // "Go to Newest" pill would only surface once the user touched the - // scroll wheel. Read flags from closure (not latestRef) so the - // prepend render sees the just-flipped isLoadingOlder=false instead - // of the previous render's snapshot; latestRef is refreshed by a - // later useEffect that hasn't fired yet at this point. + // scroll wheel. Read mutable flags from `latest` (synced by the + // useLayoutEffect declared above this one, so values are fresh for + // the current commit). const el = transcriptRef.current?.getScrollElement(); if (el) { const classification = classifyScrollPosition( @@ -472,15 +499,15 @@ export function useTranscriptScroll( clientHeight: el.clientHeight, }, { - hasMore, - isLoadingOlder, + hasMore: latest.hasMore, + isLoadingOlder: latest.isLoadingOlder, hasConversation: conversationId !== null, }, ); - if (classification.isPinned !== isPinnedToLatest) { + if (classification.isPinned !== latest.isPinnedToLatest) { setIsPinnedToLatest(classification.isPinned); } - if (classification.showScrollToLatest !== showScrollToLatest) { + if (classification.showScrollToLatest !== latest.showScrollToLatest) { setShowScrollToLatest(classification.showScrollToLatest); } @@ -504,20 +531,10 @@ export function useTranscriptScroll( } } loadOlderInFlightRef.current = true; - onLoadOlder(); + latest.onLoadOlder(); } } - }, [ - items, - conversationId, - transcriptRef, - hasMore, - isLoadingOlder, - onLoadOlder, - isPinnedToLatest, - showScrollToLatest, - engageAutoPin, - ]); + }, [items, conversationId, transcriptRef, engageAutoPin]); // ----------------------------------------------------------------------- // Container resize re-pin. When the scroll container resizes (e.g. the