-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add content debouncing to ReasoningBlock for improved streaming performance #8176
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, { useEffect, useRef, useState, useMemo, memo } 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,99 @@ 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) | ||
| } | ||
| }, [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" | ||
|
|
||
| /** | ||
| * 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) => { | ||
| 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) | ||
|
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. The comment mentions "~10 per second" but with a 100ms debounce, it's exactly 10 per second maximum. Consider updating for precision: |
||
| const updateDebouncedContent = useMemo( | ||
| () => | ||
| debounce((newContent: string) => { | ||
| setDebouncedContent(newContent) | ||
| }, 100), | ||
|
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 it intentional that the debounce delay is hardcoded to 100ms? Consider making this configurable via props for different use cases, or at least extracting it as a named constant for better maintainability. |
||
| [], | ||
| ) | ||
|
|
||
| // 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 || "") | ||
|
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. Good edge case handling here - immediately updating content and clearing pending debounces when streaming stops. This ensures the final content is shown without delay. Though if streaming rapidly toggles between true/false, could there be any race conditions? The implementation looks correct, but might be worth documenting this behavior. |
||
| // Cancel any pending debounced updates | ||
| updateDebouncedContent.clear() | ||
| } | ||
| }, [content, isStreaming, updateDebouncedContent]) | ||
|
|
||
| // Cleanup debounce on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| updateDebouncedContent.clear() | ||
| } | ||
| }, [updateDebouncedContent]) | ||
|
|
||
| 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 && ( | ||
| {(debouncedContent?.trim()?.length ?? 0) > 0 && ( | ||
| <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.
Since this is a performance-critical component with complex debouncing logic, it would benefit from test coverage. Consider adding tests to verify: