Skip to content

fix: add collapsible functionality to ReasoningBlock for better performance (#8066) #8068

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

Closed
wants to merge 1 commit into from
Closed

fix: add collapsible functionality to ReasoningBlock for better performance (#8066) #8068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions webview-ui/src/components/chat/ChatRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,8 @@ export const ChatRowContent = ({
isStreaming={isStreaming}
isLast={isLast}
metadata={message.metadata as any}
isExpanded={isExpanded}
onToggleExpand={handleToggleExpand}
/>
)
case "api_req_started":
Expand Down
48 changes: 35 additions & 13 deletions webview-ui/src/components/chat/ReasoningBlock.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,42 @@ import React, { useEffect, useRef, useState } from "react"
import { useTranslation } from "react-i18next"

import MarkdownBlock from "../common/MarkdownBlock"
import { Clock, Lightbulb } from "lucide-react"
import { Clock, Lightbulb, ChevronDown, ChevronUp } from "lucide-react"
import { ToolUseBlock } from "../common/ToolUseBlock"

interface ReasoningBlockProps {
content: string
ts: number
isStreaming: boolean
isLast: boolean
metadata?: any
isExpanded?: boolean
onToggleExpand?: () => void
}

/**
* Render reasoning with a heading and a simple timer.
* - Heading uses i18n key chat:reasoning.thinking
* - Timer runs while reasoning is active (no persistence)
* - Content can be collapsed/expanded for better performance
*/
export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockProps) => {
export const ReasoningBlock = ({
content,
isStreaming,
isLast,
isExpanded: externalIsExpanded,
onToggleExpand,
}: ReasoningBlockProps) => {
const { t } = useTranslation()

const startTimeRef = useRef<number>(Date.now())
const [elapsed, setElapsed] = useState<number>(0)

// Use internal state if no external control is provided
const [internalIsExpanded, setInternalIsExpanded] = useState(false)
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

Choose a reason for hiding this comment

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

Given the performance issues mentioned in #8066, should we consider starting with collapsed state by default? Currently it starts expanded (false becomes expanded) when using internal state. This might provide better initial performance, especially for large thinking sections.

const isExpanded = externalIsExpanded !== undefined ? externalIsExpanded : internalIsExpanded
const toggleExpand = onToggleExpand || (() => setInternalIsExpanded(!internalIsExpanded))

// Simple timer that runs while streaming
useEffect(() => {
if (isLast && isStreaming) {
Expand All @@ -35,26 +50,33 @@ export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockP

const seconds = Math.floor(elapsed / 1000)
const secondsLabel = t("chat:reasoning.seconds", { count: seconds })
const hasContent = (content?.trim()?.length ?? 0) > 0

return (
<div className="py-1">
<div className="flex items-center justify-between mb-2.5 pr-2">
<ToolUseBlock>
<div
className="flex items-center justify-between px-3 py-2 cursor-pointer hover:bg-vscode-list-hoverBackground"
onClick={toggleExpand}>
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

Choose a reason for hiding this comment

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

Could we improve accessibility here? The clickable div doesn't have keyboard navigation support. Consider adding tabIndex={0}, role="button", aria-expanded={isExpanded}, and keyboard event handlers for Enter/Space keys.

<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>
)}
<div className="flex items-center gap-2">
{elapsed > 0 && (
<span className="text-vscode-foreground tabular-nums flex items-center gap-1">
<Clock className="w-4" />
{secondsLabel}
</span>
)}
{hasContent &&
(isExpanded ? <ChevronUp className="w-4 h-4" /> : <ChevronDown className="w-4 h-4" />)}
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

Choose a reason for hiding this comment

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

Consider adding aria-labels for better screen reader support on the chevron icons. For example: aria-label="Collapse" for ChevronUp and aria-label="Expand" for ChevronDown.

</div>
</div>
{(content?.trim()?.length ?? 0) > 0 && (
<div className="px-3 italic text-vscode-descriptionForeground">
{hasContent && isExpanded && (
<div className="px-4 py-2 italic text-vscode-descriptionForeground border-t border-vscode-panel-border">
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

Choose a reason for hiding this comment

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

Is the nested padding intentional? The ToolUseBlock wrapper adds p-2, and then this content area has px-4 py-2. This might create inconsistent spacing compared to other tool blocks. Consider aligning the padding strategy.

<MarkdownBlock markdown={content} />
</div>
)}
</div>
</ToolUseBlock>
)
}
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

Choose a reason for hiding this comment

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

Missing test coverage for this component. Since this is a critical performance fix, would it be worth adding tests to ensure the collapsible functionality works correctly and prevents regression?

Loading