fix: model association priority and ui align#344
Conversation
Summary of ChangesHello @looplj, 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 refines the model association functionality by implementing a priority-based system, which dictates the order of model evaluation. It also significantly improves the user experience within the model association dialog through UI alignment adjustments, the introduction of an AutoCompleteSelect for channel selection, and robust input validation. These changes collectively aim to provide users with more precise control over model behavior and a more intuitive configuration interface. 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.
Code Review
This pull request introduces priority for model associations and improves the UI for managing them. The changes include adding validation for priority and association count, sorting associations by priority, and using an auto-complete component for better UX. The code is well-structured, but I've found a couple of areas for improvement. There's a bug in the priority input where the max attribute is inconsistent with the validation logic. I've also suggested improving type safety by avoiding the use of any when processing associations. Overall, these are great enhancements.
| <FormControl> | ||
| <Input | ||
| type='number' | ||
| min={0} | ||
| max={10} |
There was a problem hiding this comment.
The max value for the priority input is set to 10, but the form validation schema (max 100) and the onChange handler (clamps to 100) allow values up to 100. This creates an inconsistency where users can't use the input's native controls (e.g., arrow keys) to select a value above 10. This should be updated to 100 to match the validation logic.
| max={10} | |
| max={100} |
| @@ -167,7 +169,8 @@ export function ModelsAssociationDialog() { | |||
|
|
|||
| const fetchConnections = async () => { | |||
| try { | |||
| const associations: ModelAssociationInput[] = debouncedAssociations | |||
| const sortedDebouncedAssociations = [...debouncedAssociations].sort((a: any, b: any) => (a.priority ?? 0) - (b.priority ?? 0)) | |||
There was a problem hiding this comment.
Using any here reduces type safety and makes the code harder to read and maintain. It's better to use a proper type for the items in debouncedAssociations. You can use the type inferred from your Zod schema and let TypeScript handle type checking. This also makes the sorting logic cleaner as priority is a required number.
| const sortedDebouncedAssociations = [...debouncedAssociations].sort((a: any, b: any) => (a.priority ?? 0) - (b.priority ?? 0)) | |
| const sortedDebouncedAssociations = [...(debouncedAssociations as AssociationFormData['associations'])].sort((a, b) => a.priority - b.priority) |
No description provided.