fix: add proper HTTP error handling to API client #318
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).
Added links to upstream issues AutoMaker-Org#315, AutoMaker-Org#316, AutoMaker-Org#317 in CUSTOM_CHANGES.md. Updated contribution status to reflect Phase 2 completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes AutoMaker-Org#315 ## Problem HTTP methods didn't check `response.ok` before parsing JSON, causing cryptic "Unexpected end of JSON input" errors instead of showing actual server error messages. ## Solution Added `response.ok` validation to all four HTTP methods (post, get, put, httpDelete) with proper error message extraction: - Check response status before attempting JSON parse - Extract error message from response body if available - Fall back to HTTP status text if JSON parsing fails - Throw descriptive Error with actual server message ## Impact - 404 errors now show "Server not found" instead of JSON parse errors - 500 errors show actual backend error messages - Significantly improves debugging experience for MCP server issues ## Testing - ✅ Build passes with no TypeScript errors - ✅ 404 errors display proper messages - ✅ 500 errors show server error details - ✅ All HTTP methods handle errors consistently 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree bug fixes for the MCP server management system: HTTP error handling now provides meaningful server error messages instead of cryptic parse failures; synchronization waits for server persistence before success confirmation to eliminate race conditions; JSON editing exports servers as ID-preserving arrays while accepting both new and legacy object formats on import. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 addresses a critical issue in the API client's error handling, transforming vague JSON parsing failures into clear, actionable HTTP status-based messages. By implementing robust checks for 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
patches/01-fix-http-error-handling.patch (1)
10-21: Consider extracting the error handling logic to reduce duplication.The same 14-line error handling block is repeated across all four HTTP methods. Extracting it to a private helper method would improve maintainability and ensure consistency if the error handling strategy needs to change in the future.
🔎 Suggested refactor to extract error handling
Add a private helper method to the class:
private async handleErrorResponse(response: Response): Promise<never> { 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); }Then replace each error handling block with:
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); + await this.handleErrorResponse(response); }Apply this pattern to all four methods (post, get, put, httpDelete).
Also applies to: 30-41, 51-62, 72-83
apps/ui/src/lib/http-api-client.ts (1)
165-251: Consider extracting error handling to a shared helper method.The same error handling pattern is repeated in all four HTTP methods (
post,get,put,httpDelete). Extracting this to a helper would improve maintainability.🔎 Optional refactor to reduce duplication
private async handleErrorResponse(response: Response): Promise<never> { 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); } 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, }); if (!response.ok) { await this.handleErrorResponse(response); } return response.json(); } // Apply similar pattern to get, put, httpDeletepatches/02-fix-race-condition-and-json-format.patch (1)
167-362: Significant duplication between array and object format handlers.
handleSaveGlobalJsonArrayandhandleSaveGlobalJsonObjectshare nearly identical validation logic (lines 169-196 vs 283-305) andserverDataconstruction (lines 223-256 vs 320-340). Consider extracting shared logic into helper functions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CUSTOM_CHANGES.mdapps/ui/src/lib/http-api-client.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/lib/http-api-client.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/lib/http-api-client.ts
🪛 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 (4)
patches/01-fix-http-error-handling.patch (1)
10-21: Excellent fix for the JSON parse error issue.The error handling correctly validates
response.okbefore attempting to parse JSON, which directly addresses the cryptic "Unexpected end of JSON input" errors mentioned in issue #315. The fallback strategy (server error field → HTTP status text) provides meaningful error messages for debugging MCP server operations.Also applies to: 30-41, 51-62, 72-83
apps/ui/src/lib/http-api-client.ts (1)
171-183: LGTM! Error handling correctly implemented.The pattern properly validates
response.ok, extracts server-provided error messages when available, and falls back to HTTP status text. This directly addresses the PR objective of surfacing meaningful error messages instead of JSON parse failures.patches/mcp-fixes-combined-2025-12-29.patch (1)
1-451: Combined patch is consistent with individual changes.The issues identified in the individual file reviews apply here as well. The combined patch correctly integrates both the HTTP error handling and the race condition/JSON format fixes.
CUSTOM_CHANGES.md (1)
1-257: Well-structured documentation of the bug fixes.The documentation clearly explains each bug, its impact, the solution, and validation steps. The code examples and testing checklist are helpful for maintainability.
| - 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 |
There was a problem hiding this comment.
Format bare URLs as markdown links.
Per markdownlint, bare URLs should be formatted as proper markdown links for consistency and accessibility.
🔎 Suggested 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 - [#315](https://github.com/AutoMaker-Org/automaker/issues/315)
+- Issue #316: Fix race condition - [#316](https://github.com/AutoMaker-Org/automaker/issues/316)
+- Issue #317: Fix JSON format - [#317](https://github.com/AutoMaker-Org/automaker/issues/317)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 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 - [#315](https://github.com/AutoMaker-Org/automaker/issues/315) | |
| - Issue #316: Fix race condition - [#316](https://github.com/AutoMaker-Org/automaker/issues/316) | |
| - Issue #317: Fix JSON format - [#317](https://github.com/AutoMaker-Org/automaker/issues/317) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
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)
🤖 Prompt for AI Agents
In CUSTOM_CHANGES.md around lines 249 to 251 the issue entries contain bare
URLs; update each to use markdown link syntax by replacing the bare URL with a
link text (e.g., "Issue #315") followed by the URL in parentheses like [Issue
#315](https://github.com/AutoMaker-Org/automaker/issues/315); apply the same
change for Issue #316 and Issue #317 so all three lines use proper markdown
links.
| @@ -276,8 +276,15 @@ export function useMCPServers() { | ||
| // If editing an existing server, save directly (user already approved it) | ||
| 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 risk when sync fails.
When syncSettingsToServer() fails, the local state has already been mutated (updateMCPServer called at line 8). The user sees an error toast, but the UI reflects the update while the disk does not. On page refresh, the changes will be lost without warning.
Consider either:
- Storing the previous state before mutation and rolling back on sync failure
- Clearly communicating to users that changes are pending local-only state
🔎 Suggested rollback pattern
if (editingServer) {
+ const previousData = { ...editingServer };
updateMCPServer(editingServer.id, serverData);
const syncSuccess = await syncSettingsToServer();
if (!syncSuccess) {
+ updateMCPServer(editingServer.id, previousData); // Rollback
toast.error('Failed to save MCP server to disk');
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @@ -276,8 +276,15 @@ export function useMCPServers() { | |
| // If editing an existing server, save directly (user already approved it) | |
| 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; | |
| } | |
| if (editingServer) { | |
| const previousData = { ...editingServer }; | |
| updateMCPServer(editingServer.id, serverData); | |
| const syncSuccess = await syncSettingsToServer(); | |
| if (!syncSuccess) { | |
| updateMCPServer(editingServer.id, previousData); // Rollback | |
| toast.error('Failed to save MCP server to disk'); | |
| return; | |
| } | |
| toast.success('MCP server updated'); | |
| handleCloseDialog(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In patches/02-fix-race-condition-and-json-format.patch around lines 276 to 291,
updating the MCP server calls updateMCPServer before awaiting
syncSettingsToServer, which can leave UI state changed while disk sync fails;
capture the current server state before calling updateMCPServer (or avoid
mutating local state until sync succeeds), attempt sync, and if sync fails
rollback by restoring the previous server state and show the error toast; on
success proceed to show success toast and close the dialog.
![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 introduces three critical bug fixes related to MCP server management. First, it enhances HTTP error handling in http-api-client.ts by adding response.ok validation to all HTTP methods (post, get, put, httpDelete), ensuring that network errors or non-2xx responses are properly caught and reported with meaningful messages instead of generic JSON parsing failures. Second, it resolves a race condition in use-mcp-servers.ts by making sure syncSettingsToServer() completes before displaying success toasts or proceeding with auto-tests, preventing 'Server not found' errors for newly added servers and adding error toasts for failed sync operations. Finally, it addresses a JSON format inconsistency in use-mcp-servers.ts by modifying the JSON editor to export MCP servers as an array that preserves server IDs, while also updating the import logic to support both the new array format and the legacy object format for backward compatibility, ensuring server persistence and correct identification after JSON edits.
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>
Shironex
added
Testers-Requested
|
heads up, you've made multiple pull requets that have patches and random .md files in them for your plan to implement. can you please just make a pull request with the code changes without all this other stuff? |
|
fixing in #337 |
4 participants
Fixes #315
Problem
HTTP methods in
HttpApiClientdidn't checkresponse.okbefore parsing JSON, causing cryptic "Unexpected end of JSON input" errors instead of showing actual server error messages.Example: When testing an invalid MCP server (404 response), users would see:
Solution
Added
response.okvalidation to all four HTTP methods with proper error handling:Applied to:
private async post<T>(endpoint: string, body?: unknown): Promise<T>private async get<T>(endpoint: string): Promise<T>private async put<T>(endpoint: string, body?: unknown): Promise<T>private async httpDelete<T>(endpoint: string): Promise<T>Impact
Testing
npm run buildpasses with no TypeScript errorsFiles Changed
apps/ui/src/lib/http-api-client.ts- Addedresponse.okchecks to all HTTP methodsRelated
This fix is a prerequisite for proper error visibility in MCP server workflows. See related issues:
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.