feat: switch from client.actions to getToolsSchema #1174
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 8e4f421 in 26 seconds
More details
- Looked at
110lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/frameworks/vercel.spec.ts:27
- Draft comment:
Consider renaming the test to something more descriptive, like "should return tools for specified actions". - Reason this comment was not posted:
Confidence changes required:50%
The test description should be more descriptive to reflect the test's purpose.
2. js/src/frameworks/vercel.spec.ts:31
- Draft comment:
Consider renaming the test to something more descriptive, like "should create and retrieve custom action to star a repository". - Reason this comment was not posted:
Confidence changes required:50%
The test description should be more descriptive to reflect the test's purpose.
3. js/src/frameworks/vercel.spec.ts:58
- Draft comment:
Theawaitkeyword is unnecessary here.expectis not a promise and does not need to be awaited. - Reason this comment was not posted:
Confidence changes required:50%
The use ofawaitwithexpectis unnecessary and can be removed for clarity.
4. js/src/frameworks/vercel.ts:88
- Draft comment:
Ensure thatuseCaseLimitis correctly mapped and used consistently. Consider renaming tousecaseLimitto match the parsed object if necessary. - Reason this comment was not posted:
Confidence changes required:50%
ThegetToolsmethod has a potential issue with theuseCaseLimitparameter name, which should be consistent with the parsed object.
Workflow ID: wflow_QhoDZleBeFOzCV2v
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| actionSchema as ActionData | ||
| ); | ||
| }); | ||
| const tools = await this.getToolsSchema( |
There was a problem hiding this comment.
Consider adding error handling for the getToolsSchema call. If the API call fails, it could lead to an unhandled rejection. Something like:
const tools = await this.getToolsSchema(
{
apps,
tags,
useCase,
filterByAvailableApps,
actions,
useCaseLimit: usecaseLimit,
},
this.entityId
).catch(error => {
TELEMETRY_LOGGER.error('Failed to fetch tools schema', { error });
throw error;
});| return Object.fromEntries( | ||
| tools.map((tool) => [ | ||
| tool.name, | ||
| this.generateVercelTool(tool as RawActionData), |
There was a problem hiding this comment.
The type cast to RawActionData here could be unsafe. Consider adding a type guard or validation to ensure the tool object matches the RawActionData interface before casting. This would prevent potential runtime errors if the tool schema doesn't match expected format.
|
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: |
| actions: ["starRepositoryCustomAction"], | ||
| }); | ||
|
|
||
| await expect(Object.keys(tools).length).toBe(1); |
There was a problem hiding this comment.
The test case could benefit from additional assertions to verify the created action's properties. Consider adding checks for:
- The returned tool's description
- The input parameter schema structure
- The actual functionality by making a mock request
This would ensure the action is created correctly with all expected properties.
Code Review SummaryOverall AssessmentThe changes look good and improve the codebase by switching from Positive Aspects
Areas for Improvement
Suggestions Made
The changes are ready to be merged after addressing the suggested improvements. The code quality is good with minor areas for enhancement. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| }); | ||
| expect(Object.keys(tools).length).toBe(1); | ||
| }); | ||
|
|
||
| it("Should create custom action to star a repository", async () => { | ||
| await vercelAIToolSet.createAction({ | ||
| actionName: "starRepositoryCustomAction", | ||
| toolName: "github", |
There was a problem hiding this comment.
Code Quality: The added test case for creating a custom action to star a repository is a good addition. However, ensure that the test case is complete and follows the existing test structure. Consider adding assertions to verify the expected behavior of the createAction method. This will enhance the test's robustness and ensure it validates the intended functionality effectively.
🔧 Suggested Code Diff:
it("Should create custom action to star a repository", async () => {
await vercelAIToolSet.createAction({
actionName: "starRepositoryCustomAction",
toolName: "github",
+ // Add assertions to verify the action was created successfully
+ const action = tools.getAction("starRepositoryCustomAction");
+ expect(action).toBeDefined();
+ expect(action.toolName).toBe("github");
});
});📝 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.
| }); | |
| expect(Object.keys(tools).length).toBe(1); | |
| }); | |
| it("Should create custom action to star a repository", async () => { | |
| await vercelAIToolSet.createAction({ | |
| actionName: "starRepositoryCustomAction", | |
| toolName: "github", | |
| it("Should create custom action to star a repository", async () => { | |
| await vercelAIToolSet.createAction({ | |
| actionName: "starRepositoryCustomAction", | |
| toolName: "github" | |
| }); | |
| // Add assertions to verify the action was created successfully | |
| const action = tools.getAction("starRepositoryCustomAction"); | |
| expect(action).toBeDefined(); | |
| expect(action.toolName).toBe("github"); | |
| }); |
| useCase?: Optional<string>; | ||
| usecaseLimit?: Optional<number>; | ||
| filterByAvailableApps?: Optional<boolean>; | ||
| }): Promise<{ [key: string]: RawActionData }> { | ||
| }): Promise<{ [key: string]: CoreTool }> { | ||
| TELEMETRY_LOGGER.manualTelemetry(TELEMETRY_EVENTS.SDK_METHOD_INVOKED, { | ||
| method: "getTools", | ||
| file: this.fileName, |
There was a problem hiding this comment.
Potential Issue: The change in the return type from RawActionData to CoreTool indicates a significant alteration in the data structure. This could lead to compatibility issues in parts of the codebase that rely on the previous return type. It's crucial to ensure that all dependent modules or functions are updated to handle the new CoreTool structure appropriately. This change might affect data processing, serialization, or any logic that interacts with the returned data.
…posio into ft-vercel-get-tool-schema
js/src/sdk/client/core/OpenAPI.ts
Outdated
| @@ -39,7 +39,7 @@ export type OpenAPIConfig = { | |||
| }; | |||
| }; | |||
|
|
|||
| export const COMPOSIO_BASE_URL = "https://backend.composio.dev/"; | |||
| export const COMPOSIO_BASE_URL = "https://backend.composio.dev"; | |||
There was a problem hiding this comment.
The COMPOSIO_BASE_URL should not have a trailing slash. This can lead to double slashes in URLs when combined with other path segments.
📝 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.
| export const COMPOSIO_BASE_URL = "https://backend.composio.dev"; | |
| export const COMPOSIO_BASE_URL = "https://backend.composio.dev"; |
| @@ -62,7 +62,7 @@ export class VercelAIToolSet extends BaseComposioToolSet { | |||
| useCase?: Optional<string>; | |||
| usecaseLimit?: Optional<number>; | |||
| filterByAvailableApps?: Optional<boolean>; | |||
| }): Promise<{ [key: string]: RawActionData }> { | |||
| }): Promise<{ [key: string]: CoreTool }> { | |||
There was a problem hiding this comment.
The type of tools should be CoreTool[] instead of an object, as the getToolsSchema returns an array of CoreTool.
Important
Switch
getToolsinVercelAIToolSetto usegetToolsSchemaand add a test for custom action creation.getToolsinVercelAIToolSetnow usesgetToolsSchemainstead ofclient.actions.list.starRepositoryCustomActioninvercel.spec.ts.getToolsto{ [key: string]: CoreTool }.CoreToolandActionExecuteResponseimports invercel.tsandvercel.spec.tsrespectively.This description was created by
for 8e4f421. It will automatically update as commits are pushed.