[OPIK-7278] [FE] refactor: migrate LoadableSelectBox to the new design system (v2)#7551
Conversation
…n system (v2) Fork the searchable single/multi-select popover into a v2-owned component (src/v2/components/LoadableSelectBox) and align it visually with the migrated DS Select and the Project/Workspace selectors, without touching v1. Visual changes (fork): - Trigger chevron -> DS token (text-foreground/opacity-50, size-3.5) - Option rows h-10/px-4 -> h-8/px-3; selected single-select row filled (bg-primary-100/text-primary); group headers text-light-slate - In-popover search field -> compact dimension="sm" (matches Project/Workspace) - Optional per-option `icon` (local option type; opt-in, unused by default) Consumers: - Repoint 12 v2 usages to the fork; normalize the create/action footer to <ListAction size="sm"> (AddToDatasetDialog raw div -> ListAction, fixes a11y) Behavior is unchanged (client-side search preserved). v1 and the shared LoadableSelectBox are untouched. tsc/eslint/vitest green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
⏱️ pre-commit per-hook timing
⏭️ 15 skipped (no matching files changed)
|
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
…ct/Workspace selectors - Render the in-popover search inline (px-0.5) instead of absolutely positioned, so it left-aligns with the option rows (removes the pt-11 spacer hack). - Switch dividers from bg-border to the lighter bg-muted with -mx-px (search separator, group separator, select-all separator, load-more border, and the actionPanel separators in the 3 create-selects) to match the reference selectors' divider color. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
awkoy
added
test-environment
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
…e-select rows Design feedback: the selected single-select option is already conveyed by the filled blue row (bg-primary-100/text-primary), so the leading Check icon (and the min-w-4 spacer reserved for it on unselected rows) is redundant. Remove both so all single-select labels sit flush at the 12px row padding. Multiselect keeps its checkbox. Verified e2e (dataset selector). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| interface SingleSelectProps extends BaseLoadableSelectBoxProps { | ||
| value?: string; | ||
| onChange: (value: string) => void; | ||
| multiselect?: false; | ||
| renderTitle?: (option: DropdownOption<string>) => ReactElement; | ||
| } |
There was a problem hiding this comment.
renderTitle typing drops option icon
renderTitle is still typed as (option: DropdownOption<string>), so v2 consumers can’t read option.icon even though LoadableSelectBoxOption is what gets passed in — should we widen the callback to LoadableSelectBoxOption and drop the cast at the call site? That would avoid forcing icon duplication or local type assertions at the boundary.
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/components/LoadableSelectBox/LoadableSelectBox.tsx around
lines 60-65 where `LoadableSelectBoxOption` and the single-select `renderTitle` prop are
typed, fix the type inconsistency that currently prevents `renderTitle` callbacks from
safely reading `option.icon`. Refactor by changing the `renderTitle` prop type to accept
the actual option shape used by the component (e.g., `LoadableSelectBoxOption` or an
extended type that includes the optional `icon`) and then update the internal invocation
to pass `selectedOptions` without casting to the older shared contract. After the
change, remove any TypeScript cast/override at the callback boundary so consumer
callbacks have the correct type and no icon duplication/type assertions are required.
There was a problem hiding this comment.
Commit 981c2d6 addressed this comment by widening both renderTitle callback types to LoadableSelectBoxOption, allowing consumers to access option.icon. The internal invocation still uses casts, so that part of the suggestion was not fully implemented.
There was a problem hiding this comment.
Fixed in 981c2d6 — widened renderTitle to LoadableSelectBoxOption so consumers can read icon without casting.
| interface MultiSelectProps extends BaseLoadableSelectBoxProps { | ||
| value?: string[]; | ||
| onChange: (value: string[]) => void; | ||
| multiselect: true; | ||
| renderTitle?: (option: DropdownOption<string>[]) => ReactElement; | ||
| showSelectAll?: boolean; |
There was a problem hiding this comment.
Multi renderTitle typing drops option icon
renderTitle is typed as DropdownOption<string>[], so v2 callers lose LoadableSelectBoxOption.icon at the callback boundary and have to cast or duplicate data to render selected titles — should we type it as LoadableSelectBoxOption[]?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/components/LoadableSelectBox/LoadableSelectBox.tsx around
lines 67-72, the multi-select `renderTitle` prop is typed as `(option:
DropdownOption<string>[])` even though this component’s options are
`LoadableSelectBoxOption` (which includes `icon?: ReactNode`). Refactor the prop/type
definitions so the callback receives `LoadableSelectBoxOption[]` (or whatever exact
option type the component passes to `renderTitle`), and then ensure the internal call
sites use that same type. This will align the types at the callback boundary so v2
consumers can read `option.icon` without type assertions or duplicated option mapping.
There was a problem hiding this comment.
Commit 981c2d6 addressed this comment by typing multi-select renderTitle with LoadableSelectBoxOption[], preserving access to icon. Internal callback invocation casts were updated to use the same option type.
There was a problem hiding this comment.
Fixed in 981c2d6 — widened renderTitle to LoadableSelectBoxOption[] (same as the single-select fix).
| className={cn( | ||
| "group flex cursor-pointer items-center gap-2 rounded-md px-3", | ||
| option.description ? "min-h-12 py-2" : "h-8", | ||
| !multiselect && selected | ||
| ? "bg-primary-100 text-primary hover:bg-secondary" |
There was a problem hiding this comment.
Single-select lacks non-color selected cue
In the v2 fork, single-select rows only signal selection through theme colors and the Checkbox is gated behind multiselect, so the selected option stays ambiguous in the DOM for screen readers, high-contrast mode, and keyboard workflows — should we add an explicit selected marker or aria-selected on the row?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/components/LoadableSelectBox/LoadableSelectBox.tsx around
lines 349-353 (the single-select option row rendering logic inside the map/render for
options), the selected state is expressed only via conditional Tailwind classes and
there is no explicit selected marker element for single-select, plus the row is a plain
<div> without role/aria-selected. Update this option row to include proper listbox
semantics (set role="option" and aria-selected={selected}) and add a non-color-dependent
selected indicator for single-select (e.g., render a check icon or a visually-hidden
“Selected” span) while keeping the Checkbox gated behind multiselect. Ensure the
selected indicator updates in the same branch that currently toggles the bg/text classes
so DOM and visuals agree for screen readers and high-contrast/keyboard users.
There was a problem hiding this comment.
Commit 981c2d6 addressed this comment by adding listbox semantics and aria-selected={selected} to each option row, making single-select state available to assistive technologies without relying solely on color.
There was a problem hiding this comment.
Fixed in 981c2d6 — added role="listbox"/role="option" + aria-selected (and aria-multiselectable for multiselect) so selection is exposed to assistive tech. No visual change — the check icon stays removed per design.
|
🌙 Nightly cleanup: The test environment for this PR ( |
Design feedback: option rows in the base Select (e.g. the Filters "Column" picker) used 8px horizontal padding, inconsistent with the 12px rows of the migrated dropdowns. Bump the base SelectItem padding px-2 -> px-3 so all select list items share 12px x-padding. Single-line rows are already 32px tall (py-1.5 + text-sm), so no height change is needed. Note: this is the shared base Select primitive, so it affects every Select across v1 and v2, not only v2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses the Baz "extra list scan on every render" finding: filteredSelectedCount re-filtered filteredOptions on every render (hot path when the popover is open / typing). Wrap it in useMemo keyed on multiselect/filteredOptions/selectedValuesSet so it recomputes only when its inputs change. Pure perf, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In-scope fixes for code introduced by this v2 fork: - a11y: after dropping the check icon, single-select selection was conveyed by color only. Add role="listbox"/role="option" + aria-selected on the rows (and aria-multiselectable for the multiselect case) so the selected option is exposed to screen readers / high-contrast / keyboard — no visual change. - Widen renderTitle callbacks to LoadableSelectBoxOption so v2 consumers can read the new optional `icon` field without casting. - Rename NoOptions' props type SelectBoxProps -> NoOptionsProps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
| <div | ||
| className="max-h-[40vh] overflow-y-auto overflow-x-hidden" | ||
| role="listbox" | ||
| aria-multiselectable={multiselect || undefined} | ||
| > | ||
| {isLoading && ( | ||
| <div className="flex items-center justify-center"> | ||
| <Spinner /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
The role="listbox" <div> is non-focusable and contains the loading spinner and NoOptions subtree, so screen readers see non-option descendants inside the listbox and keyboard users have no way to navigate or select — should we restrict the listbox subtree to only role="option" rows (moving loading/empty states outside or marking them aria-hidden), and add tabIndex/onKeyDown/aria-activedescendant support for ArrowUp/ArrowDown/Enter/Space?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code in
apps/opik-frontend/src/v2/components/LoadableSelectBox/LoadableSelectBox.tsx around
lines 335-344. Two related ARIA issues to fix together: 1. **Listbox content**: The `div
role="listbox"` conditionally renders a loading spinner and `NoOptions`/`Button` subtree
as children, which violates the ARIA listbox model (only `role="option"` descendants are
allowed). Move the loading container and empty-state rendering (`emptyState ? emptyState
: <NoOptions .../>`) outside the `role="listbox"` div as siblings. If they must stay
inside, wrap them with `aria-hidden={true}` and `role="presentation"`, and ensure the
interactive "Load more items" Button is not inside the listbox subtree. Verify that the
only elements inside `role="listbox"` are option rows with `role="option"` and correct
`aria-selected`. 2. **Keyboard navigation**: The `role="listbox"` container is a
non-focusable `<div>` and `role="option"` rows only handle `onClick`, leaving keyboard
users unable to focus or navigate the list. Refactor to make the listbox focusable (add
`tabIndex` and manage active focus state) and implement an `onKeyDown` handler
supporting ArrowUp/ArrowDown (and Home/End) to move the active option, and Enter/Space
to select it. Update option rows (around where `role="option"` and `aria-selected` are
set, and the selection logic in the `onClick` handler near ~367) to participate in a
roving tabindex or `aria-activedescendant` model.
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
The "Add {entity}" create footers rendered ListAction with no variant, so they
fell back to the blue `primary` style. Match the Project selector's footer by
passing variant="default" (neutral text-foreground) on the create actions in
AddToDatasetDialog, FeedbackDefinitionsSelectBox, and PromptsSelectBox.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Details
Migrates the searchable single/multi-select popover (
LoadableSelectBox) to the new design system on v2 pages, so it reads consistently with the migrated DSSelectand the Project/Workspace selectors. This is a visual-only migration — no behavior changes, and v1 stays byte-identical.LoadableSelectBoxis shared by v1 and v2, so instead of an in-place restyle we fork a v2-owned copy atsrc/v2/components/LoadableSelectBox/and repoint only the 12 v2 consumers.h-10/px-4→h-8/px-3; selected single-select row filled (bg-primary-100/text-primary); group headerstext-light-slate; in-popover search field compactdimension="sm"; create/action footer normalized to<ListAction size="sm">(AddToDatasetDialograw<div>→ListAction, also fixes keyboard a11y).iconon the fork's local option type (opt-in, unused by default).src/shared/LoadableSelectBoxand all v1 usages are untouched.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Commands (in
apps/opik-frontend):npx tsc --noEmit— cleannpx eslint <changed files>— cleannpx vitest run src/v2/components/LoadableSelectBox/LoadableSelectBox.test.tsx— 4/4 pass (collapsed-trigger render + title logic)Scenarios validated:
src/shared/LoadableSelectBoxunchanged (verified viagit diff— zero changes undersrc/v1/**andsrc/shared/LoadableSelectBox/**).Environment: local Vite dev server proxied to a local backend (process mode), Chrome.
Not run: full v1 regression suite — out of scope by design (v1 is untouched).
Video evidence: none (visual diff is small and covered by the in-browser drive above).
Documentation
No documentation changes.
🤖 Generated with Claude Code