Skip to content
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 error message details #922

Merged
merged 12 commits into from
Nov 28, 2024
Merged

Conversation

himanshu-dixit
Copy link
Collaborator

@himanshu-dixit himanshu-dixit commented Nov 27, 2024

Important

Enhances error handling in CEG.handleError() with detailed messages for unknown backend errors, updates unauthorized error handling, and improves logging.

  • Behavior:
    • Adds detailed error messages for unknown backend errors in CEG.handleError() using axiosError data.
    • Provides a default possible fix for unknown errors: "Please check the network connection, request parameters, and ensure the API endpoint is correct."
  • Error Handling:
    • Updates ERROR.BACKEND.UNAUTHORIZED in CEG.handleError() to use ERROR.BACKEND.UNAUTHORIZED instead of ERROR.COMMON.API_KEY_UNAVAILABLE.
  • Logging:
    • Adds x-request-id to response logging in setAxiosClientConfig() in config.ts.
  • Misc:
    • Adds eslint script to package.json.
    • Removes unused imports in index.ts.

This description was created by Ellipsis for 0fe5285. It will automatically update as commits are pushed.

Copy link

vercel bot commented Nov 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2024 9:05am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 1842dea in 17 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/sdk/utils/error.ts:114
  • Draft comment:
    The error key for a 401 status should be ERROR.BACKEND.UNAUTHORIZED instead of ERROR.COMMON.API_KEY_UNAVAILABLE to match the predefined error registry.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_g2FTPran21xIDAKz


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

}else{
errorDetails = {
message: axiosError.message,
description: axiosError.response.data.message || axiosError.response.data.error || axiosError.message,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding null/undefined checks for axiosError.response and axiosError.response.data to prevent potential runtime errors. You could use optional chaining:

description: axiosError.response?.data?.message || 
             axiosError.response?.data?.error || 
             axiosError.message

@@ -111,8 +111,14 @@ export class CEG {
errorKey = ERROR.BACKEND.UNKNOWN;
break;
}
if (errorKey) {
if (errorKey !== ERROR.BACKEND.UNKNOWN) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition errorKey !== ERROR.BACKEND.UNKNOWN might be more clear as errorKey && errorKey !== ERROR.BACKEND.UNKNOWN to explicitly handle cases where errorKey might be undefined or null.

@shreysingla11
Copy link
Collaborator

Code Review Summary

Overall Assessment

The changes improve error handling by providing more detailed information for unknown backend errors, which is a valuable addition. The implementation is generally sound but could benefit from some minor improvements in robustness and error handling.

Strengths

✅ Adds more detailed error information for unknown errors
✅ Maintains consistency with existing error handling patterns
✅ Provides useful debugging information to users

Suggestions for Improvement

  1. Add null/undefined checks for response objects
  2. Consider more explicit condition checking for errorKey
  3. Consider extracting the error details object into a separate constant or method for better maintainability

Code Quality Rating: 8/10

Good implementation with room for minor improvements in robustness and safety.

The changes are ready to be merged after addressing the suggested improvements for safer error handling.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 a582e6b in 12 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/sdk/utils/error.ts:105
  • Draft comment:
    The change from ERROR.COMMON.API_KEY_UNAVAILABLE to ERROR.BACKEND.UNAUTHORIZED is consistent with the error definitions and improves clarity in error handling.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change from ERROR.COMMON.API_KEY_UNAVAILABLE to ERROR.BACKEND.UNAUTHORIZED is consistent with the error handling logic and aligns with the error definitions in the ERROR object.

Workflow ID: wflow_ZykxLKer1LaVTcsz


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Nov 27, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12065699873/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12065699873/html-report/report.html

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on aa8f2be in 59 seconds

More details
  • Looked at 80 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_OOF07ozxxEVwDGNM


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

errorDetails = PREDEFINED_ERROR_REGISTRY[errorKey];
}if(errorKey === ERROR.BACKEND.BAD_REQUEST){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logical error: The errorDetails for BAD_REQUEST are being overwritten with UNKNOWN error details. Use PREDEFINED_ERROR_REGISTRY[ERROR.BACKEND.BAD_REQUEST] instead.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 0fe5285 in 2 minutes and 20 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_suZ6Vcsav277clKz


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants