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: handle 400 error differently #1347

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

himanshu-dixit
Copy link
Collaborator

@himanshu-dixit himanshu-dixit commented Feb 19, 2025

Important

Improved handling of 400 errors by simplifying error messages and enhancing validation error descriptions in formatter.ts.

  • Behavior:
    • Simplified error message for COMPOSIO_SDK_ERROR_CODES.BACKEND.BAD_REQUEST in constants.ts.
    • In formatter.ts, getAPIErrorDetails() now handles 400 errors by checking for validation errors and using the response message if no validation errors are present.
    • Appends "Check .description for info." to the generic message for 400 errors.
  • Misc:
    • Minor refactoring in formatter.ts to improve error message construction logic.

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

Copy link

vercel bot commented Feb 19, 2025

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 Feb 19, 2025 0:02am


return {
message: genericMessage,
message:
genericMessage + "Check .description for info.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error message concatenation here lacks proper spacing. Consider using template literals and adding a space: ```typescript
message: ${genericMessage} Check description for info.


return {
message: genericMessage,
message:
genericMessage + "Check .description for info.",
description: `Validation Errors: ${formattedErrors}`,
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 a null/undefined check for formattedErrors before using it in the description string. If validationErrors is null/undefined, this could result in 'Validation Errors: undefined' being displayed.

@shreysingla11
Copy link
Collaborator

Code Review Summary

Overall Assessment

The changes improve error handling for 400 (Bad Request) responses, but there are a few areas that could use improvement:

Strengths

✅ Simplified error message structure
✅ Better handling of validation errors
✅ Maintains consistent error handling pattern

Areas for Improvement

  1. String concatenation could be improved using template literals
  2. Missing null/undefined checks for validation errors
  3. No type annotations for new variables
  4. No unit tests included for the changes

Recommendations

  1. Add proper null checks for validation errors
  2. Use template literals for string concatenation
  3. Add type annotations for new variables
  4. Consider adding unit tests for the new error handling logic

Code Quality Rating: 7/10

Good functionality but could use some defensive programming improvements and better TypeScript practices.

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. Reviewed everything up to 05678e4 in 1 minute and 36 seconds

More details
  • Looked at 35 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. js/src/sdk/utils/errors/src/constants.ts:35
  • Draft comment:
    The message for BAD_REQUEST was shortened, losing detail that previously specified the request was malformed. Confirm if this is intended.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is asking for confirmation of an intentional change, which violates our rule about not asking authors to confirm their intentions. The change appears to be a deliberate simplification, and the important details about malformed requests are still preserved in the possibleFix field. The shorter message is actually more consistent with other error messages in the file which are also concise.
    The shortened message could potentially make it harder for users to understand the error at first glance without reading the possibleFix field.
    The possibleFix field is the appropriate place for detailed guidance, and the change makes the error messages more consistent throughout the file. The change appears intentional and doesn't reduce the overall utility of the error handling.
    Delete this comment as it violates our rule about asking for confirmation of intentions, and the change appears to be a deliberate improvement in message consistency.
2. js/src/sdk/utils/errors/src/formatter.ts:59
  • Draft comment:
    Fallback for formattedErrors: if details isn’t an array and messageFromResponse is undefined, description could be 'Validation Errors: undefined'. Consider providing a more reliable fallback.
  • Reason this comment was not posted:
    Marked as duplicate.
3. js/src/sdk/utils/errors/src/constants.ts:35
  • Draft comment:
    The BAD_REQUEST message has been shortened. Confirm if removing the extra context ('malformed or incorrect') is intentional for backward compatibility.
  • Reason this comment was not posted:
    Marked as duplicate.
4. js/src/sdk/utils/errors/src/formatter.ts:66
  • Draft comment:
    Append a space before 'Check .description for info.' to avoid concatenating words without a space.
  • Reason this comment was not posted:
    Marked as duplicate.
5. js/src/sdk/utils/errors/src/formatter.ts:62
  • Draft comment:
    Consider a fallback if 'messageFromResponse' is undefined when 'validationErrors' is not an array, to prevent 'Validation Errors: undefined'.
  • Reason this comment was not posted:
    Marked as duplicate.
6. js/src/sdk/utils/errors/src/formatter.ts:66
  • Draft comment:
    The error message concatenates 'genericMessage' and "Check .description for info." without an explicit space. This may lead to a run-on message if 'genericMessage' does not already end with a space. Consider adding a leading space before 'Check .description for info.' or adjusting the punctuation for better readability.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_gORXcfRzvBQ55Llu


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.


return {
message: genericMessage,
message:
genericMessage + "Check .description for info.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Appending 'Check .description for info.' to genericMessage may lack proper spacing if genericMessage is empty or doesn’t end with a space.

Suggested change
genericMessage + "Check .description for info.",
genericMessage + (genericMessage ? ' ' : '') + "Check .description for info.",

Copy link

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-13412055872/coverage/index.html

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

@himanshu-dixit himanshu-dixit changed the title feat: handle 400 error different type feat: handle 400 error differently Feb 20, 2025
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.

2 participants