UI: Improve landmark navigation#33457
Conversation
|
View your CI Pipeline Execution ↗ for commit 276e724
☁️ Nx Cloud last updated this comment at |
Sidnioulz
force-pushed
the
sidnioulz/issue-32271
branch
2 times, most recently
from
aaf875d to
e37e586
Compare
Sidnioulz
added
feature request
accessibility
ci:normal
a11y: keyboard
📝 WalkthroughWalkthroughAdds landmark-based ARIA support and keyboard landmark navigation. Adds dependency Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Listener as useLandmarkIndicator
participant DOM as Document/DOM
participant Theme as Theme System
participant AnimAPI as Web Animations API
User->>Listener: Presses F6 / Shift+F6
Listener->>DOM: Capture keydown, get activeElement
Listener->>DOM: Traverse ancestors to find [data-sb-landmark]
alt landmark found
Listener->>AnimAPI: Cancel existing animation on landmark (if any)
Listener->>Theme: Read theme.color.primary
Theme-->>Listener: Return primary color
Listener->>AnimAPI: Start ::after border highlight (2px, 1.5s)
AnimAPI->>DOM: Render highlight on landmark ::after
AnimAPI->>Listener: On finish, clear animation state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/sidebar/Explorer.tsx:
- Line 27: Update the ref type for containerRef to match the rendered element:
change useRef<HTMLDivElement>(null) to useRef<HTMLElement | null>(null)
(referencing the containerRef variable in Explorer.tsx) so the ref can point to
the <nav> element and satisfy type safety wherever containerRef.current is used.
In @code/core/src/manager/components/sidebar/TestingWidget.tsx:
- Around line 257-264: The hooks cardRef = useRef<HTMLDivElement>(null) and
const { landmarkProps } = useLandmark(...) are being invoked after an early
return, violating the Rules of Hooks; move those hook calls (cardRef and
useLandmark invocation that provides landmarkProps) to execute unconditionally
before any early return or conditional logic in the TestingWidget component so
they run in the same order on every render, then use landmarkProps and cardRef
where needed after the early-return check.
🧹 Nitpick comments (5)
code/core/src/manager/components/sidebar/Heading.stories.tsx (1)
237-240: Text update correctly reflects the new skip link label.The change from "Skip to canvas" to "Skip to content" aligns with the PR's accessibility improvements. The play function will correctly focus the updated skip link elements for Chromatic testing.
Optional: Consider renaming the story export for consistency.
The story is named
SkipToCanvasLinkFocused(line 229) but now targets "Skip to content" text. For clarity, you might rename it toSkipToContentLinkFocused, though story names primarily serve as identifiers and the current name is still functional.code/core/src/components/components/Card/Card.tsx (1)
100-107: Consider placing ref after outlineProps spread.While
DOMAttributesdoesn't includeref(TypeScript prevents passing it), React convention placesreflast to prevent accidental override and provide defense in depth.♻️ Suggested prop order
- <CardOutline animation={outlineAnimation} color={outlineColor} ref={ref} {...outlineProps}> + <CardOutline animation={outlineAnimation} color={outlineColor} {...outlineProps} ref={ref}>code/core/src/manager/components/preview/FramesRenderer.tsx (1)
99-103: Removing the anchortitlehere is fine; consider dropping redundanttabIndex={0}.Since the
<a>has anhref, it’s already tabbable; keepingtabIndex={0}is harmless but unnecessary.code/core/src/manager/components/layout/useLandmarkIndicator.ts (1)
37-43: Consider browser compatibility for the Animation API.The Web Animations API is well-supported in modern browsers but may fail in older environments. Consider adding a try-catch or feature detection for maximum robustness.
♻️ Optional defensive coding enhancement
- const animation = landmarkElement.animate( - [{ border: `2px solid ${theme.color.primary}` }, { border: `2px solid transparent` }], - { - duration: 1500, - pseudoElement: '::after', - } - ); + try { + const animation = landmarkElement.animate( + [{ border: `2px solid ${theme.color.primary}` }, { border: `2px solid transparent` }], + { + duration: 1500, + pseudoElement: '::after', + } + ); - currentAnimationRef.current = animation; + currentAnimationRef.current = animation; - animation.onfinish = () => { - currentAnimationRef.current = null; - }; + animation.onfinish = () => { + currentAnimationRef.current = null; + }; + } catch (e) { + // Animation API not supported or failed, silently continue + }code/core/src/manager/components/sidebar/Search.tsx (1)
316-322: Consider adding an explicit label to the search landmark.The search landmark lacks an
aria-labeloraria-labelledbyattribute. While the input field has an associated label, the landmark region itself would benefit from explicit labeling for better accessibility.💡 Suggested enhancement
const searchLandmarkRef = useRef<HTMLDivElement>(null); const { landmarkProps } = useLandmark( { + 'aria-label': 'Search components', role: 'search', }, searchLandmarkRef );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
code/core/package.jsoncode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/hooks/useLandmark.tscode/core/src/theming/global.ts
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/Sidebar.stories.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/package.jsoncode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/theming/global.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/package.jsoncode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/theming/global.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/theming/global.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/theming/global.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/theming/global.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/hooks/useLandmark.tscode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/package.jsoncode/core/src/theming/global.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/theming/global.ts
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/panel/Panel.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/panel/Panel.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/Search.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/package.json
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/core/src/manager/components/sidebar/Tree.stories.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/node-logger` for server-side logging in Node.js code
Applied to files:
code/core/src/manager/components/sidebar/Tree.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/core/src/manager/components/sidebar/Tree.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/manager/components/sidebar/Tree.stories.tsx
🧬 Code graph analysis (13)
code/core/src/manager/components/sidebar/TestingWidget.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/sidebar/Explorer.tsx (1)
code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/components/components/Card/Card.tsx (1)
code/core/src/components/index.ts (1)
Card(46-46)
code/core/src/manager/components/sidebar/Sidebar.tsx (4)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)code/core/src/manager/components/sidebar/Explorer.tsx (1)
Explorer(19-72)code/core/src/manager/components/sidebar/SearchResults.tsx (1)
SearchResults(224-345)
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/sidebar/Tree.stories.tsx (1)
code/core/src/manager-api/root.tsx (1)
ManagerContext(79-79)
code/core/src/manager/components/preview/Toolbar.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/sidebar/Search.tsx (1)
code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/preview/Preview.tsx (1)
code/core/src/preview-api/index.ts (1)
useRef(13-13)
code/core/src/manager/components/panel/Panel.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/layout/useLandmarkIndicator.ts (2)
code/core/src/theming/index.ts (1)
useTheme(18-18)code/core/src/preview-api/index.ts (2)
useRef(13-13)useEffect(8-8)
code/core/src/manager/components/layout/Layout.tsx (1)
code/core/src/manager/components/layout/useLandmarkIndicator.ts (1)
useLandmarkIndicator(9-59)
code/core/src/theming/global.ts (1)
code/core/src/theming/base.ts (1)
color(1-42)
🪛 Biome (2.1.2)
code/core/src/manager/components/sidebar/TestingWidget.tsx
[error] 257-257: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 258-258: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (4)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: nx
- GitHub Check: nx
🔇 Additional comments (28)
code/core/src/components/components/Card/Card.tsx (2)
1-1: LGTM: Import addition is appropriate.The
DOMAttributesimport is necessary for typing the newoutlinePropsand is correctly imported from React.
95-95: LGTM: Type definition is appropriate.The
outlinePropstype correctly usesDOMAttributes<HTMLDivElement>to accept standard DOM attributes, ARIA attributes, and event handlers for the outline element.code/core/src/manager/components/sidebar/Heading.tsx (1)
90-96: Skip link text change looks good and consistent with the new landmark framing.
ariaLabel={false}is the right choice here since the anchor text provides the accessible name (as per learnings).code/core/src/manager/components/sidebar/Tree.stories.tsx (2)
15-17: Story decorator update to includeIconSymbolsmatches the component-level change.This keeps the Tree stories self-contained now that
Treeno longer injects the SVG symbols.Also applies to: 79-85
277-285: Updated play assertion for “Skip to content” matches the new label.code/core/src/manager/components/sidebar/Tree.tsx (2)
304-306: “Skip to content” label updates are consistent across node types.Also: using
ariaLabel={false}is correct since the link text is the accessible name (as per learnings).Also applies to: 445-447, 503-505
753-755: No action needed —IconSymbolsis properly mounted in the parentSidebarcomponent.The verification confirms that
<IconSymbols />is rendered at line 156 ofSidebar.tsx, which is the parent component ofTree. This global mount ensures that symbol-based icons (via<UseSymbol />) remain functional across all child components, including those that depend on it likeTreeNode,ContextMenu, andSearchResults. The refactoring correctly moved the mount to the appropriate parent level.code/core/package.json (1)
228-235: No action needed.@react-aria/landmarkis correctly placed indevDependenciesbecause it is bundled into the manager output, following Storybook's established pattern for bundled UI component libraries (similar toopenandfast-printf). All@react-aria/*packages are grouped indevDependenciesand are not listed in the esbuild global externals configuration, confirming they are bundled rather than externalized.code/core/src/theming/global.ts (1)
137-155: LGTM! Landmark focus styling implementation is sound.The landmark focus indication using
::afterpseudo-element is well-executed:
- Avoids layout shift by using an overlay approach
- Properly isolates pointer events
- Uses theme primary color for brand consistency
Per the PR objectives, ensure this styling has been tested in high-contrast mode to verify the focus outline remains visible.
code/core/src/manager/components/sidebar/types.ts (1)
54-66: LGTM! Improved state granularity for accessibility.The replacement of
isBrowsingwith three distinct flags provides clearer semantics and enables better accessibility control. The inline documentation effectively explains each flag's purpose and the distinction between visibility and DOM presence is crucial for maintaining keyboard navigation (per PR objectives).code/core/src/manager/components/layout/Layout.tsx (1)
162-163: LGTM! Landmark indicator integration is correctly placed.The
useLandmarkIndicator()hook is appropriately called in the rootLayoutcomponent, which serves as the parent container for all landmarks. The hook's implementation (per code snippets) properly manages the F6 keydown listener lifecycle with cleanup, and the animation mechanism prevents concurrent highlights.The PR acknowledges the known limitation where the preview iframe can intercept F6 events, preventing navigation from within rendered stories—an acceptable trade-off given iframe constraints.
code/core/src/manager/components/preview/utils/components.ts (1)
5-5: LGTM! Landmark element restructuring is semantically correct.Moving the
mainlandmark fromPreviewContainertoFrameWrapproperly aligns semantic roles with actual content:
PreviewContainerasdiv: appropriate for layout wrapperFrameWrapasmain: correct for the primary preview content regionThis change aligns with the PR objective of marking Preview as the main landmark. The
useLandmarkhook properly integrates the landmark with React Aria, registering the region with appropriate ARIA attributes (aria-labelledbyandrole='main').Verification confirms only one
<main>landmark exists in the manager components (the FrameWrap in Preview), with no conflicting landmarks across the codebase.code/core/src/manager/hooks/useLandmark.ts (1)
1-22: LGTM! Clean wrapper implementation.The hook properly wraps React Aria's
useLandmarkand augments it with the Storybook-specificdata-sb-landmarkattribute that enables the focus styling defined inglobal.ts. The implementation is minimal, type-safe, and actively used across eight manager components (Preview, Toolbar, Panel, Sidebar, Search, Explorer, TestingWidget, and MobileNavigation) with consistent patterns.code/core/src/manager/components/preview/Preview.tsx (2)
115-119: LGTM! Landmark integration correctly implemented.The landmark hook is properly configured with role='main' and references the visually-hidden heading, aligning with the PR objectives to mark the Preview as the main landmark region.
138-138: LGTM! Landmark props correctly applied.The ref and landmarkProps are properly spread onto the FrameWrap element, enabling both landmark navigation and visual highlighting.
code/core/src/manager/components/preview/Toolbar.tsx (2)
87-94: LGTM! Landmark integration correctly implemented.The landmark hook properly configures the toolbar as a region with appropriate labeling via the visually-hidden heading.
101-102: LGTM! Landmark props correctly applied.The ref and landmarkProps are properly spread onto the StyledSection, enabling landmark navigation and visual highlighting.
code/core/src/manager/components/panel/Panel.tsx (2)
156-160: Verify ARIA role matches semantic intent.The
<aside>element has an implicit ARIA role ofcomplementary, but the code explicitly setsrole='region', overriding this. The PR objectives specify marking the "Addon panel as aside", suggesting the complementary role should be used.Consider using
role='complementary'to align with the semantic<aside>element, or omit the role attribute entirely to use the implicit role:♻️ Proposed fix
const { landmarkProps } = useLandmark( - { 'aria-labelledby': 'storybook-panel-heading', role: 'region' }, + { 'aria-labelledby': 'storybook-panel-heading', role: 'complementary' }, asideRef );Alternatively, if the React Aria landmark hook properly handles implicit roles:
const { landmarkProps } = useLandmark( - { 'aria-labelledby': 'storybook-panel-heading', role: 'region' }, + { 'aria-labelledby': 'storybook-panel-heading' }, asideRef );
163-163: LGTM! Landmark props correctly applied.The ref and landmarkProps are properly spread onto the Aside element, assuming the role mismatch is addressed.
code/core/src/manager/components/layout/useLandmarkIndicator.ts (1)
9-58: LGTM! Landmark indicator implementation is sound.The F6 keydown handler correctly traverses the DOM to locate landmark regions and applies a visual border animation. The cleanup and dependency on
theme.color.primaryare appropriate.Note: The PR objectives mention a known limitation where the preview iframe intercepts F6 events, preventing landmark navigation from within the iframe. This is an expected constraint.
code/core/src/manager/components/sidebar/TestingWidget.tsx (1)
274-279: LGTM! Landmark props correctly applied.The ref and
outlinePropswith landmarkProps are properly applied to the HoverCard, and the visually-hidden heading provides appropriate labeling. This will work correctly once the hook placement issue is resolved.code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (2)
81-88: LGTM: Landmark integration correctly implemented.The banner landmark is properly configured with
aria-labelledbyreferencing the "Navigation controls" heading. The ref type matches the header element.
145-174: LGTM: Semantic element updates align with landmark strategy.Changing
Containertostyled.sectionandMobileBottomBartostyled.headerprovides the appropriate semantic structure for the banner landmark.code/core/src/manager/components/sidebar/Explorer.tsx (1)
37-56: LGTM: Navigation landmark properly configured.The landmark is correctly labeled via the "Stories" heading, and the semantic
navelement withrole="navigation"provides appropriate structure for assistive technologies.code/core/src/manager/components/sidebar/Search.tsx (1)
445-447: LGTM: Render props API updated to support landmark-aware navigation.The new
isNavVisible,isNavRendered, andisSearchResultRenderedflags provide clearer semantics for controlling visibility and rendering of navigation elements, aligning with the broader landmark navigation strategy.code/core/src/manager/components/sidebar/Sidebar.tsx (3)
142-156: LGTM: Banner landmark and document structure correctly implemented.The addition of a visually-hidden
h1for "Storybook" and the banner landmark on the header element provides proper document structure and navigation semantics. TheIconSymbolscomponent placement is appropriate.
213-222: Verify screen reader behavior with conditionally hidden Explorer.When
isNavVisibleis false, the Explorer remains in the DOM withsb-sr-onlyclass applied. Confirm that this approach doesn't cause screen readers to announce hidden navigation content while users are interacting with search results.This pattern is likely intentional to preserve keyboard navigation state, but validation with actual screen reader testing would be beneficial to ensure it doesn't create confusion for AT users.
31-47: LGTM: Semantic container updated to header for banner landmark.The change from
styled.divtostyled.headerprovides the appropriate semantic foundation for the banner landmark while preserving all styling.
Sidnioulz
added
patch:yes
| const animation = landmarkElement.animate( | ||
| [{ border: `2px solid ${theme.color.primary}` }, { border: `2px solid transparent` }], | ||
| { | ||
| duration: 1500, |
There was a problem hiding this comment.
We use 200ms in most other places, so 1500ms seems like a lot.
There was a problem hiding this comment.
This animation doesn't fulfill the same role, as we want to leave enough time for the region to be noticed by users.
See how it looks with 200ms:
Peek.2026-01-12.13-23.webm
| {isNavRendered && ( | ||
| <Explorer | ||
| className={isNavVisible ? undefined : 'sb-sr-only'} | ||
| dataset={dataset} | ||
| selected={selected} | ||
| isLoading={isLoading} | ||
| isBrowsing={isNavVisible} | ||
| hasEntries={hasEntries} | ||
| /> | ||
| )} | ||
| {isSearchResultRendered && ( | ||
| <SearchResults | ||
| query={query} | ||
| results={results} | ||
| closeMenu={closeMenu} | ||
| getMenuProps={getMenuProps} | ||
| getItemProps={getItemProps} | ||
| highlightedIndex={highlightedIndex} | ||
| enableShortcuts={enableShortcuts} | ||
| isLoading={isLoading} | ||
| clearLastViewed={lastViewedProps.clearLastViewed} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
What I don't like about this is that it suggests that both Explorer and SearchResults might be rendered at the same time. The Swap component was there to make it very explicit that that would never be the case. An if/else ternary on a single condition seems more appropriate.
Also, I worry that by unmounting rather than hiding, these components will lose important internal state when switching between explorer and search. Have you checked that?
There was a problem hiding this comment.
The Swap component forced both components to always render, but be unreachable. This leads to landmark elements being findable in the DOM, but not focusable.
Depending on the landmark navigation algorithm in use (react-aria, NVDA, VO, JAWS, Narrator, etc), this can lead to bugs where navigating to the next landmark does nothing (it finds the landmark, but fails to focus it). So we need to distinguish states where nav is kb-reachable (we are in the input but there's no query) and states where nav is entirely hidden (we have a query and results).
Note that the "recently opened" view and the "search results" view have never been kb-reachable. This does not change here as it's a Tree 2.0 issue to be solved.
You were right about loss of state in the tree though, so I've reworked the code to keep rendering the Explorer (still with a distinction between visually hidden and not reachable).
The SearchResults view is necessarily stateless, as it relies on the query, and it can only change/disappear when the query changes or disappears. I prefer to render it only when necessary if that's ok with you.
--
@MichaelArestad we should probably jam on the UX surrounding "recently viewed" for Tree 2.0, as it's both hard to discover and I wouldn't be able to make it keyboard-accessible with the current UX logic.
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/core/src/manager/components/sidebar/Explorer.tsx (1)
10-18:classNamefrom...restPropscan override the computedsb-sr-onlystyleThe component declares
classNameinExplorerPropsbut does not explicitly destructure it, so it ends up inrestProps. Since{...restProps}is spread after theclassNameattribute, a caller passingclassNamewould override the computed value and break the sr-only gating whenisBrowsingis false.While current call sites (Sidebar.tsx, Explorer.stories.tsx) don't pass
className, this is a defensive programming concern—explicitly destructuringclassNameand merging it prevents accidental overrides.Proposed fix (merge className, prevent override)
export interface ExplorerProps { className?: string; isLoading: boolean; isBrowsing: boolean; isHidden: boolean; hasEntries: boolean; dataset: CombinedDataset; selected: Selection; } export const Explorer: FC<ExplorerProps> = React.memo(function Explorer({ hasEntries, isLoading, isBrowsing, isHidden, dataset, selected, + className, ...restProps }) { const containerRef = useRef<HTMLElement>(null); @@ return ( <nav hidden={isHidden || undefined} aria-hidden={isHidden || undefined} - className={isBrowsing ? undefined : 'sb-sr-only'} + className={[className, isBrowsing ? null : 'sb-sr-only'].filter(Boolean).join(' ')} ref={containerRef} id="storybook-explorer-tree" data-highlighted-ref-id={highlighted?.refId} data-highlighted-item-id={highlighted?.itemId} {...landmarkProps} {...restProps} >Also applies to: 20-28, 45-55
🧹 Nitpick comments (2)
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (1)
142-149: Section/header tag changes are fine; consider whether<header>at the bottom is desired semantics
If the bottom bar is intentionally treated as a “banner” landmark (per PR goals), this is OK; otherwise, a neutral element withrole="banner"would avoid “header at the bottom” semantics.Also applies to: 151-171
code/core/src/manager/components/sidebar/Search.tsx (1)
316-318: Search landmark integration LGTM; consider adding an explicit landmark label
If landmark quick-nav announces multiple “search” regions (now or in the future), addingaria-label/aria-labelledbywould reduce ambiguity.Also applies to: 392-399
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/components/components/Card/Card.tsx
- code/core/src/manager/components/layout/useLandmarkIndicator.ts
- code/core/src/manager/components/preview/Toolbar.tsx
- code/core/src/manager/components/sidebar/types.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
🧬 Code graph analysis (4)
code/core/src/manager/components/sidebar/TestingWidget.tsx (1)
code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/sidebar/Search.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/sidebar/Explorer.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/manager/components/sidebar/Sidebar.tsx (2)
198-234: NewisNavVisible/isNavReachable/isSearchResultRenderedsplit is a good simplification; please sanity-check focus flows
Specifically: empty-search focus, non-empty-search focus, and closing results (Esc) should leave Explorer reachable again.
31-47: Banner landmark + sr-only H1 correctly implemented; no duplicate banner or IconSymbols instancesVerified:
- Sidebar and MobileNavigation render exclusively based on viewport (isMobile boolean); only one
role="banner"renders at a time- IconSymbols mounted only once in Sidebar.tsx production code; story files are test code and expected to include it
code/core/src/manager/components/sidebar/Search.tsx (1)
437-447: New nav state flags look reasonable; please verify the “Search focused → Explorer unreachable” fix in keyboard-only mode
Worth confirming for: empty input, non-empty input, Esc-to-close, and Shift+F6/F6 cycles.code/core/src/manager/components/sidebar/TestingWidget.tsx (1)
253-258: The landmark integration is already correct. Theref={cardRef}andoutlineAttrs={landmarkProps}both apply to the sameCardOutlineDOM element (the outer wrapper) because Card's implementation spreadsoutlineAttrsdirectly onto the element that receives therefat line 104 of Card.tsx. The aria-labelledby reference is properly set up. No additional hardening is needed.Likely an incorrect or invalid review comment.
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (1)
81-86: Banner uniqueness confirmed in mobile layoutThe landmark wiring is correct. In mobile mode, only the
MobileNavigationcomponent renders (lines 183–189 in Layout.tsx), while theSidebarwith its own banner renders only in desktop mode (lines 177–182). The conditional rendering ensures no duplicaterole="banner"landmarks. Thearia-labelledby={headingId}pattern correctly connects to the sr-only heading, and spreadinglandmarkPropsontoMobileBottomBarmatches proper hook usage.
Sidnioulz
force-pushed
the
sidnioulz/issue-32271
branch
from
2ff19a2 to
f9dc0a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
code/core/src/manager/components/sidebar/Heading.tsx (1)
90-96: LGTM! The skip link text change improves accessibility."Skip to content" is a more universally recognized skip link phrase than "Skip to canvas", which aligns well with standard accessibility patterns.
Minor nit: the styled component is still named
SkipToCanvasLink(line 50) while the text now says "Skip to content". Consider renaming toSkipToContentLinkfor consistency.♻️ Optional: Rename styled component for consistency
-const SkipToCanvasLink = styled(Button)(({ theme }) => ({ +const SkipToContentLink = styled(Button)(({ theme }) => ({And update the usage:
- <SkipToCanvasLink ariaLabel={false} asChild> + <SkipToContentLink ariaLabel={false} asChild>code/core/src/components/components/Card/Card.tsx (1)
100-100: Simplify redundant destructuring syntax.The
outlineAttrs: outlineAttrs = {}pattern renames the property to the same name. This can be simplified.♻️ Suggested simplification
- { outlineAnimation = 'none', outlineColor, outlineAttrs: outlineAttrs = {}, ...props }, + { outlineAnimation = 'none', outlineColor, outlineAttrs = {}, ...props },docs/get-started/browse-stories.mdx (1)
23-26: Consider clarifying that F6 is separate from the shortcuts menu.The callout is well-structured and uses appropriate
<kbd>tags for accessibility. However, given that line 19 references the Storybook shortcuts menu, users may look for F6 there and not find it.Per the PR objectives, the F6 shortcut is hardcoded by React Aria and won't appear in the shortcuts UI. Consider adding a brief note to manage expectations, such as:
<Callout variant="info" icon="♿"> - Storybook supports fast keyboard navigation between landmark regions. Press <kbd>F6</kbd> and <kbd>Shift+F6</kbd> to navigate between the sidebar, toolbar, preview, and addons panel. + Storybook supports fast keyboard navigation between landmark regions. Press <kbd>F6</kbd> and <kbd>Shift+F6</kbd> to navigate between the sidebar, toolbar, preview, and addons panel. This is a browser-standard shortcut and does not appear in the shortcuts menu. </Callout>code/core/src/manager/components/sidebar/Sidebar.tsx (1)
198-234: LGTM! Clear separation of visibility and reachability states.The rendering logic correctly maps the new UI state flags:
isNavVisible→ controls visual display (isBrowsing)isNavReachable→ controls DOM reachability (isHidden)isSearchResultRendered→ gatesSearchResultsrenderingThis ensures the nav remains keyboard-accessible (for F6 navigation) even when visually replaced by search results.
Minor nit: The fragment wrapper and braces around
Explorer(lines 210-219) could be simplified since it's no longer conditional, but this is purely cosmetic.code/core/src/manager/settings/shortcuts.tsx (2)
174-179: Update type for consistency withConfiguredShortcut.The
shortcutKeysproperty type isRecord<Feature, any>, buttoShortcutStatereturnsRecord<Feature, ConfiguredShortcut>. Updating this toRecord<Feature, ConfiguredShortcut>would improve type safety and consistency.♻️ Suggested fix
export interface ShortcutsScreenState { activeFeature: Feature; successField: Feature; - shortcutKeys: Record<Feature, any>; + shortcutKeys: Record<Feature, ConfiguredShortcut>; addonsShortcutLabels?: Record<string, string>; }
320-333: Remove unnecessary fragment wrapper.The empty fragment (
<>...</>) aroundTooltipProvideris unnecessary since it's the only element in this branch.♻️ Suggested fix
{hardcoded ? ( - <> - <TooltipProvider - tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} - placement="right" - > - <TextInput - aria-disabled - readOnly - valid={undefined} - value={shortcut ? shortcutToHumanString(shortcut) : ''} - /> - </TooltipProvider> - </> + <TooltipProvider + tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} + placement="right" + > + <TextInput + aria-disabled + readOnly + valid={undefined} + value={shortcut ? shortcutToHumanString(shortcut) : ''} + /> + </TooltipProvider> ) : (
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
code/core/package.jsoncode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/Form/styles.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/layout/useLandmarkIndicator.tscode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/sidebar/Heading.stories.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Tree.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/hooks/useLandmark.tscode/core/src/manager/settings/shortcuts.tsxcode/core/src/theming/global.tsdocs/get-started/browse-stories.mdx
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/Sidebar.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- code/core/src/manager/components/panel/Panel.tsx
- code/core/src/manager/components/layout/Layout.tsx
- code/core/src/manager/components/sidebar/TestingWidget.tsx
- code/core/src/manager/hooks/useLandmark.ts
- code/core/package.json
- code/core/src/manager/components/sidebar/Heading.stories.tsx
- code/core/src/manager/components/preview/Preview.tsx
- code/core/src/theming/global.ts
- code/core/src/manager/components/sidebar/Tree.tsx
- code/core/src/manager/components/sidebar/Search.tsx
- code/core/src/manager/components/sidebar/Tree.stories.tsx
- code/core/src/manager/components/layout/useLandmarkIndicator.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/types.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/Card/Card.tsxcode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/components/components/Form/styles.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Card/Card.tsxdocs/get-started/browse-stories.mdxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/components/components/Card/Card.tsxdocs/get-started/browse-stories.mdxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/sidebar/Sidebar.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
docs/get-started/browse-stories.mdxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Toolbar.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
🧬 Code graph analysis (3)
code/core/src/manager/components/sidebar/Explorer.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (2)
code/core/src/preview-api/index.ts (1)
useRef(13-13)code/core/src/manager/hooks/useLandmark.ts (1)
useLandmark(10-22)
code/core/src/manager/settings/shortcuts.tsx (1)
code/core/src/manager-api/modules/shortcuts.ts (1)
API_KeyCollection(95-95)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (26)
code/core/src/components/components/Form/styles.ts (1)
110-113: LGTM! Good accessibility enhancement.Expanding the disabled selector to include
aria-disabled="true"ensures consistent styling for elements that use ARIA-based disabled states (which remain focusable for screen reader accessibility). Thecursor: 'not-allowed'addition provides clear visual feedback.code/core/src/manager/components/preview/FramesRenderer.tsx (1)
99-103: LGTM!Removing the
titleattribute is appropriate here since the anchor text "Skip to sidebar" already provides a clear accessible name. TheariaLabel={false}on the Button correctly indicates that the children serve as the accessible name per project conventions.code/core/src/manager/components/preview/utils/components.ts (1)
5-19: LGTM! Landmark restructuring aligns with PR objectives.The changes correctly flatten the landmark hierarchy:
PreviewContainerdemoted frommaintodivremoves the outer landmarkFrameWrappromoted fromsectiontomainmakes it the primary main landmarkThis aligns with the PR objective to have a single main landmark at the preview level, with
landmarkPropsapplied via Preview.tsx for proper ARIA labeling.code/core/src/manager/components/preview/Toolbar.tsx (3)
1-1: LGTM!The imports for
useRefanduseLandmarkare correctly added to support the new landmark functionality.Also applies to: 21-21
87-91: LGTM! Landmark integration is well-implemented.The
useLandmarkhook is correctly used with:
role: 'region'matching the PR objective for the Toolbar landmarkaria-labelledbycorrectly referencing the existing sr-only heading (sb-preview-toolbar-titleat line 101)This approach aligns with the established pattern where the enclosing section is labeled via the sr-only heading, while the inner
StyledToolbarintentionally remains without its own aria-label. Based on learnings, this is the intended design.
98-99: LGTM!The
sectionRefand spreadlandmarkPropsare correctly applied toStyledSection, enabling F6/Shift+F6 landmark navigation for this region.code/core/src/components/components/Card/Card.tsx (1)
1-1: LGTM!The
DOMAttributesimport and the newoutlineAttrsprop are well-typed and appropriately optional. This cleanly supports the landmark focus outline attributes needed for accessibility.Also applies to: 92-96
docs/get-started/browse-stories.mdx (3)
11-13: LGTM!The section title update to "Sidebar and Preview" accurately reflects the expanded content, and the description correctly explains the relationship between story files, sidebar items, and the isolated preview iframe.
17-17: LGTM!The description of the three sidebar areas (search, explorer, component test widget) accurately reflects the landmark structure implemented in this PR and provides clear guidance for users.
19-19: LGTM!Clear and concise instruction for accessing keyboard shortcuts.
code/core/src/manager/components/sidebar/types.ts (1)
54-71: LGTM! Well-documented type expansion for granular UI state control.The separation of
isBrowsinginto three distinct flags (isNavVisible,isNavReachable,isSearchResultRendered) provides clearer semantics for accessibility-driven rendering decisions. The inline documentation concisely explains each flag's purpose.code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (3)
81-85: LGTM! Proper landmark setup for the mobile navigation bar.The
useLandmarkhook is correctly integrated withrole="banner"andaria-labelledbyreferencing the heading. This aligns with the PR objective to make MobileNavigation a proper header/banner landmark.
106-109: LGTM! Landmark props correctly applied with accessible labeling.The
landmarkPropsandrefare properly wired to theMobileBottomBar, and the visually hidden<h2>provides a clear accessible name for the banner region.
151-151: Semantic element aligns with landmark role.Using
<header>forMobileBottomBarcorrectly reflects itsrole="banner"semantics. The explicit role attribute is technically redundant since<header>has an implicit banner role (when not nested in sectioning content), but it doesn't cause issues and maintains consistency with other landmark integrations in this PR.code/core/src/manager/components/sidebar/Sidebar.tsx (3)
1-1: LGTM!Import additions are appropriate for the landmark integration.
31-47: Semantic structure correctly updated.Changing
Containerfrom<nav>to<header>aligns with the PR objective to make the Sidebar a banner landmark. The navigation role is now appropriately handled by theExplorercomponent's<nav>element.
142-153: LGTM! Proper landmark structure with accessible H1.The landmark integration correctly:
- Uses
role="banner"for the sidebar header- Provides a visually hidden
<h1>with "Storybook" as the main page heading, matching the accessibility audit recommendation from the linked issuecode/core/src/manager/components/sidebar/Explorer.tsx (4)
10-18: LGTM! Props interface properly extended.The new
classNameandisHiddenprops support the landmark integration and provide necessary control over visibility state for accessibility.
29-42: LGTM! Correct landmark configuration for navigation.The
useLandmarkhook is properly configured withrole="navigation"andaria-labelledbyreferencing the heading. The ref type change toHTMLElementaccommodates the semantic element change.
44-58: LGTM! Semantic nav element with proper hidden state handling.The implementation correctly handles two distinct hidden states:
isHidden=true→ fully hidden (hidden+aria-hidden) but still in DOM for landmark registrationisBrowsing=false→ visually hidden viasb-sr-onlybut still accessible to screen readers and keyboard navigationThis aligns with the PR objective to keep the nav reachable when Search is focused.
59-73: LGTM!The existing rendering logic for
HighlightStylesandRefcomponents is preserved within the new<nav>structure.code/core/src/manager/settings/shortcuts.tsx (5)
4-14: LGTM!The import additions are appropriate:
TooltipandTooltipProviderare used for the hardcoded shortcut UI, andAPI_KeyCollectionproperly types the shortcut arrays.
139-140: LGTM!The new landmark shortcut labels are clear and align well with the accessibility objectives.
147-155: LGTM!The
ConfiguredShortcuttype cleanly encapsulates the shortcut state, and the hardcoded landmark shortcuts correctly reflect React Aria's default F6/Shift+F6 keybindings as noted in the PR objectives.
157-172: LGTM!The function correctly merges configurable shortcuts from props with hardcoded shortcuts, ensuring hardcoded entries always take precedence. The defensive filtering against
fixedShortcutsin the second loop is acceptable for future-proofing.
305-356: LGTM!The
renderKeyInputmethod correctly distinguishes between editable and hardcoded shortcuts. Hardcoded shortcuts are rendered as read-only with an informative tooltip, while configurable shortcuts retain their full editing functionality. The implementation aligns well with the landmark navigation accessibility objectives.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.27 MB | 20.25 MB | 🎉 -24 KB 🎉 |
| Dependency size | 16.52 MB | 16.52 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🎉 -84 B 🎉 |
| Dependency size | 67.35 MB | 67.33 MB | 🎉 -24 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -36 B 🎉 |
| Dependency size | 65.92 MB | 65.90 MB | 🎉 -24 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 999 KB | 999 KB | 0 B |
| Dependency size | 36.79 MB | 36.77 MB | 🎉 -24 KB 🎉 |
| Bundle Size Analyzer | node | node |
Sidnioulz
force-pushed
the
sidnioulz/issue-32271
branch
from
277893a to
49f0abc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/get-started/browse-stories.mdx (1)
23-26: Consider adding a brief note about the preview iframe limitation.The Callout is clear and uses proper semantic markup with
<kbd>tags. However, the PR notes that F6 navigation cannot escape from within the preview iframe. For completeness, consider adding a brief caveat, though this may be too detailed for a getting-started guide.📝 Optional: Add iframe caveat
<Callout variant="info" icon="♿"> - Storybook supports fast keyboard navigation between landmark regions. Press <kbd>F6</kbd> and <kbd>Shift+F6</kbd> to navigate between the sidebar, toolbar, preview, and addons panel. + Storybook supports fast keyboard navigation between landmark regions. Press <kbd>F6</kbd> and <kbd>Shift+F6</kbd> to navigate between the sidebar, toolbar, preview, and addons panel. Note that this shortcut does not work from within the preview iframe. </Callout>code/core/src/manager/settings/shortcuts.tsx (1)
316-329: Consider removing the unnecessary fragment wrapper.The empty
<>...</>aroundTooltipProviderserves no purpose since there's only one child element.Proposed simplification
{hardcoded ? ( - <> - <TooltipProvider - tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} - placement="right" - > - <TextInput - aria-disabled - readOnly - valid={undefined} - value={shortcut ? shortcutToHumanString(shortcut) : ''} - /> - </TooltipProvider> - </> + <TooltipProvider + tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} + placement="right" + > + <TextInput + aria-disabled + readOnly + valid={undefined} + value={shortcut ? shortcutToHumanString(shortcut) : ''} + /> + </TooltipProvider> ) : (
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
code/core/src/components/components/Form/styles.tscode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager/settings/shortcuts.tsxdocs/get-started/browse-stories.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/components/components/Form/styles.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/settings/shortcuts.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/settings/shortcuts.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/settings/shortcuts.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxdocs/get-started/browse-stories.mdxcode/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxdocs/get-started/browse-stories.mdx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/Explorer.stories.tsxdocs/get-started/browse-stories.mdxcode/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
🧬 Code graph analysis (1)
code/core/src/manager/settings/shortcuts.tsx (1)
code/core/src/manager-api/modules/shortcuts.ts (1)
API_KeyCollection(95-95)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (10)
code/core/src/manager/components/sidebar/Explorer.stories.tsx (1)
76-101: LGTM!The addition of
isHidden={false}to both story components correctly satisfies the updatedExplorerPropsinterface. Setting it tofalseensures the Explorer remains visible in these story previews, which is the expected behavior for testing/demonstration purposes.docs/get-started/browse-stories.mdx (2)
11-13: LGTM!The updated heading and description clearly introduce the sidebar and preview concepts.
17-19: LGTM!The description of the three sidebar areas aligns well with the landmark structure introduced in this PR, and the keyboard shortcuts guidance is appropriately concise.
code/core/src/manager-api/modules/shortcuts.ts (3)
120-121: LGTM!The new landmark navigation shortcuts are correctly added to the
API_Shortcutsinterface, maintaining consistency with the existing pattern.
162-163: LGTM!Default shortcuts correctly match react-aria's hardcoded F6/Shift+F6 landmark navigation keys. The inline comments documenting this constraint are helpful.
280-283: LGTM!Appropriate no-op handling since these shortcuts are managed by
@react-aria/landmarkoutside the standard shortcut flow. The comment clearly explains the delegation.code/core/src/manager/settings/defaultShortcuts.tsx (1)
26-27: LGTM!Shortcuts are consistent with the API module defaults. The hardcoded comments appropriately document the react-aria constraint.
code/core/src/manager/settings/shortcuts.tsx (3)
139-140: LGTM!Labels are clear and follow the existing naming convention for shortcut descriptions.
155-168: LGTM!The
toShortcutStatefunction correctly transforms shortcut data and applies thehardcodedflag based on array membership.
330-343: LGTM!The editable shortcut input maintains the existing interaction pattern with proper focus/blur handlers and validation display.
Co-authored-by: Kyle Gach <kyle.gach@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
||
|  | ||
|
|
||
| <Callout variant="info" icon="♿"> |
kylegach
left a comment
kylegach
left a comment
There was a problem hiding this comment.
Docs updates are good! Thanks, @Sidnioulz!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager/settings/shortcuts.tsx (1)
315-328: Consider minor cleanup: remove unnecessary fragment and refine accessibility attributes.Two small suggestions for this block:
- The fragment wrapper (
<>...</>) on lines 316/328 is unnecessary sinceTooltipProvideris the only child element.- The hardcoded input can still receive focus via tab navigation but offers no interaction. Consider adding
tabIndex={-1}to remove it from the tab order, or alternatively usedisabledif the component supports it (which would also visually indicate the disabled state).♻️ Suggested cleanup
{hardcoded ? ( - <> - <TooltipProvider - tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} - placement="right" - > - <TextInput - aria-disabled - readOnly - valid={undefined} - value={shortcut ? shortcutToHumanString(shortcut) : ''} - /> - </TooltipProvider> - </> + <TooltipProvider + tooltip={<Tooltip hasChrome>This shortcut cannot be changed.</Tooltip>} + placement="right" + > + <TextInput + aria-disabled="true" + tabIndex={-1} + readOnly + valid={undefined} + value={shortcut ? shortcutToHumanString(shortcut) : ''} + /> + </TooltipProvider> ) : (
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/settings/shortcuts.tsxdocs/get-started/browse-stories.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/get-started/browse-stories.mdx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/settings/shortcuts.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/settings/shortcuts.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/settings/shortcuts.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/settings/shortcuts.tsx
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/manager/settings/shortcuts.tsx (3)
4-4: LGTM!The new imports for
Tooltip,TooltipProvider, andAPI_KeyCollectionare appropriate for the hardcoded shortcut functionality being added.Also applies to: 8-9
139-140: LGTM!The new landmark navigation shortcut labels are descriptive and consistent with the existing naming conventions.
147-167: LGTM!The
ConfiguredShortcuttype cleanly captures the shortcut state including the hardcoded flag. ThetoShortcutStatefunction correctly filters fixed shortcuts and marks hardcoded ones. The comment on lines 152-153 provides helpful context about the React Aria constraint.
UI: Improve landmark navigation (cherry picked from commit 283d093)
ndelangen
removed
the
patch:yes
5 participants
Closes #32271
What I did
Peek.2026-01-09.19-27.webm
headerelement,bannerrole, was previously a nav)searchrole)navrole)mainrole)regionrole)asiderole)header/bannerto the MobileNavigation bottom barFix preview iframe eating up F6 key events(can't find a way)Still gotta handle a few focus / keyboard handling edge cases.
Checklist for Contributors
Testing
Not tested
Manual testing
With screen reader
Without screen reader
Limitations
Documentation
@kylegach @jonniebigodes @MichaelArestad I'd like to get your opinions on how to make this feature discoverable to users. Should we add it in the shortcuts page, even though it can't be edited, just for visibility? Is there a best page to document it in the docs website?
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.