-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Separate recommended models from rest to make it more clear #4835
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
🦋 Changeset detectedLatest commit: 5e8812a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThis PR adds section headers ("Recommended models" and "All models") to model selection dropdowns. The implementation is clean and well-tested. Key Changes:
Code Quality:
Files Reviewed (26 files)
|
|
Note I am not seeing any models in the CLI, please hold. |
34499a3 to
9619a57
Compare
Nevermind, rebasing fixed the issue I was seeing with the CLI. It's working fine and these changes are anyway localized to the webview. |
hassoncs
left a comment
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.
Looks good to me! It's kind of annoying that we have two extremely similar yet different implementations of this dropdown that you had to update 😅
maybe remove those component tests, but otherwise approved!
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.
I'd probably just leave these tests out– they aren't checking anything useful
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.
Thanks! I think I removed the section you were referring to.
| if (option.type === DropdownOptionType.LABEL) { | ||
| return ( | ||
| <div | ||
| key={`section-${index}`} |
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.
#nit
| key={`section-${index}`} | |
| key={`label-${index}`} |
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.
Fixed!
| export interface GroupedModelIds { | ||
| preferredModelIds: string[] | ||
| restModelIds: string[] | ||
| hasPreferred: boolean |
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.
Do we need hasPreferred here? Could we just check to see if the preferredModelIds array is empty or not?
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.
Nope, I dithered on removing it and sure enough it is cleaner, thanks for the suggestion!
|
@hassoncs - I think I addressed your feedback, can you do another pass? |
hassoncs
left a comment
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! Thanks for updating it! :D


This PR splits out the model list into two sections, a recommended section and the rest. We already sorted this so that recommended models appeared at the top, but other than the ordering there was no hint. This makes it more clear what we recommend folks to use and what should work well.