fix(F0Link): resolve stability audit BLOCKING issues#3903
fix(F0Link): resolve stability audit BLOCKING issues#3903eliseo-juan wants to merge 1 commit intomainfrom
Conversation
- Remove href || '#' fallback; pass href conditionally so Action can use
the no-href span/button branch instead of triggering page scroll-to-top
- Add aria-hidden={true} to decorative ExternalLink icon
- Add sr-only 'opens in new tab' span for external links (WCAG 3.2.2 G201)
- Add i18n key link.opensInNewTab to i18n-provider-defaults.ts
- Remove props.title fallback from aria-label (violates WCAG 2.5.3)
- Fix tests: import from @/testing/test-utils, use zeroRender
- Fix test: replace DOM traversal querySelector with accessible sr-only query
- Fix stories: Meta<typeof F0Link> instead of Meta<ComponentProps<...>>
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
📦 Alpha Package Version PublishedUse Use |
🔍 Visual review for your branch is published 🔍Here are the links to: |
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Resolves stability-audit BLOCKING findings for F0Link by improving external-link accessibility, aligning tests with the repo’s testing utilities, and tightening Storybook typing for better inference.
Changes:
- Add external-link a11y affordances (decorative icon
aria-hidden, SR-only “opens in new tab”, removetitle→aria-labelfallback, and removehref="#"fallback behavior). - Update
F0Linkunit tests to usezeroRenderand avoid DOM-structure-coupled assertions. - Adjust Storybook meta typing to
satisfies Meta<typeof F0Link>.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/lib/providers/i18n/i18n-provider-defaults.ts | Adds default i18n string for the external-link SR-only warning. |
| packages/react/src/components/F0Link/F0Link.tsx | Implements a11y fixes and conditional href handling for F0Link. |
| packages/react/src/components/F0Link/tests/index.test.tsx | Migrates tests to zeroRender and uses an accessible assertion for the external indicator. |
| packages/react/src/components/F0Link/stories/F0Link.stories.tsx | Updates Storybook Meta typing to improve arg-type inference. |
Comments suppressed due to low confidence (1)
packages/react/src/components/F0Link/F0Link.tsx:37
F0Linkcan now omithref, which meansActionwill render a<button>(not an<a>). The component is still typed asforwardRef<HTMLAnchorElement, F0LinkProps>andhandleClickis typed withReact.MouseEvent<HTMLAnchorElement>, so the public ref/event types no longer match what can be rendered. Consider updating the ref generic andF0LinkPropsto a discriminated union (link vs button) so consumers get correct types without needing casts.
const _F0Link = forwardRef<HTMLAnchorElement, F0LinkProps>(function Link(
{
className,
children,
stopPropagation = false,
"aria-label": ariaLabel,
href,
...props
},
ref
) {
const i18n = useI18n()
const { target } = props
const external = target === "_blank"
const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => {
if (stopPropagation) {
| // Build Action props: include href only when provided so that Action can | ||
| // fall back to a button/span when no href is given (WCAG / no scroll-to-top). | ||
| const actionProps: ActionProps = ( | ||
| href !== undefined | ||
| ? { | ||
| ...props, | ||
| href, | ||
| onClick: handleClick, | ||
| rel: external ? "noopener noreferrer" : undefined, | ||
| "aria-label": ariaLabel, | ||
| className, | ||
| } | ||
| : { | ||
| ...props, | ||
| onClick: | ||
| handleClick as unknown as React.MouseEventHandler<HTMLButtonElement>, | ||
| "aria-label": ariaLabel, | ||
| className, | ||
| } | ||
| ) as ActionProps |
There was a problem hiding this comment.
actionProps is being forced to ActionProps via a cast and an as unknown as React.MouseEventHandler<HTMLButtonElement> cast. This defeats the ActionLinkProps/ActionButtonProps type safety and makes it easy to accidentally pass link-only props when rendering as a button. Prefer restructuring so TypeScript can infer the correct branch (e.g., build two separately typed prop objects and render <Action> conditionally), which should also remove the need for the onClick cast.
| ...props, | ||
| href, | ||
| onClick: handleClick, | ||
| rel: external ? "noopener noreferrer" : undefined, |
There was a problem hiding this comment.
Action already derives rel="noopener noreferrer" internally when target === "_blank" (it overrides any rel passed through props). Setting rel in F0Link is redundant and can be misleading to future readers (especially if they expect it to affect the rendered output). Consider removing rel from F0Link’s actionProps and letting Action be the single source of truth.
| rel: external ? "noopener noreferrer" : undefined, |
| }, | ||
| ref | ||
| ) { | ||
| const i18n = useI18n() | ||
| const { target } = props | ||
| const external = target === "_blank" | ||
|
|
There was a problem hiding this comment.
_F0Link is a forwardRef component but doesn’t set an explicit displayName. This repo’s conventions expect forwardRef components to set displayName, and it also helps withDataTestId preserve a meaningful name in DevTools. Add _F0Link.displayName = "F0Link" after the declaration (before wrapping with withDataTestId).
Summary
Resolves BLOCKING findings from stability audit issue #3877 for
F0Link.Changes
Accessibility
aria-hidden={true}to<F0Icon icon={ExternalLink} />so screen readers skip it<span className="sr-only">{i18n.link.opensInNewTab}</span>whenexternalis true (WCAG 2.1 SC 3.2.2 / G201)link.opensInNewTab: "opens in new tab"toi18n-provider-defaults.tsprops.titlefallback — only passaria-labelwhen explicitariaLabelprop is provided (fixes WCAG 2.5.3 "Label in Name" violation)href || "#"— passhrefconditionally soAction/linkHandlercan render<button>/<span>when no href given, preventing unwanted scroll-to-topTests
import { render, screen } from "@testing-library/react"withimport { zeroRender, screen } from "@/testing/test-utils"querySelector("svg")with accessiblegetByText("opens in new tab")queryStories
satisfies Meta<ComponentProps<typeof F0Link>>→satisfies Meta<typeof F0Link>Testing
pnpm tsc --noEmit✅pnpm lint✅pnpm vitest:ci src/components/F0Link✅ (7 passed)Closes #3877