fix(F0Select): resolve stability audit BLOCKING issues#3902
fix(F0Select): resolve stability audit BLOCKING issues#3902eliseo-juan wants to merge 1 commit intomainfrom
Conversation
- Replace hardcoded aria-label with i18n key (actions.removeItem) in SelectionPreview - Add aria-hidden='true' to decorative icon wrapper in SelectItem - Fix handleApply stale closure: add handleChangeOpenLocal to deps - Change custom trigger wrapper from div to button for Radix combobox merge - Remove duplicate aria-label from inner InputField button - Use useId() for stable selectAllId in SelectAll (no hardcoded id) - Replace console.log onChange with vi.fn() in tests - Fix openSelect helper: add fireEvent.animationStart to enable virtualizer - Replace container.querySelector with screen.getByRole for clear button - Replace console.log callbacks with void in stories - Replace args as any with typed F0SelectProps<string> cast in stories
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
📦 Alpha Package Version PublishedUse Use |
There was a problem hiding this comment.
Pull request overview
Resolves stability-audit BLOCKING issues for F0Select, focusing on accessibility fixes, a stale-closure bug, and test/story cleanup.
Changes:
- Improves a11y: i18n for “remove item” labels, hides decorative icons from SRs, stabilizes
SelectAllcheckbox IDs, and adjusts trigger semantics. - Fixes
handleApplystale-closure by correctinguseCallbackdependencies. - Cleans up tests/stories (removes
console.log, improves event usage and queries, removesas anycasts).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/lib/providers/i18n/i18n-provider-defaults.ts | Adds actions.removeItem translation key for i18n’d remove labels. |
| packages/react/src/components/F0Select/F0Select.tsx | Fixes handleApply deps and adjusts trigger rendering/ARIA behavior. |
| packages/react/src/components/F0Select/components/SelectItem.tsx | Marks decorative icon wrapper as aria-hidden to avoid corrupting option names. |
| packages/react/src/components/F0Select/components/SelectionPreview.tsx | Replaces hardcoded English aria-label with i18n translation key + interpolation. |
| packages/react/src/components/F0Select/components/SelectAll.tsx | Replaces hardcoded checkbox id with useId() to avoid duplicate IDs. |
| packages/react/src/components/F0Select/tests/F0Select.test.tsx | Aligns with testing conventions and improves interaction/query patterns. |
| packages/react/src/components/F0Select/stories/F0Select.stories.tsx | Removes console.log and replaces as any with typed prop casts. |
Comments suppressed due to low confidence (1)
packages/react/src/components/F0Select/F0Select.tsx:884
- This trigger
<button>doesn’t settype="button". Inside a form, the defaulttype="submit"can cause unintended form submission when opening the select. Settingtype="button"avoids relying onpreventDefault()and keeps behavior consistent.
<button
className="flex w-full items-center justify-between"
onClick={(e) => {
e.preventDefault()
}}
>
| <button | ||
| className="flex w-full items-center justify-between" | ||
| aria-label={label || placeholder} | ||
| > | ||
| {children} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
The custom-trigger branch wraps children inside a new <button> while SelectTrigger is already using asChild. Since children is typed as ReactNode, existing callers can (and do) pass interactive/focusable elements (e.g. a <button> or a tabIndex={0} div). Wrapping that inside another <button> produces invalid nested interactive HTML and can break focus/keyboard behavior and accessible naming (the wrapper’s aria-label will override the child’s label).
Consider making the trigger element itself be the asChild host (require children to be a single ReactElement and render it directly), and enforce/validate that it’s an actual interactive element (ideally a <button type="button">).
| const defaultSelectProps = { | ||
| error: undefined, | ||
| icon: undefined, | ||
| loading: false, | ||
| clearable: false, | ||
| labelIcon: undefined, | ||
| size: "md" as const, | ||
| disabled: false, | ||
| placeholder: "", | ||
| label: "Pick an option", | ||
| hideLabel: false, | ||
| onChange: (value: string) => { | ||
| console.log(value) | ||
| }, | ||
| onChange: vi.fn(), |
There was a problem hiding this comment.
defaultSelectProps sets size: "md", but F0Select defaults to size = "sm" (packages/react/src/components/F0Select/F0Select.tsx:109). If the intent is to test defaults, consider removing the explicit size (or set it to "sm") so tests don’t accidentally rely on a non-default size.
🔍 Visual review for your branch is published 🔍Here are the links to: |
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary
Resolves BLOCKING findings from stability audit issue #3874 for
F0Select.Changes
Accessibility
`Remove ${item.label}`aria-label with i18n keyactions.removeItem(new key added toi18n-provider-defaults.ts)aria-hidden="true"to decorative icon wrapper<div>so screen readers skip it<div>to<button>so Radix mergescomboboxrole onto interactive hostaria-label={label || placeholder}(already on outer trigger)useId()for stable id instead of hardcodedid="select-all"Bug fixes
handleChangeOpenLocaltouseCallbackdeps arrayTest / story quality
console.logonChange withvi.fn()in test setupopenSelecthelper: restorefireEvent.animationStartto enable virtualizer in JSDOMcontainer.querySelectorwithscreen.getByRolefor clear buttonconsole.logcallbacks withvoidin storiesargs as anywith typedF0SelectProps<string>cast in storiesTesting
pnpm tsc --noEmit✅pnpm lint✅pnpm vitest:ci src/components/F0Select✅ (12 passed, 1 skipped)Closes #3874