-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: optimize ReasoningBlock performance by isolating timer re-renders #8000
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import React, { useEffect, useRef, useState } from "react" | ||
| import React, { memo, useEffect, useRef, useState, useMemo } from "react" | ||
| import { useTranslation } from "react-i18next" | ||
| import debounce from "debounce" | ||
|
|
||
| import MarkdownBlock from "../common/MarkdownBlock" | ||
| import { Clock, Lightbulb } from "lucide-react" | ||
|
|
@@ -12,47 +13,102 @@ interface ReasoningBlockProps { | |
| metadata?: any | ||
| } | ||
|
|
||
| interface ElapsedTimeProps { | ||
| isActive: boolean | ||
| startTime: number | ||
| } | ||
|
|
||
| /** | ||
| * Render reasoning with a heading and a simple timer. | ||
| * - Heading uses i18n key chat:reasoning.thinking | ||
| * - Timer runs while reasoning is active (no persistence) | ||
| * Memoized timer component that only re-renders itself | ||
| * This prevents the entire ReasoningBlock from re-rendering every second | ||
| */ | ||
| export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockProps) => { | ||
| const ElapsedTime = memo(({ isActive, startTime }: ElapsedTimeProps) => { | ||
| const { t } = useTranslation() | ||
|
|
||
| const startTimeRef = useRef<number>(Date.now()) | ||
| const [elapsed, setElapsed] = useState<number>(0) | ||
|
|
||
| // Simple timer that runs while streaming | ||
| useEffect(() => { | ||
| if (isLast && isStreaming) { | ||
| const tick = () => setElapsed(Date.now() - startTimeRef.current) | ||
| tick() | ||
| if (isActive) { | ||
| const tick = () => setElapsed(Date.now() - startTime) | ||
| tick() // Initial tick | ||
| const id = setInterval(tick, 1000) | ||
| return () => clearInterval(id) | ||
| } else { | ||
| setElapsed(0) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is resetting elapsed to 0 here intentional? This might cause a brief flash if the component transitions from active to inactive and back quickly. Consider keeping the last elapsed value when inactive, or only resetting when the component unmounts. |
||
| } | ||
| }, [isLast, isStreaming]) | ||
| }, [isActive, startTime]) | ||
|
|
||
| if (elapsed === 0) return null | ||
|
|
||
| const seconds = Math.floor(elapsed / 1000) | ||
| const secondsLabel = t("chat:reasoning.seconds", { count: seconds }) | ||
|
|
||
| return ( | ||
| <span className="text-vscode-foreground tabular-nums flex items-center gap-1"> | ||
| <Clock className="w-4" /> | ||
| {secondsLabel} | ||
| </span> | ||
| ) | ||
| }) | ||
|
|
||
| ElapsedTime.displayName = "ElapsedTime" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great job isolating the timer re-renders! However, this component could benefit from test coverage. Given this is a performance-critical fix, consider adding tests to verify that:
|
||
|
|
||
| /** | ||
| * Render reasoning with a heading and a simple timer. | ||
| * - Heading uses i18n key chat:reasoning.thinking | ||
| * - Timer runs while reasoning is active (no persistence) | ||
| * - Timer is isolated in a memoized component to prevent full re-renders | ||
| * - Content updates are debounced to prevent excessive re-renders during streaming | ||
| */ | ||
| export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockProps) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider also wrapping the ReasoningBlock component itself with React.memo. While you've isolated the timer, the parent could still benefit from memoization to prevent unnecessary re-renders when none of its props have changed: |
||
| const { t } = useTranslation() | ||
| const startTimeRef = useRef<number>(Date.now()) | ||
| const [debouncedContent, setDebouncedContent] = useState<string>(content || "") | ||
|
|
||
| // Create a debounced function to update content | ||
| // This limits content updates to a maximum of ~10 per second (100ms debounce) | ||
| const updateDebouncedContent = useMemo( | ||
| () => | ||
| debounce((newContent: string) => { | ||
| setDebouncedContent(newContent) | ||
| }, 100), | ||
| [], | ||
| ) | ||
|
|
||
| // Update debounced content when content changes | ||
| useEffect(() => { | ||
| if (isStreaming) { | ||
| // During streaming, use debounced updates | ||
| updateDebouncedContent(content || "") | ||
| } else { | ||
| // When not streaming, update immediately for final content | ||
| setDebouncedContent(content || "") | ||
| // Cancel any pending debounced updates | ||
| updateDebouncedContent.clear() | ||
| } | ||
| }, [content, isStreaming, updateDebouncedContent]) | ||
|
|
||
| // Cleanup debounce on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| updateDebouncedContent.clear() | ||
| } | ||
| }, [updateDebouncedContent]) | ||
|
|
||
| // Only render markdown when there's actual content | ||
| const hasContent = (debouncedContent?.trim()?.length ?? 0) > 0 | ||
|
|
||
| return ( | ||
| <div className="py-1"> | ||
| <div className="flex items-center justify-between mb-2.5 pr-2"> | ||
| <div className="flex items-center gap-2"> | ||
| <Lightbulb className="w-4" /> | ||
| <span className="font-bold text-vscode-foreground">{t("chat:reasoning.thinking")}</span> | ||
| </div> | ||
| {elapsed > 0 && ( | ||
| <span className="text-vscode-foreground tabular-nums flex items-center gap-1"> | ||
| <Clock className="w-4" /> | ||
| {secondsLabel} | ||
| </span> | ||
| )} | ||
| <ElapsedTime isActive={isLast && isStreaming} startTime={startTimeRef.current} /> | ||
| </div> | ||
| {(content?.trim()?.length ?? 0) > 0 && ( | ||
| {hasContent && ( | ||
| <div className="px-3 italic text-vscode-descriptionForeground"> | ||
| <MarkdownBlock markdown={content} /> | ||
| <MarkdownBlock markdown={debouncedContent} /> | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
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.
Minor type safety improvement: The prop is typed as . Consider defining a proper interface for better type safety: