-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: validate mmproj from general basename #6477
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
Conversation
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.
Important
Looks good to me! 👍
Reviewed everything up to ea354ce in 1 minute and 27 seconds. Click for details.
- Reviewed
112lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:72
- Draft comment:
Good extraction of the 'general.basename' metadata to auto-set the model name. Consider applying the same sanitization rules (e.g., allowed characters) as used for the filename to ensure consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:124
- Draft comment:
The mmproj compatibility check uses case‑insensitive substring matching between baseName and modelName. Ensure this logic covers all edge cases—if you need stricter matching (e.g. full equality after normalization), consider updating the check. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_0r8CaKaG2mx72N8W
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 272ef9f in 1 minute and 16 seconds. Click for details.
- Reviewed
57lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:47
- Draft comment:
Good use of useCallback for validateGgufFile. Ensure that its dependency array (currently [modelName, serviceHub]) covers all variables used inside, especially if any other state or props are referenced in future modifications. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:275
- Draft comment:
The useEffect re-validating the MMProj file when modelName changes is a neat improvement. Confirm that rapid state changes won’t lead to unnecessary re-validations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:124
- Draft comment:
The MMProj validation logic comparing 'baseName' and 'modelName' using toLowerCase is clear. Be sure metadata always includes these keys to prevent edge case issues. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. web-app/src/containers/dialogs/ImportVisionModelDialog.tsx:138
- Draft comment:
Consider whether handling potential cancellation of concurrent validations is needed. Although not critical now, it could prevent race conditions during rapid state updates. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_c4Lw5zOIXxOxP0Rd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM
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.
Important
Looks good to me! 👍
Reviewed 0945eae in 1 minute and 39 seconds. Click for details.
- Reviewed
184lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/routes/settings/providers/$providerName.tsx:86
- Draft comment:
Good addition: the importingModel state tracks the currently importing model for displaying a loading skeleton. Consider renaming it to something like 'importingModelId' if multiple simultaneous imports might be supported in the future. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/routes/settings/providers/$providerName.tsx:115
- Draft comment:
The nested try-catch inside handleModelImportSuccess is functionally fine but could be flattened to improve readability. Consider merging error handling if possible. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/routes/settings/providers/$providerName.tsx:188
- Draft comment:
Nice useEffect that clears the importing state when the model appears in the provider's model list. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/routes/settings/providers/$providerName.tsx:199
- Draft comment:
The 10-second fallback timeout to clear importingModel is a good safeguard, but consider defining this magic number as a constant or adding a comment explaining its rationale. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. web-app/src/routes/settings/providers/$providerName.tsx:867
- Draft comment:
The importing skeleton rendering using animate-pulse provides a clear UX cue. Ensure that only one import is processed at a time; if concurrent imports are possible in the future, consider using an array to track multiple importing models. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_KZWRQCNWoCVdsm1T
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
This pull request refines the vision model import dialog by improving validation logic and error handling, and by removing unnecessary debug logging. The main changes focus on more robust extraction and verification of model metadata, particularly the
baseNamefield, and ensuring compatibility between MMProj files and selected models.Validation and Metadata Handling:
general.basenamefrom GGUF metadata and uses it to auto-set the model name if available, ensuring consistency between the file and the displayed model name.baseNamefrom the file matches or contains the current model name, and sets a validation error if there is a mismatch to prevent incompatible combinations.baseNamefrom metadata if available, improving user experience and reducing manual input errors.Code Cleanup:
console.logstatements that previously logged debugging information about file metadata and validation steps, resulting in cleaner production code. [1] [2] [3]Error Handling:
baseNamedoes not match the selected model name, and logging the mismatch for troubleshooting.Fixes Issues
Self Checklist
Important
Enhances vision model import dialog with improved validation, error handling, and code cleanup in
ImportVisionModelDialog.tsxandproviders/$providerName.tsx.general.basenamefrom GGUF metadata inImportVisionModelDialog.tsxto auto-set model name.baseNamecompatibility with model name for MMProj files, setting errors on mismatch.baseNameif available.console.logstatements fromImportVisionModelDialog.tsxfor cleaner code.baseNamemismatches in MMProj files.ImportVisionModelDialog.tsx.useEffectinImportVisionModelDialog.tsx.ProviderDetailinproviders/$providerName.tsx.This description was created by
for 0945eae. You can customize this summary. It will automatically update as commits are pushed.