Improve error handling for Helm release API requests#522
Improve error handling for Helm release API requests#522Darshika11b wants to merge 1 commit intoheadlamp-k8s:mainfrom
Conversation
Signed-off-by: [Darshika11b] <[darshikab22@gmail.com]>
illume
left a comment
There was a problem hiding this comment.
Thanks for those changes.
Can you please check the git commit messages match the git commit guidelines in the contributing guide?
Please see open review comments?
There's an issue with the DCO GitHub check. Please have a look. git commit --amend -s
There was a problem hiding this comment.
Pull request overview
This pull request adds error handling to the Helm release API functions by introducing a safeRequest wrapper that validates responses and surfaces errors more explicitly. The goal is to prevent silent failures and improve the stability of the app catalog UI when Helm API requests fail.
Changes:
- Added a
safeRequestwrapper function that checks for empty/error responses and throws meaningful errors - Updated all 9 exported API functions to use
safeRequestinstead of directrequestcalls
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (response.error) { | ||
| throw new Error(response.error); | ||
| } |
There was a problem hiding this comment.
The check for response.error assumes errors are returned as part of the response object, but if an error occurs during the request, the request function will likely throw an exception or reject the promise rather than returning an object with an error property. This check may never be triggered. Consider verifying how the ApiProxy.request function reports errors before implementing this check, or remove it if errors are always thrown as exceptions.
| try { | ||
| const response = await request(url, options); | ||
|
|
||
| if (!response) { |
There was a problem hiding this comment.
The check for falsy response values (!response) will incorrectly throw an error for valid API responses that are falsy but legitimate, such as: empty arrays [], empty objects {}, the number 0, false booleans, or empty strings "". Consider using a more specific check like 'response === null || response === undefined' to only catch truly missing responses.
| if (!response) { | |
| if (response === null || response === undefined) { |
| } | ||
|
|
||
| return response; | ||
| } catch (error: any) { |
There was a problem hiding this comment.
The catch block wraps errors that are already Error instances, potentially creating nested error messages like "Error: Error: original message". If the original error from the request function is already an Error with a message property, this will create "Error: {original message}". If it's already been thrown as an Error, this creates double-wrapping. Consider checking if the caught error is already an Error instance and re-throwing it directly, or only wrapping non-Error exceptions.
| } catch (error: any) { | |
| } catch (error: any) { | |
| if (error instanceof Error) { | |
| throw error; | |
| } |
| /** | ||
| * Helper to ensure API errors are surfaced clearly | ||
| */ | ||
| async function safeRequest(url: string, options: any) { |
There was a problem hiding this comment.
Using 'any' type for the options parameter loses type safety. Consider using a more specific type such as RequestInit or defining an interface for the expected options structure. The ApiProxy.request function likely has a specific signature that should be matched here.
| async function safeRequest(url: string, options: any) { | |
| async function safeRequest(url: string, options: RequestInit) { |
| throw new Error('Empty response from Helm API'); | ||
| } | ||
|
|
||
| if (response.error) { | ||
| throw new Error(response.error); | ||
| } | ||
|
|
||
| return response; | ||
| } catch (error: any) { | ||
| throw new Error(error?.message || 'Helm API request failed'); |
There was a problem hiding this comment.
The safeRequest wrapper now throws errors for all API failures, but the consumers of these functions (e.g., List.tsx line 40-46 and Detail.tsx lines 49-51, 55-57) don't have .catch() handlers. This will cause unhandled promise rejections. While improving error handling in the API layer is good, it creates a breaking change that will cause runtime errors in existing code. Consider either: 1) adding .catch() handlers to all consumers as part of this PR, or 2) making the safeRequest wrapper handle errors more gracefully without throwing, such as returning an error object in the response.
| throw new Error('Empty response from Helm API'); | |
| } | |
| if (response.error) { | |
| throw new Error(response.error); | |
| } | |
| return response; | |
| } catch (error: any) { | |
| throw new Error(error?.message || 'Helm API request failed'); | |
| return { error: 'Empty response from Helm API' }; | |
| } | |
| // If the backend already returns an error field, surface it as-is | |
| if (response.error) { | |
| return response; | |
| } | |
| return response; | |
| } catch (error: any) { | |
| return { error: error?.message || 'Helm API request failed' }; |
Summary
Improves robustness of Helm release API calls by ensuring
errors are surfaced consistently and clearly.
What changed
Why
Helm API requests may fail due to network issues or backend errors.
Previously, such failures were not handled consistently, leading
to unstable behavior in the app catalog UI.
Testing