-
Notifications
You must be signed in to change notification settings - Fork 2.4k
🚀feat: allow user mark model as favorite #6006
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 fd46f0d in 1 minute and 55 seconds. Click for details.
- Reviewed
371lines of code in7files - 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/DropdownModelProvider.tsx:157
- Draft comment:
Consider using strict equality (===) instead of 'includes' when comparing provider IDs. Using 'includes' may lead to unintended matches. - 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% 1. The comment has a valid point - using includes() could match partial strings like "gpt" matching "gpt3" and "gpt4". 2. Provider IDs should be exact matches, not partial matches. 3. However, I don't have enough context about the predefinedProviders structure - maybe includes() is intentional here? 4. Without seeing predefinedProviders, I can't be 100% sure this is a bug. I don't have visibility into the predefinedProviders data structure. The current code using includes() may be intentionally designed to handle provider ID patterns or variations. While strict equality would generally be safer for ID comparisons, I can't be certain that includes() isn't serving a valid purpose here without seeing the predefinedProviders implementation. Without seeing the predefinedProviders implementation, I cannot be confident that this change would be correct. The current includes() usage may be intentional.
2. web-app/src/containers/DropdownModelProvider.tsx:242
- Draft comment:
A comment explaining why favorite models are intentionally excluded from regular provider groups when not searching would improve clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests adding an explanation for why favorite models are excluded from regular provider groups. This is asking for clarification rather than making a specific code suggestion or pointing out a potential issue. It violates the rule against asking the PR author to explain their intention.
3. web-app/src/hooks/useFavoriteModel.ts:52
- Draft comment:
Consider adding a trailing newline at the end of the file for consistency with common style guidelines. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. web-app/src/routes/settings/providers/$providerName.tsx:32
- Draft comment:
Typographical error: The file path '@/containers/CustomeTooltipJoyRide' appears to contain a misspelling. The import is named 'CustomTooltipJoyRide', so consider renaming the file path to '@/containers/CustomTooltipJoyRide' if that was intended. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_KC5X9S7ju13aTaFK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
baa46ba to
ceb35cd
Compare
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 f30ea9f in 59 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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/FavoriteModelAction.tsx:13
- Draft comment:
Consider using a element (or add appropriate role and tabindex) instead of afor clickable actions to improve accessibility (keyboard support and semantics). - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VMVNmeEbFNoC0WjA
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 76341db in 46 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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/DropdownModelProvider.tsx:161
- Draft comment:
Extraneous closing brace removed. This ensures proper control flow and avoids potential syntax issues. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made without suggesting any action or asking for clarification. It doesn't provide any actionable feedback or raise any concerns about the code.
Workflow ID: wflow_RgdCNSaHOSpWvmyn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: Louis <[email protected]>
Co-authored-by: Louis <[email protected]>
Describe Your Changes
This pull request introduces a "favorites" feature for managing models in the application. Users can now mark models as favorites, which are stored persistently and displayed in a dedicated section within the model dropdown. Key changes include the addition of a new
useFavoriteModelhook, updates to the dropdown and model settings UI, and logic to handle favorite models during deletion of models or providers.New Favorites Feature Implementation:
State Management for Favorites:
useFavoriteModelhook using Zustand to manage favorite models, including methods for adding, removing, toggling, and checking if a model is a favorite. Favorites are stored persistently in local storage under the keyfavoriteModels. (web-app/src/hooks/useFavoriteModel.ts)UI Enhancements:
DropdownModelProviderto display a dedicated "Favorites" section, which is shown only when not searching. Favorite models are excluded from regular provider sections in this case. (web-app/src/containers/DropdownModelProvider.tsx) [1] [2] [3] [4] [5]FavoriteModelActioncomponent to toggle the favorite status of a model, with a heart icon indicating the current state. (web-app/src/containers/FavoriteModelAction.tsx)Integration with Existing Features:
Model Settings:
FavoriteModelActioninto the model settings page, allowing users to mark models as favorites directly from the settings UI. (web-app/src/routes/settings/providers/$providerName.tsx) [1] [2] [3]Model and Provider Deletion:
DialogDeleteModelto remove models from favorites when they are deleted. (web-app/src/containers/dialogs/DeleteModel.tsx) [1] [2]DeleteProviderto remove all favorite models associated with a provider when the provider is deleted. (web-app/src/containers/dialogs/DeleteProvider.tsx) [1] [2]Supporting Changes:
favoriteModelstolocalStorageKeyfor managing favorite models in local storage. (web-app/src/constants/localStorage.ts)These changes collectively enhance the user experience by allowing users to easily manage and access their favorite models.
Test Plan
model dropdown
Fixes Issues
Self Checklist
Important
Introduces a feature to mark models as favorites, with state management, UI updates, and integration into model settings and deletion processes.
useFavoriteModelhook to manage favorite models with methods for adding, removing, toggling, and checking favorites. Uses Zustand and persists in local storage underfavoriteModels. (useFavoriteModel.ts)DropdownModelProviderto include a "Favorites" section in the dropdown, excluding favorites from regular sections when not searching. (DropdownModelProvider.tsx)FavoriteModelActioncomponent to toggle favorite status with a heart icon. (FavoriteModelAction.tsx)FavoriteModelActioninto model settings, allowing users to mark favorites from settings UI. ($providerName.tsx)DialogDeleteModelto remove models from favorites upon deletion. (DeleteModel.tsx)DeleteProviderto remove all favorite models associated with a provider upon deletion. (DeleteProvider.tsx)favoriteModelskey tolocalStorageKeyfor managing favorites in local storage. (localStorage.ts)This description was created by
for 76341db. You can customize this summary. It will automatically update as commits are pushed.