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