From 80dcc82b32bca7da55f97a9171a0f5d1edc82535 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 13:56:36 +0000 Subject: [PATCH 1/3] refactor(web): scroll to bottom on transcript container DOM attach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `
` 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). --- .../chat/components/chat-route-content.tsx | 1 + .../chat/transcript/transcript-scroll-flag.ts | 80 ----------- .../chat/transcript/transcript-scroll.test.ts | 33 +++++ .../chat/transcript/transcript-scroll.ts | 131 ++++++++++++++++++ .../domains/chat/transcript/transcript.tsx | 15 +- .../use-deprecated-transcript-scroll.ts | 2 +- apps/web/src/domains/chat/utils/debug-api.ts | 4 +- 7 files changed, 181 insertions(+), 85 deletions(-) delete mode 100644 apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts create mode 100644 apps/web/src/domains/chat/transcript/transcript-scroll.test.ts create mode 100644 apps/web/src/domains/chat/transcript/transcript-scroll.ts diff --git a/apps/web/src/domains/chat/components/chat-route-content.tsx b/apps/web/src/domains/chat/components/chat-route-content.tsx index 62eefd4c766..ca1be1edece 100644 --- a/apps/web/src/domains/chat/components/chat-route-content.tsx +++ b/apps/web/src/domains/chat/components/chat-route-content.tsx @@ -1124,6 +1124,7 @@ export function ChatRouteContent({ const chatTranscriptProps: TranscriptProps = { items: transcriptItems, + conversationId: activeConversationId, assistantDisplayName: assistantIdentity?.name?.trim() || undefined, expandedToolCallIds: expandedToolCallIdsRef.current, onOpenRuleEditor: handleOpenRuleEditorForToolCall, diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts b/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts deleted file mode 100644 index a0a4242e745..00000000000 --- a/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts +++ /dev/null @@ -1,80 +0,0 @@ -// Runtime flag that turns OFF the deprecated transcript scroll hook. -// -// In the current shape of this work-in-progress: -// -// • Flag OFF (default) — the orchestrator runs the deprecated hook -// `useDeprecatedTranscriptScroll`, which is the existing -// production scroll-coordination logic. Production users land -// here. We keep this in the tree so we don't regress shipping -// behavior while we redesign the replacement. -// • Flag ON — the orchestrator runs neither the deprecated hook -// nor a replacement. The transcript scrolls natively with no -// JavaScript coordination at all. No auto-pin, no anchor -// correction, no chain-load, no "Go to Newest" pill. This is the -// baseline against which the eventual controller will be built. -// -// The eventual `TranscriptScrollController` (not yet introduced) will -// land behind this same flag — flipping it on will route to the new -// path. The name `toggleTranscriptScrollController` reflects the -// destination, not the current intermediate state. -// -// Why module-load read + reload-on-toggle: -// -// • React forbids conditionally calling different hooks across -// renders. By making the dispatch decision once at module-import -// time (before any component mounts), the dispatcher resolves to a -// single function identity for the entire page lifetime. -// • Toggling without a reload would leave the DOM in an inconsistent -// intermediate state (scroll listeners attached but no longer -// handled, in-flight auto-pin timers orphaned). A page reload is -// cheap and dev-only. -// -// Surface (exposed under `window._vellumDebug.flags`): -// -// toggleTranscriptScrollController() — flip current value -// toggleTranscriptScrollController(true) — force on -// toggleTranscriptScrollController(false) — force off - -const STORAGE_KEY = "vellumDebug.flags.transcriptScrollController"; - -/** Read the flag synchronously. Safe to call at module-load time. */ -export function getTranscriptScrollControllerEnabled(): boolean { - if (typeof window === "undefined") return false; - try { - return window.localStorage.getItem(STORAGE_KEY) === "true"; - } catch { - // Private-browsing modes or sandboxed contexts can throw on - // localStorage access. Treat any throw as "flag off". - return false; - } -} - -/** Persist the flag, log the new value, and reload the page so the - * dispatcher re-resolves. `value === undefined` flips the current - * value (the most common interactive case). */ -export function setTranscriptScrollControllerEnabled(value?: boolean): boolean { - if (typeof window === "undefined") return false; - const next = - value === undefined ? !getTranscriptScrollControllerEnabled() : !!value; - try { - window.localStorage.setItem(STORAGE_KEY, String(next)); - } catch { - // Persistence failed — log and bail so the user knows their - // toggle didn't stick. - console.warn( - "[vellumDebug] failed to persist transcriptScrollController flag", - ); - return getTranscriptScrollControllerEnabled(); - } - console.info( - `[vellumDebug] transcriptScrollController = ${next} — reloading…`, - ); - window.location.reload(); - return next; -} - -/** The flag value resolved exactly once at module load. The - * dispatcher reads this constant so hook-rule order stays stable - * across the page lifetime. */ -export const TRANSCRIPT_SCROLL_CONTROLLER_ENABLED = - getTranscriptScrollControllerEnabled(); diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts b/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts new file mode 100644 index 00000000000..56987e93ea9 --- /dev/null +++ b/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts @@ -0,0 +1,33 @@ +/** + * Tests for the transcript scroll utilities. + * + * `bun:test` runs without a real DOM environment, so we exercise the + * pure surface of these utilities (the key function + the ref's + * imperative DOM mutation) using plain object fakes. The flag itself + * is read once at module load, so we cannot toggle it inside a test + * — instead the tests assert behavior at the import-time-resolved + * value and document the contract on both sides via inspection of the + * helpers' contracts. + */ + +import { describe, expect, test } from "bun:test"; + +import { + TRANSCRIPT_SCROLL_CONTROLLER_ENABLED, + getTranscriptScrollContainerKey, +} from "@/domains/chat/transcript/transcript-scroll"; + +describe("getTranscriptScrollContainerKey", () => { + test("returns undefined when the controller flag is OFF", () => { + // In the test environment there is no `window.localStorage` entry + // set, so the module-load read of the flag resolves to `false`. + // The key helper must short-circuit to `undefined` so React + // reconciles by position — preserving the deprecated-hook era + // behavior where the scroll container is not remounted on + // conversation switch. + expect(TRANSCRIPT_SCROLL_CONTROLLER_ENABLED).toBe(false); + expect(getTranscriptScrollContainerKey("conv-1")).toBeUndefined(); + expect(getTranscriptScrollContainerKey(null)).toBeUndefined(); + expect(getTranscriptScrollContainerKey(undefined)).toBeUndefined(); + }); +}); diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll.ts b/apps/web/src/domains/chat/transcript/transcript-scroll.ts new file mode 100644 index 00000000000..b7aa329b588 --- /dev/null +++ b/apps/web/src/domains/chat/transcript/transcript-scroll.ts @@ -0,0 +1,131 @@ +// Transcript scroll utilities — single home for the imperative +// replacement of `useDeprecatedTranscriptScroll`. Everything in this +// file is gated internally by `TRANSCRIPT_SCROLL_CONTROLLER_ENABLED` +// so callers in component files never branch on the flag themselves. +// +// In the current shape of this work-in-progress: +// +// • Flag OFF (default) — the orchestrator runs the deprecated hook +// `useDeprecatedTranscriptScroll`, which is the existing +// production scroll-coordination logic. The utilities exported +// from this file no-op in OFF, so callers can wire them at the +// final shape today without affecting shipping behavior. +// • Flag ON — the deprecated hook returns its no-op result and the +// utilities here take over piecewise as features migrate. The +// ON path is the baseline against which the eventual +// `TranscriptScrollController` will be built. +// +// Why module-load read + reload-on-toggle: +// +// • React forbids conditionally calling different hooks across +// renders. By making the dispatch decision once at module-import +// time (before any component mounts), the dispatcher resolves to a +// single function identity for the entire page lifetime. +// • Toggling without a reload would leave the DOM in an inconsistent +// intermediate state (scroll listeners attached but no longer +// handled, in-flight auto-pin timers orphaned). A page reload is +// cheap and dev-only. +// +// Surface (exposed under `window._vellumDebug.flags`): +// +// toggleTranscriptScrollController() — flip current value +// toggleTranscriptScrollController(true) — force on +// toggleTranscriptScrollController(false) — force off + +import { useCallback, type MutableRefObject } from "react"; + +const STORAGE_KEY = "vellumDebug.flags.transcriptScrollController"; + +/** Read the flag synchronously. Safe to call at module-load time. */ +export function getTranscriptScrollControllerEnabled(): boolean { + if (typeof window === "undefined") return false; + try { + return window.localStorage.getItem(STORAGE_KEY) === "true"; + } catch { + // Private-browsing modes or sandboxed contexts can throw on + // localStorage access. Treat any throw as "flag off". + return false; + } +} + +/** Persist the flag, log the new value, and reload the page so the + * dispatcher re-resolves. `value === undefined` flips the current + * value (the most common interactive case). */ +export function setTranscriptScrollControllerEnabled(value?: boolean): boolean { + if (typeof window === "undefined") return false; + const next = + value === undefined ? !getTranscriptScrollControllerEnabled() : !!value; + try { + window.localStorage.setItem(STORAGE_KEY, String(next)); + } catch { + // Persistence failed — log and bail so the user knows their + // toggle didn't stick. + console.warn( + "[vellumDebug] failed to persist transcriptScrollController flag", + ); + return getTranscriptScrollControllerEnabled(); + } + console.info( + `[vellumDebug] transcriptScrollController = ${next} — reloading…`, + ); + window.location.reload(); + return next; +} + +/** The flag value resolved exactly once at module load. The + * dispatcher reads this constant so hook-rule order stays stable + * across the page lifetime. */ +export const TRANSCRIPT_SCROLL_CONTROLLER_ENABLED = + getTranscriptScrollControllerEnabled(); + +// --------------------------------------------------------------------------- +// Imperative scroll utilities +// --------------------------------------------------------------------------- +// +// The utilities below are the imperative replacements that piecewise +// take over from `useDeprecatedTranscriptScroll`. They listen to DOM +// lifecycle events (element attached, scroll, resize) rather than +// reacting to React state changes, which is the whole point of the +// migration — see `/workspace/scratch/scroll-imperative-spec.md`. + +/** React key for the transcript scroll container element. Combine + * with `useTranscriptScrollContainerRef` on the same element so a + * conversation switch — or a fresh page load on a conversation + * detail route — triggers a fresh DOM attach. That attach is the + * event our callback ref listens for to scroll to the latest message. + * + * Returns `undefined` when the controller flag is OFF, which + * preserves the deprecated-hook era reconciliation (no remount on + * conversation switch; the deprecated hook handles switching via + * its own conversation-id effect). */ +export function getTranscriptScrollContainerKey( + conversationId: string | null | undefined, +): string | undefined { + if (!TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) return undefined; + return conversationId ?? "empty"; +} + +/** Callback ref for the transcript scroll container `
`. Forwards + * the attached element to `forwardTo` so existing imperative callers + * (pull-to-refresh, debug API, the transcript's own + * `useImperativeHandle`) keep working. + * + * When the controller flag is ON, additionally scrolls to bottom on + * attach. Combined with `getTranscriptScrollContainerKey()` on the + * same element, this is the imperative implementation of + * "open every conversation view at the latest message" — feature #4 + * in the migration tracker. The trigger is the DOM attach event, not + * a React state change. */ +export function useTranscriptScrollContainerRef( + forwardTo: MutableRefObject, +): (el: HTMLDivElement | null) => void { + return useCallback( + (el: HTMLDivElement | null) => { + forwardTo.current = el; + if (!TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) return; + if (!el) return; + el.scrollTop = el.scrollHeight; + }, + [forwardTo], + ); +} diff --git a/apps/web/src/domains/chat/transcript/transcript.tsx b/apps/web/src/domains/chat/transcript/transcript.tsx index b9cafe22c9c..5e012d72556 100644 --- a/apps/web/src/domains/chat/transcript/transcript.tsx +++ b/apps/web/src/domains/chat/transcript/transcript.tsx @@ -16,6 +16,10 @@ import type { TranscriptItem } from "@/domains/chat/transcript/types"; import { LatestTurnRow } from "@/domains/chat/transcript/latest-turn-row"; import { PullRefreshSpinner } from "@/domains/chat/transcript/pull-refresh-spinner"; import { TranscriptRow } from "@/domains/chat/transcript/transcript-row"; +import { + getTranscriptScrollContainerKey, + useTranscriptScrollContainerRef, +} from "@/domains/chat/transcript/transcript-scroll"; import { PULL_THRESHOLD_PX, usePullToRefresh, @@ -39,6 +43,10 @@ export type RefreshOutcome = export interface TranscriptProps { items: TranscriptItem[]; + /** 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; assistantDisplayName?: string | null; onSecretSubmit: (requestId: string, value: string) => void; onConfirmationDecision: (requestId: string, decision: string) => void; @@ -150,9 +158,11 @@ export interface TranscriptHandle { export const Transcript = forwardRef( function Transcript(props, ref) { - const { items, onPullRefresh, pullRefreshEnabled, ...rest } = props; + const { items, conversationId, onPullRefresh, pullRefreshEnabled, ...rest } = + props; const scrollRef = useRef(null); const contentRef = useRef(null); + const scrollContainerRef = useTranscriptScrollContainerRef(scrollRef); const viewportMinHeight = useViewportMinHeight(scrollRef); const pullEnabled = !!pullRefreshEnabled && !!onPullRefresh; @@ -249,7 +259,8 @@ export const Transcript = forwardRef( return (
diff --git a/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts b/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts index c0df817879e..50eb60b0a3b 100644 --- a/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts +++ b/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts @@ -34,7 +34,7 @@ import { import type { TranscriptItem } from "@/domains/chat/transcript/types"; 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"; export type { TranscriptHandle }; diff --git a/apps/web/src/domains/chat/utils/debug-api.ts b/apps/web/src/domains/chat/utils/debug-api.ts index ff3fc52eb35..aafa1f91c06 100644 --- a/apps/web/src/domains/chat/utils/debug-api.ts +++ b/apps/web/src/domains/chat/utils/debug-api.ts @@ -43,7 +43,7 @@ import type { import { recordChatDiagnostic } from "@/domains/chat/utils/diagnostics"; import type { DisplayMessage } from "@/domains/chat/utils/reconcile"; import type { ReconcileActiveConversationResult } from "@/domains/chat/hooks/use-message-reconciliation"; -import { setTranscriptScrollControllerEnabled } from "@/domains/chat/transcript/transcript-scroll-flag"; +import { setTranscriptScrollControllerEnabled } from "@/domains/chat/transcript/transcript-scroll"; import { classifyScrollPosition, type TranscriptHandle, @@ -697,7 +697,7 @@ const API_NS = "api"; * flip — call from the console to flip a localStorage-persisted flag. * Toggles that change React hook ordering (e.g. swapping which scroll * coordinator runs) reload the page so the new value takes effect - * cleanly. See `transcript-scroll-flag.ts` for the storage layer. + * cleanly. See `transcript-scroll.ts` for the storage layer. */ export interface VellumDebugFlagsApi { /** Flip the parallel `useTranscriptScrollController` path on or off. From c0e13f42f9262b53827b36185ba5fb8a25975520 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 16:48:32 +0000 Subject: [PATCH 2/3] review iter: split flag file, drop key helper, observe content for seed-then-grow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../chat/transcript/transcript-scroll-flag.ts | 79 ++++++ .../chat/transcript/transcript-scroll.test.ts | 232 +++++++++++++++-- .../chat/transcript/transcript-scroll.ts | 243 +++++++++--------- .../domains/chat/transcript/transcript.tsx | 17 +- .../use-deprecated-transcript-scroll.ts | 2 +- apps/web/src/domains/chat/utils/debug-api.ts | 4 +- 6 files changed, 422 insertions(+), 155 deletions(-) create mode 100644 apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts b/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts new file mode 100644 index 00000000000..9384ce68848 --- /dev/null +++ b/apps/web/src/domains/chat/transcript/transcript-scroll-flag.ts @@ -0,0 +1,79 @@ +// Runtime flag that turns OFF the deprecated transcript scroll hook. +// +// This file is intentionally isolated from the rest of the scroll +// utilities so it can be deleted in one move when the migration is +// complete — no other file in `transcript/` depends on it except +// `use-deprecated-transcript-scroll.ts` and `transcript-scroll.ts`, +// both of which will be deleted/refactored at that point. +// +// In the current shape of this work-in-progress: +// +// • Flag OFF (default) — the orchestrator runs the deprecated hook +// `useDeprecatedTranscriptScroll`, which is the existing +// production scroll-coordination logic. Production users land +// here. We keep this in the tree so we don't regress shipping +// behavior while we redesign the replacement. +// • Flag ON — the deprecated hook returns its no-op result. The +// replacement utilities in `transcript-scroll.ts` take over +// piecewise as features migrate. +// +// Why module-load read + reload-on-toggle: +// +// • React forbids conditionally calling different hooks across +// renders. By making the dispatch decision once at module-import +// time (before any component mounts), the dispatcher resolves to a +// single function identity for the entire page lifetime. +// • Toggling without a reload would leave the DOM in an inconsistent +// intermediate state (scroll listeners attached but no longer +// handled, in-flight auto-pin timers orphaned). A page reload is +// cheap and dev-only. +// +// Surface (exposed under `window._vellumDebug.flags`): +// +// toggleTranscriptScrollController() — flip current value +// toggleTranscriptScrollController(true) — force on +// toggleTranscriptScrollController(false) — force off + +const STORAGE_KEY = "vellumDebug.flags.transcriptScrollController"; + +/** Read the flag synchronously. Safe to call at module-load time. */ +export function getTranscriptScrollControllerEnabled(): boolean { + if (typeof window === "undefined") return false; + try { + return window.localStorage.getItem(STORAGE_KEY) === "true"; + } catch { + // Private-browsing modes or sandboxed contexts can throw on + // localStorage access. Treat any throw as "flag off". + return false; + } +} + +/** Persist the flag, log the new value, and reload the page so the + * dispatcher re-resolves. `value === undefined` flips the current + * value (the most common interactive case). */ +export function setTranscriptScrollControllerEnabled(value?: boolean): boolean { + if (typeof window === "undefined") return false; + const next = + value === undefined ? !getTranscriptScrollControllerEnabled() : !!value; + try { + window.localStorage.setItem(STORAGE_KEY, String(next)); + } catch { + // Persistence failed — log and bail so the user knows their + // toggle didn't stick. + console.warn( + "[vellumDebug] failed to persist transcriptScrollController flag", + ); + return getTranscriptScrollControllerEnabled(); + } + console.info( + `[vellumDebug] transcriptScrollController = ${next} — reloading…`, + ); + window.location.reload(); + return next; +} + +/** The flag value resolved exactly once at module load. The + * dispatcher reads this constant so hook-rule order stays stable + * across the page lifetime. */ +export const TRANSCRIPT_SCROLL_CONTROLLER_ENABLED = + getTranscriptScrollControllerEnabled(); diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts b/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts index 56987e93ea9..c9a844a1af1 100644 --- a/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts +++ b/apps/web/src/domains/chat/transcript/transcript-scroll.test.ts @@ -1,33 +1,215 @@ /** - * Tests for the transcript scroll utilities. + * Tests for the imperative transcript scroll utilities. * - * `bun:test` runs without a real DOM environment, so we exercise the - * pure surface of these utilities (the key function + the ref's - * imperative DOM mutation) using plain object fakes. The flag itself - * is read once at module load, so we cannot toggle it inside a test - * — instead the tests assert behavior at the import-time-resolved - * value and document the contract on both sides via inspection of the - * helpers' contracts. + * `bun:test` runs without a real DOM, so we exercise `attachSnapToLatest` + * — the pure imperative function the hook delegates to — against + * a minimal fake element shape. The hook itself is thin glue around + * this function; testing the function covers the load-bearing + * behavior (initial snap, re-snap on content resize, gesture disengage). */ import { describe, expect, test } from "bun:test"; -import { - TRANSCRIPT_SCROLL_CONTROLLER_ENABLED, - getTranscriptScrollContainerKey, -} from "@/domains/chat/transcript/transcript-scroll"; - -describe("getTranscriptScrollContainerKey", () => { - test("returns undefined when the controller flag is OFF", () => { - // In the test environment there is no `window.localStorage` entry - // set, so the module-load read of the flag resolves to `false`. - // The key helper must short-circuit to `undefined` so React - // reconciles by position — preserving the deprecated-hook era - // behavior where the scroll container is not remounted on - // conversation switch. - expect(TRANSCRIPT_SCROLL_CONTROLLER_ENABLED).toBe(false); - expect(getTranscriptScrollContainerKey("conv-1")).toBeUndefined(); - expect(getTranscriptScrollContainerKey(null)).toBeUndefined(); - expect(getTranscriptScrollContainerKey(undefined)).toBeUndefined(); +import { attachSnapToLatest } from "@/domains/chat/transcript/transcript-scroll"; + +type Listener = (...args: unknown[]) => void; + +type FakeElement = { + scrollTop: number; + scrollHeight: number; + addEventListener(event: string, listener: Listener): void; + removeEventListener(event: string, listener: Listener): void; + fire(event: string): void; + listenerCount(event: string): number; +}; + +function createFakeElement(scrollHeight: number): FakeElement { + const listeners = new Map>(); + return { + scrollTop: 0, + scrollHeight, + addEventListener(event, listener) { + let set = listeners.get(event); + if (!set) { + set = new Set(); + listeners.set(event, set); + } + set.add(listener); + }, + removeEventListener(event, listener) { + listeners.get(event)?.delete(listener); + }, + fire(event) { + listeners.get(event)?.forEach((l) => l()); + }, + listenerCount(event) { + return listeners.get(event)?.size ?? 0; + }, + }; +} + +class FakeResizeObserver { + static instances: FakeResizeObserver[] = []; + callback: ResizeObserverCallback; + observed: Element[] = []; + disconnected = false; + constructor(callback: ResizeObserverCallback) { + this.callback = callback; + FakeResizeObserver.instances.push(this); + } + observe(el: Element): void { + this.observed.push(el); + } + unobserve(): void {} + disconnect(): void { + this.disconnected = true; + } + fire(): void { + this.callback([], this as unknown as ResizeObserver); + } +} + +function installFakeResizeObserver(): void { + FakeResizeObserver.instances = []; + (globalThis as { ResizeObserver?: unknown }).ResizeObserver = + FakeResizeObserver; +} + +function uninstallFakeResizeObserver(): void { + delete (globalThis as { ResizeObserver?: unknown }).ResizeObserver; +} + +describe("attachSnapToLatest", () => { + test("snaps to bottom on initial attach", () => { + installFakeResizeObserver(); + const container = createFakeElement(1000); + const content = createFakeElement(1000); + container.scrollTop = 0; + + const stop = attachSnapToLatest({ + container: container as unknown as HTMLElement, + content: content as unknown as HTMLElement, + }); + + expect(container.scrollTop).toBe(1000); + + stop(); + uninstallFakeResizeObserver(); + }); + + test("re-snaps on content ResizeObserver fire (covers seed-then-grow race)", () => { + installFakeResizeObserver(); + const container = createFakeElement(500); + const content = createFakeElement(500); + + attachSnapToLatest({ + container: container as unknown as HTMLElement, + content: content as unknown as HTMLElement, + }); + + expect(container.scrollTop).toBe(500); + + // Simulate `useViewportMinHeight` seeding LatestTurnRow's minHeight: + // content grows, scrollHeight grows. + container.scrollHeight = 1500; + FakeResizeObserver.instances[0].fire(); + + expect(container.scrollTop).toBe(1500); + + uninstallFakeResizeObserver(); + }); + + test("stops re-snapping after user wheel gesture", () => { + installFakeResizeObserver(); + const container = createFakeElement(500); + const content = createFakeElement(500); + + attachSnapToLatest({ + container: container as unknown as HTMLElement, + content: content as unknown as HTMLElement, + }); + + // User scrolls up. + container.scrollTop = 100; + container.fire("wheel"); + + // ResizeObserver fires after disengage — should NOT re-snap. + container.scrollHeight = 1500; + FakeResizeObserver.instances[0].fire(); + + expect(container.scrollTop).toBe(100); + expect(FakeResizeObserver.instances[0].disconnected).toBe(true); + + uninstallFakeResizeObserver(); + }); + + test("stops re-snapping after touchmove and keydown gestures too", () => { + installFakeResizeObserver(); + const container1 = createFakeElement(500); + const content1 = createFakeElement(500); + attachSnapToLatest({ + container: container1 as unknown as HTMLElement, + content: content1 as unknown as HTMLElement, + }); + container1.scrollTop = 100; + container1.fire("touchmove"); + container1.scrollHeight = 1500; + FakeResizeObserver.instances[0].fire(); + expect(container1.scrollTop).toBe(100); + + const container2 = createFakeElement(500); + const content2 = createFakeElement(500); + attachSnapToLatest({ + container: container2 as unknown as HTMLElement, + content: content2 as unknown as HTMLElement, + }); + container2.scrollTop = 100; + container2.fire("keydown"); + container2.scrollHeight = 1500; + FakeResizeObserver.instances[1].fire(); + expect(container2.scrollTop).toBe(100); + + uninstallFakeResizeObserver(); + }); + + test("teardown removes listeners and disconnects observer", () => { + installFakeResizeObserver(); + const container = createFakeElement(500); + const content = createFakeElement(500); + + const stop = attachSnapToLatest({ + container: container as unknown as HTMLElement, + content: content as unknown as HTMLElement, + }); + + expect(container.listenerCount("wheel")).toBe(1); + expect(container.listenerCount("touchmove")).toBe(1); + expect(container.listenerCount("keydown")).toBe(1); + + stop(); + + expect(container.listenerCount("wheel")).toBe(0); + expect(container.listenerCount("touchmove")).toBe(0); + expect(container.listenerCount("keydown")).toBe(0); + expect(FakeResizeObserver.instances[0].disconnected).toBe(true); + + uninstallFakeResizeObserver(); + }); + + test("no ResizeObserver available — snaps once and returns inert teardown", () => { + // SSR / older test environments. + const container = createFakeElement(800); + const content = createFakeElement(800); + + const stop = attachSnapToLatest({ + container: container as unknown as HTMLElement, + content: content as unknown as HTMLElement, + }); + + expect(container.scrollTop).toBe(800); + expect(container.listenerCount("wheel")).toBe(0); + + // Should not throw. + stop(); }); }); diff --git a/apps/web/src/domains/chat/transcript/transcript-scroll.ts b/apps/web/src/domains/chat/transcript/transcript-scroll.ts index b7aa329b588..6ad6eb24fee 100644 --- a/apps/web/src/domains/chat/transcript/transcript-scroll.ts +++ b/apps/web/src/domains/chat/transcript/transcript-scroll.ts @@ -1,131 +1,136 @@ -// Transcript scroll utilities — single home for the imperative -// replacement of `useDeprecatedTranscriptScroll`. Everything in this -// file is gated internally by `TRANSCRIPT_SCROLL_CONTROLLER_ENABLED` -// so callers in component files never branch on the flag themselves. +// Transcript scroll utilities — the imperative replacement for +// `useDeprecatedTranscriptScroll`. Listens to DOM lifecycle events +// (element attached, content resized, user gesture) rather than +// reacting to React state changes. Full spec lives at +// `/workspace/scratch/scroll-imperative-spec.md`. // -// In the current shape of this work-in-progress: -// -// • Flag OFF (default) — the orchestrator runs the deprecated hook -// `useDeprecatedTranscriptScroll`, which is the existing -// production scroll-coordination logic. The utilities exported -// from this file no-op in OFF, so callers can wire them at the -// final shape today without affecting shipping behavior. -// • Flag ON — the deprecated hook returns its no-op result and the -// utilities here take over piecewise as features migrate. The -// ON path is the baseline against which the eventual -// `TranscriptScrollController` will be built. -// -// Why module-load read + reload-on-toggle: -// -// • React forbids conditionally calling different hooks across -// renders. By making the dispatch decision once at module-import -// time (before any component mounts), the dispatcher resolves to a -// single function identity for the entire page lifetime. -// • Toggling without a reload would leave the DOM in an inconsistent -// intermediate state (scroll listeners attached but no longer -// handled, in-flight auto-pin timers orphaned). A page reload is -// cheap and dev-only. -// -// Surface (exposed under `window._vellumDebug.flags`): -// -// toggleTranscriptScrollController() — flip current value -// toggleTranscriptScrollController(true) — force on -// toggleTranscriptScrollController(false) — force off - -import { useCallback, type MutableRefObject } from "react"; - -const STORAGE_KEY = "vellumDebug.flags.transcriptScrollController"; - -/** Read the flag synchronously. Safe to call at module-load time. */ -export function getTranscriptScrollControllerEnabled(): boolean { - if (typeof window === "undefined") return false; - try { - return window.localStorage.getItem(STORAGE_KEY) === "true"; - } catch { - // Private-browsing modes or sandboxed contexts can throw on - // localStorage access. Treat any throw as "flag off". - return false; - } -} +// Gating against `TRANSCRIPT_SCROLL_CONTROLLER_ENABLED` lives inside +// each utility's body so component files import them without +// branching on the flag themselves. -/** Persist the flag, log the new value, and reload the page so the - * dispatcher re-resolves. `value === undefined` flips the current - * value (the most common interactive case). */ -export function setTranscriptScrollControllerEnabled(value?: boolean): boolean { - if (typeof window === "undefined") return false; - const next = - value === undefined ? !getTranscriptScrollControllerEnabled() : !!value; - try { - window.localStorage.setItem(STORAGE_KEY, String(next)); - } catch { - // Persistence failed — log and bail so the user knows their - // toggle didn't stick. - console.warn( - "[vellumDebug] failed to persist transcriptScrollController flag", - ); - return getTranscriptScrollControllerEnabled(); - } - console.info( - `[vellumDebug] transcriptScrollController = ${next} — reloading…`, - ); - window.location.reload(); - return next; -} +import { useCallback, useRef, type MutableRefObject } from "react"; -/** The flag value resolved exactly once at module load. The - * dispatcher reads this constant so hook-rule order stays stable - * across the page lifetime. */ -export const TRANSCRIPT_SCROLL_CONTROLLER_ENABLED = - getTranscriptScrollControllerEnabled(); +import { TRANSCRIPT_SCROLL_CONTROLLER_ENABLED } from "@/domains/chat/transcript/transcript-scroll-flag"; -// --------------------------------------------------------------------------- -// Imperative scroll utilities -// --------------------------------------------------------------------------- -// -// The utilities below are the imperative replacements that piecewise -// take over from `useDeprecatedTranscriptScroll`. They listen to DOM -// lifecycle events (element attached, scroll, resize) rather than -// reacting to React state changes, which is the whole point of the -// migration — see `/workspace/scratch/scroll-imperative-spec.md`. - -/** React key for the transcript scroll container element. Combine - * with `useTranscriptScrollContainerRef` on the same element so a - * conversation switch — or a fresh page load on a conversation - * detail route — triggers a fresh DOM attach. That attach is the - * event our callback ref listens for to scroll to the latest message. +/** + * Wire the transcript scroll container + content callback refs. * - * Returns `undefined` when the controller flag is OFF, which - * preserves the deprecated-hook era reconciliation (no remount on - * conversation switch; the deprecated hook handles switching via - * its own conversation-id effect). */ -export function getTranscriptScrollContainerKey( - conversationId: string | null | undefined, -): string | undefined { - if (!TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) return undefined; - return conversationId ?? "empty"; -} - -/** Callback ref for the transcript scroll container `
`. Forwards - * the attached element to `forwardTo` so existing imperative callers - * (pull-to-refresh, debug API, the transcript's own - * `useImperativeHandle`) keep working. + * Behavior when the controller flag is ON: + * 1. On scroll container DOM attach (which fires on conversation + * switch via `key={conversationId}` in `transcript.tsx`, and on + * fresh detail-page loads), the container is snapped to bottom. + * 2. A `ResizeObserver` watches the content wrapper. Until the user + * interacts, every content height change re-snaps to bottom. + * This covers the seed-then-grow race where `useViewportMinHeight` + * seeds `LatestTurnRow`'s `minHeight` in a post-paint effect, + * growing `scrollHeight` after the initial attach snap. + * 3. The first `wheel`/`touchmove`/`keydown` on the container + * disengages the observer — the user is now in control. + * + * When the controller flag is OFF, both callbacks forward to the + * passed-in refs without observing or scrolling — the deprecated hook + * still owns scroll coordination in that path. * - * When the controller flag is ON, additionally scrolls to bottom on - * attach. Combined with `getTranscriptScrollContainerKey()` on the - * same element, this is the imperative implementation of - * "open every conversation view at the latest message" — feature #4 - * in the migration tracker. The trigger is the DOM attach event, not - * a React state change. */ -export function useTranscriptScrollContainerRef( - forwardTo: MutableRefObject, -): (el: HTMLDivElement | null) => void { - return useCallback( + * Both callbacks are returned as memoized identities so React doesn't + * tear them down between renders. The internal state (observer + + * gesture listeners) is keyed off element identity, not callback + * identity, so the wiring survives parent re-renders and tears down + * exactly when the underlying element changes. + */ +export function useTranscriptScrollOnAttach(args: { + scrollContainerRef: MutableRefObject; + contentRef: MutableRefObject; +}): { + scrollContainerCallbackRef: (el: HTMLDivElement | null) => void; + contentCallbackRef: (el: HTMLDivElement | null) => void; +} { + // Refs for tearing down the previous attach when the element + // changes (e.g. conversation switch with `key={conversationId}`). + const teardownRef = useRef<(() => void) | null>(null); + + const teardown = useCallback(() => { + teardownRef.current?.(); + teardownRef.current = null; + }, []); + + const scrollContainerCallbackRef = useCallback( (el: HTMLDivElement | null) => { - forwardTo.current = el; + args.scrollContainerRef.current = el; + // No work here — all setup waits for the content ref so we + // can attach the ResizeObserver to the inner wrapper. The + // content ref is guaranteed to fire on the same commit since + // it's a child of this element. + }, + [args.scrollContainerRef], + ); + + const contentCallbackRef = useCallback( + (el: HTMLDivElement | null) => { + args.contentRef.current = el; if (!TRANSCRIPT_SCROLL_CONTROLLER_ENABLED) return; + + // Tear down whatever was wired to the previous content/container. + teardown(); + if (!el) return; - el.scrollTop = el.scrollHeight; + const container = args.scrollContainerRef.current; + if (!container) return; + + teardownRef.current = attachSnapToLatest({ container, content: el }); }, - [forwardTo], + [args.scrollContainerRef, args.contentRef, teardown], ); + + return { scrollContainerCallbackRef, contentCallbackRef }; +} + +/** + * Snap the scroll container to bottom and keep it there through async + * layout settling. Returns a teardown function that disconnects the + * `ResizeObserver` and removes the gesture listeners. + * + * Pure imperative function — takes plain DOM elements, no React. The + * React wiring lives in `useTranscriptScrollOnAttach`; this function + * is independently testable with a fake `HTMLElement`. + */ +export function attachSnapToLatest(args: { + container: HTMLElement; + content: HTMLElement; +}): () => void { + const { container, content } = args; + + // Initial attach snap — synchronous, during commit, before paint. + container.scrollTop = container.scrollHeight; + + // Browsers without `ResizeObserver` get the initial snap only. + // Modern browsers (and the iOS WKWebView) all have it; this guard + // is for SSR / test environments. + if (typeof ResizeObserver === "undefined") { + return () => {}; + } + + let active = true; + + const stop = (): void => { + if (!active) return; + active = false; + observer.disconnect(); + container.removeEventListener("wheel", stop); + container.removeEventListener("touchmove", stop); + container.removeEventListener("keydown", stop); + }; + + const observer = new ResizeObserver(() => { + if (!active) return; + container.scrollTop = container.scrollHeight; + }); + observer.observe(content); + + // User-gesture disengage. The user touching the scroll container in + // any way means they're driving — stop fighting them. + container.addEventListener("wheel", stop, { passive: true }); + container.addEventListener("touchmove", stop, { passive: true }); + container.addEventListener("keydown", stop); + + return stop; } diff --git a/apps/web/src/domains/chat/transcript/transcript.tsx b/apps/web/src/domains/chat/transcript/transcript.tsx index 5e012d72556..e1a106100b3 100644 --- a/apps/web/src/domains/chat/transcript/transcript.tsx +++ b/apps/web/src/domains/chat/transcript/transcript.tsx @@ -16,10 +16,7 @@ import type { TranscriptItem } from "@/domains/chat/transcript/types"; import { LatestTurnRow } from "@/domains/chat/transcript/latest-turn-row"; import { PullRefreshSpinner } from "@/domains/chat/transcript/pull-refresh-spinner"; import { TranscriptRow } from "@/domains/chat/transcript/transcript-row"; -import { - getTranscriptScrollContainerKey, - useTranscriptScrollContainerRef, -} from "@/domains/chat/transcript/transcript-scroll"; +import { useTranscriptScrollOnAttach } from "@/domains/chat/transcript/transcript-scroll"; import { PULL_THRESHOLD_PX, usePullToRefresh, @@ -162,7 +159,11 @@ export const Transcript = forwardRef( props; const scrollRef = useRef(null); const contentRef = useRef(null); - const scrollContainerRef = useTranscriptScrollContainerRef(scrollRef); + const { scrollContainerCallbackRef, contentCallbackRef } = + useTranscriptScrollOnAttach({ + scrollContainerRef: scrollRef, + contentRef, + }); const viewportMinHeight = useViewportMinHeight(scrollRef); const pullEnabled = !!pullRefreshEnabled && !!onPullRefresh; @@ -259,8 +260,8 @@ export const Transcript = forwardRef( return (
@@ -269,7 +270,7 @@ export const Transcript = forwardRef( * height changes (async min-height settle, late image loads, * streaming growth). Wrapping all rows in a single observed * element is cheaper than observing each row individually. */} -
+
{/* History items in chronological order — oldest at top. */} {partition.historyItems.map((item) => ( diff --git a/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts b/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts index 50eb60b0a3b..c0df817879e 100644 --- a/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts +++ b/apps/web/src/domains/chat/transcript/use-deprecated-transcript-scroll.ts @@ -34,7 +34,7 @@ import { import type { TranscriptItem } from "@/domains/chat/transcript/types"; import type { TranscriptHandle } from "@/domains/chat/transcript/transcript"; -import { TRANSCRIPT_SCROLL_CONTROLLER_ENABLED } from "@/domains/chat/transcript/transcript-scroll"; +import { TRANSCRIPT_SCROLL_CONTROLLER_ENABLED } from "@/domains/chat/transcript/transcript-scroll-flag"; export type { TranscriptHandle }; diff --git a/apps/web/src/domains/chat/utils/debug-api.ts b/apps/web/src/domains/chat/utils/debug-api.ts index aafa1f91c06..ff3fc52eb35 100644 --- a/apps/web/src/domains/chat/utils/debug-api.ts +++ b/apps/web/src/domains/chat/utils/debug-api.ts @@ -43,7 +43,7 @@ import type { import { recordChatDiagnostic } from "@/domains/chat/utils/diagnostics"; import type { DisplayMessage } from "@/domains/chat/utils/reconcile"; import type { ReconcileActiveConversationResult } from "@/domains/chat/hooks/use-message-reconciliation"; -import { setTranscriptScrollControllerEnabled } from "@/domains/chat/transcript/transcript-scroll"; +import { setTranscriptScrollControllerEnabled } from "@/domains/chat/transcript/transcript-scroll-flag"; import { classifyScrollPosition, type TranscriptHandle, @@ -697,7 +697,7 @@ const API_NS = "api"; * flip — call from the console to flip a localStorage-persisted flag. * Toggles that change React hook ordering (e.g. swapping which scroll * coordinator runs) reload the page so the new value takes effect - * cleanly. See `transcript-scroll.ts` for the storage layer. + * cleanly. See `transcript-scroll-flag.ts` for the storage layer. */ export interface VellumDebugFlagsApi { /** Flip the parallel `useTranscriptScrollController` path on or off. From 6467ec2d3b03c999c2782920886c5d00f7367620 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 16:56:08 +0000 Subject: [PATCH 3/3] review iter: TranscriptProps.conversationId required (no ? no docstring) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../chat/transcript/transcript-subagent-inline.test.tsx | 9 +++++++++ apps/web/src/domains/chat/transcript/transcript.test.tsx | 5 +++++ apps/web/src/domains/chat/transcript/transcript.tsx | 5 +---- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/apps/web/src/domains/chat/transcript/transcript-subagent-inline.test.tsx b/apps/web/src/domains/chat/transcript/transcript-subagent-inline.test.tsx index 154e7eb9fdf..2f4c3241fc7 100644 --- a/apps/web/src/domains/chat/transcript/transcript-subagent-inline.test.tsx +++ b/apps/web/src/domains/chat/transcript/transcript-subagent-inline.test.tsx @@ -176,6 +176,7 @@ describe("Transcript — inline subagent rendering (PR 8)", () => { const { getAllByTestId } = render( { const { queryAllByTestId } = render( { const { container, getByTestId } = render( { const { getAllByTestId } = render( { const { getAllByTestId } = render( { const { getAllByTestId } = render( { const { queryAllByTestId } = render( { const { getAllByTestId } = render( { const html = renderToStaticMarkup( { const html = renderToStaticMarkup( { const html = renderToStaticMarkup( { const html = renderToStaticMarkup( { const html = renderToStaticMarkup( void; onConfirmationDecision: (requestId: string, decision: string) => void;