Core: Use canonical links in sidebar and menu#33400
Conversation
Sidnioulz
added
maintenance
|
View your CI Pipeline Execution ↗ for commit 6d9ccc2
☁️ Nx Cloud last updated this comment at |
|
Note: a shared util was started but discarded as Gert is also writing one. I had added a new manager error type: export class EntryHasNoPathError extends StorybookError {
constructor(
public data: {
entry: API_HashEntry;
}
) {
super({
name: 'EntryHasNoPathError',
category: Category.MANAGER_UTILS,
code: 1,
message: `Entry of type "${data.entry.type}" cannot be converted to a URL path. Only 'story' and 'docs' entries have paths. Full entry: ${JSON.stringify(
data.entry,
null,
2
)}`,
});
}
}And I had edited three code paths:
|
📝 WalkthroughWalkthroughThis PR refactors sidebar navigation components by introducing href-based link routing with internal/external navigation handling in Menu, splitting the StoryNode component into specialized StoryBranchNode and StoryLeafNode variants, and applying theme-based styling updates to modal and mobile drawer components. Updates accompanying story files and mocks accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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
🧹 Nitpick comments (6)
code/core/src/manager/components/sidebar/Menu.tsx (1)
97-102: Consider addingrel="noopener noreferrer"for external links and reviewingrel="canonical"usage.Two concerns:
External links with
target="_blank"should includerel="noopener noreferrer"to prevent the new page from accessingwindow.opener(security best practice).
rel="canonical"is typically used to tell search engines which URL is the "preferred" version when duplicate content exists. Using it on all internal navigation links may not achieve the intended SEO benefit and could confuse crawlers.🔎 Suggested fix
{...(link.href && { as: 'a', href: link.href, target: link.internal ? undefined : '_blank', - rel: link.internal ? 'canonical' : undefined, + rel: link.internal ? undefined : 'noopener noreferrer', })}code/core/src/manager-api/modules/stories.ts (3)
63-64: DuplicateDEFAULT_REF_IDconstant.This constant already exists in
code/core/src/manager/components/sidebar/Sidebar.tsx(line 26). Consider importing from a shared location to avoid duplication and potential drift.
456-488: Duplicated URL construction logic.The URL template on lines 470 and 487 is identical. Consider extracting to a helper function.
🔎 Suggested refactor
+ const buildEntryUrl = (entry: API_HashEntry, refId: string | undefined): string => { + const entryId = !refId || refId === DEFAULT_REF_ID ? entry.id : `${refId}_${entry.id}`; + return `./?path=/${entry.type}/${entryId}`; + }; + getSiblingComponentHref: (direction) => { const result = findSiblingComponent(direction); if (!result) { return null; } const { filteredIndex, refs, refId } = store.getState(); const hash = refId ? refs[refId].filteredIndex || {} : filteredIndex; const entry = hash?.[result]; if (!entry || (entry.type !== 'docs' && entry.type !== 'story')) { return null; } - // TODO: port this to getStoryUrls once available. - return `./?path=/${entry.type}/${!refId || refId === DEFAULT_REF_ID ? entry.id : `${refId}_${entry.id}`}`; + return buildEntryUrl(entry, refId); },
472-488: Redundant story data fetch ingetSiblingStoryHref.
findSiblingStoryalready callsapi.getData(storyId, refId)internally (line 368). ThengetSiblingStoryHrefcalls it again (line 479). Consider returning the resolved hash from the helper or restructuring to avoid the duplicate lookup.code/core/src/manager/components/sidebar/TreeNode.tsx (2)
154-175: Inner function name mismatch.The exported component is
StoryBranchNode, but the inner function passed toReact.memois namedStoryNode. This causes the component to appear as "StoryNode" in React DevTools and error stack traces, which can be confusing during debugging.🔎 Suggested fix
export const StoryBranchNode: FC<ComponentProps<typeof BranchNode>> = React.memo( - function StoryNode({ + function StoryBranchNode({ theme, children, isExpandable = false, isExpanded = false, isSelected, ...props }) {
177-192: Inner function name mismatch.Same issue as
StoryBranchNode- the inner function is namedStoryNodebut should beStoryLeafNodefor clarity in DevTools and stack traces.🔎 Suggested fix
-export const StoryLeafNode: FC<ComponentProps<typeof LeafNode>> = React.memo(function StoryNode({ +export const StoryLeafNode: FC<ComponentProps<typeof LeafNode>> = React.memo(function StoryLeafNode({ theme, children, ...props }) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/manager-api/modules/stories.ts(4 hunks)code/core/src/manager/components/sidebar/Menu.tsx(1 hunks)code/core/src/manager/components/sidebar/Tree.tsx(4 hunks)code/core/src/manager/components/sidebar/TreeNode.tsx(3 hunks)code/core/src/manager/container/Menu.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/manager/container/Menu.tsxcode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/Menu.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/manager/container/Menu.tsxcode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/Menu.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/container/Menu.tsxcode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/Menu.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/manager/container/Menu.tsxcode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/Menu.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/manager/container/Menu.tsxcode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/Menu.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
📚 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-api/modules/stories.tscode/core/src/manager/components/sidebar/TreeNode.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/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
code/core/src/manager/components/sidebar/TreeNode.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/TreeNode.tsx
🧬 Code graph analysis (3)
code/core/src/manager-api/modules/stories.ts (2)
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
DEFAULT_REF_ID(27-27)code/core/src/csf/csf-factories.ts (1)
story(114-118)
code/core/src/manager/components/sidebar/Tree.tsx (1)
code/core/src/manager/components/sidebar/TreeNode.tsx (6)
DocumentNode(140-152)StoryLeafNode(177-192)ComponentNode(117-138)GroupNode(96-115)StoryBranchNode(154-175)TestNode(194-209)
code/core/src/manager/components/sidebar/Menu.tsx (1)
code/lib/cli-storybook/src/link.ts (1)
link(15-103)
⏰ 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: Core Unit Tests, windows-latest
🔇 Additional comments (8)
code/core/src/manager/container/Menu.tsx (3)
24-27: LGTM!The
internalflag extension toMenuItemis clean and integrates well with the existing type structure.
81-92: LGTM!The
aboutmenu item correctly adds href for SEO/UX purposes while retaining theonClickhandler for router-based navigation.
164-175: No changes needed. Theapiparameter is already included in the dependency array on line 174:[api, enableShortcuts, shortcutKeys]. All similar menu items (down,prev,next) follow the same correct pattern.Likely an incorrect or invalid review comment.
code/core/src/manager/components/sidebar/Menu.tsx (1)
106-127: LGTM!The click handler correctly prevents default navigation for both disabled links and internal links that should use router-based navigation while passing the link metadata to the onClick callback.
code/core/src/manager/components/sidebar/Tree.tsx (4)
50-58: LGTM!Import changes correctly align with the new
StoryBranchNodeandStoryLeafNodeexports fromTreeNode.tsx.
265-265: LGTM!Correct use of
StoryLeafNodefor docs/story leaf nodes that are not expandable.
389-391: LGTM!The branch node mapping correctly uses
StoryBranchNodefor story items that have children, maintaining consistency with the new node type structure.
470-470: LGTM!Correct use of
StoryLeafNodeas the fallback for non-test story/docs items.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/core/src/manager-api/modules/stories.ts (1)
341-381: Inconsistent fallback handling betweenfindSiblingComponentandfindSiblingStory.Line 353 uses
refs[refId].filteredIndex || {}while line 374 usesrefs[refId].filteredIndexwithout the fallback. This inconsistency is also repeated ingetSiblingComponentHref(line 463) andgetSiblingStoryHref(line 479).Both approaches eventually return undefined/null when the index is unavailable, but the inconsistent pattern could cause confusion or subtle bugs. Consider standardizing the approach across all four locations.
🔎 Suggested fix for consistency
Apply the same fallback pattern to both helpers and their corresponding href methods:
const findSiblingStory = (direction: Direction): StoryId | undefined => { const { filteredIndex, storyId, refs, refId } = store.getState(); const story = api.getData(storyId, refId); if (!story) { return undefined; } - const hash = refId ? refs[refId].filteredIndex : filteredIndex; + const hash = refId ? refs[refId].filteredIndex || {} : filteredIndex; if (!hash) { return undefined; } return api.findSiblingStoryId(storyId, hash, direction, false); };And similarly for
getSiblingStoryHrefat line 479.
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/TreeNode.stories.tsx (1)
91-171: Consider addingStoryLeafNodeexamples for completeness.The
NestedandNestedLongNamestories demonstrate how various node types render at different depths, but only includeStoryBranchNodefor story nodes. AddingStoryLeafNodeexamples at some depths would provide more complete coverage of how leaf nodes render in hierarchical contexts.Example: Add a StoryLeafNode to the Nested story
<StoryBranchNode depth={2}>Two</StoryBranchNode> + <StoryLeafNode depth={2}>Two (leaf)</StoryLeafNode> <TestNode depth={3}>Three</TestNode>code/core/src/manager-api/modules/stories.ts (1)
456-487: URL construction pattern differs fromgotoStoryand duplicates logic.The URL construction in these methods uses
refIdfrom state:!refId || refId === DEFAULT_REF_ID ? entry.id : `${refId}_${entry.id}`However,
gotoStory(line 509) usesentry.refIdfrom the entry itself:entry.refId ? `${entry.refId}_${entry.id}` : entry.idUsing
entry.refIdwould be more consistent and reliable, since the entry already hasrefIdpopulated when retrieved from the index. Consider aligning with thegotoStorypattern.Additionally, the URL construction logic is duplicated between
getSiblingComponentHrefandgetSiblingStoryHref. As noted in the TODO comments, this should be extracted to a shared utility (e.g.,getStoryUrls) once available.🔎 Proposed fix for URL construction consistency
getSiblingComponentHref: (direction) => { const result = findSiblingComponent(direction); if (!result) { return null; } const { filteredIndex, refs, refId } = store.getState(); const hash = refId ? refs[refId].filteredIndex || {} : filteredIndex; const entry = hash?.[result]; if (!entry || (entry.type !== 'docs' && entry.type !== 'story')) { return null; } // TODO: port this to getStoryUrls once available. - return `./?path=/${entry.type}/${!refId || refId === DEFAULT_REF_ID ? entry.id : `${refId}_${entry.id}`}`; + return `./?path=/${entry.type}/${entry.refId ? `${entry.refId}_${entry.id}` : entry.id}`; },Apply the same change to
getSiblingStoryHrefat line 486.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/TreeNode.stories.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
🧠 Learnings (4)
📚 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/TreeNode.stories.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/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
code/core/src/manager/components/sidebar/TreeNode.stories.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/TreeNode.stories.tsxcode/core/src/manager-api/modules/stories.ts
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/TreeNode.stories.tsx (1)
code/core/src/manager/components/sidebar/TreeNode.tsx (4)
StoryLeafNode(177-192)StoryBranchNode(154-175)TestNode(194-209)ComponentNode(117-138)
⏰ 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 (6)
code/core/src/manager/components/sidebar/TreeNode.stories.tsx (4)
6-13: LGTM: Import changes correctly reflect the component split.The imports now include both
StoryBranchNodeandStoryLeafNode, aligning with the refactoring that splits the previousStoryNodeinto separate branch and leaf components.
19-19: LGTM: Appropriate default component selection.Setting
StoryLeafNodeas the default component is a sensible choice, as leaf nodes represent the simpler, more common case.
34-35: LGTM: Clear demonstration of branch vs. leaf distinction.The Types story effectively showcases both node variants with descriptive labels that clarify their semantic differences.
173-351: LGTM: Consistent use of leaf nodes for selection testing.The
SelectionandSelectionWithLongNamestories appropriately useStoryLeafNodethroughout, correctly testing selection and highlighting states on leaf nodes.code/core/src/manager-api/modules/stories.ts (2)
442-455: Clean refactoring with shared helper functions.Extracting the sibling-finding logic into
findSiblingComponentandfindSiblingStoryreduces duplication and enables reuse by the newgetSibling*Hrefmethods. The navigation logic remains unchanged.
63-64: The constant value'storybook_internal'is confirmed correct and consistently used throughout the codebase to determine when to omit the ref prefix from generated URLs.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 538 | 🚨 +4 🚨 |
| Self size | 646 KB | 646 KB | 🚨 +64 B 🚨 |
| Dependency size | 59.16 MB | 59.19 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 278 | 🚨 +4 🚨 |
| Self size | 24 KB | 24 KB | 0 B |
| Dependency size | 44.09 MB | 44.12 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 64 | 68 | 🚨 +4 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +30 B 🚨 |
| Dependency size | 5.95 MB | 5.98 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 170 | 🚨 +4 🚨 |
| Self size | 18 KB | 18 KB | 🚨 +52 B 🚨 |
| Dependency size | 31.23 MB | 31.26 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Sidnioulz
force-pushed
the
sidnioulz/exp-use-links-to-navigate
branch
from
6d65c94 to
63b3420
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/core/src/manager/components/sidebar/Menu.stories.tsx (4)
19-43: Good consolidation of API mocks.The new
getAPIMocks()helper centralizes common mock definitions and includes the new sibling navigation methods (getSiblingComponentHref,getSiblingStoryHref) mentioned in the PR objectives.Consider making this helper accept parameters for commonly-overridden values (e.g.,
isWhatsNewUnread, custom shortcuts) to eliminate the override pattern seen in the stories below.Optional: Parameterized mock factory
-const getAPIMocks = () => ({ +const getAPIMocks = (overrides: Partial<ReturnType<typeof getAPIMocks>> = {}) => ({ getShortcutKeys: fn() .mockName('api::getShortcutKeys') .mockImplementation(() => ({})), getAddonsShortcuts: fn() .mockName('api::getAddonsShortcuts') .mockImplementation(() => ({})), versionUpdateAvailable: fn() .mockName('api::versionUpdateAvailable') .mockImplementation(() => false), isWhatsNewUnread: fn() .mockName('api::isWhatsNewUnread') .mockImplementation(() => false), getDocsUrl: fn() .mockName('api::getDocsUrl') .mockImplementation(() => 'https://storybook.js.org/docs/'), toggleNav: fn().mockName('api::toggleNav'), toggleToolbar: fn().mockName('api::toggleToolbar'), togglePanel: fn().mockName('api::togglePanel'), jumpToComponent: fn().mockName('api::jumpToComponent'), getSiblingComponentHref: fn().mockName('api::getSiblingComponentHref'), jumpToStory: fn().mockName('api::jumpToStory'), getSiblingStoryHref: fn().mockName('api::getSiblingStoryHref'), emit: fn().mockName('api::emit'), + ...overrides, });This would allow stories to call
getAPIMocks({ isWhatsNewUnread: fn().mockImplementation(() => true) })instead of spreading and overriding.
119-126: Remove redundant mock overrides.Lines 121-125 override methods with implementations identical to the
getAPIMocks()defaults. These overrides can be safely removed since spreadinggetAPIMocks()already provides the same values.Simplified code
const menu = useMenu({ api: { ...getAPIMocks(), - // @ts-expect-error (Converted from ts-ignore) - getShortcutKeys: () => ({}), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, - isWhatsNewUnread: () => false, - getDocsUrl: () => 'https://storybook.js.org/docs/', }, showToolbar: false,
170-190: Remove redundant mock overrides.Only
getShortcutKeys(lines 172-185) needs to override the default fromgetAPIMocks(). The other overrides on lines 186-189 are redundant.Simplified code
const menu = useMenu({ api: { ...getAPIMocks(), // @ts-expect-error (invalid) getShortcutKeys: () => ({ shortcutsPage: ['⌘', '⇧', ','], toggleNav: ['⌥', 'S'], togglePanel: ['⌥', 'A'], toolbar: ['⌥', 'T'], panelPosition: ['⌥', 'D'], fullScreen: ['⌥', 'F'], search: ['⌥', 'K'], prevComponent: ['⌥', '↑'], nextComponent: ['⌥', '↓'], prevStory: ['⌥', '←'], nextStory: ['⌥', '→'], collapseAll: ['⌥', '⇧', '↑'], }), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, - isWhatsNewUnread: () => false, - getDocsUrl: () => 'https://storybook.js.org/docs/', }, showToolbar: false,
223-230: Remove redundant mock overrides.Only
isWhatsNewUnread(line 228) needs to override the default fromgetAPIMocks()to returntrue. The other overrides on lines 225-227 and 229 are redundant.Simplified code
const menu = useMenu({ api: { ...getAPIMocks(), - // @ts-expect-error (invalid) - getShortcutKeys: () => ({}), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, isWhatsNewUnread: () => true, - getDocsUrl: () => 'https://storybook.js.org/docs/', }, showToolbar: false,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Menu.stories.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/manager/components/sidebar/Menu.stories.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/manager/components/sidebar/Menu.stories.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/Menu.stories.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/manager/components/sidebar/Menu.stories.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/manager/components/sidebar/Menu.stories.tsx
🧠 Learnings (6)
📓 Common learnings
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/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.
📚 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/Menu.stories.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/Menu.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 implementations should match the expected return type of the original function
Applied to files:
code/core/src/manager/components/sidebar/Menu.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/Menu.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 properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/manager/components/sidebar/Menu.stories.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: Core Unit Tests, windows-latest
- GitHub Check: nx
|
@ghengeveld @ndelangen could either of you please review this PR? I still need to merge it to get indexing results from Google Search Console for the About page project. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager-api/modules/stories.ts (1)
399-399: Redundant state read.Line 399 retrieves
refIdfrom state, but it was already retrieved on line 382 and nothing modifies the store between these lines. The second read shadows the outer variable and adds unnecessary overhead.🔎 Suggested simplification
const result = api.findSiblingStoryId(storyId, hash, direction, true); if (result) { - const { refId } = store.getState(); api.selectStory(result, undefined, { ref: refId }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager/container/Menu.tsx
- code/core/src/manager/components/sidebar/Menu.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-api/modules/stories.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-api/modules/stories.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager-api/modules/stories.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-api/modules/stories.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-api/modules/stories.ts
🧠 Learnings (3)
📓 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.
📚 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-api/modules/stories.ts
📚 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-api/modules/stories.ts
⏰ 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
Sidnioulz
force-pushed
the
sidnioulz/exp-use-links-to-navigate
branch
from
3df2150 to
82a081c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/TreeNode.tsx (2)
155-176: Fix the function name to match the export.The component is exported as
StoryBranchNodebut the internal function is namedStoryNode(line 156). This mismatch can cause confusion in React DevTools and stack traces.🔎 Proposed fix
export const StoryBranchNode: FC<ComponentProps<typeof BranchNode>> = React.memo( - function StoryNode({ + function StoryBranchNode({ theme, children, isExpandable = false, isExpanded = false, isSelected, ...props }) {
178-193: Fix the function name to match the export.The component is exported as
StoryLeafNodebut the internal function is namedStoryNode(line 178). This mismatch can cause confusion in React DevTools and stack traces.🔎 Proposed fix
-export const StoryLeafNode: FC<ComponentProps<typeof LeafNode>> = React.memo(function StoryNode({ +export const StoryLeafNode: FC<ComponentProps<typeof LeafNode>> = React.memo(function StoryLeafNode({ theme, children, ...props }) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/container/Menu.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/manager/container/Menu.tsx
- code/core/src/manager/components/sidebar/Tree.tsx
- code/core/src/manager-api/modules/stories.ts
- code/core/src/manager/components/sidebar/Menu.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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/Menu.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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/Menu.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/Menu.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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/Menu.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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.tsxcode/core/src/manager/components/sidebar/Menu.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/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.
📚 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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.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/TreeNode.tsxcode/core/src/manager/components/sidebar/TreeNode.stories.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/TreeNode.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/TreeNode.stories.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/TreeNode.stories.tsx (1)
code/core/src/manager/components/sidebar/TreeNode.tsx (2)
StoryLeafNode(178-193)StoryBranchNode(155-176)
⏰ 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/Menu.tsx (1)
97-127: LGTM! Clean implementation of internal vs external link handling.The conditional rendering logic correctly:
- Converts menu items with
hrefto anchor elements- Opens external links in new tabs while keeping internal links in the same context
- Applies
rel="canonical"to internal navigation links for SEO- Prevents browser navigation for internal links while preserving
hreffor accessibility and UXThe click handler appropriately guards against disabled state and prevents default navigation for internal links.
code/core/src/manager/components/sidebar/TreeNode.tsx (1)
155-193: Clean refactoring to separate branch and leaf story nodes.The split of the previous
StoryNodeintoStoryBranchNode(for expandable stories with tests) andStoryLeafNode(for leaf stories) provides clearer semantics and better aligns with the component hierarchy structure.code/core/src/manager/components/sidebar/TreeNode.stories.tsx (3)
6-13: LGTM! Imports correctly updated.The imports properly reflect the new
StoryBranchNodeandStoryLeafNodecomponents, replacing the previousStoryNodeexport.
30-38: Nice addition showing both branch and leaf variants.The Types story effectively demonstrates the distinction between
StoryBranchNode(for stories with tests) andStoryLeafNode(for leaf node stories), making the component API clear.
91-110: Correct usage of StoryBranchNode and StoryLeafNode throughout stories.The stories appropriately use:
StoryBranchNodefor nested/hierarchical examples where stories may have childrenStoryLeafNodefor selection examples where stories are terminal nodesThis aligns well with the semantic distinction introduced in TreeNode.tsx.
Also applies to: 173-211
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/sidebar/Menu.tsx:
- Line 141: The anchor rendering in Menu.tsx incorrectly sets rel={link.internal
? 'canonical' : 'noreferrer'}; change it to omit or neutralize the rel for
internal links (e.g., set rel={link.internal ? undefined : 'noreferrer'} or
remove the rel prop when link.internal is true) and apply the same fix to the
mobile About button rendering; update the conditional that produces 'canonical'
so internal anchors do not use the canonical rel value.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/components/components/Modal/Modal.styled.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/sidebar/Menu.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/Menu.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/components/components/Modal/Modal.styled.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/Menu.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/components/components/Modal/Modal.styled.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/Menu.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/components/components/Modal/Modal.styled.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/Menu.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/components/components/Modal/Modal.styled.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/Menu.tsxcode/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsxcode/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsxcode/core/src/manager/components/mobile/about/MobileAbout.tsxcode/core/src/components/components/Modal/Modal.styled.tsx
🧠 Learnings (3)
📚 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/Menu.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/Menu.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/Menu.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: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (8)
code/core/src/components/components/Modal/Modal.styled.tsx (2)
115-115: LGTM: Theme-based styling for bottom-drawer variant.The addition of the
themeparameter and application ofborderRadius, along with usingtheme.background.content, properly aligns the bottom-drawer styling with the theme system and creates visual consistency across mobile components.Also applies to: 141-141, 146-146
141-142: No action required. Theoverflow: 'hidden'on the bottom-drawer variant is intentional and follows an established design pattern. Mobile drawer consumers (MobileMenuDrawer, MobileAddonsDrawer) properly handle scrolling through child components like ScrollArea, which manage their own scroll behavior independently. This prevents double scrollbars and gives full control over scroll implementation, consistent with similar container patterns in the codebase.code/core/src/manager/components/mobile/about/MobileAbout.tsx (1)
122-122: LGTM: Consistent border radius styling.The
borderRadius: '10px 10px 0 0'addition rounds the top corners and aligns with the visual pattern established in Modal.styled.tsx for bottom-drawer variants.code/core/src/manager/components/mobile/navigation/MobileAddonsDrawer.tsx (1)
22-32: LGTM: Simplified drawer implementation.Removing the
StyledModalwrapper and usingModaldirectly withvariant="bottom-drawer"centralizes styling logic and reduces redundancy. The explicitheight="42vh"prop appropriately sizes the addon drawer.code/core/src/manager/components/mobile/navigation/MobileMenuDrawer.tsx (1)
23-34: LGTM: Consistent drawer refactoring.The removal of
StyledModalin favor of directModalusage mirrors the MobileAddonsDrawer pattern and centralizes styling. Theheight="80vh"appropriately accommodates the menu navigation and nestedMobileAboutcomponent.code/core/src/manager/components/sidebar/Menu.tsx (3)
114-135: Well-structured onClick handler flow.The onClick logic properly handles:
- Disabled state guard (prevents any interaction)
- Internal link navigation (prevents browser navigation while preserving href)
- Link-specific onClick callback with complete metadata
- Menu closing behavior via closeOnClick flag
122-124: Internal link routing is correctly implemented.The
link.internalflag is properly set for menu items (about,guide, andshortcutsall haveinternal: true), and thelink.onClickcallbacks properly invoke navigation through API methods (api.changeSettingsTab(),api.toggleNav(),api.jumpToComponent(), etc.). The preventDefault pattern correctly prevents browser navigation while ensuring the appropriate API method is called to handle routing. No action needed.
175-178: The mobile About button navigation works correctly—no fix needed.The
onClickhandler properly invokessetMobileAboutOpento toggle the modal state. ThepreventDefault()is intentional because navigation is handled via React state, not URL navigation. The MobileAbout component is rendered in MobileMenuDrawer and responds to state changes, displaying the about panel when opened.Likely an incorrect or invalid review comment.
| <a | ||
| href={link.href} | ||
| target={link.internal ? undefined : '_blank'} | ||
| rel={link.internal ? 'canonical' : 'noreferrer'} |
There was a problem hiding this comment.
Critical: rel="canonical" is incorrect for anchor tags.
The rel="canonical" attribute is semantically invalid on <a> elements. The canonical link relation is intended for <link> elements in the document <head> to indicate the preferred URL for search engine indexing, not for navigation anchors.
For internal navigation links:
- Omit the
relattribute entirely, OR - Use a descriptive value like
rel="internal"if needed for styling/scripting
This same issue appears on line 182 for the mobile About button.
🔧 Suggested fix
{link.href ? (
<a
href={link.href}
target={link.internal ? undefined : '_blank'}
- rel={link.internal ? 'canonical' : 'noreferrer'}
+ rel={link.internal ? undefined : 'noreferrer'}
>
{linkContent}
</a>And for the mobile button (line 182):
- <a href="./?path=/settings/about" rel="canonical">
+ <a href="./?path=/settings/about">
<CogIcon />
</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rel={link.internal ? 'canonical' : 'noreferrer'} | |
| rel={link.internal ? undefined : 'noreferrer'} |
🤖 Prompt for AI Agents
In @code/core/src/manager/components/sidebar/Menu.tsx at line 141, The anchor
rendering in Menu.tsx incorrectly sets rel={link.internal ? 'canonical' :
'noreferrer'}; change it to omit or neutralize the rel for internal links (e.g.,
set rel={link.internal ? undefined : 'noreferrer'} or remove the rel prop when
link.internal is true) and apply the same fix to the mobile About button
rendering; update the conditional that produces 'canonical' so internal anchors
do not use the canonical rel value.
ghengeveld
added
needs qa
ndelangen
removed
needs qa
Sidnioulz
added
the
experiment
4 participants
What I did
awhen relevantrel="canonical"attr to help crawlers index our contentChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
ø
Manual testing
buttons to avoid disrupting changes made by @ghengeveld, but we could move these back to be links when we rework the sidebar for consistency)Documentation
MIGRATION.MD
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
Release Notes
Improvements
Style
✏️ Tip: You can customize this high-level summary in your review settings.