-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: filter downloaded model on hub screen #5113
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 9f09137 in 2 minutes and 38 seconds. Click for details.
- Reviewed
166
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
draft 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/Card.tsx:39
- Draft comment:
Ensure removal of 'line-clamp-1' is intentional; without it long titles might disrupt layout. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/containers/DropdownModelProvider.tsx:88
- Draft comment:
Optional chaining on 'currentModel?.settings' improves error handling. Consider a stricter type guard if the ModelSetting requires more specific model data. - Reason this comment was not posted:
Comment was on unchanged code.
3. web-app/src/routes/hub.tsx:103
- Draft comment:
The downloaded filter assumes 'llamaProvider' is available. Ensure a fallback or proper initialization if 'llamaProvider' might be undefined. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. web-app/src/routes/hub.tsx:253
- Draft comment:
Consider adding ARIA attributes or explicit labels to the downloaded filter Switch for better accessibility. - 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 Switch component is already properly labeled through its visual proximity to the "Downloaded" text span. In React/web semantics, when a switch/checkbox is placed next to text within the same container div, screen readers typically associate them correctly. The visual label should be sufficient for accessibility in this case. I could be wrong about how screen readers interpret the relationship between the Switch and its adjacent text label. Some accessibility guidelines do recommend explicit ARIA labels even when visual labels exist. While explicit ARIA labels can help in some cases, they're unnecessary and potentially redundant when proper visual labeling and semantic HTML structure is already in place, as it is here. The comment should be deleted as the Switch component already has sufficient accessibility through its associated visual label text.
Workflow ID: wflow_vQTE11EcprsXNSIS
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 a911eb5 in 59 seconds. Click for details.
- Reviewed
191
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
draft 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:63
- Draft comment:
Consider checking if 'provider' (from getProviderByName) is non-null before casting to ProviderObject when passing it to ProvidersAvatar, to avoid potential runtime errors. - 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/containers/ProvidersAvatar.tsx:6
- Draft comment:
To improve readability and efficiency, store the result of getProviderLogo(provider.provider) in a variable instead of calling it multiple times. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. web-app/src/containers/ProvidersAvatar.tsx:9
- Draft comment:
Consider caching the result of getProviderTitle(provider.provider) to avoid duplicate computation, especially if the function becomes more complex in the future. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
4. web-app/src/containers/ProvidersMenu.tsx:67
- Draft comment:
Using the array index as key (line 67) can be fragile if the providers array changes order. Consider using a unique identifier if available. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. web-app/src/containers/ProvidersMenu.tsx:102
- Draft comment:
The onClick handler for the 'Add Provider' component is an empty function. Remove it if it's not needed to prevent unnecessary code. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. web-app/src/lib/utils.ts:32
- Draft comment:
Ensure that consumers of getProviderLogo properly handle the case when an undefined value is returned for unknown providers. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
Workflow ID: wflow_8uJl10mRQ2aHlZso
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 cd2beb1 in 48 seconds. Click for details.
- Reviewed
21
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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:68
- Draft comment:
Added 'max-h-[32px]' may restrict the button's height. Please verify that this fixed max height remains responsive and doesn't truncate content on varying screens. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. web-app/src/containers/DropdownModelProvider.tsx:100
- Draft comment:
The new 'alignOffset={-8}' property can affect dropdown positioning. Ensure the negative offset aligns correctly across devices and does not cause layout inconsistencies. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_ORVv1et3RkywBloC
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 introduces UI improvements, filtering enhancements, and minor code adjustments in the
web-app/src
directory. The most notable changes include adding a "Downloaded" filter toggle in the Hub view, improving the model filtering logic, and refining the usage of optional chaining for better error handling.UI Enhancements:
Switch
component and accompanying label. (web-app/src/routes/hub.tsx
, web-app/src/routes/hub.tsxR253-R262)Filtering Improvements:
showOnlyDownloaded
and checks againstllamaProvider
models. (web-app/src/routes/hub.tsx
, web-app/src/routes/hub.tsxL86-R112)Code Adjustments:
showOnlyDownloaded
and moved the initialization ofllamaProvider
to an earlier point in theHub
component. (web-app/src/routes/hub.tsx
, [1] [2]CardItem
with a simpler class to improve readability. (web-app/src/containers/Card.tsx
, web-app/src/containers/Card.tsxL39-R39)DropdownModelProvider
by adding an optional chaining check forcurrentModel?.settings
. (web-app/src/containers/DropdownModelProvider.tsx
, web-app/src/containers/DropdownModelProvider.tsxL88-R88)Fixes Issues
Self Checklist
Important
Enhances Hub view with a "Downloaded" filter toggle, updates filtering logic, and refines code for better error handling and readability.
hub.tsx
to filter models by download status.Switch
component for the toggle.hub.tsx
to apply search and downloaded filters usingshowOnlyDownloaded
andllamaProvider
models.showOnlyDownloaded
state and movedllamaProvider
initialization inhub.tsx
.CardItem
with a simpler class.DropdownModelProvider.tsx
with optional chaining forcurrentModel?.settings
.ProvidersAvatar
component to handle provider logos inDropdownModelProvider.tsx
andProvidersMenu.tsx
.getProviderLogo
import inDropdownModelProvider.tsx
andProvidersMenu.tsx
.This description was created by
for cd2beb1. You can customize this summary. It will automatically update as commits are pushed.