-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: prevent MCP connection error when toggling Always Allow #8599
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.
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
Conversation
- Add connection state check before fetching tools list - Handle disconnected servers gracefully when updating config - Catch and log errors without disrupting UI flow Fixes #8598
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code again? At least I'm consistent in my mistakes.
| // but we should handle it gracefully | ||
| if (connection.type === "disconnected" || connection.server.status === "disconnected") { | ||
| console.warn(`Server ${serverName} is disconnected. Updating configuration only.`) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: This validation is redundant and creates misleading behavior. The warning message suggests the operation might fail, but updateServerToolList (called on line 1726) performs the exact same connection checks and handles disconnected servers gracefully. This creates two problems:
- Redundant checks: Connection state is validated twice
- Misleading warning: The warning implies failure, but the config update succeeds regardless of connection state
Suggestion: Remove this pre-flight validation entirely. The updateServerToolList method already handles disconnected servers correctly by:
- Checking connection state before fetching tools (line 1693)
- Catching and logging fetch errors (line 1696-1700)
- Still updating the config file even if tools can't be fetched
The current implementation adds no value and confuses the control flow.
| } catch (error) { | ||
| // If fetching tools fails, log the error but don't throw | ||
| // This prevents the "Connection closed" error from disrupting the UI | ||
| console.error(`Failed to fetch tools after updating ${listName} for ${serverName}:`, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: The error is caught and logged, but the UI isn't notified that the tools list might be stale. Consider adding a visual indicator or updating connection.server.error to inform users that the tools list couldn't be refreshed, even though the config was updated successfully.
Description
This PR fixes the "MCP error -32000: Connection closed" error that occurs when users click the "Always Allow" checkbox for MCP server tools.
Problem
When toggling the "Always Allow" setting for an MCP tool, the system immediately attempted to fetch the tools list without checking if the connection was still valid. This caused a connection closed error that disrupted the user experience.
Solution
Changes
updateServerToolListto check connection status before fetching toolstoggleToolAlwaysAllowwith pre-flight connection validationTesting
Related Issue
Fixes #8598
Review Confidence
Implementation review showed 92% confidence with no security issues or breaking changes identified.
Important
Fixes MCP connection error by adding connection validation and error handling in
McpHub.tswhen toggling 'Always Allow'.updateServerToolListbefore fetching tools.toggleToolAlwaysAllowwith pre-flight connection validation.McpHub.ts.This description was created by
for 4444027. You can customize this summary. It will automatically update as commits are pushed.