Skip to content

Fix streaming transition and inline composer layout #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
IanLindsley merged 4 commits into from
Mar 4, 2026
Merged

Fix streaming transition and inline composer layout #9

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecc7786
Disable auto-scroll to fix jarring jump, enlarge welcome text
ILindsley Mar 3, 2026
4c1a5dd
Fix streaming transition, inline composer layout, bump to v1.3.1
ILindsley Mar 4, 2026
7cfefe7
Address code review: add code component, fix rAF batching, add diverg…
ILindsley Mar 4, 2026
ab965f7
Remove rAF from finalizeComplete to fix race condition
ILindsley Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "bt-servant-web-client",
"version": "1.3.0",
"version": "1.3.1",
"private": true,
"scripts": {
"dev": "next dev",
Expand Down
2 changes: 1 addition & 1 deletion src/app/(auth)/login/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export default function LoginPage() {

{/* Footer */}
<p className="mt-3 text-center font-sans text-[10px] text-[#8a8985] dark:text-[#6b6a68]">
BT Servant Web v1.3.0
BT Servant Web v1.3.1
</p>
</div>
</div>
Expand Down
151 changes: 117 additions & 34 deletions src/components/assistant-ui/thread.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,85 @@ import {
faArrowDown,
} from "@fortawesome/pro-regular-svg-icons";
import { useState, useEffect, useRef, useCallback, type FC } from "react";
import Markdown from "react-markdown";
import remarkGfm from "remark-gfm";
import { cn } from "@/lib/utils";

// Animation constants (like lasker-app)
// Animation constants
const CHARS_PER_TICK = 2;
const CHARS_PER_TICK_COMPLETING = CHARS_PER_TICK; // Same rate during catch-up for smooth feel
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

const TICK_MS = 16; // ~60fps

// Markdown components matching MarkdownText for seamless streaming→complete swap.
// Keep in sync with defaultComponents in src/components/assistant-ui/markdown-text.tsx
/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars */
const streamingMarkdownComponents = {
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test comment

Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

h1: ({ node: _n, className, ...props }: any) => (
<h1
className={cn(
"mb-8 scroll-m-20 text-4xl font-extrabold tracking-tight last:mb-0",
className
)}
{...props}
/>
),
h2: ({ node: _n, className, ...props }: any) => (
<h2
className={cn(
"mt-8 mb-4 scroll-m-20 text-3xl font-semibold tracking-tight first:mt-0 last:mb-0",
className
)}
{...props}
/>
),
h3: ({ node: _n, className, ...props }: any) => (
<h3
className={cn(
"mt-6 mb-4 scroll-m-20 text-2xl font-semibold tracking-tight first:mt-0 last:mb-0",
className
)}
{...props}
/>
),
p: ({ node: _n, className, ...props }: any) => (
<p
className={cn("mt-5 mb-5 leading-7 first:mt-0 last:mb-0", className)}
{...props}
/>
),
a: ({ node: _n, className, ...props }: any) => (
<a
className={cn(
"text-primary font-medium underline underline-offset-4",
className
)}
{...props}
/>
),
ul: ({ node: _n, className, ...props }: any) => (
<ul
className={cn("my-5 ml-6 list-disc [&>li]:mt-2", className)}
{...props}
/>
),
ol: ({ node: _n, className, ...props }: any) => (
<ol
className={cn("my-5 ml-6 list-decimal [&>li]:mt-2", className)}
{...props}
/>
),
pre: ({ node: _n, className, ...props }: any) => (
<pre
className={cn(
"overflow-x-auto rounded-t-none! rounded-b-lg bg-black p-4 text-white",
className
)}
{...props}
/>
),
};
/* eslint-enable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars */

const ScrollToBottomButton: FC<{ visible: boolean; onClick: () => void }> = ({
visible,
onClick,
Expand Down Expand Up @@ -98,6 +172,7 @@ export const Thread: FC = () => {
<ThreadPrimitive.Viewport
ref={viewportRef}
onScroll={handleScroll}
autoScroll
className="flex grow flex-col overflow-y-auto overscroll-none px-4 pt-8"
>
<ThreadPrimitive.Messages components={{ Message: ChatMessage }} />
Expand All @@ -113,7 +188,7 @@ export const Thread: FC = () => {
/>
<Composer />
<p className="mt-2 text-center font-sans text-xs text-[#9a9893]">
BT Servant Web v1.3.0
BT Servant Web v1.3.1
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

</p>
</div>
</AssistantIf>
Expand Down Expand Up @@ -150,7 +225,7 @@ const ThreadWelcome: FC = () => {
<div className="flex w-full max-w-3xl flex-col items-center">
{/* Welcome message */}
<div className="mb-8">
<p className="text-center text-lg text-[#1a1a18] sm:text-2xl dark:text-[#eee]">
<p className="text-center text-2xl text-[#1a1a18] sm:text-3xl dark:text-[#eee]">
Hello, I&apos;m BT Servant. How can I serve you today?
</p>
</div>
Expand Down Expand Up @@ -196,7 +271,7 @@ const ThreadWelcome: FC = () => {
{/* Footer */}
<div className="shrink-0 pb-4">
<p className="text-center font-sans text-xs text-[#9a9893]">
BT Servant Web v1.3.0
BT Servant Web v1.3.1
</p>
</div>
</div>
Expand Down Expand Up @@ -227,30 +302,28 @@ const Composer: FC = () => {
return (
<div className="mx-auto w-full max-w-3xl">
<ComposerPrimitive.Root className="flex w-full flex-col rounded-2xl border border-transparent bg-white p-0.5 shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.035),0_0_0_0.5px_rgba(0,0,0,0.08)] transition-shadow duration-200 focus-within:shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.075),0_0_0_0.5px_rgba(0,0,0,0.15)] hover:shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.05),0_0_0_0.5px_rgba(0,0,0,0.12)] dark:bg-[#1f1e1b] dark:shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.4),0_0_0_0.5px_rgba(108,106,96,0.15)] dark:focus-within:shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.5),0_0_0_0.5px_rgba(108,106,96,0.3)] dark:hover:shadow-[0_0.25rem_1.25rem_rgba(0,0,0,0.4),0_0_0_0.5px_rgba(108,106,96,0.3)]">
<div className="m-3.5 flex flex-col gap-3.5">
<div className="relative">
<div className="m-3.5 flex items-center gap-3">
<div className="relative min-w-0 flex-1">
<div className="max-h-96 w-full overflow-y-auto">
<ComposerPrimitive.Input
placeholder="How can I help you today?"
className="block min-h-6 w-full resize-none bg-transparent font-sans text-[#1a1a18] outline-none placeholder:text-[#9a9893] dark:text-[#eee] dark:placeholder:text-[#9a9893]"
className="block min-h-8 w-full resize-none bg-transparent font-sans text-lg text-[#1a1a18] outline-none placeholder:text-lg placeholder:text-[#9a9893] dark:text-[#eee] dark:placeholder:text-[#9a9893]"
/>
</div>
</div>
<div className="flex w-full items-center gap-2">
<div className="relative flex min-w-0 flex-1 shrink items-center gap-2">
{/* Voice button - hidden when audio disabled */}
{AUDIO_ENABLED && voiceRecorder.isSupported && (
<button
type="button"
onClick={() => setShowVoiceRecorder(true)}
disabled={isLoading}
className="flex h-8 min-w-8 items-center justify-center overflow-hidden rounded-lg border border-[#00000015] bg-transparent px-1.5 text-[#6b6a68] transition-all hover:bg-[#f5f5f0] hover:text-[#1a1a18] active:scale-[0.98] disabled:opacity-50 dark:border-[#6c6a6040] dark:text-[#9a9893] dark:hover:bg-[#393937] dark:hover:text-[#eee]"
aria-label="Voice message"
>
<MicIcon width={16} height={16} />
</button>
)}
</div>
<div className="flex shrink-0 items-center gap-2">
{/* Voice button - hidden when audio disabled */}
{AUDIO_ENABLED && voiceRecorder.isSupported && (
<button
type="button"
onClick={() => setShowVoiceRecorder(true)}
disabled={isLoading}
className="flex h-8 min-w-8 items-center justify-center overflow-hidden rounded-lg border border-[#00000015] bg-transparent px-1.5 text-[#6b6a68] transition-all hover:bg-[#f5f5f0] hover:text-[#1a1a18] active:scale-[0.98] disabled:opacity-50 dark:border-[#6c6a6040] dark:text-[#9a9893] dark:hover:bg-[#393937] dark:hover:text-[#eee]"
aria-label="Voice message"
>
<MicIcon width={16} height={16} />
</button>
)}
<ComposerPrimitive.Send className="flex h-8 w-8 items-center justify-center rounded-lg bg-[#ae5630] transition-colors hover:bg-[#c4633a] active:scale-95 disabled:pointer-events-none disabled:opacity-50 dark:bg-[#ae5630] dark:hover:bg-[#c4633a]">
<ArrowUpIcon width={16} height={16} className="text-white" />
</ComposerPrimitive.Send>
Expand Down Expand Up @@ -300,19 +373,21 @@ const UserMessage: FC = () => {
};

// Animated text hook for streaming - handles character-by-character reveal
function useAnimatedText(text: string): [string, boolean] {
function useAnimatedText(
text: string,
charsPerTick: number = CHARS_PER_TICK
): [string, boolean] {
const [displayedLength, setDisplayedLength] = useState(text.length);
// Track previous text to detect resets
const [prevText, setPrevText] = useState(text);

// Detect text reset and update state together
if (text !== prevText) {
setPrevText(text);
// If text was cleared, reset displayed length
if (
text.length === 0 ||
(prevText.length > 0 && !text.startsWith(prevText))
) {
// Only reset when text is cleared (new message); on growth (including
// the complete-event replacement) keep current position so animation
// catches up smoothly instead of jumping.
if (text.length === 0) {
setDisplayedLength(0);
}
}
Expand All @@ -322,12 +397,12 @@ function useAnimatedText(text: string): [string, boolean] {
if (displayedLength < text.length) {
const interval = setInterval(() => {
setDisplayedLength((prev) =>
Math.min(prev + CHARS_PER_TICK, text.length)
Math.min(prev + charsPerTick, text.length)
);
}, TICK_MS);
return () => clearInterval(interval);
}
}, [text.length, displayedLength]);
}, [text.length, displayedLength, charsPerTick]);

const isAnimationDone = displayedLength >= text.length;
return [
Expand All @@ -342,25 +417,33 @@ const AnimatedText: FC<{
isCompleting: boolean;
onAnimationCaughtUp: () => void;
}> = ({ text, isCompleting, onAnimationCaughtUp }) => {
const [displayedText, isAnimationDone] = useAnimatedText(text);
const [displayedText, isAnimationDone] = useAnimatedText(
text,
isCompleting ? CHARS_PER_TICK_COMPLETING : CHARS_PER_TICK
);
const calledRef = useRef(false);

// Reset the called flag when isCompleting transitions to true
useEffect(() => {
if (isCompleting) {
calledRef.current = false;
}
}, [isCompleting]);

// Fire callback when completing AND animation has caught up
useEffect(() => {
if (isCompleting && isAnimationDone && !calledRef.current) {
calledRef.current = true;
onAnimationCaughtUp();
}
}, [isCompleting, isAnimationDone, onAnimationCaughtUp]);

return <span className="whitespace-pre-wrap">{displayedText}</span>;
return (
<Markdown
remarkPlugins={[remarkGfm]}
components={streamingMarkdownComponents}
>
{displayedText}
</Markdown>
);
};

const AssistantMessage: FC = () => {
Expand Down
24 changes: 14 additions & 10 deletions src/hooks/use-chat-runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,38 +141,43 @@ export function useChatRuntime() {

pendingCompleteRef.current = null;
setIsCompleting(false);
setMessages((prev) => [...prev, pending.message]);
setIsLoading(false);
setStatusMessage(null);
setStreamingText("");
// Add the final message and clear streaming in one batch on next frame
// to avoid a visual flash from renderer swap
requestAnimationFrame(() => {
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

setMessages((prev) => [...prev, pending.message]);
setStreamingText("");
});
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

}, []);

// Define handlers before sendMessage so they can be in the dependency array
const handleComplete = useCallback((data: ChatResponse) => {
const joinedResponse = data.responses.join("\n\n");
const currentStreaming = streamingTextRef.current;

const assistantMessage = createMessage(
`assistant-${Date.now()}`,
"assistant",
joinedResponse,
data.voice_audio_base64 || undefined
);

const currentStreaming = streamingTextRef.current;

// If no streaming text was accumulated, or complete text diverges from
// what was streamed, swap immediately (no animation to wait for)
if (!currentStreaming || !joinedResponse.startsWith(currentStreaming)) {
// If no streaming text was accumulated, swap immediately
if (!currentStreaming) {
Copy link
Copy Markdown
Collaborator Author

@IanLindsley IanLindsley Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;
}

setMessages((prev) => [...prev, assistantMessage]);
setIsLoading(false);
setStatusMessage(null);
setStreamingText("");
return;
}

// Defer swap: store pending data and set full text so animation finishes
// Defer swap: update streaming text to the full response so AnimatedText
// can animate the remaining characters, then finalizeComplete swaps in
// the permanent message once the animation catches up.
pendingCompleteRef.current = { message: assistantMessage };
setIsCompleting(true);
setStreamingText(joinedResponse);
setIsCompleting(true);
setStatusMessage(null);
}, []);

Expand Down Expand Up @@ -313,7 +318,6 @@ export function useChatRuntime() {
setStatusMessage(null);
return;
}
console.error("Chat error:", error);
handleError("Sorry, I encountered an error. Please try again.");
} finally {
abortControllerRef.current = null;
Expand Down
Loading