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 new file mode 100644 index 00000000000..7ce0b01f338 --- /dev/null +++ b/apps/web/src/domains/chat/transcript/use-transcript-scroll.burst.test.tsx @@ -0,0 +1,319 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +/** + * Regression tests for the load-older burst bug. + * + * Bug: when the user scrolls to the top, the browser fires many scroll + * events (~60/sec). Each one was independently calling `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, overwriting `savedAnchorRef` + * with progressively different scrollTop values and producing jittery + * scroll restoration after the prepend. + * + * Fix: synchronous `loadOlderInFlightRef` that flips true the moment + * we fire, gating subsequent triggers within the same gesture, and a + * mirror useEffect that syncs from the prop on settle. + * + * These tests mount the real hook against a fake scroll element and + * dispatch real DOM scroll events — the pure-function test suite in + * `use-transcript-scroll.test.ts` cannot catch this class of bug + * because it never exercises the latestRef commit timing. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { createRef } from "react"; + +import { + useTranscriptScroll, + type UseTranscriptScrollArgs, +} from "./use-transcript-scroll"; +import type { TranscriptItem } from "./types"; + +// --------------------------------------------------------------------------- +// Fake scroll element +// --------------------------------------------------------------------------- +// +// A minimal element that lets the test drive `scrollTop`, `scrollHeight`, +// and `clientHeight` directly. The hook's scroll listener attaches via +// `addEventListener("scroll", ...)`, so dispatchEvent on this element +// routes through the listener exactly as a real scroll gesture would. + +function createScrollElement(opts: { + scrollTop?: number; + scrollHeight?: number; + clientHeight?: number; +}): HTMLDivElement { + const el = document.createElement("div"); + let scrollTop = opts.scrollTop ?? 0; + let scrollHeight = opts.scrollHeight ?? 5000; + let clientHeight = opts.clientHeight ?? 800; + + Object.defineProperty(el, "scrollTop", { + configurable: true, + get: () => scrollTop, + set: (v: number) => { + scrollTop = v; + }, + }); + Object.defineProperty(el, "scrollHeight", { + configurable: true, + get: () => scrollHeight, + set: (v: number) => { + scrollHeight = v; + }, + }); + Object.defineProperty(el, "clientHeight", { + configurable: true, + get: () => clientHeight, + set: (v: number) => { + clientHeight = v; + }, + }); + return el; +} + +function makeMessageItem(key: string): TranscriptItem { + return { + key, + kind: "message", + message: { + id: key, + role: "user", + content: "x", + conversationId: "c1", + createdAt: 0, + } as any, + }; +} + +// --------------------------------------------------------------------------- + +describe("useTranscriptScroll — load-older burst regression", () => { + beforeEach(() => { + // Make sure each test owns a clean DOM. + document.body.innerHTML = ""; + }); + + afterEach(() => { + cleanup(); + }); + + test("burst of scroll events at the top fires onLoadOlder exactly once", () => { + let onLoadOlderCalls = 0; + const scrollEl = createScrollElement({ + scrollTop: 500, + scrollHeight: 5000, + clientHeight: 800, + }); + document.body.appendChild(scrollEl); + + const transcriptRef = createRef<{ + scrollToLatest: (opts?: { behavior?: "auto" | "smooth" }) => void; + getScrollElement: () => HTMLDivElement | null; + } | null>(); + (transcriptRef as any).current = { + scrollToLatest: () => {}, + getScrollElement: () => scrollEl, + }; + + const items: TranscriptItem[] = [ + makeMessageItem("m1"), + makeMessageItem("m2"), + makeMessageItem("m3"), + ]; + + const initialArgs: UseTranscriptScrollArgs = { + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }; + + renderHook((args: UseTranscriptScrollArgs) => useTranscriptScroll(args), { + initialProps: initialArgs, + }); + + // Move scrollTop into the load-older window (<= 200 px) and fire + // a burst of scroll events WITHOUT rerendering the hook. This is the + // exact pattern that produced the bug: the parent's + // `setIsLoadingOlder(true)` cannot reach the hook between events. + act(() => { + scrollEl.scrollTop = 50; + for (let i = 0; i < 20; i += 1) { + scrollEl.dispatchEvent(new Event("scroll")); + } + }); + + expect(onLoadOlderCalls).toBe(1); + }); + + test("after a settled load (isLoadingOlder true→false), a new burst at the top fires again", () => { + let onLoadOlderCalls = 0; + const scrollEl = createScrollElement({ + scrollTop: 500, + scrollHeight: 5000, + clientHeight: 800, + }); + document.body.appendChild(scrollEl); + + const transcriptRef = { + current: { + scrollToLatest: () => {}, + getScrollElement: () => scrollEl, + }, + }; + + const items: TranscriptItem[] = [ + makeMessageItem("m1"), + makeMessageItem("m2"), + ]; + + const { rerender } = renderHook( + (args: UseTranscriptScrollArgs) => useTranscriptScroll(args), + { + initialProps: { + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }, + }, + ); + + // First burst → fires once. + act(() => { + scrollEl.scrollTop = 50; + for (let i = 0; i < 10; i += 1) { + scrollEl.dispatchEvent(new Event("scroll")); + } + }); + expect(onLoadOlderCalls).toBe(1); + + // Parent flips isLoadingOlder=true (simulating fetch in flight). + rerender({ + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: true, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }); + + // Burst while loading → blocked. + act(() => { + for (let i = 0; i < 10; i += 1) { + scrollEl.dispatchEvent(new Event("scroll")); + } + }); + expect(onLoadOlderCalls).toBe(1); + + // Page lands: items prepended, isLoadingOlder flips back to false. + // Scroll restoration would normally bump scrollTop away from the + // top — simulate that here so the next burst is at the new top. + const itemsAfterPrepend: TranscriptItem[] = [ + makeMessageItem("m0"), + ...items, + ]; + scrollEl.scrollTop = 400; // restored to mid-page + rerender({ + transcriptRef: transcriptRef as any, + items: itemsAfterPrepend, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }); + + // User scrolls back to the top → a NEW burst should fire onLoadOlder + // exactly once more (total = 2). + act(() => { + scrollEl.scrollTop = 30; + for (let i = 0; i < 10; i += 1) { + scrollEl.dispatchEvent(new Event("scroll")); + } + }); + expect(onLoadOlderCalls).toBe(2); + }); + + test("failed load (true→false without items change) still releases the lock", () => { + let onLoadOlderCalls = 0; + const scrollEl = createScrollElement({ + scrollTop: 50, + scrollHeight: 5000, + clientHeight: 800, + }); + document.body.appendChild(scrollEl); + + const transcriptRef = { + current: { + scrollToLatest: () => {}, + getScrollElement: () => scrollEl, + }, + }; + + const items: TranscriptItem[] = [makeMessageItem("m1")]; + + const { rerender } = renderHook( + (args: UseTranscriptScrollArgs) => useTranscriptScroll(args), + { + initialProps: { + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }, + }, + ); + + act(() => { + scrollEl.dispatchEvent(new Event("scroll")); + }); + expect(onLoadOlderCalls).toBe(1); + + // Loading flips on, then back off WITHOUT items changing (fetch + // failed or returned an empty page). The lock should release so a + // user-initiated retry can fire. + rerender({ + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: true, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }); + rerender({ + transcriptRef: transcriptRef as any, + items, + conversationId: "c1", + hasMore: true, + isLoadingOlder: false, + onLoadOlder: () => { + onLoadOlderCalls += 1; + }, + }); + + 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 134b041060a..99555bb0d84 100644 --- a/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts +++ b/apps/web/src/domains/chat/transcript/use-transcript-scroll.ts @@ -273,6 +273,28 @@ export function useTranscriptScroll( // ---------- Saved anchor for prepend preservation --------------------- const savedAnchorRef = useRef(null); + // ---------- Synchronous load-older in-flight lock --------------------- + // + // 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 + // 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 + // re-fires overwrite `savedAnchorRef` with progressively different + // scrollTop values mid-gesture, producing jittery scroll restoration + // after the prepend. + // + // 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). + const loadOlderInFlightRef = useRef(false); + const prevIsLoadingOlderRef = useRef(isLoadingOlder); + // ---------- Previous items ref (for change detection) ----------------- const previousItemsRef = useRef(items); @@ -358,6 +380,19 @@ export function useTranscriptScroll( 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 action = decideItemsChangeAction({ items, previousItems: prev, @@ -451,7 +486,13 @@ export function useTranscriptScroll( // Underfilled viewport: no scroll event can fire, so kick load-older // from here. Skip the anchor save during auto-pin (resize observer re-pins). - if (classification.shouldLoadOlder) { + // Gate on the synchronous in-flight lock so a chain-load sequence + // (response prepends → items change → effect re-runs near top) cannot + // double-fire on a single render cycle. + if ( + classification.shouldLoadOlder && + !loadOlderInFlightRef.current + ) { if (!shouldAutoPinRef.current) { const firstItem = items[0]; if (firstItem) { @@ -462,6 +503,7 @@ export function useTranscriptScroll( }; } } + loadOlderInFlightRef.current = true; onLoadOlder(); } } @@ -610,7 +652,7 @@ export function useTranscriptScroll( setShowScrollToLatest(classification.showScrollToLatest); } - if (classification.shouldLoadOlder) { + if (classification.shouldLoadOlder && !loadOlderInFlightRef.current) { // Capture the top-most visible item AND the current scrollHeight so // the items-effect can restore the reader's viewport after the // older-page prepend lands. The restore is @@ -623,6 +665,12 @@ export function useTranscriptScroll( scrollHeight: metrics.scrollHeight, }; } + // Flip the synchronous lock BEFORE firing so re-entrant scroll + // events within the same gesture see the in-flight state + // immediately, without waiting for React to commit the parent's + // `setIsLoadingOlder(true)` and the mirror useEffect to refresh + // `latestRef`. + loadOlderInFlightRef.current = true; latest.onLoadOlder(); } }, []);