feat(web/chat): imperative load-older via ResizeObserver#32306
feat(web/chat): imperative load-older via ResizeObserver#32306vellum-apollo-bot[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79fd2d4f27
ℹ️ 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".
| const onScroll = (): void => { | ||
| if (savedAnchor !== null) return; // already pending a restore |
There was a problem hiding this comment.
Clear pending anchor when older load does not prepend
When an older-page request fails, is cancelled, or returns no prepended items while hasMore remains true, savedAnchor never gets cleared because the only clear path is the ResizeObserver branch where the first item key changes. From then on, this early return suppresses every near-top scroll and the user cannot retry loading older history until the transcript remounts. Clear the pending anchor when the load settles without a prepend, or otherwise allow a retry once isLoadingOlder is false again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dissolved by the simplification — the new shape has no saved anchor at all. attachLoadOlderOnTop is a stateless RO that checks container.scrollTop on every tick and calls onLoadOlder() if near the top. There's no savedAnchor ref to leak. Force-pushed 228f351915 with the rewrite; this thread's file:line no longer exists.
| export function useTranscriptScrollOnAttach(args: { | ||
| scrollContainerRef: MutableRefObject<HTMLDivElement | null>; | ||
| contentRef: MutableRefObject<HTMLDivElement | null>; | ||
| items: ReadonlyArray<TranscriptItem>; |
There was a problem hiding this comment.
This looks like we are reintroducing the same imperative issues especially with the useEffect below. Let's dramatically simplify this PR to focus on just the load older behavior when the resize observer detects we are at the top
There was a problem hiding this comment.
Yep — the useEffect mirroring props into stateRef is exactly the latestRef smell from the deprecated hook. Bug-shape audit thanks me later. Going to:
- Rip out
attachLoadOlderAnchor(scroll listener + anchor save/restore) andattachUnderfilledChainLoadentirely. - Add one new function:
attachLoadOlderOnTop({ container, content, onLoadOlder })— a ResizeObserver on the content, checksscrollTop < thresholdon every tick, firesonLoadOlder(). No state. No anchor. - Wire it via a
useEffect([hasMore, isLoadingOlder, onLoadOlder])inside the hook. No state mirror — the effect re-runs naturally on prop change with fresh closures. - Drop the
itemsprop from the hook surface (not needed anymore). - Revert
attachSnapToLatestand thescrollContainerCallbackRef/contentCallbackRefplumbing to exactly what's onmainfrom refactor(web): scroll to bottom on transcript container DOM attach #32239. The ref-order bug fix moves to its own PR.
Things naturally falling out:
- Chain-load (feat: add top-level setup.sh script #7) — initial RO tick fires when observe() attaches with underfilled content. Loop continues because
isLoadingOlderflipping false re-runs the effect, fresh RO does another initial tick, etc. - Anchor restore (Replace all references of
GCP_REGIONwithus-central-1#6) — drops out. Browser's nativeoverflow-anchorhandles it for chat. If we hit cases where it doesn't, that's a separate PR with explicit reasoning. - Codex P2 (clear-pending-anchor) — N/A because there's no saved anchor.
One known regression: user scrolling up to top with NO content change won't trigger (RO doesn't fire on pure scroll). Streaming/image-load/anything else fires it. Want me to add a scroll listener for that case here, or follow-up PR with the rest of the migration? I'll default to follow-up since you said RO-only.
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.
79fd2d4 to
228f351
Compare
|
Force-pushed
Stat: Known follow-ups left explicit:
Re-requesting review. |
|
Closing — even in the simplified shape this isn't materially different from what the deprecated hook already does for the load-older trigger, just split across files. Migration cost without architectural win. New direction: rip out the |
…ted hook The parallel imperative scroll module (`transcript-scroll.ts` + `transcript-scroll-flag.ts`) was an experiment to migrate `useDeprecatedTranscriptScroll` to a stateless attachable model. In practice the simplified shape ended up functionally equivalent to a chunk of the existing hook, just split across files — migration cost without an architectural win. Reverting that direction: - Delete `transcript-scroll.ts`, `transcript-scroll-flag.ts`, and `transcript-scroll.test.ts` (introduced in PR #32239, extended + reverted in PR #32306). - Remove the kill-switch + `DISABLED_RESULT` short-circuit at the top of the hook — there's no parallel implementation to flag-gate anymore. - Remove the callback-ref plumbing in `transcript.tsx` (PR #32239 shape) and go back to direct `ref={scrollRef}` / `ref={contentRef}` attachment. - Drop `toggleTranscriptScrollController` from the debug-api surface and its setter import. - Update the doc reference in `impersonate-version-flag.ts`. Rename the deprecated hook to the canonical name now that there's no migration in flight: - `use-deprecated-transcript-scroll.ts` → `use-transcript-scroll.ts` - `useDeprecatedTranscriptScroll` → `useTranscriptScroll` - `UseDeprecatedTranscriptScrollArgs/Return` types renamed accordingly - All import sites updated (`chat-route-content.tsx`, `debug-api.ts`, `debug-api.test.ts`, the hook's own test file) Forward plan: QA the remaining bugs in the existing hook and progressively simplify in place.
…ted hook (#32374) The parallel imperative scroll module (`transcript-scroll.ts` + `transcript-scroll-flag.ts`) was an experiment to migrate `useDeprecatedTranscriptScroll` to a stateless attachable model. In practice the simplified shape ended up functionally equivalent to a chunk of the existing hook, just split across files — migration cost without an architectural win. Reverting that direction: - Delete `transcript-scroll.ts`, `transcript-scroll-flag.ts`, and `transcript-scroll.test.ts` (introduced in PR #32239, extended + reverted in PR #32306). - Remove the kill-switch + `DISABLED_RESULT` short-circuit at the top of the hook — there's no parallel implementation to flag-gate anymore. - Remove the callback-ref plumbing in `transcript.tsx` (PR #32239 shape) and go back to direct `ref={scrollRef}` / `ref={contentRef}` attachment. - Drop `toggleTranscriptScrollController` from the debug-api surface and its setter import. - Update the doc reference in `impersonate-version-flag.ts`. Rename the deprecated hook to the canonical name now that there's no migration in flight: - `use-deprecated-transcript-scroll.ts` → `use-transcript-scroll.ts` - `useDeprecatedTranscriptScroll` → `useTranscriptScroll` - `UseDeprecatedTranscriptScrollArgs/Return` types renamed accordingly - All import sites updated (`chat-route-content.tsx`, `debug-api.ts`, `debug-api.test.ts`, the hook's own test file) Forward plan: QA the remaining bugs in the existing hook and progressively simplify in place. Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
What
Adds the load-older trigger for the imperative transcript-scroll controller. When the controller flag is on and pagination has more, a
ResizeObserveron the transcript content firesonLoadOlderwhenever the scroll container is within 200px of the top.Gated behind
TRANSCRIPT_SCROLL_CONTROLLER_ENABLED(OFF by default). Zero behavior change for everyone today.Shape
attachLoadOlderOnTopis a pure imperative function:No state, no anchor, no scroll listener. The React idiomatic re-run on prop change replaces the deprecated hook'''s
latestRefmirror.Why this shape
The deprecated hook uses a state-mirror pattern: a
useEffectcopies fresh props into a ref each render, and imperative handlers read from that ref. That pattern produced ATL-644, PR #31826 P1, and PR #31878 in the bug-shape audit.An earlier version of this PR rebuilt the same pattern in miniature. Per review, replaced with a deps-driven
useEffectthat re-attaches with fresh closures on every prop change.Covered by construction
observe()fires once with current measurements. Underfilled transcripts trigger an older fetch.isLoadingOlderflipping back to false re-runs the effect; a fresh observer'''s initial tick measures the post-prepend layout. Continues until the viewport fills orhasMoreflips false.Out of scope (deliberate)
ResizeObserverdoesn'''t fire on pure scroll events. A scroll-listener trigger ships in its own PR (S2 in the Solve-Chat-SSE plan).overflow-anchorfor chat for now. If we hit a case where it doesn'''t hold the reading row, that'''s its own PR with explicit reasoning — not a genericlatestRefrebuild.attachSnapToLatestreverted to exactly what'''s onmain. The ref-order bug stays for a dedicated fix PR.Tests
transcript-scroll.test.ts— 5 new pure-function tests: fire-on-initial-tick, threshold-skip, streaming-multi-tick, teardown-disconnects, RO-unavailable-fallback.use-transcript-scroll-on-attach.test.tsx— 5 new React integration tests using@testing-library/react: gated when hasMore=false, gated while isLoadingOlder=true, fires onLoadOlder on initial RO tick, tears down + re-attaches on toggle, teardown on unmount.All 10 new tests pass.
tsc+eslintclean on touched files. Same 4 pre-existingTranscriptMessageBodyfailures present onorigin/mainare not regressions.Stat
5 files changed, 452 insertions(+), 6 deletions(-)AGENTS.md compliance
apps/web/AGENTS.mdreviewed. New code stays insidedomains/chat/transcript/and the imperative module boundary. No cross-domain imports introduced.