-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for zod error #1146
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update improves the SDK's error handling by integrating Zod error formatting. It enhances the Changes
🔗 Related PRs
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 6262c9f in 35 seconds
More details
- Looked at
69
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/src/sdk/utils/errors/src/formatter.ts:61
- Draft comment:
Consider providing a default value forvalidationErrors
to avoidJSON.stringify
onundefined
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
If validationErrors is undefined, JSON.stringify(undefined) returns 'undefined' as a string, which isn't necessarily wrong in this error handling context. The formattedErrors string will still work fine in the template literal. While providing a default value could be cleaner, this isn't causing any bugs or serious issues.
The comment points out a real code style improvement opportunity. JSON.stringify(undefined) is a bit sloppy even if it technically works.
However, this is an error handling utility where the exact format of the error message is not critical. The current code works fine and the suggested change would be purely cosmetic.
While technically correct, this comment suggests a minor code style improvement that doesn't materially impact functionality. Following our rules about only keeping strongly necessary comments, we should remove it.
Workflow ID: wflow_PaI4aUSywrYC4J0f
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -53,13 +54,25 @@ export const getAPIErrorDetails = ( | |||
} | |||
|
|||
switch (errorCode) { | |||
case COMPOSIO_SDK_ERROR_CODES.BACKEND.BAD_REQUEST: | |||
const validationErrors = axiosError.response?.data?.errors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding type validation for the validation errors to ensure type safety:
const isValidationError = (errors: unknown): errors is Record<string, unknown>[] | Record<string, unknown> => {
return errors !== null && typeof errors === 'object';
};
const validationErrors = axiosError.response?.data?.errors;
if (validationErrors && isValidationError(validationErrors)) {
// process the errors
}
This would help prevent runtime errors if the error structure is unexpected.
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
Code Review SummaryOverall AssessmentThe changes to add Zod error support are well-structured and maintain consistency with the existing error handling system. The implementation properly handles both array and single object validation error formats. Strengths✅ Good error handling structure with detailed error information Suggestions for Improvement
Code Quality Rating: 8/10The implementation is solid with good error handling patterns, but could benefit from the suggested improvements for better maintainability and type safety. The PR is ready to merge after addressing the suggested improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on c1fd196 in 23 seconds
More details
- Looked at
54
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/src/sdk/index.spec.ts:61
- Draft comment:
The test case still uses 'errors' instead of 'details'. Update the expected object to use 'details' to match the changes in the formatter.ts file. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_WN4ZzhMloAt6GukN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 6c60dd0 in 10 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/src/sdk/index.spec.ts:83
- Draft comment:
Ensure that the error description is correctly formatted as a JSON string, as intended by the Zod error formatting enhancement. Consider adding assertions to verify the JSON structure and content. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_M1w6svVwCrltpMp6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
🔍 Review Summary
Release Note
Purpose
Changes
Enhancement
formatter.ts
to format and display validation errors forBAD_REQUEST
errors, providing clearer error descriptions for debugging and request validation.Impact
Original Description