-
Notifications
You must be signed in to change notification settings - Fork 66
feat: add pos parameter to move_card for list position control #52
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Schutzworks, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the card movement functionality by adding an optional position parameter. This allows users to specify where a card should be placed within a list, offering greater flexibility and control over board organization. The change involves updating both the tool's input definition and the client-side API call to accommodate this new parameter. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a pos parameter to the move_card tool, which is a great feature for controlling card placement. The implementation correctly threads the new parameter through to the Trello API. My main feedback is to improve the input validation for the new pos parameter in the Zod schema to make it more strict, ensuring it only accepts valid values ("top", "bottom", or a positive number) as expected by the Trello API. This will prevent invalid data from being passed to the client and improve the tool's robustness.
src/index.ts
Outdated
| pos: z | ||
| .union([z.string(), z.number()]) | ||
| .optional() | ||
| .describe('Position in target list: "top", "bottom", or a positive number'), |
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 current Zod schema for pos is z.union([z.string(), z.number()]), which is too permissive. It allows any string (not just "top" or "bottom") and any number (including negative numbers and zero), which could lead to unexpected behavior or API errors that are not caught by validation.
According to the Trello API documentation and the description you've provided, pos should be either "top", "bottom", or a positive number.
To enforce this and provide better type safety and validation, you should refine the schema. I suggest using z.enum(['top', 'bottom']) for the string part and z.number().positive() for the number part.
| pos: z | |
| .union([z.string(), z.number()]) | |
| .optional() | |
| .describe('Position in target list: "top", "bottom", or a positive number'), | |
| pos: z | |
| .union([z.enum(['top', 'bottom']), z.number().positive()]) | |
| .optional() | |
| .describe('Position in target list: "top", "bottom", or a positive number'), |
Closes #51
Adds an optional pos parameter to the move_card tool, allowing control over card position within the target list.
Changes:
Added pos to move_card input schema (accepts "top", "bottom", or numeric value)
Updated moveCard client method to include pos in the API request
Usage:
javascript// Move card to top of list
move_card({ cardId: "xxx", listId: "yyy", pos: "top" })
// Move card to specific position
move_card({ cardId: "xxx", listId: "yyy", pos: 140737487919104 })
Tested and working in Claude Desktop.