fix(site/AgentsPage): eliminate streaming animation on chat navigation#24139
fix(site/AgentsPage): eliminate streaming animation on chat navigation#24139DanielleMaywood wants to merge 1 commit intomainfrom
Conversation
772c14c to
76a5348
Compare
76a5348 to
49ad85e
Compare
The chat store starts empty on every navigation (key={agentId} forces
remount) and is populated via useEffect, which fires after children
have already rendered with an empty store via useSyncExternalStore.
This produces a visible flash where the timeline paints empty then
fills — and for running chats the sudden appearance of all messages
triggers the streaming-text animation on every paragraph.
Seed the store with the available REST-fetched messages (and chat
status) inside the useState initializer so child components see data
on their very first render. The existing useEffect still handles
subsequent updates (refetches, WebSocket-delivered messages) and
detects the pre-seeded data as a no-op via chatMessagesEqualByValue.
49ad85e to
a080abc
Compare
mafredri
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix. The useLayoutEffect swap is the canonical React pattern for pre-paint external store hydration, and the effect ordering analysis holds up: keyed remount means fresh refs, no stale state leaks, no SSR concern. All 50 existing tests pass. One nit inline, two non-diff findings below.
Severity count: 0 P0-P2, 2 P3, 1 P4.
P3 useChatStore.ts:210 - Em-dash character (U+2014) in new comment. (Convention scan) The new comment reads appear at once —. Project convention prohibits em-dash characters (U+2014). Replace with a double hyphen or restructure the sentence.
P4 useChatStore.ts:6 - useEffectEvent polyfill is dead code now that React 19.2.2 ships it natively. (Ging-ts) The polyfill in hookPolyfills.ts says "should be deleted as soon as the official versions are available." React 19 ships useEffectEvent as a real export. 20 call sites import from the polyfill. Switching to import { useEffectEvent } from "react" and deleting the file is mechanical. Not this PR's job.
Four reviewers independently noted that the queued messages hydration (L284) still uses useEffect, leaving a theoretical one-frame flash of stale queued messages on chat switch. All four agree the visual impact is low (secondary UI element, usually empty), and it's not actionable for this PR.
Two reviewers noted the fix is untestable in jsdom: renderHook + act() synchronizes both useLayoutEffect and useEffect within the same tick, so a revert to useEffect would pass all 50 tests. This is a fundamental limitation of the test environment, not a gap in this PR.
Meruem noted wsStatusReceivedRef can be stale on first render after chat switch (layout effect reads it before the ref-reset useEffect clears it), but this ordering existed pre-PR and keyed remount reinitializes the ref anyway.
"The code held up. Boring in the best way. ♠" -- Hisoka
🤖 This review was automatically generated with Coder Agents.
| }, [chatID, chatMessages, store]); | ||
|
|
||
| useEffect(() => { | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
P3 Second useLayoutEffect has no comment explaining the layout timing choice. (Bisky P3, Gon P3, Nami Note)
Three reviewers independently flagged this. The first promotion (L207) has a 6-line comment explaining why useLayoutEffect is needed. This one silently changed with no equivalent explanation. The existing comment (L265-268) explains the WS-vs-REST priority guard, not the hook choice.
"A maintainer reading line 264 cannot tell whether
useLayoutEffectis deliberate without scrolling up 50 lines and inferring that the same rationale applies." -- Gon
Suggested one-liner above L264:
// useLayoutEffect for the same pre-paint reason as the
// message-sync effect above.🤖
Two fixes for the visual flash when navigating to a running chat:
1. Seed the chat store at creation time
The store starts empty on every navigation (
key={agentId}forcesremount) and was populated via
useEffect— which fires after childrenhave already rendered with an empty store. Now the
useStateinitializerpre-populates the store with cached REST data so
useSyncExternalStorereaders see messages on their very first render.
2. Skip smooth-text animation on first update
When navigating to a running chat, the WebSocket replays the in-progress
response. Each paragraph’s
SmoothTextEnginestarted atvisibleLength=0and animated the full text in — causing layout shifts as lines wrapped.
Now the engine snaps to the full length on its first streaming update;
only genuinely new text arriving afterward gets the smooth reveal.