feat: improve error display with collapsible UI and contextual titles #7401
Conversation
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error display in the chat interface by introducing collapsible UI components and contextual error titles for improved user experience and clarity.
- Unified error display UX with collapsible headers, copy-to-clipboard functionality, and visual consistency between regular errors and diff errors
- Added optional title field to messages for displaying contextual error information like "File Not Found" or "Tool Call Error: <tool_name>"
- Updated all tool implementations to provide tool-specific error titles and propagate contextual information
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| webview-ui/src/components/chat/ChatRow.tsx | Implements collapsible error UI with copy functionality matching diff_error design |
| webview-ui/src/components/chat/tests/ChatRow.spec.tsx | Comprehensive test suite for error UI behaviors and custom titles |
| packages/types/src/message.ts | Adds optional title field to message schema |
| src/core/task/Task.ts | Updates say() method to support custom error titles |
| src/core/prompts/responses.ts | Enhances toolError formatter to include tool names |
| src/core/prompts/tests/responses-tool-error.spec.ts | Tests for tool error formatting with/without tool names |
| src/core/assistant-message/presentAssistantMessage.ts | Integrates tool names into error display |
| src/core/tools/*.ts | Updates all tool files to pass tool names and contextual titles |
| src/i18n/locales/en/tools.json | Adds common error title translations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| className={`${ | ||
| isErrorExpanded ? "border-b border-vscode-editorGroup-border" : "" | ||
| } font-normal text-base text-vscode-editor-foreground flex items-center justify-between cursor-pointer focus:outline focus:outline-2 focus:outline-vscode-focusBorder`} |
There was a problem hiding this comment.
[nitpick] This long className string with template literals could be extracted to a utility function or use a classnames library for better readability and maintainability. The same pattern is duplicated in the diff_error section.
src/core/task/Task.ts
Outdated
| `Roo tried to use ${toolName}${ | ||
| relPath ? ` for '${relPath.toPosix()}'` : "" | ||
| } without value for required parameter '${paramName}'. Retrying...`, | ||
| undefined, // images | ||
| undefined, // partial | ||
| undefined, // checkpoint | ||
| undefined, // progressStatus | ||
| { title: `Tool Call Error: ${toolName}` }, // Custom title for the error |
There was a problem hiding this comment.
[nitpick] Multiple undefined parameters make this function call hard to read and maintain. Consider using an options object or named parameters to improve readability.
| await cline.say( | ||
| "error", | ||
| t("mcp:errors.invalidJsonArgument", { toolName: params.tool_name }), | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { title: t("tools:errors.invalidInput") }, | ||
| ) |
There was a problem hiding this comment.
[nitpick] The same pattern of multiple undefined parameters is repeated here and in other tool files. This makes the code harder to read and maintain. Consider refactoring the say() method to use an options object.
| await cline.say( | |
| "error", | |
| t("mcp:errors.invalidJsonArgument", { toolName: params.tool_name }), | |
| undefined, | |
| undefined, | |
| undefined, | |
| undefined, | |
| { title: t("tools:errors.invalidInput") }, | |
| ) | |
| await cline.say({ | |
| type: "error", | |
| message: t("mcp:errors.invalidJsonArgument", { toolName: params.tool_name }), | |
| title: t("tools:errors.invalidInput"), | |
| }) |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { title: "Parse Error" }, |
There was a problem hiding this comment.
Avoid using an inline English fallback string for the title. Rather than hardcoding 'Parse Error', use a translatable string (e.g. t('tools:errors.parseError')) so that the message is consistently localized.
| { title: "Parse Error" }, | |
| { title: t('tools:errors.parseError') }, |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this excellent enhancement to error display! The implementation of collapsible UI and contextual titles is well done with great backward compatibility.
Review Summary
Positive Aspects:
- ✅ Excellent backward compatibility maintained with optional
message.titlefield - ✅ Comprehensive test coverage added
- ✅ Good UX parity between error and diff_error displays
- ✅ Clean implementation of contextual tool error titles
Suggestions for Improvement:
-
Accessibility improvements needed - The collapsible error UI in
ChatRow.tsx(lines 1109-1158) lacks proper accessibility attributes. Consider addingrole="button",tabIndex={0},aria-expanded, and keyboard handlers for Enter/Space keys to match the diff_error implementation which already has these. -
Style consistency - The error UI implementation uses inline style objects while the project guidelines specify using Tailwind CSS classes. The diff_error section already uses Tailwind classes correctly.
-
i18n consistency issue - The test file
ChatRow.spec.tsxexpects "Diff Error" as the title, but the actual locale file has "Edit Unsuccessful". This mismatch could cause test failures. -
Code duplication - The collapsible header logic is duplicated between error and diff_error implementations. Consider extracting a shared
CollapsibleHeadercomponent. -
Language detection for CodeBlock - Generic errors are rendered with
language="xml", but not all errors are XML. Consider using "text" by default or auto-detecting when<error>tags are present. -
Consider centralizing error titles - Common error titles like "File Not Found", "Parse Error" etc. are currently hardcoded. Consider moving them to i18n files for better maintainability.
Overall, this is a solid implementation that significantly improves the error display UX. The suggestions above are mostly about code consistency and maintainability rather than functionality issues.
hannesrudolph
added
the
Issue/PR - Triage
dosubot
bot
added
size:L
| <span className="font-bold">{title}</span> | ||
| </div> | ||
| <div className="flex items-center"> | ||
| <VSCodeButton |
There was a problem hiding this comment.
Consider adding an explicit aria-label (e.g., 'Copy error text') to the VSCodeButton to improve accessibility for screen readers.
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
hannesrudolph
left a comment
hannesrudolph
left a comment
There was a problem hiding this comment.
Thank you for this excellent enhancement to error display! I've reviewed the changes and found that several issues from the previous review have been resolved.
✅ Resolved Issues from Previous Review:
- Accessibility improvements - The CollapsibleErrorSection component now has proper accessibility attributes (role="button", tabIndex={0}, aria-expanded, and keyboard handlers for Enter/Space keys)
- i18n consistency - The locale file correctly has "Edit Unsuccessful" matching the code
- Language detection - Now intelligently detects whether to use "xml" or "text" based on error content
📝 Remaining Suggestions for Improvement:
1. Style Consistency (webview-ui/src/components/chat/ChatRow.tsx)
The error handling section (around lines 859-891 for subtask_result) still uses inline style objects while the project guidelines specify using Tailwind CSS classes. The CollapsibleErrorSection component correctly uses Tailwind - could we apply the same pattern throughout ChatRow.tsx for consistency?
For example, instead of:
style={{ marginTop: "4px", backgroundColor: "var(--vscode-badge-background)" }}
Consider:
className="mt-1 bg-vscode-badge-background"
2. Component Reusability Opportunity
Great job extracting CollapsibleErrorSection! Consider making it even more generic to reduce duplication between error and diff_error cases. Both share the same collapsible header logic, copy functionality, and expand/collapse behavior. A more generic component could accept the icon, title, and content as props.
3. Error Title Centralization (src/core/prompts/responses.ts)
Nice improvement with the contextual tool error titles! For future enhancement, consider moving common error titles like "File Not Found", "Parse Error", "Invalid Line Number" to the i18n files. This would centralize error messages and make them easier to maintain and translate.
💚 Positive Highlights:
- Excellent test coverage - Comprehensive tests for all new functionality including edge cases
- Great backward compatibility - Optional message.title field ensures no breaking changes
- Solid accessibility implementation - The CollapsibleErrorSection component sets a good example for the codebase
- Smart language detection - Automatically choosing between "xml" and "text" based on content
Overall, this is a solid implementation that significantly improves the error display UX. The remaining suggestions are mainly about code consistency and maintainability rather than functional issues. Well done!
dosubot
bot
added
size:XXL
|
Small PR |
dosubot
bot
added
size:L
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
… insertContent title
…Rejected; execCommand/fetchInstructions i18n
…th; title-case error titles)
…hangesRejected') in tools
…tion/executeCommand); keep i18n generic noChanges/changesRejected
hannesrudolph
force-pushed
the
feat/error-display-improvements
branch
from
b51ce70 to
1734b67
Compare
…'No QueryClient set' error
|
Progress update: fixed webview unit tests by wrapping ChatRow tests with QueryClientProvider and aligned tool error titles to Title Case. All platform unit tests now pass. One integration test is failing (list_files symlink visibility). I’m investigating src/services/glob/list-files.ts to ensure symlinked files/dirs appear in non-recursive listings. Will push a follow-up fix shortly. |
…ode and only follow symlinks when recursive=true
…l symlinked files in non-recursive mode
|
I like the idea of a better error display, though I think it would be a lot nicer to say "Error Reading File" than "Tool Call Error: read_file". Ideally the end users don't need to know the internal names of our tools. Would also be nice to be more consistent between sentence and title case, though I defer to @brunobergher on which one we should align in. |
The original scope of this PR was to simply make it so we don't have red error blobs and to drag them into a more unified error display. The exact wording and such has become a sticking point and I am not sure what to do. I was just trying to fix the problem of the red error blobs since it is gross. |
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
| }, | ||
| "executeCommand": { | ||
| "workingDirMissing": "Arbeitsverzeichnis nicht gefunden: '{{workingDir}}'", | ||
| "shellIntegrationGenericError": "Befehlsausführung aufgrund unerwarteten Shell-Integrationsfehlers fehlgeschlagen." |
There was a problem hiding this comment.
The error message for "shellIntegrationGenericError" seems to be missing a word for clarity. Consider updating "Befehlsausführung aufgrund unerwarteten Shell-Integrationsfehlers fehlgeschlagen." to "Befehlsausführung aufgrund eines unerwarteten Shell-Integrationsfehlers fehlgeschlagen."
| "shellIntegrationGenericError": "Befehlsausführung aufgrund unerwarteten Shell-Integrationsfehlers fehlgeschlagen." | |
| "shellIntegrationGenericError": "Befehlsausführung aufgrund eines unerwarteten Shell-Integrationsfehlers fehlgeschlagen." |
| "fileAlreadyExists": "फ़ाइल पहले से मौजूद है", | ||
| "directoryNotFound": "निर्देशिका नहीं मिली", | ||
| "invalidPath": "अमान्य पथ", | ||
| "readError": "त्रुटि पढ़ें", |
There was a problem hiding this comment.
Typo: The 'readError' message uses “त्रुटि पढ़ें”, which is inconsistent with the 'writeError' message (“लिखने में त्रुटि”). Consider changing it to “पढ़ने में त्रुटि” for consistency.
| "readError": "त्रुटि पढ़ें", | |
| "readError": "पढ़ने में त्रुटि", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "errors": { | ||
| "toolCallError": "ツール呼び出しエラー: {{toolName}}", | ||
| "toolExecutionError": "ツール実行エラー", | ||
| "missingParamForTool": "Roo が必須パラメータ '{{paramName}}' の値なしで {{toolName}} を使用しようとしました。再试行...", |
There was a problem hiding this comment.
Typo: The word '再试行' appears to be in Simplified Chinese. For consistency with Japanese language, consider using '再試行'.
| "missingParamForTool": "Roo が必須パラメータ '{{paramName}}' の値なしで {{toolName}} を使用しようとしました。再试行...", | |
| "missingParamForTool": "Roo が必須パラメータ '{{paramName}}' の値なしで {{toolName}} を使用しようとしました。再試行...", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "connectionError": "연결 오류" | ||
| }, | ||
| "executeCommand": { | ||
| "workingDirMissing": "작업 디렉터리를 찾을 수 없음: '{{workingDir}}'", |
There was a problem hiding this comment.
The message on line 45 uses "디렉터리" whereas the 'directoryNotFound' message (line 35) uses "디렉토리". Consider using a consistent term for 'directory' throughout the file.
| "workingDirMissing": "작업 디렉터리를 찾을 수 없음: '{{workingDir}}'", | |
| "workingDirMissing": "작업 디렉토리를 찾을 수 없음: '{{workingDir}}'", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
github-project-automation
bot
moved this from PR [Changes Requested]
to Done
in Roo Code Roadmap
Title: feat: improve error display with collapsible UI and contextual titles
Summary
What changed
Why
User impact
Tests
Internationalization
Compatibility
Performance and security
Important
Enhances error display with collapsible UI and contextual titles, ensuring backward compatibility and adding comprehensive tests.
ChatRow.tsxwith copy-to-clipboard functionality.CollapsibleErrorSectioncomponent for error display.message.titlefor contextual error titles inmessage.ts,Task.ts, andpresentAssistantMessage.ts.responses.ts.toolNameormessage.titleis absent.ChatRow.spec.tsx.responses-tool-error.spec.ts.This description was created by
for 9f7b8ba. You can customize this summary. It will automatically update as commits are pushed.
Important
Enhances error display with collapsible UI and contextual titles, ensuring backward compatibility and adding comprehensive tests.
ChatRow.tsxwith copy-to-clipboard functionality.CollapsibleErrorSectioncomponent for error display.message.titlefor contextual error titles inmessage.ts,Task.ts, andpresentAssistantMessage.ts.responses.ts.toolNameormessage.titleis absent.ChatRow.spec.tsx.responses-tool-error.spec.ts.This description was created by
for b20c4d8. You can customize this summary. It will automatically update as commits are pushed.