Create new integration if integrationId not provided

[plxity]

Create new integration if integration ID is not provided is the connectedAccounts.initiate method.

---

Important

Add logic to create a new integration in ConnectedAccounts.initiate if integrationId is not provided.

• Behavior:
  • In ConnectedAccounts.initiate, create a new integration if integrationId is not provided and authMode is specified.
  • Use Apps.get to retrieve app details and Integrations.create to create a new integration with a timestamped name.
• Types:
  • integrationId in InitiateConnectionDataReq is now optional.
• Misc:
  • Minor formatting changes in connectedAccounts.ts.

^{This description was created by for c2ceb80. It will automatically update as commits are pushed.}

[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	Nov 22, 2024 7:02pm

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to c2ceb80 in 43 seconds

More details

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

1. js/src/sdk/models/connectedAccounts.ts:4

• Draft comment:
  Duplicate import of client. Consider removing one of the import statements to avoid confusion.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_jArZWVgx8Gg80fTT

---

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.

[ellipsis-dev]

Using appName! assumes appName is always defined, which might not be the case. Consider adding a check to ensure appName is not undefined before using it.

Suggested change

	const app = await this.apps.get({ appKey: appName! });
	const app = appName ? await this.apps.get({ appKey: appName }) : null;

[ellipsis-dev]

Using integration!.id! assumes integration and integration.id are always defined, which might not be the case. Consider adding a check to ensure integration and integration.id are not undefined before using them.

Suggested change

	integrationId = integration!.id!;
	integrationId = integration?.id;

[shreysingla11]

The authConfig type is using any which could lead to runtime errors. Consider defining a proper interface for better type safety:

interface AuthConfig {
    clientId?: string;
    clientSecret?: string;
    [key: string]: string | undefined;
}

[plxity]

It's not required here.

[shreysingla11]

Using non-null assertions (!) on app.appId and integration.id could lead to runtime errors if these values are undefined. Consider adding proper validation:

if (!app?.appId) {
    throw new Error(`App not found or invalid app ID for ${appName}`);
}
if (!integration?.id) {
    throw new Error('Failed to create integration');
}
integrationId = integration.id;

[plxity]

I've added it in the next commit

[shreysingla11]

The initiate method should validate required parameters before proceeding. Consider adding input validation:

if (!integrationId && !appName) {
    throw new Error('Either integrationId or appName must be provided');
}
if (!integrationId && !authMode) {
    throw new Error('authMode is required when creating new integration');
}

[shreysingla11]

Consider extracting the integration creation logic into a separate private method for better code organization and reusability:

private async createNewIntegration(
    appName: string,
    authMode: string,
    authConfig: AuthConfig
): Promise<string> {
    const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
    const app = await this.apps.get({ appKey: appName });
    if (!app?.appId) {
        throw new Error(`Invalid app: ${appName}`);
    }
    
    const integration = await this.integrations.create({
        appId: app.appId,
        name: `integration_${timestamp}`,
        authScheme: authMode,
        authConfig,
        useComposioAuth: false,
    });
    
    return integration?.id ?? throw new Error('Failed to create integration');
}

[plxity]

Not required.

[shreysingla11]

Code Review Summary

Overall Assessment

The changes introduce good functionality for dynamic integration creation, but there are several areas that need attention for better reliability and maintainability.

Strengths

✅ Good separation of concerns with proper class dependencies
✅ Flexible approach to handle both existing and new integrations
✅ Consistent error handling pattern using CEG.handleError

Areas for Improvement

1. Type Safety

   • Replace any type in authConfig with proper interface
   • Add proper null checks instead of non-null assertions

2. Input Validation

   • Add validation for required parameters
   • Validate appName and authMode when creating new integration

3. Code Organization

   • Extract integration creation logic to a separate method
   • Consider adding utility functions for common operations

4. Documentation

   • Add JSDoc comments for the modified methods
   • Document the new parameters and their requirements

Testing Requirements

• Unit tests for new integration creation flow
• Error case testing for invalid inputs
• Integration tests for the complete flow

The changes are generally good but implementing the suggested improvements would make the code more robust and maintainable.

Rating: 7/10 - Good functionality but needs improvements in type safety and validation.

[github-actions]

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

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