fix(stream): add shared StreamingText component for smooth streaming animation#3423
fix(stream): add shared StreamingText component for smooth streaming animation#3423waleedlatif1 wants to merge 3 commits intostagingfrom
Conversation
PR SummaryMedium Risk Overview Updates floating chat, workspace chat, and Copilot message rendering to use Adjusts Written by Cursor Bugbot for commit 7935b21. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR introduces a shared Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Server
participant useChatStreaming
participant MessageComponent
participant StreamingText
participant rAF as requestAnimationFrame
Server->>useChatStreaming: SSE chunk (contentChunk)
useChatStreaming->>useChatStreaming: accumulatedText += chunk
useChatStreaming->>MessageComponent: setMessages({ content: accumulatedText, isStreaming: true })
MessageComponent->>StreamingText: content=accumulatedText, isStreaming=true
Note over StreamingText: useEffect fires<br/>contentRef.current = content
alt isAnimatingRef = false
StreamingText->>rAF: requestAnimationFrame(animateText)
StreamingText->>StreamingText: isAnimatingRef = true
end
rAF->>StreamingText: animateText() callback
StreamingText->>StreamingText: slice content[0..index+CHARS_PER_FRAME]
StreamingText->>StreamingText: setDisplayedContent(slice)
StreamingText->>rAF: requestAnimationFrame(animateText) [loop]
Server->>useChatStreaming: event=end / event=final
useChatStreaming->>MessageComponent: setMessages({ isStreaming: false })
MessageComponent->>StreamingText: isStreaming=false
Note over StreamingText: useEffect cleanup cancels rAF<br/>setDisplayedContent(fullContent)
StreamingText->>MessageComponent: renders complete content
Last reviewed commit: 754bf99 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
...kspace/[workspaceId]/w/[workflowId]/components/chat/components/chat-message/chat-message.tsx
Outdated
Show resolved
Hide resolved
...mponents/copilot/components/copilot-message/components/smooth-streaming/smooth-streaming.tsx
Outdated
Show resolved
Hide resolved
...mponents/copilot/components/copilot-message/components/smooth-streaming/smooth-streaming.tsx
Outdated
Show resolved
Hide resolved
|
@greptile |
|
@cursor review |
...kspace/[workspaceId]/w/[workflowId]/components/chat/components/chat-message/chat-message.tsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@greptile |
| useEffect(() => { | ||
| contentRef.current = content | ||
|
|
||
| if (content.length === 0) { | ||
| setDisplayedContent('') | ||
| indexRef.current = 0 | ||
| return | ||
| } | ||
|
|
||
| if (isStreaming) { | ||
| if (indexRef.current < content.length) { | ||
| const animateText = () => { | ||
| const currentContent = contentRef.current | ||
| const currentIndex = indexRef.current | ||
| if (currentIndex < currentContent.length) { | ||
| const nextIndex = Math.min(currentIndex + CHARS_PER_FRAME, currentContent.length) | ||
| setDisplayedContent(currentContent.slice(0, nextIndex)) | ||
| indexRef.current = nextIndex | ||
| rafRef.current = requestAnimationFrame(animateText) | ||
| } else { | ||
| isAnimatingRef.current = false | ||
| } | ||
| } | ||
|
|
||
| if (!isAnimatingRef.current) { | ||
| if (rafRef.current) { | ||
| cancelAnimationFrame(rafRef.current) | ||
| } | ||
| isAnimatingRef.current = true | ||
| rafRef.current = requestAnimationFrame(animateText) | ||
| } | ||
| } | ||
| } else { | ||
| if (rafRef.current) { | ||
| cancelAnimationFrame(rafRef.current) | ||
| } | ||
| setDisplayedContent(content) | ||
| indexRef.current = content.length | ||
| isAnimatingRef.current = false | ||
| } | ||
|
|
||
| return () => { | ||
| if (rafRef.current) { | ||
| cancelAnimationFrame(rafRef.current) | ||
| } | ||
| isAnimatingRef.current = false | ||
| } | ||
| }, [content, isStreaming]) |
There was a problem hiding this comment.
Animation loop restarted on every streaming chunk
The useEffect cleanup always calls cancelAnimationFrame(rafRef.current) and resets isAnimatingRef.current = false. This means for every incoming content chunk — which can arrive tens of times per second — the running rAF loop is cancelled and a completely new one is scheduled.
This is unnecessary because contentRef.current is already updated at the top of the effect (line 49), so any in-flight animation loop would naturally pick up the latest content on its next frame without needing a restart. The indexRef.current is also preserved across restarts, so there is no correctness gain from the cancellation cycle.
Consider removing the rAF cancellation from the cleanup and only cancelling when isStreaming transitions to false:
useEffect(() => {
contentRef.current = content
if (content.length === 0) {
setDisplayedContent('')
indexRef.current = 0
return
}
if (isStreaming) {
// contentRef is already updated above; the running loop will pick it up.
if (!isAnimatingRef.current && indexRef.current < content.length) {
isAnimatingRef.current = true
rafRef.current = requestAnimationFrame(animateText)
}
} else {
if (rafRef.current) cancelAnimationFrame(rafRef.current)
rafRef.current = null
setDisplayedContent(content)
indexRef.current = content.length
isAnimatingRef.current = false
}
return () => {
if (!isStreaming && rafRef.current) {
cancelAnimationFrame(rafRef.current)
}
isAnimatingRef.current = false
}
}, [content, isStreaming])This way, a single rAF loop runs continuously while streaming and simply catches up to contentRef.current on each frame, avoiding the cancel/restart overhead on every chunk update.
| if (content.length === 0) { | ||
| setDisplayedContent('') | ||
| indexRef.current = 0 | ||
| return | ||
| } |
There was a problem hiding this comment.
isAnimatingRef not explicitly reset in the empty-content path
When the early-return path is taken (content.length === 0), isAnimatingRef.current is left at whatever value it had. The cleanup registered by the previous effect run resets it to false before this effect body executes, so in practice this works correctly. However, if the component is unmounted immediately after content becomes '' — with no prior non-empty effect run — React will not execute a cleanup for this invocation (since none was returned), and isAnimatingRef may remain inconsistent.
Adding the explicit reset makes the invariant self-contained and easier to reason about:
| if (content.length === 0) { | |
| setDisplayedContent('') | |
| indexRef.current = 0 | |
| return | |
| } | |
| if (content.length === 0) { | |
| setDisplayedContent('') | |
| indexRef.current = 0 | |
| isAnimatingRef.current = false | |
| return | |
| } |
1 participant
Summary
StreamingTextcomponent with rAF-based animation (1800 chars/sec vs old 333 chars/sec)SmoothStreamingTextto delegate to shared componentStreamingIndicatornow comes from@/components/uiType of Change
Testing
Tested manually
Checklist