feat: create new axios instance for each composio instance #1379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
utkarsh-dixit
requested review from
himanshu-dixit and
plxity
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
himanshu-dixit
approved these changes
Feb 27, 2025
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
Comment on lines
+14
to
+17
| connectedAccouns = new ConnectedAccounts( | ||
| backendClient, | ||
| backendClient.instance | ||
| ); |
There was a problem hiding this comment.
Variable connectedAccouns is misspelled (missing 't'). Should be connectedAccounts. This will cause issues with variable references.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| connectedAccouns = new ConnectedAccounts( | |
| backendClient, | |
| backendClient.instance | |
| ); | |
| connectedAccounts = new ConnectedAccounts( | |
| backendClient, | |
| backendClient.instance | |
| ); |
Comment on lines
+20
to
+28
| _connectedAccounts = new ConnectedAccounts( | ||
| backendClient, | ||
| backendClient.instance | ||
| ); | ||
| _entity = new Entity(backendClient, "default"); | ||
| _connectedAccounts = new ConnectedAccounts(backendClient); | ||
| _actions = new Actions(backendClient); | ||
| _connectedAccounts = new ConnectedAccounts( | ||
| backendClient, | ||
| backendClient.instance | ||
| ); |
There was a problem hiding this comment.
Duplicate initialization of _connectedAccounts variable within beforeAll() block. Second initialization is redundant and should be removed.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| _connectedAccounts = new ConnectedAccounts( | |
| backendClient, | |
| backendClient.instance | |
| ); | |
| _entity = new Entity(backendClient, "default"); | |
| _connectedAccounts = new ConnectedAccounts(backendClient); | |
| _actions = new Actions(backendClient); | |
| _connectedAccounts = new ConnectedAccounts( | |
| backendClient, | |
| backendClient.instance | |
| ); | |
| _connectedAccounts = new ConnectedAccounts( | |
| backendClient, | |
| backendClient.instance | |
| ); | |
| _entity = new Entity(backendClient, "default"); |
js/src/sdk/models/actions.spec.ts
Outdated
| actions = new Actions(backendClient); | ||
| connectedAccouns = new ConnectedAccounts(backendClient); | ||
| actions = new Actions(backendClient, backendClient.instance); | ||
| connectedAccouns = new ConnectedAccounts(backendClient, backendClient.instance); |
There was a problem hiding this comment.
Variable connectedAccouns is misspelled (should be connectedAccounts). This typo could cause confusion and maintenance issues.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| connectedAccouns = new ConnectedAccounts(backendClient, backendClient.instance); | |
| connectedAccounts = new ConnectedAccounts(backendClient, backendClient.instance); |
Comment on lines
102
to
106
| await apiClient.triggers.switchTriggerInstanceStatus({ | ||
| client: this.client, | ||
| path: { triggerId: parsedData.triggerId }, | ||
| body: { | ||
| enabled: true, |
There was a problem hiding this comment.
Inconsistent path parameter usage in disable() vs enable() - disable() passes full parsedData object while enable() extracts triggerId, which could cause runtime errors
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| await apiClient.triggers.switchTriggerInstanceStatus({ | |
| client: this.client, | |
| path: { triggerId: parsedData.triggerId }, | |
| body: { | |
| enabled: true, | |
| await apiClient.triggers.switchTriggerInstanceStatus({ | |
| client: this.client, | |
| path: { triggerId: parsedData.triggerId }, | |
| body: { | |
| enabled: true, |
js/src/sdk/models/backendClient.ts
Outdated
Comment on lines
41
to
48
| this.instance = createClient(createConfig({ | ||
| baseURL: this.baseUrl, | ||
| headers: { | ||
| "X-API-KEY": `${this.apiKey}`, | ||
| "X-SOURCE": "js_sdk", | ||
| "X-RUNTIME": this.runtime, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
The throwOnError configuration is missing in the new client setup which could change error handling behavior. Add throwOnError: true to maintain consistent error handling.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| this.instance = createClient(createConfig({ | |
| baseURL: this.baseUrl, | |
| headers: { | |
| "X-API-KEY": `${this.apiKey}`, | |
| "X-SOURCE": "js_sdk", | |
| "X-RUNTIME": this.runtime, | |
| }, | |
| })); | |
| this.instance = createClient(createConfig({ | |
| baseURL: this.baseUrl, | |
| headers: { | |
| "X-API-KEY": `${this.apiKey}`, | |
| "X-SOURCE": "js_sdk", | |
| "X-RUNTIME": this.runtime, | |
| }, | |
| throwOnError: true, | |
| })); |
js/src/sdk/models/backendClient.ts
Outdated
Comment on lines
4
to
8
| import { setAxiosClientConfig } from "../utils/config"; | ||
| import { CEG } from "../utils/error"; | ||
| import { COMPOSIO_SDK_ERROR_CODES } from "../utils/errors/src/constants"; | ||
| import { removeTrailingSlashIfExists } from "../utils/string"; | ||
| import { Client, createClient, createConfig } from "@hey-api/client-axios"; |
There was a problem hiding this comment.
The setAxiosClientConfig utility function is imported but never used after refactoring, which may skip important axios configuration. Either remove the import or apply the configuration to the new client.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| import { setAxiosClientConfig } from "../utils/config"; | |
| import { CEG } from "../utils/error"; | |
| import { COMPOSIO_SDK_ERROR_CODES } from "../utils/errors/src/constants"; | |
| import { removeTrailingSlashIfExists } from "../utils/string"; | |
| import { Client, createClient, createConfig } from "@hey-api/client-axios"; | |
| import { CEG } from "../utils/error"; | |
| import { COMPOSIO_SDK_ERROR_CODES } from "../utils/errors/src/constants"; | |
| import { removeTrailingSlashIfExists } from "../utils/string"; | |
| import { Client, createClient, createConfig } from "@hey-api/client-axios"; |
Comment on lines
223
to
227
| if (!uniqueKey) { | ||
| const apps = await apiClient.apps.getApps(); | ||
| const apps = await apiClient.apps.getApps({ client: this.client }); | ||
| const app = apps.data?.items.find((app) => app.appId === data.appId); | ||
| uniqueKey = app!.key; | ||
| throw CEG.getCustomError( |
There was a problem hiding this comment.
The getOrCreateIntegration method throws an error when uniqueKey is found instead of continuing execution, preventing the intended get-or-create functionality
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| if (!uniqueKey) { | |
| const apps = await apiClient.apps.getApps(); | |
| const apps = await apiClient.apps.getApps({ client: this.client }); | |
| const app = apps.data?.items.find((app) => app.appId === data.appId); | |
| uniqueKey = app!.key; | |
| throw CEG.getCustomError( | |
| if (!uniqueKey) { | |
| const apps = await apiClient.apps.getApps({ client: this.client }); | |
| const app = apps.data?.items.find((app) => app.appId === data.appId); | |
| uniqueKey = app!.key; | |
| if (!uniqueKey) throw CEG.getCustomError( |
Comment on lines
+64
to
69
| private client: Client; | ||
|
|
||
| constructor(backendClient: AxiosBackendClient, client: Client) { | ||
| this.backendClient = backendClient; | ||
| this.client = client; | ||
| } |
There was a problem hiding this comment.
Constructor parameter client is added but not validated for null/undefined which could cause runtime errors when accessing this.client in methods
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| private client: Client; | |
| constructor(backendClient: AxiosBackendClient, client: Client) { | |
| this.backendClient = backendClient; | |
| this.client = client; | |
| } | |
| private client: Client; | |
| constructor(backendClient: AxiosBackendClient, client: Client) { | |
| if (!backendClient) throw new Error('backendClient is required'); | |
| if (!client) throw new Error('client is required'); | |
| this.backendClient = backendClient; | |
| this.client = client; | |
| } |
![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! Incremental review on 7751b51 in 1 minute and 30 seconds
More details
- Looked at
336lines of code in14files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. js/src/sdk/models/integrations.ts:220
- Draft comment:
In getOrCreateIntegration, the code always throws after setting 'uniqueKey'. This likely should only throw if 'uniqueKey' is still undefined after attempting to retrieve it. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. js/src/sdk/models/backendClient.ts:34
- Draft comment:
Consider validating the API key before creating the axios client instance for clarity and potential performance benefits. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. js/src/sdk/models/integrations.ts:220
- Draft comment:
Bug: In getOrCreateIntegration, after deriving uniqueKey from the app lookup, the code unconditionally throws an error. It should only throw if uniqueKey remains falsy. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. js/src/sdk/models/backendClient.ts:48
- Draft comment:
Consider checking for a valid API key before creating the axios client instance to avoid instantiating a client with an empty key. - Reason this comment was not posted:
Comment was on unchanged code.
5. js/src/sdk/models/triggers.ts:217
- Draft comment:
Avoid using 'as unknown as SingleTriggerRes' for type assertion; consider a safer type check or proper Zod validation to ensure type safety. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. js/src/sdk/models/connectedAccounts.spec.ts:18
- Draft comment:
Avoid using '@ts-ignore' in tests; consider defining proper types for test data to maintain type safety. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. js/src/sdk/index.spec.ts:176
- Draft comment:
The expected error description string "er is currently unable to handle the reque" appears to be truncated. It looks like it might be a typographical error. Consider updating it to something like "...handle the request" if that was the intended message. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. js/src/sdk/models/Entity.spec.ts:75
- Draft comment:
Typo found: 'successfull' should probably be 'successful' to maintain consistent spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. js/src/sdk/models/actions.spec.ts:9
- Draft comment:
Typographical Error: The variable name 'connectedAccouns' appears to be mistyped. Consider renaming it to 'connectedAccounts' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. js/src/sdk/models/actions.spec.ts:58
- Draft comment:
Typographical Error: The property name 'successfull' seems to be misspelled. It is recommended to correct it to 'successful'. This issue appears at both line 58 and line 74. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. js/src/sdk/models/actions.ts:141
- Draft comment:
In the comment for the execute method, it says 'This doesn't execute the local action and is wrapper over backend.' Consider changing it to '... and is a wrapper over the backend.' This small adjustment makes the sentence read more naturally. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. js/src/sdk/models/actions.ts:114
- Draft comment:
The comment currently uses 'Todo:' at line 114. Consider updating it to 'TODO:' to match common coding conventions for marking tasks or reminders. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_gxadc0baUVDgWw4M
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: |
js/src/sdk/models/backendClient.ts
Outdated
| @@ -37,8 +38,14 @@ | |||
| this.runtime = runtime || ""; | |||
| this.apiKey = apiKey; | |||
| this.baseUrl = removeTrailingSlashIfExists(baseUrl); | |||
| this.instance = axiosClient.instance; | |||
|
|
|||
| this.instance = createClient(createConfig({ | |||
There was a problem hiding this comment.
Consider adding error handling for client creation:
this.instance = createClient(createConfig({
baseURL: this.baseUrl,
headers: {
"X-API-KEY": `${this.apiKey}`,
"X-SOURCE": "js_sdk",
"X-RUNTIME": this.runtime,
},
})).catch(error => {
throw CEG.getCustomError(
COMPOSIO_SDK_ERROR_CODES.COMMON.CLIENT_INITIALIZATION_FAILED,
`Failed to initialize axios client: ${error.message}`
);
});
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9b9a8b0 in 3 minutes and 17 seconds
More details
- Looked at
790lines of code in17files - Skipped
1files when reviewing. - Skipped posting
19drafted comments based on config settings.
1. js/src/sdk/models/integrations.ts:221
- Draft comment:
In getOrCreateIntegration, after retrieving the app key (uniqueKey), an error is unconditionally thrown. If uniqueKey is retrieved from app data, avoid throwing an error. - Reason this comment was not posted:
Comment was on unchanged code.
2. js/src/sdk/models/triggers.ts:179
- Draft comment:
The comment 'Bad type inference' suggests that type coercion is hiding issues. Consider revising the types or improving the zod schema to avoid using 'as unknown as ...'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. js/src/sdk/models/triggers.ts:393
- Draft comment:
In the subscribe method, ensure errors from PusherUtils.getPusherClient and subscription logic are properly handled to improve stability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. js/src/sdk/models/integrations.ts:172
- Draft comment:
When retrieving the app key in create and getOrCreateIntegration, confirm if throwing an error immediately after assignment is intended. Consider throwing only if no key is found. - Reason this comment was not posted:
Comment was on unchanged code.
5. js/src/sdk/models/triggers.spec.ts:112
- Draft comment:
Test 'should throw an error if trigger not found' expects error.message to contain 'Trigger not found'. This assumption may be too strict; consider checking an error code or broader message. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. js/src/sdk/models/integrations.ts:222
- Draft comment:
BUG: In getOrCreateIntegration, the block throws an error even after retrieving a uniqueKey. Remove the unconditional throw to allow successful creation when a key is found. - Reason this comment was not posted:
Comment was on unchanged code.
7. js/src/sdk/models/backendClient.ts:90
- Draft comment:
Consider verifying that returning 'this.instance.instance' correctly exposes the underlying axios instance as intended. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. js/src/sdk/models/triggers.ts:217
- Draft comment:
Improvement: Avoid using 'as unknown as SingleTriggerRes'. Update type definitions for getTriggerInfo to improve type safety. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. js/src/sdk/index.spec.ts:172
- Draft comment:
It looks like the error description contains an incomplete string "er is currently unable to handle the reque". You might want to check if this is a typo or if the intended error message got cut off. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. js/src/sdk/index.ts:182
- Draft comment:
Consider revising the error message on line 182 to replace 'None' with 'null' or 'undefined' for consistency with JavaScript conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. js/src/sdk/models/Entity.spec.ts:72
- Draft comment:
Typo: The property 'successfull' appears to be misspelled. Consider renaming it to 'successful' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. js/src/sdk/models/actions.spec.ts:9
- Draft comment:
Typographical error: the variable 'connectedAccouns' on line 9 (and its usage on line 14) appears to be misspelled. It should likely be 'connectedAccounts' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. js/src/sdk/models/actions.spec.ts:55
- Draft comment:
Typographical error: the property name 'successfull' on line 55 (and similarly on line 71) might be a typo. Consider using 'successful' if that accurately reflects the intended property. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. js/src/sdk/models/actions.ts:143
- Draft comment:
Typographical suggestion: In the comment for the 'execute' method, consider changing 'is wrapper over backend' to 'is a wrapper over the backend' for improved clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. js/src/sdk/models/actions.ts:115
- Draft comment:
Typographical suggestion: Change 'Todo:' to 'TODO:' in the comment for consistency with common conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. js/src/sdk/models/connectedAccounts.ts:297
- Draft comment:
Minor naming inconsistency: In the ConnectionRequest constructor, the parameter 'redirectUri' is used but the instance property is named 'redirectUrl'. Consider using consistent naming (either 'redirectUri' everywhere or 'redirectUrl' everywhere) to reduce potential confusion. - Reason this comment was not posted:
Comment was on unchanged code.
17. js/src/sdk/models/integrations.ts:97
- Draft comment:
Typo in the JSDoc for the get method: update the parameter type from{IntegrationGetParam}to{IntegrationGetRequiredParam}and modify the description to clarify that an integration ID is expected, not an integration name. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. js/src/sdk/models/integrations.ts:157
- Draft comment:
Typo in the JSDoc for the create method: change the return type documentation fromIntegrationGetResponsetoIntegrationGetResto match the actual type returned. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. js/src/sdk/models/integrations.ts:264
- Draft comment:
Typo in the JSDoc for the delete method: update the parameter type fromIntegrationListDatatoIntegrationDeleteParamand adjust the return type annotation to reflectIntegrationDeleteRes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_zI0WnALzbvfAZxZR
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.
| getAxiosInstance() { | ||
| return axiosClient.instance; | ||
| return this.instance.instance; |
There was a problem hiding this comment.
Returning this.instance.instance is a bit confusing. Consider renaming the inner property or adding a comment to clarify why a double instance is needed.
| @@ -24,7 +25,7 @@ | |||
| * The runtime environment where the client is being used. | |||
| */ | |||
| public runtime: string; | |||
| public instance: AxiosInstance; | |||
| public instance: Client; | |||
There was a problem hiding this comment.
Consider adding JSDoc for the instance property to improve code documentation:
/**
* The axios client instance specific to this backend client.
* Each Composio instance gets its own axios instance to prevent
* configuration conflicts between multiple SDK instances.
*/
public instance: Client;| getAxiosInstance() { | ||
| return axiosClient.instance; | ||
| return this.instance.instance; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a cleanup method to properly dispose axios instances when no longer needed:
public dispose(): void {
if (this.instance) {
// Cancel any pending requests
this.instance.cancelAllRequests();
// Clear any internal state
this.instance = null;
}
}
|
I've reviewed the changes and here's my assessment: Positive Aspects:
Suggestions for Improvement:
Overall Assessment: Rating: 8/10 - Good quality changes with room for minor improvements in error handling and documentation. |
js/src/sdk/models/backendClient.ts
Outdated
Comment on lines
39
to
46
| this.instance = createClient(createConfig({ | ||
| baseURL: this.baseUrl, | ||
| headers: { | ||
| "X-API-KEY": `${this.apiKey}`, | ||
| "X-SOURCE": "js_sdk", | ||
| "X-RUNTIME": this.runtime, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Redundant configuration setup - headers and baseURL are set twice (in constructor and initializeApiClient), which is inefficient and could cause issues if configs diverge. Remove config from constructor.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| this.instance = createClient(createConfig({ | |
| baseURL: this.baseUrl, | |
| headers: { | |
| "X-API-KEY": `${this.apiKey}`, | |
| "X-SOURCE": "js_sdk", | |
| "X-RUNTIME": this.runtime, | |
| }, | |
| })); | |
| this.instance = createClient(createConfig({ | |
| baseURL: this.baseUrl, | |
| headers: { | |
| "X-API-KEY": `${this.apiKey}`, | |
| "X-SOURCE": "js_sdk", | |
| "X-RUNTIME": this.runtime, | |
| }, | |
| })); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9a6ec0d in 1 minute and 41 seconds
More details
- Looked at
71lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. js/src/sdk/models/backendClient.ts:6
- Draft comment:
Ensure imported setAxiosClientConfig is used appropriately; consider adding a brief inline comment to explain its purpose if its behavior is non-obvious. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. js/src/sdk/models/backendClient.ts:103
- Draft comment:
Using this.instance.instance for setAxiosClientConfig is less intuitive; consider renaming properties or adding a comment to clarify the structure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code is using a wrapper Client class that contains an Axios instance. While this.instance.instance might look odd, it's likely coming from the @hey-api/client-axios library's API design. Renaming these properties would require changes to the library or creating confusing local aliases. The structure seems clear enough given the types and context.
The comment raises a valid point about code readability, and nested .instance properties could be confusing to new developers.
However, this naming likely comes from the external library's design, and changing it locally would create more confusion or require upstream changes. The types make the structure clear enough.
Delete the comment as it suggests changes that would be difficult to implement and the current structure, while not perfect, is clear enough given the types and context.
3. js/src/sdk/models/backendClient.ts:39
- Draft comment:
Validate apiKey and baseUrl before creating the axios instance to avoid unnecessary instance creation if inputs are invalid. - Reason this comment was not posted:
Comment was on unchanged code.
4. js/src/sdk/models/backendClient.ts:92
- Draft comment:
The configuration is applied twice: once in createClient and again in initializeApiClient. Consider consolidating or clearly documenting why both steps are needed to avoid duplication. - Reason this comment was not posted:
Comment was on unchanged code.
5. js/src/sdk/models/backendClient.ts:107
- Draft comment:
The use of 'this.instance.instance' may be confusing; consider renaming one of them for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. js/src/sdk/models/backendClient.ts:29
- Draft comment:
There's a minor typographical error in the JSDoc comment for the constructor. The comment says 'Creates an instance of apiClientDetails' but the class name is 'AxiosBackendClient'. Consider updating the comment for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_EiBmWEP36Y6lZFOA
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.
| /** | ||
| * Initializes the API client with the provided configuration. | ||
| * @private | ||
| */ | ||
| private initializeApiClient() { | ||
| axiosClient.setConfig({ | ||
| this.instance.setConfig({ |
There was a problem hiding this comment.
The initializeApiClient method duplicates header and baseURL config from the constructor. Consider refactoring to avoid repetition.
js/src/sdk/models/backendClient.ts
Outdated
Comment on lines
95
to
107
| // this.instance.setConfig({ | ||
| // baseURL: removeTrailingSlashIfExists(this.baseUrl), | ||
| // headers: { | ||
| // common: { | ||
| // "X-API-KEY": `${this.apiKey}`, | ||
| // "X-SOURCE": "js_sdk", | ||
| // "X-RUNTIME": this.runtime, | ||
| // } | ||
| // }, | ||
| // throwOnError: true, | ||
| // }); | ||
|
|
||
| setAxiosClientConfig(axiosClient.instance); | ||
| this.instance = axiosClient.instance; | ||
| // setAxiosClientConfig(this.instance.instance); |
There was a problem hiding this comment.
Commenting out initializeApiClient() implementation and setAxiosClientConfig() call breaks API client configuration and header setup. This will cause API requests to fail.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ee70499 in 1 minute and 37 seconds
More details
- Looked at
66lines of code in2files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. js/src/sdk/index.spec.ts:58
- Draft comment:
Ensure axios defaults are correctly set. This change relies on axios instance exporting headers at the top level instead of under 'common'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that axios defaults are correctly set, which is a form of asking the author to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested. The comment does not provide a specific suggestion or point out a specific issue with the code.
2. js/src/sdk/models/backendClient.ts:42
- Draft comment:
Remove or clarify commented-out 'common' wrapper in headers if it's not intended for future use. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Commented code can be a code smell and make maintenance harder. However, this appears to be an active refactoring where the author is deliberately keeping the commented code temporarily. The comment doesn't suggest a clear action - "remove or clarify" is not specific enough. The author likely has a reason for keeping this commented code during the refactoring.
The commented code might be important documentation of the API structure or might be needed for a future change. I might be too quick to dismiss it.
While documentation is important, proper documentation should be in comments explaining the code, not in commented-out code. If the code is needed later, it's in version control.
The comment should be deleted as it doesn't provide clear actionable feedback and the commented code appears to be part of an intentional refactoring process.
3. js/src/sdk/index.spec.ts:58
- Draft comment:
Ensure that accessing headers directly (defaults.headers['X-API-KEY']) is consistent with how the axios instance is configured. Confirm this change works across all header setups. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. js/src/sdk/models/backendClient.ts:42
- Draft comment:
Remove the commented 'common' header wrapper if it's no longer needed to clarify the header configuration. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. js/src/sdk/models/backendClient.ts:95
- Draft comment:
If the initializeApiClient method is no longer used, remove its commented-out configuration (and related setAxiosClientConfig call) to reduce code clutter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. js/src/sdk/index.spec.ts:176
- Draft comment:
There appears to be a typographical error in the expected error description string. The substring "er is currently unable to handle the reque" looks truncated and potentially intended to be "server is currently unable to handle the request". Please verify and correct if needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. js/src/sdk/models/backendClient.ts:29
- Draft comment:
In the constructor documentation (line 29), the comment mentions 'apiClientDetails' instead of 'AxiosBackendClient'. Please update this documentation to match the class name for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_rAD5alPeAPHM7EYw
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/sdk/models/backendClient.ts
Outdated
| }, | ||
| throwOnError: true, | ||
| }); | ||
| // this.instance.setConfig({ |
There was a problem hiding this comment.
Remove commented-out initialization code if it's no longer needed.
Comment on lines
278
to
282
| params = await processor({ | ||
| params: params, | ||
| actionName: action, | ||
| client: this.client.backendClient.instance, | ||
| }); |
There was a problem hiding this comment.
Potential runtime error if this.client or this.client.backendClient is undefined/null. Add null checks before accessing instance property.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| params = await processor({ | |
| params: params, | |
| actionName: action, | |
| client: this.client.backendClient.instance, | |
| }); | |
| params = await processor({ | |
| params: params, | |
| actionName: action, | |
| client: this.client?.backendClient?.instance, | |
| }); |
js/src/sdk/index.spec.ts
Outdated
Comment on lines
73
to
84
| name: "AppNotFoundError", | ||
| message: "Not found", | ||
| }; | ||
| mock.onGet(/.*\/api\/v1\/apps/).reply(404, mockError); | ||
|
|
||
| const client = new Composio({ | ||
| apiKey: COMPOSIO_API_KEY, | ||
| baseUrl: getTestConfig().BACKEND_HERMES_URL, | ||
| }); | ||
|
|
||
| const mock = new AxiosMockAdapter(client.backendClient.getAxiosInstance()); | ||
|
|
||
| mock.onGet(/.*\/api\/v1\/apps/).reply(404, mockError); |
There was a problem hiding this comment.
Mock adapter instance mock is used before being initialized in the first test case, which could cause test failures. The mock setup should happen before client creation.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| name: "AppNotFoundError", | |
| message: "Not found", | |
| }; | |
| mock.onGet(/.*\/api\/v1\/apps/).reply(404, mockError); | |
| const client = new Composio({ | |
| apiKey: COMPOSIO_API_KEY, | |
| baseUrl: getTestConfig().BACKEND_HERMES_URL, | |
| }); | |
| const mock = new AxiosMockAdapter(client.backendClient.getAxiosInstance()); | |
| mock.onGet(/.*\/api\/v1\/apps/).reply(404, mockError); | |
| const client = new Composio({ | |
| apiKey: COMPOSIO_API_KEY, | |
| baseUrl: getTestConfig().BACKEND_HERMES_URL, | |
| }); | |
| const mock = new AxiosMockAdapter(client.backendClient.getAxiosInstance()); | |
| mock.onGet(/.*\/api\/v1\/apps/).reply(404, mockError); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5543edc in 2 minutes and 0 seconds
More details
- Looked at
222lines of code in5files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:280
- Draft comment:
Ensure that passing the axios instance via 'client: this.client.backendClient.instance' is always defined. This change looks correct but verify that the backendClient instance is properly initialized in all cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that a certain instance is always defined and initialized properly. This falls under the category of asking the author to double-check or verify something, which is against the rules.
2. js/src/sdk/index.spec.ts:82
- Draft comment:
Good update: using a local AxiosMockAdapter with each client's axios instance improves test isolation. Ensure that resetting and restoring mocks doesn't conflict with the global mock declared at the top. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that there is no conflict with the global mock. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
3. js/src/sdk/utils/processor/file.ts:61
- Draft comment:
Updated processFileUpload to require a 'client' parameter is correctly propagated to getFileDataAfterUploadingToS3. Confirm that all calls to FILE_INPUT_PROCESSOR pass the client correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm that all calls to FILE_INPUT_PROCESSOR pass the 'client' parameter correctly. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or to double-check things. Therefore, this comment should be removed.
4. js/src/sdk/utils/processor/fileUtils.ts:40
- Draft comment:
Passing the client parameter to uploadFileToS3 and subsequently to apiClient.actionsV2.createFileUploadUrl is consistent. Verify that the client configuration matches the unique axios instance setup. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. js/src/types/base_toolset.ts:55
- Draft comment:
The TPreProcessor type signature has been updated to include 'client: Client'. Ensure all processor implementations are updated accordingly to handle the client parameter. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that all processor implementations are updated to handle the new 'client' parameter. This falls under asking the author to ensure something is done, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
6. js/src/sdk/base.toolset.ts:280
- Draft comment:
Ensure that the 'client' passed (this.client.backendClient.instance) conforms to the expected Client type from '@hey-api/client-axios'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to ensure that a specific object conforms to a type. It is not suggesting a specific code change or asking for a test to be written. It is more of a reminder or a prompt to double-check something, which violates the rules.
7. js/src/sdk/index.spec.ts:82
- Draft comment:
The test redeclares 'mock' which shadows the globally defined mock. Consider using a unique variable name or reusing the global one for clarity. - 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 changes in the diff show a pattern of moving from a global mock to local mocks in each test. This appears to be an intentional refactoring to improve test isolation. Each test now creates and resets its own mock, which is generally better practice than sharing a global mock. The shadowing is likely intentional part of this refactoring.
The comment might be technically correct about the shadowing, but am I missing that this could be an intentional architectural change to improve test isolation?
The pattern of creating local mocks appears consistently throughout the changes, suggesting this is an intentional refactoring to improve test design rather than an oversight.
The comment should be deleted because it appears to be critiquing what is actually an intentional and beneficial refactoring to improve test isolation.
8. js/src/sdk/index.spec.ts:238
- Draft comment:
Missing semicolon after 'mock.reset()'. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. js/src/sdk/utils/processor/file.ts:57
- Draft comment:
Verify that the new 'client' parameter in FILE_INPUT_PROCESSOR is correctly routed to downstream file upload functions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
10. js/src/sdk/utils/processor/fileUtils.ts:74
- Draft comment:
Confirm that returning 'signedURL' on a 403 error is intentional, as this bypasses throwing an error. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. js/src/sdk/index.spec.ts:184
- Draft comment:
There appears to be a typographical error in the expected error description on line 184: "er is currently unable to handle the reque". It seems like the string might be truncated or misspelled. Consider correcting it to something like "server is currently unable to handle the request". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ycI3hMZ5QEfxMOBb
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 f92f531 in 55 seconds
More details
- Looked at
45lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. js/src/sdk/actionRegistry.ts:187
- Draft comment:
Consider using the getAxiosInstance() method instead of directly accessing 'this.client.backendClient.instance' for better encapsulation. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. js/src/sdk/models/backendClient.ts:107
- Draft comment:
Remove or update the commented-out code 'setAxiosClientConfig(this.instance.instance)' if it is no longer needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. js/src/sdk/actionRegistry.ts:187
- Draft comment:
Ensure the axios instance from the current Composio instance is passed. Confirm thatthis.client.backendClient.instanceis always defined. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. js/src/sdk/models/backendClient.ts:92
- Draft comment:
Remove obsolete commented code to reduce clutter if it is no longer necessary. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. js/src/sdk/models/backendClient.ts:38
- Draft comment:
Consider consolidating the axios configuration. The instance config is set in the constructor via createClient() and then overwritten in initializeApiClient(). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. js/src/sdk/models/backendClient.ts:29
- Draft comment:
Typo in the constructor JSDoc: it says 'Creates an instance of apiClientDetails.' but the class is named 'AxiosBackendClient'. Please update the comment to match the class name for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_qkcSCMGpNAhgOfq3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Skipped PR review on 4e7c94a because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 136a46a in 2 minutes and 4 seconds
More details
- Looked at
140lines of code in5files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. js/src/sdk/index.spec.ts:55
- Draft comment:
Minor formatting cleanup: the inline API key assertions now use a single-line format. This is acceptable. Ensure that these tests continue to cover all edge cases for multiple client configurations. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%
None
2. js/src/sdk/index.spec.ts:77
- Draft comment:
Removed excessive blank lines for better readability. No functional impact, but consistent formatting is appreciated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. js/src/sdk/index.spec.ts:137
- Draft comment:
The call to mock.reset() now includes a semicolon. This cleanup is fine; just verify that multiple resets in afterEach and in tests do not unintentionally interfere with the mock state. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. js/src/sdk/models/backendClient.ts:38
- Draft comment:
The axios client instance is created before checking if the API key is provided. Consider moving the API key validation (line 50) to the start of the constructor to avoid unnecessary client instantiation when the API key is missing. - Reason this comment was not posted:
Comment was on unchanged code.
5. js/src/sdk/models/backendClient.ts:108
- Draft comment:
There is a commented-out call to setAxiosClientConfig. If this configuration is no longer needed, consider removing the commented code to keep the file clean. - Reason this comment was not posted:
Comment was on unchanged code.
6. js/src/sdk/utils/processor/file.ts:57
- Draft comment:
The parameter formatting now includes a trailing comma. This improves consistency and is fine as long as the linter rules permit it. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. js/src/sdk/utils/processor/fileUtils.ts:10
- Draft comment:
The use of readFileSync in readFileContent is synchronous. For improved performance in environments where asynchronous I/O is preferred, consider using an asynchronous file read method. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. js/src/types/base_toolset.ts:1
- Draft comment:
The import order has been adjusted (Client import moved). Ensure consistency with the rest of the project’s style guidelines. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
9. js/src/sdk/index.spec.ts:55
- Draft comment:
Consolidated multi-line expect statements into single lines improves readability and consistency in the tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. js/src/sdk/index.spec.ts:235
- Draft comment:
Semicolon added after mock.reset() for consistent code style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. js/src/sdk/models/backendClient.ts:1
- Draft comment:
Removed unused import 'setAxiosClientConfig' and reformatted the createClient call for better readability. Consider removing commented-out code if it's no longer needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. js/src/sdk/utils/processor/file.ts:57
- Draft comment:
Trailing comma added in destructuring improves consistency in parameter definitions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. js/src/sdk/utils/processor/fileUtils.ts:1
- Draft comment:
Duplicate import of 'Client' has been cleaned up to reduce clutter and potential confusion. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. js/src/types/base_toolset.ts:1
- Draft comment:
Reordered the 'Client' import to the top and removed a duplicate import, enhancing clarity and consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
15. js/src/sdk/index.spec.ts:178
- Draft comment:
The error description substring "er is currently unable to handle the reque" appears incomplete (likely missing words like 'server' and the end of 'request'). Please update it to the intended full message. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. js/src/sdk/models/backendClient.ts:28
- Draft comment:
Minor documentation inconsistency: The JSDoc comment for the constructor on line 28 refers to 'apiClientDetails' but the class name is 'AxiosBackendClient'. Consider updating the JSDoc comment to reflect the correct class name. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_wiDMLOTaEaMKPAh5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Comment on lines
197
to
206
| const error = e as ComposioError; | ||
| expect(error.errCode).toBe(COMPOSIO_SDK_ERROR_CODES.BACKEND.SERVER_ERROR); | ||
| } | ||
| mock.reset(); | ||
| }); | ||
|
|
||
| it("should give request timeout error", async () => { | ||
| it.skip("should give request timeout error", async () => { | ||
| const client = new Composio({ | ||
| apiKey: COMPOSIO_API_KEY, | ||
| baseUrl: getTestConfig().BACKEND_HERMES_URL, |
There was a problem hiding this comment.
Skipping error handling tests without replacement or alternative coverage could mask potential issues in error handling logic. Tests should be fixed or replaced rather than skipped.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| const error = e as ComposioError; | |
| expect(error.errCode).toBe(COMPOSIO_SDK_ERROR_CODES.BACKEND.SERVER_ERROR); | |
| } | |
| mock.reset(); | |
| }); | |
| it("should give request timeout error", async () => { | |
| it.skip("should give request timeout error", async () => { | |
| const client = new Composio({ | |
| apiKey: COMPOSIO_API_KEY, | |
| baseUrl: getTestConfig().BACKEND_HERMES_URL, | |
| it("should throw an error if apiKey is not provided", async () => { | |
| const client = new Client(); | |
| await expect(client.init()).rejects.toThrow("API key is required"); | |
| }); | |
| it("should handle 404 error gracefully", async () => { | |
| const client = new Client("test-key"); | |
| mockAxios.onGet().reply(404); | |
| await expect(client.getData()).rejects.toThrow("Resource not found"); | |
| }); | |
| it("should handle 400 error gracefully", async () => { | |
| const client = new Client("test-key"); | |
| mockAxios.onGet().reply(400, { error: "Bad request" }); | |
| await expect(client.getData()).rejects.toThrow("Invalid request parameters"); | |
| }); | |
| it("should handle 500 and 502 error gracefully", async () => { | |
| const client = new Client("test-key"); | |
| mockAxios.onGet().reply(500); | |
| await expect(client.getData()).rejects.toThrow("Server error"); | |
| mockAxios.onGet().reply(502); | |
| await expect(client.getData()).rejects.toThrow("Gateway error"); | |
| }); | |
| it("should give request timeout error", async () => { | |
| const client = new Client("test-key"); | |
| mockAxios.onGet().timeout(); | |
| await expect(client.getData()).rejects.toThrow("Request timed out"); | |
| }); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b4065cd in 1 minute and 45 seconds
More details
- Looked at
49lines of code in1files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. js/src/sdk/index.spec.ts:28
- Draft comment:
Avoid skipping tests without a clear comment or TODO explaining why they're skipped. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to provide a reason for skipping tests, which is a request for clarification. This violates the rule against asking the author to explain or confirm their intention. Therefore, this comment should be removed.
2. js/src/sdk/index.spec.ts:66
- Draft comment:
Document why this 404 test is skipped, to avoid losing coverage for error handling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for documentation on why a test is skipped. This is not a request for a code change or a suggestion for improvement, but rather a request for additional information. According to the rules, comments should not ask the PR author to update documentation or provide explanations. Therefore, this comment should be removed.
3. js/src/sdk/index.spec.ts:102
- Draft comment:
Consider adding a comment for skipping the 400 error test to clarify if it's temporary. - 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%
While it's true that skipped tests should ideally have explanations, this seems like a broader pattern where multiple error handling tests were skipped. The comment is asking for information rather than pointing out a clear issue. It's asking the author to explain their intention, which violates our rules. Additionally, there may be a temporary testing refactor happening that the reviewer isn't aware of.
The comment could be identifying a legitimate testing best practice issue. Skipped tests without explanation can lead to confusion later.
While documenting skipped tests is good practice, this comment violates our rule about not asking authors to explain their intentions. If skipped tests are an issue, it should be raised as a broader architectural discussion.
Delete the comment as it violates our rule about not asking authors to explain their intentions or confirm things.
4. js/src/sdk/index.spec.ts:140
- Draft comment:
Skipping the 500/502 error test should include a comment on why it's disabled. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to include a comment explaining why a test is disabled. This falls under asking the author to explain or confirm their intention, which is against the rules. Therefore, this comment should be removed.
5. js/src/sdk/index.spec.ts:203
- Draft comment:
Add rationale for skipping the request timeout test to ensure proper tracking. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to add a rationale for skipping a test. This is similar to asking for an update to the PR description or asking for confirmation of intention, which is against the rules. The comment does not provide a specific code suggestion or point out a specific issue with the code itself.
6. js/src/sdk/index.spec.ts:66
- Draft comment:
Please add a comment explaining why this 404 error simulation test is skipped; leaving tests skipped without context can lead to neglect. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to add an explanation for a skipped test. While it is generally good practice to document why tests are skipped, the comment is essentially asking for an explanation or confirmation, which violates the rule against asking the PR author to explain or confirm their intention. Therefore, this comment should be removed.
7. js/src/sdk/index.spec.ts:99
- Draft comment:
Ensure there is a comment or associated issue for skipping the 400 error handling test to prevent merge of unverified error handling logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that there is a comment or issue associated with skipping a test. This is a request for verification and ensuring something is done, which violates the rules.
8. js/src/sdk/index.spec.ts:137
- Draft comment:
Document the reason for skipping the 500/502 error handling test. If it's a temporary measure, add a TODO with reference to a tracking issue. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a reasonable request - skipping tests without explanation can lead to technical debt and confusion. However, we see that multiple tests are being skipped, suggesting this is likely part of a broader change. The comment is asking for explanation of just one skipped test when there's clearly a pattern. This suggests the comment may be missing the bigger picture.
The comment focuses on a single test when multiple tests are being skipped, suggesting there may be a broader architectural or testing strategy change happening that we don't have context for.
While documenting skipped tests is generally good practice, making one-off comments about individual skipped tests when there's clearly a broader pattern may not be the most helpful approach.
The comment should be removed. While it makes a valid point about documentation, it's likely missing broader context about why multiple tests are being skipped, and could lead to inconsistent documentation if applied to just one test.
9. js/src/sdk/index.spec.ts:203
- Draft comment:
Add rationale for skipping the request timeout error test to ensure future maintainers understand the context behind this skip. - 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 asks for documentation about why the test was skipped. However, this appears to be part of a larger pattern where multiple error handling tests were skipped at once. The comment is essentially asking the author to explain their intention, which violates our rules. We should trust that the author had a good reason for skipping these tests.
Maybe understanding why these tests were skipped could be important for future maintenance. The comment could help prevent accidental test disablement.
While documentation can be helpful, asking authors to explain their intentions violates our rules. If the skip was problematic, it would be caught in code review. The pattern of skipping multiple error tests suggests this is an intentional change.
Delete the comment as it violates our rule about not asking authors to explain their intentions or confirm their changes.
10. js/src/sdk/index.spec.ts:178
- Draft comment:
There appears to be a potential typographical error: the substring 'er is currently unable to handle the reque' seems incomplete. It likely should be something like 'server is currently unable to handle the request'. Please verify the intended message and update accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_JC8l8260Vz9jEzuX
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.
| @@ -25,7 +25,7 @@ describe("Basic SDK spec suite", () => { | |||
| expect(client).toBeInstanceOf(Composio); | |||
| }); | |||
|
|
|||
| it("should throw an error if apiKey is not provided", async () => { | |||
| it.skip("should throw an error if apiKey is not provided", async () => { | |||
There was a problem hiding this comment.
Avoid skipping the test for missing API key unless there's a clear rationale. If the duplicate test below suffices, consider removing one instead of skipping.
Suggested change
| it.skip("should throw an error if apiKey is not provided", async () => { | |
| it("should throw an error if apiKey is not provided", async () => { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Composioinstance now creates its ownaxiosinstance inindex.ts, allowing multiple clients with different API keys.index.spec.tsensure differentaxiosinstances for different clients and correct API key usage.Entity.ts,actions.ts,activeTriggers.ts,apps.ts,connectedAccounts.ts,integrations.ts, andtriggers.tsto acceptaxiosinstance.AxiosBackendClientinbackendClient.tsnow usescreateClientfrom hey-api/client-axios.index.spec.tsto verify multiple clients with different API keys.Entity.spec.ts,actions.spec.ts,activeTriggers.spec.ts,apps.spec.ts,connectedAccounts.spec.ts,integrations.spec.ts, andtriggers.spec.tsto accommodate newaxiosinstance handling.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 9b9a8b0. It will automatically update as commits are pushed.
Important
Each
Composioinstance now creates its ownaxiosinstance, allowing multiple clients with different API keys, with tests verifying this behavior.Composioinstance now creates its ownaxiosinstance inindex.ts, allowing multiple clients with different API keys.index.spec.tsensure differentaxiosinstances for different clients and correct API key usage.Entity.ts,actions.ts,activeTriggers.ts,apps.ts,connectedAccounts.ts,integrations.ts, andtriggers.tsto acceptaxiosinstance.AxiosBackendClientinbackendClient.tsnow usescreateClientfrom@hey-api/client-axios.index.spec.tsto verify multiple clients with different API keys.Entity.spec.ts,actions.spec.ts,activeTriggers.spec.ts,apps.spec.ts,connectedAccounts.spec.ts,integrations.spec.ts, andtriggers.spec.tsto accommodate newaxiosinstance handling.This description was created by
for b4065cd. It will automatically update as commits are pushed.