refactor(web/chat): drop transcript-scroll experiment, rename deprecated hook#32374
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd9ca40f96
ℹ️ 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".
| /** 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 { |
There was a problem hiding this comment.
In this commit's target tree, live code still imports the deleted/renamed modules: transcript.tsx imports transcript-scroll, use-transcript-scroll.ts and debug-api.ts import transcript-scroll-flag, and several files still import use-deprecated-transcript-scroll. Because these files are removed/renamed here without updating those callers, apps/web cannot resolve the modules during Vite/TypeScript builds, so the chat bundle is broken until the import sites and exported hook names are migrated in the same change.
Useful? React with 👍 / 👎.
…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.
vellum-apollo-bot
Bot
force-pushed
the
apollo/transcript-scroll-rename
branch
from
bd9ca40 to
18c956f
Compare
1 participant
What
Rip out the parallel imperative scroll experiment and rename the canonical hook.
Why
The
transcript-scroll.ts/transcript-scroll-flag.tsmodule was meant as the migration target foruseDeprecatedTranscriptScroll. In practice — after #32239 (snap-to-bottom on switch) landed and #32306 explored load-older — the simplified shape ended up functionally equivalent to a chunk of the existing hook, just split across files. Migration cost without architectural win.Better path: keep the hook that works, drop the parallel module, rename it to its canonical name, and progressively simplify in place after a QA pass on the remaining bugs.
Deletions
apps/web/src/domains/chat/transcript/transcript-scroll.tsapps/web/src/domains/chat/transcript/transcript-scroll-flag.tsapps/web/src/domains/chat/transcript/transcript-scroll.test.tsIn-place edits
use-deprecated-transcript-scroll.ts→ drop the kill-switch +DISABLED_RESULTshort-circuit and the flag import.transcript.tsx→ dropuseTranscriptScrollOnAttachimport + callback-ref plumbing; restore plainref={scrollRef}/ref={contentRef}.debug-api.ts→ droptoggleTranscriptScrollControllerfromVellumDebugFlagsApiand its setter import.impersonate-version-flag.ts→ update stale doc reference.Renames
use-deprecated-transcript-scroll.ts→use-transcript-scroll.tsuseDeprecatedTranscriptScroll→useTranscriptScrollUseDeprecatedTranscriptScrollArgs/UseDeprecatedTranscriptScrollReturn→UseTranscriptScrollArgs/UseTranscriptScrollReturnStat
10 files changed, 16 insertions(+), 480 deletions(-)— net -464 lines.Tests
tsc --noEmitclean.eslint --max-warnings=0clean.bun test src/domains/chat/transcript/— 120 pass / 4 fail. Same 4 pre-existingTranscriptMessageBodyfailures present onorigin/main. No regressions.bun test src/domains/chat/— 1365 pass / 72 fail (matches main's 72; 6 fewer passes correspond to the deletedtranscript-scroll.test.ts's 6 tests).Behavioral impact
Zero. The flag was OFF by default; the parallel path never ran for any user. The hook that was running before is the hook that's running after, just under the canonical name.
Forward plan
@dvargasfuertes QA pass on the remaining bugs in the (now-canonical)
useTranscriptScrollhook, then progressive simplification in place.