Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions apps/web/src/domains/chat/transcript/use-transcript-scroll.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -454,3 +454,124 @@ describe("integration — handleScroll-style dispatch via pure helpers", () => {
expect(handle.calls.scrollToLatest.length).toBe(0);
});
});

// Regression for the items-effect underfilled-viewport kick. Captured
// scrollState from the bug report (2026-05-22):
//
// { scrollTop: 0, scrollHeight: 1370, clientHeight: 1370,
// hasMore: true, isLoadingOlder: false, itemCount: 2,
// shouldLoadOlder: true, diagnosis: "NEAR TOP ... but NOT loading" }
//
// scrollHeight === clientHeight means the user CANNOT scroll. handleScroll
// can never fire, so without an items-effect kick the load-older path
// stays gated forever even though `shouldLoadOlder=true`. These tests
// lock in the precise classification and the dispatch contract.
describe("integration — items-effect dispatch on underfilled viewport", () => {
test("scrollHeight === clientHeight with hasMore=true classifies as shouldLoadOlder=true", () => {
const c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: false, hasConversation: true },
);
expect(c.shouldLoadOlder).toBe(true);
// Underfilled also reads as pinned-to-latest (distanceFromBottom=0
// is well under the 64 px threshold) — the items effect needs to be
// willing to load AND keep `isPinned=true`.
expect(c.isPinned).toBe(true);
expect(c.distanceFromBottom).toBe(0);
});

test("items-effect-style dispatch fires onLoadOlder when classify says so", () => {
// Mirrors the hook's items-effect path: read DOM metrics, classify,
// and if `shouldLoadOlder` is true, call `onLoadOlder()`. This is
// the call site the bug was missing — `handleScroll` had it,
// the items effect did not.
const onLoadOlder = mock(() => {});
const c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: false, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).toHaveBeenCalledTimes(1);
});

test("items-effect dispatch does NOT fire when a load is already in flight", () => {
// After the first kick lands, `isLoadingOlder` flips to true. The
// next items change (e.g. an unrelated re-render) must NOT fire a
// second kick — `classification.shouldLoadOlder` is the only gate
// and it must reflect that.
const onLoadOlder = mock(() => {});
const c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: true, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).not.toHaveBeenCalled();
});

test("items-effect dispatch stops kicking once viewport overflows", () => {
// After enough older pages prepend, scrollHeight exceeds
// clientHeight + LOAD_OLDER_THRESHOLD_PX (200 px). The cascade
// terminates: classify reports shouldLoadOlder=false and the kick
// does not fire.
const onLoadOlder = mock(() => {});
const c = classifyScrollPosition(
// scrollTop bumped well below the 200 px threshold (the auto-pin
// window will have re-pinned to latest, so scrollTop sits near
// the bottom of an overflowing viewport).
{ scrollTop: 5000, scrollHeight: 6370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: false, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).not.toHaveBeenCalled();
});

test("items-effect dispatch stops kicking once hasMore=false", () => {
// Conversation has fewer messages than fit in the viewport AND no
// older history exists. The cascade must terminate cleanly with no
// further calls.
const onLoadOlder = mock(() => {});
const c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: false, isLoadingOlder: false, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).not.toHaveBeenCalled();
});

test("chain-load: false→true→false sequence kicks again when still underfilled", () => {
// Codex P1 regression. The dispatch site must observe the CURRENT
// render's isLoadingOlder flag, not a stale ref. Sequence:
// 1. Initial: isLoadingOlder=false, items underfill → kick.
// 2. Loading: isLoadingOlder=true, same items → no kick.
// 3. Prepended: isLoadingOlder=false, items grew but still
// underfill → MUST kick again to chain-load.
// If the dispatch reads a stale flag from the previous render, step 3
// sees isLoadingOlder=true and silently skips the kick, stranding
// conversations that need multiple older pages to overflow.
const onLoadOlder = mock(() => {});

// Step 1: initial, underfilled
let c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: false, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).toHaveBeenCalledTimes(1);

// Step 2: loading in flight
c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 1370, clientHeight: 1370 },
{ hasMore: true, isLoadingOlder: true, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).toHaveBeenCalledTimes(1);

// Step 3: prepend landed but still underfilled
c = classifyScrollPosition(
{ scrollTop: 0, scrollHeight: 2200, clientHeight: 2200 },
{ hasMore: true, isLoadingOlder: false, hasConversation: true },
);
if (c.shouldLoadOlder) onLoadOlder();
expect(onLoadOlder).toHaveBeenCalledTimes(2);
});
});
46 changes: 35 additions & 11 deletions apps/web/src/domains/chat/transcript/use-transcript-scroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,10 @@ 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. Reading via the latestRef avoids putting the
// hasMore/isLoadingOlder/conversationKey flags in the dep array.
// 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.
const el = transcriptRef.current?.getScrollElement();
if (el) {
const classification = classifyScrollPosition(
Expand All @@ -380,22 +382,44 @@ export function useTranscriptScroll(
clientHeight: el.clientHeight,
},
{
hasMore: latestRef.current.hasMore,
isLoadingOlder: latestRef.current.isLoadingOlder,
hasConversation: latestRef.current.conversationKey !== null,
hasMore,
isLoadingOlder,
hasConversation: conversationKey !== null,
},
);
if (classification.isPinned !== latestRef.current.isPinnedToLatest) {
if (classification.isPinned !== isPinnedToLatest) {
setIsPinnedToLatest(classification.isPinned);
}
if (
classification.showScrollToLatest !==
latestRef.current.showScrollToLatest
) {
if (classification.showScrollToLatest !== showScrollToLatest) {
setShowScrollToLatest(classification.showScrollToLatest);
}

// 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) {
if (!shouldAutoPinRef.current) {
Comment on lines +399 to +400

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use current loading flags before chaining underfilled loads

This new items-effect load kick can stall after the first page because it relies on classification computed from latestRef.current (which is only refreshed in a later useEffect). In the common sequence isLoadingOlder: false -> true -> false while prepended items arrive, the layout effect on the prepend render still sees the previous isLoadingOlder=true snapshot, so shouldLoadOlder is false and no second kick fires even if the viewport is still unscrollable and hasMore remains true. That leaves the transcript stuck again for conversations that need multiple older pages to overflow.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in e021b2c.

The items effect now reads hasMore, isLoadingOlder, conversationKey, items, and onLoadOlder from the closure instead of latestRef.current, and those flags are in the dep array. The stale-snapshot window you described (the prepend render runs the layout effect before the latestRef-refreshing useEffect fires) was a real bug — confirmed by tracing the false → true → false sequence.

Added a regression test (chain-load: false→true→false sequence kicks again when still underfilled) that walks the three-step sequence and asserts a second kick fires on step 3 when the viewport is still underfilled. Would have caught this exact issue if the dispatch read from a stale ref.

const firstItem = items[0];
if (firstItem) {
savedAnchorRef.current = {
key: firstItem.key,
scrollTop: el.scrollTop,
scrollHeight: el.scrollHeight,
};
}
}
onLoadOlder();
}
}
}, [items, conversationKey, transcriptRef]);
}, [
items,
conversationKey,
transcriptRef,
hasMore,
isLoadingOlder,
onLoadOlder,
isPinnedToLatest,
showScrollToLatest,
]);

// -----------------------------------------------------------------------
// Container resize re-pin. When the scroll container resizes (e.g. the
Expand Down
Loading