Conversation
Greptile SummaryThis PR extracts the hardcoded maximum association priority value ( Confidence Score: 5/5Safe to merge — all hardcoded occurrences of 10 have been replaced with the new constant, including the previously flagged error message. The change is a clean, focused refactor with no logic changes. All three usage sites (schema validation, error message, and input clamping) are correctly updated. The prior concern about the hardcoded value in the error message has been addressed in this PR. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User types priority value] --> B[onChange handler]
B --> C["Math.max(0, Math.min(MAX_ASSOCIATION_PRIORITY, value))"]
C --> D[Clamped value written to form state]
D --> E[Zod schema validation]
E --> F{".min(0).max(MAX_ASSOCIATION_PRIORITY)"}
F -->|valid| G[Form submits]
F -->|invalid| H["Error: Priority cannot exceed MAX_ASSOCIATION_PRIORITY"]
Reviews (2): Last reviewed commit: "fix: max model association priority, clo..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces a MAX_ASSOCIATION_PRIORITY constant to the models-association-dialog.tsx component, updating the Zod validation schema and the UI input logic to use this centralized value. While the logic was updated, the validation error message in the schema remains hardcoded to "10" and should be updated to use the constant to ensure consistency.
| z.object({ | ||
| type: z.enum(['channel_model', 'channel_regex', 'model', 'regex', 'channel_tags_model', 'channel_tags_regex']), | ||
| priority: z.number().min(0, 'Priority must be at least 0').max(10, 'Priority cannot exceed 10'), | ||
| priority: z.number().min(0, 'Priority must be at least 0').max(MAX_ASSOCIATION_PRIORITY, 'Priority cannot exceed 10'), |
There was a problem hiding this comment.
The error message for the max validation is still hardcoded to "10". It should use the MAX_ASSOCIATION_PRIORITY constant to ensure consistency if the limit is changed in the future.
| priority: z.number().min(0, 'Priority must be at least 0').max(MAX_ASSOCIATION_PRIORITY, 'Priority cannot exceed 10'), | |
| priority: z.number().min(0, 'Priority must be at least 0').max(MAX_ASSOCIATION_PRIORITY, `Priority cannot exceed ${MAX_ASSOCIATION_PRIORITY}`), |
No description provided.