Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initiate connection and TS export #1025

Merged
merged 14 commits into from
Dec 19, 2024
Merged

feat: initiate connection and TS export #1025

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6f270ce
feat: initiateConnection and export
himanshu-dixit Dec 17, 2024
5286754
feat: export type
himanshu-dixit Dec 17, 2024
eb5c55c
feat: entity change
himanshu-dixit Dec 17, 2024
06192ee
feat: beta
himanshu-dixit Dec 17, 2024
9dd9473
feat: remove file
himanshu-dixit Dec 17, 2024
ce8ccfe
feat: intercace to type
himanshu-dixit Dec 17, 2024
7f2b415
featr: SP
himanshu-dixit Dec 17, 2024
ed5cc67
feat: beta
himanshu-dixit Dec 17, 2024
2324464
feat: lock file
himanshu-dixit Dec 17, 2024
e06f1ce
feat: update version
himanshu-dixit Dec 17, 2024
752fa8f
feat: error message
himanshu-dixit Dec 17, 2024
6f7bd04
feat: entity
himanshu-dixit Dec 18, 2024
f206f40
Merge branch 'master' into ft-initiateConnection-and-ts-export
himanshu-dixit Dec 18, 2024
dcde3f1
Merge branch 'master' into ft-initiateConnection-and-ts-export
himanshu-dixit Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions js/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const fs = require('fs');
let content = fs.readFileSync('dist/index.d.ts', 'utf8');
content = content.replace(/type\s+([A-Za-z0-9_]+)\s*=/g, 'export type $1 =');
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*=/g, 'export interface $1 =');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern for replacing interfaces is incorrect. It should not have an equal sign after the interface name. Use:

Suggested change
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*=/g, 'export interface $1 =');
content = content.replace(/interface\s+([A-Za-z0-9_]+)/g, 'export interface $1');

fs.writeFileSync('dist/index.d.ts', content);
5 changes: 3 additions & 2 deletions js/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "composio-core",
"version": "0.4.5",
"version": "0.4.6-beta",
"description": "",
"main": "dist/index.js",
"scripts": {
Expand Down Expand Up @@ -45,7 +45,7 @@
"@rollup/plugin-json": "^6.1.0",
"@rollup/plugin-node-resolve": "^15.2.3",
"@rollup/plugin-replace": "^6.0.1",
"@rollup/plugin-typescript": "^11.1.6",
"@rollup/plugin-typescript": "^12",
"@swc/core": "^1.7.10",
"@swc/helpers": "^0.5.12",
"@types/cli-progress": "^3.11.6",
Expand Down Expand Up @@ -80,6 +80,7 @@
"resolve-package-path": "^4.0.3",
"rollup": "^4.9.1",
"rollup-plugin-dts": "^6.1.0",
"rollup-plugin-ignore": "^1.0.10",
"rollup-plugin-terser": "^7.0.2",
"ts-jest": "^29.1.2",
"ts-loader": "^9.5.1",
Expand Down
18 changes: 13 additions & 5 deletions js/pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 0 additions & 5 deletions js/rollup.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,5 @@ export default [
]
},
...generateBundleAndTypeBundles('index'),
...generateBundleAndTypeBundles('frameworks/cloudflare'),
...generateBundleAndTypeBundles('frameworks/langchain'),
...generateBundleAndTypeBundles('frameworks/openai'),
...generateBundleAndTypeBundles('frameworks/langgraph'),
...generateBundleAndTypeBundles('frameworks/vercel'),
...generateBundleAndTypeBundles('sdk/index'),
];
5 changes: 5 additions & 0 deletions js/scripts/replace-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const fs = require('fs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is identical to the existing one. Consider using the existing script instead of duplicating it.

  • Type declaration export modifier script (index.js)

let content = fs.readFileSync('dist/index.d.ts', 'utf8');
content = content.replace(/type\s+([A-Za-z0-9_]+)\s*=/g, 'export type $1 =');
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*=/g, 'export interface $1 =');
fs.writeFileSync('dist/index.d.ts', content);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure Robustness of Regex for TypeScript Declarations
The current implementation uses regex to modify TypeScript declaration files, which can be error-prone. It's crucial to ensure that the regex patterns are comprehensive and do not inadvertently alter unintended parts of the file.

  • Verify that the regex patterns correctly match all valid 'type' and 'interface' declarations.
  • Consider edge cases such as multiline declarations or comments that might be affected by the regex.
  • Add tests to validate the changes in various scenarios to prevent potential compilation or runtime issues.
  • Review the necessity of these changes to ensure they align with the project's TypeScript strategy.

This approach will help maintain the integrity of the TypeScript declarations and prevent potential errors. 🛠️

🔧 Suggested Code Diff:

const fs = require('fs');
let content = fs.readFileSync('dist/index.d.ts', 'utf8');
// Improved regex to handle multiline and comments
content = content.replace(/type\s+([A-Za-z0-9_]+)\s*=\s*([^;]+);/gm, 'export type $1 = $2;');
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*{/gm, 'export interface $1 {');
fs.writeFileSync('dist/index.d.ts', content);
📝 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 fs = require('fs');
let content = fs.readFileSync('dist/index.d.ts', 'utf8');
content = content.replace(/type\s+([A-Za-z0-9_]+)\s*=/g, 'export type $1 =');
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*=/g, 'export interface $1 =');
fs.writeFileSync('dist/index.d.ts', content);
const fs = require('fs');
try {
let content = fs.readFileSync('dist/index.d.ts', 'utf8');
// Improved regex to handle multiline and comments
content = content.replace(/type\s+([A-Za-z0-9_]+)\s*=\s*([^;]+);/gm, 'export type $1 = $2;');
content = content.replace(/interface\s+([A-Za-z0-9_]+)\s*{/gm, 'export interface $1 {');
fs.writeFileSync('dist/index.d.ts', content);
} catch (error) {
console.error('Error processing TypeScript declarations:', error);
}

2 changes: 1 addition & 1 deletion js/setup_cli.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ mv temp_file dist/cli/index

rm dist/cli/index.js


node scripts/replace-type.js
2 changes: 1 addition & 1 deletion js/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const ACTIONS = {
// actions list end here
};

const COMPOSIO_VERSION = `0.4.5`;
const COMPOSIO_VERSION = `0.4.6-beta`;

module.exports = {
APPS,
Expand Down
52 changes: 24 additions & 28 deletions js/src/sdk/models/Entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const ZExecuteActionParams = z.object({
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;

const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Optional 'appName' Parameter in ZInitiateConnectionParams
The recent change to make the 'appName' parameter optional in the ZInitiateConnectionParams schema could introduce logical errors if the application logic assumes 'appName' is always provided. This could potentially break existing functionality that relies on 'appName' being mandatory.

  • Review the usage of ZInitiateConnectionParams throughout the codebase to ensure that this change does not introduce any logical errors.
  • Consider adding validation or default values to handle cases where 'appName' is not provided.
  • Ensure that any dependent functionality is updated to handle the optional nature of 'appName'.

This change requires careful consideration to avoid unintended side effects. 🛠️

🔧 Suggested Code Diff:

 type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;

const ZInitiateConnectionParams = z.object({
-  appName: z.string(),
+  appName: z.string().optional().default('defaultAppName'),
  authConfig: z.record(z.any()).optional(),
  integrationId: z.string().optional(),
  authMode: z.string().optional(),
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().optional().default('defaultAppName'),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
});

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure 'appName' Optionality is Handled
The change to make 'appName' optional could lead to logical errors if the rest of the codebase assumes it is always present.

  • Review all instances where 'appName' is used to ensure they can handle it being undefined.
  • Consider providing a default value or handling the absence of 'appName' gracefully to prevent potential bugs.
  • Ensure that any documentation or API contracts are updated to reflect this change.

This change requires careful consideration to avoid introducing unexpected behavior. 🛠️

🔧 Suggested Code Diff:

 type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;

const ZInitiateConnectionParams = z.object({
-  appName: z.string(),
+  appName: z.string().optional().default('defaultAppName'),
  authConfig: z.record(z.any()).optional(),
  integrationId: z.string().optional(),
  authMode: z.string().optional(),
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().optional().default('defaultAppName'),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
});

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure Optional 'appName' is Handled Appropriately
The recent change to make the appName field optional in ZInitiateConnectionParams could lead to logical errors if the rest of the codebase assumes appName is always present.

  • Review all instances where ZInitiateConnectionParams is used to ensure they handle cases where appName might be undefined.
  • Consider whether appName is critical for the application's functionality. If it is, it might be better to keep it mandatory.
  • If making appName optional is necessary, ensure that default values or error handling are implemented where needed.

This change requires careful consideration to avoid introducing bugs. 🛠️

🔧 Suggested Code Diff:

 type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;

 const ZInitiateConnectionParams = z.object({
-  appName: z.string(),
+  appName: z.string().optional(),
   authConfig: z.record(z.any()).optional(),
   integrationId: z.string().optional(),
   authMode: z.string().optional(),
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
});
// Ensure that wherever ZInitiateConnectionParams is used, appName is checked for undefined
// Example usage:
function handleConnection(params: z.infer<typeof ZInitiateConnectionParams>) {
if (!params.appName) {
throw new Error('appName is required for this operation.');
}
// Proceed with the operation assuming appName is defined
}

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure Optional 'appName' is Handled Appropriately
The recent change to make the appName parameter optional in ZInitiateConnectionParams could lead to potential issues if the rest of the codebase assumes appName is always present.

  • Review all instances where ZInitiateConnectionParams is used to ensure that the absence of appName is handled correctly.
  • Consider providing a default value for appName or implementing logic to manage cases where it is not provided.
  • Ensure that any dependent logic or functions that rely on appName are updated to handle its optional nature.

This change is crucial to prevent unexpected behavior and maintain the integrity of the application logic. 🛠️

🔧 Suggested Code Diff:

 type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;

const ZInitiateConnectionParams = z.object({
-  appName: z.string(),
+  appName: z.string().optional().default('defaultAppName'),
  authConfig: z.record(z.any()).optional(),
  integrationId: z.string().optional(),
  authMode: z.string().optional(),
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().optional().default('defaultAppName'),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
});

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Ensure Valid Configuration for Connection Parameters
The recent change to make appName optional introduces a potential logical error by allowing both appName and integrationId to be optional. This could lead to invalid configurations. To prevent this, it's crucial to implement a validation check ensuring that at least one of these fields is provided.

  • Consider adding a custom validation function to enforce this rule.
  • This will help maintain the integrity of the configuration and prevent potential runtime errors.

Please address this to ensure robust and error-free code. 🚀

🔧 Suggested Code Diff:

const ZInitiateConnectionParams = z.object({
  appName: z.string().optional(),
  authConfig: z.record(z.any()).optional(),
  integrationId: z.string().optional(),
  authMode: z.string().optional(),
}).refine(data => data.appName || data.integrationId, {
  message: "Either 'appName' or 'integrationId' must be provided.",
});
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
}).refine(data => data.appName || data.integrationId, {
message: "Either 'appName' or 'integrationId' must be provided.",
});

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure 'appName' Optionality is Handled Gracefully
The recent change to make the 'appName' parameter optional in ZInitiateConnectionParams could introduce logical errors if the rest of the codebase assumes 'appName' is always present. This is particularly critical if 'appName' is used in key operations or decision-making processes.

Actionable Steps:

  • Codebase Audit: Conduct a thorough review of all instances where 'appName' is used to ensure that the absence of this parameter does not lead to runtime errors or unexpected behavior.
  • Validation: Implement validation checks where 'appName' is critical. This could involve throwing an error or providing a default value if 'appName' is not supplied.
  • Testing: Add test cases to cover scenarios where 'appName' is missing to ensure the application behaves as expected.

By addressing these points, you can mitigate potential issues arising from this change. 🛠️

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure 'appName' Optionality is Handled Properly
The recent change to make the 'appName' parameter optional in the ZInitiateConnectionParams object introduces a potential logical error. This could lead to unexpected behavior if the application logic assumes 'appName' is always present.

Actionable Steps:

  • Code Review: Thoroughly review the codebase to identify all instances where ZInitiateConnectionParams is used. Ensure that these instances handle the case where 'appName' might be undefined.
  • Default Values: Consider setting a default value for 'appName' if it is not provided. This can prevent potential null reference errors.
  • Validation Checks: Implement validation checks to ensure that the absence of 'appName' does not lead to errors in the application logic.

By addressing these points, you can mitigate the risk of unexpected behavior due to the optionality of 'appName'.

🔧 Suggested Code Diff:

const ZInitiateConnectionParams = z.object({
-  appName: z.string().optional(),
+  appName: z.string().default('defaultAppName'),
  authConfig: z.record(z.any()).optional(),
  integrationId: z.string().optional(),
  authMode: z.string().optional(),
📝 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
type TExecuteActionParams = z.infer<typeof ZExecuteActionParams>;
const ZInitiateConnectionParams = z.object({
appName: z.string(),
appName: z.string().optional(),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
const ZInitiateConnectionParams = z.object({
appName: z.string().default('defaultAppName'),
authConfig: z.record(z.any()).optional(),
integrationId: z.string().optional(),
authMode: z.string().optional(),
});

Comment on lines 27 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure 'appName' Optionality is Handled Appropriately
The recent change to make the appName parameter optional in ZInitiateConnectionParams could lead to logical errors if the rest of the codebase assumes appName is always present. This change requires a thorough review of all instances where appName is used to ensure that the code can handle cases where it is undefined.

Actionable Steps:

  • Code Review: Conduct a comprehensive review of the codebase to identify all usages of appName.
  • Validation: Implement checks or default values where appName is critical to prevent potential null reference errors.
  • Testing: Add test cases to cover scenarios where appName is not provided to ensure robustness.

By following these steps, you can mitigate the risk of introducing bugs due to this change. 🛠️

Expand Down Expand Up @@ -297,21 +297,36 @@ export class Entity {
ZInitiateConnectionParams.parse(data);
const { redirectUrl, labels } = data.config || {};

// Get the app details from the client
const app = await this.apps.get({ appKey: appName });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");

const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;

if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
if (!integration && authMode) {
const app = await this.apps.get({ appKey: appName! });

integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Logical Error in Integration Setup
The recent changes introduce a potential logical error by altering the condition for fetching app details and creating an integration. This could lead to incorrect behavior if the app details are required for further processing, even when an integration is already present.

  • Ensure that app details are fetched when necessary, regardless of the presence of an integration.
  • Consider scenarios where both app details and integration are required and adjust the condition accordingly to prevent failures in establishing connections or incorrect integration setups.

Please review the logic to ensure it aligns with the intended functionality. 🛠️

🔧 Suggested Code Diff:

      let integration = integrationId
        ? await this.integrations.get({ integrationId: integrationId })
        : null;

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

      if (!integration && authMode) {
-        const app = await this.apps.get({ appKey: appName! });

        integration = await this.integrations.create({
          appId: app.appId!,
📝 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
ZInitiateConnectionParams.parse(data);
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
if (!integration && authMode) {
const app = await this.apps.get({ appKey: appName! });
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
const { redirectUrl, labels } = data.config || {};
// Fetch the app details from the client
const app = await this.apps.get({ appKey: appName! });
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!integration && authMode) {
integration = await this.integrations.create({
appId: app.appId!,
});
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Logical Error in Integration and Authentication Check
The recent changes introduce a potential logical error by modifying the conditions for checking integration availability and authentication mode. The original logic included checks for test connectors and the no_auth flag, which are now removed. This could lead to incorrect behavior if the new conditions do not align with the intended functionality.

  • Ensure that the new condition accurately reflects the intended behavior.
  • Verify that removing the test connector check and no_auth flag does not cause regressions or unintended side effects.
  • Consider adding tests to validate the new logic and ensure it aligns with the expected outcomes.

🛠️ Actionable Steps:

  • Review the logic thoroughly to confirm the new condition is correct.
  • Add unit tests to cover scenarios affected by this change.

🔧 Suggested Code Diff:

      let integration = integrationId
        ? await this.integrations.get({ integrationId: integrationId })
        : null;

-      if (!integration && authMode) {
+      if (!integration && (authMode || (app && app.testConnectors && app.testConnectors.length > 0 && app.no_auth === false))) {
        const app = await this.apps.get({ appKey: appName! });

        integration = await this.integrations.create({
          appId: app.appId!,
          name: `integration_${timestamp}`,
📝 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
ZInitiateConnectionParams.parse(data);
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
if (!integration && authMode) {
const app = await this.apps.get({ appKey: appName! });
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName! });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!integration && (authMode || (app && app.testConnectors && app.testConnectors.length > 0 && app.no_auth === false))) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
// Additional properties for integration creation can be added here
});
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Logical Error in Integration and Authentication Handling
The recent changes introduce a potential logical error in the way integration availability and authentication mode are handled. This could lead to incorrect behavior, especially in scenarios where the integration is not available and the authentication mode is required.

  • Ensure that the logic correctly checks for both the presence of an integration and the necessity of an authentication mode.
  • Consider restructuring the conditions to handle cases where the integration is not available and authentication is mandatory.
  • Additional checks might be necessary to prevent potential authentication failures.

Please review the logic to ensure it aligns with the intended functionality and does not inadvertently cause authentication issues.

🔧 Suggested Code Diff:

      let integration = integrationId
        ? await this.integrations.get({ integrationId: integrationId })
        : null;

-      if (!integration && authMode) {
+      if (!integration && (authMode || someOtherCondition)) {
        const app = await this.apps.get({ appKey: appName! });

        integration = await this.integrations.create({
          appId: app.appId!,
          name: `integration_${timestamp}`,
📝 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
ZInitiateConnectionParams.parse(data);
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
if (!integration && authMode) {
const app = await this.apps.get({ appKey: appName! });
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!integration && (authMode || someOtherCondition)) {
const app = await this.apps.get({ appKey: appName! });
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
// Ensure additional necessary fields are included here
});
}
// Additional logic to handle cases where integration is available or other conditions
// Ensure proper error handling and logging for any failures

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Logical Error in Integration Creation Logic
The recent changes introduce a potential logical error in the integration creation logic. The condition for checking the availability of an integration and the authentication mode has been altered, which might lead to incorrect behavior. This could result in integrations being created when they shouldn't be, or vice versa, especially affecting authentication scenarios.

  • Ensure that the logic accurately reflects the intended behavior.
  • Double-check the conditions to prevent unintended integration creation or omission.
  • Consider the implications of the authMode and integrationId checks to maintain correct application behavior.

Please review the logic and adjust the conditions to align with the intended functionality. 🛠️

🔧 Suggested Code Diff:

      let integration = integrationId
        ? await this.integrations.get({ integrationId: integrationId })
        : null;

-      if (!integration && authMode) {
+      if (!integration && (authMode || someOtherCondition)) {
        const app = await this.apps.get({ appKey: appName! });

        integration = await this.integrations.create({
          appId: app.appId!,
          name: `integration_${timestamp}`,
📝 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
ZInitiateConnectionParams.parse(data);
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
if (!integration && authMode) {
const app = await this.apps.get({ appKey: appName! });
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
const { redirectUrl, labels } = data.config || {};
// Get the app details from the client
const app = await this.apps.get({ appKey: appName! });
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
// Ensure integration is created only when necessary
if (!integration && (authMode || someOtherCondition)) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
// Additional parameters can be added here if needed
});
}
// Further processing with the integration object can be done here

authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
}

if (!integration && !authMode) {
const app = await this.apps.get({ appKey: appName! });

const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;

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) {
// @ts-ignore
logger.debug(
"autheScheme:",
authScheme.name,
Expand All @@ -323,25 +338,6 @@ export class Entity {

throw new Error(`Please pass authMode and authConfig.`);
}
}

const timestamp = new Date().toISOString().replace(/[-:.]/g, "");

let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
// Create a new integration if not provided
if (!integration && authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
}

if (!integration && !authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Reintroduce Integration Validation Logic
The recent code changes have removed critical validation and creation logic for the integration object. This can lead to scenarios where integrations are not properly set up, causing potential failures in the connection setup process.

  • Ensure that the code checks for an existing integrationId and retrieves the integration if it exists.
  • If integrationId is not provided, and authMode is available, create a new integration to maintain a valid setup.
  • This logic is crucial for ensuring that the integration setup is complete and functional.

Please reintroduce the removed logic to prevent any disruptions in the integration process. 🚀

🔧 Suggested Code Diff:

      const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
      let integration = integrationId
        ? await this.integrations.get({ integrationId: integrationId })
        : null;
      // Create a new integration if not provided
      if (!integration && authMode) {
        integration = await this.integrations.create({
          appId: app.appId!,
          name: `integration_${timestamp}`,
          authScheme: authMode,
          authConfig: authConfig,
          useComposioAuth: false,
        });
      }

      if (!integration && !authMode) {
        throw new Error(`Please pass authMode and authConfig.`);
      }
📝 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
throw new Error(`Please pass authMode and authConfig.`);
}
}
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
// Create a new integration if not provided
if (!integration && authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
}
if (!integration && !authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId
? await this.integrations.get({ integrationId: integrationId })
: null;
// Create a new integration if not provided
if (!integration && authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
}
if (!integration && !authMode) {
throw new Error(`Please pass authMode and authConfig.`);
}

Expand Down
1 change: 0 additions & 1 deletion js/src/sdk/models/connectedAccounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { BackendClient } from "./backendClient";
import { Integrations } from "./integrations";
import { Apps } from "./apps";
import { CEG } from "../utils/error";
import { SDK_ERROR_CODES } from "../utils/errors/src/constants";
import { TELEMETRY_LOGGER } from "../utils/telemetry";
import { TELEMETRY_EVENTS } from "../utils/telemetry/events";

Expand Down
Loading