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 all 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
7 changes: 5 additions & 2 deletions js/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
"keywords": [],
"author": "Utkarsh Dixit <[email protected]>",
"license": "ISC",
"dependencies": {
"pusher-js": "8.4.0-rc2"
},
"devDependencies": {
"@ai-sdk/openai": "^0.0.36",
"@cloudflare/workers-types": "^4.20240718.0",
Expand All @@ -45,7 +48,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 @@ -75,11 +78,11 @@
"open": "^8.4.0",
"openai": "^4.50.0",
"prettier": "^3.4.2",
"pusher-js": "8.4.0-rc2",
"regenerator-runtime": "^0.14.1",
"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
25 changes: 17 additions & 8 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'),
];
9 changes: 9 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,9 @@
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(/declare\s+class\s+/g, 'export declare class ');
content = content.replace(/declare\s+const\s+/g, 'export declare const ');

content = content.replace("export { ACTIONS, APPS, CloudflareToolSet, Composio, LangGraphToolSet, LangchainToolSet, OpenAIToolSet, VercelAIToolSet, Workspace };", '');
fs.writeFileSync('dist/index.d.ts', content);
Comment on lines +1 to +9
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Potential API Breaking Change
The current changes modify the TypeScript declaration file by converting type and class declarations into export statements. This could potentially alter the module's API and affect its consumers.

  • Ensure that all consumers of the module are updated to handle the new export structure.
  • Consider providing a migration guide to assist users in adapting to these changes.
  • Verify that these changes are necessary and align with the overall module design and usage.

This change could break existing consumers if they rely on the previous non-exported declarations. Please review carefully to avoid unintended disruptions. 🚨

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`;

module.exports = {
APPS,
Expand Down
4 changes: 2 additions & 2 deletions js/src/env/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ export class ShellFactory {
}
}

export interface IExecuteActionMetadata {
export type IExecuteActionMetadata = {
entityId?: string | null;
}
};

export class Workspace {
id: string;
Comment on lines 110 to 118
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consider Retaining Interface for Flexibility
The change from an interface to a type for IExecuteActionMetadata could potentially introduce issues if the interface was being extended elsewhere in the codebase. Interfaces in TypeScript offer more flexibility as they can be extended, which is beneficial for maintaining reusable and scalable code.

  • Review the usage of IExecuteActionMetadata throughout the codebase to ensure no existing functionality is broken.
  • If the interface was being extended, consider keeping it as an interface or refactor the code to accommodate the new type definition.

This change could lead to runtime errors or unexpected behavior if not handled carefully. 🛠️

🔧 Suggested Code Diff:

- export type IExecuteActionMetadata = {
+ export interface IExecuteActionMetadata {
  entityId?: string | null;
- };
+ }
📝 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
}
}
export interface IExecuteActionMetadata {
export type IExecuteActionMetadata = {
entityId?: string | null;
}
};
export class Workspace {
id: string;
export interface IExecuteActionMetadata {
entityId?: string | null;
}
export class Workspace {
id: string;
// Additional properties and methods for Workspace
}

Comment on lines 110 to 118
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consideration of Interface to Type Change
The change from an interface to a type for IExecuteActionMetadata could potentially introduce issues if the interface was being extended or used in a way that leverages interface-specific features.

  • Review Usage: Ensure that IExecuteActionMetadata is not being extended elsewhere in the codebase. If it is, this change might break existing functionality.
  • Interface Benefits: Interfaces in TypeScript allow for declaration merging and can be extended, which might be necessary for future scalability.
  • Refactor if Necessary: If the interface was being extended, consider maintaining it as an interface or refactor the code to accommodate the change.

This change should be carefully reviewed to avoid unintended side effects. 🛠️

🔧 Suggested Code Diff:

export interface IExecuteActionMetadata {
  entityId?: string | null;
}
📝 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
}
}
export interface IExecuteActionMetadata {
export type IExecuteActionMetadata = {
entityId?: string | null;
}
};
export class Workspace {
id: string;
export interface IExecuteActionMetadata {
entityId?: string | null;
}

Expand Down
4 changes: 2 additions & 2 deletions js/src/env/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { ExecEnv } from "./factory";

export interface IWorkspaceConfig {
export type IWorkspaceConfig = {
composioAPIKey?: string | null;
composioBaseURL?: string | null;
githubAccessToken?: string | null;
environment?: { [key: string]: string };
}
};

export class WorkspaceConfig<
TConfig extends IWorkspaceConfig = IWorkspaceConfig,
Expand Down
4 changes: 2 additions & 2 deletions js/src/env/docker/workspace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function getFreePort(): Promise<number> {
});
}

export interface IDockerConfig extends IWorkspaceConfig {
export type IDockerConfig = IWorkspaceConfig & {
/** Name of the docker image. */
image?: string;

Expand All @@ -37,7 +37,7 @@ export interface IDockerConfig extends IWorkspaceConfig {

/** Volumes to bind inside the container */
volumes?: { [key: string]: any };
}
};

export class DockerWorkspace extends RemoteWorkspace {
public docker: Docker;
Expand Down
4 changes: 2 additions & 2 deletions js/src/env/e2b/workspace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ const TOOLSERVER_URL = "https://{host}/api";

const ENV_E2B_TEMPLATE = "E2B_TEMPLATE";

export interface IE2BConfig extends IWorkspaceConfig {
export type IE2BConfig = IWorkspaceConfig & {
template?: string;
apiKey?: string;
port?: number;
}
};

export class E2BWorkspace extends RemoteWorkspace {
sandbox: Sandbox | undefined;
Comment on lines 11 to 21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consider Impact of Changing Interface to Type Alias
The change from an interface to a type alias using intersection may have unintended consequences. Interfaces and type aliases in TypeScript have different behaviors, especially regarding extension and merging. This change could affect other parts of the codebase that rely on the previous interface structure.

  • Review all usages of IE2BConfig to ensure compatibility with the new type alias.
  • Consider maintaining the interface if it is extended or implemented elsewhere to avoid potential issues.
  • Test thoroughly to ensure no unexpected behavior is introduced.

This change requires careful consideration to avoid logical errors in the application. 🛠️

🔧 Suggested Code Diff:

- export interface IE2BConfig extends IWorkspaceConfig {
+ export type IE2BConfig = IWorkspaceConfig & {
  template?: string;
  apiKey?: string;
  port?: number;
};
📝 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 ENV_E2B_TEMPLATE = "E2B_TEMPLATE";
export interface IE2BConfig extends IWorkspaceConfig {
export type IE2BConfig = IWorkspaceConfig & {
template?: string;
apiKey?: string;
port?: number;
}
};
export class E2BWorkspace extends RemoteWorkspace {
sandbox: Sandbox | undefined;
export interface IE2BConfig extends IWorkspaceConfig {
template?: string;
apiKey?: string;
port?: number;
}
export class E2BWorkspace extends RemoteWorkspace {
sandbox: Sandbox | undefined;

Comment on lines 11 to 21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consider Impact of Changing Interface to Type Alias
The change from an interface to a type alias using intersection may introduce issues if there are existing implementations relying on the interface's structure. This could affect how the type is extended or implemented, potentially causing issues in existing code that relies on the interface's behavior.

  • Review the usage of IE2BConfig throughout the codebase to ensure compatibility.
  • Consider maintaining the interface if it is being implemented by classes or other interfaces to avoid breaking changes.

This change should be carefully evaluated to ensure it does not disrupt existing functionality. 🛠️

🔧 Suggested Code Diff:

-export type IE2BConfig = IWorkspaceConfig & {
+export interface IE2BConfig extends IWorkspaceConfig {
  template?: string;
  apiKey?: string;
  port?: number;
};
📝 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 ENV_E2B_TEMPLATE = "E2B_TEMPLATE";
export interface IE2BConfig extends IWorkspaceConfig {
export type IE2BConfig = IWorkspaceConfig & {
template?: string;
apiKey?: string;
port?: number;
}
};
export class E2BWorkspace extends RemoteWorkspace {
sandbox: Sandbox | undefined;
export interface IE2BConfig extends IWorkspaceConfig {
template?: string;
apiKey?: string;
port?: number;
};
export class E2BWorkspace extends RemoteWorkspace {
sandbox: Sandbox | undefined;

Expand Down
4 changes: 2 additions & 2 deletions js/src/sdk/actionRegistry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import apiClient from "../sdk/client/client";
import { CEG } from "./utils/error";

type ExecuteRequest = Omit<ActionProxyRequestConfigDTO, "connectedAccountId">;
export interface CreateActionOptions {
export type CreateActionOptions = {
actionName?: string;
toolName?: string;
description?: string;
Expand All @@ -23,7 +23,7 @@ export interface CreateActionOptions {
authCredentials: Record<string, any> | undefined,
executeRequest: (data: ExecuteRequest) => Promise<any>
) => Promise<Record<string, any>>;
}
};

interface ParamsSchema {
definitions: {
Expand Down
4 changes: 2 additions & 2 deletions js/src/sdk/client/core/CancelablePromise.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ export class CancelError extends Error {
}
}

export interface OnCancel {
export type OnCancel = {
readonly isResolved: boolean;
readonly isRejected: boolean;
readonly isCancelled: boolean;

(cancelHandler: () => void): void;
}
};

export class CancelablePromise<T> implements Promise<T> {
private _isResolved: boolean;
Comment on lines 11 to 23
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Change from Interface to Type for OnCancel
The modification from an interface to a type for OnCancel could potentially disrupt existing code that extends or implements this interface. This change might lead to runtime errors or unexpected behavior if not handled properly.

  • Ensure that OnCancel is not being extended or implemented elsewhere in the codebase.
  • If it is, consider refactoring those parts to accommodate the new type definition.
  • Conduct thorough testing to verify that the change does not introduce any new issues.

This change requires careful consideration to avoid breaking existing functionality. 🛠️

Comment on lines 11 to 23
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consider Reverting OnCancel to an Interface
The change from an interface to a type for OnCancel could potentially introduce issues if the interface was being extended or implemented elsewhere. Interfaces offer more flexibility for extension, which is crucial for maintaining extensibility in a codebase.

  • Review the usage of OnCancel throughout the codebase to ensure no existing functionality is broken.
  • If extensibility is required, consider reverting OnCancel back to an interface.

This change could impact the code's robustness and maintainability. 🛠️

🔧 Suggested Code Diff:

-export type OnCancel = {
+export interface OnCancel {
  readonly isResolved: boolean;
  readonly isRejected: boolean;
  readonly isCancelled: boolean;

  (cancelHandler: () => void): void;
-}+;
};
📝 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
}
}
export interface OnCancel {
export type OnCancel = {
readonly isResolved: boolean;
readonly isRejected: boolean;
readonly isCancelled: boolean;
(cancelHandler: () => void): void;
}
};
export class CancelablePromise<T> implements Promise<T> {
private _isResolved: boolean;
export interface OnCancel {
readonly isResolved: boolean;
readonly isRejected: boolean;
readonly isCancelled: boolean;
(cancelHandler: () => void): void;
}
export class CancelablePromise<T> implements Promise<T> {
private _isResolved: boolean;

Expand Down
97 changes: 55 additions & 42 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,56 +297,69 @@ 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 isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;

if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
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,
"\n",
"fields:",
authScheme.fields
);
}

throw new Error(`Please pass authMode and authConfig.`);
}
if (!integrationId && !appName) {
throw CEG.getCustomError(SDK_ERROR_CODES.COMMON.INVALID_PARAMS_PASSED, {
message: "Please pass appName or integrationId",
description:
"We need atleast one of the params to initiate a connection",
});
}

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

let integration = integrationId
const isIntegrationIdPassed = !!integrationId;
Comment on lines 297 to +311
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Re-evaluate Removal of Authentication Checks
The removal of the logic for checking isTestConnectorAvailable and handling authMode could introduce a logical error if these checks are necessary for the correct functioning of the integration process. These checks seem to ensure that the application has the necessary authentication schemes available before proceeding. Removing them without ensuring they are redundant or handled elsewhere could lead to incorrect behavior or errors in the integration process.

Actionable Suggestions:

  • Review the necessity of the removed checks: Ensure that the logic for isTestConnectorAvailable and authMode is not required for the integration process. If they are necessary, reinstate them or ensure they are handled elsewhere in the code.
  • Provide Documentation: If these checks are indeed redundant, add comments or documentation explaining why they are no longer needed to prevent confusion for future developers.

Potential Risks:

  • Authentication Failures: Without these checks, there might be scenarios where the integration fails due to missing authentication configurations.
  • Increased Debugging Time: Future developers might spend unnecessary time debugging issues related to missing authentication checks.

Please ensure these considerations are addressed to maintain the robustness of the integration process. 🛠️

Comment on lines 297 to +311
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 Authentication Logic for App Connections
The removal of the authentication logic is a critical issue that could lead to failures in establishing necessary connections for apps that require authentication. The previous logic ensured that if an app required authentication, it would either use an existing test connector or create a new integration with the appropriate authentication scheme.

Actionable Suggestions:

  • Reintroduce Authentication Checks: Ensure that the logic to check for available authentication schemes is reintroduced. This is crucial for apps that require authentication.
  • Integration Creation: If authMode is provided, create a new integration with the specified authentication scheme and configuration. If not, ensure that the app can use an existing test connector or handle the absence of authentication gracefully.
  • Error Handling: Provide clear error messages if authentication is required but not provided, guiding the user to supply the necessary parameters.

Considerations:

  • Review the app's requirements for authentication and ensure that the logic aligns with these requirements.
  • Test the changes thoroughly to ensure that connections are established correctly for both authenticated and non-authenticated apps.

🔧 Suggested Code Diff:

+      // Reintroduce authentication logic
+      const app = await this.apps.get({ appKey: appName });
+      const isTestConnectorAvailable =
+        app.testConnectors && app.testConnectors.length > 0;
+
+      if (!isTestConnectorAvailable && app.no_auth === false) {
+        if (!authMode) {
+          logger.debug(
+            "Auth schemes not provided, available auth schemes and authConfig"
+          );
+          for (const authScheme of app.auth_schemes) {
+            logger.debug(
+              "autheScheme:",
+              authScheme.name,
+              "\n",
+              "fields:",
+              authScheme.fields
+            );
+          }
+          throw new Error(`Please pass authMode and authConfig.`);
+        }
+        // Create integration with authMode
+        integration = await this.integrations.create({
+          appId: app.appId!,
+          name: `integration_${timestamp}`,
+          authScheme: authMode,
+          authConfig: authConfig,
+          useComposioAuth: false,
+        });
+      }
📝 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 isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
// @ts-ignore
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,
"\n",
"fields:",
authScheme.fields
);
}
throw new Error(`Please pass authMode and authConfig.`);
}
if (!integrationId && !appName) {
throw CEG.getCustomError(SDK_ERROR_CODES.COMMON.INVALID_PARAMS_PASSED, {
message: "Please pass appName or integrationId",
description:
"We need atleast one of the params to initiate a connection",
});
}
/* Get the integration */
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId
const isIntegrationIdPassed = !!integrationId;
const { redirectUrl, labels } = data.config || {};
// Reintroduce authentication logic
const app = await this.apps.get({ appKey: appName });
const isTestConnectorAvailable =
app.testConnectors && app.testConnectors.length > 0;
if (!isTestConnectorAvailable && app.no_auth === false) {
if (!authMode) {
logger.debug(
"Auth schemes not provided, available auth schemes and authConfig"
);
for (const authScheme of app.auth_schemes) {
logger.debug(
"autheScheme:",
authScheme.name,
"\n",
"fields:",
authScheme.fields
);
}
throw new Error(`Please pass authMode and authConfig.`);
}
// Create integration with authMode
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
}
if (!integrationId && !appName) {
throw CEG.getCustomError(SDK_ERROR_CODES.COMMON.INVALID_PARAMS_PASSED, {
message: "Please pass appName or integrationId",
description:
"We need at least one of the params to initiate a connection",
});
}
/* Get the integration */
const timestamp = new Date().toISOString().replace(/[-:.]/g, "");
let integration = integrationId;
// 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}`,
useComposioAuth: true,
});
}

Comment on lines 297 to +311
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Re-evaluate Removal of Authentication Checks
The removal of the logic checking for isTestConnectorAvailable and authMode is concerning as it might lead to unintended behavior, especially in authentication flows. These checks seem to ensure that the application handles authentication correctly, particularly when test connectors are not available.

Actionable Suggestions:

  • Reassess the necessity of these checks: Determine if the conditions they were checking are still relevant in the current context. If they are, reintegrate them into the code.
  • Provide Documentation: If these checks are no longer needed, document the reasons for their removal and how the new logic compensates for their absence.
  • Testing: Ensure thorough testing of authentication flows to confirm that the removal does not introduce any regressions or errors.

By addressing these points, you can maintain the integrity of the authentication process and prevent potential issues. 🛡️

let integration = isIntegrationIdPassed
? 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 (isIntegrationIdPassed && !integration) {
throw CEG.getCustomError(SDK_ERROR_CODES.COMMON.INVALID_PARAMS_PASSED, {
message: "Integration not found",
description: "The integration with the given id does not exist",
});
}

if (!integration && !authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
useComposioAuth: true,
});
/* If integration is not found, create a new integration */
if (!isIntegrationIdPassed) {
const app = await this.apps.get({ appKey: appName! });

if (authMode) {
integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
authScheme: authMode,
authConfig: authConfig,
useComposioAuth: false,
});
} else {
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) {
logger.debug(
"authScheme:",
authScheme.name,
"\n",
"fields:",
authScheme.fields
);
}

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

integration = await this.integrations.create({
appId: app.appId!,
name: `integration_${timestamp}`,
useComposioAuth: true,
});
}
}

// Initiate the connection process
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
4 changes: 2 additions & 2 deletions js/src/sdk/utils/pusher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const PUSHER_CLUSTER = "mt1";

type PusherClient = any;

export interface TriggerData {
export type TriggerData = {
appName: string;
clientId: number;
payload: {};
Comment on lines 5 to 11
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential Issue:

Consider Retaining Interface for Flexibility
The change from an interface to a type for TriggerData could lead to potential issues if there are existing implementations that rely on the flexibility of interfaces. Interfaces in TypeScript are more extendable and can be implemented by classes, which might be necessary for future scalability or current usage patterns.

  • Review the usage of TriggerData across the codebase to ensure no existing implementations are affected.
  • If structural typing or future extensibility is required, consider retaining the interface.

This change could lead to unexpected behavior if not thoroughly checked. Please ensure that the change aligns with the overall design and usage of TriggerData. 🛠️

🔧 Suggested Code Diff:

- export type TriggerData = {
+ export interface TriggerData {
  appName: string;
  clientId: number;
  payload: {};
📝 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 PusherClient = any;
export interface TriggerData {
export type TriggerData = {
appName: string;
clientId: number;
payload: {};
export interface TriggerData {
appName: string;
clientId: number;
payload: Record<string, unknown>;
}

Expand All @@ -23,7 +23,7 @@ export interface TriggerData {
status: string;
};
};
}
};

export class PusherUtils {
static pusherClient: PusherClient;
Expand Down
Loading