-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: compatibility imported model #6271
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 f6e4d55 in 2 minutes and 10 seconds. Click for details.
- Reviewed
82lines 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/containers/ModelSupportStatus.tsx:35
- Draft comment:
Return type updated to include null for error states. Ensure downstream handling (in useEffect and rendering) consistently interprets null vs 'RED'. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/containers/ModelSupportStatus.tsx:49
- Draft comment:
Using 'await fs.existsSync' suggests an asynchronous call. Verify that fs.existsSync is async; if it’s truly synchronous consider removing await or renaming to reflect async behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The name existsSync suggests synchronous behavior, but this could be misleading. The fs module is from a custom package @janhq/core, not Node.js. Without seeing the implementation, we can't be sure if it's truly synchronous. The code uses await consistently with this method, suggesting it might actually be async despite the name. I might be too hesitant - if something is named Sync it's usually synchronous by convention. The comment could be pointing out a real issue. However, this is a custom fs implementation from @janhq/core, not the standard Node.js fs module. Without seeing its implementation, removing await could break the code. Since we don't have strong evidence that fs.existsSync is actually synchronous in this custom implementation, we should err on the side of caution and delete the comment.
3. web-app/src/containers/ModelSupportStatus.tsx:70
- Draft comment:
After invoking 'read_yaml', validate that modelConfig and modelConfig.model_path exist to avoid runtime errors from misconfigured YAML. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While runtime validation could add an extra safety layer, the existing error handling (try-catch) will catch any YAML parsing errors. TypeScript's type system should catch most issues at compile time. The code is in a try-catch block that returns null on any error, providing a safe fallback. Adding explicit validation would be defensive programming but not strictly necessary. The comment raises a valid concern about runtime safety. YAML parsing could technically succeed but return unexpected structure. The existing error handling and type system provide sufficient safety. Any runtime errors will be caught by the try-catch and handled gracefully by returning null. The comment should be deleted as the code already has adequate error handling through try-catch and type safety through TypeScript.
4. web-app/src/containers/ModelSupportStatus.tsx:88
- Draft comment:
The error catch returns null while useEffect sets status to 'RED' on error. Consider unifying error state representation for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The inconsistency is real and could cause confusion. The inner function returns null which gets handled by the outer function to set 'RED' status. However, this two-step approach is actually good design - the utility function returns null to indicate failure, while the UI component decides how to represent that failure. The current implementation is better than the suggested change. Maybe the two-level error handling is intentional for separation of concerns? The inner function shouldn't make UI-level decisions about status colors. Yes, the current implementation is actually better - it maintains separation between the utility function and UI logic. The suggestion would blur this boundary. The comment should be deleted because the current implementation is better - it properly separates the utility function's error handling from the UI component's status representation.
5. web-app/src/containers/ModelSupportStatus.tsx:76
- Draft comment:
The regex /^[A-Za-z]:/ is used to detect Windows absolute paths. Consider adding a comment to clarify this check. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_tBctUj2f8QKjihQl
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.
LGTM

Describe Your Changes
This pull request enhances the logic for resolving model file paths in the
ModelSupportStatuscomponent, improving support for both standard and imported models. The main changes focus on detecting the correct model file (model.gguformodel.yml), handling absolute and relative paths, and updating the return type to better reflect error states.Model file path resolution improvements:
model.gguffile, and if not found, fallback to reading model configuration frommodel.ymlfor imported models. This ensures compatibility with both downloaded and imported models.model_pathfrom the YAML configuration.'RED' | 'YELLOW' | 'GREEN'to'RED' | 'YELLOW' | 'GREEN' | nullto better indicate when model files are missing or an error occurs.Dependency and API usage updates:
fsfrom@janhq/corefor file existence checks, andinvokefrom@tauri-apps/api/corefor reading YAML configuration files.Fixes Issues
Self Checklist
Important
Improves model file path resolution in
ModelSupportStatus.tsxto support both standard and imported models, handling absolute/relative paths and updating return types for error states.checkModelSupportWithPathinModelSupportStatus.tsxto first check formodel.gguf, then fallback tomodel.ymlfor imported models.model_pathfrom YAML.'RED' | 'YELLOW' | 'GREEN'to'RED' | 'YELLOW' | 'GREEN' | nullto indicate missing files or errors.fsfrom@janhq/corefor file checks andinvokefrom@tauri-apps/api/corefor reading YAML.This description was created by
for f6e4d55. You can customize this summary. It will automatically update as commits are pushed.