-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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: support custom actions in vercel toolkit (with type safety improvements) #1175
feat: support custom actions in vercel toolkit (with type safety improvements) #1175
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to b5aac36 in 36 seconds
More details
- Looked at
231
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. js/src/frameworks/vercel.spec.ts:28
- Draft comment:
The test case is checking ifObject.keys(tools)
is an instance ofArray
, which is incorrect.Object.keys()
always returns an array. Consider usingArray.isArray()
to check if the result is an array.
expect(Array.isArray(Object.keys(tools))).toBe(true);
- Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. js/src/frameworks/vercel.spec.ts:60
- Draft comment:
The test case is checking ifObject.keys(tools)
is an instance ofArray
, which is incorrect.Object.keys()
always returns an array. Consider usingArray.isArray()
to check if the result is an array.
expect(Array.isArray(Object.keys(tools))).toBe(true);
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_xyMOK3rkaMgHorl0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
}); | ||
} | ||
|
||
async createAction(options: CreateActionOptions) { | ||
return this.userActionRegistry.createAction(options); | ||
async createAction<P extends Parameters = z.ZodObject<{}>>(options: CreateActionOptions<P>) { | ||
return this.userActionRegistry.createAction<P>(options); | ||
} | ||
|
||
private isCustomAction(action: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor: The change ensures type safety by using generics, aligning with the ActionRegistry
class.
actionsList.items?.forEach((actionSchema) => { | ||
// @ts-ignore | ||
const tools: { [key: string]: CoreTool } = {}; | ||
actionsList.forEach((actionSchema) => { | ||
tools[actionSchema.name!] = this.generateVercelTool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a type guard instead of non-null assertion on actionSchema.name
. This could prevent runtime errors if the name is undefined. Example:
if (!actionSchema.name) {
console.warn('Action schema missing name, skipping...');
return;
}
tools[actionSchema.name] = this.generateVercelTool(actionSchema);
@@ -67,7 +83,7 @@ export class ActionRegistry { | |||
throw new Error("You must provide actionName for this action"); | |||
} | |||
if (!options.inputParams) { | |||
options.inputParams = z.object({}); | |||
options.inputParams = z.object({}) as P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion as P
here could be unsafe. Consider adding a type guard or validation to ensure the empty object conforms to the expected type P. Alternatively, you could make inputParams
required and remove this default initialization.
toolName: "github", | ||
description: "Star A Github Repository For Given `Repo` And `Owner`", | ||
inputParams: params, | ||
callback: async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error case tests for the callback function. The current test only covers the success case. Add tests for scenarios where the callback might fail or return unsuccessful results.
Code Review SummaryOverall AssessmentThe changes introduce valuable improvements to the Vercel toolkit's custom actions system with better type safety and generic inference. The code quality is good, but there are a few areas that could be improved for better reliability and maintainability. Strengths
Areas for Improvement
Recommendations
Overall, this is a solid PR that improves the codebase. With the suggested improvements implemented, it would be ready for merge. |
Thanks for the fix @nicolas-angelo 🚀 . Merging the PR. |
…ovements) (#1175) Co-authored-by: Himanshu Dixit <hudixt@gmail.com>
This PR introduces two small enhancements:
Generics-Based Inference in
createAction
createAction
function to accept a generic parameter P (extending a Zod schema type)inputParams
in the callback are now automatically inferred based on the provided Zod schema, reducing manual type annotations.Inclusion of Custom Actions in Vercel Toolkit’s
getTools
getTools
,Important
Enhance
createAction
with generics-based inference and integrate custom actions into Vercel Toolkit'sgetTools
.createAction
:createAction
inactionRegistry.ts
to accept a generic parameterP
extending a Zod schema type.inputParams
in the callback are inferred based on the provided Zod schema.getTools
:getTools
invercel.ts
to include custom actions.getToolsSchema
inbase.toolset.ts
to filter and include custom actions.vercel.spec.ts
to verify inclusion and execution of custom actions.This description was created by for b5aac36. It will automatically update as commits are pushed.