fix: race condition and JSON format issues in MCP server management#319
fix: race condition and JSON format issues in MCP server management#319juniorcammel wants to merge 4 commits intoAutoMaker-Org:mainfrom
Conversation
Phase 1 complete - Document fixes for 3 MCP server bugs: - Bug AutoMaker-Org#1: JSON format inconsistency (array vs object, IDs lost) - Bug AutoMaker-Org#2: HTTP error handling (cryptic error messages) - Bug AutoMaker-Org#3: Race condition in auto-test (server not found) All bugs validated with multiple MCP server types (stdio + HTTP). Patches available for re-application after upstream updates. Related to upstream PR AutoMaker-Org#286 (MCP server support).
|
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. 📝 WalkthroughWalkthroughThis change fixes race conditions and data integrity issues in MCP server management by ensuring disk synchronization completes before UI feedback is shown, and by preserving server IDs in JSON editing through support for both array and legacy object formats with comprehensive validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as UI Component
participant Store as Zustand Store
participant Disk as Disk/Backend
participant Toast as Toast System
rect rgb(220, 240, 255)
Note over User,Toast: Before Fix (Race Condition)
User->>UI: Add/Edit/Toggle/Delete Server
UI->>Store: Update state immediately
Note over UI,Toast: ❌ Feedback shown before sync
UI->>Toast: Show success/loading
UI->>Disk: Sync settings (async, not awaited)
Note over Toast,Disk: ⚠️ Dependent ops execute<br/>before disk state ready
Disk-->>UI: Sync completes (too late)
end
rect rgb(240, 255, 240)
Note over User,Toast: After Fix (Correct Sequencing)
User->>UI: Add/Edit/Toggle/Delete Server
UI->>Store: Update state immediately
UI->>Disk: Await sync settings to complete
alt Sync Success
Disk-->>UI: ✅ Sync confirmed
UI->>Toast: Show success message
Note over Toast: ✅ Feedback after<br/>disk state confirmed
else Sync Failure
Disk-->>UI: ❌ Sync error
UI->>Toast: Show error message
UI->>Store: Revert/abort operation
Note over Store: ❌ State rolled back
end
end
sequenceDiagram
actor User
participant UI as UI Component
participant Parser as JSON Parser
participant Handler as Format Handler
participant Store as Zustand Store
participant Disk as Disk/Backend
participant Toast as Toast System
User->>UI: Open or Save JSON Edit
alt Opening JSON (Export)
UI->>Store: Fetch MCP servers
Store-->>UI: Return server list
UI->>Handler: Build array with IDs
Handler-->>UI: Servers array + IDs preserved
UI->>UI: Display JSON editor
end
alt Saving JSON (Import)
User->>UI: Submit edited JSON
UI->>Parser: Parse JSON string
Parser-->>UI: Parsed object
alt Array Format Detected
UI->>Handler: Route to handleSaveGlobalJsonArray
Handler->>Handler: Validate fields & structure
Handler->>Handler: Match by ID first, then name
Handler->>Store: Update/create/delete servers
Note over Handler: IDs preserved from array
else Object Format Detected
UI->>Handler: Route to handleSaveGlobalJsonObject
Handler->>Handler: Validate fields & structure
Handler->>Handler: Match by name only
Handler->>Store: Update/create/delete servers
Note over Handler: Legacy format support
end
Store-->>UI: State updated
UI->>Disk: Await sync to complete
alt Sync Success
Disk-->>UI: ✅ Confirmed
UI->>Toast: Show success message
UI->>UI: Reset editor state
else Sync Failure
Disk-->>UI: ❌ Error
UI->>Toast: Show error message
Note over Toast: Operation aborted
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (1)
Comment |
Summary of ChangesHello @juniorcammel, 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 delivers crucial stability and usability improvements to MCP server management. It eliminates frustrating 'Server not found' errors by ensuring that server data is fully persisted before success notifications are displayed or auto-tests are run. Additionally, it rectifies a data integrity issue with the JSON editor, guaranteeing that server IDs are preserved during export and import, while maintaining backward compatibility with older configurations. The underlying HTTP communication also benefits from more informative error reporting. 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
|
Added links to PRs AutoMaker-Org#318 and AutoMaker-Org#319 in CUSTOM_CHANGES.md. Updated contribution status to reflect Phase 3 completion. Phase 3 completed: - PR AutoMaker-Org#318: HTTP error handling (fixes AutoMaker-Org#315) - PR AutoMaker-Org#319: Race condition + JSON format (fixes AutoMaker-Org#316, AutoMaker-Org#317) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![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 effectively addresses two important issues: a race condition in MCP server management and a data integrity problem with JSON exports. The changes to await the syncSettingsToServer function before showing success toasts are well-implemented across all relevant handlers, ensuring UI feedback aligns with the persisted state. The move to an array format for JSON editing correctly preserves server IDs, and the support for both old and new formats provides good backward compatibility. I've added a few suggestions to improve error handling in the new JSON editing logic and a note on the documentation.
CUSTOM_CHANGES.md
Outdated
| - [ ] `npm run dev:web` works correctly | ||
| - [ ] `npm run dev:electron` works correctly |
There was a problem hiding this comment.
I noticed that some items in the 'Build Tests' checklist are still unchecked. It would be great to complete this checklist to ensure all tests have passed before merging.
| } catch (error) { | ||
| toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error')); | ||
| } |
There was a problem hiding this comment.
The current error handling prefixes all errors with 'Invalid JSON:', which can be misleading for data validation errors (e.g., a missing server name). It's better to distinguish between JSON syntax errors and other validation errors to provide clearer feedback to the user.
} catch (error) {
if (error instanceof SyntaxError) {
toast.error('Invalid JSON: ' + error.message);
} else if (error instanceof Error) {
toast.error(error.message);
} else {
toast.error('An unknown error occurred while saving.');
}
}| if (typeof config !== 'object' || config === null) { | ||
| toast.error('Invalid server config in array'); | ||
| throw new Error('Invalid server config'); | ||
| } |
There was a problem hiding this comment.
To avoid showing duplicate error toasts, the toast.error call should be removed from this helper function. The calling function handleSaveGlobalJsonEdit will now handle displaying the error toast. Please apply this change to all validation checks in both handleSaveGlobalJsonArray and handleSaveGlobalJsonObject, and ensure the Error constructor receives the user-friendly message.
if (typeof config !== 'object' || config === null) {
throw new Error('Invalid server config in array');
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (2)
323-339: Import flow has partial state on sync failure.If sync fails after importing multiple servers, all servers are added to local state but none persisted to disk. On app restart, all imported servers will be lost. Consider tracking newly added server IDs and rolling them back on sync failure.
595-598: Sync result not checked in single-server JSON edit.Unlike other handlers,
handleSaveJsonEditdoesn't check the return value ofsyncSettingsToServer(). This is inconsistent with the pattern applied elsewhere and means sync failures go unnoticed.🔎 Proposed fix
updateMCPServer(jsonEditServer.id, updateData); - await syncSettingsToServer(); - toast.success('Server configuration updated'); + const syncSuccess = await syncSettingsToServer(); + + if (!syncSuccess) { + toast.error('Failed to save server configuration to disk'); + return; + } + + toast.success('Server configuration updated'); setJsonEditServer(null);
🧹 Nitpick comments (2)
patches/01-fix-http-error-handling.patch (1)
10-21: Consider extracting duplicate error handling into a helper.The same error-handling pattern is repeated in all four HTTP methods. While correct, this creates maintenance burden.
🔎 Proposed refactor to reduce duplication
// Add a private helper method: private async handleResponse<T>(response: Response): Promise<T> { if (!response.ok) { let errorMessage = `HTTP ${response.status}: ${response.statusText}`; try { const errorData = await response.json(); if (errorData.error) { errorMessage = errorData.error; } } catch { // If parsing JSON fails, use status text } throw new Error(errorMessage); } return response.json(); } // Then use in each method: private async post<T>(endpoint: string, body?: unknown): Promise<T> { const response = await fetch(`${this.serverUrl}${endpoint}`, { method: 'POST', headers: this.getHeaders(), body: body ? JSON.stringify(body) : undefined, }); return this.handleResponse<T>(response); }CUSTOM_CHANGES.md (1)
249-251: Use markdown link syntax for URLs.Per markdownlint (MD034), bare URLs should be wrapped in angle brackets or use proper link syntax for better rendering and accessibility.
🔎 Proposed fix
-- Issue #315: Fix HTTP error handling - https://github.com/AutoMaker-Org/automaker/issues/315 -- Issue #316: Fix race condition - https://github.com/AutoMaker-Org/automaker/issues/316 -- Issue #317: Fix JSON format - https://github.com/AutoMaker-Org/automaker/issues/317 +- Issue #315: Fix HTTP error handling - <https://github.com/AutoMaker-Org/automaker/issues/315> +- Issue #316: Fix race condition - <https://github.com/AutoMaker-Org/automaker/issues/316> +- Issue #317: Fix JSON format - <https://github.com/AutoMaker-Org/automaker/issues/317>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CUSTOM_CHANGES.mdapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tspatches/01-fix-http-error-handling.patchpatches/02-fix-race-condition-and-json-format.patchpatches/mcp-fixes-combined-2025-12-29.patch
🧰 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(196-247)apps/ui/src/components/views/settings-view/mcp-servers/types.ts (1)
ServerType(3-3)libs/types/src/settings.ts (1)
MCPServerConfig(188-213)
🪛 LanguageTool
CUSTOM_CHANGES.md
[grammar] ~263-~263: Ensure spelling is correct
Context: ...es - Performance impact is minimal (~50-200ms for sync operations) - Bug #2 (HTTP err...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
CUSTOM_CHANGES.md
249-249: Bare URL used
(MD034, no-bare-urls)
250-250: Bare URL used
(MD034, no-bare-urls)
251-251: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (8)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (5)
345-356: LGTM with note on consistency.The race condition fix pattern is correctly applied. The state inconsistency concern from the edit path applies here as well, though toggling is lower impact than adding/editing.
606-639: LGTM!The change to export servers as an array with preserved IDs addresses the JSON format issue. The server configuration now round-trips correctly through the JSON editor.
641-672: LGTM!The dual-format support with proper sync validation follows the correct pattern. Good separation of concerns by delegating to helper functions.
757-828: LGTM!The object format handler correctly tracks processed names and removes servers that are no longer in the JSON. Unlike the array handler, this doesn't have the same ID-tracking issue because it tracks by name, which is known from the input.
739-754: Cleanup iterates pre-mutation server list
Newly added servers aren’t in the originalmcpServerssnapshot and won’t be removed.Likely an incorrect or invalid review comment.
patches/mcp-fixes-combined-2025-12-29.patch (1)
1-5: Combined patch contains changes already reviewed.This patch combines the HTTP error handling and MCP server fixes. The issues identified in the individual file reviews (state inconsistency on sync failure, newly added servers removal bug) apply to this patch as well.
patches/02-fix-race-condition-and-json-format.patch (1)
1-5: Patch file mirrors main implementation.This patch contains the same changes reviewed in
use-mcp-servers.ts. The issues identified there apply here.CUSTOM_CHANGES.md (1)
1-267: Documentation is comprehensive and well-structured.The CUSTOM_CHANGES.md file provides clear documentation of the bug fixes with before/after code examples, testing checklists, and contribution tracking. This will be valuable for maintaining these patches and upstream contribution.
| if (editingServer) { | ||
| updateMCPServer(editingServer.id, serverData); | ||
|
|
||
| const syncSuccess = await syncSettingsToServer(); | ||
|
|
||
| if (!syncSuccess) { | ||
| toast.error('Failed to save MCP server to disk'); | ||
| return; | ||
| } | ||
|
|
||
| toast.success('MCP server updated'); | ||
| await syncSettingsToServer(); | ||
| handleCloseDialog(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
State inconsistency on sync failure.
When syncSettingsToServer() fails, the local Zustand store already contains the update from updateMCPServer(), but the disk state doesn't. The user sees an error toast, yet the UI shows the updated server. On next app restart, the changes will be lost.
Consider rolling back the local state on sync failure, or at minimum, inform the user that the change exists only locally.
🔎 Proposed fix with rollback
if (editingServer) {
+ // Capture previous state for potential rollback
+ const previousState = mcpServers.find(s => s.id === editingServer.id);
updateMCPServer(editingServer.id, serverData);
const syncSuccess = await syncSettingsToServer();
if (!syncSuccess) {
+ // Rollback local state
+ if (previousState) {
+ updateMCPServer(editingServer.id, previousState);
+ }
toast.error('Failed to save MCP server to disk');
return;
}| const handleDelete = async (id: string) => { | ||
| removeMCPServer(id); | ||
| await syncSettingsToServer(); | ||
|
|
||
| const syncSuccess = await syncSettingsToServer(); | ||
|
|
||
| if (!syncSuccess) { | ||
| toast.error('Failed to save changes to disk'); | ||
| return; | ||
| } | ||
|
|
||
| setDeleteConfirmId(null); | ||
| toast.success('MCP server removed'); | ||
| }; |
There was a problem hiding this comment.
Deleted server reappears after failed sync.
If syncSettingsToServer() fails, the server is removed from UI state but persists on disk. On app restart, the "deleted" server will reappear, confusing the user.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 358 to 370, the code removes the server from local state before
persisting changes so if syncSettingsToServer() fails the deleted server remains
on disk and will reappear; change the flow to perform the persistent sync first
and only remove from state after sync succeeds (or keep the optimistic removal
but immediately restore the server if sync fails). Specifically: attempt to sync
the updated settings to server before calling removeMCPServer (or save the
removed server data to a temp variable so you can call addMCPServer to rollback
on failure), show error toast and do not clear delete confirmation when sync
fails, and only clear setDeleteConfirmId and show success toast after successful
sync and state update.
| return response.json(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "return.*204|res\.status\(204\)|\.send\(\)|\.end\(\)" --type=ts -g '!node_modules/**'Repository: AutoMaker-Org/automaker
Length of output: 297
Handle JSON parse errors
Unconditional response.json() will throw on empty or invalid JSON. Wrap it in try/catch or verify Content-Type/status before parsing to avoid unhandled exceptions.
🤖 Prompt for AI Agents
In patches/01-fix-http-error-handling.patch around lines 23-24, the code calls
response.json() unconditionally which throws on empty or invalid JSON; update
the code to guard parsing by either checking
response.headers.get('content-type') includes 'application/json' and
response.status is not 204/205 before calling json(), or wrap response.json() in
a try/catch and return a safe default (null or {}) on parse failure; ensure any
caught errors are handled or rethrown with context so callers don't get an
unhandled exception.
Shironex
added
Testers-Requested
|
I'm curious why your PR submits patches instead of just including the fixed code? |
|
@juniorcammel Can you elaborate on what @webdevcody asked? |
|
Thanks for pointing that out @webdevcody! You're absolutely right. The patch files shouldn't be in this PR. Those patches exist in my local fork to easily re-apply these fixes after pulling upstream updates, but they're not meant for contribution to the main repo. The actual fix is just the changes to |
Fixes AutoMaker-Org#316, Fixes AutoMaker-Org#317 ## Problem 1: Race Condition (AutoMaker-Org#316) Success toasts and auto-tests executed before `syncSettingsToServer()` completed, causing "Server not found" errors because servers weren't persisted to disk yet. **Impact**: Add server → success toast → auto-test fails immediately ## Problem 2: JSON Format (AutoMaker-Org#317) JSON editor exported servers as object keyed by name (losing IDs), but backend required IDs to locate servers. After editing JSON, backend couldn't find servers. **Impact**: Edit JSON → server IDs lost → backend operations fail ## Solution ### Race Condition Fix Wait for sync completion before showing success notifications: ```typescript const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { toast.error('Failed to save MCP server to disk'); return; } toast.success('MCP server added'); // Only after sync succeeds ``` Applied to: - `handleSave` (edit flow) - `handleSecurityWarningConfirm` (add/import flows) - `handleToggleEnabled` (enable/disable) - `handleDelete` (remove server) ### JSON Format Fix Changed export from object to array with IDs preserved: **Before** (loses IDs): ```json { "mcpServers": { "shadcn": { "type": "stdio", "command": "cmd", ... } } } ``` **After** (preserves IDs): ```json { "mcpServers": [ { "id": "mcp-1767024860547-90lvecvle", "name": "shadcn", "type": "stdio", "command": "cmd", ... } ] } ``` Supports both formats for backward compatibility with Claude Desktop. ## Impact - ✅ Auto-test no longer fails with "Server not found" after adding - ✅ Success toasts appear only after persistence completes - ✅ JSON editor preserves server IDs - ✅ Backend operations succeed after JSON edits - ✅ Backward compatible with Claude Desktop object format ## Testing - ✅ `npm run build` passes with no TypeScript errors - ✅ Add server → sync completes → success toast → auto-test succeeds - ✅ JSON export shows array format with IDs - ✅ JSON edit preserves server IDs - ✅ Legacy object format still imports correctly - ✅ Tested with 3 MCP servers (shadcn, playwright, context7) ## Files Changed - `apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts` - Fixed async timing in 4 server operation handlers - Changed JSON export format from object to array - Added dual-format support for JSON import - Created helper functions for array and object processing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
These files are for local fork maintenance only and should not be included in upstream PRs: - CUSTOM_CHANGES.md (fork-specific changelog) - patches/*.patch (re-application tools for fork) The actual fix remains in use-mcp-servers.ts
juniorcammel
force-pushed
the
fix/race-condition-and-json-format
branch
from
53e4087 to
628ccc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (2)
308-343: State inconsistency on sync failure in both add and import flows.When
syncSettingsToServer()fails:
- Add flow (line 312): The server is added to local state but not persisted to disk.
- Import flow (lines 324-326): Multiple servers are added to local state but not persisted.
In both cases, the UI shows the new servers, but they will disappear on app restart.
🔎 Proposed fix with rollback for both paths
if (pendingServerData.type === 'add' && pendingServerData.serverData) { - addMCPServer(pendingServerData.serverData); + const newServer = addMCPServer(pendingServerData.serverData); const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback: remove the server we just added + if (newServer?.id) { + removeMCPServer(newServer.id); + } toast.error('Failed to save MCP server to disk'); return; } toast.success('MCP server added'); handleCloseDialog(); } else if (pendingServerData.type === 'import' && pendingServerData.importServers) { + const addedServerIds: string[] = []; for (const serverData of pendingServerData.importServers) { - addMCPServer(serverData); + const server = addMCPServer(serverData); + if (server?.id) addedServerIds.push(server.id); } const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback: remove all servers we just added + for (const id of addedServerIds) { + removeMCPServer(id); + } toast.error('Failed to save MCP servers to disk'); return; }
595-600: Missing error handling forsyncSettingsToServer().Line 596 calls
await syncSettingsToServer()but does not check the result or handle errors. If the sync fails:
- No error toast is shown to the user
- The success toast is displayed regardless (line 598)
- Local state is updated but not persisted to disk
This is inconsistent with the error-handling pattern used elsewhere in this PR and constitutes a silent failure.
🔎 Proposed fix with consistent error handling and rollback
updateMCPServer(jsonEditServer.id, updateData); - await syncSettingsToServer(); + const syncSuccess = await syncSettingsToServer(); + + if (!syncSuccess) { + toast.error('Failed to save server configuration to disk'); + return; + } toast.success('Server configuration updated'); setJsonEditServer(null); setJsonEditValue('');
♻️ Duplicate comments (6)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (6)
277-290: State inconsistency on sync failure remains unresolved.When
syncSettingsToServer()fails, the local Zustand store contains the update fromupdateMCPServer()(line 278), but the disk does not. The user sees an error toast, yet the UI still shows the updated server. On app restart, the changes are lost, leading to data loss and user confusion.Consider rolling back the local state on sync failure.
🔎 Proposed fix with rollback
if (editingServer) { + // Capture previous state for potential rollback + const previousState = mcpServers.find(s => s.id === editingServer.id); updateMCPServer(editingServer.id, serverData); const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback local state + if (previousState) { + updateMCPServer(editingServer.id, previousState); + } toast.error('Failed to save MCP server to disk'); return; }
358-370: Deleted server reappears after failed sync.When
syncSettingsToServer()fails, the server is removed from local state (line 359) but persists on disk. On app restart, the "deleted" server will reappear, causing user confusion.🔎 Proposed fix with rollback
const handleDelete = async (id: string) => { + // Save the server for potential rollback + const deletedServer = mcpServers.find(s => s.id === id); removeMCPServer(id); const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback: restore the server + if (deletedServer) { + addMCPServer(deletedServer); + } toast.error('Failed to save changes to disk'); return; }
669-671: Misleading error messages for validation errors.The catch block prefixes all errors with "Invalid JSON:", which is misleading for validation errors thrown by the helper functions (e.g., "Server name is required"). Distinguish between JSON syntax errors and validation errors for clearer user feedback.
🔎 Proposed fix to distinguish error types
} catch (error) { - toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error')); + if (error instanceof SyntaxError) { + toast.error('Invalid JSON: ' + error.message); + } else if (error instanceof Error) { + toast.error(error.message); + } else { + toast.error('An unknown error occurred while saving.'); + } }
675-703: Remove duplicate error toasts from validation logic.Lines 679, 688, 694, and 699 call
toast.errorbefore throwing an error. The caller's catch block (lines 669-671) also displays a toast, potentially showing duplicate error messages. The helper should only throw errors, and the caller should handle toast notifications.Additionally, error messages could include the array index to help users identify which server config is invalid.
🔎 Proposed fix to remove duplicate toasts
const handleSaveGlobalJsonArray = async (serversArray: unknown[]) => { // Validate all servers first - for (const config of serversArray) { + for (let i = 0; i < serversArray.length; i++) { + const config = serversArray[i]; if (typeof config !== 'object' || config === null) { - toast.error('Invalid server config in array'); - throw new Error('Invalid server config'); + throw new Error(`Invalid server config at index ${i}`); } const serverConfig = config as Record<string, unknown>; const name = serverConfig.name as string; const serverType = (serverConfig.type as string) || 'stdio'; if (!name || typeof name !== 'string') { - toast.error('Server name is required'); - throw new Error('Server name required'); + throw new Error(`Server name is required at index ${i}`); } if (serverType === 'stdio') { if (!serverConfig.command || typeof serverConfig.command !== 'string') { - toast.error(`Command is required for "${name}" (stdio)`); - throw new Error('Command required'); + throw new Error(`Command is required for "${name}" (stdio)`); } } else if (serverType === 'sse' || serverType === 'http') { if (!serverConfig.url || typeof serverConfig.url !== 'string') { - toast.error(`URL is required for "${name}" (${serverType})`); - throw new Error('URL required'); + throw new Error(`URL is required for "${name}" (${serverType})`); } } }Apply similar changes to
handleSaveGlobalJsonObject(lines 758-828).
345-356: State inconsistency on sync failure in toggle flow.When
syncSettingsToServer()fails, the server's enabled state is toggled in local state (line 346) but not persisted to disk. On app restart, the toggle is lost, and the server reverts to its previous enabled state.🔎 Proposed fix with rollback
const handleToggleEnabled = async (server: MCPServerConfig) => { + const previousEnabled = server.enabled; updateMCPServer(server.id, { enabled: !server.enabled }); const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback toggle + updateMCPServer(server.id, { enabled: previousEnabled }); toast.error('Failed to save MCP server to disk'); return; }
641-672: State inconsistency on sync failure in global JSON edit.When
handleSaveGlobalJsonArrayorhandleSaveGlobalJsonObjectexecutes (lines 650, 653), they callupdateMCPServer,addMCPServer, andremoveMCPServermultiple times, mutating local state. IfsyncSettingsToServer()fails at line 659, these mutations persist in memory but not on disk. On app restart, all changes are lost, yet the UI shows them.This affects multiple servers and is more severe than single-server edits.
🔎 Proposed fix: capture state before mutations
const handleSaveGlobalJsonEdit = async () => { try { const parsed = JSON.parse(globalJsonValue); // Support both formats: array and object const servers = parsed.mcpServers || parsed; + // Capture current state for rollback + const previousServers = [...mcpServers]; + if (Array.isArray(servers)) { // Array format (new format with IDs) await handleSaveGlobalJsonArray(servers); } else if (typeof servers === 'object' && servers !== null) { // Object format (legacy Claude Desktop format) await handleSaveGlobalJsonObject(servers); } else { toast.error('Invalid format: expected array or object with server configurations'); return; } const syncSuccess = await syncSettingsToServer(); if (!syncSuccess) { + // Rollback: restore previous state + // Clear current servers + for (const server of mcpServers) { + removeMCPServer(server.id); + } + // Re-add previous servers + for (const server of previousServers) { + addMCPServer(server); + } toast.error('Failed to save MCP servers to disk'); return; }Note: This assumes
addMCPServercan accept a fullMCPServerConfigwithid. If not, you may need to adjust the Zustand store API.
📜 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
🔇 Additional comments (1)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
606-639: Good implementation of ID preservation in global JSON export.The array format with IDs preserved (lines 607-635) addresses the JSON data integrity issue from #317. The wrapper
{ mcpServers: array }maintains compatibility with the import flow.
|
ty for the fix, I'm grabbing and combining your other one into #325 |
5 participants
Fixes #316, Fixes #317
This PR addresses two critical bugs in MCP server management that affect data persistence and timing.
Problem 1: Race Condition (#316)
Success toasts and auto-tests executed before
syncSettingsToServer()completed, causing "Server not found" errors during auto-test because servers weren't persisted to disk yet.User Impact:
Problem 2: JSON Format (#317)
JSON editor exported servers as an object keyed by name (losing server IDs), but backend required IDs to locate servers. After editing JSON, backend couldn't find servers because IDs were missing.
User Impact:
Solution
Race Condition Fix
Wait for
syncSettingsToServer()completion before showing success notifications:Applied to:
handleSave(edit existing server)handleSecurityWarningConfirm(add new / import multiple)handleToggleEnabled(enable/disable toggle)handleDelete(remove server)JSON Format Fix
Changed export format from object to array with IDs preserved:
Before (object format - loses IDs):
{ "mcpServers": { "shadcn": { "type": "stdio", "command": "cmd", "args": ["/c", "npx", "shadcn@latest", "mcp"] } } }After (array format - preserves IDs):
{ "mcpServers": [ { "id": "mcp-1767024860547-90lvecvle", "name": "shadcn", "type": "stdio", "command": "cmd", "args": ["/c", "npx", "shadcn@latest", "mcp"], "enabled": true } ] }Key changes:
handleSaveGlobalJsonArrayandhandleSaveGlobalJsonObjectImpact
Race Condition
JSON Format
Testing
Build:
npm run buildpasses with no TypeScript errorsRace Condition:
JSON Format:
Real MCP Servers Tested:
Files Changed
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts:handleSaveasync timinghandleSecurityWarningConfirmasync timinghandleToggleEnabledasync timinghandleDeleteasync timinghandleOpenGlobalJsonEditto export array with IDshandleSaveGlobalJsonEditwith dual-format supporthandleSaveGlobalJsonArrayhelper (array format processor)handleSaveGlobalJsonObjecthelper (legacy object format processor)Dependencies
Requires: #318 (HTTP error handling) for proper error visibility during sync operations
Additional Context
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.