fix(chat): kick load-older from items effect when viewport cannot scroll#31826
Conversation
Captures the exact scenario from the bug report (scrollHeight === clientHeight, scrollTop=0, hasMore=true, isLoadingOlder=false, shouldLoadOlder=true) and locks in the dispatch contract for the items effect's load-older kick: it fires when classify says so, and stops cleanly once isLoadingOlder=true, hasMore=false, or the viewport overflows the 200 px threshold.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ab8e1f1b3
ℹ️ 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".
| if (classification.shouldLoadOlder) { | ||
| if (!shouldAutoPinRef.current) { |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
The items-effect classification was reading hasMore/isLoadingOlder/ conversationKey/items from latestRef, but latestRef is refreshed by a useEffect that runs AFTER this useLayoutEffect. On the prepend render (isLoadingOlder: true → false), the stale ref kept the gate as true, shouldLoadOlder evaluated to false, and the kick was silently skipped — stalling conversations that need multiple older pages to overflow. Switch to closure reads and add the flags to the dep array so the effect also re-runs on loading-state transitions. Add a regression test that walks the false→true→false sequence and asserts a second kick fires when the viewport is still underfilled.
…hind a dev flag (PR 1 of 3)
Adds an imperative `TranscriptScrollController` class that owns the
DOM-shaped concerns of transcript scrolling — scroll/content
ResizeObservers, user-input gesture listeners, the auto-pin window
timer, and the saved anchor snapshot. Plus a parallel
`useTranscriptScrollController` hook that drives the controller and
exposes the same `{ isPinnedToLatest, showScrollToLatest,
scrollToLatest, handleScroll }` surface as the legacy hook.
The legacy `useTranscriptScroll` is **untouched**. The orchestrator
picks one of the two hooks at module-load time based on a localStorage
flag, resolved through a small `useTranscriptScrollDispatch` indirection
so React sees a stable hook identity across renders.
Toggling at runtime is exposed via a new `window._vellumDebug.flags`
namespace:
window._vellumDebug.flags.toggleTranscriptScrollController() // flip
window._vellumDebug.flags.toggleTranscriptScrollController(true) // force on
window._vellumDebug.flags.toggleTranscriptScrollController(false) // force off
The toggle persists to localStorage, logs the new value, and reloads
the page so the dispatcher re-resolves cleanly. **Default: off.**
Production users get the legacy hook until we flip the default.
This PR ships the parallel implementation at near-feature parity with
the legacy hook so the toggle is a fair comparison. PR 2 will evolve
the new path to drive the controller via explicit imperative method
calls from the orchestrator (`onConversationSwitched`,
`onMessageSubmitted`, `beforeOlderPageFetch`,
`afterOlderPageApplied`) — replacing the inferred dispatches in the
kitchen-sink items `useLayoutEffect` that is the bug factory called
out in scratch/scroll-imperative-spec.md §2. PR 3 splits the
underfilled-viewport chain-load into a pagination-owned hook. When
the new path reaches parity the flag flips on permanently and
`use-transcript-scroll.ts` is deleted.
## Files
NEW:
- `apps/web/src/domains/chat/transcript/transcript-scroll-controller.ts`
- `apps/web/src/domains/chat/transcript/transcript-scroll-controller.test.ts`
- `apps/web/src/domains/chat/transcript/use-transcript-scroll-controller.ts`
- `apps/web/src/domains/chat/transcript/use-transcript-scroll-dispatch.ts`
- `apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts`
MODIFIED:
- `apps/web/src/domains/chat/components/chat-route-content.tsx` — calls
`useTranscriptScrollDispatch` instead of `useTranscriptScroll`.
- `apps/web/src/domains/chat/utils/debug-api.ts` — `installVellumDebugApi`
now takes a `flagsApi` argument; attaches/detaches `_vellumDebug.flags`
alongside `chat` and `events`.
- `apps/web/src/domains/chat/utils/debug-api.test.ts` — install-test
callsites updated to pass a fake `flagsApi`; `DebugWindow` type extended.
UNTOUCHED:
- `apps/web/src/domains/chat/transcript/use-transcript-scroll.ts` — same as
origin/main.
## Tests
- 23 new unit tests in `transcript-scroll-controller.test.ts` — happy-dom,
cover element attach/detach idempotency, ResizeObserver wiring,
user-input gestures, anchor bookkeeping, auto-pin timer expiry &
re-engage & detach cancellation.
- 41 existing tests in `use-transcript-scroll.test.ts` untouched & passing.
- `debug-api.test.ts` install tests extended for the new `flags`
attach/detach + identity-gate.
ATL-644 / PR #31826 / PR #31878 chain.
Adds the load-older trigger for the imperative transcript-scroll controller, gated behind the existing TRANSCRIPT_SCROLL_CONTROLLER flag. When the controller is enabled and pagination has more, a ResizeObserver on the transcript content fires `onLoadOlder` whenever the scroll container is within 200px of the top. ### Why this shape The deprecated hook uses a 'latestRef' state-mirror pattern: a `useEffect` copies fresh props into a ref each render, and imperative handlers read from that ref. This shape produced ATL-644, PR #31826 P1, and PR #31878 in the bug-shape audit. This PR uses the React-idiomatic shape: a `useEffect` whose deps are the pagination props. When `hasMore`/`isLoadingOlder`/ `onLoadOlder` change, the effect tears down and re-attaches with fresh closures. No state mirror, no stale-snapshot bug class. ### Covered by construction - **Initial chain-load** — `observe()` fires once with current measurements; underfilled transcripts trigger an older fetch. - **Repeat chain-load** — `isLoadingOlder` flipping back to false re-runs the effect; a fresh observer's initial tick measures the post-prepend layout. - **Streaming-triggered detection** — any content height change while the user is near the top fires the observer. ### Out of scope (deliberate) - **Pure scroll-to-top with no content change** — `ResizeObserver` doesn't fire on scroll. Scroll-listener trigger is a separate PR. - **Anchor save/restore after prepend** — relying on the browser's native `overflow-anchor` for now. If we hit cases where it doesn't hold the reading row, that's its own PR with explicit reasoning, not a generic latestRef rebuild. - **PR #32239 callback-ref order bug** — silent because the flag is off by default; will fix in a follow-up dedicated to that. ### Tests - `transcript-scroll.test.ts` — 5 new pure-function tests covering fire-on-top-tick, threshold-skip, multi-tick behavior, teardown, and ResizeObserver-unavailable fallback. - `use-transcript-scroll-on-attach.test.tsx` — 5 new React integration tests using `@testing-library/react` that mount the hook, exercise the prop-change lifecycle, and assert the observer is attached / torn down / re-attached correctly. ### AGENTS.md compliance `apps/web/AGENTS.md` reviewed. New code stays inside `domains/chat/transcript/` and the imperative module boundary. No cross-domain imports introduced.
Problem
A user reported the chat transcript stuck at the top of a short conversation with no way to load older history. Captured via
window._vellumDebug.chat.getScrollState():{ "scrollTop": 0, "scrollHeight": 1370, "clientHeight": 1370, "distanceFromBottom": 0, "distanceFromTop": 0, "isPinnedToLatest": true, "showScrollToLatest": false, "hasMore": true, "isLoadingOlder": false, "itemCount": 2, "shouldLoadOlder": true, "diagnosis": "NEAR TOP (distanceFromTop=0px) and shouldLoadOlder=true but NOT loading — scroll handler may be stuck. itemCount=2" }scrollHeight === clientHeightis the smoking gun: the user cannot scroll because the content doesn't overflow the viewport.Root Cause
use-transcript-scroll.tshas two places that read scroll metrics and classify position:handleScroll— fires on user scroll events. Acts on every classification field includingshouldLoadOlder.useLayoutEffect— runs after every items render. Acts onisPinnedandshowScrollToLatest, but silently discardsshouldLoadOlder.When initial layout puts
scrollHeight === clientHeight:handleScrollnever runs.onLoadOlder()is never called.hasMore=true && shouldLoadOlder=trueforever.The debug API's diagnosis (
"scroll handler may be stuck") was a red herring — the handler isn't stuck, it just has nothing to fire on.Fix
The items effect now also acts on
classification.shouldLoadOlder, mirroring the scroll handler's anchor-save +onLoadOlder()pattern.Anchor handling differs by context:
scrollTopto where the old first-item used to be, fighting auto-pin.handleScrolldoes, so the reader's row stays visually stable across the prepend.The cascade self-terminates without extra gating:
classification.shouldLoadOlderalready requireshasMore && !isLoadingOlder && hasConversation. Once any of those flips false (history exhausted, request in flight, or viewport finally overflows the 200 px threshold), the kick stops.Tests
Adds 5 regression tests in a new
integration — items-effect dispatch on underfilled viewportdescribe block, locking in:shouldLoadOlder=true.onLoadOlderwhen classify says so.isLoadingOlder,hasMore, and viewport overflow — so the cascade terminates cleanly in every termination scenario.