Skip to content

fix: make error display less jarring by matching diff_error style #6782

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

fix: make error display less jarring by matching diff_error style #6782

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae8e4d8
fix: make error display less jarring by matching diff_error style
roomote Aug 7, 2025
eb4bd66
refactor(webview-ui): replace VSCodeButton with IconButton in ChatRow…
Aug 21, 2025
d303138
fix(webview-ui): accessible DisclosureHeader and refactor error/diff_…
Aug 21, 2025
0f47482
chore(webview-ui): use warning triangle and warning color for error h…
Aug 21, 2025
aeb1ea6
feat: extract and display error title from error messages
Aug 21, 2025
8fcd218
fix: extract and display meaningful error titles from error messages
Aug 21, 2025
a3b3446
feat: implement comprehensive error title extraction system
Aug 22, 2025
001d38c
fix(chat): add Missing Required Parameter title mapping for tool arg …
Aug 22, 2025
55f7053
refactor(chat): avoid brittle parsing; map apply_diff param errors to…
Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 91 additions & 8 deletions webview-ui/src/components/chat/ChatRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ export const ChatRowContent = ({
const [reasoningCollapsed, setReasoningCollapsed] = useState(true)
const [isDiffErrorExpanded, setIsDiffErrorExpanded] = useState(false)
const [showCopySuccess, setShowCopySuccess] = useState(false)
const [isErrorExpanded, setIsErrorExpanded] = useState(false)
const [showErrorCopySuccess, setShowErrorCopySuccess] = useState(false)
const [isEditing, setIsEditing] = useState(false)
const [editedContent, setEditedContent] = useState("")
const [editMode, setEditMode] = useState<Mode>(mode || "code")
Expand Down Expand Up @@ -1132,15 +1134,96 @@ export const ChatRowContent = ({
)
case "error":
return (
<>
{title && (
<div style={headerStyle}>
{icon}
{title}
<div>
Copy link
Contributor Author

@roomote roomote bot Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

<div
style={{
marginTop: "0px",
overflow: "hidden",
marginBottom: "8px",
}}>
<div
style={{
borderBottom: isErrorExpanded
? "1px solid var(--vscode-editorGroup-border)"
: "none",
fontWeight: "normal",
fontSize: "var(--vscode-font-size)",
color: "var(--vscode-editor-foreground)",
display: "flex",
alignItems: "center",
justifyContent: "space-between",
cursor: "pointer",
}}
onClick={() => setIsErrorExpanded(!isErrorExpanded)}>
Copy link
Contributor Author

@roomote roomote bot Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clickable div lacks accessibility attributes. Consider adding:

This would improve keyboard navigation and screen reader support.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding accessibility attributes (e.g. role="button" and tabIndex) to the clickable error header div to improve keyboard navigation and screen reader support.

Suggested change
onClick={() => setIsErrorExpanded(!isErrorExpanded)}>
onClick={() => setIsErrorExpanded(!isErrorExpanded)} role="button" tabIndex={0}>

<div
style={{
display: "flex",
alignItems: "center",
gap: "10px",
flexGrow: 1,
}}>
<span
className="codicon codicon-error"
style={{
color: "var(--vscode-errorForeground)",
opacity: 0.8,
fontSize: 16,
marginBottom: "-1.5px",
}}></span>
<span style={{ fontWeight: "bold", color: "var(--vscode-errorForeground)" }}>
Copy link
Contributor Author

@roomote roomote bot Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{t("chat:error")}
</span>
</div>
<div style={{ display: "flex", alignItems: "center" }}>
<VSCodeButton
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

appearance="icon"
style={{
padding: "3px",
height: "24px",
marginRight: "4px",
color: "var(--vscode-editor-foreground)",
display: "flex",
alignItems: "center",
justifyContent: "center",
background: "transparent",
}}
onClick={(e) => {
e.stopPropagation()

// Call copyWithFeedback and handle the Promise
copyWithFeedback(message.text || "").then((success) => {
if (success) {
// Show checkmark
setShowErrorCopySuccess(true)

// Reset after a brief delay
setTimeout(() => {
setShowErrorCopySuccess(false)
}, 1000)
}
})
}}>
<span
className={`codicon codicon-${showErrorCopySuccess ? "check" : "copy"}`}></span>
</VSCodeButton>
<span
className={`codicon codicon-chevron-${isErrorExpanded ? "up" : "down"}`}></span>
</div>
</div>
)}
<p style={{ ...pStyle, color: "var(--vscode-errorForeground)" }}>{message.text}</p>
</>
{isErrorExpanded && (
<div
style={{
padding: "8px",
backgroundColor: "var(--vscode-editor-background)",
borderTop: "none",
}}>
<p style={{ ...pStyle, color: "var(--vscode-errorForeground)" }}>
{message.text}
</p>
</div>
)}
</div>
</div>
)
case "completion_result":
return (
Expand Down
Loading