feat: better error, handle 400 errors other format #928
Conversation
…e-other-400-errors
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 0fc25c1 in 1 minute and 4 seconds
More details
- Looked at
523lines of code in8files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_FnWrofcFnZpZuijr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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: |
…io into ft-handle-other-400-errors
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 25d68c5 in 31 seconds
More details
- Looked at
30lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/backendClient.spec.ts:22
- Draft comment:
The error message in the test does not match the error message in the constants file. It should be updated to match the message inconstants.ts. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems speculative as it assumes the error message should match a constant from another file, but there is no evidence in the diff to support this. The diff only shows a change in the error message string, and there is no indication that it should match a constant. Without strong evidence, the comment should be removed.
I might be missing context from other parts of the codebase that are not shown in the diff. The constants file might indeed have a relevant error message, but this is not evident from the diff provided.
Given the rules, I should only consider the diff provided. Without evidence in the diff, I should not assume the existence of a relevant constant in another file.
The comment should be deleted as it is speculative and not supported by evidence in the diff.
Workflow ID: wflow_6B7kVmBsYahq2864
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dfed375 in 1 minute and 23 seconds
More details
- Looked at
724lines of code in17files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_ON9yTHaurRs6oAs4
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.
js/src/utils/external.ts
Outdated
| const child = spawn('node', ['-e', ` | ||
| const axios = require('axios'); | ||
| // Send a POST request to the telemetry server with the reporting payload | ||
| axios.post('${info.url}', { |
There was a problem hiding this comment.
The headers should be passed as a second argument to the axios.post method, not inside the data object.
| axios.post('${info.url}', { | |
| axios.post('${info.url}', ${JSON.stringify(info.reportingPayload)}, { headers: { 'Content-Type': 'application/json' } }); |
js/src/utils/external.ts
Outdated
| reportingPayload: Record<string, unknown> | ||
| }) { | ||
| // Create a new XMLHttpRequest object | ||
| const xhr = new XMLHttpRequest(); |
There was a problem hiding this comment.
Consider adding error handling for network errors and timeouts in the sendBrowserReq function to ensure robustness.
js/src/sdk/utils/errors/src/base.ts
Outdated
| } | ||
|
|
||
| // Wait for logError to complete before continuing | ||
| logError({ |
There was a problem hiding this comment.
Consider awaiting the logError call or handling the promise to avoid unhandled promise rejections.
| } | ||
| } | ||
|
|
||
| processBatch() { |
There was a problem hiding this comment.
Consider adding error handling within the processBatch method to catch and handle any errors that occur during the execution of processBatchCallback.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 516e38e in 15 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/utils/error.ts:61
- Draft comment:
The cast tounknownis unnecessary when casting toAxiosError. You can directly castaxiosErrortoAxiosError. - Reason this comment was not posted:
Confidence changes required:50%
The cast tounknownis unnecessary when casting toAxiosError.
Workflow ID: wflow_7OyhcCc9g1avW562
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…mposio into ft-handle-other-400-errors
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e601a60 in 12 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/backendClient.spec.ts:22
- Draft comment:
Typo in the test description: 'should throw and error' should be 'should throw an error'. - Reason this comment was not posted:
Confidence changes required:10%
The error message in the test has a typo in the word 'and'. It should be 'an'.
Workflow ID: wflow_BNv9QRFmMwWe6KR5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 624d764 in 47 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/utils/common.ts:3
- Draft comment:
Consider checking fornulland arrays separately to avoid unintended stringification.
return obj !== null && typeof obj === 'object' && !Array.isArray(obj) ? JSON.stringify(obj) : obj;
- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_dB5r9ol1cbzq540r
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -60,7 +60,7 @@ export class Triggers { | |||
| }); | |||
| return response.data as {status: string, triggerId: string}; | |||
| } catch (error) { | |||
| throw CEG.handleError(error); | |||
| throw CEG.handleAllError(error); | |||
There was a problem hiding this comment.
The code contains a bug where throw CEG.handleAllError(error); is used in multiple catch blocks. However, CEG.handleAllError(error) itself throws an exception internally, which means that the throw statement attempts to throw undefined. This leads to runtime errors or unintended behavior, as the original error may be lost or result in unhandled exceptions. The throw keyword should be removed from these calls to allow CEG.handleAllError(error); to execute correctly. Additionally, within the handleAllError method, if isResponseNotPresent is true, the call to handleNonResponseAxiosError(error) always throws an error, making any subsequent code unreachable. This indicates a logical error that could lead to unexpected behavior.
Please check all the places.
There was a problem hiding this comment.
Instead throw and throw. Added CEG.handleAllError(error) with argument shouldThrow, otherwise it returns error object.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8ad6673 in 16 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/package.json:17
- Draft comment:
Consider using double quotes for consistency with other scripts.
"prettier": "prettier --write \"src/**/*.ts\"",
- Reason this comment was not posted:
Confidence changes required:50%
The change in the lint and prettier scripts to include quotes around the file paths is a good practice to handle paths with spaces. However, the change in the prettier script is not consistent with the rest of the scripts, which use double quotes. It's better to maintain consistency across the scripts.
Workflow ID: wflow_PXV6G6gTKRX9sIwl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…mposio into ft-handle-other-400-errors
P.S. - Need to be tested with cases before merging.
logger.tsby settinghandleExceptionsandhandleRejectionstofalse.Reverted to original error throwing using
CEG.throwCustomError()inbackendClient.tsandindex.ts.Improved error format by refactoring
CEGinceg.tsand introducinggetBackendErrorResponseFormat()inbackendErrorResponse.ts.constants.ts.SERVER_UNAVAILABLEinconstants.ts.telemetry/index.tsandtelemetry/events.ts.BatchProcessorinbase/batchProcessor.tsfor batching telemetry events.package.jsonversion to0.3.0-beta.2.Added
eslint.config.mjsandprettier.config.mjsfor code formatting.Updated
typedoc.jsonto exclude test files.This description was created by
<https://www.ellipsis.dev?ref=ComposioHQ%2Fcomposio&utm_source=github&utm_medium=referral%7Chttps://img.shields.io/badge/Ellipsis-blue?color=175173%7CEllipsis><sup> for b7522dd. It will automatically update as commits are pushed.
Important
Improves error handling and formatting, adds telemetry logging, and updates configuration for code formatting.
logger.tsby settinghandleExceptionsandhandleRejectionstofalse.CEG.throwCustomError()inbackendClient.tsandindex.ts.CEGinceg.tsand introducinggetBackendErrorResponseFormat()inbackendErrorResponse.ts.constants.ts.SERVER_UNAVAILABLEinconstants.ts.telemetry/index.tsandtelemetry/events.ts.BatchProcessorinbase/batchProcessor.tsfor batching telemetry events.package.jsonversion to0.3.0-beta.2.eslint.config.mjsandprettier.config.mjsfor code formatting.typedoc.jsonto exclude test files.This description was created by
for 8ad6673. It will automatically update as commits are pushed.