-
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
Conversation
- Add collapsible UI with chevron icon that shows on hover - Default to collapsed state showing only last 2 lines - Move elapsed time counter next to thinking label - Add smooth transitions and gradient mask for collapsed state
- Add reasoningBlockCollapsed setting to global state - Add backend message handler for persisting collapsed state - Add keyboard shortcut (Ctrl/Cmd+Shift+T) to toggle all reasoning blocks - Default to collapsed state for better UX
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.
Reviewed my own code. Found it surprisingly readable for once.
| * - 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) => { |
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:
- Collapse/expand functionality
- Keyboard shortcut handling (Ctrl/Cmd+Shift+T)
- State persistence via global settings
- Timer functionality during streaming
| <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" |
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.
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()}
>| vscode.postMessage({ type: "setReasoningBlockCollapsed", bool: !newState }) | ||
| } | ||
| }, | ||
| [isCollapsed, setReasoningBlockCollapsed], |
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.
Potential memory leak: The keyboard event listener is recreated on every render due to the isCollapsed dependency. Consider using a ref to track the current state:
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]);| style={ | ||
| isCollapsed | ||
| ? { | ||
| maxHeight: "3em", // Approximately 2 lines |
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.
Hardcoded styling values: Consider extracting these magic numbers to CSS variables or configuration:
maxHeight: "3em"could bevar(--reasoning-block-collapsed-height)- Gradient percentages (0%, 30%) could be configurable
Summary
This PR improves the UX of ReasoningBlock components by adding collapsible/expandable states with the following features:
Features Added:
reasoningBlockCollapsedsetting that persists user preferenceImplementation Details:
Testing:
Fixes the issue where reasoning blocks take up too much screen space during long thinking sessions.