feat: enhance MCP server management and JSON import/export functionality#325
feat: enhance MCP server management and JSON import/export functionality#325webdevcody merged 2 commits intomainfrom
Conversation
- Introduced pending sync handling for MCP servers to improve synchronization reliability. - Updated auto-test logic to skip servers pending sync, ensuring accurate testing. - Enhanced JSON import/export to support both array and object formats, preserving server IDs. - Added validation for server configurations during import to prevent errors. - Improved error handling and user feedback for sync operations and server updates.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds pending-sync tracking and awaits server syncs in MCP server add/update/enable/ import flows, changes JSON import/export to array-based formats with per-server validation, and removes Electron-only guards from settings migration functions to allow execution in non-Electron contexts. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as UI Component
participant Hook as use-mcp-servers Hook
participant Store as Settings Storage / API
participant Server as MCP Server
User->>UI: Submit add/update/import/enable action
UI->>Hook: invoke add/update/enable/import
Hook->>Store: persist local settings (create/update)
Store-->>Hook: returns server record(s) with id(s)
rect rgb(220,235,255)
Note over Hook: Mark new/changed IDs in pendingSyncServerIdsRef
end
Hook->>Store: syncSettingsToServer()
alt sync succeeds
Store-->>Hook: sync ok
Hook->>Server: auto-test (silent, per-server if enabled)
Server-->>Hook: test result
Hook->>UI: success toast (if applicable)
Hook->>Hook: clear pending IDs
else sync fails
Store-->>Hook: error
Hook->>UI: error toast
Hook->>Store: revert local change (if needed)
Hook->>Hook: clear pending IDs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the management of MCP servers by introducing a 'pending sync' state to ensure data consistency during operations like adding or importing servers. It also overhauls the JSON import and export capabilities, allowing for more flexible data structures and better preservation of server configurations, including unique identifiers. These changes collectively aim to make server management more robust, user-friendly, and adaptable to different deployment contexts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances MCP server management by improving synchronization reliability, expanding JSON import/export capabilities, and strengthening error handling. The introduction of a pending sync state for servers is a smart move to prevent race conditions with auto-testing. The refactoring of JSON handling to support both array and object formats, while preserving server IDs, is a major improvement for data integrity and user experience. The code is cleaner and more robust, especially with the extraction of parsing logic into a helper function and better error feedback. My review includes suggestions to further simplify some of the new logic and to fix a potential bug in the validation of imported server configurations.
| // Validate all servers first | ||
| for (const serverConfig of serversArray) { | ||
| const name = serverConfig.name as string; | ||
| if (!name || typeof name !== 'string') { | ||
| toast.error('Each server must have a name'); | ||
| return false; | ||
| } | ||
|
|
||
| if (typeof servers !== 'object' || Array.isArray(servers)) { | ||
| toast.error('Invalid format: expected object with server configurations'); | ||
| return; | ||
| const serverType = (serverConfig.type as string) || 'stdio'; | ||
| if (serverType === 'stdio') { | ||
| if (!serverConfig.command || typeof serverConfig.command !== 'string') { | ||
| toast.error(`Command is required for "${name}" (stdio)`); | ||
| return false; | ||
| } | ||
| } else if (serverType === 'sse' || serverType === 'http') { | ||
| if (!serverConfig.url || typeof serverConfig.url !== 'string') { | ||
| toast.error(`URL is required for "${name}" (${serverType})`); | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation loop in handleSaveGlobalJsonArray does not check for duplicate server names within the input array. This could lead to incorrect behavior later in the function, as it relies on finding a newly added server by its name to get its ID. If duplicate names exist, it might retrieve the wrong server's ID.
// Validate all servers first
const names = new Set<string>();
for (const serverConfig of serversArray) {
const name = serverConfig.name as string;
if (!name || typeof name !== 'string') {
toast.error('Each server must have a name');
return false;
}
if (names.has(name)) {
toast.error(`Duplicate server name found: "${name}"`);
return false;
}
names.add(name);
const serverType = (serverConfig.type as string) || 'stdio';
if (serverType === 'stdio') {
if (!serverConfig.command || typeof serverConfig.command !== 'string') {
toast.error(`Command is required for "${name}" (stdio)`);
return false;
}
} else if (serverType === 'sse' || serverType === 'http') {
if (!serverConfig.url || typeof serverConfig.url !== 'string') {
toast.error(`URL is required for "${name}" (${serverType})`);
return false;
}
}
}| const newServers = useAppStore.getState().mcpServers; | ||
| const newServerId = newServers[currentCount]?.id; | ||
| if (newServerId) { | ||
| pendingSyncServerIdsRef.current.add(newServerId); | ||
| } | ||
|
|
||
| const syncSuccess = await syncSettingsToServer(); | ||
|
|
||
| // Clear pending sync and trigger auto-test after sync | ||
| if (newServerId) { | ||
| pendingSyncServerIdsRef.current.delete(newServerId); | ||
| if (syncSuccess) { | ||
| const newServer = useAppStore.getState().mcpServers.find((s) => s.id === newServerId); | ||
| if (newServer && newServer.enabled !== false) { | ||
| testServer(newServer, true); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The code to get the newly added server is a bit inefficient. It retrieves the server's ID, and then later re-queries the entire server list from the store to find the server object by that ID. You can get the server object directly when you get the ID, which simplifies the code and avoids an extra store access and search.
const newServers = useAppStore.getState().mcpServers;
const newServer = newServers[currentCount];
if (newServer) {
pendingSyncServerIdsRef.current.add(newServer.id);
}
const syncSuccess = await syncSettingsToServer();
// Clear pending sync and trigger auto-test after sync
if (newServer) {
pendingSyncServerIdsRef.current.delete(newServer.id);
if (syncSuccess) {
if (newServer.enabled !== false) {
testServer(newServer, true);
}
}
}| const newServers = useAppStore.getState().mcpServers.slice(currentCount); | ||
| const newServerIds = newServers.map((s) => s.id); | ||
| newServerIds.forEach((id) => pendingSyncServerIdsRef.current.add(id)); | ||
|
|
||
| const syncSuccess = await syncSettingsToServer(); | ||
|
|
||
| // Clear pending sync and trigger auto-test after sync | ||
| newServerIds.forEach((id) => pendingSyncServerIdsRef.current.delete(id)); | ||
| if (syncSuccess) { | ||
| const latestServers = useAppStore.getState().mcpServers; | ||
| for (const id of newServerIds) { | ||
| const server = latestServers.find((s) => s.id === id); | ||
| if (server && server.enabled !== false) { | ||
| testServer(server, true); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling imported servers is a bit redundant. It gets the new server objects, maps them to an array of IDs, and then later iterates through the IDs to find the server objects again from the store. You can iterate over the newServers array of objects directly to make the code more efficient and readable.
const newServers = useAppStore.getState().mcpServers.slice(currentCount);
newServers.forEach((s) => pendingSyncServerIdsRef.current.add(s.id));
const syncSuccess = await syncSettingsToServer();
// Clear pending sync and trigger auto-test after sync
newServers.forEach((s) => pendingSyncServerIdsRef.current.delete(s.id));
if (syncSuccess) {
for (const server of newServers) {
if (server && server.enabled !== false) {
testServer(server, true);
}
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
446-454: Consider documenting the command parsing limitation.The regex handles basic quoting but doesn't support escaped quotes (e.g.,
"arg with \"nested\" quotes"). This is likely acceptable for most MCP server commands, but a brief comment would help future maintainers understand the limitation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/hooks/use-settings-migration.ts
💤 Files with no reviewable changes (1)
- apps/ui/src/hooks/use-settings-migration.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (6)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (6)
49-50: LGTM!Good use of a ref to track pending sync state without triggering re-renders. This correctly prevents race conditions during auto-testing.
134-150: LGTM!The filtering logic correctly skips both previously auto-tested servers and those pending sync, preventing unnecessary test attempts during synchronization.
473-561: LGTM!Good implementation supporting both legacy object format and new array format. The validation, deduplication by name, and security warning flow are well-handled.
563-595: LGTM!The array-based export format with preserved IDs enables full-fidelity re-import. The conditional field inclusion keeps the output clean.
820-892: LGTM!Good legacy format support. Since object keys are inherently unique, this avoids the duplicate name issue present in the array handler.
894-930: LGTM!Clean format detection and delegation. The sync-and-feedback pattern is consistent with other handlers.
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
Show resolved
Hide resolved
- Added rollback functionality for server updates on sync failure to maintain local state integrity. - Enhanced logic for identifying newly added servers during addition and import processes, ensuring accurate pending sync tracking. - Implemented duplicate server name validation during configuration to prevent errors in server management.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
281-289: Good addition of rollback logic on sync failure.This properly addresses the past review comment about reverting local state when sync fails. The rollback ensures UI consistency when persistence fails.
Optional: Consider deep copy for nested objects
The shallow copy with spread operator works for the current use case, but a deep copy would be more robust for nested fields like
env,args, andheaders:- const previousData = { ...editingServer }; + const previousData = structuredClone(editingServer);This prevents potential issues if the store performs deep merges or if nested objects are mutated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
🧬 Code graph analysis (1)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (3)
apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(194-244)libs/types/src/settings.ts (1)
MCPServerConfig(189-214)apps/ui/src/components/views/settings-view/mcp-servers/types.ts (1)
ServerType(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (3)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (3)
384-404: Well-implemented toggle with rollback and auto-test.The handler properly reverts the enabled state on sync failure and auto-tests newly enabled servers. This provides good user experience and maintains consistency with other handlers.
417-471: Excellent validation and format compatibility.The
parseServerConfighelper properly validates required fields and supports both inline command format (Claude Desktop style) and separate command/args format. This improves import reliability and user experience.
738-764: Thorough validation addresses past review concerns.The duplicate name validation at lines 746-750 properly addresses the past review comment about potential name collisions. Combined with the per-server type validation, this ensures only valid configurations are imported.
Based on past review comments, this validation prevents the issue where duplicate names could cause incorrect server ID retrieval during import.
| updateMCPServer(jsonEditServer.id, updateData); | ||
| await syncSettingsToServer(); | ||
|
|
||
| const syncSuccess = await syncSettingsToServer(); | ||
| if (!syncSuccess) { | ||
| toast.error('Failed to save settings to disk'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Local state not reverted on sync failure.
The handler updates the server at line 686 but doesn't rollback if syncSettingsToServer() fails at line 687. This creates an inconsistency where the UI reflects changes that aren't persisted, similar to the issue addressed at lines 281-289 and 384-393.
🔎 Proposed fix to add rollback
+ const previousServer = structuredClone(jsonEditServer);
updateMCPServer(jsonEditServer.id, updateData);
const syncSuccess = await syncSettingsToServer();
if (!syncSuccess) {
+ updateMCPServer(jsonEditServer.id, previousServer);
toast.error('Failed to save settings to disk');
return;
}Note: Using structuredClone for a deep copy ensures nested objects (env, headers, args) are properly restored.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 686 to 691, the code updates the local server state then calls
syncSettingsToServer() but does not revert the local update if the sync fails,
leaving the UI inconsistent with persisted state; fix by capturing a deep copy
of the original server (e.g., const prev = structuredClone(jsonEditServer) or
the full servers list) before calling updateMCPServer, then perform the update,
call await syncSettingsToServer() inside try/catch, and if it returns false or
throws, call updateMCPServer(prev.id, prev) to rollback the local change, show
the toast error, and return.
| addMCPServer(serverData); | ||
| // Get the newly added server ID | ||
| const newServers = useAppStore.getState().mcpServers; | ||
| const newServer = newServers.find((s) => s.name === serverName); | ||
| if (newServer) { | ||
| processedIds.add(newServer.id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent ID retrieval method compared to other handlers.
The name-based lookup for newly added servers is less robust than the Set-based approach used at lines 315-334 and 345-373. While the duplicate name validation at lines 746-750 prevents collisions, the approach is still fragile and inefficient.
🔎 Proposed fix using Set-based approach for consistency
+ const addedIds = new Set<string>();
+ const existingIds = new Set(mcpServers.map((s) => s.id));
+
// Update or add servers
for (const serverConfig of serversArray) {
// ... build serverData ...
const existingServer = serverId ? existingById.get(serverId) : existingByName.get(serverName);
if (existingServer) {
updateMCPServer(existingServer.id, serverData);
processedIds.add(existingServer.id);
} else {
addMCPServer(serverData);
- // Get the newly added server ID
+ // Find the newly added server by comparing IDs
const newServers = useAppStore.getState().mcpServers;
- const newServer = newServers.find((s) => s.name === serverName);
+ const newServer = newServers.find((s) => !existingIds.has(s.id) && !addedIds.has(s.id));
if (newServer) {
processedIds.add(newServer.id);
+ addedIds.add(newServer.id);
+ existingIds.add(newServer.id); // Update for next iteration
}
}
}This approach matches the pattern used elsewhere in this file (lines 315-334) and is more robust against edge cases.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 806 to 814, the code looks up the newly added server by name which
is less robust and inconsistent with the Set-based ID retrieval used elsewhere;
change this to the same pattern used at lines ~315-334 by capturing the
pre-addition set of IDs, calling addMCPServer, then computing the difference
between the new state IDs and the previous set to get the new server ID(s) (or,
if addMCPServer returns the created ID, use that return value), and add those
ID(s) into processedIds instead of searching by name.
1 participant
Fix: Race condition causes 'Server not found' when adding MCP servers #316
#317
grabbing #320 and #319
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.