fix(F0Alert): stability audit BLOCKING fixes#3900
Conversation
- Add alertVariantOptions const array, derive AlertVariant from it - Make variant optional in F0AlertProps (has default in component) - Remove dead useRef / containerRef - Add role=alert (critical/warning) or role=status to alert container - Add sr-only new-tab warning to target=_blank link - Fix meta to use satisfies pattern, StoryObj<typeof meta> - Import alertVariantOptions for argTypes options - Add withSnapshot import and Snapshot story (all 5 variants) - Create __tests__/F0Alert.test.tsx (13 tests, full coverage)
✅ 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
Addresses the BLOCKING findings from the F0Alert stability audit (#3873) by aligning types with component defaults, improving accessibility semantics, updating Storybook patterns, and adding behavioral tests for the component.
Changes:
- Export
alertVariantOptionsand deriveAlertVariantfrom it; makevariantoptional to match the component default. - Add variant-derived ARIA role to the alert container and add an sr-only “opens in new tab” warning for
_blanklinks. - Update Storybook meta/story typings and add Snapshot story; add a new Vitest test suite covering core behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/components/F0Alert/types.ts | Introduces exported alertVariantOptions, derives AlertVariant, and makes variant optional. |
| packages/react/src/components/F0Alert/F0Alert.tsx | Removes dead ref, adds container role logic, and adds sr-only new-tab warning in link. |
| packages/react/src/components/F0Alert/stories/F0Alert.stories.tsx | Updates Storybook meta/story typing patterns, uses alertVariantOptions in argTypes, and adds Snapshot story. |
| packages/react/src/components/F0Alert/tests/F0Alert.test.tsx | Adds a new test suite covering rendering, actions/links, roles, default variant, and dataTestId. |
| const alertRole = | ||
| variant === "critical" || variant === "warning" ? "alert" : "status" | ||
|
|
||
| return ( | ||
| <div ref={containerRef} className="@container"> | ||
| <div className={alertVariants({ variant })}> | ||
| <div className="@container"> | ||
| <div role={alertRole} className={alertVariants({ variant })}> |
There was a problem hiding this comment.
The new container role={alertRole} may not have the intended effect because F0AvatarAlert renders role="alert" unconditionally. That means info/positive variants still expose a nested role="alert" region (and critical/warning may expose multiple). Consider hiding the avatar/icon wrapper from the accessibility tree (e.g., aria-hidden) and/or ensuring F0AvatarAlert doesn’t set role="alert" when used decoratively inside an alert container.
| it("renders role=alert for critical variant", () => { | ||
| render(<F0Alert title="Critical" description="Urgent" variant="critical" />) | ||
|
|
||
| // The alert container and F0AvatarAlert both use role="alert" for critical/warning | ||
| const alertRegions = screen.getAllByRole("alert") | ||
| expect(alertRegions.length).toBeGreaterThanOrEqual(1) | ||
| }) | ||
|
|
||
| it("renders role=alert for warning variant", () => { | ||
| render( | ||
| <F0Alert title="Warning" description="Be careful" variant="warning" /> | ||
| ) | ||
|
|
||
| // The alert container and F0AvatarAlert both use role="alert" for warning | ||
| const alertRegions = screen.getAllByRole("alert") | ||
| expect(alertRegions.length).toBeGreaterThanOrEqual(1) | ||
| }) | ||
|
|
||
| it("renders role=status for info variant", () => { | ||
| render(<F0Alert title="Info" description="FYI" variant="info" />) | ||
|
|
||
| expect(screen.getByRole("status")).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
The role-related tests are currently too permissive: getAllByRole("alert") can pass due to F0AvatarAlert’s own role="alert", even if the alert container role logic regresses. Consider asserting the role on the specific container element (e.g., the element wrapping title/description), and for non-critical/warning variants assert there are no role="alert" regions rendered.
🔍 Visual review for your branch is published 🔍Here are the links to: |
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Summary
Applies all BLOCKING findings from stability audit issue #3873.
alertVariantOptionsconst array — export fromtypes.ts, deriveAlertVariantfrom it; use in stories argTypesvariantnow optional — has default"neutral"in component, type must matchuseRef/containerRef— created but never readroleon alert container —role="alert"for critical/warning,role="status"otherwise (WCAG 4.1.3)<span className="sr-only"> (opens in new tab)</span>ontarget="_blank"link (WCAG 2.4.4)satisfies Meta<typeof F0Alert>,StoryObj<typeof meta>,alertVariantOptionsin argTypes,Snapshotstory with all 5 variants__tests__/F0Alert.test.tsx— 13 tests covering all behaviorsRelated
Quality gate
pnpm tsc --noEmit✅pnpm lint✅pnpm vitest:ci src/components/F0Alert✅ (13/13 passing)