-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add collapsible reasoning blocks with auto-expand setting #7874
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 |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| import React, { useState, useEffect, useContext } from "react" | ||
| import { ChevronDown, ChevronRight } from "lucide-react" | ||
| import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "../ui/collapsible" | ||
| import { ExtensionStateContext } from "../../context/ExtensionStateContext" | ||
| import { ReasoningBlock } from "./ReasoningBlock" | ||
|
|
||
| interface CollapsibleReasoningBlockProps { | ||
| content: string | ||
| ts: number | ||
| isStreaming: boolean | ||
| isLast: boolean | ||
| metadata?: any | ||
| } | ||
|
|
||
| export const CollapsibleReasoningBlock: React.FC<CollapsibleReasoningBlockProps> = ({ | ||
|
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. Missing test coverage for this new component. Since this is a new UI component with state management and user interactions, shouldn't we add unit tests to ensure the collapsible behavior works correctly? |
||
| content, | ||
| ts, | ||
| isStreaming, | ||
| isLast, | ||
| metadata, | ||
| }) => { | ||
| const extensionState = useContext(ExtensionStateContext) | ||
| const autoExpand = extensionState?.autoExpandReasoningBlocks ?? false | ||
|
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. Critical issue: The new |
||
|
|
||
| // Start with the configured default state | ||
| const [isOpen, setIsOpen] = useState(autoExpand) | ||
|
|
||
| // Update when the setting changes | ||
| useEffect(() => { | ||
| setIsOpen(autoExpand) | ||
| }, [autoExpand]) | ||
|
|
||
| // Extract first line or preview of the reasoning content | ||
| const getPreviewText = () => { | ||
|
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. Performance optimization: The |
||
| if (!content) return "Thinking..." | ||
| const lines = content.split("\n").filter((line) => line.trim()) | ||
| if (lines.length === 0) return "Thinking..." | ||
|
|
||
| // Get first meaningful line (skip empty lines) | ||
| const firstLine = lines[0] | ||
| const maxLength = 100 | ||
|
|
||
| if (firstLine.length > maxLength) { | ||
| return firstLine.substring(0, maxLength) + "..." | ||
| } | ||
| return firstLine + (lines.length > 1 ? "..." : "") | ||
| } | ||
|
|
||
| return ( | ||
| <Collapsible open={isOpen} onOpenChange={setIsOpen}> | ||
| <div className="bg-vscode-editorWidget-background border border-vscode-editorWidget-border rounded-md overflow-hidden"> | ||
| <CollapsibleTrigger className="flex items-center justify-between w-full p-3 hover:bg-vscode-list-hoverBackground transition-colors"> | ||
|
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. Accessibility improvement: Consider adding aria-expanded attribute for better screen reader support: |
||
| <div className="flex items-center gap-2"> | ||
| {isOpen ? ( | ||
| <ChevronDown className="h-4 w-4 text-vscode-descriptionForeground" /> | ||
| ) : ( | ||
| <ChevronRight className="h-4 w-4 text-vscode-descriptionForeground" /> | ||
| )} | ||
| <span className="text-sm font-medium text-vscode-descriptionForeground">Reasoning</span> | ||
|
Contributor
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 header label "Reasoning" is hardcoded. Consider wrapping it in a translation function to support multiple languages. This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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 "Reasoning" label is hardcoded and not using i18n. For consistency with the rest of the UI, consider using: |
||
| {!isOpen && ( | ||
| <span className="text-sm text-vscode-descriptionForeground ml-2 truncate max-w-[500px]"> | ||
| {getPreviewText()} | ||
| </span> | ||
| )} | ||
| </div> | ||
| </CollapsibleTrigger> | ||
|
|
||
| <CollapsibleContent> | ||
| <div className="border-t border-vscode-editorWidget-border"> | ||
| <ReasoningBlock | ||
| content={content} | ||
| ts={ts} | ||
| isStreaming={isStreaming} | ||
| isLast={isLast} | ||
| metadata={metadata} | ||
| /> | ||
| </div> | ||
| </CollapsibleContent> | ||
| </div> | ||
| </Collapsible> | ||
| ) | ||
| } | ||
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.
Type safety concern: Using
anytype here. Could we define a proper type for metadata or remove it if it's not being used?