feat: unify error display with reusable ErrorBanner component #7359
Conversation
- Create ErrorBanner component for consistent error/warning display - Replace generic error rendering with ErrorBanner in ChatRow - Replace diff_error implementation with ErrorBanner - Add comprehensive tests for ErrorBanner component - Provides collapsible, less jarring error presentation with copy functionality Fixes #7358
dosubot
bot
added
size:L
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| justifyContent: "space-between", | ||
| cursor: details ? "pointer" : "default", | ||
| }} | ||
| onClick={details ? handleToggleExpand : undefined}> |
There was a problem hiding this comment.
Could we enhance accessibility here? Consider adding:
aria-expanded={isExpanded}to the clickable divrole="button"andtabIndex={0}for keyboard navigation- Keyboard event handlers (onKeyDown for Enter/Space)
aria-live="polite"for the copy success feedback
This would make the component more accessible to screen reader users.
|
|
||
| const handleToggleExpand = useCallback(() => { | ||
| setIsExpanded(!isExpanded) | ||
| }, [isExpanded]) |
There was a problem hiding this comment.
Performance optimization opportunity: Instead of including isExpanded in the dependency array, could we use the functional update pattern?
| }, [isExpanded]) | |
| const handleToggleExpand = useCallback(() => { | |
| setIsExpanded(prev => !prev) | |
| }, []) |
This removes the dependency and prevents unnecessary re-creation of the callback.
| defaultExpanded = false, | ||
| onCopy, | ||
| actions, | ||
| detailsLanguage = "xml", |
There was a problem hiding this comment.
Is xml always a valid language for the CodeBlock component? Consider validating that the detailsLanguage prop value is supported, or documenting the valid options in the prop interface.
| icon = document.querySelector(".codicon-info") | ||
| expect(icon).toHaveStyle({ color: "var(--vscode-charts-blue)" }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Great test coverage! Consider adding a few more test cases:
- Keyboard navigation (Enter/Space to expand/collapse)
- Different
detailsLanguagevalues - Very long error messages to ensure proper text wrapping
- Edge cases like empty details or special characters
| /** Optional callback when copy button is clicked */ | ||
| onCopy?: () => void | ||
| /** Optional additional actions to display on the right side */ | ||
| actions?: React.ReactNode |
There was a problem hiding this comment.
The actions prop type is quite broad. If we know the specific use cases, could we define a more specific type? For example:
actions?: React.ReactElement<ButtonProps> | React.ReactElement<ButtonProps>[]
Summary
This PR addresses Issue #7358 by unifying error display patterns in the chat view to use a consistent, less jarring banner approach.
Problem
Error messages in the chat view were visually jarring and inconsistent. The generic error path rendered a bold red header and paragraph, which demanded attention even for routine or recoverable issues.
Solution
Created a reusable
ErrorBannercomponent that provides:Changes
ErrorBannercomponent inwebview-ui/src/components/common/ErrorBanner.tsxChatRow.tsxto use ErrorBanner for both generic errors and diff_error casesTesting
Impact
Screenshots
The new error presentation follows the same subtle pattern as diff_error, with a collapsible banner that doesn't overwhelm the interface.
Fixes #7358
Important
Introduces
ErrorBannercomponent for consistent error display in chat view, replacing redundant error handling code and adding comprehensive tests.ErrorBannercomponent inErrorBanner.tsxfor consistent error display.ChatRow.tsxwithErrorBannerfordiff_erroranderrorcases.ErrorBannersupports warning, error, and info variants, with optional copy-to-clipboard and expandable details.ErrorBanner.spec.tsxto verifyErrorBannerfunctionality.ChatRow.tsx.This description was created by
for 82520b9. You can customize this summary. It will automatically update as commits are pushed.