feat: new integration create and initaite connection + reinitiate and update API schema #1189
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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: |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| @@ -14,7 +14,7 @@ | |||
| "test:watch": "jest --testMatch=\"**/*.spec.ts\" --watch", | |||
| "test:coverage": "jest --coverage --testMatch=\"**/*.spec.ts\"", | |||
| "type-docs": "typedoc", | |||
| "openapispec:generate": "npx @hey-api/openapi-ts", | |||
| "openapispec:generate": "npx @hey-api/openapi-ts ", | |||
There was a problem hiding this comment.
The openapispec:generate script in package.json has a trailing space, which might cause issues with some tools or scripts.
📝 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.
| "openapispec:generate": "npx @hey-api/openapi-ts ", | |
| "openapispec:generate": "npx @hey-api/openapi-ts" |
| sortBy: { | ||
| enum: ["alphabet", "usage", "no_sort"], | ||
| type: "string", | ||
| description: "Sort the apps by usage or alphabetically", | ||
| example: "usage", | ||
| default: "alphabetically", | ||
| }, |
There was a problem hiding this comment.
The default value for the "sortBy" property in AppQueryDTO is described as "alphabetically" but the example value is "usage". It would be better to align the default value with the example or vice versa.
📝 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.
| sortBy: { | |
| enum: ["alphabet", "usage", "no_sort"], | |
| type: "string", | |
| description: "Sort the apps by usage or alphabetically", | |
| example: "usage", | |
| default: "alphabetically", | |
| }, | |
| sortBy: { | |
| enum: ["alphabet", "usage", "no_sort"], | |
| type: "string", | |
| description: "Sort the apps by usage or alphabetically", | |
| example: "alphabet", | |
| default: "alphabet", | |
| }, |
| * Filter to include locally developed/testing apps in the response. Must be 'true' or 'false' | ||
| */ | ||
| includeLocal?: "true" | "false"; | ||
| /** | ||
| * Sort the apps by usage or alphabetically | ||
| */ | ||
| sortBy?: "alphabet" | "usage" | "no_sort"; |
There was a problem hiding this comment.
The includeLocal property in AppQueryDTO is defined as a string enum, but it should be a boolean to be consistent with its usage and description.
📝 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.
| * Filter to include locally developed/testing apps in the response. Must be 'true' or 'false' | |
| */ | |
| includeLocal?: "true" | "false"; | |
| /** | |
| * Sort the apps by usage or alphabetically | |
| */ | |
| sortBy?: "alphabet" | "usage" | "no_sort"; | |
| /** | |
| * Filter to include locally developed/testing apps in the response. Must be 'true' or 'false' | |
| */ | |
| includeLocal?: boolean; | |
| /** | |
| * Sort the apps by usage or alphabetically | |
| */ | |
| sortBy?: "alphabet" | "usage" | "no_sort"; |
![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 c3fc6de in 1 minute and 42 seconds
More details
- Looked at
5282lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/client/schemas.gen.ts:1970
- Draft comment:
The$GetConnectorListResDTOschema has an incorrect$refforitems. It should be an array ofConnectorListItemDTO. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. js/src/sdk/client/types.gen.ts:1706
- Draft comment:
status?: string;
- Reason this comment was not posted:
Marked as duplicate.
3. js/src/sdk/client/schemas.gen.ts:2385
- Draft comment:
Ensure thestatusfield in$ConnectedAccountResponseDTOincludes 'EXPIRED' as an option. - Reason this comment was not posted:
Comment did not seem useful.
4. js/src/sdk/client/schemas.gen.ts:2543
- Draft comment:
Ensure thestatusfield in$GetConnectionsQueryDtois a string to match the description of being a comma-separated list. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_lzBSaUSYj6aDtcLp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| type: "string", | ||
| description: "Sort the apps by usage or alphabetically", | ||
| example: "usage", | ||
| default: "alphabetically", |
There was a problem hiding this comment.
There's an inconsistency in the sortBy field default value. The default is set to "alphabetically" but this value is not included in the enum which only allows ["alphabet", "usage", "no_sort"]. This could cause validation errors. Consider changing the default to "alphabet" to match the enum values.
| }, | ||
| logo: { | ||
| triggerType: { | ||
| enum: ["poll", "webhook"], |
There was a problem hiding this comment.
The triggerType enum in TriggerLogItemDTO is missing documentation for its values. Consider adding descriptions for what "poll" and "webhook" trigger types mean and when they are used. This will help with API documentation and client understanding.
| }, | ||
| triggerClientResponse: { | ||
| type: "object", | ||
| description: "Payload sent by the client when the trigger was activated.", |
There was a problem hiding this comment.
The triggerClientResponse and triggerClientPayload in TriggerLogItemDTO have the same description: "Payload sent by the client when the trigger was activated". This seems incorrect as they represent different things - one is the request payload and the other is the response. Please update the descriptions to accurately reflect their purposes.
| type: "object", | ||
| }, | ||
| type: "array", | ||
| description: "List of availavle versions of the action.", |
There was a problem hiding this comment.
The available_versions field in ActionDetails has a typo in its description: "List of availavle versions of the action". Should be "available" instead of "availavle".
| }, | ||
| integration_id: { | ||
| type: "string", | ||
| description: "List of app unique keys to filter by", |
There was a problem hiding this comment.
The integration_id field in AppFilterDTO has an incorrect description. It's currently using the same description as unique_key: "List of app unique keys to filter by". This should be updated to accurately describe what the integration_id field is used for.
Code Review SummaryOverall AssessmentThe code changes look generally good with some minor issues that need attention. The changes primarily focus on:
Issues Found
Positive Aspects
Suggestions for Improvement
Code Quality Rating: 8/10Good overall quality with attention to type safety and error handling, but needs minor documentation improvements and validation fixes. |
| const connection = await apiClient.connectionsV2.initiateConnectionV2({ | ||
| body: { | ||
| app: { | ||
| uniqueKey: payload.appName!, | ||
| integrationId: payload.integrationId, | ||
| }, | ||
| config: { | ||
| name: payload.appName!, | ||
| useComposioAuth: !!payload.authMode && !!payload.authConfig, | ||
| authScheme: payload.authMode as z.infer<typeof ZAuthMode>, | ||
| integrationSecrets: payload.authConfig, | ||
| }, | ||
| connection: { | ||
| entityId: payload.entityId, | ||
| initiateData: payload.connectionParams as Record<string, unknown>, | ||
| extra: { | ||
| redirectURL: payload.redirectUri, | ||
| labels: payload.labels || [], | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing validation for appName or integrationId can lead to runtime errors if both are absent. Add checks before API call. Non-null assertion on appName is unsafe; validate or provide a default to prevent runtime errors.
| entityId: payload.entityId, | ||
| initiateData: payload.connectionParams as Record<string, unknown>, |
There was a problem hiding this comment.
Direct type casting of connectionParams may allow invalid data types, causing downstream issues. Validate before casting.
| extra: { | ||
| redirectURL: payload.redirectUri, | ||
| labels: payload.labels || [], | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (!isTestConnectorAvailable && app.no_auth === false) { | ||
| logger.debug( | ||
| "Auth schemes not provided, available auth schemes and authConfig" | ||
| ); | ||
| // @ts-ignore | ||
| for (const authScheme of app.auth_schemes) { | ||
| logger.debug( | ||
| "authScheme:", | ||
| authScheme.name, | ||
| "\n", | ||
| "fields:", | ||
| authScheme.fields | ||
| ); | ||
| } | ||
| const connectionResponse = connection?.data?.connectionResponse; |
There was a problem hiding this comment.
Removing error handling for missing 'integrationId' or 'appName' may cause issues if these are required.
|
|
||
| constructor(backendClient: BackendClient) { |
There was a problem hiding this comment.
Removing 'integrations' and 'apps' properties may cause issues if they are used elsewhere in the code.
| }, | ||
| }, | ||
| type: "object", | ||
| required: ["authScheme"], |
There was a problem hiding this comment.
Removing 'name' from required fields may cause issues if 'name' is expected elsewhere in the code.
| }, | ||
| labels: { | ||
| items: { | ||
| type: "string", |
There was a problem hiding this comment.
Changing 'labels' type from object to string may break functionality if the code expects an object.
| "Details about the integration associated with this connection.", | ||
| }, | ||
| connectionResponse: { | ||
| $ref: "#/components/schemas/ConnectionResponseV2", |
There was a problem hiding this comment.
Changing 'connectionResponse' from object to reference may cause issues if the reference is not defined correctly.
| /** | ||
| * Name of the integration | ||
| */ | ||
| name?: string; |
There was a problem hiding this comment.
Changing 'name' to optional may cause issues if 'name' is required elsewhere in the code.
| /** | ||
| * Array of labels to associate with the connection for organization and filtering. | ||
| */ | ||
| labels?: Array<string>; |
There was a problem hiding this comment.
Changing 'labels' from object to string array may break functionality if the code expects an object.
| /** | ||
| * Response data containing connection details and status. | ||
| */ | ||
| connectionResponse: ConnectionResponseV2; |
There was a problem hiding this comment.
Changing 'connectionResponse' from object to reference may cause issues if the reference is not defined correctly.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 2b99c9d in 1 minute and 48 seconds
More details
- Looked at
354lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/connectedAccounts.ts:185
- Draft comment:
Typo in function name 'reiniateConnection'. It should be 'reinitiateConnection'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_yS6wMhMsu7aJwzre
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.
| sed -i '' '/export type in = '\''query'\'' | '\''header'\'';/d' src/sdk/client/types.gen.ts | ||
| sed -i '' 's/successfull: boolean/successfull?: boolean/g' src/sdk/client/types.gen.ts |
There was a problem hiding this comment.
The sed command lacks error handling, potentially failing silently on some platforms.
| * Whether the action execution was successfully executed or not. If this is false, error field will be populated with the error message. | ||
| * @deprecated | ||
| */ | ||
| appId: string; | ||
| successfull?: boolean; |
There was a problem hiding this comment.
The change from 'successfull: boolean' to 'successfull?: boolean' introduces a potential issue where the 'successfull' property might be undefined, leading to runtime errors if not handled properly.
| /** | ||
| * Array of connector items matching the query parameters. Each item contains detailed information about a connector and its associated connections. | ||
| */ | ||
| items: Array<{ | ||
| [key: string]: unknown; | ||
| }>; | ||
| items: Array<ConnectorListItemDTO>; |
There was a problem hiding this comment.
The change from 'items: ConnectorListItemDTO' to 'items: Array' may cause issues if the consuming code expects a single object instead of an array.
| @@ -1,4 +1,4 @@ | |||
| export const env = process.env.TEST_ENV || "prod" | |||
| export const env = process.env.TEST_ENV || "staging" | |||
There was a problem hiding this comment.
Default environment changed from 'prod' to 'staging' which could affect production deployments if TEST_ENV is not explicitly set. This is a potentially risky change.
| const connection = await entity2.initiateConnection({ | ||
| appName: "github", | ||
| }); | ||
| expect(connection.connectionStatus).toBe("INITIATED"); |
There was a problem hiding this comment.
Inconsistent auth state due to mismatched requirements for authMode and authScheme. Align their requirements to prevent issues.
| logger.info( | ||
| `🚀 Upgrade available! Your composio-core version (${currentVersionFromPackageJson}) is behind. Latest version: ${latestVersion}.` | ||
| ); |
There was a problem hiding this comment.
Switching from console.warn() to logger.info() reduces visibility of upgrade notifications. Warning level is more appropriate for version upgrade messages to ensure they are noticed.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3cd6021 in 24 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_sXd6i4UsXUUSuIWv
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! Reviewed everything up to 7fd7f3f in 1 minute and 36 seconds
More details
- Looked at
5745lines of code in14files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js/src/sdk/index.ts:129
- Draft comment:
The comment above this line mentionsconsole.warn, but the code useslogger.info. Consider updating the comment for consistency. - Reason this comment was not posted:
Confidence changes required:10%
The change in thecheckForLatestVersionFromNPMmethod to uselogger.infoinstead ofconsole.warnis a good practice for consistent logging. However, the comment above thelogger.infocall still mentionsconsole.warn, which can be misleading. It should be updated to reflect the actual logging method used.
2. js/src/sdk/models/connectedAccounts.ts:184
- Draft comment:
ThereinitiateConnectionmethod should return theConnectionRequestobject after creating it. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. js/src/sdk/models/integrations.ts:174
- Draft comment:
ThegetOrCreateIntegrationmethod correctly usesapiClient.appConnectorV2.getOrCreateConnector, consistent with the introduction of V2 services. - Reason this comment was not posted:
Confidence changes required:10%
ThegetOrCreateIntegrationmethod in theIntegrationsclass usesapiClient.appConnectorV2.getOrCreateConnector, which aligns with the new V2 service introduced in the PR. This method is consistent with the changes and correctly retrieves the app usingapiClient.apps.getApps(). It also handles errors usingCEG.handleAllError, which is consistent with the error handling strategy in the codebase.
Workflow ID: wflow_DQu2L2203QZq9aST
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
himanshu-dixit
changed the title
Important
This pull request enhances the SDK with new DTOs for error handling, introduces methods for managing integrations and connections, and updates models to support new API endpoints.
types.gen.ts.integrations.tsandconnectedAccounts.ts.index.tsandEntity.spec.ts.ZReinitiateConnectionPayloadDtotoconnectedAccount.tsfor reinitiating connections.ZAuthModetointegration.tsfor defining authentication modes.integrations.spec.ts.Entity.spec.ts.checkForLatestVersionFromNPM()inindex.tsto log info instead of warning.This description was created by
for 3cd6021. It will automatically update as commits are pushed.