-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: resolve reasoning block performance regression (#7999) #8243
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.
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
Conversation
- Extract ElapsedTime component to prevent parent re-renders - Add isExpanded prop to conditionally render markdown content - Implement content debouncing during streaming (~100ms) - Pass isExpanded from ChatRow to ReasoningBlock This addresses the performance regression where the 1Hz timer was causing excessive re-renders of the entire reasoning block component tree. Fixes #7999
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| */ | ||
| export const ElapsedTime = memo(({ isStreaming, isLast }: ElapsedTimeProps) => { | ||
| const { t } = useTranslation() | ||
| const startTimeRef = useRef<number>(Date.now()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timer reset issue: The startTimeRef is initialized once with Date.now() and never updates when the component re-renders. This could show incorrect elapsed times if the component is reused for different reasoning blocks.
Consider resetting the timer when streaming starts:
useEffect(() => {
if (isLast && isStreaming) {
startTimeRef.current = Date.now() // Reset timer
const tick = () => setElapsed(Date.now() - startTimeRef.current)
// ...
}
}, [isLast, isStreaming])| return () => clearInterval(id) | ||
| if (isStreaming) { | ||
| // Debounce content updates to ~10 updates per second max | ||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing cleanup for debounced timeout: When the component unmounts while streaming, the timeout isn't cleared which could cause a memory leak or state update on unmounted component.
Consider tracking and cleaning up the timeout:
const timeoutRef = useRef<NodeJS.Timeout>()
useEffect(() => {
if (isStreaming) {
timeoutRef.current = setTimeout(() => {
setDebouncedContent(content)
}, 100)
} else {
setDebouncedContent(content)
}
return () => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current)
}
}
}, [content, isStreaming])| const startTimeRef = useRef<number>(Date.now()) | ||
| const [elapsed, setElapsed] = useState<number>(0) | ||
| // Debounce content updates during streaming | ||
| const [debouncedContent, setDebouncedContent] = useState(content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using React 18's useDeferredValue: For a cleaner approach to content debouncing, you might want to explore useDeferredValue which is designed for this exact use case:
import { useDeferredValue } from 'react'
const deferredContent = useDeferredValue(content)
// Use deferredContent in renderThis would handle the debouncing automatically and integrate better with React's concurrent features.
|
No longer auto expanding |
Summary
This PR addresses the performance regression in the reasoning UI where a 1Hz timer was causing excessive re-renders during streaming, leading to CPU spikes and degraded UX, especially in long conversations.
Problem
As identified in #7999, the reasoning block component was re-rendering every second due to an internal timer, even when the content was collapsed. This caused:
Solution
Implemented all four suggested improvements from the issue:
1. ✅ Timer Isolation
ElapsedTimecomponent2. ✅ Content Gating
isExpandedprop to conditionally render markdown content3. ✅ Content Debouncing
4. ✅ Timer Width Stability
tabular-numsCSS class to prevent width jitterChanges
webview-ui/src/components/chat/ElapsedTime.tsx- Isolated timer componentwebview-ui/src/components/chat/ReasoningBlock.tsx- Added memoization, debouncing, and conditional renderingwebview-ui/src/components/chat/ChatRow.tsx- Pass isExpanded prop to ReasoningBlockTesting
Performance Impact
These changes should:
Fixes #7999
cc @hannesrudolph @nabilfreeman @daniel-lxs
Important
Improves reasoning UI performance by isolating timer logic, adding content gating, implementing debouncing, and ensuring timer width stability.
ElapsedTimecomponent inElapsedTime.tsxto prevent re-renders ofReasoningBlock.isExpandedprop inReasoningBlock.tsxto conditionally render markdown content.ReasoningBlock.tsx.ReasoningBlock.tsx: Added memoization, debouncing, and conditional rendering.ChatRow.tsx: PassisExpandedprop toReasoningBlock.This description was created by
for bc72495. You can customize this summary. It will automatically update as commits are pushed.