fix: make error display less jarring by matching diff_error style #6782
Conversation
- Added collapsible UI for error messages similar to diff_error display - Added copy button for error text - Added expand/collapse chevron indicator - Error text now appears in a subtle background container when expanded - Maintains error icon and color but in a less jarring presentation
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| <div style={headerStyle}> | ||
| {icon} | ||
| {title} | ||
| <div> |
There was a problem hiding this comment.
I notice there's significant code duplication between the error and diff_error implementations. Both share nearly identical structure for the collapsible UI, copy functionality, and expand/collapse behavior. Could we consider extracting a reusable component like to reduce this duplication and make future maintenance easier?
| justifyContent: "space-between", | ||
| cursor: "pointer", | ||
| }} | ||
| onClick={() => setIsErrorExpanded(!isErrorExpanded)}> |
There was a problem hiding this comment.
The clickable div lacks accessibility attributes. Consider adding:
This would improve keyboard navigation and screen reader support.
| fontSize: 16, | ||
| marginBottom: "-1.5px", | ||
| }}></span> | ||
| <span style={{ fontWeight: "bold", color: "var(--vscode-errorForeground)" }}> |
There was a problem hiding this comment.
Is it intentional that the "Error" label uses color while the diff_error "Diff Error" label uses the default color? This creates a visual inconsistency between the two similar components. If this is meant to indicate severity (errors being more critical than diff errors), that's fine, but wanted to confirm this was deliberate.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| </span> | ||
| </div> | ||
| <div style={{ display: "flex", alignItems: "center" }}> | ||
| <VSCodeButton |
There was a problem hiding this comment.
For better accessibility, add an aria-label to the VSCodeButton (e.g. aria-label="Copy error message") so that its purpose is clear to screen readers.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Draft / In Progress]
in Roo Code Roadmap
hannesrudolph
added
PR - Draft / In Progress
and removed
PR - Needs Preliminary Review
labels
… error/diff_error copy controls; use StandardTooltip with i18n; remove unused import
dosubot
bot
added
size:L
There was a problem hiding this comment.
Pull Request Overview
This PR improves the error display in the chat interface by making it less visually jarring and more consistent with the existing diff_error styling. The error display is now collapsible with a cleaner, more subtle appearance.
- Modified error display to use a collapsible UI with expand/collapse functionality
- Added copy button functionality for error text
- Implemented visual improvements to make the error display less jarring while maintaining clarity
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| background: "transparent", | ||
| }} | ||
| onClick={(e) => { | ||
| <IconButton |
There was a problem hiding this comment.
The copy feedback logic for the diff error and the new error display is duplicated. Consider extracting this into a reusable function to reduce code duplication.
| copyWithFeedback(message.text || "").then((success) => { | ||
| if (success) { | ||
| // Show checkmark | ||
| setShowCopySuccess(true) |
There was a problem hiding this comment.
The copy feedback state being set here should be setShowErrorCopySuccess(true) instead of setShowCopySuccess(true) to match the new error display state variables.
|
|
||
| // Reset after a brief delay | ||
| setTimeout(() => { | ||
| setShowCopySuccess(false) |
There was a problem hiding this comment.
The copy feedback state being set here should be setShowErrorCopySuccess(false) instead of setShowCopySuccess(false) to match the new error display state variables.
| setShowCopySuccess(false) | |
| setShowErrorCopySuccess(false) |
| justifyContent: "space-between", | ||
| cursor: "pointer", | ||
| }} | ||
| onClick={() => setIsErrorExpanded(!isErrorExpanded)}> |
There was a problem hiding this comment.
Consider adding accessibility attributes (e.g. role="button" and tabIndex) to the clickable error header div to improve keyboard navigation and screen reader support.
| onClick={() => setIsErrorExpanded(!isErrorExpanded)}> | |
| onClick={() => setIsErrorExpanded(!isErrorExpanded)} role="button" tabIndex={0}> |
| <IconButton | ||
| iconClass={showErrorCopySuccess ? "codicon-check" : "codicon-copy"} | ||
| title={t("chat:codeblock.tooltips.copy_code")} | ||
| onClick={(e: React.MouseEvent) => { |
There was a problem hiding this comment.
The copy button’s onClick handler in the error block duplicates logic found in the diff_error block. Consider abstracting this copy-with-feedback functionality into a reusable component or helper to reduce duplication.
…error headers (aria, keyboard, chevron consistency)
| {onCopy && ( | ||
| <IconButton | ||
| iconClass={copyIconClass ?? "codicon-copy"} | ||
| title={copyTitle ?? "Copy"} |
There was a problem hiding this comment.
The default copy title is hardcoded as 'Copy'. Consider using a translatable string (via an i18n function) instead of a literal string to adhere to internationalization guidelines.
| title={copyTitle ?? "Copy"} | |
| title={copyTitle ?? t("copy")} |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
…eader to match diff_error
There was a problem hiding this comment.
New case for 'file_not_found_error' mirrors the diff_error UI. Consider using a more specific translation key (e.g., 'chat:fileNotFoundError') instead of reusing 'chat:error' to provide clearer context in the UI.
| title={<span style={{ fontWeight: "bold" }}>{t("chat:error")}</span>} | |
| title={<span style={{ fontWeight: "bold" }}>{t("chat:fileNotFoundError")}</span>} |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
hannesrudolph
force-pushed
the
fix/error-display-styling
branch
from
a7a3988 to
0f47482
Compare
- Parse error messages to extract title before first colon - Display extracted title (e.g. 'File Not Found') instead of generic 'Error' - Remove redundant 'Error' prefix from extracted titles - Maintain full error message in expanded content for context
- Parse error messages to extract specific error types (File Not Found, Permission Denied, etc.) - Display extracted title instead of generic 'Error' in header - Handle 'Error reading file: File not found:' pattern specifically - Maintain full error message in expanded content for context
- Created errorTitleExtractor utility with pattern matching for all error types - Handles MCP errors, file operations, tool errors, API errors, and embeddings errors - Extracts meaningful titles instead of generic 'Error' for better UX - Added comprehensive test suite with 44 test cases covering all error patterns - Updated ChatRow.tsx to use the new extraction utility This addresses the concern that error messages should display specific, meaningful titles rather than just 'Error' in the chat view.
dosubot
bot
added
size:XL
…errors and tests - Map "Roo tried to use ... without value for required parameter" to 'Missing Required Parameter' - Keep existing 'Invalid Tool Arguments' mapping - Add unit test covering the exact screenshot string - No backend changes; only ChatView title extraction
… localized diff error title
- Prefer t('chat:diffError.title') for apply_diff missing param messages
- Keep invalid JSON argument as 'Invalid Tool Arguments'
- Add stable tool-scoped fallbacks for other tools
- Update tests accordingly (45 passing)
|
|
||
| // Special-case: if tool use is missing a required param for apply_diff, title as the diff error (localized). | ||
| // Example: "Roo tried to use apply_diff without value for required parameter 'path'. Retrying..." | ||
| const missingRequiredParamRe = |
There was a problem hiding this comment.
Consider refining the regex for missing required parameters so that it only applies to errors from the apply_diff tool. Currently, the regex matches any error of the form 'Roo tried to use ... without value for required parameter', which might unintentionally affect errors from other tools.
github-project-automation
bot
moved this from PR [Draft / In Progress]
to Done
in Roo Code Roadmap
Summary
BEFORE
AFTER
This PR addresses the Slack request to make the
say(error, ...)display less jarring by matching the styling of the diff_error display.Changes
Before
The error display was a simple red text with an error icon, which was visually jarring.
After
The error display now matches the diff_error style with:
Testing
Important
Improved error display in
ChatRow.tsxby introducing collapsible UI withDisclosureHeaderand extracting error titles usingextractErrorTitle().ChatRow.tsxto use a collapsible UI similar todiff_error.DisclosureHeadercomponent for collapsible headers inDisclosureHeader.tsx.extractErrorTitle()inerrorTitleExtractor.tsto derive titles from error messages.extractErrorTitle()inerrorTitleExtractor.spec.ts.VSCodeButtonimport inChatRow.tsx.This description was created by
for 55f7053. You can customize this summary. It will automatically update as commits are pushed.