feat: add action registry and unit tests in JS SDK #775
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5b65639 in 1 minute and 3 seconds
More details
- Looked at
1007lines of code in6files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js/src/sdk/actionRegistry.spec.ts:62
- Draft comment:
The test case description is misleading. It should mention that the error is due to the missingactionName, not the anonymous function. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The test case description does mention both the anonymous function and the missingactionName, so the comment might be incorrect. The description seems to accurately reflect the test case's purpose, which is to check for an error when an anonymous function is used without specifying anactionName. The comment might be suggesting a more precise wording, but the current description is not misleading.
I might be overlooking the possibility that the description could be more precise, even if it's not misleading. The comment could be suggesting a refinement rather than pointing out an error.
Even if the comment suggests a refinement, the current description is not misleading, and the test case is understandable. Therefore, the comment might not be necessary.
The comment should be deleted because the test case description is not misleading and accurately reflects the test's purpose.
2. js/src/sdk/actionRegistry.ts:73
- Draft comment:
Consider usingtoLocaleLowerCase()consistently for locale-aware case conversion, as used increateAction. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out an inconsistency in the use oftoLowerCase()vstoLocaleLowerCase(). This is a valid code quality suggestion, as using consistent methods for similar operations can prevent locale-related bugs. The comment is actionable and clear, suggesting a specific change to improve code consistency.
The comment assumes thattoLocaleLowerCase()is preferable without context on whether locale-specific behavior is needed. If locale-specific behavior is not required,toLowerCase()might be more appropriate.
Even if locale-specific behavior is not required, usingtoLocaleLowerCase()consistently can prevent future issues if the code is used in a locale-sensitive context.
The comment is valid and suggests a clear, actionable improvement to the code. It should be kept.
3. js/src/sdk/base.toolset.ts:223
- Draft comment:
Replacethis.customActionswiththis.customActionRegistryto access the correct registry. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_SlH3rtELnzSybucs
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.
| baseUrl: connection.connectionParams?.baseUrl, | ||
| } | ||
| } | ||
| if (typeof callback !== "function") { |
There was a problem hiding this comment.
The check for typeof callback !== "function" is redundant here as it is already checked in createAction.
Suggested change
| if (typeof callback !== "function") { |
js/src/sdk/base.toolset.ts
Outdated
| @@ -233,7 +271,21 @@ export class ComposioToolSet { | |||
| } | |||
| } | |||
| const uniqueLocalActions = Array.from(localActions.values()); | |||
| const toolsActions = [...apps.items!, ...uniqueLocalActions]; | |||
|
|
|||
| const toolsWithCustomActions = Array.from(this.customActions.values()).filter((action: any) => { | |||
There was a problem hiding this comment.
Replace this.customActions with this.customActionRegistry to access the correct registry.
Suggested change
| const toolsWithCustomActions = Array.from(this.customActions.values()).filter((action: any) => { | |
| const toolsWithCustomActions = Array.from(this.customActionRegistry.values()).filter((action: any) => { |
| @@ -112,9 +112,6 @@ export class ConnectionRequest { | |||
| */ | |||
| async getAuthInfo(data: GetConnectionInfoData["path"]): Promise<GetConnectionInfoResponse> { | |||
| const res = await client.connections.getConnectionInfo({ path: data }); | |||
There was a problem hiding this comment.
Consider adding error handling for the API call to prevent unhandled promise rejections.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0c65fc7 in 33 seconds
More details
- Looked at
196lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/actionRegistry.ts:29
- Draft comment:
Typo in property namecomosoioSchema. It should becomposioSchema. This typo is present in multiple places (lines 52, 53, 76). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is pointing out a typo that was corrected in the diff. Since the typo has been fixed in all mentioned instances, the comment is no longer relevant. The issue it raises has already been resolved by the changes in the diff.
I might be missing if there are other instances of the typo outside the provided diff, but based on the diff, the issue seems resolved.
The task is to focus on the changes in the diff, and the typo issue has been addressed there. Therefore, the comment is not needed.
The comment should be deleted because the typo it points out has already been corrected in the diff.
2. js/src/sdk/base.toolset.ts:254
- Draft comment:
Unnecessary use of.toLocaleLowerCase()on string literal "custom". Consider removing it for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Inbase.toolset.ts, the methodgetToolsSchemahas a filter condition that checks for tags with a hardcoded value "custom". This should be case-insensitive, and the current implementation already converts both sides to lowercase, which is correct. However, the.toLocaleLowerCase()method is used unnecessarily on a string literal, which is redundant.
3. js/src/sdk/base.toolset.ts:248
- Draft comment:
Unnecessary use of.toLocaleLowerCase()on string literal "custom". Consider removing it for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Inbase.toolset.ts, thegetToolsSchemamethod has a filter condition that checks for actions usingfilters.actions.includes(action.metadata.actionName!). This should be case-insensitive, and the current implementation correctly converts both sides to lowercase. However, the.toLocaleLowerCase()method is used unnecessarily on a string literal, which is redundant.
4. js/src/sdk/base.toolset.ts:198
- Draft comment:
Unnecessary use of.toLocaleLowerCase()on string literal "custom". Consider removing it for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Inbase.toolset.ts, thegetToolsSchemamethod has a filter condition that checks for actions usingfilters.actions.includes(action.metadata.actionName!). This should be case-insensitive, and the current implementation correctly converts both sides to lowercase. However, the.toLocaleLowerCase()method is used unnecessarily on a string literal, which is redundant.
Workflow ID: wflow_i7X5snR5Rnw1l6gY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 40c7625 in 29 seconds
More details
- Looked at
289lines of code in6files - Skipped
2files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/examples/e2e/demo.ts:17
- Draft comment:
The async function is not invoked. Add();at the end to execute it.
})();
- Reason this comment was not posted:
Comment was on unchanged code.
2. js/src/utils/shared.ts:121
- Draft comment:
Setting the type to "string" by default might not be appropriate. Consider handling missing types more explicitly or logging a warning.
// Consider handling missing types more explicitly or logging a warning instead of defaulting to "string".
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change in the code sets a default type of 'string' when no type is provided. This could lead to incorrect assumptions about the data if 'string' is not the intended type. The comment suggests a potential improvement by handling missing types more explicitly, which is a valid concern. The comment is actionable and suggests a code quality improvement.
The comment might be considered speculative since it assumes that setting a default type to 'string' is inappropriate without knowing the context. However, it does suggest a clear improvement to the code.
The comment is not speculative because it addresses a potential issue with the current implementation and suggests a specific improvement. It is a valid concern that setting a default type could lead to incorrect data handling.
The comment is valid as it suggests a clear improvement to handle missing types more explicitly, which could prevent potential issues with data handling.
Workflow ID: wflow_AhSXSauP7y7GmTp1
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.
| } | ||
|
|
||
| async getAllActions(): Promise<Array<any>> { | ||
| return Array.from(this.customActions.values()).map((action: any) => action); |
There was a problem hiding this comment.
The getAllActions method should return only the schema, not the entire action object. Update the map function to return action.schema.
Suggested change
| return Array.from(this.customActions.values()).map((action: any) => action); | |
| return Array.from(this.customActions.values()).map((action: any) => action.schema); |
himanshu-dixit
approved these changes
Oct 25, 2024
| @@ -282,11 +312,25 @@ export class ComposioToolSet { | |||
| throw new Error("Not implemented"); | |||
| } | |||
|
|
|||
| async createAction(options: CreateActionOptions) { | |||
There was a problem hiding this comment.
shouldn't this be defined as composio client level?
Comment on lines
+35
to
37
| } catch (error) { | ||
| console.log(chalk.red((error as any).message)); | ||
| return; |
There was a problem hiding this comment.
catch and throwAxiosError
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 27e4829 in 12 seconds
More details
- Looked at
19lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/package.json:3
- Draft comment:
Consider updating the version to "0.3.0" instead of "0.2.4-2" to better reflect the addition of new features like the ActionRegistry, which is a minor update. - Reason this comment was not posted:
Confidence changes required:50%
The version change in package.json is consistent with a minor update, which is appropriate for adding new features like the ActionRegistry.
Workflow ID: wflow_GAVPwsiD4E7NEUDe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 61ae68c in 26 seconds
More details
- Looked at
128lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js/src/sdk/actionRegistry.spec.ts:67
- Draft comment:
Theparamsproperty is deprecated. UseinputParamsinstead to match the updatedCreateActionOptionsinterface.
inputParams: params,
- Reason this comment was not posted:
Marked as duplicate.
2. js/src/sdk/actionRegistry.spec.ts:86
- Draft comment:
Theparamsproperty is deprecated. UseinputParamsinstead to match the updatedCreateActionOptionsinterface.
inputParams: params,
- Reason this comment was not posted:
Marked as duplicate.
3. js/src/sdk/actionRegistry.spec.ts:114
- Draft comment:
Theparamsproperty is deprecated. UseinputParamsinstead to match the updatedCreateActionOptionsinterface.
inputParams: params,
- Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_jd1sLPn99cmQsP8t
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces
ActionRegistryfor managing custom actions in the JS SDK, with unit tests and integration into existing services.ActionRegistryclass inactionRegistry.tsfor managing custom actions.actionRegistry.spec.tsfor testingActionRegistryfunctionality.ActionRegistryintoComposioToolSetinbase.toolset.ts.getToolsSchemaandgetActionsSchemato include custom actions.ThrowOnErrortotrueinservices.gen.tsfor various service methods.getAuthInfo()inconnectedAccounts.ts.This description was created by
for 61ae68c. It will automatically update as commits are pushed.