-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: improve ux import model #6233
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.
Caution
Changes requested ❌
Reviewed everything up to 6ee044d in 2 minutes and 23 seconds. Click for details.
- Reviewed
93lines 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/routes/settings/providers/$providerName.tsx:531
- Draft comment:
Consider refactoring the 'await getProviders().then(setProviders)' into a more readable async/await style for clarity. For example: 'const providers = await getProviders(); setProviders(providers);'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/routes/settings/providers/$providerName.tsx:524
- Draft comment:
In the catch block for pullModel, only console.error is called. It might improve UX to also display a toast error so the user is informed when the import fails. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_VAgg5jfKeWpa1VlK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 37.52%Your code coverage diff: -0.06% ▾ Uncovered files and lines
|
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.
Caution
Changes requested ❌
Reviewed 6c612d8 in 2 minutes and 16 seconds. Click for details.
- Reviewed
140lines 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/routes/settings/providers/$providerName.tsx:549
- Draft comment:
Refactored the inline onClick handler to use handleImportModel, which improves readability and reduces duplication. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/routes/settings/providers/$providerName.tsx:115
- Draft comment:
Ensure that 'open' returns a string as expected. If there's potential for an array (even with multiple:false), consider adding a type guard for safety. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_guZGpMJEpDL1HEJ7
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 b828d3f in 1 minute and 28 seconds. Click for details.
- Reviewed
122lines of code in2files - 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/routes/settings/providers/$providerName.tsx:373
- Draft comment:
Consider handling the asynchronous update of provider settings more robustly. The onChange callback updates settings by calling updateSettings and updateProvider without awaiting or error handling. This might lead to race conditions and unhandled errors. Consider awaiting these calls and handling errors before calling stopAllModels(). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/routes/settings/providers/$providerName.tsx:125
- Draft comment:
Ensure consistency in duplicate model detection: the import check compares fileName with model.name, whereas refresh logic uses model.id. Consider standardizing on a single unique identifier for models. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_tI08FT4wGDoHVNz3
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
Describe Your Changes
This pull request improves the user experience when importing models in the provider settings page. The main changes add feedback and error handling to the import process, preventing duplicate imports and showing a loading indicator during long-running operations.
Model Import UX Improvements:
importingModelstate to track when a model is being imported and to disable the import button during the process. [1] [2]Error Handling and Validation:
importingModelstate is reset to false in all code paths, including error and early return scenarios. [1] [2]Minor Improvements:
Fixes Issues
<img width="314" height="281" alt="Screenshot 2025-08-19 at 21 21 35" src="https://github.com/user-attachments/assets/602d1522-8efa-4348-8100-d908bbe656c0"


/>
Self Checklist
Important
Improves model import UX in provider settings with loading state, error handling, and duplicate prevention.
importingModelstate in$providerName.tsxto track import process and disable button.importingModelstate resets in all scenarios, including errors.Cargo.lockto includedarlingversion0.21.2andrmcpversion0.5.0.This description was created by
for b828d3f. You can customize this summary. It will automatically update as commits are pushed.