refactor(web): scroll to bottom on transcript container DOM attach by vellum-apollo-bot[bot] · Pull Request #32239 · vellum-ai/vellum-assistant

vellum-apollo-bot · 2026-05-27T13:57:14Z

First imperative replacement piece for useDeprecatedTranscriptScroll. When the transcript scroll container is attached to the DOM in the context of a conversation, snap to bottom. This is the DOM lifecycle event, not a React state change — key={conversationId} on the scroll container ensures the <div> re-attaches on conversation switch and on fresh detail-page loads, and the callback ref fires at attach time before paint.

Why

Toggling the transcriptScrollController flag back on revealed that both conversation switch and conversation detail page refresh were leaving the transcript scrolled to the top of the stream instead of opening at the latest message. That's feature #4 of 11 in the imperative-refactor tracker — and the first one we're picking off.

What

Renamed transcript-scroll-flag.ts → transcript-scroll.ts. Single home for all scroll utilities, per Vargas. Existing exports (getTranscriptScrollControllerEnabled, setTranscriptScrollControllerEnabled, TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) move over unchanged.
Added getTranscriptScrollContainerKey(conversationId) — returns the conversation id when the controller flag is ON, undefined when OFF (preserves the deprecated-hook era reconciliation where the container doesn't remount on switch).
Added useTranscriptScrollContainerRef(forwardTo) — callback ref that forwards the attached <div> to forwardTo.current (so existing imperative callers like the useImperativeHandle, pull-to-refresh, and debug API keep working) and, when the controller flag is ON, sets el.scrollTop = el.scrollHeight at attach time.
Wired both utilities into Transcript and threaded conversationId through chat-route-content.tsx.

The gating against TRANSCRIPT_SCROLL_CONTROLLER_ENABLED lives inside the utility bodies. The component file imports them without knowing about the flag.

Behavior matrix

Flag	Conversation switch	Fresh page load on conversation URL
OFF (default)	Deprecated hook's `useLayoutEffect([conversationId])` resets to bottom — unchanged	Deprecated hook's conversation-reset effect — unchanged
ON	Container re-attaches (new key), callback ref sets `scrollTop = scrollHeight` before paint — fixed	Container mounts, callback ref sets `scrollTop = scrollHeight` before paint — fixed

Trigger axis

useEffect keyed on conversationId is state-driven. A callback ref that fires on DOM attach is event-driven — same axis as the existing ResizeObserver and wheel/touchmove listeners in the deprecated hook. That's the direction the whole migration is moving.

Tests

transcript-scroll.test.ts — asserts the key helper short-circuits to undefined when the controller flag is OFF (the test environment's resolved value).
Existing transcript.test.tsx, use-deprecated-transcript-scroll.test.ts, debug-api.test.ts, chat-scroll-area.test.tsx, chat-body.test.tsx all green.
bunx tsc --noEmit: same 18 pre-existing errors on origin/main, none introduced.
bun run lint: clean.

What this doesn't try to handle

Async height settling after the initial snap (late images, streaming tokens arriving after the first paint) — that's covered by features #1 (pin-to-latest) and #9 (content-resize re-pin), both still in the deprecated hook. Migration tracker: /workspace/scratch/scroll-imperative-tracker.md.

First imperative replacement piece for `useDeprecatedTranscriptScroll`: when the transcript scroll container is attached to the DOM in the context of a conversation, snap to bottom. This is the DOM lifecycle event, not a React state change — `key={conversationId}` ensures the `<div>` re-attaches on conversation switch and on a fresh detail-page load, and the callback ref fires at attach time before paint. This restores the "open every conversation view at latest" behavior under the controller flag. Without it, the no-op deprecated hook was leaving the transcript scrolled to the top on both conversation switch and page refresh. Consolidation: renamed `transcript-scroll-flag.ts` → `transcript-scroll.ts` and moved the two new utilities into the same file. All scroll utilities now live in one place; the gating against `TRANSCRIPT_SCROLL_CONTROLLER_ENABLED` lives inside each utility's body so component files import them without branching on the flag. Tracker: /workspace/scratch/scroll-imperative-tracker.md (feature #4 of 11).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80dcc82b32

ℹ️ 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".

chatgpt-codex-connector · 2026-05-27T13:59:41Z

+      forwardTo.current = el;
+      if (!TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) return;
+      if (!el) return;
+      el.scrollTop = el.scrollHeight;


Re-pin after latest-turn height is seeded

When the transcript scroll controller flag is enabled, this attach-time snap runs before useViewportMinHeight seeds the latest-turn row's minHeight from clientHeight; for common conversations where the latest turn is shorter than the viewport, that subsequent render increases scrollHeight after this line has already clamped scrollTop to the old max. Because the flag also disables useDeprecatedTranscriptScroll, there is no content resize observer left to re-pin, so fresh loads/switches still land above the latest message despite the intended fix.

Useful? React with 👍 / 👎.

Good catch — confirmed by reading use-viewport-min-height.ts: setHeight(clientHeight) runs in a post-paint useEffect, which fires after the callback ref. The LatestTurnRow re-renders with the seeded minHeight, scrollHeight grows by ~viewportHeight, and the attach-time snap is left above the actual bottom.

Addressed in c0e13f4. attachSnapToLatest now sets up a ResizeObserver on the content wrapper that re-snaps on every content height change until the first wheel / touchmove / keydown on the container. The hook surface grew to take both refs and return two callback refs (scrollContainerCallbackRef for the outer scroll container, contentCallbackRef for the inner wrapper) so the observer can attach to the right element.

Also covers late image loads and streaming token growth as side benefits during the auto-pin window.

dvargasfuertes · 2026-05-27T15:27:05Z

    return (
      <div
-        ref={scrollRef}
+        key={getTranscriptScrollContainerKey(conversationId)}


It's safe to always make this just key={conversationId}

Done in c0e13f4 — dropped the helper, container now always uses key={conversationId}. OFF path verified safe: deprecated hook still resets state on conversationId change and the items-effect re-fires scrollToLatest inside the auto-pin window after the remount.

dvargasfuertes · 2026-05-27T15:31:02Z


 import type { TranscriptHandle } from "@/domains/chat/transcript/transcript";
-import { TRANSCRIPT_SCROLL_CONTROLLER_ENABLED } from "@/domains/chat/transcript/transcript-scroll-flag";
+import { TRANSCRIPT_SCROLL_CONTROLLER_ENABLED } from "@/domains/chat/transcript/transcript-scroll";


let's keep the -flag stuff in its own file so we can cleanly remove it when we're done

Done in c0e13f4 — restored transcript-scroll-flag.ts as a standalone 80-line file containing only the storage + reload logic. transcript-scroll.ts now imports TRANSCRIPT_SCROLL_CONTROLLER_ENABLED from it. Clean rm target when the migration completes.

…ed-then-grow Three review comments addressed: • dvargasfuertes (transcript.tsx:262) — drop `getTranscriptScrollContainerKey()` helper. The scroll container now always remounts on conversation switch via `key={conversationId}`. With the flag OFF the deprecated hook still resets pin state on conversationId change and the items-effect re-fires scrollToLatest inside the auto-pin window, so the OFF path is unchanged behaviorally. • dvargasfuertes (use-deprecated-transcript-scroll.ts:37) — restored `transcript-scroll-flag.ts` as its own file containing only the flag storage + reload-on-toggle logic. `transcript-scroll.ts` imports `TRANSCRIPT_SCROLL_CONTROLLER_ENABLED` from it. When the migration is done, deleting the flag file is one rm. • chatgpt-codex-connector P2 (transcript-scroll.ts:127) — real race: `useViewportMinHeight` seeds LatestTurnRow's `minHeight = clientHeight` in a post-paint effect, growing scrollHeight after the attach-time snap. With the controller flag ON the deprecated hook's ResizeObserver no longer runs, so nothing re-pins. Fix: `attachSnapToLatest` now sets up a `ResizeObserver` on the content wrapper that re-snaps on every content height change until the first `wheel`/`touchmove`/`keydown` on the container. New hook shape: `useTranscriptScrollOnAttach({ scrollContainerRef, contentRef })` returns `{ scrollContainerCallbackRef, contentCallbackRef }`. Wired both into `Transcript` — container callback ref forwards only; content callback ref triggers `attachSnapToLatest` (the pure imperative function the hook delegates to). Six new tests cover initial snap, re-snap on resize, three gesture-disengage paths, teardown, and the no-ResizeObserver fallback. Local: lint clean; tsc same 18 pre-existing errors as main.

dvargasfuertes · 2026-05-27T16:52:34Z

+  /** Active conversation id. Drives the scroll container's React key so
+   *  the DOM element re-attaches on conversation switch — that attach
+   *  is the DOM lifecycle event the scroll utilities listen for. */
+  conversationId?: string | null;


make this requrired and delete the docstring

Done in 6467ec2. Type is now conversationId: string | null (no ?, docstring gone). Production callsite already passes activeConversationId: string | null so no change there. 14 test callsites updated to pass conversationId={null} explicitly.

Per dvargasfuertes (transcript.tsx:46). Type stays `string | null` — production callsite passes `activeConversationId: string | null` and nullable conversations are still a valid state (empty conversation pre-mint). Test callsites updated to pass `conversationId={null}` (transcript.test.tsx ×5, transcript-subagent-inline.test.tsx ×9).

#32239 ref-order bug Tackles features #5 + #6 + #7 from the scroll-imperative migration tracker and patches a silent bug in PR #32239 where the controller flag's new path never actually ran. ## #32239 ref-order bug fix React fires callback refs **child-before-parent** on mount. The merged shape attached inside `contentCallbackRef` (inner, fires first) and read `scrollContainerRef.current`, which was still null because the outer ref hadn't fired yet — the `if (!container) return` guard hit and `attachSnapToLatest` silently no-op'd. The bug stayed silent because the controller flag is OFF by default; nobody exercised the new path. Pure-function unit tests against fake elements couldn't catch it either. Fix: move all attach + teardown into `scrollContainerCallbackRef` (outer, fires last). Both refs are set by then. ## #5 + #6 — Load-older on scroll-to-top + anchor-restore after prepend These are two halves of the same lifecycle and ship together. New `attachLoadOlderAnchor`: - Scroll listener: when scrolled within 200px of top and `getState().hasMore && !isLoadingOlder`, snapshot `{ key: items[0].key, scrollTop, scrollHeight }` and call `onLoadOlder()`. - Content `ResizeObserver`: when a saved anchor exists AND the first-item key has changed (clean prepend signal), restore `scrollTop = savedScrollTop + (newScrollHeight - savedScrollHeight)` and clear the anchor. The first-item-key change is the load-bearing signal — no inference over arbitrary items diffs. Streaming growth, image loads, and in-place expansion all fire the same ResizeObserver but don't change `items[0].key`, so restoration correctly skips them. The `if (savedAnchor !== null) return` guard at the top of the scroll handler prevents double-firing during the in-flight fetch window. ## #7 — Underfilled-viewport chain-load New `attachUnderfilledChainLoad`: - Content `ResizeObserver` checks `scrollHeight <= clientHeight && hasMore && !isLoadingOlder` on every content tick. - Fires `onLoadOlder()` repeatedly until the viewport overflows or `hasMore` flips. Self-throttling via `isLoadingOlder`. - Initial `tryLoad()` call covers the attached-with-already-underfilled case before any RO tick. ## Hook surface `useTranscriptScrollOnAttach` grew to accept pagination state (`items`, `hasMore`, `isLoadingOlder`, `onLoadOlder`) and mirrors them into a `stateRef` updated each render. The three attachables read fresh state via `getState` closures so they never need to re-bind listeners on prop change. ## Flag read moved to call time `getTranscriptScrollControllerEnabled()` is now called inside the attach callback (was a module-load const). Justification: this is an early-return guard, not a hook-dispatch site — no rules-of-hooks concern. Side benefit: tests can toggle via `localStorage` without fighting module-import ordering. ## Tests - **17 new unit tests** in `transcript-scroll.test.ts` for the three attachables against fake elements. - **4 new integration tests** in `use-transcript-scroll-on-attach.test.tsx` that mount the hook through React. These would have caught the #32239 ref-order bug — verified by temporarily reverting to the broken shape; all 4 failed as expected. 20 tests total in the new file pair, 4 pre-existing TranscriptMessageBody failures on `origin/main` (tracker C3, unrelated).

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.

…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>

chatgpt-codex-connector Bot reviewed May 27, 2026

View reviewed changes

dvargasfuertes requested changes May 27, 2026

View reviewed changes

dvargasfuertes reviewed May 27, 2026

View reviewed changes

dvargasfuertes approved these changes May 27, 2026

View reviewed changes

dvargasfuertes merged commit 595071c into main May 27, 2026
7 checks passed

dvargasfuertes deleted the apollo/transcript-mount-scroll-to-bottom branch May 27, 2026 18:42

vellum-apollo-bot Bot mentioned this pull request May 27, 2026

feat(web/chat): imperative load-older via ResizeObserver #32306

Closed

vellum-apollo-bot Bot mentioned this pull request May 28, 2026

refactor(web/chat): drop transcript-scroll experiment, rename deprecated hook #32374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(web): scroll to bottom on transcript container DOM attach#32239

refactor(web): scroll to bottom on transcript container DOM attach#32239
dvargasfuertes merged 3 commits into
mainfrom
apollo/transcript-mount-scroll-to-bottom

vellum-apollo-bot Bot commented May 27, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 27, 2026

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Uh oh!

dvargasfuertes May 27, 2026

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Uh oh!

dvargasfuertes May 27, 2026

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Uh oh!

dvargasfuertes May 27, 2026

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

vellum-apollo-bot Bot commented May 27, 2026

Why

What

Behavior matrix

Trigger axis

Tests

What this doesn't try to handle

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 27, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 27, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 27, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant