UI: A11y consolidation branch - for CI - do not merge #32458
Conversation
Sidnioulz
changed the title
|
View your CI Pipeline Execution ↗ for commit df215d6
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 508 KB | 478 KB | 🎉 -31 KB 🎉 |
| Dependency size | 2.80 MB | 2.80 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-docs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 2.07 MB | 2.06 MB | 🎉 -9 KB 🎉 |
| Dependency size | 9.44 MB | 9.43 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-onboarding
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 332 KB | 368 KB | 🚨 +36 KB 🚨 |
| Dependency size | 667 B | 667 B | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-vitest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 6 | 6 | 0 |
| Self size | 501 KB | 490 KB | 🎉 -11 KB 🎉 |
| Dependency size | 1.53 MB | 1.53 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 11 | 11 | 0 |
| Self size | 330 KB | 319 KB | 🎉 -10 KB 🎉 |
| Dependency size | 1.30 MB | 1.30 MB | 🚨 +18 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 44 | 🚨 +1 🚨 |
| Self size | 30.16 MB | 42.62 MB | 🚨 +12.46 MB 🚨 |
| Dependency size | 17.30 MB | 17.33 MB | 🚨 +29 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 14 | 14 | 0 |
| Self size | 23 KB | 23 KB | 🎉 -18 B 🎉 |
| Dependency size | 1.67 MB | 1.66 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 531 | 531 | 0 |
| Self size | 950 KB | 939 KB | 🎉 -11 KB 🎉 |
| Dependency size | 58.43 MB | 58.41 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 4.10 MB | 4.01 MB | 🎉 -81 KB 🎉 |
| Dependency size | 21.62 MB | 21.59 MB | 🎉 -31 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 14 | 14 | 0 |
| Self size | 14 KB | 14 KB | 🚨 +18 B 🚨 |
| Dependency size | 1.65 MB | 1.64 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 157 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 23.01 MB | 22.97 MB | 🎉 -31 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 114 | 0 |
| Self size | 37 KB | 37 KB | 🎉 -24 B 🎉 |
| Dependency size | 19.56 MB | 19.53 MB | 🎉 -31 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 25 KB | 25 KB | 0 B |
| Dependency size | 43.35 MB | 43.33 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 19 | 19 | 0 |
| Self size | 59 KB | 59 KB | 🎉 -52 B 🎉 |
| Dependency size | 26.79 MB | 26.78 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/sveltekit
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 20 | 20 | 0 |
| Self size | 58 KB | 58 KB | 0 B |
| Dependency size | 26.85 MB | 26.84 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 109 | 109 | 0 |
| Self size | 38 KB | 38 KB | 🚨 +30 B 🚨 |
| Dependency size | 43.78 MB | 43.77 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 15 | 15 | 0 |
| Self size | 20 KB | 20 KB | 0 B |
| Dependency size | 1.70 MB | 1.68 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 188 | 🚨 +1 🚨 |
| Self size | 904 KB | 890 KB | 🎉 -14 KB 🎉 |
| Dependency size | 79.76 MB | 92.25 MB | 🚨 +12.49 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 170 | 🚨 +1 🚨 |
| Self size | 35 KB | 35 KB | 🚨 +42 B 🚨 |
| Dependency size | 76.19 MB | 88.67 MB | 🚨 +12.49 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 45 | 🚨 +1 🚨 |
| Self size | 1.55 MB | 1.55 MB | 🚨 +8 B 🚨 |
| Dependency size | 47.46 MB | 59.94 MB | 🚨 +12.49 MB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 22 KB | 12 KB | 🎉 -10 KB 🎉 |
| Dependency size | 785 B | 785 B | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 68 | 68 | 0 |
| Self size | 36 KB | 26 KB | 🎉 -10 KB 🎉 |
| Dependency size | 5.97 MB | 5.97 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 894 KB | 884 KB | 🎉 -10 KB 🎉 |
| Dependency size | 28 KB | 18 KB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
vanessayuenn
requested review from
kylegach and
jonniebigodes
as code owners
Sidnioulz
force-pushed
the
a11y-consolidation
branch
from
e940687 to
fa2c259
Compare
WalkthroughThis PR replaces numerous IconButton/tooltip UIs with Button/ToggleButton and new Popover/WithPopover/Tooltip primitives, introduces Select and ToggleButton components, and refactors Tabs/Toolbar/Bar into ARIA-driven components (TabsView, TabList, TabPanel, AbstractToolbar/Toolbar). Modal is reimplemented with react-aria, exposing ModalContext and Close. A new accessibility model adds Button.ariaLabel, ariaDescription, and shortcut support, plus InteractiveTooltipWrapper. Tooling shifts from tooltip lists to Select across backgrounds, viewport, pseudo-states, and themes. A CSF test-syntax feature lands (Story.test, toTestId, vitest transformer), adds subtype to story index entries, and updates manager/preview to handle tests. Theming tokens/colors and many stories/tests are updated accordingly. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Btn as Button
participant ITW as InteractiveTooltipWrapper
participant TT as Tooltip/Popover
User->>Btn: Focus/Hover/Click
activate Btn
Btn-->>ITW: props { ariaLabel, tooltip, shortcut }
ITW-->>TT: show tooltip? (based on props, global shortcuts enabled)
TT-->>User: Tooltip (label [+ shortcut]) [optional]
Btn-->>User: onClick handler
deactivate Btn
sequenceDiagram
autonumber
actor User
participant Sel as Select
participant Pop as MinimalistPopover
participant App as Feature State
User->>Sel: Click/Key (Enter/Space/Arrows)
Sel->>Pop: open listbox
Pop-->>User: Options (+ Reset when provided)
User->>Pop: Select/Deselect option(s)
Pop-->>Sel: onSelect/onDeselect/onChange
Sel-->>App: Update globals/state (e.g., background/viewport/pseudo-states)
sequenceDiagram
autonumber
actor User
participant Trig as DialogTrigger
participant Modal as Modal (react-aria)
participant Ctx as ModalContext
participant App as onOpenChange
User->>Trig: Click open
Trig->>Modal: open=true
Modal->>Ctx: provide { close }
User->>Modal: Click Close / Escape / Outside
Modal->>App: onOpenChange(false)
App-->>Modal: controlled open sync
sequenceDiagram
autonumber
participant SB as CSF Factories
participant IDX as StoryIndexGenerator
participant MAN as Manager API
participant VIT as Vitest Transformer/Runner
SB-->>SB: story.test(name, fn) -> create child tests
SB-->>IDX: indexInputs (stories + tests, subtype)
IDX-->>MAN: Prepared index (subtype, parent links)
VIT-->>SB: loadCsf -> getStoryTests
VIT-->>VIT: generate describe/test with toTestId, DOUBLE_SPACES
VIT-->>Runner: execute tests (per story/test)
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
code/core/src/manager/components/mobile/about/MobileAbout.tsx (1)
127-141: Restore focus-visible styles after all: unset on anchors.Keyboard users may lose focus indication.
const LinkLine = styled.a(({ theme }) => ({ all: 'unset', display: 'flex', alignItems: 'center', justifyContent: 'space-between', fontSize: theme.typography.size.s2 - 1, height: 52, borderBottom: `1px solid ${theme.appBorderColor}`, cursor: 'pointer', padding: '0 10px', + outline: 'none', + '&:focus-visible': { + outline: `2px solid ${theme.color.secondary}`, + outlineOffset: 2, + borderRadius: 4, + },code/core/src/manager/components/notifications/NotificationItem.tsx (1)
212-216: A11y: Avoid nesting a Button inside a Link.Button inside anchor is invalid and problematic for AT/keyboard. Render the link only around the content, with the dismiss button as a sibling.
- if (link) { - return ( - <NotificationLink to={link} duration={duration} style={{ zIndex }}> - <ItemContent icon={icon} content={content} /> - <DismissNotificationItem onDismiss={onDismiss} /> - </NotificationLink> - ); - } + if (link) { + return ( + <Notification duration={duration} style={{ zIndex }}> + <NotificationLink to={link}> + <ItemContent icon={icon} content={content} /> + </NotificationLink> + <DismissNotificationItem onDismiss={onDismiss} /> + </Notification> + ); + }Also applies to: 155-167
code/addons/a11y/src/components/VisionSimulator.tsx (1)
53-67: Fix React unknown prop leakage (“filter” prop on span)Passing a prop named filter to a DOM element will emit React warnings and may break SSR. Use a transient prop (e.g. $filter) and map it internally.
-const ColorIcon = styled.span<{ filter: string }>( +const ColorIcon = styled.span<{ $filter: string }>( { background: 'linear-gradient(to right, #F44336, #FF9800, #FFEB3B, #8BC34A, #2196F3, #9C27B0)', borderRadius: '1rem', display: 'block', height: '1rem', width: '1rem', }, - ({ filter }) => ({ - filter: getFilter(filter), + ({ $filter }) => ({ + filter: getFilter($filter), }), ({ theme }) => ({ boxShadow: `${theme.appBorderColor} 0 0 0 1px inset`, }) ); … - icon: <ColorIcon filter={name} />, + icon: <ColorIcon $filter={name} />,Also applies to: 77-79
code/core/src/manager/components/sidebar/Tree.tsx (4)
345-354: Don’t use a button for a non-interactive status; drop role and make it decorativeA button with role="status" is incorrect and ends up focusable without action. Render a decorative element instead.
- <StatusButton - ariaLabel={`Test status: ${statusValue.replace('status-value:', '')}`} - role="status" - type="button" - status={statusValue} - selectedItem={isSelected} - > - {icon} - </StatusButton> + <StatusButton asChild ariaLabel={false} status={statusValue} selectedItem={isSelected}> + <span aria-hidden="true">{icon}</span> + </StatusButton>
448-461: Same: decorative status dot for branchesMake the status indicator non-interactive and aria-hidden.
- <StatusButton - type="button" - status={itemStatus} - ariaLabel={`Test status: ${itemStatus.replace('status-value:', '')}`} - > - <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot"> - <UseSymbol type="dot" /> - </svg> - </StatusButton> + <StatusButton asChild ariaLabel={false} status={itemStatus}> + <span aria-hidden="true"> + <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot"> + <UseSymbol type="dot" /> + </svg> + </span> + </StatusButton>
136-156: Make the skip link available on all viewportsCurrently hidden under 600px; skip links must be reachable on mobile too. Use a visually-hidden pattern and reveal on focus.
-const SkipToContentLink = styled(Button)(({ theme }) => ({ - display: 'none', - '@media (min-width: 600px)': { - display: 'block', - fontSize: '10px', - overflow: 'hidden', - width: 1, - height: '20px', - boxSizing: 'border-box', - opacity: 0, - padding: 0, - - '&:focus': { - opacity: 1, - padding: '5px 10px', - background: 'white', - color: theme.color.secondary, - width: 'auto', - }, - }, -})); +const SkipToContentLink = styled(Button)(({ theme }) => ({ + position: 'absolute', + left: '-9999px', + top: 'auto', + width: '1px', + height: '1px', + overflow: 'hidden', + clip: 'rect(1px, 1px, 1px, 1px)', + clipPath: 'inset(50%)', + whiteSpace: 'nowrap', + border: 0, + padding: 0, + zIndex: 9999, + fontSize: '10px', + '&:focus': { + left: '8px', + top: '8px', + width: 'auto', + height: 'auto', + clip: 'auto', + clipPath: 'none', + overflow: 'visible', + padding: '6px 10px', + background: 'white', + color: theme.color.secondary, + boxShadow: `0 0 0 2px ${theme.color.secondary}`, + }, +}));
345-354: Remove role="status" from StatusButton usagesStatusButton in code/core/src/manager/components/sidebar/Tree.tsx (around line 347) still has role="status" — remove this attribute from the StatusButton. Tests in test-storybooks/portable-stories-kitchen-sink/component-testing.spec.ts query [role="status"] (multiple locations); update those tests if you remove the attribute. Do not change intentional role="status" uses in Loader (code/core/src/components/components/Loader/Loader.tsx:112,152) or StatusBadge (code/core/src/component-testing/components/StatusBadge.tsx:48).
code/core/src/component-testing/components/Interaction.tsx (1)
210-223: Remove aria-label to avoid overriding visible text.RowLabel has inner text (MethodCall). aria-label="Interaction step" masks that content for SR users. Let the visible text be the accessible name, or move extra context to aria-describedby.
- <RowLabel - aria-label="Interaction step" + <RowLabelcode/core/src/manager/components/preview/Toolbar.tsx (1)
32-40: Pass token array (API_KeyCollection) into Button.shortcut — not a human string.Button.shortcut is typed as API_KeyCollection and Button converts tokens to aria-keyshortcuts via shortcutToAriaKeyshortcuts; passing shortcutToHumanString(...) (a human string) breaks aria-keyshortcuts/tooltip composition. Change fullScreenMapper to return the token array from api.getShortcutKeys().fullScreen.
File: code/core/src/manager/components/preview/Toolbar.tsx (around lines 32–40)
- shortcut: shortcutToHumanString(api.getShortcutKeys().fullScreen), + shortcut: api.getShortcutKeys().fullScreen,
code/core/src/components/components/Button/helpers/InteractiveTooltipWrapper.tsx
Show resolved
Hide resolved
code/core/src/components/components/Button/helpers/InteractiveTooltipWrapper.tsx
Show resolved
Hide resolved
code/core/src/components/components/Button/helpers/useAriaDescription.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
code/core/src/use-sync-external-store/shim/index.js (1)
1-1: Add default export for wider interopSome consumers import the shim as a default. Export both named and default to be safe.
Apply:
-export { useSyncExternalStore } from 'react'; +import { useSyncExternalStore } from 'react'; +export { useSyncExternalStore }; +export default useSyncExternalStore;scripts/build/utils/generate-bundle.ts (1)
254-256: Guard fs.watch filename nullsfs.watch may pass a falsy filename; join() would throw. Add a guard.
Apply:
- watch(join(DIR_CWD, 'dist'), { recursive: true }, (_event, filename) => { - console.log(`compiled ${picocolors.cyan(join(DIR_REL, 'dist', filename))}`); - }); + watch(join(DIR_CWD, 'dist'), { recursive: true }, (_event, filename) => { + if (!filename) return; + console.log(`compiled ${picocolors.cyan(join(DIR_REL, 'dist', String(filename)))}`); + });code/core/src/use-sync-external-store/README.md (1)
3-3: Fix MD034 (bare URL) + tiny grammarFormat the link and fix “provide(s)”.
Apply:
-This package exists because React provide `use-sync-external-store` for React 16/17 compat, but they only provide it as a CJS package. See https://github.com/facebook/react/issues/11503. +This package exists because React provides `use-sync-external-store` for React 16/17 compat, but they only provide it as a CJS package. See [facebook/react#11503](https://github.com/facebook/react/issues/11503).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
code/core/package.json(2 hunks)code/core/src/manager/globals/exports.ts(3 hunks)code/core/src/use-sync-external-store/README.md(1 hunks)code/core/src/use-sync-external-store/package.json(1 hunks)code/core/src/use-sync-external-store/shim/index.d.ts(1 hunks)code/core/src/use-sync-external-store/shim/index.js(1 hunks)scripts/build/utils/generate-bundle.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/package.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
code/core/src/use-sync-external-store/README.md
3-3: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/core/src/manager/globals/exports.ts (2)
532-535: ToggleButton implemented and exported — no action required. Implementation: code/core/src/components/components/ToggleButton/ToggleButton.tsx; exported from code/core/src/components/index.ts (line 55).
518-522: Confirmed — Select is implemented and exported from the components barrel.
Implementation: code/core/src/components/components/Select/Select.tsx (form variant: code/core/src/components/components/Form/Select.tsx). Export: code/core/src/components/index.ts — line 56: export { Select } from './components/Select/Select';scripts/build/utils/generate-bundle.ts (2)
92-96: Alias LGTMMapping to the local ESM-friendly shim is the right call.
132-134: Good: propagate shared aliases into runtimePrevents alias divergence between build targets.
code/core/src/use-sync-external-store/shim/index.d.ts (1)
1-5: Export default in types to mirror runtimeAdd a default export so both named and default imports are typed.
File: code/core/src/use-sync-external-store/shim/index.d.ts (lines 1–5)
export function useSyncExternalStore<Snapshot>( subscribe: (onStoreChange: () => void) => () => void, getSnapshot: () => Snapshot, getServerSnapshot?: () => Snapshot ): Snapshot; +export default useSyncExternalStore;Ripgrep reported "No files were searched" when checking for default imports; manual verification required to confirm whether any default imports of "use-sync-external-store" exist in the repo.
code/core/src/use-sync-external-store/package.json (1)
1-36: ```shell
#!/bin/bash
set -euo pipefailecho "=== Target package.json ==="
if [ -f "code/core/src/use-sync-external-store/package.json" ]; then
echo "FOUND: code/core/src/use-sync-external-store/package.json"
jq '.' code/core/src/use-sync-external-store/package.json || true
else
echo "MISSING: code/core/src/use-sync-external-store/package.json"
fi
echoecho "=== Root package.json summary ==="
if [ -f package.json ]; then
jq '{name: .name, workspaces: .workspaces, packages: .packages, private: .private, publishConfig: .publishConfig}' package.json -c || true
else
echo "NO root package.json"
fi
echoecho "=== Monorepo manifests ==="
for f in pnpm-workspace.yaml pnpm-workspace.yml lerna.json turbo.json nx.json workspace.json; do
if [ -f "$f" ]; then
echo "FOUND: $f"
sed -n '1,200p' "$f"
else
echo "ABSENT: $f"
fi
echo
doneecho "=== Search for workspace / publish references (force include ignored/hidden) ==="
rg -n --hidden -uu -S 'workspaces|packages|publishConfig|publish|private' || true
echoecho "=== Search for occurrences of this package name/path ==="
rg -n --hidden -uu -S 'use-sync-external-store' || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/core/src/components/components/Select/Select.tsx (1)
366-371: Fix: start index when Reset exists and nothing is selected (condition inverted).When a reset option exists and nothing is selected, we should start on the first real option, not Reset.
- const listStart = resetOption && hasSelection ? 1 : 0; + const listStart = resetOption && !hasSelection ? 1 : 0;
🧹 Nitpick comments (4)
code/core/src/components/components/Select/Select.tsx (4)
387-392: Consistency: PageUp should honor listStart to avoid opening on Reset.Without this, pressing PageUp can open focused on Reset when nothing is selected.
- } else if (e.key === 'PageUp') { - openAt(Math.max(0, (hasSelection ? indexOfFirstSelected : listEnd) - PAGE_STEP_SIZE)); - } + } else if (e.key === 'PageUp') { + openAt( + Math.max( + listStart, + (hasSelection ? indexOfFirstSelected : listEnd) - PAGE_STEP_SIZE + ) + ); + }
258-277: Docstring mismatch: comment says Reset appears only when there’s a selection, but code always adds it when onReset is defined.Align the comment with behavior (or gate the item by selection length).
- // Reset option appears if a handler is defined and there are selected options. + // Reset option is available whenever onReset is provided; in multi-select it looks disabled + // when nothing is selected.
139-143: Outside click handling duplicated (useInteractOutside + useOverlay isDismissable).Both can close on outside interaction; keep one to avoid double-calls and simplify.
- useInteractOutside({ - ref: popoverRef, - onInteractOutside: handleClose, - });Optionally keep only
useOverlay({ isDismissable: true, ... })as you already do.Also applies to: 152-163
201-201: Unused ref: listboxRef isn’t read.Remove the ref to reduce noise.
- const listboxRef = useRef<HTMLUListElement>(null);- ref={listboxRef}Also applies to: 501-503
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
code/core/src/components/components/Button/Button.tsx(8 hunks)code/core/src/components/components/Select/Select.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/components/components/Button/Button.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/components/components/Select/Select.tsx (3)
code/core/src/components/components/Button/Button.tsx (2)
ButtonProps(15-50)Button(52-132)code/core/src/components/components/Select/helpers.tsx (3)
ResetOption(14-16)PAGE_STEP_SIZE(18-18)Listbox(20-28)code/core/src/components/components/Select/SelectOption.tsx (1)
SelectOption(97-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
381-396: Fix reversed ariaLabel and include “all” for clarityWhen fully expanded, the action is “Collapse all”; otherwise “Expand all”.
Apply:
- ariaLabel={isFullyExpanded ? 'Expand' : 'Collapse'} + ariaLabel={isFullyExpanded ? 'Collapse all' : 'Expand all'}
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
63-79: Don’t suppress focus ring; use :focus-visible with a clear indicatorCurrent rule removes outline on keyboard focus. Prefer :focus-visible with a visible focus style.
Apply:
- '&:hover, &:focus': { - outline: 'none', - background: 'var(--tree-node-background-hover)', - }, + '&:hover': { + background: 'var(--tree-node-background-hover)', + }, + '&:focus-visible': { + outline: '2px solid var(--sb-focus-color, currentColor)', + outlineOffset: 2, + background: 'var(--tree-node-background-hover)', + },
136-156: Make skip link available on all viewportsCurrently hidden with display:none below 600px, making it unreachable on mobile. Show at all widths; reveal on focus.
Apply:
-const SkipToContentLink = styled(Button)(({ theme }) => ({ - display: 'none', - '@media (min-width: 600px)': { - display: 'block', +const SkipToContentLink = styled(Button)(({ theme }) => ({ + display: 'block', fontSize: '10px', overflow: 'hidden', width: 1, height: '20px', boxSizing: 'border-box', opacity: 0, padding: 0, '&:focus': { opacity: 1, padding: '5px 10px', background: 'white', color: theme.color.secondary, width: 'auto', }, - }, }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Tree.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
code/core/src/manager/components/sidebar/StatusButton.tsx (1)
StatusButton(89-91)
🔇 Additional comments (4)
code/core/src/manager/components/sidebar/Tree.tsx (4)
4-4: LGTM: unified Button/ListItem importImport changes look correct and consistent with consolidation.
339-342: Skip link passthrough is correctUsing asChild with ariaLabel={false} is the right approach to avoid duplicate SR labeling.
Please confirm that an element with id="storybook-preview-wrapper" exists in the rendered DOM so the link target is valid.
345-353: Good: explicit SR label on status buttonClear, deterministic label for leaf status. No changes requested.
239-255: No action — TooltipLinkList correctly forwards 'aria-label'TooltipLinkList.NormalLink is derived from ListItemProps and TooltipLinkList renders ListItem with {...rest}; ListItem spreads {...rest} onto Item (the DOM element), so the hyphenated 'aria-label' in your Link object will be forwarded. See TooltipLinkList.tsx and ListItem.tsx.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
63-79: Hover/focus background likely broken here (undefined CSS var) + weak focus styleCollapseButton uses var(--tree-node-background-hover), but that var is defined on LeafNodeStyleWrapper, not on the root header. Result: no hover/focus background; plus outline is removed without an accessible replacement.
-const CollapseButton = styled.button({ +const CollapseButton = styled.button(({ theme }) => ({ all: 'unset', display: 'flex', padding: '0px 8px', borderRadius: 4, - transition: 'color 150ms, box-shadow 150ms', + transition: 'color 150ms, box-shadow 150ms, background 150ms', gap: 6, alignItems: 'center', cursor: 'pointer', height: 28, - '&:hover, &:focus': { - outline: 'none', - background: 'var(--tree-node-background-hover)', - }, -}); + // Use local fallback so hover/focus works outside LeafNode context + '&:hover, &:focus-visible': { + outline: 'none', + background: `var(--tree-node-background-hover, ${theme.background.content})`, + // Provide visible focus treatment + boxShadow: `0 0 0 2px ${theme.color.secondary}66`, + }, +}));
346-353: Status icon is non-interactive but rendered as a buttonNo onClick is provided; screen readers announce a “button” that does nothing. Render non-interactive content via asChild and label the graphic.
- <StatusButton - ariaLabel={`Test status: ${statusValue.replace('status-value:', '')}`} - data-testid="tree-status-button" - type="button" - status={statusValue} - selectedItem={isSelected} - > - {icon} - </StatusButton> + <StatusButton + asChild + status={statusValue} + data-testid="tree-status-button" + selectedItem={isSelected} + > + <span + role="img" + aria-label={`Test status: ${statusValue.replace('status-value:', '')}`} + > + {icon} + </span> + </StatusButton>
♻️ Duplicate comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
382-397: Fix reversed label; it should describe the action (“Expand all”/“Collapse all”)When fully expanded, action collapses; otherwise expands. Also include “all” for clarity.
- ariaLabel={isFullyExpanded ? 'Expand' : 'Collapse'} + ariaLabel={isFullyExpanded ? 'Collapse all' : 'Expand all'}
449-460: Non-interactive status dot should not be a buttonSame issue as noted earlier: no handler but button semantics. Use asChild and a labelled, non-interactive wrapper; hide the inner svg from SRs.
- <StatusButton - type="button" - status={itemStatus} - ariaLabel={`Test status: ${itemStatus.replace('status-value:', '')}`} - > - <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot"> - <UseSymbol type="dot" /> - </svg> - </StatusButton> + <StatusButton asChild status={itemStatus}> + <span + role="img" + aria-label={`Test status: ${itemStatus.replace('status-value:', '')}`} + > + <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot" aria-hidden="true"> + <UseSymbol type="dot" /> + </svg> + </span> + </StatusButton>
🧹 Nitpick comments (7)
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts (7)
98-100: Harden watch-mode cleanup (assert state after toggle).Avoid mixing awaits inside the conditional and assert it actually turns off. This reduces flake in teardown.
- const watchModeToggle = page.getByLabel("Watch mode", { exact: true }); - if (await watchModeToggle.isVisible() && await watchModeToggle.getAttribute("aria-checked") === "true") { - await watchModeToggle.click(); - } + const watchModeToggle = page.getByLabel("Watch mode", { exact: true }); + if (await watchModeToggle.isVisible()) { + const isOn = (await watchModeToggle.getAttribute("aria-checked")) === "true"; + if (isOn) { + await watchModeToggle.click(); + await expect(watchModeToggle).toHaveAttribute("aria-checked", "false"); + } + }
157-157: Anchor the error-filter regex for selector stability.Anchor start/end to avoid accidental partial matches if surrounding copy changes.
Example (apply to all occurrences shown in the ranges):
-const errorFilter = page.getByLabel(/Filter main navigation to show \d+ tests with errors/); +const errorFilter = page.getByLabel(/^Filter main navigation to show \d+ tests with errors$/);Also applies to: 231-231, 288-288
162-167: Prefer composed locators over CSS descendant selectors for status buttons.Bind via getByTestId filtered by the owning item; reduces coupling to DOM structure/spacing.
Example (replicate for each status lookup in the ranges):
-const failingStoryElement = page.locator( - '[data-item-id="addons-group-test--mismatch-failure"] [data-testid="tree-status-button"]' -); +const failingStoryElement = page + .locator('[data-item-id="addons-group-test--mismatch-failure"]') + .getByTestId("tree-status-button");Also applies to: 175-180, 236-241, 245-250, 293-299, 302-307, 349-354, 388-394
209-212: Use exact: true consistently for “Watch mode”.Some places use exact: true, others don’t. Align to avoid matching derivatives.
-const watchModeButton = await page.getByLabel("Watch mode"); +const watchModeButton = await page.getByLabel("Watch mode", { exact: true });
279-283: Replace fixed sleeps with state-based waits when enabling watch mode.Wait for aria-checked to reflect ON instead of arbitrary timeouts. This removes test flakiness.
Example (apply pattern in each block):
-await page.getByLabel("Watch mode", { exact: true }).click(); -// We shouldn't have to do an arbitrary wait, but because there is no UI for loading state yet, we have to -await page.waitForTimeout(8000); +const watchToggle = page.getByLabel("Watch mode", { exact: true }); +await watchToggle.click(); +await expect(watchToggle).toHaveAttribute("aria-checked", "true");And for 3000ms waits in other tests:
-await page.getByLabel("Watch mode").click(); -// We shouldn't have to do an arbitrary wait, but because there is no UI for loading state yet, we have to -await page.waitForTimeout(3000); +const watchToggle = page.getByLabel("Watch mode", { exact: true }); +await watchToggle.click(); +await expect(watchToggle).toHaveAttribute("aria-checked", "true");Also applies to: 333-337, 372-376, 412-416
460-461: Tighten the coverage label matcher.If the accessible name is expected to be exactly like “NN% coverage)”, anchor the start to avoid false positives.
- .getByLabel(/% coverage\)$/) + .getByLabel(/^\d+% coverage\)$/)Also applies to: 740-741
208-212: Minor naming nit: toggle vs button.Variable is named watchModeButton here but watchModeToggle elsewhere. Pick one to reduce cognitive overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/sidebar/Tree.tsx(7 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
code/core/src/manager/components/sidebar/StatusButton.tsx (1)
StatusButton(89-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
4-4: LGTM: consolidated Button/ListItem importMatches the Button-centric a11y refactor.
339-342: Skip link naming OK; confirm Button accepts ariaLabel={false}Accessible name comes from the anchor text. Please confirm Button’s ariaLabel typing allows boolean false (string | false); otherwise drop the prop.
Sidnioulz
force-pushed
the
a11y-consolidation
branch
2 times, most recently
from
10adeba to
a6cc1c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
63-78: Focus-visible regression on CollapseButton; add fallback and visible ringUsing var(--tree-node-background-hover) without a fallback plus outline: none can produce no visible focus in contexts where the var isn’t defined (e.g., root). Add a fallback color and a :focus-visible ring.
Apply:
-const CollapseButton = styled.button({ +const CollapseButton = styled.button(({ theme }) => ({ all: 'unset', display: 'flex', padding: '0px 8px', borderRadius: 4, transition: 'color 150ms, box-shadow 150ms', gap: 6, alignItems: 'center', cursor: 'pointer', height: 28, '&:hover, &:focus': { - outline: 'none', - background: 'var(--tree-node-background-hover)', + outline: 'none', + background: `var(--tree-node-background-hover, ${ + theme.base === 'dark' + ? darken(0.35, theme.color.secondary) + : lighten(0.45, theme.color.secondary) + })`, }, -}); + '&:focus-visible': { + outline: `2px solid ${theme.color.secondary}`, + outlineOffset: 2, + }, +}));
345-354: Non-interactive status icon should not be a button (leaf items)This advertises a “button” with no action. Render as decorative/labelled non-interactive content via asChild.
Apply:
- <StatusButton - ariaLabel={`Test status: ${statusValue.replace('status-value:', '')}`} - data-testid="tree-status-button" - type="button" - status={statusValue} - selectedItem={isSelected} - > - {icon} - </StatusButton> + <StatusButton + asChild + status={statusValue} + selectedItem={isSelected} + data-testid="tree-status-button" + > + <span + role="img" + aria-label={`Test status: ${statusValue.replace('status-value:', '')}`} + > + {icon} + </span> + </StatusButton>
♻️ Duplicate comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
449-460: Non-interactive status dot should not be a button (group/component)Same issue: button semantics without a handler. Render a non-interactive element via asChild; keep styling.
Apply:
- <StatusButton - type="button" - status={itemStatus} - ariaLabel={`Test status: ${itemStatus.replace('status-value:', '')}`} - > - <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot"> - <UseSymbol type="dot" /> - </svg> - </StatusButton> + <StatusButton asChild status={itemStatus}> + <span + role="img" + aria-label={`Test status: ${itemStatus.replace('status-value:', '')}`} + > + <svg key="icon" viewBox="0 0 6 6" width="6" height="6" type="dot"> + <UseSymbol type="dot" /> + </svg> + </span> + </StatusButton>
382-387: Fix reversed ariaLabel, and expose toggle state with aria-pressedWhen fully expanded, action is “Collapse all”; otherwise “Expand all”. Also surface toggle state for SRs.
Apply:
- <Button + <Button padding="small" variant="ghost" className="sidebar-subheading-action" - ariaLabel={isFullyExpanded ? 'Expand' : 'Collapse'} + ariaLabel={isFullyExpanded ? 'Collapse all' : 'Expand all'} + aria-pressed={isFullyExpanded} data-action="expand-all" data-expanded={isFullyExpanded} onClick={(event) => { event.preventDefault(); // @ts-expect-error (non strict) setFullyExpanded(); }} > {isFullyExpanded ? <CollapseIconSvg /> : <ExpandAltIcon />} - </Button> + </Button>Also applies to: 395-397
🧹 Nitpick comments (5)
code/core/src/manager/components/sidebar/Tree.tsx (1)
339-342: Avoid passing ariaLabel={false}If Button.ariaLabel isn’t typed to accept boolean, this can render aria-label="false" or TS errors. The link text already provides the accessible name.
Apply:
- <SkipToContentLink asChild ariaLabel={false}> + <SkipToContentLink asChild> <a href="#storybook-preview-wrapper">Skip to canvas</a> </SkipToContentLink>test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts (4)
98-100: Harden watch‑mode selection: scope to testing module and use role="switch".Prevents ambiguity if multiple “Watch mode” controls exist and stabilizes state checks.
Apply this diff (repeat the scoped switch click for all occurrences):
- const watchModeToggle = page.getByLabel("Watch mode", { exact: true }); - if (await watchModeToggle.isVisible() && await watchModeToggle.getAttribute("aria-checked") === "true") { - await watchModeToggle.click(); + const testingModule = page.locator('#storybook-testing-module'); + const watchSwitch = testingModule.getByRole('switch', { name: 'Watch mode', exact: true }); + if (await watchSwitch.isVisible() && (await watchSwitch.getAttribute('aria-checked')) === 'true') { + await watchSwitch.click(); + await expect(watchSwitch).toHaveAttribute('aria-checked', 'false'); }- const watchModeButton = await page.getByLabel("Watch mode"); + const watchModeSwitch = page.locator('#storybook-testing-module').getByRole('switch', { name: 'Watch mode', exact: true });- await page.getByLabel("Watch mode", { exact: true }).click(); + await page.locator('#storybook-testing-module').getByRole('switch', { name: 'Watch mode', exact: true }).click();- await page.getByLabel("Watch mode").click(); + await page.locator('#storybook-testing-module').getByRole('switch', { name: 'Watch mode', exact: true }).click();If the control is a button with aria-pressed (not a switch), use getByRole('button', { name: 'Watch mode' }) and assert aria-pressed instead.
Also applies to: 209-209, 279-279, 333-333, 372-372, 412-412
157-158: Make error‑filter label robust to singular/plural.Avoids flake when there’s exactly 1 error.
- const errorFilter = page.getByLabel(/Filter main navigation to show \d+ tests with errors/); + const errorFilter = page.getByLabel(/Filter main navigation to show \d+ (test|tests) with errors/);Apply the same change to all occurrences.
Also applies to: 231-233, 288-290
162-167: Reduce brittleness of status selectors; assert accessible name.Scope to the item container and use getByTestId on that container; prefer toHaveAccessibleName over raw aria-label.
- const failingStoryElement = page.locator( - '[data-item-id="addons-group-test--mismatch-failure"] [data-testid="tree-status-button"]' - ); - await expect(failingStoryElement).toHaveAttribute("aria-label","Test status: success"); + const failingStory = page.locator('[data-item-id="addons-group-test--mismatch-failure"]'); + const failingStoryButton = failingStory.getByTestId('tree-status-button'); + await expect(failingStoryButton).toHaveAccessibleName('Test status: success');Optionally add a tiny helper near the top of the file to DRY this up:
const treeStatus = (page: Page, id: string) => page.locator(`[data-item-id="${id}"]`).getByTestId('tree-status-button');Also applies to: 175-180, 236-241, 245-251, 293-299, 302-307, 349-355, 388-395
460-461: Guard against multiple matches and stray whitespace in coverage label.Minor stability tweak.
- const sbPercentageText = await page - .getByLabel(/% coverage\)$/) - .textContent(); + const sbPercentageText = (await page.getByLabel(/% coverage\)$/).first().textContent())?.trim() ?? "";Also applies to: 740-741
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/sidebar/Tree.tsx(7 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
code/core/src/manager/components/sidebar/StatusButton.tsx (1)
StatusButton(89-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (1)
4-4: LGTM: consolidated importsSwitch to Button/ListItem looks good and aligns with the new a11y API.
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts (1)
530-531: LGTM: status assertions via test id + accessible name.Good move to assert by test id and the user‑visible status string; clear and resilient.
Also applies to: 624-625, 629-630, 673-675, 678-680
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/core/src/manager/components/preview/tools/remount.tsx (1)
59-64: Set type="button" to avoid accidental form submissionsExplicitly set the button type; some button impls default to "submit".
<StyledAnimatedButton key="remount" padding="small" variant="ghost" ariaLabel="Reload story" + type="button" onClick={remountComponent}
🧹 Nitpick comments (11)
code/core/src/manager/components/preview/tools/remount.tsx (2)
18-20: Simplify styled generic; Pick<> is unnecessary noiseThe base Button props (incl. disabled) are already preserved by styled(Button). You can drop the Pick for cleaner types. If TS complains in this codebase, ignore this nit.
-const StyledAnimatedButton = styled(Button)< - AnimatedButtonProps & Pick<ComponentProps<typeof Button>, 'disabled'> ->(({ theme, animating, disabled }) => ({ +const StyledAnimatedButton = styled(Button)<AnimatedButtonProps>( + ({ theme, animating, disabled }) => ({ opacity: disabled ? 0.5 : 1, svg: { animation: animating ? `${theme.animation.rotate360} 1000ms ease-out` : undefined, }, - }, -})); + }) +);
61-64: Optional: add tooltip for discoverability (mouse users)Button now supports tooltip. Keeping ariaLabel is good; tooltip helps sighted users.
padding="small" variant="ghost" ariaLabel="Reload story" + tooltip="Reload story"code/core/src/manager/components/mobile/about/MobileAbout.tsx (7)
30-31: Avoid nested scroll containers (ScrollArea + Container).Container has overflow:auto while content also sits in a ScrollArea; this can cause double scrollbars and erratic touch/keyboard scrolling. Let ScrollArea own scrolling.
- overflow: 'auto', + overflow: 'hidden',Also applies to: 96-96
32-40: Align accessible name with visible label/icon.Visible text says “Back” with an Arrow icon, but ariaLabel/tooltip say “Close about section”. Either use CrossIcon + “Close”, or keep Arrow + “Back” and drop ariaLabel so the visible text is the name. Also mark the icon decorative.
- <CloseButton - onClick={() => setMobileAboutOpen(false)} - ariaLabel="Close about section" - tooltip="Close about section" - variant="ghost" - > - <ArrowLeftIcon /> + <CloseButton + onClick={() => setMobileAboutOpen(false)} + tooltip="Back" + variant="ghost" + > + <ArrowLeftIcon aria-hidden /> Back </CloseButton>
32-37: Optional: add Escape shortcut for parity with Modal close.Matches CloseButton in Modal and improves keyboard UX.
<CloseButton onClick={() => setMobileAboutOpen(false)} tooltip="Back" + shortcut={['Escape']} variant="ghost" >
42-47: Announce “opens in a new tab” for external links using target=_blank.Improve SR clarity by adding an aria-label per link.
<LinkLine href="https://github.com/storybookjs/storybook" target="_blank" rel="noopener noreferrer" + aria-label="GitHub (opens in a new tab)" > ... <LinkLine href="https://storybook.js.org/docs/react/get-started/install/?ref=ui" target="_blank" rel="noopener noreferrer" + aria-label="Documentation (opens in a new tab)" > ... - <Link href="https://chromatic.com" target="_blank" rel="noopener noreferrer"> + <Link + href="https://chromatic.com" + target="_blank" + rel="noopener noreferrer" + aria-label="Chromatic (opens in a new tab)" + >Also applies to: 53-57, 68-70
38-38: Mark decorative icons as aria-hidden.Prevents redundant announcements; text already conveys meaning.
- <ArrowLeftIcon /> + <ArrowLeftIcon aria-hidden /> ... - <GithubIcon /> + <GithubIcon aria-hidden /> ... - <ShareAltIcon width={12} /> + <ShareAltIcon width={12} aria-hidden /> ... - <StorybookIcon /> + <StorybookIcon aria-hidden /> ... - <ShareAltIcon width={12} /> + <ShareAltIcon width={12} aria-hidden />Also applies to: 48-48, 51-51, 59-59, 62-62
72-77: Consistency: open external “Community” link in a new tab (like others).Keeps users in the app.
<Link href="https://github.com/storybookjs/storybook/graphs/contributors" rel="noopener noreferrer" + target="_blank" >
41-41: Use nav semantics for the external links group.Improves structure for AT; add aria-label on the container.
- <LinkContainer> + <LinkContainer aria-label="Related links">-const LinkContainer = styled.div({}); +const LinkContainer = styled.nav({});Also applies to: 133-134
code/addons/a11y/src/components/VisionSimulator.tsx (2)
69-71: Simplify state to a stable key (string) instead of object.Only
nameis used; considerconst [filterName, setFilterName] = useState<string|null>(null)to reduce churn and make control wiring simpler.
72-81: Stabilize option identity and format description.
- Use a stable
id(e.g.,'deuteranopia') forvalue, and a localized label fortitle, so future i18n of names doesn’t breakurl(#id)lookups.- Minor: format percentages with
Intl.NumberFormat(e.g., max 2 decimals) to avoid0.0001%noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/addons/a11y/src/components/VisionSimulator.tsx(3 hunks)code/core/src/components/components/Button/helpers/InteractiveTooltipWrapper.tsx(1 hunks)code/core/src/components/components/Select/Select.tsx(1 hunks)code/core/src/manager/components/mobile/about/MobileAbout.tsx(5 hunks)code/core/src/manager/components/preview/Toolbar.tsx(3 hunks)code/core/src/manager/components/preview/tools/remount.tsx(3 hunks)code/core/src/manager/components/sidebar/Tree.tsx(7 hunks)code/core/src/theming/global.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/components/components/Button/helpers/InteractiveTooltipWrapper.tsx
- code/core/src/components/components/Select/Select.tsx
- code/core/src/manager/components/sidebar/Tree.tsx
- code/core/src/manager/components/preview/Toolbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager/components/mobile/about/MobileAbout.tsx (2)
code/core/src/components/index.ts (2)
ScrollArea(47-47)StorybookIcon(81-81)code/core/src/components/components/Modal/Modal.styled.tsx (1)
CloseButton(65-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (8)
code/core/src/manager/components/preview/tools/remount.tsx (1)
52-56: LGTM: FORCE_REMOUNT wiring fixed correctlySubscribing in useEffect with cleanup prevents handler leaks. Dependency on api is appropriate.
code/core/src/theming/global.ts (1)
119-122: SR-only improvements LGTM; keep as progressive enhancement.Adding whiteSpace: 'nowrap' and clipPath: 'inset(50%)' matches current a11y guidance and safely complements the legacy clip fallback.
Please sanity-check:
- VoiceOver (macOS/iOS) and NVDA: element remains announced and focusable.
- Safari iOS 15–16: fallback to clip works when clip-path support varies.
- .sb-hidden-until-focus: on focus, element re-enters normal flow correctly (since the sr-only rule no longer matches due to :not(:focus)).
code/core/src/manager/components/mobile/about/MobileAbout.tsx (3)
4-4: LGTM on new imports.
126-131: LGTM on layout extraction (InnerArea).
164-167: LGTM on mobile-friendly CloseButton sizing.code/addons/a11y/src/components/VisionSimulator.tsx (3)
53-67: LGTM: transient prop for styling.Using
$filteravoids DOM prop leakage; mapping to CSSfilteris clear.
93-101: Select is uncontrolled — defaultOptions + callbacks are correct here.Select does not expose a controlled
selected/selectedKeyprop; its API is built arounddefaultOptions,onSelect(option: string),onReset, andonChange. UsingdefaultOptions={filter?.name}andonSelect={s => setFilter({ name: s })}is fine.
Caveat: Select only syncs fromdefaultOptionswhen it’s truthy and relies on its reset handling to clear internal selection; if you need true controlled behavior, update the Select API to accept a controlledselectedprop and wire its state accordingly.Likely an incorrect or invalid review comment.
3-3: Confirm Select API and import surface
- Select props match core Select: defaultOptions?: string | string[]; onSelect?: (option: string) => void; ariaLabel: string. (code/core/src/components/components/Select/Select.tsx)
- Can't verify that 'storybook/internal/components' is a supported/public import surface for addons in this repo — confirm you intentionally depend on that internal path or switch to the public export (or the core component package).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/a11y/src/components/A11YPanel.stories.tsx (1)
3-5: Drop unused testing utilities after disabling story.With
ReadyWithResultscommented out,expect,userEvent, andwithinare now unused. ESLint/TypeScript will flag these imports, breaking the build. Please trim the import list.-import { expect, fn, userEvent, within } from 'storybook/test'; +import { fn } from 'storybook/test';
🧹 Nitpick comments (2)
code/core/src/components/components/Modal/Modal.stories.tsx (2)
241-243: Remove unused children from args where explicit children are provided.These stories pass children via args then override with explicit JSX; the args value is ignored, adding noise.
Apply these removals:
@@ StyledComponents args - children: <SampleModalContent />, @@ WithError args - children: <SampleModalContent />, @@ AlwaysOpen args - children: <SampleModalContent />, @@ LongContent args - children: <SampleModalContent />, @@ DialogTransitions args - children: <SampleModalContent />, @@ BottomDrawerTransitions args - children: <SampleModalContent />,Also applies to: 304-308, 351-356, 521-526, 575-580, 624-629
22-24: Avoid invoking test helpers at render; use an action for the demo button.Calling fn() on render recreates a mock each render. Use an action callback for consistency with the rest of the stories.
- <button onClick={fn()}>Sample Button</button> + <button onClick={() => action('sample-button-click')()}>Sample Button</button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/a11y/src/components/A11YPanel.stories.tsx(1 hunks)code/core/src/components/components/Modal/Modal.stories.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/components/components/Modal/Modal.stories.tsxcode/addons/a11y/src/components/A11YPanel.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/components/components/Modal/Modal.stories.tsxcode/addons/a11y/src/components/A11YPanel.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Modal/Modal.stories.tsx (5)
code/core/src/components/index.ts (2)
Modal(44-44)Button(53-53)code/core/src/components/components/Modal/Modal.tsx (1)
Modal(136-136)code/core/src/components/components/Button/Button.tsx (1)
Button(59-146)code/core/src/components/components/Popover/WithPopover.stories.tsx (2)
Base(38-43)AlwaysOpen(147-159)code/core/src/components/components/Button/Button.stories.tsx (1)
Base(23-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (1)
code/core/src/components/components/Modal/Modal.stories.tsx (1)
8-8: Broken preview import path (one directory short).The path climbs to code/.storybook/preview; it should target repo-root/.storybook/preview.
Apply this fix:
-import preview from '../../../../../.storybook/preview'; +import preview from '../../../../../../.storybook/preview';Optional: verify the target exists and align other story files:
#!/bin/bash # Find preview file(s) fd -H --glob ".storybook/preview.*" -t f -E "node_modules" # Inspect imports to preview across the repo to ensure consistency rg -nP -C2 "import\s+preview\s+from\s+['\"](\.\.\/)+\.storybook\/preview['\"]"Based on past review comments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/core/src/components/components/tooltip/WithTooltip.stories.tsx (3)
188-190: Remove unnecessary await before expect
expectisn’t async; keep the await only onfindByText.Apply this diff:
- play: async () => { - await expect(await screen.findByText('Lorem ipsum dolor sit')).toBeInTheDocument(); - }, + play: async () => { + expect(await screen.findByText('Lorem ipsum dolor sit')).toBeInTheDocument(); + },
200-203: Avoid awaiting a sync query
queryByTextis synchronous. Remove unnecessary awaits.Apply this diff:
- play: async () => { - await expect(await screen.queryByText('Lorem ipsum dolor sit')).not.toBeInTheDocument(); - }, + play: async () => { + expect(screen.queryByText('Lorem ipsum dolor sit')).not.toBeInTheDocument(); + },
37-43: tabIndex on button trigger is redundantButtons are focusable by default;
tabIndex={0}is unnecessary here.Apply this diff:
- children: <Trigger tabIndex={0}>Focus me!</Trigger>, + children: <Trigger>Focus me!</Trigger>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/components/components/Popover/Popover.tsx(1 hunks)code/core/src/components/components/shared/overlayHelpers.tsx(1 hunks)code/core/src/components/components/tooltip/WithTooltip.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/components/components/Popover/Popover.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/components/components/tooltip/WithTooltip.stories.tsxcode/core/src/components/components/shared/overlayHelpers.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/components/components/tooltip/WithTooltip.stories.tsxcode/core/src/components/components/shared/overlayHelpers.tsx
🧬 Code graph analysis (2)
code/core/src/components/components/tooltip/WithTooltip.stories.tsx (4)
code/core/src/components/components/tooltip/TooltipNote.tsx (1)
TooltipNote(23-25)code/core/src/components/components/tooltip/lazy-WithTooltip.tsx (1)
WithTooltip(8-12)code/core/src/components/components/shared/overlayHelpers.tsx (2)
Trigger(60-70)OverlayTriggerDecorator(77-88)code/core/src/components/components/Popover/Popover.tsx (1)
Popover(77-114)
code/core/src/components/components/shared/overlayHelpers.tsx (1)
code/core/src/components/index.ts (2)
PopperPlacement(63-63)convertToReactAriaPlacement(62-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (3)
code/core/src/components/components/shared/overlayHelpers.tsx (1)
24-43: Fix top/bottom start/end placement mapping.The fallback currently yields invalid values like "top start"/"bottom end", which React Aria treats as unknown placements. This breaks overlay positioning for those variants. Please map the top/bottom start/end cases explicitly and return
punchanged otherwise.if (p === 'right-start') { return 'right top'; } - return p.replace('-', ' ') as NonNullable<PositionProps['placement']>; + if (p === 'top-start') { + return 'top left'; + } + + if (p === 'top-end') { + return 'top right'; + } + + if (p === 'bottom-start') { + return 'bottom left'; + } + + if (p === 'bottom-end') { + return 'bottom right'; + } + + return p as NonNullable<PositionProps['placement']>; });code/core/src/components/components/tooltip/WithTooltip.stories.tsx (2)
45-97: Placements story coverage looks solidGood spread of placement variants; helpful for visual/a11y verification.
102-102: Don’t invoke React components; pass JSX elementCall-site should pass a JSX element, not invoke the component function.
Apply this diff:
- tooltip: SampleTooltipNote(), + tooltip: <SampleTooltipNote />,
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/core/src/components/components/Modal/Modal.stories.tsx (3)
28-36: Pass actions directly to onClick.Use
onClick={action('...')}instead of wrapping and invokingaction('...')()to avoid creating new functions on each render and to align with Storybook idioms.Apply the pattern below (example for the first block; repeat similarly for others):
- <Button ariaLabel={false} variant="solid" onClick={() => action('save')()}> + <Button ariaLabel={false} variant="solid" onClick={action('save')}> Save </Button> ... - <Button ariaLabel={false} variant="outline" onClick={() => action('cancel')()}> + <Button ariaLabel={false} variant="outline" onClick={action('cancel')}> Cancel </Button>Also applies to: 279-293, 558-566, 607-615, 657-665
437-441: Don’t wrap findBy in waitFor.*
findBy*already waits; wrapping it inwaitForis redundant and can mask failures. Useawait screen.findByRole(...)directly.- const closeButton = await waitFor( - () => screen.findByRole('button', { name: 'Close modal' }), - { timeout: 3000 } - ); + const closeButton = await screen.findByRole('button', { name: 'Close modal' });
430-433: Use findBy instead of immediate queryBy after opening.**Opening the modal is async; using
queryByTextimmediately can be flaky. PreferfindByTextto await presence.- await userEvent.keyboard('{Enter}'); - await expect(screen.queryByText('Sample Modal')).toBeInTheDocument(); + await userEvent.keyboard('{Enter}'); + await expect(await screen.findByText('Sample Modal')).toBeInTheDocument();- await userEvent.click(trigger); - await expect(screen.queryByText('Sample Modal')).toBeInTheDocument(); + await userEvent.click(trigger); + await expect(await screen.findByText('Sample Modal')).toBeInTheDocument();- await userEvent.click(trigger); - await expect(screen.queryByText('Sample Modal')).toBeInTheDocument(); + await userEvent.click(trigger); + await expect(await screen.findByText('Sample Modal')).toBeInTheDocument();Also applies to: 495-498, 510-513
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/Modal/Modal.stories.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/components/components/Modal/Modal.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/components/components/Modal/Modal.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Modal/Modal.stories.tsx (5)
code/core/src/components/components/Modal/Modal.tsx (1)
Modal(136-136)code/core/src/components/components/Button/Button.tsx (1)
Button(59-146)code/core/src/components/components/tooltip/WithTooltip.stories.tsx (2)
Base(30-35)AlwaysOpen(181-191)code/core/src/components/components/Button/Button.stories.tsx (1)
Base(23-25)code/core/src/test/testing-library.ts (1)
userEvent(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (4)
code/core/src/components/components/Modal/Modal.stories.tsx (4)
112-115: Good use of ariaLabel={false} for text-labeled trigger.Since the button has visible text, omitting an aria-label is correct; screen readers will use the text content.
372-406: Open-change callback test looks solid.The play function verifies both open and close transitions through the callback; querying the close control by accessible name is appropriate.
5-5: Confirm package import stability for actions.Ensure
storybook/actionsis the intended public/internal entry for this package version; otherwise switch to@storybook/addon-actionsto avoid future breakages.
8-8: Fix preview import path (duplicate from earlier review).The relative import likely resolves to
code/.storybook/preview(missing), not repo-root.storybook/preview. Adjust the path (e.g., climb one more level or use the correct extension/alias) so this compiles.Run to locate the actual preview file and compute the correct relative path:
#!/bin/bash # Find preview files in repo fd -H --glob ".storybook/preview.*" -t f -E "node_modules" # Optional: show relative path from this story's folder fd -H --glob ".storybook/preview.*" -t f -E "node_modules" \ | xargs -I{} realpath --relative-to="code/core/src/components/components/Modal" "{}"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
code/core/src/components/components/tooltip/ListItem.tsx (3)
168-186: Tighten roles, set type="button", and handle disabled links and LinkWrapper.
- Avoid redundant roles on native elements (button/a).
- Ensure buttons don’t submit forms unexpectedly.
- Don’t pass href to custom LinkWrapper; use to only.
- Make disabled links non-focusable and non-interactive.
Apply this diff:
-const getItemProps = memoize(100)(({ onClick, input, href, LinkWrapper }) => ({ - ...(onClick && { - as: 'button', - role: 'button', - onClick, - }), +const getItemProps = memoize(100)(({ onClick, input, href, LinkWrapper, disabled }) => ({ + // button-like + ...(onClick && !href && { + as: 'button', + type: 'button', + onClick, + }), ...(input && { as: 'label', }), - ...(href && { - as: 'a', - role: 'link', - href, - ...(LinkWrapper && { - as: LinkWrapper, - to: href, - }), - }), + // link-like + ...(href && !disabled && !LinkWrapper && { + as: 'a', + href, + }), + ...(href && !disabled && LinkWrapper && { + as: LinkWrapper, + to: href, + role: 'link', // only needed for non-native link wrappers + }), + // disabled link fallback (non-focusable, non-interactive) + ...(href && disabled && { + as: 'span', + 'aria-disabled': true, + tabIndex: -1, + }), }));
224-227: Don’t log deprecation on every render.Wrap in useEffect so it runs once per mount (or move to module scope).
Apply these diffs:
- deprecate( - '`ListItem` is deprecated and will be removed in Storybook 11, use `MenuItem` instead.' - ); + useEffect(() => { + deprecate( + '`ListItem` is deprecated and will be removed in Storybook 11, use `MenuItem` instead.' + ); + }, []);Also add useEffect to imports:
-import React, { type ComponentProps, type ReactNode, type SyntheticEvent, forwardRef } from 'react'; +import React, { type ComponentProps, type ReactNode, type SyntheticEvent, forwardRef, useEffect } from 'react';If client-logger.deprecate already dedupes per message internally, this is optional; please confirm.
204-206: Type the forwarded ref for better DX.Specify a concrete ref element type to avoid Ref for consumers.
Apply this diff:
-const ListItem = forwardRef((props: ListItemProps, ref) => { +const ListItem = forwardRef<HTMLElement, ListItemProps>((props, ref) => {code/core/src/components/components/Modal/Modal.stories.tsx (5)
224-227: Prefer explicit ModalProps over ComponentProps for clarity.If
ModalPropsis exported, use it forModalWithTriggerto avoid fragile inference fromtypeof Modal(which is augmented viaObject.assign).-}: { triggerText: string } & React.ComponentProps<typeof Modal>) => { +}: { triggerText: string } & ModalProps) => {If
ModalPropsisn’t exported, ignore.
395-404: Remove unnecessary awaits on synchronous expectations.
toHaveBeenCalledWithis synchronous; awaiting it is unnecessary.- await userEvent.click(trigger); - await expect(args.onOpenChange).toHaveBeenCalledWith(true); + await userEvent.click(trigger); + expect(args.onOpenChange).toHaveBeenCalledWith(true); @@ - await userEvent.click(closeButton); - await expect(args.onOpenChange).toHaveBeenCalledWith(false); + await userEvent.click(closeButton); + expect(args.onOpenChange).toHaveBeenCalledWith(false);
435-438: Avoid wrapping findBy in waitFor; use findBy with timeout instead.**
findBy*already waits. Wrapping it inwaitForis redundant and harder to read.- const closeButton = await waitFor( - () => screen.findByRole('button', { name: 'Close modal' }), - { timeout: 3000 } - ); + const closeButton = await screen.findByRole('button', { name: 'Close modal' });- const closeButton = await waitFor(() => screen.findByLabelText('Close modal'), { - timeout: 3000, - }); + const closeButton = await screen.findByLabelText('Close modal');Also applies to: 499-501
427-466: Harden keyboard navigation assertions to reduce flakiness.The initial focus target after opening can vary by implementation. Instead of assuming a first Tab lands on Close, assert the set of focusables and cycle deterministically, or first assert which element gets initial focus and tab relative to it.
Briefly:
- Capture
document.activeElementright after open.- Derive next expected focusable with your trap order and tab accordingly.
- Keep the cycle assertions; avoid assumptions about the first position.
350-370: Non-dismissible modal is OK for demo; consider an escape hatch in docs.
open: truewith both dismissals disabled and no close button can trap users in the canvas. Consider a note or hidden emergency close (e.g., press Shift+Escape) for manual testing sessions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/Modal/Modal.stories.tsx(1 hunks)code/core/src/components/components/tooltip/ListItem.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/components/components/tooltip/ListItem.tsxcode/core/src/components/components/Modal/Modal.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/components/components/tooltip/ListItem.tsxcode/core/src/components/components/Modal/Modal.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Modal/Modal.stories.tsx (5)
code/core/src/components/index.ts (2)
Modal(44-44)Button(53-53)code/core/src/components/components/Modal/Modal.tsx (1)
Modal(136-136)code/core/src/components/components/Button/Button.tsx (1)
Button(59-146)code/core/src/components/components/tooltip/WithTooltip.stories.tsx (2)
Base(30-35)AlwaysOpen(181-191)code/core/src/test/testing-library.ts (1)
userEvent(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (8)
code/core/src/components/components/tooltip/ListItem.tsx (4)
141-145: A11y: Nice use of :focus-visible.Good to see a clear, high-contrast focus outline for keyboard users.
61-70: Flex overflow fix is correct.min-width: 0 on flex children prevents unintended overflow. Good catch.
Also applies to: 119-123
156-165: Verify :has() selector support or add a light fallback.:has() still lacks support in some environments. Confirm it matches our supported browser matrix. If needed, consider a non-:has fallback for pointer/background on label items.
Would you like me to draft a fallback that preserves hover/intent without :has()?
229-229: Ref forwarding looks good.The ref is correctly forwarded to the underlying element.
code/core/src/components/components/Modal/Modal.stories.tsx (4)
8-8: Fix preview import path or switch to module alias.This still looks wrong. From this file,
../../../../../.storybook/previewresolves tocode/.storybook/preview, which likely doesn’t exist. Please point to the actual preview file (probably one more level up) or use the repo alias if available (e.g.,storybook/preview).Example fixes:
- Relative (if preview is at repo root):
../../../../../../.storybook/preview- Alias:
storybook/previewProposed change (alias-based, safer in monorepo):
-import preview from '../../../../../.storybook/preview'; +import preview from 'storybook/preview';Run to locate preview config(s) and validate the correct import:
#!/bin/bash # Find preview files fd -H -t f -E node_modules --glob ".storybook/preview.*" # Show candidate relative paths from this story's folder fd -H -t f --glob ".storybook/preview.*" | xargs -I{} realpath --relative-to="code/core/src/components/components/Modal" "{}"
11-39: A11y: Button ariaLabel usage is appropriate.All buttons with visible text set
ariaLabel={false}, avoiding redundant labels while keeping accessible names via text. Good.Please confirm that icon-only buttons (if added later) always provide a non-empty ariaLabel per Button’s runtime check in code/core/src/components/components/Button/Button.tsx.
Also applies to: 239-302, 524-576, 578-675
41-44: Unique story id confirmed. No duplicates ofoverlay-Modalfound across the repo.
45-83: ArgTypes match ModalProps – confirmed dismissOnClickOutside, dismissOnEscape, width, height, open, defaultOpen, onOpenChange and ariaLabel controls directly correspond to the props defined in Modal.tsx.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/core/src/components/components/Modal/Modal.stories.tsx (3)
241-244: Remove redundant args.children where stories provide explicit children.These stories spread args (which include children) and also pass explicit children, making args.children dead/unreachable and potentially confusing.
Apply this diff:
export const StyledComponents = meta.story({ args: { - width: 600, - children: <SampleModalContent />, + width: 600, },export const WithError = meta.story({ args: { - width: 500, - children: <SampleModalContent />, + width: 500, },export const AlwaysOpen = meta.story({ args: { open: true, dismissOnClickOutside: false, dismissOnEscape: false, - children: <SampleModalContent />, },export const LongContent = meta.story({ args: { height: 400, ariaLabel: 'Long content modal', - children: <SampleModalContent />, },export const DialogTransitions = meta.story({ args: { variant: 'dialog', ariaLabel: 'Dialog with transitions', - children: <SampleModalContent />, },export const BottomDrawerTransitions = meta.story({ args: { variant: 'bottom-drawer', ariaLabel: 'Bottom drawer with transitions', - children: <SampleModalContent />, },Also applies to: 306-309, 351-356, 547-551, 601-605, 650-654
27-36: Use action('...') directly instead of action('...')().Passing the action creator directly preserves the click event and simplifies handlers.
Apply this diff:
<Modal.Close asChild> - <Button ariaLabel={false} variant="solid" onClick={() => action('save')()}> + <Button ariaLabel={false} variant="solid" onClick={action('save')}> Save </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('cancel')()}> + <Button ariaLabel={false} variant="outline" onClick={action('cancel')}> Cancel </Button> </Modal.Close><Modal.Actions> <Modal.Close asChild> - <Button ariaLabel={false} variant="solid" onClick={() => action('primary')()}> + <Button ariaLabel={false} variant="solid" onClick={action('primary')}> Primary Action </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('secondary')()}> + <Button ariaLabel={false} variant="outline" onClick={action('secondary')}> Secondary Action </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('cancel')()}> + <Button ariaLabel={false} variant="outline" onClick={action('cancel')}> Cancel </Button> </Modal.Close> </Modal.Actions><Modal.Actions> <Modal.Close asChild> - <Button ariaLabel={false} variant="solid" onClick={() => action('save')()}> + <Button ariaLabel={false} variant="solid" onClick={action('save')}> Save </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('cancel')()}> + <Button ariaLabel={false} variant="outline" onClick={action('cancel')}> Cancel </Button> </Modal.Close> </Modal.Actions><Modal.Actions> <Modal.Close asChild> - <Button ariaLabel={false} variant="solid" onClick={() => action('understood')()}> + <Button ariaLabel={false} variant="solid" onClick={action('understood')}> Got it </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('close')()}> + <Button ariaLabel={false} variant="outline" onClick={action('close')}> Close </Button> </Modal.Close> </Modal.Actions><Modal.Actions> <Modal.Close asChild> - <Button ariaLabel={false} variant="solid" onClick={() => action('understood')()}> + <Button ariaLabel={false} variant="solid" onClick={action('understood')}> Got it </Button> </Modal.Close> <Modal.Close asChild> - <Button ariaLabel={false} variant="outline" onClick={() => action('close')()}> + <Button ariaLabel={false} variant="outline" onClick={action('close')}> Close </Button> </Modal.Close> </Modal.Actions>Also applies to: 278-293, 579-589, 628-637, 678-687
410-413: Simplify find queries in play functions.Use findBy* directly instead of wrapping findBy* in waitFor.
Apply this diff:
- const closeButton = await waitFor(() => screen.findByLabelText('Close modal'), { - timeout: 3000, - }); + const closeButton = await screen.findByLabelText('Close modal', { timeout: 3000 });- const closeButton = await waitFor( - () => screen.findByRole('button', { name: 'Close modal' }), - { timeout: 3000 } - ); + const closeButton = await screen.findByRole('button', { name: 'Close modal' });- const closeButton = await waitFor(() => screen.findByLabelText('Close modal'), { - timeout: 3000, - }); + const closeButton = await screen.findByLabelText('Close modal', { timeout: 3000 });Also applies to: 452-457, 521-525
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/Modal/Modal.stories.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/components/components/Modal/Modal.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/components/components/Modal/Modal.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Modal/Modal.stories.tsx (5)
code/core/src/components/components/Modal/Modal.tsx (1)
Modal(136-136)code/core/src/components/components/Button/Button.tsx (1)
Button(59-146)code/core/src/components/components/tooltip/WithTooltip.stories.tsx (2)
Base(30-35)AlwaysOpen(181-191)code/core/src/components/components/Button/Button.stories.tsx (1)
Base(23-25)code/core/src/test/testing-library.ts (1)
userEvent(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (1)
code/core/src/components/components/Modal/Modal.stories.tsx (1)
8-8: Fix incorrect preview import path (climb 6 levels).The current path climbs to code/.storybook/preview, not repo-root/.storybook/preview. Bump one more ../ to avoid a module resolution error in stories.
Apply this diff:
-import preview from '../../../../../.storybook/preview'; +import preview from '../../../../../../.storybook/preview';Run to confirm the correct preview location and the relative path from this file:
#!/bin/bash # Find preview files anywhere in the repo fd -H --glob ".storybook/preview.*" -t f # Show relative path(s) from this story file fd -H --glob ".storybook/preview.*" -t f | xargs -I{} realpath --relative-to="code/core/src/components/components/Modal" "{}"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
code/core/src/component-testing/components/InteractionsPanel.stories.tsx (8)
13-13: Avoid importing from another .stories file to share argsCross-importing stories couples stories together, risks circular deps, and shares mock instances. Prefer moving shared args/mocks to a non-story helper or define fresh mocks locally.
Can you confirm
./Toolbar.storiesdoes not import from this file or useInteractionsPanel, to avoid cycles?
57-58: Don’t reuse mock functions from another story; create fresh mocks per storyReferencing
ToolbarStories.args.controlsshares mock instances across stories and can cause flaky expectations. Clone states and build new mocks locally.Apply this diff:
- controls: ToolbarStories.args.controls, - controlStates: ToolbarStories.args.controlStates, + controls: createControls(), + controlStates: { ...ToolbarStories.args.controlStates },Add this helper (outside the selected lines, e.g., above
const meta = ...):function createControls() { return { start: fn().mockName('controls.start'), back: fn().mockName('controls.back'), goto: fn().mockName('controls.goto'), next: fn().mockName('controls.next'), end: fn().mockName('controls.end'), rerun: fn().mockName('controls.rerun'), }; }
78-83: Good move to step-based play; also clear shared mocks upfrontUsing
stepimproves traceability. Clear any shared mock state at the start to avoid bleed from imported args or prior runs.Apply this diff:
const canvas = within(canvasElement); + // Reset mocks in case they're shared across stories/imports + Object.values(args.controls ?? {}).forEach((m: any) => m?.mockClear?.());
84-89: Use findBy and await the assertion to avoid timing flakiness*Prefer
findByLabelTextoverwaitFor(() => getBy...), and wrap the expectation inwaitForto handle async event propagation.Apply this diff:
- await step('Go to start', async () => { - const btn = await waitFor(() => canvas.getByLabelText('Go to start')); - await userEvent.click(btn); - await expect(args.controls.start).toHaveBeenCalled(); - }); + await step('Go to start', async () => { + const btn = await canvas.findByLabelText('Go to start'); + await userEvent.click(btn); + await waitFor(() => expect(args.controls.start).toHaveBeenCalled()); + });
90-95: Same here: prefer findBy and await the assertion*This improves stability and readability.
Apply this diff:
- await step('Go back', async () => { - const btn = await waitFor(() => canvas.getByLabelText('Go back')); - await userEvent.click(btn); - await expect(args.controls.back).toHaveBeenCalled(); - }); + await step('Go back', async () => { + const btn = await canvas.findByLabelText('Go back'); + await userEvent.click(btn); + await waitFor(() => expect(args.controls.back).toHaveBeenCalled()); + });
96-101: If “Next” is intentionally disabled, assert disabled and avoid clickingClicking disabled controls can throw with userEvent depending on config. Assert the disabled state explicitly and skip the click.
Apply this diff:
- await step('Go forward', async () => { - const btn = await waitFor(() => canvas.getByLabelText('Go forward')); - await userEvent.click(btn); - await expect(args.controls.next).not.toHaveBeenCalled(); - }); + await step('Go forward', async () => { + const btn = await canvas.findByLabelText('Go forward'); + expect(btn).toBeDisabled(); + await expect(args.controls.next).not.toHaveBeenCalled(); + });If “Next” should be enabled in this scenario, flip to
toHaveBeenCalled()and keep the click (with awaited assertion).
102-107: Mirror the disabled check for “End” and avoid clickingSame rationale as for “Next”.
Apply this diff:
- await step('Go to end', async () => { - const btn = await waitFor(() => canvas.getByLabelText('Go to end')); - await userEvent.click(btn); - await expect(args.controls.end).not.toHaveBeenCalled(); - }); + await step('Go to end', async () => { + const btn = await canvas.findByLabelText('Go to end'); + expect(btn).toBeDisabled(); + await expect(args.controls.end).not.toHaveBeenCalled(); + });If “End” should be enabled here, flip to
toHaveBeenCalled()and keep the click (with awaited assertion).
108-113: Use findBy and await the assertion for “Rerun”*Align with the other steps for consistency and robustness.
Apply this diff:
- await step('Rerun', async () => { - const btn = await waitFor(() => canvas.getByLabelText('Rerun')); - await userEvent.click(btn); - await expect(args.controls.rerun).toHaveBeenCalled(); - }); + await step('Rerun', async () => { + const btn = await canvas.findByLabelText('Rerun'); + await userEvent.click(btn); + await waitFor(() => expect(args.controls.rerun).toHaveBeenCalled()); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/component-testing/components/InteractionsPanel.stories.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/component-testing/components/InteractionsPanel.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/component-testing/components/InteractionsPanel.stories.tsx
🧬 Code graph analysis (1)
code/core/src/component-testing/components/InteractionsPanel.stories.tsx (1)
code/core/src/test/testing-library.ts (1)
userEvent(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/addons/a11y/src/components/A11YPanel.stories.tsx (1)
4-4: Remove unusedwaitForimport (after the fix).Prevents lint failures and keeps imports tidy. As per coding guidelines.
-import { expect, fn, userEvent, waitFor, within } from 'storybook/test'; +import { expect, fn, userEvent, within } from 'storybook/test';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/a11y/src/components/A11YPanel.stories.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/addons/a11y/src/components/A11YPanel.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/addons/a11y/src/components/A11YPanel.stories.tsx
🧬 Code graph analysis (1)
code/addons/a11y/src/components/A11YPanel.stories.tsx (1)
code/core/src/test/testing-library.ts (1)
userEvent(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
| const btn = await waitFor(() => | ||
| canvas.findByRole('button', { name: /Rerun accessibility scan/ }) | ||
| ); | ||
| await userEvent.click(btn); |
There was a problem hiding this comment.
Fix waitFor misuse: it returns Promise, so btn becomes undefined.
waitFor resolves to void; assign the element using findByRole directly, then click it.
- const btn = await waitFor(() =>
- canvas.findByRole('button', { name: /Rerun accessibility scan/ })
- );
+ const btn = await canvas.findByRole('button', { name: /Rerun accessibility scan/i });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const btn = await waitFor(() => | |
| canvas.findByRole('button', { name: /Rerun accessibility scan/ }) | |
| ); | |
| await userEvent.click(btn); | |
| // replace waitFor+findByRole with findByRole directly | |
| const btn = await canvas.findByRole('button', { name: /Rerun accessibility scan/i }); | |
| await userEvent.click(btn); |
🤖 Prompt for AI Agents
In code/addons/a11y/src/components/A11YPanel.stories.tsx around lines 178 to
181, the test misuses waitFor which returns void so btn becomes undefined;
replace the waitFor wrapper by directly awaiting the element: assign btn using
await canvas.findByRole('button', { name: /Rerun accessibility scan/ }) (or
await within(canvas).findByRole(...) if using within), then call await
userEvent.click(btn); remove the waitFor call and ensure any necessary
testing-library helpers are correctly imported.
The code on this branch is already reviewed in past PRs. The
a11y-consolidationbranch serves as a checkpoint to perform manual tests and limit the day-to-day impact of the a11y project on other project cycles.Feel free to edit this message with checks to perfom / fixes to write.
To do
Sanity checks
Selectonce theAriaToolbarPR is merged too (does it exit the entire toolbar or still go to the sibling)Selectstill announces current value in NVDAWithTooltipto react-aria's overlay stack solves the double tooltip issue onSelecton some Chromatic testsPost-integration tasks
Regressions
Severe issues
code/core/src/use-sync-external-store/shim is still neededSmall cleanups
These are bonus for UI/usability, not regressions or issues.
Summary by CodeRabbit