feat: improve error handling in HttpApiClient #337
Conversation
- Added error handling for HTTP responses in the HttpApiClient class. - Enhanced error messages to include status text and parsed error data, improving debugging and user feedback.
📝 WalkthroughWalkthroughThe HttpApiClient now implements consistent error handling across all HTTP request methods (POST, GET, PUT, DELETE). When a response fails, the code constructs informative error messages from status codes and response bodies, then throws standardized errors instead of returning raw responses. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 enhances the robustness of the application's HTTP communication by introducing standardized and informative error handling. It ensures that any unsuccessful API calls are caught and reported with detailed messages, which will greatly improve debugging capabilities and provide clearer feedback when issues arise. 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 introduces error handling for HTTP responses in HttpApiClient, which is a great improvement for robustness. However, the implementation introduces significant code duplication across the post, get, put, and httpDelete methods. My review focuses on refactoring this duplicated logic into a single helper method to improve maintainability. I've also identified a potential bug in the error handling logic that could cause a crash if an error response from the server is not in the expected format, and provided a fix for it.
| 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(); |
There was a problem hiding this comment.
This error handling logic is duplicated across post, get, put, and httpDelete methods. Additionally, there's a potential bug: if the error response body is valid JSON but not an object (e.g., null or a string), accessing errorData.error will throw a TypeError, masking the original HTTP error.
To fix this and improve maintainability, I recommend extracting the logic into a private helper method. You can add this method to the HttpApiClient class:
private async handleHttpResponse<T>(response: Response): Promise<T> {
if (!response.ok) {
let errorMessage = `HTTP ${response.status}: ${response.statusText}`;
try {
const errorData = await response.json();
// Use optional chaining to safely access the `error` property.
if (errorData?.error) {
errorMessage = errorData.error;
}
} catch {
// If parsing JSON fails, fall back to the status text.
}
throw new Error(errorMessage);
}
return response.json();
}Then, you can simplify this post method by using the new helper.
return this.handleHttpResponse(response);| 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(); |
There was a problem hiding this comment.
Please apply the same refactoring here as suggested for the post method. Using the handleHttpResponse helper will fix the potential bug and remove duplicated code.
return this.handleHttpResponse(response);| 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(); |
There was a problem hiding this comment.
This method also contains the duplicated error handling logic. Please refactor it to use the handleHttpResponse helper for consistency and to fix the underlying bug.
return this.handleHttpResponse(response);| 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(); |
There was a problem hiding this comment.
To complete the refactoring, please replace the duplicated error handling logic in this method with a call to the handleHttpResponse helper as well.
return this.handleHttpResponse(response);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/lib/http-api-client.ts (1)
573-584: Extract duplicate error handling into a shared helper method.The error handling logic is duplicated across all four HTTP methods (
post,get,put,httpDelete). Extracting this into a private helper method would improve maintainability and ensure consistent error handling.🔎 Suggested refactor
Add a private helper method:
+ 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 simplify each method:
private async post<T>(endpoint: string, body?: unknown): Promise<T> { await waitForApiKeyInit(); const response = await fetch(`${this.serverUrl}${endpoint}`, { method: 'POST', headers: this.getHeaders(), credentials: 'include', body: body ? JSON.stringify(body) : undefined, }); 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); } return response.json(); }Apply the same pattern to
get,put, andhttpDelete.Also applies to: 597-608, 623-634, 648-659
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/lib/http-api-client.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/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
⏰ 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 (1)
apps/ui/src/lib/http-api-client.ts (1)
573-584: Good addition of comprehensive error handling.The error handling implementation correctly extracts detailed error information from HTTP responses and provides informative error messages for debugging. The pattern of attempting to parse JSON error data while gracefully falling back to status text is sound.
Also applies to: 597-608, 623-634, 648-659
Fix: HTTP API client shows cryptic errors instead of server messages #315
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.