feat: create new axios instance for each composio instance

[aiswaryasankar]

No description provided.

[entelligence-ai-pr-reviews]

🚀 Code Review Initiated

The review process for this pull request has started. Our system is analyzing the changes for:

• Code quality, performance, and potential issues
• Adherence to project guidelines
• Integration of user-provided learnings

You will receive structured and actionable feedback shortly! ⏳

[vercel]

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 28, 2025 7:38pm

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to b4065cd in 1 minute and 43 seconds

More details

• Looked at 1063 lines of code in 22 files
• Skipped 0 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. js/src/sdk/actionRegistry.ts:188

• Draft comment:
  Passing the axios client as 'this.client.backendClient.instance' works, but consider centralizing or renaming the property (e.g. to 'httpClient') for clarity and consistency.
• 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%
  While the suggestion to rename for clarity isn't wrong, it's more of a code style preference than a critical issue. The current name is descriptive enough - it clearly indicates it's a client instance from the backend client. Renaming would require changes across potentially multiple files. This feels like a minor stylistic suggestion rather than an important code quality issue.
  The property name could potentially be confusing to new developers. The multiple levels of nesting (client.backendClient.instance) might indicate a design that could be simplified.
  While valid points, the current name is functional and clear enough. Making this change would require broader refactoring that may not be worth the effort given the limited benefit.
  This comment should be removed as it suggests a minor stylistic change that isn't critical to code quality and could require changes across multiple files.

2. js/src/sdk/base.toolset.ts:280

• Draft comment:
  Ensure that passing 'client: this.client.backendClient.instance' into pre-processors is consistently documented. It might be clearer to rename the parameter to 'httpClient' so that its purpose is explicit.
• 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 comment makes a reasonable suggestion about making the code clearer through better naming. However, it's phrased as "Ensure that..." which violates our rules. It's also asking for documentation which isn't strictly necessary for the code to work. The suggestion to rename to httpClient is reasonable but not critical enough to warrant a comment.
  The parameter name 'client' might actually be fine as is - we don't have enough context about how these processors are documented or used elsewhere in the codebase. The rename could make things more confusing if 'client' is the standard term used elsewhere.
  While the rename suggestion has merit, this kind of minor naming suggestion that isn't fixing a real problem should probably be handled in code review discussions rather than automated comments.
  Delete this comment as it's phrased as a verification request and makes non-critical suggestions about documentation and naming that don't highlight any real issues.

3. js/src/sdk/index.spec.ts:28

• Draft comment:
  Several tests are marked as skipped (it.skip). Please ensure there is a comment explaining why these tests are skipped for clarity.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that there is a comment explaining why tests are skipped. This falls under asking the author to ensure something is done, which violates the rules.

4. js/src/sdk/models/backendClient.ts:111

• Draft comment:
  The getAxiosInstance() method returns 'this.instance.instance'; verify that this double accessor correctly exposes the underlying axios instance. Consider adding a comment for clarity.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50%
  The comment is asking the author to verify the behavior of a method, which violates the rule against asking the author to confirm their intention or to double-check things. However, it also suggests adding a comment for clarity, which is a valid suggestion. The comment is partially useful, but the part about verification should be removed.

5. js/src/sdk/models/integrations.ts:168

• Draft comment:
  In create() and getOrCreateIntegration(), if 'uniqueKey' is not provided, the code attempts to derive it. However, if not found, it throws an error immediately. Consider providing more graceful error-handling or fallback behavior.
• 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/activeTriggers.ts:160

• Draft comment:
  The getTriggerConfig() method casts res.data with 'as unknown as SingleTriggerRes'. This double cast indicates type uncertainty. Consider refining the types to avoid unsafe casting.
• 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/utils/processor/file.ts:40

• Draft comment:
  The file upload processing uses synchronous file reading (require('fs').readFileSync). Consider using an asynchronous API (fs.promises) for improved performance and non-blocking behavior.
• 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/utils/processor/fileUtils.ts:43

• Draft comment:
  The MD5 hash is computed on the file's base64 string. Verify that this approach covers all edge cases and produces consistent hashes; consider hashing the raw binary content instead.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50%
  The comment suggests a specific improvement by recommending hashing the raw binary content instead of the base64 string. However, it also asks the author to verify that the current approach covers all edge cases, which violates the rule against asking for verification. The suggestion to consider an alternative approach is valid, but the request for verification is not.

9. js/src/types/base_toolset.ts:55

• Draft comment:
  The preprocessor and postprocessor types now require a Client instance. Ensure that all consuming functions pass the proper Client from this new axios client instance.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that all consuming functions pass the proper Client instance. This falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.

10. js/src/types/base_toolset.ts:19

• Draft comment:
  Consider replacing 'z.any()' with 'z.unknown()' in the definitions for the 'properties' field (and similarly in the 'response' field) to enforce stricter 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.

11. js/src/sdk/index.spec.ts:178

• Draft comment:
  It appears that the expected substring in error.description ('er is currently unable to handle the reque') is truncated and may be a typographical error. Please verify if it should read something like 'server is currently unable to handle the request' or adjust to the correct complete 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.

12. js/src/sdk/models/Entity.spec.ts:75

• Draft comment:
  Typo: The property 'successfull' appears to be misspelled. Consider renaming it to 'successful' 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:9

• Draft comment:
  Typographical error: 'connectedAccouns' should be renamed to 'connectedAccounts' to maintain consistency and 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.

14. js/src/sdk/models/actions.spec.ts:58

• Draft comment:
  Typographical error: The property 'successfull' appears to have a misspelling; consider renaming it to 'successful'.
• 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_89FC6UrCBEduXoNm

---

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

[shreysingla11]

Several critical error handling tests have been skipped without explanation. These tests cover important error scenarios (404, 400, 500, 502, timeout). Either:

1. Add TODO comments explaining why they're skipped and create follow-up tickets
2. Fix and re-enable these tests
3. Remove them if they're no longer relevant

Skipping these tests could hide potential issues with error handling in the new client instance implementation.

[shreysingla11]

The AxiosBackendClient class is creating a new axios instance in the constructor but not properly cleaning it up. Consider adding a cleanup method to properly dispose of the instance when it's no longer needed:

public dispose() {
  if (this.instance) {
    // Clean up any event listeners or resources
    this.instance = null;
  }
}

This would help prevent memory leaks, especially in scenarios where multiple client instances are created and destroyed.

[shreysingla11]

The ConnectionRequest class is storing the client instance but not properly handling errors in the saveUserAccessData method. Consider improving error handling:

async saveUserAccessData(data: SaveUserAccessDataParam) {
  try {
    ZSaveUserAccessDataParam.parse(data);
    const { data: connectedAccount } = await apiClient.connections.getConnection({
      client: this.client,
      path: { connectedAccountId: this.connectedAccountId },
    });
    if (!connectedAccount) {
      throw CEG.getCustomError(
        COMPOSIO_SDK_ERROR_CODES.COMMON.CONNECTED_ACCOUNT_NOT_FOUND,
        {
          message: "Connected account not found",
          connectedAccountId: this.connectedAccountId
        }
      );
    }
    // ... rest of the code
  } catch (error) {
    throw CEG.handleAllError(error);
  }
}

This would provide better error context and consistent error handling across the SDK.

[shreysingla11]

Code Review Summary

Overall Assessment

The changes to create separate axios instances for each Composio instance are well-structured and improve isolation between client instances. However, there are a few areas that need attention:

1. Critical Issues:

   • Several error handling tests are skipped without explanation
   • Missing cleanup mechanism for axios instances
   • Inconsistent error handling in some classes

2. Code Quality: 7/10

   • Good separation of concerns
   • Proper type safety with TypeScript
   • Clean implementation of client instance management
   • Could use better documentation and error handling

3. Security: 8/10

   • Good isolation of API keys between instances
   • Proper header management for each client
   • No credential leakage between instances

4. Performance: 8/10

   • Minimal overhead from multiple instances
   • Good resource management
   • Consider adding instance cleanup

Recommendations

1. Re-enable skipped tests or document why they're skipped
2. Add cleanup mechanism for axios instances
3. Standardize error handling across all classes
4. Add more documentation for the new client instance management

The changes are generally good and improve the SDK's architecture, but addressing these issues would make it more robust.