Core: Onboarding guide #32874
Core: Onboarding guide #32874
Conversation
📝 WalkthroughWalkthroughExtended checklist data/model and store (muted/accepted/done/skipped), introduced FocusRing + FocusTarget and useLocationHash, refactored UI primitives (Button, Card, Listbox, Collapsible), replaced static sidebar checklist with a hook-driven ChecklistModule and updated menu/sidebar APIs and stories. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Guide as GuidePage/Checklist UI
participant Hook as useChecklist
participant Store as ChecklistStore
participant Focus as FocusTarget/useLocationHash
User->>Guide: open Guide / Sidebar
Guide->>Hook: call useChecklist()
Hook->>Store: read state (loaded, muted, accepted, done, skipped)
Store-->>Hook: return state
Hook->>Guide: return sections, nextItems, progress, callbacks
Guide->>Guide: render items & progress
User->>Guide: click item action / accept / skip / mute
Guide->>Store: call accept/done/skip/mute
Store->>Store: update state and emit change
Store-->>Hook: state updated
Hook-->>Guide: updated derived state
Guide->>User: UI updates
User->>Focus: navigate to `#hash`
Focus->>Focus: useLocationHash detects hash, focus element, apply highlight, clear after duration
sequenceDiagram
participant Sidebar as Sidebar Component
participant Consumer as Manager Consumer
participant MenuHook as useMenu
participant ListboxUI as SidebarMenuList
Consumer->>MenuHook: call useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts })
MenuHook->>Hook: call useChecklist() for guide progress
MenuHook-->>Consumer: return NormalLink[][]
Consumer->>ListboxUI: render menu rows via Listbox components
ListboxUI->>User: present clickable ListboxItems with ListboxIcon/progress
User->>ListboxUI: click link
ListboxUI->>Consumer: invoke navigation/onClick handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 405b934
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
code/core/src/components/components/Button/Button.tsx (4)
42-51: Guard clicks when disabled/readOnly (also foras="a").Currently clicks still propagate, animate, and call
onClick. Block interaction at the handler level.Apply this diff:
- const handleClick = (event: SyntheticEvent) => { - if (onClick) { - onClick(event); - } - - if (animation === 'none') { - return; - } - setIsAnimating(true); - }; + const handleClick = (event: SyntheticEvent) => { + if (disabled || readOnly) { + event.preventDefault?.(); + event.stopPropagation?.(); + return; + } + onClick?.(event); + if (animation !== 'none') setIsAnimating(true); + };
63-76: Wire a11y and interactivity props (aria-disabled, tabIndex, role) and drop handler when not interactive.Make non-interactive elements non-focusable; add semantics for div/label.
Apply this diff:
- <StyledButton + {/* + Treat readOnly as non-interactive UI: aria-disabled + no handler. + For div/label, expose role=button only when interactive. + */} + <StyledButton as={Comp} ref={ref} variant={variant} size={size} padding={padding} disabled={disabled} - readOnly={readOnly} + readOnly={readOnly} active={active} animating={isAnimating} animation={animation} - onClick={handleClick} + role={as === 'div' || as === 'label' ? 'button' : undefined} + tabIndex={disabled || readOnly ? -1 : undefined} + aria-disabled={disabled || readOnly || undefined} + onClick={disabled || readOnly ? undefined : handleClick} {...props} />
83-90: Prevent leaking styling props (incl. readOnly) to the DOM.
isPropValiddoesn’t filter tag-specific props;readOnlyandsizemay end up on a<button>/<div>. Explicitly exclude styling-only props.Apply this diff:
-const StyledButton = styled('button', { - shouldForwardProp: (prop) => isPropValid(prop), -})< +const FILTER_PROPS = new Set([ + 'variant', + 'size', + 'padding', + 'active', + 'animating', + 'animation', + 'readOnly', +]); + +const StyledButton = styled('button', { + shouldForwardProp: (prop) => isPropValid(prop) && !FILTER_PROPS.has(String(prop)), +})<
62-77: Stories and RTL tests for readOnly behavior not yet implemented—required before merge.The review requirements have not been met:
Missing readOnly stories: Button.stories.tsx lacks stories demonstrating disabled vs readOnly behavior across
asvariants (existing stories are Base, Variants, PseudoStates, Active, WithIcon, IconOnly, Sizes, Disabled, WithHref, Animated—none cover readOnly).Missing RTL tests: No test files exist for the Button component. RTL tests asserting onClick is prevented when readOnly or disabled are required.
onClick handler issue: The handleClick function (lines 42–45) does not check the readOnly or disabled state before invoking the callback, allowing onClick to fire even when readOnly is true. This must be guarded.
No tabIndex implementation: No tabIndex handling found in the component for focus behavior tests.
Add the missing stories and tests, and prevent onClick execution when
readOnlyordisabledis true.code/core/src/manager/components/sidebar/Tree.tsx (2)
321-331: Aria label inverted (Expand/Collapse) on root toggleLabel reads “Expand” when fully expanded and “Collapse” when not, which is reversed.
Apply:
- aria-label={isFullyExpanded ? 'Expand' : 'Collapse'} + aria-label={isFullyExpanded ? 'Collapse' : 'Expand'}
398-405: Guard PRELOAD_ENTRIES against empty childrenWhen a component/story has no children,
children[0]is undefined and will be emitted.Apply:
- onMouseEnter={() => { - if (item.type === 'component' || item.type === 'story') { - api.emit(PRELOAD_ENTRIES, { - ids: [children[0]], - options: { target: refId }, - }); - } - }} + onMouseEnter={() => { + if ((item.type === 'component' || item.type === 'story') && children.length > 0) { + api.emit(PRELOAD_ENTRIES, { + ids: [children[0]], + options: { target: refId }, + }); + } + }}code/core/src/components/components/Card/Card.stories.tsx (1)
8-10: Add React type-only import forReactNodeAvoid relying on a global
Reactnamespace in types.Apply:
-import preview from '../../../../../.storybook/preview'; -import { Card } from './Card'; +import preview from '../../../../../.storybook/preview'; +import { Card } from './Card'; +import type { ReactNode } from 'react'; -const Contents = ({ children }: { children: React.ReactNode }) => ( +const Contents = ({ children }: { children: ReactNode }) => ( <div style={{ padding: 16 }}>{children}</div> );
🧹 Nitpick comments (22)
code/core/src/components/components/Button/Button.tsx (5)
21-21: Ref type is too narrow for polymorphic “as”.When
as="a" | "div" | "label", the ref isn’t anHTMLButtonElement. Widen toHTMLElement(or adopt a proper polymorphic typing later).Apply this diff:
-export const Button = forwardRef<HTMLButtonElement, ButtonProps>( +export const Button = forwardRef<HTMLElement, ButtonProps>(
103-103: Cursor for readOnly should be ‘default’, not ‘inherit’.‘inherit’ can pick up odd parents. Use ‘default’ to signal non-interactive.
Apply this diff:
- cursor: disabled ? 'not-allowed' : readOnly ? 'inherit' : 'pointer', + cursor: disabled ? 'not-allowed' : readOnly ? 'default' : 'pointer',
184-187: Keep focus-visible even when readOnly, or remove from tab order.You gate
&:focusunder!readOnly, yet the element may still be focusable. Either always render a focus ring or settabIndex={-1}(proposed above) so it’s not focusable.Apply this diff to always allow the focus ring:
- ...(!readOnly && { - '&:hover': { + ...(!readOnly && { + '&:hover': { color: theme.barHoverColor, background: transparentize(0.86, theme.barHoverColor), }, - - '&:active': { + '&:active': { color: theme.barSelectedColor, background: transparentize(0.9, theme.barSelectedColor), }, - - '&:focus': { - boxShadow: `${rgba(theme.barHoverColor, 1)} 0 0 0 1px inset`, - outline: 'none', - }, }), + '&:focus': { + boxShadow: `${rgba(theme.barHoverColor, 1)} 0 0 0 1px inset`, + outline: 'none', + },- ...(!readOnly && { - '&:hover': { + ...(!readOnly && { + '&:hover': { /* ... */ }, - '&:active': { + '&:active': { /* ... */ }, - - '&:focus': { - boxShadow: `${rgba(theme.color.secondary, 1)} 0 0 0 1px inset`, - outline: 'none', - }, }), + '&:focus': { + boxShadow: `${rgba(theme.color.secondary, 1)} 0 0 0 1px inset`, + outline: 'none', + },Also applies to: 256-259
262-265: Don’t animate icons on readOnly.Skip SVG animation when non-interactive.
Apply this diff:
- animation: - animating && animation !== 'none' ? `${theme.animation[animation]} 1000ms ease-out` : '', + animation: + !readOnly && animating && animation !== 'none' + ? `${theme.animation[animation]} 1000ms ease-out` + : '',
8-19: Consider proper polymorphic typing foras+ element-specific props.With
as="a", consumers can’t passhref(not inButtonHTMLAttributes<HTMLButtonElement>). A discriminated union or a polymorphic helper type would improve DX and safety.If you want, I can provide a minimal
PolymorphicComponentProps<As, OwnProps>pattern.code/core/src/manager/components/TextFlip.stories.tsx (1)
10-10: Consider removing debug styling.The red border appears to be for development/debugging purposes. Consider removing it or making it conditional for Chromatic visual testing only.
Apply this diff if the border is not needed:
- <div style={{ display: 'inline-block', border: '1px solid red' }}> + <div style={{ display: 'inline-block' }}> <TextFlip {...args} /> </div>Or keep it only for visual testing:
- <div style={{ display: 'inline-block', border: '1px solid red' }}> + <div style={{ display: 'inline-block' }}> <TextFlip {...args} /> </div>code/core/src/manager/components/sidebar/Sidebar.tsx (1)
161-175: Use isDevelopment prop consistently for gatingYou compute
isDevelopmentalready; use it instead of referencingglobal.CONFIG_TYPEdirectly.Apply:
- {!isLoading && global.CONFIG_TYPE === 'DEVELOPMENT' && <ChecklistModule />} + {!isLoading && isDevelopment && <ChecklistModule />}code/core/src/manager/container/Menu.stories.tsx (1)
9-37: Ensure mock store init actually runs in stories
beforeEachon CSF meta may not execute in Storybook runtime. Prefer a decorator that seeds the store per render.Apply:
export default { component: TooltipLinkList, decorators: [ - (storyFn) => ( + (storyFn) => { + // Seed mock store before each story render + mockStore.setState({ + loaded: true, + muted: false, + completed: ['add-component'], + skipped: ['add-5-10-components'], + }); + return ( <div style={{ height: '300px', }} > <WithTooltip placement="top" startOpen tooltip={storyFn()}> <div>Tooltip</div> </WithTooltip> </div> - ), + ); + }, ], - beforeEach: async () => { - mockStore.setState({ - loaded: true, - muted: false, - completed: ['add-component'], - skipped: ['add-5-10-components'], - }); - }, excludeStories: ['links'], } satisfies Meta<typeof TooltipLinkList>;code/core/src/manager/settings/GuidePage.tsx (1)
; keep semantic action
50-63: Replace obsoleteUse CSS centering instead of
<center>. KeepLinkif you want inline style, but avoid obsolete tags.Apply:
+const FooterNote = styled.div({ + textAlign: 'center', +}); @@ - {checklist.muted ? ( - <center> + {checklist.muted ? ( + <FooterNote> Want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(false)}>Show in sidebar</Link> - </center> + </FooterNote> ) : ( - <center> + <FooterNote> Don't want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(checklist.allItems.map(({ id }) => id))}> Remove from sidebar </Link> - </center> + </FooterNote> )}code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
35-46: Consider renaming "Proxy" story to avoid shadowing global.The export name
Proxyshadows the JavaScript globalProxyobject. While this is a named export and less problematic than a local variable, it could still cause confusion.Consider renaming:
-export const Proxy = meta.story({ +export const ProxyExample = meta.story({ render: () => (code/core/src/manager/settings/index.tsx (2)
46-53: Remove meaningless key prop on TabButton.
key="id"is a literal string and not needed outside list rendering.Apply:
- key="id"
122-124: Gate the route with enableWhatsNew to avoid dangling navigation.The tab is hidden when
enableWhatsNewis false, but the route remains active. Keep them consistent.- <Route path="whats-new"> - <WhatsNewPage key="whats-new" /> - </Route> + {enableWhatsNew && ( + <Route path="whats-new"> + <WhatsNewPage key="whats-new" /> + </Route> + )}code/core/src/components/components/Card/Card.tsx (1)
52-56: Avoid leakingcolorprop to the DOM; use a non-DOM prop name.Using
coloras a custom prop risks forwarding it to the DOM. The diff above renames it tooutlineColorKey. Keep non-DOM prop names (prefixed or renamed) to avoid React warnings.Also applies to: 63-66
code/core/src/manager/container/Menu.tsx (1)
47-50: AvoidreadOnlyon a button; use a non-interactive container.Buttons without actions are confusing for a11y and keyboard users. The diff above swaps it for a non-interactive div with status semantics.
Also applies to: 91-96
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
55-70: Use isomorphic timeout type
NodeJS.Timeoutcan conflict in DOM builds. PreferReturnType<typeof setTimeout>.Apply:
- let timeout: NodeJS.Timeout; + let timeout: ReturnType<typeof setTimeout>;code/core/src/manager/settings/Checklist/Checklist.tsx (2)
269-285: Remove unusednodeRefto reduce allocations
nodeRefis created but never attached or read. Drop it and the related import.Apply:
-import React, { createRef, useMemo } from 'react'; +import React, { useEffect, useMemo } from 'react'; @@ type ChecklistItem = ChecklistData['sections'][number]['items'][number] & { isCompleted: boolean; isLockedBy: string[]; isSkipped: boolean; - nodeRef?: React.RefObject<HTMLLIElement>; }; @@ - const nodeRef = createRef<HTMLLIElement>(); - return [item.id, { ...item, isCompleted, isLockedBy, isSkipped, nodeRef }]; + return [item.id, { ...item, isCompleted, isLockedBy, isSkipped }];
333-336: Optional: reflect hash-target state in ListboxItemactivestylingPassing
activeimproves affordance when an item is hash-targeted.Apply:
- <ListboxItem key={item.id}> + <ListboxItem key={item.id} active={itemId === locationHash}>code/core/src/components/components/Collapsible/Collapsible.tsx (4)
25-26: Avoid requiringcollapsedvia type intersection.Intersecting with
ComponentProps<typeof CollapsibleContent>can accidentally makecollapsedrequired. UsingOmit<..., 'id' | 'collapsed' | 'aria-hidden'>(as in the diff above) prevents this and keepscollapsedoptional.Please build TS to ensure
Collapsedstory (which omitscollapsed) type-checks.
29-41: Remove duplicatearia-hidden.
aria-hiddenis set in bothCollapsibleandCollapsibleContent. Keep it in one place to avoid confusion (the diff above keeps it inCollapsibleContentonly).
78-84: Enhance a11y and flexibility oftoggleProps.Spreading
disabledon non-button elements is ineffective. Addaria-disabledalongsidedisabledso it’s useful for non-native controls.- const toggleProps = { - disabled, - onClick: toggleCollapsed, + const toggleProps = { + disabled, + 'aria-disabled': !!disabled, + onClick: toggleCollapsed, 'aria-controls': contentId, 'aria-expanded': !isCollapsed, } as const;
67-75: Reconsider defaultstopPropagation().Always stopping propagation can interfere with parent click handlers. Either make this behavior opt-in (e.g., a parameter) or document it clearly.
Confirm no parent components rely on click bubbling from summary controls.
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
39-41: Interaction sanity-check.
Toggledclicks “Close” (default open) which collapses content; good smoke test. Consider adding an assertion onaria-expandedfor robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/components/components/Button/Button.tsx(4 hunks)code/core/src/components/components/Card/Card.stories.tsx(1 hunks)code/core/src/components/components/Card/Card.tsx(4 hunks)code/core/src/components/components/Collapsible/Collapsible.stories.tsx(3 hunks)code/core/src/components/components/Collapsible/Collapsible.tsx(2 hunks)code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx(0 hunks)code/core/src/components/components/FocusProxy/FocusProxy.tsx(0 hunks)code/core/src/components/components/FocusRing/FocusRing.stories.tsx(1 hunks)code/core/src/components/components/FocusRing/FocusRing.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.stories.tsx(3 hunks)code/core/src/components/components/Listbox/Listbox.tsx(4 hunks)code/core/src/components/components/shared/useLocationHash.ts(1 hunks)code/core/src/components/index.ts(2 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager/components/TextFlip.stories.tsx(1 hunks)code/core/src/manager/components/TextFlip.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Explorer.tsx(0 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Menu.tsx(2 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(0 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(3 hunks)code/core/src/manager/components/sidebar/Sidebar.tsx(5 hunks)code/core/src/manager/components/sidebar/TestingModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Tree.tsx(1 hunks)code/core/src/manager/components/sidebar/TreeNode.tsx(1 hunks)code/core/src/manager/components/sidebar/useChecklist.ts(1 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/container/Menu.tsx(12 hunks)code/core/src/manager/container/Sidebar.tsx(2 hunks)code/core/src/manager/globals/exports.ts(3 hunks)code/core/src/manager/settings/Checklist/Checklist.stories.tsx(2 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(5 hunks)code/core/src/manager/settings/Checklist/checklistData.tsx(7 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)code/core/src/manager/settings/GuidePage.tsx(2 hunks)code/core/src/manager/settings/index.tsx(3 hunks)code/core/src/shared/checklist-store/index.ts(2 hunks)
💤 Files with no reviewable changes (4)
- code/core/src/components/components/FocusProxy/FocusProxy.tsx
- code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx
- code/core/src/manager/components/sidebar/Explorer.tsx
- code/core/src/manager/components/sidebar/SearchResults.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/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/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/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/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/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/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.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/ChecklistModule.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsxcode/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/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/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/TextFlip.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/container/Sidebar.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/cli/globalSettings.tscode/core/src/components/index.tscode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/core/src/manager/settings/GuidePage.stories.tsx
🧬 Code graph analysis (23)
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
ChecklistModule(67-205)
code/core/src/manager/components/TextFlip.stories.tsx (1)
code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(57-104)
code/core/src/components/components/shared/useLocationHash.ts (2)
code/renderers/web-components/template/components/Html.js (1)
globalThis(5-5)code/core/src/components/index.ts (1)
useLocationHash(96-96)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (3)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-243)code/core/src/manager/manager-stores.mock.ts (1)
checklistStore(57-57)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
code/core/src/components/components/FocusRing/FocusRing.tsx (3)
FocusRing(23-41)FocusProxy(17-21)FocusTarget(43-73)
code/core/src/manager/settings/GuidePage.tsx (2)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(12-57)code/core/src/manager/settings/Checklist/Checklist.tsx (1)
Checklist(243-446)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-243)
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(35-44)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (6)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
Collapsible(12-37)code/core/src/components/components/Listbox/Listbox.tsx (6)
ListboxButton(61-68)Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(57-104)code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/manager/components/sidebar/Menu.stories.tsx (1)
code/core/src/manager/container/Menu.tsx (1)
useMenu(59-257)
code/core/src/manager/components/sidebar/Menu.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (5)
Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/components/index.ts (5)
Listbox(48-48)ListboxItem(52-52)ListboxAction(49-49)ListboxIcon(51-51)ListboxText(53-53)
code/core/src/manager/components/TextFlip.tsx (1)
code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
code/core/src/components/index.ts (1)
Collapsible(44-44)
code/core/src/manager/settings/GuidePage.stories.tsx (3)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-65)code/core/src/manager-api/root.tsx (1)
ManagerContext(78-78)code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
Default(36-45)
code/core/src/manager/container/Menu.tsx (3)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(12-57)code/core/src/components/components/Listbox/Listbox.tsx (2)
ListboxButton(61-68)ListboxIcon(96-103)code/core/src/manager-api/modules/shortcuts.ts (1)
getAddonsShortcuts(187-189)
code/core/src/manager/settings/index.tsx (1)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-65)
code/core/src/manager/settings/Checklist/Checklist.tsx (6)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
ChecklistItem(8-8)useChecklist(12-57)code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)code/core/src/components/components/FocusRing/FocusRing.tsx (2)
FocusProxy(17-21)FocusTarget(43-73)code/core/src/components/components/Listbox/Listbox.tsx (1)
ListboxItem(17-59)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Actions(112-116)code/core/src/components/components/Button/Button.tsx (1)
Button(21-79)
code/core/src/components/components/Card/Card.stories.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(35-44)
code/core/src/manager/container/Sidebar.tsx (2)
code/core/src/manager/container/Menu.tsx (1)
useMenu(59-257)code/core/src/manager/manager-stores.ts (1)
experimental_useStatusStore(13-13)
code/core/src/components/components/Listbox/Listbox.tsx (1)
code/core/src/components/index.ts (5)
ListboxItem(52-52)ListboxAction(49-49)ListboxButton(50-50)ListboxText(53-53)ListboxIcon(51-51)
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
useCollapsible(58-93)
code/core/src/components/components/Listbox/Listbox.stories.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (4)
ListboxButton(61-68)ListboxItem(17-59)ListboxAction(70-79)ListboxText(81-94)code/core/src/components/index.ts (7)
ListboxButton(50-50)ListboxItem(52-52)ListboxAction(49-49)ProgressSpinner(92-92)Badge(28-28)Form(66-66)ListboxText(53-53)
🪛 Biome (2.1.2)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx
[error] 35-35: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
code/core/src/manager/components/sidebar/Menu.tsx
[error] 80-80: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
code/core/src/components/components/Collapsible/Collapsible.tsx
[error] 17-17: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
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] 59-59: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
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] 61-61: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
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] 67-67: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
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] 77-77: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
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)
code/core/src/manager/container/Sidebar.tsx
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (36)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
21-29: LGTM! Theme-aware styling improvement.The refactoring to use a function-based style with the theme parameter enables dynamic shadow rendering that adapts to the theme. The CSS variable
--card-box-shadowallows child components to access the computed shadow value.code/core/src/cli/globalSettings.ts (1)
18-24: Union type formutedfield is correctly handled with proper type narrowing.The schema extension is sound. The
mutedfield's union type (boolean | string[]) is being properly consumed throughout the codebase:
- The store's
mutefunction usesArray.isArray(value)to distinguish between boolean (blanket muting) and array (selective item muting), properly merging array values with existing state- The UI component uses
Array.isArray(muted)before accessing array methods likeincludes()- Initialization defaults to
falsewhen the value is undefinedThe dual-use pattern (boolean for whole-checklist muting, array for selective item muting) is intentional and correctly implemented with type guards.
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
5-6: LGTM: Checklist store integration follows established patterns.The addition of
universalChecklistStoreinitialization in the globalbeforeEachhook and the import ofglobalfor production-mode testing are both consistent with the broader checklist integration across the codebase.Also applies to: 13-13, 107-112
158-164: LGTM: Proper cleanup of global state modification.The production-mode simulation correctly mutates
global.CONFIG_TYPEand restores the original value via the cleanup function returned frombeforeEach. This ensures test isolation.code/core/src/manager/components/TextFlip.tsx (1)
1-104: LGTM: Well-implemented text flip animation component.The component correctly uses
react-transition-groupto animate between text values, with proper support for:
- Direction detection via
localeComparewith numeric sorting- Accessibility via
aria-hiddenon stale content- Reduced motion preferences
- Ref forwarding to avoid findDOMNode warnings
The dual-Transition pattern (one for entering, one for exiting) is the correct approach for simultaneous enter/exit animations.
code/core/src/manager/components/sidebar/TreeNode.tsx (1)
91-93: LGTM: Styling adjustment for first tree node.The CSS rule removes the top margin from the first
RootNode, which aligns with the layout changes for the onboarding guide integration.code/core/src/manager/settings/GuidePage.stories.tsx (1)
1-36: LGTM: Story setup follows established patterns.The GuidePage story properly mocks the ManagerContext and initializes the checklist store state, consistent with other checklist-related story files in the codebase.
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
24-31: LGTM: Complete checklist store initialization.The addition of
loadedandmutedfields aligns with the expanded checklist store schema and is consistent with other story files in this PR.code/core/src/core-server/utils/checklist.ts (1)
17-22: LGTM: Complete checklist store initialization with new fields.The addition of
loaded: trueandmuted(with proper fallback) extends the store state to match the updated schema. The immediate setting ofloaded: trueis appropriate since settings are loaded synchronously from the settings file.code/core/src/manager/globals/exports.ts (1)
1-3: LGTM: Public API expanded with new components and utilities.The additions of
FocusOutline,FocusRing,FocusTarget,ListboxIcon, anduseLocationHashto the public exports are correctly present in the generated file. These exports are automatically discovered since they are exported fromcode/core/src/components/index.ts, which is bundled as part of the auto-generation process incode/core/scripts/generate-source-files.ts. No manual generator configuration changes are required—the mechanism is fully automatic.code/core/src/manager/settings/GuidePage.tsx (1)
37-50: Nice: unified state via useChecklistSwitching to
useChecklistkeeps Guide and Sidebar in sync. Looks good.code/core/src/components/index.ts (1)
46-54: Public API expansion looks goodExporting Focus components, ListboxIcon, and useLocationHash aligns with usage across manager UI. No issues spotted.
Also applies to: 96-97
code/core/src/manager/components/sidebar/Tree.tsx (1)
716-720: Stale hasOrphans references resolved; spacing regression requires manual verification
- ✓ No stale
hasOrphansprop references found in codebase- ✓ Orphan hoisting logic and
isOrphanprop remain intact- ⚠ Cannot verify whether styled Container had conditional orphan margin or if visual regression occurs — the original Container definition is not accessible. Manual testing of sidebar spacing with hoisted orphans is required to confirm no regression.
code/core/src/components/components/shared/useLocationHash.ts (1)
37-41: LGTM!The hook correctly subscribes to the hash monitor, handles cleanup, and returns the hash without the leading
#character.code/core/src/manager/components/sidebar/Menu.tsx (1)
4-12: LGTM!The new Listbox-related imports are correctly added and align with the refactored menu rendering.
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (2)
3-3: LGTM!The checklistStore import is correctly added and used in the story configuration.
36-44: LGTM!The story args correctly merge checklist data and store, with explicit state fields for testing different scenarios.
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (3)
20-33: LGTM!The Active and Temporary stories correctly demonstrate the FocusRing component with different duration behaviors.
48-64: LGTM!The Target story correctly demonstrates hash-based focus behavior with appropriate timing for the play function.
65-67: LGTM!The beforeEach hook correctly resets the hash state between story runs to ensure consistent test behavior.
code/core/src/manager/components/sidebar/Menu.stories.tsx (2)
13-13: LGTM!The mock store setup correctly initializes checklist state for consistent story behavior across all tests.
Also applies to: 32-39
66-79: LGTM!All three stories correctly updated to use the new useMenu object-based signature, consistently passing the required properties.
Also applies to: 108-134, 157-170
code/core/src/manager/container/Sidebar.tsx (2)
36-53: LGTM!The mapper refactor correctly exposes the necessary properties for menu construction while maintaining clean separation of concerns.
57-73: LGTM!The Consumer render correctly computes the menu using the new useMenu signature and passes all required props to SidebarComponent. The Biome warning about hook usage is a false positive—the Consumer render prop is effectively the component body, and hook usage here is valid.
code/core/src/manager/components/sidebar/useChecklist.ts (3)
8-10: LGTM!The type extraction and allItems computation are clean and efficient.
12-27: LGTM!The hook setup correctly handles checklist state with proper memoization. The
Array.isArray(muted)check on line 24 defensively handles cases wheremutedmight have different types.
29-56: LGTM with consideration for property spreading.The computed values correctly implement the round-robin selection logic for next items and progress calculation. The return statement spreads multiple objects (
checklistData,checklistStore,state) which could potentially cause property name collisions, but this appears intentional for the consumer API.code/core/src/components/components/Listbox/Listbox.stories.tsx (4)
3-3: LGTM!The Badge and ProgressSpinner imports are correctly added and used in the story examples.
20-22: LGTM!The aria-label addition improves accessibility for the icon-only button.
35-49: LGTM!The new story items effectively demonstrate integration with Badge and ProgressSpinner components.
50-62: LGTM!The active prop correctly moved to ListboxItem and the checkbox label text is properly wrapped in ListboxText for consistent styling.
code/core/src/components/components/Listbox/Listbox.tsx (1)
12-15: Widened sibling selector may cause unintended bordersChanging to '& + *' affects any following sibling, not just another Listbox. If the intent was only stacked Listboxes, revert to a narrower selector.
Apply if appropriate:
- '& + *': { + '& + &': { borderTop: `1px solid ${theme.appBorderColor}`, },code/core/src/components/components/FocusRing/FocusRing.tsx (1)
17-21: Confirm :has() support or add a non-:has fallback
&:has(#id:focus-visible)is great but not universally reliable in older engines. Consider a fallback (e.g., also styling when the proxy itself is focus-visible).code/core/src/manager/settings/Checklist/checklistData.tsx (1)
14-20: Structured, content‑rich checklist with sequencing looks goodAction objects,
afterdependencies, and JSX content improve UX and clarity. Ensure the consumer invokespredicatefunctions; otherwise these items won’t auto-complete. See my Checklist.tsx comment for a concrete fix.Also applies to: 24-41, 45-75, 83-104, 106-125, 127-148, 150-172, 174-195, 203-214, 216-227, 229-239
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (2)
7-14: Stories align with new API; type-only import is correct.Use of
togglePropsandReturnType<typeof useCollapsible>is tidy and keeps runtime bundle clean.
43-54: Controlled story looks good.Local state toggles and play action validate the controlled path.
Run:
code/core/src/components/components/Collapsible/Collapsible.tsx
Outdated
Show resolved
Hide resolved
yannbf
added
ci:merged
There was a problem hiding this comment.
Actionable comments posted: 5
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/components/components/Button/Button.tsx (1)
41-74: Guard clicks whenreadOnlyis true.
readOnlycurrently only changes styles; the handler still fires, so consumers can’t rely on it to freeze actions. Bail out (and suppress animation) whenreadOnlyis set to prevent unintended side effects.- const handleClick: ButtonProps['onClick'] = (event) => { - if (onClick) { - onClick(event); - } + const handleClick: ButtonProps['onClick'] = (event) => { + if (disabled || readOnly) { + event.preventDefault(); + return; + } + onClick?.(event);
♻️ Duplicate comments (8)
code/core/src/components/components/Collapsible/Collapsible.tsx (2)
62-97: Controlled/uncontrolled behavior remains unclear.The hook still mixes controlled and uncontrolled patterns:
useEffectsyncs whencollapsedchanges (suggesting controlled), buttoggleCollapseddirectly mutates local state even whencollapsedis provided (suggesting uncontrolled). This hybrid approach can lead to confusing behavior where prop updates and local toggles compete.Past review recommended choosing one of:
- Fully controlled: Accept
collapsed+onCollapsedChange, remove local state- Fully uncontrolled: Use
defaultCollapsedfor initial value only, remove sync effect- Controllable: Support both via standard
useControllableStatepatternThe current implementation may work for this PR's scope, but consider clarifying the intended pattern before broader adoption.
32-35: Prevent prop collisions and remove duplicatearia-hidden.Spreading
{...props}beforeid,collapsed, andaria-hiddenallows callers to override these critical props, which could break the component's controlled behavior. Additionally,aria-hiddenis set both here (line 35) and inCollapsibleContent(line 44), creating a redundant duplicate.The past review suggested using
Omit<ComponentProps<typeof CollapsibleContent>, 'id' | 'collapsed' | 'aria-hidden'>in the type definition (line 25) and removing the duplicatearia-hiddenhere.Apply this diff to prevent collisions and remove the duplicate:
} & ComponentProps<typeof CollapsibleContent>) => { +} & Omit<ComponentProps<typeof CollapsibleContent>, 'id' | 'collapsed' | 'aria-hidden'>) => { const internalState = useCollapsible(collapsed, disabled); - const state = providedState || internalState; + const state = providedState ?? internalState; return ( <> {typeof summary === 'function' ? summary(state) : summary} <CollapsibleContent {...props} id={state.contentId} collapsed={state.isCollapsed} - aria-hidden={state.isCollapsed} > {typeof children === 'function' ? children(state) : children} </CollapsibleContent>code/core/src/components/components/FocusRing/FocusRing.tsx (1)
34-38: Guard timeout creation whenhighlightDurationis undefined.This issue was flagged in a previous review but remains unresolved. Currently, a timeout is created even when
highlightDurationis undefined, causingsetTimeoutto schedule with a 0ms delay. This creates unnecessary overhead and unexpected behavior.Apply this diff to guard the timeout:
useEffect(() => { setVisible(active); - const timeout = setTimeout(setVisible, highlightDuration, false); - return () => clearTimeout(timeout); + if (typeof highlightDuration === 'number' && highlightDuration > 0) { + const timeout = setTimeout(() => setVisible(false), highlightDuration); + return () => clearTimeout(timeout); + } }, [active, highlightDuration]);code/core/src/components/components/Button/Button.tsx (1)
9-10: Don’t adddivtoaswithout button semantics.Rendering the control as a
<div>means it no longer has a native button role or keyboard activation (Enter/Space), which breaks WCAG 2.1.1. Please either drop'div'from the union or normalize the props so non-button hosts getrole="button",tabIndex=0, and key handlers (and remove them when the control is non-interactive). (bocoup.github.io)@@ const Comp = asChild ? Slot : as; + const isNativeButtonHost = Comp === 'button' || Comp === Slot; + const interactive = !disabled && !readOnly; @@ - <StyledButton + <StyledButton as={Comp} ref={ref} @@ onClick={handleClick} + {...(!isNativeButtonHost && + interactive && { + role: props.role ?? 'button', + tabIndex: props.tabIndex ?? 0, + onKeyDown: (event) => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + handleClick?.(event); + } + }, + })} + {...(!isNativeButtonHost && !interactive && { + role: props.role, + tabIndex: props.tabIndex ?? -1, + })} {...props} />code/core/src/manager/components/sidebar/ChecklistModule.tsx (2)
85-171: Fix invalid nested list semantics (li inside li).Lines 85-98 nest
ListboxItemcomponents inside anotherListboxItem. SinceListboxItemrenders<li>elements (per the relevant code snippets), this creates invalid HTML and breaks accessibility. This issue was flagged in a previous review and remains unaddressed.Flatten the structure so each
ListboxItemcorresponds to a single<li>. Use non-list containers (e.g.,divor wrapper elements) for inner layout grouping.Also applies to: 127-156
67-91: Add production guard for dev-only checklist module.The module links to
/settings/guide(lines 91, 132, 180), which is a dev-only route according to the previous review. The module should either be hidden in production or link to an alternative destination. This issue was flagged previously and remains unaddressed.Apply this diff to hide the module in production:
+import { global } from '@storybook/global'; + export const ChecklistModule = () => { const api = useStorybookApi(); const { loaded, allItems, nextItems, progress, mute } = useChecklist(); + + if (global.CONFIG_TYPE !== 'DEVELOPMENT') return null;Also applies to: 132-134, 180-180
code/core/src/shared/checklist-store/index.ts (1)
6-12: Clarifymutedsemantics; boolean vs list leads to inconsistent behavior.The union type
boolean | Array<TaskId>formutedcreates ambiguity. Based on theuseChecklist.isReadyimplementation (which only checksArray.isArray(muted)), a booleantruewon't suppress items as intended. Consider using a discriminated union or separate flags (mutedAll: booleanandmutedIds: TaskId[]) to make the intent explicit and ensure all consumers handle both cases correctly.code/core/src/components/components/Listbox/Listbox.tsx (1)
17-27: Preventactiveprop from leaking to the DOM.The
activeprop is style-only but will be forwarded to the<div>element, causing React DOM attribute warnings. UseshouldForwardPropto filter it out.Apply this diff:
-export const ListboxItem = styled.div<{ active?: boolean }>(({ active, theme }) => ({ +export const ListboxItem = styled('div', { + shouldForwardProp: (prop) => prop !== 'active', +})<{ active?: boolean }>(({ active, theme }) => ({ display: 'flex', alignItems: 'center',
🧹 Nitpick comments (5)
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
55-55: Consider using explicitnumbertype for browser context.
ReturnType<typeof setTimeout>may resolve toNodeJS.Timeoutin some TypeScript configurations. Since this is browser-only code, explicitly typing asnumber | undefinedis clearer and avoids Node.js type references.Apply this diff:
- let timeout: ReturnType<typeof setTimeout>; + let timeout: number | undefined;As per a past review comment by valentinpalkovic.
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
38-42: Clarify prop spreading order and potential conflicts.The story spreads both
checklistDataandchecklistStore, then adds explicitaccepted,done, andskippedfields. IfchecklistStorealready contains these fields, the explicit values will override them. Verify this is the intended behavior for story controls.code/core/src/manager/components/sidebar/useChecklist.ts (1)
42-70: Consider including callback dependencies instead of the callback itself.The dependency array includes
isOpen, which is a callback function. Typically, you should include the dependencies of the callback (accepted,done,skipped) rather than the function itself. While React will detect the function reference change when its dependencies change, it's more explicit to include the actual dependencies.Apply this diff:
- }, [api, index, loaded, allItems, isOpen]); + }, [api, index, loaded, allItems, accepted, done, skipped]);code/core/src/manager/settings/Checklist/Checklist.tsx (1)
276-276: Remove unusednodeRefcreation.The
nodeRefis created for each item but never consumed in the component rendering logic. Creating refs inuseMemois also problematic as they'll be recreated whenever dependencies change, breaking ref stability.Apply this diff to remove the unused code:
const isLockedBy = item.after?.filter((id) => !accepted.includes(id) && !done.includes(id)) ?? []; const isSkipped = skipped.includes(item.id); - const nodeRef = createRef<HTMLLIElement>(); - return [item.id, { ...item, isAccepted, isDone, isLockedBy, isSkipped, nodeRef }]; + return [item.id, { ...item, isAccepted, isDone, isLockedBy, isSkipped }];And update the type definition:
type ChecklistItem = ChecklistData['sections'][number]['items'][number] & { isAccepted: boolean; isDone: boolean; isLockedBy: string[]; isSkipped: boolean; - nodeRef?: React.RefObject<HTMLLIElement>; };code/core/src/manager/container/Menu.tsx (1)
92-96: Consider using a non-interactive element for progress display.While
as="div"changes the element type, thereadOnlyprop is not a valid div attribute andListboxButtonstill applies button-like styling. For a truly non-interactive progress indicator, consider using a plain styled element or add appropriate ARIA attributes.Replace with a semantic status indicator:
- right: progress < 100 && ( - <ListboxButton as="div" readOnly padding="none"> - <ProgressCircle percentage={progress} running={false} size={16} width={1.5} /> - {progress}% - </ListboxButton> - ), + right: + progress < 100 && ( + <div + role="status" + aria-label={`Onboarding progress ${progress}%`} + style={{ display: 'flex', alignItems: 'center', gap: 4, padding: '4px 0' }} + > + <ProgressCircle percentage={progress} running={false} size={16} width={1.5} /> + {progress}% + </div> + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/components/components/Button/Button.tsx(5 hunks)code/core/src/components/components/Collapsible/Collapsible.tsx(3 hunks)code/core/src/components/components/FocusRing/FocusRing.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.stories.tsx(3 hunks)code/core/src/components/components/Listbox/Listbox.tsx(4 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager/components/TextFlip.stories.tsx(1 hunks)code/core/src/manager/components/TextFlip.tsx(1 hunks)code/core/src/manager/components/preview/tools/zoom.tsx(0 hunks)code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Menu.tsx(2 hunks)code/core/src/manager/components/sidebar/RefBlocks.tsx(0 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(0 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(3 hunks)code/core/src/manager/components/sidebar/useChecklist.ts(1 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/container/Menu.tsx(12 hunks)code/core/src/manager/settings/Checklist/Checklist.stories.tsx(2 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(6 hunks)code/core/src/manager/settings/Checklist/checklistData.tsx(8 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)code/core/src/shared/checklist-store/index.ts(1 hunks)
💤 Files with no reviewable changes (3)
- code/core/src/manager/components/sidebar/SearchResults.tsx
- code/core/src/manager/components/sidebar/RefBlocks.tsx
- code/core/src/manager/components/preview/tools/zoom.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- code/core/src/manager/components/sidebar/Menu.tsx
- code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
- code/core/src/cli/globalSettings.ts
- code/core/src/manager/components/TextFlip.tsx
- code/core/src/manager/settings/GuidePage.stories.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/components/sidebar/useChecklist.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/container/Menu.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/TextFlip.stories.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/components/sidebar/useChecklist.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/container/Menu.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/TextFlip.stories.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/components/sidebar/useChecklist.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/container/Menu.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/TextFlip.stories.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/useChecklist.tscode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/core-server/utils/checklist.tscode/core/src/manager/container/Menu.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/TextFlip.stories.tsx
🧠 Learnings (19)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.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/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/TextFlip.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should be placed in beforeEach blocks
Applied to files:
code/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/.storybook/** : Place internal UI Storybook configuration in `code/.storybook/` and maintain it there
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.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/ChecklistModule.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/manager/container/Menu.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/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.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/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : In application code, use Storybook loggers instead of `console.*` (client code: `storybook/internal/client-logger`; server code: `storybook/internal/node-logger`)
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Applied to files:
code/core/src/manager/settings/Checklist/checklistData.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/core/src/manager/container/Menu.stories.tsx
🧬 Code graph analysis (12)
code/core/src/manager/components/sidebar/useChecklist.ts (3)
code/core/src/manager/settings/Checklist/checklistData.tsx (2)
ChecklistData(8-30)checklistData(32-268)code/core/src/manager-api/root.tsx (2)
useStorybookApi(294-297)useStorybookState(290-293)code/core/src/manager/components/sidebar/mockdata.large.ts (1)
index(12-26726)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (3)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(32-268)code/core/src/manager/manager-stores.mock.ts (1)
checklistStore(57-57)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (6)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
Collapsible(12-41)code/core/src/components/components/Listbox/Listbox.tsx (6)
ListboxButton(61-68)Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(63-100)code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
code/core/src/manager-api/root.tsx (1)
API(94-107)
code/core/src/manager/components/sidebar/Menu.stories.tsx (1)
code/core/src/manager/container/Menu.tsx (1)
useMenu(60-262)
code/core/src/components/components/Listbox/Listbox.tsx (1)
code/core/src/components/index.ts (6)
Listbox(48-48)ListboxItem(52-52)ListboxAction(49-49)ListboxButton(50-50)ListboxText(53-53)ListboxIcon(51-51)
code/core/src/components/components/FocusRing/FocusRing.tsx (2)
code/core/src/components/index.ts (5)
FocusOutline(46-46)FocusProxy(46-46)FocusRing(46-46)FocusTarget(46-46)useLocationHash(96-96)code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)
code/core/src/core-server/utils/checklist.ts (1)
code/core/src/manager-api/tests/url.test.js (1)
store(143-151)
code/core/src/manager/settings/Checklist/Checklist.tsx (6)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
ChecklistData(8-30)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)code/core/src/components/components/FocusRing/FocusRing.tsx (2)
FocusProxy(17-21)FocusTarget(43-73)code/core/src/components/components/Listbox/Listbox.tsx (1)
ListboxItem(17-59)code/core/src/components/components/Button/Button.tsx (1)
Button(20-78)
code/core/src/components/components/Listbox/Listbox.stories.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (4)
ListboxButton(61-68)ListboxItem(17-59)ListboxAction(70-79)ListboxText(81-94)code/core/src/components/index.ts (7)
ListboxButton(50-50)ListboxItem(52-52)ListboxAction(49-49)ProgressSpinner(92-92)Badge(28-28)Form(66-66)ListboxText(53-53)
code/core/src/manager/container/Menu.tsx (3)
code/core/src/manager-api/root.tsx (1)
API(94-107)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/components/components/Listbox/Listbox.tsx (2)
ListboxButton(61-68)ListboxIcon(96-103)
code/core/src/manager/components/TextFlip.stories.tsx (1)
code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(63-100)
🔇 Additional comments (18)
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
107-113: Well-structured test initialization.The checklist store initialization ensures all stories start with a consistent, known state. The setup is comprehensive and follows the established pattern for state management in this test file.
159-165: Excellent cleanup pattern for production mode simulation.The production mode simulation properly captures and restores the original
CONFIG_TYPEvalue, ensuring test isolation and preventing state pollution across stories.code/core/src/components/components/Collapsible/Collapsible.tsx (2)
1-8: LGTM: Imports support new functionality.The addition of
SyntheticEventanduseCallbackappropriately supports the updated event handling and memoization intoggleCollapsed.
47-60: LGTM: Optional collapsed prop with sensible defaults.The typed optional
collapsedprop with a default value and the CSS transitions (includingprefers-reduced-motionsupport) are well-implemented.code/core/src/components/components/FocusRing/FocusRing.tsx (1)
17-21::has()selector is acceptable for Storybook core components.Storybook targets modern browsers by default, and
:has()is supported in current major browsers (Chrome/Edge v105+, Safari 15.4+, Firefox v121+). SinceFocusRingis part of Storybook's internal UI, no additional compatibility measures are needed.code/core/src/manager/container/Menu.stories.tsx (1)
29-37: LGTM! Mock setup follows best practices.The
beforeEachhook properly initializes the mock store state with all required fields. This follows the learned pattern of placing mock implementations inbeforeEachblocks.Based on learnings
code/core/src/manager/components/sidebar/Menu.stories.tsx (2)
67-80: LGTM! useMenu API migration is consistent.The refactored
useMenuhook now accepts a single destructured object parameter instead of multiple arguments. All required parameters (api,showToolbar,isPanelShown,isNavShown,enableShortcuts) are provided correctly across all stories.Also applies to: 109-135, 158-171
32-40: LGTM! Mock setup follows best practices.The
beforeEachhook properly initializes the mock store state, following the learned pattern for mock implementations.Based on learnings
code/core/src/components/components/Listbox/Listbox.stories.tsx (3)
20-22: LGTM! Accessibility improvement.Adding
aria-label="Options"to the ellipsis button improves screen reader support for icon-only buttons.
35-49: LGTM! New story items demonstrate expanded component capabilities.The new items showcase inline buttons with progress indicators and badges, properly demonstrating the extended public API surface of Listbox components.
56-62: LGTM! Active state refactor aligns with component updates.Moving the
activeprop fromListboxActiontoListboxItemis consistent with the component API changes visible in the relevant code snippets, whereListboxItemnow accepts and styles theactiveprop.code/core/src/manager/components/sidebar/useChecklist.ts (3)
79-86: LGTM! Round-robin next items selection is well-implemented.The logic to select up to 3 ready items in a round-robin fashion across sections is clear and correct. It ensures users can "choose their own adventure" across different sections.
55-68: LGTM! Subscription management handles cleanup correctly.The effect properly subscribes when items become open and cleans up subscriptions (if returned) when items are no longer open. The subscription return type
void | (() => void)is handled correctly.
29-35: No clarification needed—the type is already consistent and properly handled.The
mutedfield type is already defined asboolean | Array<TaskId>in the store (code/core/src/shared/checklist-store/index.ts line 8), which is exactly the union type the review comment suggests should exist. The initialization tofalsealigns with the boolean part of the union, and the code properly guards withArray.isArray(muted)before calling.includes(). The update logic and CLI schema consistently support both boolean and array values. The implementation is correct as-is.Likely an incorrect or invalid review comment.
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
71-74: LGTM! nextItems memo properly creates node refs for transitions.The memoized
nextarray correctly attaches acreateRefto each next item for use in theTransitionGroup, enabling proper transition animations.code/core/src/manager/settings/Checklist/checklistData.tsx (2)
8-30: LGTM! ChecklistData interface is well-structured.The interface properly defines the hierarchical checklist data model with sections and items. The optional fields (
after,content,action,subscribe) provide flexibility, and thesubscribefunction signature correctly allows optional cleanup by returningvoid | (() => void).
41-44: LGTM! Action-based flow is well-designed.The
actionfield withonClickreceiving bothapiandacceptcallback enables flexible item completion flows with user interaction.code/core/src/core-server/utils/checklist.ts (1)
20-22: Verify if thedonefield should be persisted—current implementation appears intentional but warrants confirmation.Based on code analysis, the
donefield is deliberately not being persisted: it's neither loaded from settings during initialization nor saved to settings on state changes. Additionally, theresetaction excludesdonefrom its cleanup (removing only fromacceptedandskipped), suggesting it may have different semantics than the other persistent fields.However, the asymmetry is worth confirming:
acceptanddoneboth track task completion in similar ways, yet onlyacceptis persisted. This could indicatedoneis intentionally session-only state, or it could be an oversight where both should be persisted together.
| state?: ReturnType<typeof useCollapsible>; | ||
| } & ComponentProps<typeof CollapsibleContent>) => { | ||
| const internalState = useCollapsible(collapsed, disabled); | ||
| const state = providedState || internalState; |
There was a problem hiding this comment.
Use nullish coalescing (??) instead of logical OR (||).
Line 27 uses || which will fall back to internalState if providedState is any falsy value (including 0, false, ''). Since state objects could theoretically have falsy properties that are valid, use the nullish coalescing operator ?? to only fall back when providedState is null or undefined.
Apply this diff:
- const state = providedState || internalState;
+ const state = providedState ?? internalState;🤖 Prompt for AI Agents
In code/core/src/components/components/Collapsible/Collapsible.tsx around line
27, the assignment uses logical OR which treats any falsy providedState (0,
false, '', etc.) as absent; change the fallback to use nullish coalescing so
providedState is only ignored when it is null or undefined (replace the "||"
usage with "??" so state = providedState ?? internalState).
| useEffect(() => { | ||
| let timeout: ReturnType<typeof setTimeout>; | ||
|
|
||
| setActive(false); | ||
| if (locationHash === targetHash) { | ||
| setTimeout(() => { | ||
| setActive(true); | ||
| nodeRef.current?.focus({ preventScroll: true }); | ||
| nodeRef.current?.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| }, 0); | ||
| if (highlightDuration) { | ||
| timeout = setTimeout(setActive, highlightDuration, false); | ||
| } | ||
| } | ||
|
|
||
| return () => clearTimeout(timeout); | ||
| }, [locationHash, targetHash, highlightDuration]); |
There was a problem hiding this comment.
Critical: Capture setTimeout result to prevent memory leak.
Line 59 calls setTimeout but doesn't capture the returned timeout ID. This prevents cleanup and causes a memory leak, as the timeout cannot be cleared when the effect re-runs or the component unmounts.
Apply this diff:
useEffect(() => {
let timeout: ReturnType<typeof setTimeout>;
setActive(false);
if (locationHash === targetHash) {
- setTimeout(() => {
+ const immediateTimeout = setTimeout(() => {
setActive(true);
nodeRef.current?.focus({ preventScroll: true });
nodeRef.current?.scrollIntoView({ behavior: 'smooth', block: 'center' });
}, 0);
if (highlightDuration) {
timeout = setTimeout(setActive, highlightDuration, false);
}
+ return () => {
+ clearTimeout(immediateTimeout);
+ clearTimeout(timeout);
+ };
}
- return () => clearTimeout(timeout);
}, [locationHash, targetHash, highlightDuration]);📝 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.
| useEffect(() => { | |
| let timeout: ReturnType<typeof setTimeout>; | |
| setActive(false); | |
| if (locationHash === targetHash) { | |
| setTimeout(() => { | |
| setActive(true); | |
| nodeRef.current?.focus({ preventScroll: true }); | |
| nodeRef.current?.scrollIntoView({ behavior: 'smooth', block: 'center' }); | |
| }, 0); | |
| if (highlightDuration) { | |
| timeout = setTimeout(setActive, highlightDuration, false); | |
| } | |
| } | |
| return () => clearTimeout(timeout); | |
| }, [locationHash, targetHash, highlightDuration]); | |
| useEffect(() => { | |
| let timeout: ReturnType<typeof setTimeout>; | |
| let immediateTimeout: ReturnType<typeof setTimeout>; | |
| setActive(false); | |
| if (locationHash === targetHash) { | |
| immediateTimeout = setTimeout(() => { | |
| setActive(true); | |
| nodeRef.current?.focus({ preventScroll: true }); | |
| nodeRef.current?.scrollIntoView({ behavior: 'smooth', block: 'center' }); | |
| }, 0); | |
| if (highlightDuration) { | |
| timeout = setTimeout(setActive, highlightDuration, false); | |
| } | |
| } | |
| return () => { | |
| clearTimeout(immediateTimeout); | |
| clearTimeout(timeout); | |
| }; | |
| }, [locationHash, targetHash, highlightDuration]); |
🤖 Prompt for AI Agents
In code/core/src/components/components/FocusRing/FocusRing.tsx around lines 54
to 70, the immediate setTimeout call is not capturing its return value so it
cannot be cleared on cleanup; capture that timeout ID (e.g., assign the result
to the existing timeout variable), also ensure the timeout variable is declared
with appropriate type (ReturnType<typeof setTimeout>), and clear both timeouts
in the effect cleanup (clearTimeout(timeout) and clearTimeout(theCapturedId) or
clear the single variable if you store both in it) so no dangling timers remain
when the effect re-runs or the component unmounts.
| </p> | ||
| </> | ||
| ), | ||
| // subscribe: ({ done }) => done(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented code.
The commented-out subscribe line should either be implemented or removed to maintain code cleanliness.
- // subscribe: ({ done }) => done(),📝 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.
| // subscribe: ({ done }) => done(), |
🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/checklistData.tsx around line 66,
there's a commented-out line "// subscribe: ({ done }) => done()," that should
be removed (or implemented if subscription behavior is needed); remove the
commented code to clean up the file, or replace it with a proper subscribe
implementation consistent with the surrounding API and ensure any required
imports/types are added when implementing.
| skip: (id: TaskId) => { | ||
| universalStore.setState((state) => ({ | ||
| completed: state.completed.filter((v) => v !== id), | ||
| ...state, | ||
| accepted: state.accepted.filter((v) => v !== id), | ||
| skipped: state.skipped.includes(id) ? state.skipped : [...state.skipped, id], | ||
| })); | ||
| }, |
There was a problem hiding this comment.
Consider removing from done array when skipping.
The skip method removes the item from accepted but not from done. If an item can be automatically marked as done via subscribe, skipping it should likely clear that state as well for consistency.
Apply this diff if items should be cleared from done when skipped:
skip: (id: TaskId) => {
universalStore.setState((state) => ({
...state,
accepted: state.accepted.filter((v) => v !== id),
+ done: state.done.filter((v) => v !== id),
skipped: state.skipped.includes(id) ? state.skipped : [...state.skipped, id],
}));
},📝 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.
| skip: (id: TaskId) => { | |
| universalStore.setState((state) => ({ | |
| completed: state.completed.filter((v) => v !== id), | |
| ...state, | |
| accepted: state.accepted.filter((v) => v !== id), | |
| skipped: state.skipped.includes(id) ? state.skipped : [...state.skipped, id], | |
| })); | |
| }, | |
| skip: (id: TaskId) => { | |
| universalStore.setState((state) => ({ | |
| ...state, | |
| accepted: state.accepted.filter((v) => v !== id), | |
| done: state.done.filter((v) => v !== id), | |
| skipped: state.skipped.includes(id) ? state.skipped : [...state.skipped, id], | |
| })); | |
| }, |
🤖 Prompt for AI Agents
In code/core/src/shared/checklist-store/index.ts around lines 59 to 65, the skip
handler removes the id from accepted but leaves it in done; update the state
mutation to also remove the id from done (e.g., set done: state.done.filter(d =>
d !== id)) so skipped items are cleared from the done array for consistency when
an item is skipped.
| reset: (id: TaskId) => { | ||
| universalStore.setState((state) => ({ | ||
| completed: state.completed.filter((v) => v !== id), | ||
| ...state, | ||
| accepted: state.accepted.filter((v) => v !== id), | ||
| skipped: state.skipped.filter((v) => v !== id), | ||
| })); | ||
| }, |
There was a problem hiding this comment.
Consider clearing done state on reset.
The reset method clears accepted and skipped but leaves done intact. If reset is meant to return an item to its initial state (as suggested by the "Undo" button in Checklist.tsx), it should also clear the done array.
Apply this diff to fully reset item state:
reset: (id: TaskId) => {
universalStore.setState((state) => ({
...state,
accepted: state.accepted.filter((v) => v !== id),
+ done: state.done.filter((v) => v !== id),
skipped: state.skipped.filter((v) => v !== id),
}));
},📝 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.
| reset: (id: TaskId) => { | |
| universalStore.setState((state) => ({ | |
| completed: state.completed.filter((v) => v !== id), | |
| ...state, | |
| accepted: state.accepted.filter((v) => v !== id), | |
| skipped: state.skipped.filter((v) => v !== id), | |
| })); | |
| }, | |
| reset: (id: TaskId) => { | |
| universalStore.setState((state) => ({ | |
| ...state, | |
| accepted: state.accepted.filter((v) => v !== id), | |
| done: state.done.filter((v) => v !== id), | |
| skipped: state.skipped.filter((v) => v !== id), | |
| })); | |
| }, |
🤖 Prompt for AI Agents
In code/core/src/shared/checklist-store/index.ts around lines 66 to 72, the
reset function removes the id from accepted and skipped but leaves done
unchanged; update the state update to also remove the id from done (e.g., set
done: state.done.filter(v => v !== id)) so reset fully returns an item to its
initial state.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 23.06 MB | 24.07 MB | 🚨 +1.01 MB 🚨 |
| Dependency size | 17.36 MB | 17.36 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 0 B |
| Dependency size | 73.02 MB | 74.03 MB | 🚨 +1.01 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 69.45 MB | 70.46 MB | 🚨 +1.01 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 0 B |
| Dependency size | 40.42 MB | 41.43 MB | 🚨 +1.01 MB 🚨 |
| Bundle Size Analyzer | node | node |
…first task for each section rather than first 3 open tasks
…lar set of (known) tasks
…e ChecklistModule
This reverts commit a55985b.
…o allow muting the widget
… in the settings page navigation
…es, update Listbox components to use div elements, and adjust related logic in ChecklistModule and useChecklist hook.
…h current prop, preventing broken animations during rapid changes.
ghengeveld
force-pushed
the
onboarding-guide
branch
from
fd39890 to
405b934
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
code/core/src/components/components/Card/Card.tsx (4)
35-44: Prop naming is inconsistent with Outline component.The
outlineColorprop from CardProps is passed ascolorto the Outline component (line 40), but Outline expects a prop namedcolor(line 54). While this works, the previous review recommended renaming the Outline prop tooutlineColorKeyto avoid collision with CSS property names and maintain consistency with the CardProps interface.
52-56: Critical: Fix Outline color prop type.The
color?: keyof typeof colortype is invalid due to the broken type import. This was flagged in a previous review. Change tooutlineColorKey?: keyof Theme['color'](or similar) after importing the Theme type, and update the prop usage accordingly.
3-3: Critical: Fix broken type import.The
import type { color }import is invalid and was flagged in a previous review. Thecolorexport from 'storybook/theming' is likely a runtime object, not a type. ImportThemetype instead and usekeyof Theme['color']for typing color keys throughout this file.
29-33: Critical: Fix outlineColor type definition.The
outlineColor?: keyof typeof colortype is invalid due to the brokencolortype import. This was flagged in a previous review. Change tokeyof Theme['color']after importing theThemetype.code/core/src/shared/checklist-store/index.ts (3)
6-12: Muted field semantics remain unclear.The
mutedfield still mixesbooleanandTaskId[]types, which can lead to inconsistent behavior in UI code. Past review comment already flagged this issue.
59-64: Skip method doesn't clear done state.The
skipmethod removes the item fromacceptedbut not fromdone. If an item was automatically marked as done viasubscribe, skipping it should likely clear that state for consistency. Past review comment already raised this concern.
66-72: Reset method doesn't clear done state.The
resetmethod clearsacceptedandskippedbut leavesdoneintact. If reset is meant to return an item to its initial state, it should also clear the done array. Past review comment already flagged this.code/core/src/components/components/Listbox/Listbox.tsx (1)
17-59: [Duplicate] Preventactiveprop from leaking to the DOM.The past review comment correctly identified that the
activeprop will be forwarded to the<div>element, potentially causing React DOM warnings. This issue remains unaddressed.Apply the fix from the previous review:
-export const ListboxItem = styled.div<{ active?: boolean }>(({ active, theme }) => ({ +export const ListboxItem = styled('div', { + shouldForwardProp: (prop) => prop !== 'active', +})<{ active?: boolean }>(({ active, theme }) => ({
🧹 Nitpick comments (5)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
35-35: Consider renaming the story export to avoid shadowing the global Proxy.The export name
Proxyshadows the JavaScript globalProxyobject. While this is story-file-scoped and unlikely to cause issues in practice, consider a clearer name likeProxyExampleorWithFocusProxyfor better clarity.Apply this diff:
-export const Proxy = meta.story({ +export const ProxyExample = meta.story({ render: () => (code/core/src/components/components/Listbox/Listbox.tsx (1)
7-15: Semantic HTML consideration: replacingulwithdiv.Changing the container from
<ul>to<div>(andListboxItemfrom<li>to<div>on line 17) removes the native list semantics. If this Listbox is used to render navigational menus or lists of options, consider adding ARIA attributes likerole="list"to the container androle="listitem"to items to preserve accessibility semantics.Verify whether ARIA roles are needed based on the usage context. If the Listbox is used for navigation or option lists, apply this diff:
-export const Listbox = styled.div(({ theme }) => ({ +export const Listbox = styled.div(({ theme }) => ({ + role: 'list', listStyle: 'none',And update
ListboxItem:-export const ListboxItem = styled.div<{ active?: boolean }>(({ active, theme }) => ({ +export const ListboxItem = styled('div', { + shouldForwardProp: (prop) => prop !== 'active', +})<{ active?: boolean }>(({ active, theme }) => ({ + // Add role="listitem" where ListboxItem is used, or via defaultPropsBased on learnings
code/core/src/manager/container/Menu.tsx (1)
120-120: Consider usingnullinstead of empty fragments for icons.Several menu items use
<></>for the icon when inactive (e.g., lines 120, 132, 144, 155, 166, 177, 188, 199). Usingnullorundefinedwould be more idiomatic and slightly more efficient than rendering empty React fragments.Apply this pattern:
- icon: isNavShown ? <CheckIcon /> : <></>, + icon: isNavShown ? <CheckIcon /> : null,Also applies to: 132-132, 144-144, 155-155, 166-166, 177-177, 188-188, 199-199
code/core/src/manager/settings/Checklist/Checklist.tsx (2)
276-276: RefactorcreateReftouseReffor better performance.Using
createRefinside auseMemo(line 276) creates a new ref object on every dependency change, which may not be necessary. If the ref needs to be stable across renders for the same item, consider using aMapwithuseRefor accepting that refs are recreated when dependencies change (which appears intentional here based on theuseMemodependencies).If refs should be stable per item ID across renders:
+ const refsMap = useRef<Map<string, React.RefObject<HTMLLIElement>>>(new Map()); + const itemsById: Record<ChecklistItem['id'], ChecklistItem> = useMemo( () => Object.fromEntries( sections.flatMap(({ items }) => items.map((item) => { const isAccepted = accepted.includes(item.id); const isDone = done.includes(item.id); const isLockedBy = item.after?.filter((id) => !accepted.includes(id) && !done.includes(id)) ?? []; const isSkipped = skipped.includes(item.id); - const nodeRef = createRef<HTMLLIElement>(); + if (!refsMap.current.has(item.id)) { + refsMap.current.set(item.id, createRef<HTMLLIElement>()); + } + const nodeRef = refsMap.current.get(item.id)!; return [item.id, { ...item, isAccepted, isDone, isLockedBy, isSkipped, nodeRef }]; }) ) ), [sections, accepted, done, skipped] );However, given that refs are tied to specific item state and the current pattern works, this refactor may not be necessary.
336-445: Complex nesting: verify focus behavior and keyboard navigation.The nesting of
ListboxItem→FocusTarget→FocusProxy→Collapsible→ItemSummarycreates multiple interactive layers. Ensure keyboard navigation (Tab, Enter, Space) and screen reader announcements work correctly across collapsed/expanded states, especially when items are locked or completed.Manual testing is recommended for:
- Tab order through checklist items
- Space/Enter behavior on collapsed vs expanded items
- Screen reader announcements for locked, completed, and skipped states
- Focus indicator visibility during keyboard navigation
If you'd like, I can generate a testing checklist for these accessibility scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/components/components/Button/Button.tsx(5 hunks)code/core/src/components/components/Card/Card.stories.tsx(1 hunks)code/core/src/components/components/Card/Card.tsx(4 hunks)code/core/src/components/components/Collapsible/Collapsible.stories.tsx(3 hunks)code/core/src/components/components/Collapsible/Collapsible.tsx(3 hunks)code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx(0 hunks)code/core/src/components/components/FocusProxy/FocusProxy.tsx(0 hunks)code/core/src/components/components/FocusRing/FocusRing.stories.tsx(1 hunks)code/core/src/components/components/FocusRing/FocusRing.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.stories.tsx(3 hunks)code/core/src/components/components/Listbox/Listbox.tsx(4 hunks)code/core/src/components/components/shared/useLocationHash.ts(1 hunks)code/core/src/components/index.ts(2 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager/components/TextFlip.stories.tsx(1 hunks)code/core/src/manager/components/TextFlip.tsx(1 hunks)code/core/src/manager/components/preview/tools/zoom.tsx(0 hunks)code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Explorer.tsx(0 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Menu.tsx(2 hunks)code/core/src/manager/components/sidebar/RefBlocks.tsx(0 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(0 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(3 hunks)code/core/src/manager/components/sidebar/Sidebar.tsx(5 hunks)code/core/src/manager/components/sidebar/TestingModule.tsx(1 hunks)code/core/src/manager/components/sidebar/Tree.tsx(1 hunks)code/core/src/manager/components/sidebar/TreeNode.tsx(1 hunks)code/core/src/manager/components/sidebar/useChecklist.ts(1 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/container/Menu.tsx(12 hunks)code/core/src/manager/container/Sidebar.tsx(2 hunks)code/core/src/manager/globals/exports.ts(3 hunks)code/core/src/manager/settings/Checklist/Checklist.stories.tsx(2 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(6 hunks)code/core/src/manager/settings/Checklist/checklistData.tsx(8 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)code/core/src/manager/settings/GuidePage.tsx(2 hunks)code/core/src/manager/settings/index.tsx(3 hunks)code/core/src/shared/checklist-store/index.ts(1 hunks)
💤 Files with no reviewable changes (6)
- code/core/src/manager/components/sidebar/Explorer.tsx
- code/core/src/manager/components/preview/tools/zoom.tsx
- code/core/src/manager/components/sidebar/RefBlocks.tsx
- code/core/src/components/components/FocusProxy/FocusProxy.tsx
- code/core/src/manager/components/sidebar/SearchResults.tsx
- code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- code/core/src/components/index.ts
- code/core/src/manager/components/TextFlip.tsx
- code/core/src/manager/components/sidebar/TestingModule.tsx
- code/core/src/core-server/utils/checklist.ts
- code/core/src/manager/settings/GuidePage.stories.tsx
- code/core/src/manager/container/Menu.stories.tsx
- code/core/src/manager/components/sidebar/Sidebar.tsx
- code/core/src/manager/settings/Checklist/checklistData.tsx
- code/core/src/components/components/Listbox/Listbox.stories.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/manager/container/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/manager/container/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/manager/container/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/settings/GuidePage.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/useChecklist.tscode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/manager/container/Sidebar.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/components/components/shared/useLocationHash.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/shared/checklist-store/index.tscode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/components/TextFlip.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/components/components/FocusRing/FocusRing.tsxcode/core/src/components/components/Collapsible/Collapsible.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
🧠 Learnings (20)
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.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/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/globals/exports.tscode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.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/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should be placed in beforeEach blocks
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.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/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/.storybook/** : Place internal UI Storybook configuration in `code/.storybook/` and maintain it there
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsx
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : In application code, use Storybook loggers instead of `console.*` (client code: `storybook/internal/client-logger`; server code: `storybook/internal/node-logger`)
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/ChecklistModule.stories.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/ChecklistModule.tsxcode/core/src/components/components/Button/Button.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/components/components/Card/Card.tsxcode/core/src/components/components/FocusRing/FocusRing.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
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/ChecklistModule.stories.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx
🧬 Code graph analysis (19)
code/core/src/manager/settings/GuidePage.tsx (2)
code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/manager/settings/Checklist/Checklist.tsx (1)
Checklist(233-456)
code/core/src/manager/components/sidebar/useChecklist.ts (2)
code/core/src/manager/settings/Checklist/checklistData.tsx (2)
ChecklistData(8-30)checklistData(32-268)code/core/src/manager-api/root.tsx (2)
useStorybookApi(294-297)useStorybookState(290-293)
code/core/src/manager/components/sidebar/Menu.stories.tsx (1)
code/core/src/manager/container/Menu.tsx (1)
useMenu(60-262)
code/core/src/manager/container/Sidebar.tsx (2)
code/core/src/manager/container/Menu.tsx (1)
useMenu(60-262)code/core/src/manager/manager-stores.ts (1)
experimental_useStatusStore(13-13)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (6)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
Collapsible(12-41)code/core/src/components/components/Listbox/Listbox.tsx (6)
ListboxButton(61-68)Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(63-100)code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)
code/core/src/manager/components/sidebar/Tree.tsx (1)
code/core/src/manager/components/sidebar/IconSymbols.tsx (1)
IconSymbols(28-109)
code/core/src/components/components/Card/Card.tsx (1)
code/core/src/components/index.ts (1)
Card(45-45)
code/core/src/components/components/shared/useLocationHash.ts (1)
code/core/src/components/index.ts (1)
useLocationHash(96-96)
code/core/src/manager/settings/Checklist/Checklist.tsx (7)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
ChecklistData(8-30)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)code/core/src/components/components/FocusRing/FocusRing.tsx (2)
FocusProxy(17-21)FocusTarget(43-73)code/core/src/components/components/Listbox/Listbox.tsx (1)
ListboxItem(17-59)code/core/src/components/components/Button/Button.tsx (1)
Button(20-78)
code/core/src/manager/container/Menu.tsx (3)
code/core/src/manager-api/root.tsx (1)
API(94-107)code/core/src/manager/components/sidebar/useChecklist.ts (1)
useChecklist(17-100)code/core/src/components/components/Listbox/Listbox.tsx (2)
ListboxButton(61-68)ListboxIcon(96-103)
code/core/src/manager/components/sidebar/Menu.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (5)
Listbox(7-15)ListboxItem(17-59)ListboxAction(70-79)ListboxIcon(96-103)ListboxText(81-94)code/core/src/components/index.ts (5)
Listbox(48-48)ListboxItem(52-52)ListboxAction(49-49)ListboxIcon(51-51)ListboxText(53-53)
code/core/src/components/components/Card/Card.stories.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(35-44)
code/core/src/manager/components/TextFlip.stories.tsx (1)
code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(63-100)
code/core/src/manager/settings/index.tsx (1)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-65)
code/core/src/components/components/FocusRing/FocusRing.tsx (1)
code/core/src/components/components/shared/useLocationHash.ts (1)
useLocationHash(37-41)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (3)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(32-268)code/core/src/manager/manager-stores.mock.ts (1)
checklistStore(57-57)code/core/src/manager/manager-stores.ts (1)
checklistStore(25-25)
code/core/src/components/components/Listbox/Listbox.tsx (1)
code/core/src/components/index.ts (6)
Listbox(48-48)ListboxItem(52-52)ListboxAction(49-49)ListboxButton(50-50)ListboxText(53-53)ListboxIcon(51-51)
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (2)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
useCollapsible(62-97)code/core/src/test/testing-library.ts (1)
userEvent(123-126)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
code/core/src/components/components/FocusRing/FocusRing.tsx (3)
FocusRing(23-41)FocusProxy(17-21)FocusTarget(43-73)
🪛 Biome (2.1.2)
code/core/src/manager/container/Sidebar.tsx
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
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)
code/core/src/components/components/FocusRing/FocusRing.stories.tsx
[error] 35-35: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (2)
- GitHub Check: merged
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (31)
code/core/src/manager/components/sidebar/TreeNode.tsx (1)
90-93: LGTM! Coordinated styling adjustment.This first-of-type margin reset properly compensates for the removal of the Container's top margin in Tree.tsx, ensuring consistent spacing for the first RootNode.
code/core/src/manager/components/sidebar/Tree.tsx (1)
716-719: LGTM! Code simplification.Replacing the styled Container with a plain div simplifies the code while maintaining functionality. The spacing previously handled by Container's conditional top margin is now managed by the first-of-type rule in RootNode (TreeNode.tsx).
code/core/src/components/components/Button/Button.tsx (1)
15-15: LGTM: readOnly implementation is thorough and correct.The
readOnlyprop is properly integrated throughout the component:
- Correctly gates cursor behavior (line 102)
- Excludes readOnly buttons from disabled opacity dimming (line 136)
- Consistently prevents interactive styling (:hover, :active, :focus) when readOnly is true (lines 172-187, 214-259)
Also applies to: 30-30, 69-69, 95-259
code/core/src/manager/components/TextFlip.stories.tsx (1)
1-38: LGTM: Clean story implementation.The TextFlip stories follow standard Storybook patterns with clear examples demonstrating the component's animation behavior for increasing and decreasing values.
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
62-96: LGTM: useCollapsible hook implementation is correct.The hook properly:
- Maintains internal collapsed state with external prop synchronization
- Guards toggle behavior with disabled check (line 74)
- Provides comprehensive toggleProps with proper ARIA attributes (lines 82-87)
- Returns all necessary state and handlers for external usage
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
1-54: LGTM: Stories properly reflect the updated Collapsible API.The updated stories correctly demonstrate:
- New toggleProps pattern (line 13)
- Collapsed state control (lines 27-31)
- Disabled behavior (lines 33-37)
- Interactive examples via play functions (lines 40-41, 53)
code/core/src/manager/globals/exports.ts (1)
500-503: LGTM: Public API exports properly updated.The new exports (FocusOutline, FocusRing, FocusTarget, ListboxIcon, useLocationHash) are correctly added to the globals manifest, aligning with the PR's expansion of the public component surface.
Also applies to: 520-520, 559-559
code/core/src/components/components/FocusRing/FocusRing.stories.tsx (1)
48-68: LGTM: Target story properly demonstrates hash-driven focus behavior.The story correctly:
- Sets up hash navigation trigger (line 51)
- Uses a delay before interaction to allow initialization (line 62)
- Cleans up hash state between test runs (lines 65-67)
code/core/src/components/components/Card/Card.tsx (4)
63-76: Excellent accessibility and progressive enhancement.The implementation includes several best practices:
- CSS variable fallback for extensibility (line 63)
- Progressive enhancement with
interpolate-sizesupport (lines 67-72)- Respects
prefers-reduced-motion(lines 74-76)These additions improve both accessibility and user experience.
78-97: Well-structured lifecycle animation states.The enter/exit lifecycle classes provide a clean API for animation libraries. Using modern CSS properties like
contentVisibilityandblockSizewith proper fallbacks forinterpolate-sizeensures smooth animations while maintaining performance.
125-128: Update prop reference if renamed.If the
colorprop is renamed tooutlineColorKey(as suggested in the previous review), this conditional check on line 125 will need to be updated accordingly.
131-131: Good extensibility pattern for pseudo-element styles.The conditional merge of
styles['&:before']allows consumers to customize the pseudo-element while safely handling cases where the property doesn't exist or isn't an object.code/core/src/components/components/Card/Card.stories.tsx (1)
12-106: Approved: well-structured story refactoring with valid color values.The refactoring from a single "All" story to individual named stories improves testability and documentation clarity. All 13 color values used in the stories (
positive,warning,negative,primary,secondary,ancillary,orange,gold,green,seafoam,purple,ultraviolet,mediumdark) are valid theme colors fromstorybook/theming/base.ts, and the Card component'soutlineColortype definition (keyof typeof color) provides compile-time safety.code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
105-114: LGTM! Proper checklist state initialization.The beforeEach hook correctly initializes the universalChecklistStore with the new state model (loaded, muted, accepted, done, skipped), providing consistent test setup across stories.
159-165: LGTM! Proper global state cleanup.The beforeEach hook correctly saves and restores
global.CONFIG_TYPE, ensuring test isolation and preventing side effects between stories.code/core/src/manager/components/sidebar/Menu.stories.tsx (2)
32-40: LGTM! Consistent mock store initialization.The beforeEach hook properly initializes the mock checklist store with the new state model, ensuring consistent test setup for all menu stories.
67-80: LGTM! Updated to new useMenu signature.The story correctly uses the new useMenu API with a single object parameter containing all required props (api, showToolbar, isPanelShown, isNavShown, enableShortcuts).
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
24-32: LGTM! Story properly initialized with new state model.The beforeEach hook correctly initializes the mock store with the new checklist state structure (loaded, muted, accepted, done, skipped), providing appropriate test data for the ChecklistModule component.
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
36-44: LGTM! Story properly merges data and store.The Default story correctly combines
checklistData(sections/items),checklistStore(methods), and explicit state fields (accepted, done, skipped) to provide complete props for the Checklist component. This approach gives explicit control over the checklist state for testing.code/core/src/cli/globalSettings.ts (1)
18-24: Thedonefield is intentionally session-only and should not be persisted.The core-server initialization loads and saves only
muted,accepted, andskippedfrom globalSettings—notdone. Thedonearray resets each session and is not meant to persist across app restarts. The schema is correct as written.Likely an incorrect or invalid review comment.
code/core/src/shared/checklist-store/index.ts (1)
73-82: No issues found after verification.The
mute()method's type coercion is intentional design. The type annotationmuted: boolean | Array<TaskId>explicitly allows both modes, and actual usage patterns confirm:
mute(false)clears all mutes (switch to boolean mode)mute(array)sets specific muted items (switch to array mode)The merge logic correctly handles the state transition: when
state.mutedisfalseandmute(array)is called, coercingfalseto[]is semantically correct (nothing muted = no items). The theoretical edge case (state.muted = truewithmute(array)) does not occur in the codebase. Nomute(true)calls were found.code/core/src/manager/settings/GuidePage.tsx (1)
50-62: No functional bug, but the truthiness check lacks precision for maintainability.The conditional
checklist.muted ?works correctly for the actual state transitions in current code—falsevs. an array of all item IDs—but it's inconsistent with the explicit type-checking pattern used inuseChecklist.ts(line 32:!(Array.isArray(muted) && muted.includes(item.id))).Consider making the check more explicit for clarity:
{Array.isArray(checklist.muted) && checklist.muted.length > 0 ? ( // show "Show in sidebar" ) : ( // show "Remove from sidebar" )}This prevents confusion about edge cases and aligns with the pattern already established in the codebase.
code/core/src/components/components/Listbox/Listbox.tsx (2)
96-103: LGTM: NewListboxIconcomponent is well-structured.The new
ListboxIconcomponent uses the CSS variable pattern consistently with the rest of the Listbox API and provides proper centering for icon content.
12-14: Selector change appears safe in current codebase but is architecturally broader than necessary.Verification found that all Listbox elements in the codebase only have Listbox siblings or no siblings. The
& + *selector applies borders only between consecutive Listbox groups (Menu.tsx, ChecklistModule.tsx) and in the Groups story (Listbox.stories.tsx lines 70–93), where all three adjacent elements are Listbox components. No unintended borders are occurring.However, the concern about broadening the selector is valid: the change from
& + &to& + *makes the rule apply to any adjacent sibling, not just Listbox. While this poses no immediate risk, it could cause unintended styling if a non-Listbox element is placed as a sibling to Listbox in the future. If the selector broadening was intentional, confirm the rationale; otherwise, consider reverting to& + &to maintain the original restrictive behavior.code/core/src/manager/container/Menu.tsx (3)
86-100: LGTM: Previous concern about exposing the guide in production has been addressed.The guide menu item is now correctly gated with
CONFIG_TYPE === 'DEVELOPMENT'(line 238), and the progress indicator uses a non-interactiveListboxButtonwithas="div"andreadOnly, addressing the past review comment.Also applies to: 238-238
48-50: Styled component for ProgressCircle is concise and correct.The
ProgressCirclestyled component correctly applies the theme's secondary color for visual consistency with the active state.
204-218: No changes required.The
getDocsUrlimplementation incode/core/src/manager-api/modules/versions.ts(lines 92-130) is well-tested and robust. It correctly handles theversioned: trueandrenderer: trueparameters used in Menu.tsx, always returning a valid URL. The method includes defensive checks (e.g., verifyingglobal.STORYBOOK_RENDERERexists before using it) and covers all supported environments. Comprehensive test coverage confirms expected behavior across various version and renderer scenarios.code/core/src/manager/settings/Checklist/Checklist.tsx (4)
367-389: Accessibility of lock state is well-implemented.The lock state uses a
WithTooltipwith an informative message listing prerequisite items, and the button has properaria-label,disabled, andreadOnlyattributes. This provides good accessibility for users who need to understand why an item is locked.
248-251: LGTM:isCompletedhelper is correctly implemented.The
isCompletedcallback correctly checks if an item is in any of the completion states (accepted, done, or skipped) and is properly memoized with all necessary dependencies.
266-282: Lock state computation is correct.The
isLockedBylogic (lines 273-274) correctly filters prerequisite items that are neither accepted nor done, ensuring items are only unlocked when all dependencies are satisfied. This aligns with theisReadylogic in theuseChecklisthook.
390-416: No issues found — the subscribe completion mechanism works correctly.Items with
subscribecallbacks are properly managed through the auto-completion path: the subscribe callback is invoked in theuseChecklisthook (lines 57–62), callsdone()when ready, which updates the universal store'sdonearray, and the component'sisCompletedcheck includes items from thedonelist. The absence of a manual "Mark as complete" button for subscribed items (line 407) is intentional—these items are self-managed, not user-managed.Likely an incorrect or invalid review comment.
| useEffect(() => { | ||
| if (!index || !loaded) { | ||
| return; | ||
| } | ||
|
|
||
| for (const item of allItems) { | ||
| if (!item.subscribe) { | ||
| continue; | ||
| } | ||
|
|
||
| const open = isOpen(item); | ||
| const subscribed = subscriptions.has(item.id); | ||
| if (open && !subscribed) { | ||
| subscriptions.set( | ||
| item.id, | ||
| item.subscribe({ | ||
| api, | ||
| index, | ||
| item, | ||
| done: () => checklistStore.done(item.id), | ||
| skip: () => checklistStore.skip(item.id), | ||
| }) | ||
| ); | ||
| } else if (subscribed && !open) { | ||
| subscriptions.get(item.id)?.(); | ||
| subscriptions.delete(item.id); | ||
| } | ||
| } | ||
| }, [api, index, loaded, allItems, isOpen]); |
There was a problem hiding this comment.
Clean up checklist subscriptions on unmount
Each open item that exposes subscribe registers listeners, but the effect never tears them down if the component unmounts or loaded/index flips falsy. That leaks callbacks and leaves the shared subscriptions map populated indefinitely. Please add a cleanup that calls every stored unsubscribe and clears the map.
- useEffect(() => {
- if (!index || !loaded) {
- return;
- }
-
- for (const item of allItems) {
+ useEffect(() => {
+ if (index && loaded) {
+ for (const item of allItems) {
if (!item.subscribe) {
continue;
}
const open = isOpen(item);
const subscribed = subscriptions.has(item.id);
if (open && !subscribed) {
subscriptions.set(
item.id,
item.subscribe({
api,
index,
item,
done: () => checklistStore.done(item.id),
skip: () => checklistStore.skip(item.id),
})
);
} else if (subscribed && !open) {
subscriptions.get(item.id)?.();
subscriptions.delete(item.id);
}
}
- }, [api, index, loaded, allItems, isOpen]);
+ }
+
+ return () => {
+ for (const [id, unsubscribe] of subscriptions) {
+ unsubscribe?.();
+ subscriptions.delete(id);
+ }
+ };
+ }, [api, index, loaded, allItems, isOpen]);📝 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.
| useEffect(() => { | |
| if (!index || !loaded) { | |
| return; | |
| } | |
| for (const item of allItems) { | |
| if (!item.subscribe) { | |
| continue; | |
| } | |
| const open = isOpen(item); | |
| const subscribed = subscriptions.has(item.id); | |
| if (open && !subscribed) { | |
| subscriptions.set( | |
| item.id, | |
| item.subscribe({ | |
| api, | |
| index, | |
| item, | |
| done: () => checklistStore.done(item.id), | |
| skip: () => checklistStore.skip(item.id), | |
| }) | |
| ); | |
| } else if (subscribed && !open) { | |
| subscriptions.get(item.id)?.(); | |
| subscriptions.delete(item.id); | |
| } | |
| } | |
| }, [api, index, loaded, allItems, isOpen]); | |
| useEffect(() => { | |
| if (index && loaded) { | |
| for (const item of allItems) { | |
| if (!item.subscribe) { | |
| continue; | |
| } | |
| const open = isOpen(item); | |
| const subscribed = subscriptions.has(item.id); | |
| if (open && !subscribed) { | |
| subscriptions.set( | |
| item.id, | |
| item.subscribe({ | |
| api, | |
| index, | |
| item, | |
| done: () => checklistStore.done(item.id), | |
| skip: () => checklistStore.skip(item.id), | |
| }) | |
| ); | |
| } else if (subscribed && !open) { | |
| subscriptions.get(item.id)?.(); | |
| subscriptions.delete(item.id); | |
| } | |
| } | |
| } | |
| return () => { | |
| for (const [id, unsubscribe] of subscriptions) { | |
| unsubscribe?.(); | |
| subscriptions.delete(id); | |
| } | |
| }; | |
| }, [api, index, loaded, allItems, isOpen]); |
🤖 Prompt for AI Agents
In code/core/src/manager/components/sidebar/useChecklist.ts around lines 42 to
70, the effect that registers checklist item subscriptions never cleans them up
on unmount or when index/loaded become falsy, leaking listeners and leaving the
subscriptions map populated; add a cleanup function returned from the useEffect
that iterates over subscriptions, calls each stored unsubscribe function if
present, and then clears the subscriptions map so all listeners are removed when
the effect is torn down or re-run.
| {({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts, ...state }) => { | ||
| const menu = useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts }); | ||
| const allStatuses = experimental_useStatusStore(); | ||
|
|
||
| return ( | ||
| <SidebarComponent {...fromState} allStatuses={allStatuses} onMenuClick={onMenuClick} /> | ||
| <SidebarComponent | ||
| {...state} | ||
| menu={menu} | ||
| onMenuClick={onMenuClick} | ||
| allStatuses={allStatuses} | ||
| enableShortcuts={enableShortcuts} | ||
| /> |
There was a problem hiding this comment.
Move hook calls out of the Consumer render prop
useMenu and experimental_useStatusStore are being invoked inside the Consumer’s render function. That render prop is a nested function, so this trips the Rule of Hooks and will fail linting (and risks subtly breaking if React ever changes how often the render prop runs). Hoist the logic into a proper component so the hooks run at the top level of that component instead.
- <Consumer filter={mapper}>
- {({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts, ...state }) => {
- const menu = useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts });
- const allStatuses = experimental_useStatusStore();
-
- return (
- <SidebarComponent
- {...state}
- menu={menu}
- onMenuClick={onMenuClick}
- allStatuses={allStatuses}
- enableShortcuts={enableShortcuts}
- />
- );
- }}
- </Consumer>
+ <Consumer filter={mapper}>
+ {(props) => (
+ <SidebarWithMenu {...props} onMenuClick={onMenuClick} />
+ )}
+ </Consumer>+const SidebarWithMenu = React.memo(
+ ({
+ api,
+ showToolbar,
+ isPanelShown,
+ isNavShown,
+ enableShortcuts,
+ onMenuClick,
+ ...state
+ }: ReturnType<typeof mapper> & SidebarProps) => {
+ const menu = useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts });
+ const allStatuses = experimental_useStatusStore();
+
+ return (
+ <SidebarComponent
+ {...state}
+ menu={menu}
+ onMenuClick={onMenuClick}
+ allStatuses={allStatuses}
+ enableShortcuts={enableShortcuts}
+ />
+ );
+ }
+);📝 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.
| {({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts, ...state }) => { | |
| const menu = useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts }); | |
| const allStatuses = experimental_useStatusStore(); | |
| return ( | |
| <SidebarComponent {...fromState} allStatuses={allStatuses} onMenuClick={onMenuClick} /> | |
| <SidebarComponent | |
| {...state} | |
| menu={menu} | |
| onMenuClick={onMenuClick} | |
| allStatuses={allStatuses} | |
| enableShortcuts={enableShortcuts} | |
| /> | |
| <Consumer filter={mapper}> | |
| {(props) => ( | |
| <SidebarWithMenu {...props} onMenuClick={onMenuClick} /> | |
| )} | |
| </Consumer> | |
| const SidebarWithMenu = React.memo( | |
| ({ | |
| api, | |
| showToolbar, | |
| isPanelShown, | |
| isNavShown, | |
| enableShortcuts, | |
| onMenuClick, | |
| ...state | |
| }: ReturnType<typeof mapper> & SidebarProps) => { | |
| const menu = useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts }); | |
| const allStatuses = experimental_useStatusStore(); | |
| return ( | |
| <SidebarComponent | |
| {...state} | |
| menu={menu} | |
| onMenuClick={onMenuClick} | |
| allStatuses={allStatuses} | |
| enableShortcuts={enableShortcuts} | |
| /> | |
| ); | |
| } | |
| ); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
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)
🤖 Prompt for AI Agents
In code/core/src/manager/container/Sidebar.tsx around lines 58 to 69, hooks
(useMenu and experimental_useStatusStore) are being called inside the Consumer
render prop which breaks the Rules of Hooks; extract those calls into a
top-level React component: create a new functional component (e.g.,
SidebarConsumerInner) that accepts the Consumer props (api, showToolbar,
isPanelShown, isNavShown, enableShortcuts, and the rest), call useMenu and
experimental_useStatusStore at the top of that component, build the menu and
allStatuses, and return the SidebarComponent with the same props (spread the
rest props and pass menu, allStatuses, onMenuClick, enableShortcuts); then
replace the inline render function in the Consumer with this new component so
hooks run at the component top level.
See #32644
Telescopes on #32795
What I did
This PR handles all the UI improvements made for the onboarding guide / checklist since #32795.
Checklist data and completion behavior goes in #32799.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 pull request has been released as version
0.0.0-pr-32874-sha-fd398904. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-32874-sha-fd398904onboarding-guidefd3989041762178366)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=32874Summary by CodeRabbit
New Features
UI/UX Improvements