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.

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: switch vercel from zod to jsonSchema #1156

Merged
merged 7 commits into from
Jan 4, 2025
Merged

Conversation

himanshu-dixit
Copy link
Collaborator

@himanshu-dixit himanshu-dixit commented Jan 4, 2025

Important

Switches parameter validation from zod to jsonSchema in vercel.ts and filters active accounts in base.toolset.ts.

  • Behavior:
    • Switch from zod to jsonSchema for parameter validation in generateVercelTool() in vercel.ts.
    • Adds status: "ACTIVE" and showActiveOnly: true to connected account list in ComposioToolSet in base.toolset.ts.
  • Code Changes:
    • Replace zExecuteToolCallParams with ZExecuteToolCallParams in vercel.ts.
    • Remove jsonSchemaToModel usage in vercel.ts.

This description was created by Ellipsis for 4d13503. It will automatically update as commits are pushed.



EntelligenceAI PR Summary

The recent update includes minor enhancements: adding filters for active connections in base.toolset.ts and refining parameter management in vercel.ts. These adjustments are not substantial.

Copy link

vercel bot commented Jan 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2025 5:47am

Copy link

Walkthrough

The PR introduces minor changes by adding filters for active connections in base.toolset.ts and refactoring parameter handling in vercel.ts. These changes are not significant.

🔗 Related PRs

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @bot + *your message*
Example: @bot Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @bot + *feedback*
Example: @bot Do not comment on `save_auth` function !

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 e67a6b8 in 12 seconds

More details
  • Looked at 45 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. js/src/frameworks/vercel.ts:43
  • Draft comment:
    Avoid using @ts-ignore unless absolutely necessary. Ensure jsonSchema is correctly typed to avoid this.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. js/src/frameworks/vercel.ts:45
  • Draft comment:
    Avoid using @ts-ignore unless absolutely necessary. Ensure jsonSchema is correctly typed to avoid this.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_OqgxBNZHnTIgm1wc


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -1,10 +1,10 @@
import { tool } from "ai";
import { jsonSchema, tool } from "ai";
import { z } from "zod";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're switching from Zod to JSON Schema, the z import from 'zod' appears to be unused in this file. Consider removing it to keep the imports clean and avoid confusion.

Copy link

github-actions bot commented Jan 4, 2025

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12607995357/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12607995357/html-report/report.html

@shreysingla11
Copy link
Collaborator

Code Review Summary

Changes Overview

  • Switching from Zod to JSON Schema for parameter validation in Vercel integration
  • Adding connection filtering with active status checks
  • Removing jsonSchemaToModel utility in favor of direct jsonSchema usage

Key Concerns

  1. Type Safety: The introduction of @ts-ignore comments bypasses TypeScript checks, which could lead to runtime issues
  2. Unused Dependencies: The Zod import remains despite the switch to JSON Schema
  3. Documentation: The changes would benefit from additional documentation explaining the validation strategy change

Recommendations

  1. Create proper type definitions instead of using @ts-ignore
  2. Remove unused Zod import and clean up related code
  3. Add documentation about the switch from Zod to JSON Schema
  4. Consider adding error handling for cases where no active connections are found

Code Quality Rating: 6/10

  • Positives: Clean implementation of JSON Schema integration, improved connection filtering
  • Areas for Improvement: Type safety, documentation, cleanup of unused code

Please address the comments in the review before merging.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 00af088 in 9 seconds

More details
  • Looked at 17 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/frameworks/vercel.ts:43
  • Draft comment:
    The @ts-ignore comment is replaced with @ts-expect-error, which is more appropriate as it indicates an expected error that should be resolved in the future. Ensure that this error is indeed expected and documented for future resolution.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The @ts-ignore comment is replaced with @ts-expect-error, which is more appropriate as it indicates an expected error that should be resolved in the future.

Workflow ID: wflow_wDVexHeriMuZPYv1


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 4d13503 in 31 seconds

More details
  • Looked at 32 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/frameworks/vercel.ts:43
  • Draft comment:
    Using as unknown as any can lead to potential type safety issues. Consider defining a proper type or interface for schema.parameters.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment raises a valid point about type safety, as as unknown as any bypasses TypeScript's type checking. However, looking at the previous code, it used @ts-expect-error with a comment explaining the types are JSONSchemaV7. This suggests there may be a legitimate reason for the type assertion that can't be easily fixed. Without seeing the full type definitions and understanding why the original @ts-expect-error was needed, we can't be confident this is actually fixable.
    I might be too quick to assume the type assertion is necessary - there could be a proper way to type this that I'm missing. The original @ts-expect-error comment could have been wrong.
    While true, without access to the full type definitions and the ai package's requirements, we can't be confident enough that this comment is actionable. The change from @ts-expect-error to type assertion suggests the team is aware of and has considered the typing issues.
    Delete the comment since we don't have enough context to be confident this is a fixable issue, and the previous code suggests the type assertion may be intentional/necessary.

Workflow ID: wflow_ld4NN3rf1XfMHIPf


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@himanshu-dixit himanshu-dixit changed the title feat: swtich vercel from zod to jsonSchema feat: switch vercel from zod to jsonSchema Jan 4, 2025
@himanshu-dixit himanshu-dixit merged commit 94f1584 into master Jan 4, 2025
13 of 14 checks passed
@himanshu-dixit himanshu-dixit deleted the ft-vercel-switch branch January 4, 2025 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants