- Notifications
You must be signed in to change notification settings - Fork 23
feat: SBOM|Package list pages -> filtering by license #759
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
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Reviewer's GuideThis PR adds license-based filtering to SBOM and package list pages by extending the OpenAPI spec with a new license endpoint, introducing a useFetchLicenses query hook, extending the filter framework with an async multiselect filter type and UI components, and integrating this new filter into the SBOM and package contexts. Class diagram for new and updated filter componentsclassDiagram class MultiSelect { +id: string +onChange(selections: MultiSelectOptionProps[]): void +options: MultiSelectOptionProps[] +selections: MultiSelectOptionProps[] +placeholderText: string +searchString: string +searchInputAriaLabel: string +labelColor +noResultsMessage: string +showChips: boolean +onSearchChange(value: string): void +isDisabled: boolean +isScrollable: boolean } class SearchInputComponent { +id: string +placeholder: string +ariaLabel: string +onSearchChange(value: string): void +onClear(): void +onKeyHandling(event): void +onClick(): void +inputValue: string +inputRef +selections: MultiSelectOptionProps[] +isDropdownOpen: boolean +activeItem: MultiSelectOptionProps } class useAutocompleteHandlers { +options: MultiSelectOptionProps[] +searchString: string +selections: MultiSelectOptionProps[] +onChange(selections: MultiSelectOptionProps[]): void +menuRef +searchInputRef +onSearchChange(value: string): void } class AsyncMultiselectFilterControl { +category: IMultiselectFilterCategory +filterValue +setFilterValue +showToolbarItem +isDisabled: boolean } class MultiSelectOptionProps { +id: string +name: string | () => string +labelName: string | () => string +tooltip: string | () => string +optionProps } MultiSelect --> SearchInputComponent MultiSelect --> useAutocompleteHandlers AsyncMultiselectFilterControl --> MultiSelect MultiSelectOptionProps <.. MultiSelect MultiSelectOptionProps <.. AsyncMultiselectFilterControl Class diagram for useFetchLicenses query hookclassDiagram class useFetchLicenses { +params: HubRequestParams +disableQuery: boolean +result: { data: LicenseText[], total: number, params } +isFetching: boolean +fetchError: AxiosError +refetch } class LicenseText { +license: string } useFetchLicenses --> LicenseText File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review: ## Individual Comments ### Comment 1 <location> `client/src/app/components/FilterToolbar/AsyncMultiselectFilterControl.tsx:30-31` </location> <code_context> +}: React.PropsWithChildren< + IMultiselectFilterControlProps<TItem> +>): React.JSX.Element | null => { + const optionMap = React.useRef( + new Map<string, FilterSelectOptionProps | null>(), + ); + </code_context> <issue_to_address> **issue:** The 'optionMap' is initialized but never populated. Since 'optionMap' remains empty, 'getOptionFromOptionValue' cannot perform lookups. Consider initializing 'optionMap' with values from 'selectOptions' if label lookup is required. </issue_to_address> ### Comment 2 <location> `client/src/app/components/MultiSelect/MultiSelect.tsx:87-88` </location> <code_context> + onSearchChange, + }); + + const createItemId = (value: string) => + `select-typeahead-${value.replace(" ", "-")}`; + + const inputGroup = ( </code_context> <issue_to_address> **suggestion:** The 'replace' only replaces the first space in the value. Use 'replaceAll' or a regular expression to ensure all spaces are replaced for consistent ID formatting. Suggested implementation: ```typescript const createItemId = (value: string) => `select-typeahead-${value.replaceAll(" ", "-")}`; ``` If you need to support environments where `replaceAll` is not available, you can use: `value.replace(/\s/g, "-")` instead of `value.replaceAll(" ", "-")`. </issue_to_address> ### Comment 3 <location> `client/src/app/components/MultiSelect/useMultiSelectHandlers.ts:104-113` </location> <code_context> + } + }; + + const handleMenuArrowKeys = (key: string) => { + let indexToFocus = 0; + if (isDropdownOpen) { + if (key === "ArrowUp") { + if (focusedItemIndex === null || focusedItemIndex === 0) { + indexToFocus = options.length - 1; + } else { + indexToFocus = focusedItemIndex - 1; + } + } + if (key === "ArrowDown") { + if ( + focusedItemIndex === null || + focusedItemIndex === options.length - 1 + ) { + indexToFocus = 0; + } else { + indexToFocus = focusedItemIndex + 1; + } + } + } + setFocusedItemIndex(indexToFocus); + const focusedItem = options.filter( + ({ optionProps }) => !optionProps?.isDisabled, + )[indexToFocus]; + setActiveItem(focusedItem); + }; + </code_context> <issue_to_address> **issue (bug_risk):** Filtering out disabled options may cause index mismatch. Index calculations should be based on the filtered array to ensure 'focusedItem' corresponds to the correct option. </issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
client/src/app/components/FilterToolbar/AsyncMultiselectFilterControl.tsx Show resolved Hide resolved
# Conflicts: # client/src/app/pages/package-list/package-context.tsx
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## main #759 +/- ## ========================================== - Coverage 59.91% 58.26% -1.65% ========================================== Files 157 163 +6 Lines 2699 2868 +169 Branches 612 652 +40 ========================================== + Hits 1617 1671 +54 - Misses 849 956 +107 - Partials 233 241 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
gildub 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.
It works just fine.
I can't believe how many components are needed to get the Licence filter.
Besides cosmetics my only concern is the issue about refetching licenses systematically. Please see inline.
client/src/app/components/AsyncMultiSelect/AsyncMultiSelect.tsx Outdated Show resolved Hide resolved
client/src/app/components/AsyncMultiSelect/useMultiSelectHandlers.ts Outdated Show resolved Hide resolved
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
| /backport |
1 similar comment
| /backport |
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com> (cherry picked from commit 307eabd)
| Successfully created backport PR for |
GET /api/v1/license.Summary by Sourcery
Enable filtering SBOM and package lists by license through an asynchronous multi-select control and new license API endpoint.
New Features:
Enhancements: