-
Notifications
You must be signed in to change notification settings - Fork 2.7k
UI: Render reasoning as plain italic (match <thinking>) #7752
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
…tyle; remove bounding container
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.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| * Render reasoning as simple italic text, matching how <thinking> content is shown. | ||
| * No borders, boxes, headers, timers, or collapsible behavior. | ||
| */ | ||
| export const ReasoningBlock = ({ content }: 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 for this component. Even though it's simpler now, would it make sense to add basic tests to ensure it renders correctly and maintains the expected styling?
| )} | ||
| {isCollapsed ? <CaretDownIcon /> : <CaretUpIcon />} | ||
| </div> | ||
| <div className="px-3 py-1"> |
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.
Consider adding accessibility support here. Since we're relying solely on visual styling (italic text) to distinguish reasoning content, could we add an ARIA label or role to help screen reader users? Something like:
|
|
||
| const { mcpServers, alwaysAllowMcp, currentCheckpoint, mode, apiConfiguration } = useExtensionState() | ||
| const { info: model } = useSelectedModel(apiConfiguration) | ||
| const [reasoningCollapsed, setReasoningCollapsed] = useState(true) |
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.
The state and are no longer used after the simplification. Should we remove this unused import and state declaration?
| } | ||
| }, [thought, prevThought]) | ||
|
|
||
| /** |
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.
Nice documentation! Could we expand it slightly to explain the design rationale? For example:
| {isCollapsed ? <CaretDownIcon /> : <CaretUpIcon />} | ||
| </div> | ||
| <div className="px-3 py-1"> | ||
| <div className="italic text-muted-foreground"> |
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.
Consider using a more semantic class name for better maintainability. Instead of just , perhaps or ? This would make the styling intent clearer:
…g in message.metadata; render timer in UI
…t size, align inline
…dy padding (px-3); make lightbulb icon yellow
00afe75 to
ea322fd
Compare
…fect guard; add ReasoningMeta typing; reduce any casts
- Remove unnecessary persistence logic for timer state - Remove updateMessageReasoningMeta handler from webviewMessageHandler - Remove ReasoningMeta type and related fields from WebviewMessage - Keep simple ephemeral timer that runs while reasoning is active - Only show timer when elapsed time is greater than 0
daniel-lxs
left a comment
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.
LGTM
Remove bounding box/collapse and render reasoning with a heading and persistent timer.
Summary
Changes
Typing
{ messageTs: number; reasoningMeta?: { startedAt?: number; elapsedMs?: number } }.
i18n
Notes
Testing
Screenshot: