-
Notifications
You must be signed in to change notification settings - Fork 2.6k
ux: Collapse thinking blocks by default #8249
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,8 +1,11 @@ | ||
| import React, { useEffect, useRef, useState } from "react" | ||
| import React, { useEffect, useRef, useState, useCallback } from "react" | ||
| import { useTranslation } from "react-i18next" | ||
| import { useExtensionState } from "@src/context/ExtensionStateContext" | ||
| import { vscode } from "@src/utils/vscode" | ||
|
|
||
| import MarkdownBlock from "../common/MarkdownBlock" | ||
| import { Lightbulb } from "lucide-react" | ||
| import { Lightbulb, ChevronDown, ChevronRight } from "lucide-react" | ||
| import { cn } from "@/lib/utils" | ||
|
|
||
| interface ReasoningBlockProps { | ||
| content: string | ||
|
|
@@ -16,12 +19,45 @@ interface ReasoningBlockProps { | |
| * Render reasoning with a heading and a simple timer. | ||
| * - Heading uses i18n key chat:reasoning.thinking | ||
| * - Timer runs while reasoning is active (no persistence) | ||
| * - Can be collapsed to show only last 2 lines of content | ||
| */ | ||
| export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockProps) => { | ||
| const { t } = useTranslation() | ||
| const { reasoningBlockCollapsed, setReasoningBlockCollapsed } = useExtensionState() | ||
|
|
||
| // Initialize collapsed state based on global setting (default to collapsed) | ||
| const [isCollapsed, setIsCollapsed] = useState(reasoningBlockCollapsed !== false) | ||
|
|
||
| const startTimeRef = useRef<number>(Date.now()) | ||
| const [elapsed, setElapsed] = useState<number>(0) | ||
| const contentRef = useRef<HTMLDivElement>(null) | ||
|
|
||
| // Update collapsed state when global setting changes | ||
| useEffect(() => { | ||
| setIsCollapsed(reasoningBlockCollapsed !== false) | ||
| }, [reasoningBlockCollapsed]) | ||
|
|
||
| // Handle keyboard shortcut for toggling collapsed state | ||
| const handleKeyDown = useCallback( | ||
| (e: KeyboardEvent) => { | ||
| // Ctrl/Cmd + Shift + T to toggle reasoning blocks | ||
| if ((e.ctrlKey || e.metaKey) && e.shiftKey && e.key === "T") { | ||
| e.preventDefault() | ||
| const newState = !isCollapsed | ||
| setIsCollapsed(newState) | ||
| // Update global setting | ||
| setReasoningBlockCollapsed(!newState) | ||
| // Persist to backend | ||
| vscode.postMessage({ type: "setReasoningBlockCollapsed", bool: !newState }) | ||
| } | ||
| }, | ||
| [isCollapsed, setReasoningBlockCollapsed], | ||
|
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. Potential memory leak: The keyboard event listener is recreated on every render due to the const isCollapsedRef = useRef(isCollapsed);
isCollapsedRef.current = isCollapsed;
const handleKeyDown = useCallback((e: KeyboardEvent) => {
if ((e.ctrlKey || e.metaKey) && e.shiftKey && e.key === 'T') {
e.preventDefault();
const newState = !isCollapsedRef.current;
setIsCollapsed(newState);
setReasoningBlockCollapsed(!newState);
vscode.postMessage({ type: 'setReasoningBlockCollapsed', bool: !newState });
}
}, [setReasoningBlockCollapsed]); |
||
| ) | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener("keydown", handleKeyDown) | ||
| return () => window.removeEventListener("keydown", handleKeyDown) | ||
| }, [handleKeyDown]) | ||
|
|
||
| // Simple timer that runs while streaming | ||
| useEffect(() => { | ||
|
|
@@ -36,22 +72,50 @@ export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockP | |
| const seconds = Math.floor(elapsed / 1000) | ||
| const secondsLabel = t("chat:reasoning.seconds", { count: seconds }) | ||
|
|
||
| const handleToggle = () => { | ||
| setIsCollapsed(!isCollapsed) | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| <div className="flex items-center justify-between mb-2.5 pr-2"> | ||
| <div className="group"> | ||
| <div | ||
| className="flex items-center justify-between mb-2.5 pr-2 cursor-pointer select-none" | ||
|
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 issue: This clickable header should have proper ARIA attributes for screen reader users: <div
className="flex items-center justify-between mb-2.5 pr-2 cursor-pointer select-none"
onClick={handleToggle}
role="button"
aria-expanded={!isCollapsed}
aria-label="Toggle reasoning block"
tabIndex={0}
onKeyDown={(e) => e.key === 'Enter' && handleToggle()}
> |
||
| onClick={handleToggle}> | ||
| <div className="flex items-center gap-2"> | ||
| <Lightbulb className="w-4" /> | ||
| <span className="font-bold text-vscode-foreground">{t("chat:reasoning.thinking")}</span> | ||
| {elapsed > 0 && ( | ||
| <span className="text-sm text-vscode-descriptionForeground tabular-nums">{secondsLabel}</span> | ||
| )} | ||
| </div> | ||
| <div className="opacity-0 group-hover:opacity-100 transition-opacity"> | ||
| {isCollapsed ? <ChevronRight className="w-4" /> : <ChevronDown className="w-4" />} | ||
| </div> | ||
| {elapsed > 0 && ( | ||
| <span className="text-sm text-vscode-descriptionForeground tabular-nums flex items-center gap-1"> | ||
| {secondsLabel} | ||
| </span> | ||
| )} | ||
| </div> | ||
| {(content?.trim()?.length ?? 0) > 0 && ( | ||
| <div className="border-l border-vscode-descriptionForeground/20 ml-2 pl-4 pb-1 text-vscode-descriptionForeground"> | ||
| <MarkdownBlock markdown={content} /> | ||
| <div | ||
| ref={contentRef} | ||
| className={cn( | ||
| "border-l border-vscode-descriptionForeground/20 ml-2 pl-4 pb-1 text-vscode-descriptionForeground", | ||
| isCollapsed && "relative overflow-hidden", | ||
| )} | ||
| style={ | ||
| isCollapsed | ||
| ? { | ||
| maxHeight: "3em", // Approximately 2 lines | ||
|
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. Hardcoded styling values: Consider extracting these magic numbers to CSS variables or configuration:
|
||
| maskImage: "linear-gradient(to top, transparent 0%, black 30%)", | ||
| WebkitMaskImage: "linear-gradient(to top, transparent 0%, black 30%)", | ||
| } | ||
| : undefined | ||
| }> | ||
| {isCollapsed ? ( | ||
| // When collapsed, render content in a container that shows bottom-aligned text | ||
| <div className="flex flex-col justify-end" style={{ minHeight: "3em" }}> | ||
| <MarkdownBlock markdown={content} /> | ||
| </div> | ||
| ) : ( | ||
| <MarkdownBlock markdown={content} /> | ||
| )} | ||
| </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.
Missing test coverage: This new component lacks unit tests. Consider adding tests for: