Fix streaming transition and inline composer layout

[IanLindsley]

Summary

• Fix jarring scroll jump when streaming ends: updates streamingText to the full response on complete, animates at constant rate, and uses matching styled markdown components for seamless renderer swap
• Change composer from stacked to inline layout with vertically centered placeholder text
• Re-enable auto-scroll now that the transition is smooth
• Bump version to 1.3.1

Test plan

• Send a chat message and verify streaming text animates smoothly at a constant rate
• Verify no visual jump or scroll jolt when streaming completes
• Verify auto-scroll follows new content during streaming
• Verify composer placeholder "How can I help you today?" is vertically centered in both welcome and chat states
• Verify version shows v1.3.1 in login page and chat footer

🤖 Generated with Claude Code

[IanLindsley]

test

[IanLindsley]

test comment

[IanLindsley]

Code Review Summary

This PR addresses a real UX problem -- the visual jump when streaming completes -- by animating the remaining text at a constant rate and rendering streaming text with matching markdown components. The composer layout change to inline is clean. The approach is sound overall, but the duplicated markdown components and the requestAnimationFrame timing strategy introduce maintenance and correctness risks that should be addressed.

Issues: CRITICAL: 0 | HIGH: 0 | MEDIUM: 3 | LOW: 2

Blocking Issues

1. [MEDIUM] Duplicated markdown components will drift out of sync (thread.tsx:40). The streaming markdown components are already missing code and CodeHeader overrides from markdown-text.tsx, so inline code and code blocks will visually jump on swap. This partially undermines the stated goal.

2. [MEDIUM] requestAnimationFrame defers state outside React batching (use-chat-runtime.ts:148). Between the synchronous state updates and the rAF callback, there is a render with an inconsistent intermediate state. Browser tab backgrounding can extend this window.

3. [MEDIUM] Removed prefix-divergence guard risks garbled text (use-chat-runtime.ts:167). If the backend's final response differs from streamed progress (post-processing, encoding differences), the animation will show wrong characters at wrong positions because displayedLength is not reset on text replacement.

Other Issues

4. [LOW] Dead constant CHARS_PER_TICK_COMPLETING (thread.tsx:34). Identical to CHARS_PER_TICK, making the ternary a no-op.

5. [LOW] Version hardcoded in 4 locations (thread.tsx:191, thread.tsx:274, login/page.tsx:138, package.json). Pre-existing, but grows more fragile each bump.

Positive Highlights

• The overall approach of rendering streaming text through a matching Markdown renderer to eliminate the visual swap is the right solution.
• The composer layout refactor from stacked to inline is clean and well-structured.
• The useAnimatedText hook is a nice abstraction with clean separation of concerns.
• Re-enabling autoScroll is the right call now that the transition is smooth.
• Good use of useCallback and refs to avoid stale closures in the animation lifecycle.

Recommendation: REQUEST CHANGES -- the three MEDIUM issues should be addressed before merge. Issue #1 (missing code component) is quick to fix and directly affects the "seamless swap" goal. Issue #3 (removed divergence guard) is a correctness risk that needs either the guard restored or explicit documentation that the backend guarantees prefix consistency.

[IanLindsley]

[MEDIUM] Duplicated markdown component definitions will drift out of sync

The comment on line 38 says "Keep in sync with defaultComponents in markdown-text.tsx", but this is a manual invariant with no enforcement. The streaming components are already missing the code and CodeHeader overrides that exist in markdown-text.tsx (lines 131-143). This means:

1. Inline code during streaming renders with browser defaults (no bg-muted rounded border px-1 font-semibold styling).
2. Code blocks during streaming have no copy-to-clipboard header.
3. When the animation finishes and swaps to MarkdownText, the code block styling will visually jump.

This partially undermines the stated goal of "seamless renderer swap".

Recommendation: Extract the shared component definitions into a single source of truth. For example, export a sharedMarkdownComponents object from markdown-text.tsx that both the memoized version and the streaming version import. The streaming version can omit CodeHeader if the copy button is not desired during streaming, but code styling should match. At minimum, add the code component override now to avoid a visual discrepancy.

[IanLindsley]

[MEDIUM] requestAnimationFrame defers state updates outside React's batching, risking a flash frame

finalizeComplete sets isCompleting(false) and isLoading(false) synchronously, then defers setMessages and setStreamingText("") to a requestAnimationFrame callback. Between the synchronous updates and the rAF callback, there is one render cycle where:

• isCompleting is false
• streamingText still has content (so the streaming message is still in allMessages)
• isLoading is false

During that intermediate render, the AssistantMessage component sees isStreaming=true (from the streaming message metadata) but isCompleting=false, which puts AnimatedText in a state where it will not fire the completion callback. This is a benign but unnecessary intermediate state.

More importantly, requestAnimationFrame callbacks can be delayed by browser tab backgrounding or heavy paint work, which could extend this intermediate state.

Recommendation: Use ReactDOM.flushSync or batch all state updates together synchronously (React 18+ auto-batches inside event handlers and effects). If the goal is to avoid a flash from the renderer swap, consider using startTransition or a useLayoutEffect to coordinate the swap, rather than relying on frame timing.

[IanLindsley]

[MEDIUM] Removing the prefix-divergence guard may cause garbled animation on corrected completions

Previously, the code checked !joinedResponse.startsWith(currentStreaming) and would swap immediately if the complete response diverged from what was streamed. This handled the edge case where the backend corrects or reformats the final response relative to the streamed progress chunks.

Now with that guard removed, if the backend's final joinedResponse differs from what was streamed (e.g., the backend does post-processing, or progress events had encoding differences), the code will set streamingText to a completely different string. The useAnimatedText hook no longer resets displayedLength when the text changes but does not shrink (line 390 only resets on text.length === 0), so the animation would show displayedLength characters of the new text -- wrong characters at wrong positions.

Is the backend guaranteed to always produce a joinedResponse that is a strict superset of the streamed progress chunks? If not, this is a correctness issue.

Recommendation: Either (a) keep a guard that falls back to immediate swap when the complete text does not start with the currently displayed text, or (b) document explicitly that the backend contract guarantees prefix consistency. A safe middle ground:

if (!currentStreaming || !joinedResponse.startsWith(currentStreaming)) {
  // Immediate swap -- no animation needed or safe
  setMessages((prev) => [...prev, assistantMessage]);
  setIsLoading(false);
  setStatusMessage(null);
  setStreamingText("");
  return;
}

[IanLindsley]

[LOW] Dead constant -- CHARS_PER_TICK_COMPLETING is identical to CHARS_PER_TICK

Since this constant equals CHARS_PER_TICK, the ternary in AnimatedText (line 422) that switches between them is a no-op. This adds cognitive overhead for readers who expect the two constants to differ.

Recommendation: Either remove the constant and the ternary (just pass CHARS_PER_TICK unconditionally), or if the plan is to tune this value later, add a TODO comment explaining the intent.

[IanLindsley]

[LOW] Version string is hardcoded in four locations

The version v1.3.1 is now hardcoded in login/page.tsx:138, thread.tsx:191, thread.tsx:274, and package.json. Every version bump requires updating all four places (plus package-lock.json), which is error-prone.

Recommendation: Import the version from package.json or define a shared constant:

// src/lib/constants.ts
import pkg from '../../package.json';
export const APP_VERSION = pkg.version;

Then use APP_VERSION in all display locations. This is a pre-existing issue, not introduced by this PR, so flagging as LOW.

[IanLindsley]

[MEDIUM] Race condition: rAF in finalizeComplete can clobber state from a rapid follow-up sendMessage

pendingCompleteRef.current is nulled synchronously on line 142, but the state updates (setIsLoading(false), setMessages, setStreamingText("")) are deferred to a requestAnimationFrame callback. If the user sends a new message between the synchronous null-out and the rAF execution:

1. sendMessage (line 199) checks pendingCompleteRef.current -- it is already null, so the force-finalize branch is skipped.
2. sendMessage sets isLoading(true) and streamingText("") for the new request.
3. The pending rAF fires and executes setIsLoading(false), undoing the loading state for the new message. It also appends the old assistant message (captured in the closure) and clears streamingText.

The result: the loading indicator disappears while the new request is in flight, and the user sees no feedback.

Recommendation: Guard the rAF callback with a check that no new request has started. One clean approach is to use a generation counter:

const generationRef = useRef(0);

const finalizeComplete = useCallback(() => {
  const pending = pendingCompleteRef.current;
  if (!pending) return;

  pendingCompleteRef.current = null;
  const gen = generationRef.current;
  requestAnimationFrame(() => {
    // Bail if a new sendMessage started since we scheduled this
    if (generationRef.current !== gen) return;
    setIsCompleting(false);
    setIsLoading(false);
    setStatusMessage(null);
    setMessages((prev) => [...prev, pending.message]);
    setStreamingText("");
  });
}, []);

// In sendMessage, increment at the start:
generationRef.current++;

Alternatively, move the state updates out of rAF entirely now that all five are batched together (React 18 auto-batches in event handlers, effects, and callbacks).

[IanLindsley]

Follow-up Review

All four previously flagged issues have been addressed. The fixes are well-considered -- in particular, moving the divergence guard into useAnimatedText (issue #3) is a better design than the original, and the code component (issue #1) uses a pragmatic className-based detection that avoids coupling to @assistant-ui/react-markdown internals.

Resolved

• [MEDIUM] Missing code component in streamingMarkdownComponents -- Fixed. code handler added at thread.tsx:103-116 with className-based code-block detection and matching bg-muted rounded border px-1 font-semibold styling. Inline code now renders identically during streaming and after swap.
• [MEDIUM] requestAnimationFrame defers state outside React batching -- Fixed. All five state updates (setIsCompleting, setIsLoading, setStatusMessage, setMessages, setStreamingText) are now inside the rAF callback, eliminating the intermediate render with inconsistent state.
• [MEDIUM] Removed prefix-divergence guard -- Fixed, and improved. The guard now lives in useAnimatedText (thread.tsx:400-410) with three branches: reset on empty text, clamp when text shrinks below displayed length, and reset on content divergence. This is more robust than the original handleComplete-level guard because it protects at the animation layer regardless of caller.
• [LOW] Dead CHARS_PER_TICK_COMPLETING constant -- Fixed. Removed entirely; CHARS_PER_TICK is now passed directly.

New Issues

• [MEDIUM] rAF race condition in finalizeComplete -- pendingCompleteRef.current is nulled synchronously but state updates are deferred to rAF. If the user sends a new message in that window, the rAF fires and sets isLoading(false), clobbering the new request's loading state. See inline comment for details and fix options.

Status: 4/4 original issues resolved. 1 new MEDIUM issue found. Recommendation: APPROVE after addressing the rAF race condition (or accepting the risk for rapid-fire sends during the animation completion window).

[IanLindsley]

Follow-up Review (Third Pass)

Previously Flagged Issues

From initial review (all resolved in second pass):

1. [MEDIUM] Duplicated markdown components missing code -- Resolved
2. [MEDIUM] requestAnimationFrame defers state outside React batching -- Resolved
3. [MEDIUM] Removed prefix-divergence guard -- Resolved
4. [LOW] Dead CHARS_PER_TICK_COMPLETING constant -- Resolved
5. [LOW] Version hardcoded in 4 locations -- Acknowledged (pre-existing)

From second review:
6. [MEDIUM] rAF race condition in finalizeComplete -- Resolved. The requestAnimationFrame wrapper has been removed entirely (use-chat-runtime.ts:138-149). All five state updates (setIsCompleting, setIsLoading, setStatusMessage, setMessages, setStreamingText) are now synchronous within finalizeComplete, and React 18+ auto-batches them into a single render. There is no longer a timing gap between nulling pendingCompleteRef.current and executing the state transitions, so a rapid sendMessage cannot interleave and have its isLoading(true) clobbered. This was the cleanest fix of the options suggested.

Verification of Current State

• useAnimatedText divergence guard (thread.tsx:398-410): The three-branch guard (reset on empty, clamp on shrink, reset on content divergence) is still intact and correctly protects against garbled text at the animation layer.
• Streaming markdown components (thread.tsx:39-117): All component classNames remain in sync with defaultComponents in markdown-text.tsx. The only intentional omission is CodeHeader (copy-to-clipboard is not practical during streaming). The code component styling (bg-muted rounded border px-1 font-semibold) matches exactly.
• handleComplete flow (use-chat-runtime.ts:152-179): The immediate-swap path (!currentStreaming) and the deferred-swap path (set pendingCompleteRef, update streamingText, set isCompleting) are both clean. The ordering of setStreamingText before setIsCompleting ensures useAnimatedText sees the new text before AnimatedText checks the completing flag.
• sendMessage force-finalize (use-chat-runtime.ts:196-201): Correctly handles the edge case where a user sends a new message while a completion is pending -- appends the old assistant message synchronously before starting the new request.
• console.error removal (diff line in catch block): The removed console.error("Chat error:", error) means unexpected errors will not appear in browser dev tools. The user still sees the error via handleError, so this is a minor debugging-convenience tradeoff, not a correctness issue.

New Issues

None found.

Positive Highlights

• Choosing to remove requestAnimationFrame entirely rather than adding a generation counter was the right call -- it is simpler, eliminates the entire class of timing bugs, and React 18 auto-batching makes the rAF unnecessary.
• The code is now clean and well-structured across both files. The animation lifecycle (useAnimatedText -> AnimatedText -> finalizeComplete) has clear responsibilities at each layer.

Status: 5/5 previously flagged issues resolved. 0 new issues. Recommendation: APPROVE