Skip to content

fix: surface MCP server errors to UI with toast notifications #7640

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 1 commit into from
Closed

fix: surface MCP server errors to UI with toast notifications #7640

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 19 additions & 7 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,10 @@ export class McpHub {
* @param error The error object
*/
private showErrorMessage(message: string, error: unknown): void {
console.error(`${message}:`, error)
const errorMessage = error instanceof Error ? error.message : String(error)
const fullMessage = `${message}: ${errorMessage}`
console.error(fullMessage)
vscode.window.showErrorMessage(fullMessage)
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

Consider implementing rate limiting for error notifications. If MCP servers fail repeatedly during reconnection attempts, users might get spammed with toast notifications. Perhaps we could aggregate errors over a time window or implement a cooldown period?

}
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

⚠️ Critical Issue: This PR claims to fix issue #7639, but that issue is about "Terminal spawns new VS Code terminals instead of reusing existing one" - a completely different problem. This PR actually adds MCP server error notifications to the UI, which doesn't address the terminal reuse issue at all. Is this the wrong issue reference?


public setupWorkspaceFoldersWatcher(): void {
Expand Down Expand Up @@ -692,11 +695,14 @@ export class McpHub {

// Set up stdio specific error handling
transport.onerror = async (error) => {
console.error(`Transport error for "${name}":`, error)
const errorMessage = error instanceof Error ? error.message : `${error}`
const fullMessage = `Transport error for MCP server "${name}": ${errorMessage}`
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

The error message formatting is inconsistent across transport types. Notice that stdio doesn't include "(stdio)" suffix while SSE includes "(SSE)" and streamable-http includes "(streamable-http)". Could we standardize this for consistency?

console.error(fullMessage)
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

I notice we're calling console.error twice for the same error - once here in the transport error handler and once inside showErrorMessage (line 262). This results in duplicate console logging. Should we remove the console.error from these transport handlers since showErrorMessage already handles it?

vscode.window.showErrorMessage(fullMessage)
const connection = this.findConnection(name, source)
if (connection) {
connection.server.status = "disconnected"
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
this.appendErrorMessage(connection, errorMessage)
}
await this.notifyWebviewOfServerChanges()
}
Expand Down Expand Up @@ -747,11 +753,14 @@ export class McpHub {

// Set up Streamable HTTP specific error handling
transport.onerror = async (error) => {
console.error(`Transport error for "${name}" (streamable-http):`, error)
const errorMessage = error instanceof Error ? error.message : `${error}`
const fullMessage = `Transport error for MCP server "${name}" (streamable-http): ${errorMessage}`
console.error(fullMessage)
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

Same duplicate console.error issue here - it's logged both here and in showErrorMessage.

vscode.window.showErrorMessage(fullMessage)
const connection = this.findConnection(name, source)
if (connection) {
connection.server.status = "disconnected"
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
this.appendErrorMessage(connection, errorMessage)
}
await this.notifyWebviewOfServerChanges()
}
Expand Down Expand Up @@ -790,11 +799,14 @@ export class McpHub {

// Set up SSE specific error handling
transport.onerror = async (error) => {
console.error(`Transport error for "${name}":`, error)
const errorMessage = error instanceof Error ? error.message : `${error}`
const fullMessage = `Transport error for MCP server "${name}" (SSE): ${errorMessage}`
console.error(fullMessage)
Copy link
Contributor Author

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

And again here - duplicate console.error logging.

vscode.window.showErrorMessage(fullMessage)
const connection = this.findConnection(name, source)
if (connection) {
connection.server.status = "disconnected"
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
this.appendErrorMessage(connection, errorMessage)
}
await this.notifyWebviewOfServerChanges()
}
Expand Down
Loading