Core: Define checklist tasks #32799
Core: Define checklist tasks #32799
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces a new TourGuide component system to replace the existing GuidedTour, adds a Survey component to the onboarding addon, refactors the onboarding flow to support survey completion tracking, expands the checklist system with universal stores and enhanced hooks, and adds new UI components including Optional and HighlightElement. The PR reorganizes manager store exports with internal naming conventions and removes deprecated onboarding components while adding new example stories. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Manager as Storybook Manager
participant Onboarding as Onboarding Addon
participant Survey as Survey Component
participant TourGuide as TourGuide
participant ChecklistStore as Checklist Store
User->>Manager: Load Storybook
Manager->>Onboarding: Check onboarding state
alt onboarding='survey'
Onboarding->>Survey: Render survey
User->>Survey: Complete survey
Survey->>ChecklistStore: update survey completed
Survey->>Manager: Update URL (onboarding=false)
else onboarding=true
Manager->>TourGuide: Initialize tour
User->>TourGuide: Navigate steps
User->>TourGuide: Complete/dismiss tour
TourGuide->>ChecklistStore: Update item state
end
Manager->>User: Show completion state
sequenceDiagram
participant TourGuide
participant Joyride as react-joyride
participant HighlightElement
participant DOM
TourGuide->>Joyride: Mount with steps config
Joyride->>HighlightElement: Render highlight for target
HighlightElement->>DOM: Query target element
HighlightElement->>DOM: Apply overlay + highlight styles
alt pulsating enabled
HighlightElement->>DOM: Inject pulsating keyframes
DOM->>DOM: Animate pulsate
end
Joyride->>Joyride: Show tooltip
TourGuide->>Joyride: on(ACTIONS.NEXT)
Joyride->>HighlightElement: Update target selector
HighlightElement->>DOM: Update overlay position
sequenceDiagram
participant Component as UI Component
participant useChecklist as useChecklist Hook
participant ChecklistStore as Checklist Store
participant Index as Story Index
Component->>useChecklist: Subscribe to checklist state
useChecklist->>ChecklistStore: Get state (accepted, done, skipped)
useChecklist->>Index: Get story index (throttled)
useChecklist->>useChecklist: Compute item availability/readiness
useChecklist->>useChecklist: Build availableItems + collections
useChecklist->>Component: Return items, nextItems, progress
User->>Component: Click item action
Component->>useChecklist: accept/done/skip
useChecklist->>ChecklistStore: setState(...) with update
ChecklistStore->>ChecklistStore: Emit telemetry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
Comment |
|
View your CI Pipeline Execution ↗ for commit aad4c79
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 509 KB | 🚨 +509 KB 🚨 |
| Dependency size | 0 B | 2.97 MB | 🚨 +2.97 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-docs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 18 | 🚨 +18 🚨 |
| Self size | 0 B | 1.88 MB | 🚨 +1.88 MB 🚨 |
| Dependency size | 0 B | 10.21 MB | 🚨 +10.21 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-links
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 15 KB | 🚨 +15 KB 🚨 |
| Dependency size | 0 B | 5 KB | 🚨 +5 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-onboarding
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 56 KB | 🚨 +56 KB 🚨 |
| Dependency size | 0 B | 670 B | 🚨 +670 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook-addon-pseudo-states
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 24 KB | 🚨 +24 KB 🚨 |
| Dependency size | 0 B | 689 B | 🚨 +689 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-themes
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 20 KB | 🚨 +20 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-vitest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 6 | 🚨 +6 🚨 |
| Self size | 0 B | 509 KB | 🚨 +509 KB 🚨 |
| Dependency size | 0 B | 1.53 MB | 🚨 +1.53 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 11 | 🚨 +11 🚨 |
| Self size | 0 B | 330 KB | 🚨 +330 KB 🚨 |
| Dependency size | 0 B | 1.30 MB | 🚨 +1.30 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 73 KB | 🚨 +73 KB 🚨 |
| Dependency size | 0 B | 31.87 MB | 🚨 +31.87 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 43 | 🚨 +43 🚨 |
| Self size | 0 B | 24.89 MB | 🚨 +24.89 MB 🚨 |
| Dependency size | 0 B | 17.36 MB | 🚨 +17.36 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 126 KB | 🚨 +126 KB 🚨 |
| Dependency size | 0 B | 30.00 MB | 🚨 +30.00 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 191 | 🚨 +191 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 28.58 MB | 🚨 +28.58 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 23 KB | 🚨 +23 KB 🚨 |
| Dependency size | 0 B | 1.67 MB | 🚨 +1.67 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 533 | 🚨 +533 🚨 |
| Self size | 0 B | 749 KB | 🚨 +749 KB 🚨 |
| Dependency size | 0 B | 58.91 MB | 🚨 +58.91 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 124 | 🚨 +124 🚨 |
| Self size | 0 B | 3.83 MB | 🚨 +3.83 MB 🚨 |
| Dependency size | 0 B | 21.87 MB | 🚨 +21.87 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 14 KB | 🚨 +14 KB 🚨 |
| Dependency size | 0 B | 1.65 MB | 🚨 +1.65 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 157 | 🚨 +157 🚨 |
| Self size | 0 B | 31 KB | 🚨 +31 KB 🚨 |
| Dependency size | 0 B | 23.11 MB | 🚨 +23.11 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 114 | 🚨 +114 🚨 |
| Self size | 0 B | 37 KB | 🚨 +37 KB 🚨 |
| Dependency size | 0 B | 19.66 MB | 🚨 +19.66 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 273 | 🚨 +273 🚨 |
| Self size | 0 B | 25 KB | 🚨 +25 KB 🚨 |
| Dependency size | 0 B | 43.87 MB | 🚨 +43.87 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 199 | 🚨 +199 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 33.12 MB | 🚨 +33.12 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 19 | 🚨 +19 🚨 |
| Self size | 0 B | 59 KB | 🚨 +59 KB 🚨 |
| Dependency size | 0 B | 26.79 MB | 🚨 +26.79 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/sveltekit
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 20 | 🚨 +20 🚨 |
| Self size | 0 B | 58 KB | 🚨 +58 KB 🚨 |
| Dependency size | 0 B | 26.85 MB | 🚨 +26.85 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 109 | 🚨 +109 🚨 |
| Self size | 0 B | 38 KB | 🚨 +38 KB 🚨 |
| Dependency size | 0 B | 43.95 MB | 🚨 +43.95 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 15 | 🚨 +15 🚨 |
| Self size | 0 B | 20 KB | 🚨 +20 KB 🚨 |
| Dependency size | 0 B | 1.70 MB | 🚨 +1.70 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 928 KB | 🚨 +928 KB 🚨 |
| Dependency size | 0 B | 74.83 MB | 🚨 +74.83 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 169 | 🚨 +169 🚨 |
| Self size | 0 B | 35 KB | 🚨 +35 KB 🚨 |
| Dependency size | 0 B | 71.26 MB | 🚨 +71.26 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/core-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 12 KB | 🚨 +12 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 44 | 🚨 +44 🚨 |
| Self size | 0 B | 1.55 MB | 🚨 +1.55 MB 🚨 |
| Dependency size | 0 B | 42.25 MB | 🚨 +42.25 MB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/csf-plugin
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 9 | 🚨 +9 🚨 |
| Self size | 0 B | 9 KB | 🚨 +9 KB 🚨 |
| Dependency size | 0 B | 1.27 MB | 🚨 +1.27 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
eslint-plugin-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 35 | 🚨 +35 🚨 |
| Self size | 0 B | 139 KB | 🚨 +139 KB 🚨 |
| Dependency size | 0 B | 3.15 MB | 🚨 +3.15 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 22 KB | 🚨 +22 KB 🚨 |
| Dependency size | 0 B | 788 B | 🚨 +788 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 68 | 🚨 +68 🚨 |
| Self size | 0 B | 36 KB | 🚨 +36 KB 🚨 |
| Dependency size | 0 B | 5.98 MB | 🚨 +5.98 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 170 | 🚨 +170 🚨 |
| Self size | 0 B | 21 KB | 🚨 +21 KB 🚨 |
| Dependency size | 0 B | 31.20 MB | 🚨 +31.20 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-server-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 10 | 🚨 +10 🚨 |
| Self size | 0 B | 8 KB | 🚨 +8 KB 🚨 |
| Dependency size | 0 B | 1.20 MB | 🚨 +1.20 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 30 KB | 🚨 +30 KB 🚨 |
| Dependency size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 57 | 🚨 +57 🚨 |
| Self size | 0 B | 829 KB | 🚨 +829 KB 🚨 |
| Dependency size | 0 B | 12.92 MB | 🚨 +12.92 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 9 KB | 🚨 +9 KB 🚨 |
| Dependency size | 0 B | 716 KB | 🚨 +716 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 48 KB | 🚨 +48 KB 🚨 |
| Dependency size | 0 B | 230 KB | 🚨 +230 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 61 KB | 🚨 +61 KB 🚨 |
| Dependency size | 0 B | 211 KB | 🚨 +211 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 43 KB | 🚨 +43 KB 🚨 |
| Dependency size | 0 B | 47 KB | 🚨 +47 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
ghengeveld
force-pushed
the
checklist-tasks
branch
5 times, most recently
from
9e334b3 to
090a9f8
Compare
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/19066601453 |
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/19066665012 |
…mponent, and reuse it in TestingModule
ghengeveld
force-pushed
the
checklist-tasks
branch
from
e72c6b2 to
624bfc6
Compare
… that can't be undone after completion
| 'TourGuide', | ||
| 'TourTooltip', |
There was a problem hiding this comment.
Why are these public API components?
| { | ||
| directory: '../addons/onboarding/example-stories', | ||
| }, |
There was a problem hiding this comment.
Can you explain the purpose of these stories?
| TourGuide.render = (props: ComponentProps<typeof TourGuide>) => { | ||
| let container = document.getElementById('storybook-tour'); | ||
| if (!container) { | ||
| container = document.createElement('div'); | ||
| container.id = 'storybook-tour'; | ||
| document.body.appendChild(container); | ||
| } | ||
| root = root ?? createRoot(container); | ||
| root.render( | ||
| <ThemeProvider theme={convert(themes.light)}> | ||
| <TourGuide | ||
| {...props} | ||
| onComplete={() => { | ||
| props.onComplete?.(); | ||
| root?.render(null); | ||
| root = null; | ||
| }} | ||
| onDismiss={() => { | ||
| props.onDismiss?.(); | ||
| root?.render(null); | ||
| root = null; | ||
| }} | ||
| /> | ||
| </ThemeProvider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
What is going on here?
This is a react component.. but the react component has a custom method called render which then starts mounting itself?
Can you explain why this is the right approach? because this does not seem like the right approach to me.
| export const Optional = ({ | ||
| content, | ||
| fallback, | ||
| }: { | ||
| content: ReactElement; | ||
| fallback?: ReactElement; | ||
| }) => { | ||
| const contentRef = useRef<HTMLDivElement>(null); | ||
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| const [hidden, setHidden] = useState<boolean | null>(null); | ||
|
|
||
| const contentWidth = useRef(contentRef.current?.offsetWidth ?? 0); | ||
| const wrapperWidth = useRef(wrapperRef.current?.offsetWidth ?? 0); | ||
|
|
||
| useEffect(() => { | ||
| if (contentRef.current && wrapperRef.current) { | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| wrapperWidth.current = wrapperRef.current?.offsetWidth || wrapperWidth.current; | ||
| contentWidth.current = contentRef.current?.offsetWidth || contentWidth.current; | ||
| setHidden(contentWidth.current > wrapperWidth.current); | ||
| }); | ||
| resizeObserver.observe(wrapperRef.current); | ||
| return () => resizeObserver.disconnect(); | ||
| } | ||
| }, []); | ||
|
|
||
| return ( | ||
| <Wrapper ref={wrapperRef}> | ||
| <Content isHidden={hidden} ref={contentRef}> | ||
| {content} | ||
| </Content> | ||
|
|
||
| {fallback && <Content isHidden={!hidden}>{fallback}</Content>} | ||
| </Wrapper> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Can you explain what does component does, and why Optional is the right name for this?
And why it must be a public facing component, when we're debating in "Theming 2.0 project" if supplying a component library is something we want to continue doing?
| setTimeout(() => { | ||
| mockStore.setState({ | ||
| loaded: true, | ||
| muted: false, | ||
| accepted: ['controls'], | ||
| done: ['install-storybook', 'render-component', 'viewports'], | ||
| skipped: ['more-components', 'more-stories'], | ||
| }); | ||
| }, 4000); | ||
| setTimeout(() => { | ||
| mockStore.setState({ | ||
| loaded: true, | ||
| muted: false, | ||
| accepted: ['controls'], | ||
| done: ['install-storybook', 'render-component', 'viewports'], | ||
| skipped: ['more-components', 'more-stories', 'install-vitest'], | ||
| }); | ||
| }, 8000); |
There was a problem hiding this comment.
These show be managed using promises somehow, so storybook has any idea that the play function is actually still running; and the steps show up in the interactions panel.
| "./internal/manager/manager-stores": { | ||
| "types": "./dist/manager/manager-stores.d.ts", | ||
| "default": "./dist/manager/manager-stores.js" | ||
| }, |
There was a problem hiding this comment.
Please explain this new entry-point?
| /** | ||
| * Returns the current story index. | ||
| * | ||
| * @returns {API_PreparedStoryIndex | undefined} The current story index, or undefined if not yet | ||
| * loaded. | ||
| */ | ||
| getIndex: () => API_PreparedStoryIndex | undefined; |
There was a problem hiding this comment.
Can you explain why this is a public facing API we need, when we want users to subscribe to state, rather then keep polling.
Is this API even used? In this PR all I can see is this API getting mocked away.
ndelangen
left a comment
ndelangen
left a comment
There was a problem hiding this comment.
I don't think this is ready
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/manager/settings/Checklist/Checklist.tsx (1)
300-309: Replacearia-labelwithariaLabelprop on Button and ToggleButton componentsVerification confirms the review comment is accurate. The Button component at
code/core/src/components/components/Button/Button.tsxdefinesariaLabel?: string | false, and ToggleButton extends ButtonProps, inheriting this prop. The current code in Checklist.tsx at lines 306, 358, 378, 422, and 435 uses rawaria-labelinstead of the expectedariaLabelprop.Per the Button documentation,
ariaLabelis mandatory for buttons without text or with text relying on visual cues. For text-labeled buttons where children provide the accessible name, passariaLabel={false}instead of omitting it.All proposed diffs are correct and align with the established convention documented in the codebase.
♻️ Duplicate comments (10)
code/core/src/components/components/Optional/Optional.tsx (1)
19-54: Address prior review feedback: clarify component purpose and naming.A previous reviewer (ndelangen) asked for clarification on this component's purpose and whether "Optional" is the right name, especially given discussions about whether Storybook should continue providing a public component library.
The component appears to detect overflow and switch between primary content and a fallback. Consider:
- Whether this needs to be in the public API surface
- A more descriptive name that conveys overflow/responsive behavior (e.g.,
OverflowFallback,ResponsiveContent)code/core/src/manager/globals/runtime.ts (1)
10-10: Address prior review feedback: justify manager-stores globalization.A previous reviewer (ndelangen) asked why the extra globalization of manager-stores is required, noting that the file existed before. Please clarify:
- Why manager stores need to be exposed as runtime globals
- What use cases require global access to internal_checklistStore/internal_universalChecklistStore
- Whether this could be achieved through standard imports instead
Also applies to: 42-42
code/.storybook/main.ts (1)
84-86: Address prior review feedback: explain purpose of onboarding example-stories.A previous reviewer (ndelangen) asked for clarification on the purpose of these stories. Please document:
- What role these example stories play in the onboarding flow
- Why they don't have a
titlePrefix(unlike other story directories)- Whether they're meant for demonstration, testing, or user-facing onboarding content
code/core/package.json (1)
121-124: Address prior review feedback: justify new manager-stores export.A previous reviewer (ndelangen) asked for an explanation of this new entry-point. Please clarify:
- Why manager-stores need to be exposed as a public export
- What external consumers (addons, integrations) require access to internal stores
- Whether this is intended for public API usage or internal-only with
internal_naming convention as a signalcode/core/src/manager-api/modules/stories.ts (1)
123-129: Address maintainer's previous concern about this API.A maintainer previously questioned why this public API is needed when users should subscribe to state rather than poll, and noted that the API appears to only be mocked in the PR. Please clarify the use case and verify whether this API is actually consumed.
#!/bin/bash # Search for actual usage of getIndex() beyond mocks and type definitions rg -nP --type=ts --type=tsx 'getIndex\s*\(' -g '!*.stories.tsx' -g '!*.test.ts' -g '!*.mock.ts' -C3code/core/src/components/components/TourGuide/TourTooltip.tsx (1)
60-73: Clarify custom CTA styling and align withButtonaccessibility convention
NextButtonintroduces a custom visual treatment (background, no border/shadow, polished‑based hover colors) and is the only Button in this file that doesn’t explicitly setariaLabel:const NextButton = styled(Button)(({ theme }) => ({ background: theme.color.lightest, border: 'none', boxShadow: 'none', color: theme.base === 'light' ? theme.color.secondary : darken(0.18, theme.color.secondary), '&:hover, &:focus': { background: transparentize(0.1, theme.color.lightest), color: theme.base === 'light' ? lighten(0.1, theme.color.secondary) : darken(0.3, theme.color.secondary), }, })); // ... <NextButton {...primaryProps}>{index + 1 === size ? 'Done' : 'Next'}</NextButton>
- From an a11y standpoint, the text children (“Done”/“Next”) are fine as the accessible name, but to match the Button convention used elsewhere it would be more explicit to pass
ariaLabel={false}on this CTA.- On the visual side, this is still the one button in the tooltip with a bespoke hover/focus treatment. If that’s intentional per design, all good; otherwise, consider reusing an existing Button variant or theme token so we don’t drift away from the shared button patterns.
Also, since this introduces a new dependency on
polished, just ensure it’s already incode/core’s deps and tree‑shaking impact is acceptable.Also applies to: 157-159
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
28-35: Reworkplayto use async/await instead of rawsetTimeoutso Storybook can track itThe Default story currently drives checklist progress via untracked timeouts:
beforeEach: async () => { mockStore.setState({ loaded: true, muted: false, accepted: ['controls'], done: ['install-storybook', 'render-component'], skipped: ['more-components', 'more-stories'], }); }, export const Default = meta.story({ play: () => { setTimeout(() => { mockStore.setState({ loaded: true, muted: false, accepted: ['controls'], done: ['install-storybook', 'render-component', 'viewports'], skipped: ['more-components', 'more-stories'], }); }, 4000); setTimeout(() => { mockStore.setState({ loaded: true, muted: false, accepted: ['controls'], done: ['install-storybook', 'render-component', 'viewports'], skipped: ['more-components', 'more-stories', 'install-vitest'], }); }, 8000); }, });Because
playreturnsvoidand usessetTimeoutdirectly, Storybook has no idea that the story is still “running”, and the interactions panel can’t properly reflect these steps — exactly the concern raised in the earlier review.You can keep the behavior but make it observable by returning a Promise / using
async:-export const Default = meta.story({ - play: () => { - setTimeout(() => { - mockStore.setState({ - loaded: true, - muted: false, - accepted: ['controls'], - done: ['install-storybook', 'render-component', 'viewports'], - skipped: ['more-components', 'more-stories'], - }); - }, 4000); - setTimeout(() => { - mockStore.setState({ - loaded: true, - muted: false, - accepted: ['controls'], - done: ['install-storybook', 'render-component', 'viewports'], - skipped: ['more-components', 'more-stories', 'install-vitest'], - }); - }, 8000); - }, -}); +export const Default = meta.story({ + play: async () => { + const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + + await wait(4000); + mockStore.setState({ + loaded: true, + muted: false, + accepted: ['controls'], + done: ['install-storybook', 'render-component', 'viewports'], + skipped: ['more-components', 'more-stories'], + }); + + await wait(4000); + mockStore.setState({ + loaded: true, + muted: false, + accepted: ['controls'], + done: ['install-storybook', 'render-component', 'viewports'], + skipped: ['more-components', 'more-stories', 'install-vitest'], + }); + }, +});This way Storybook knows the play function is still running, and the interactions tooling can represent the full sequence.
Also applies to: 39-60
code/core/src/manager/globals/exports.ts (1)
313-360: Confirm globals map matches actual exports & intended surfaceThe new entries for
internal_checklistStore/internal_universalChecklistStore, the added internal components (HighlightElement,ListboxHoverItem,Optional,TourGuide,TourTooltip), and the newstorybook/internal/manager/manager-storesblock all look consistent with the rest of the PR, but they do expand the effective public surface of these entrypoints.Two things seem worth double‑checking before merge:
- That
storybook/manager-apiandstorybook/internal/manager/manager-storesactually export exactly the identifiers listed here (including the experimental store helpers), to avoid runtimeundefinedwhen the manager bundler externalizes these modules.- That we indeed want the newly added UI components to be reachable via
storybook/internal/components(even if “internal”), since this becomes a de‑facto supported surface for addons and stories.If either list drifts from the real exports, or if any of these are meant to remain private to core, it would be better to adjust here (or in
sourcefiles.ts) now rather than after release.Also applies to: 485-587, 656-665
code/core/src/components/components/TourGuide/TourGuide.tsx (2)
14-36: Replace@ts-ignoreonStepwith a type‑safe alias derived from Joyride
StepDefinitioncurrently relies on:// @ts-ignore Ignore circular reference Stepinside the
Partial<Pick<...>>, which suppresses type checking againstreact-joyride’sStepshape. That makes it easy for ourStepDefinitionto drift from what Joyride actually expects.You can usually avoid the circularity and keep things type‑safe by deriving the base step type from Joyride’s own props instead of referencing
Stepdirectly, e.g.:type JoyrideStep = ComponentProps<typeof Joyride>['steps'][number]; type StepDefinition = { key?: string; highlight?: string; hideNextButton?: boolean; onNext?: ({ next }: { next: () => void }) => void; } & Partial< Pick< JoyrideStep, | 'content' | 'disableBeacon' | 'disableOverlay' | 'floaterProps' | 'offset' | 'placement' | 'spotlightClicks' | 'styles' | 'target' | 'title' > >;(or an equivalent approach that fits your typings).
This keeps
StepDefinitionin sync with Joyride’s ownstepsprop without needing to suppress the type checker.
173-200: Type therenderhelper explicitly or move it to a separate function
TourGuide.renderis assigned after the function component is defined:let root: ReturnType<typeof createRoot> | null = null; TourGuide.render = (props: ComponentProps<typeof TourGuide>) => { ... };As written,
TourGuideis a plain function component (const TourGuide = (props) => ...), so TypeScript doesn’t know it has arenderproperty; this is likely a type error (“Property 'render' does not exist on type ...”) even though it works at runtime.Two clearer options:
- Give
TourGuidea composite type that includesrender:type TourGuideProps = { step?: string; steps: StepDefinition[]; onNext?: ({ next }: { next: () => void }) => void; onComplete?: () => void; onDismiss?: () => void; }; interface TourGuideComponent { (props: TourGuideProps): JSX.Element | null; render: (props: TourGuideProps) => void; } export const TourGuide: TourGuideComponent = (props) => { // existing component body }; let root: ReturnType<typeof createRoot> | null = null; TourGuide.render = (props) => { // existing render helper body };
- Alternatively, export a separate helper instead of a static:
export const renderTourGuide = (props: TourGuideProps) => { ... };Either way, you avoid hanging an untyped static off a function component and make the public API of this module explicit and type‑checked.
🧹 Nitpick comments (26)
code/core/src/manager/settings/GuidePage.tsx (1)
50-64: Consider replacing deprecated<center>tags with CSS.The file uses the deprecated HTML
<center>tag in three places (lines 51, 53, 58). Consider refactoring to use a styled component withtextAlign: 'center'for modern React/CSS practices.Example refactor:
+const CenteredMessage = styled.div({ + textAlign: 'center', +}); + export const GuidePage = () => { const checklist = useChecklist(); return ( <Container> <Intro> <h1>Guide</h1> <p> Learn the basics. Set up Storybook. You know the drill. This isn't your first time setting up software so get to it! </p> </Intro> <Checklist {...checklist} /> {checklist.openItems.length === 0 ? ( - <center>Your work here is done!</center> + <CenteredMessage>Your work here is done!</CenteredMessage> ) : checklist.muted ? ( - <center> + <CenteredMessage> Want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(false)}>Show in sidebar</Link> - </center> + </CenteredMessage> ) : ( - <center> + <CenteredMessage> Don't want to see this in the sidebar?{' '} <Link onClick={() => checklist.mute(checklist.allItems.map(({ id }) => id))}> Remove from sidebar </Link> - </center> + </CenteredMessage> )} </Container> ); };code/core/src/components/components/Particles.tsx (2)
111-111: Consider using Fisher-Yates shuffle for better randomization.The current
sortRandomlyimplementation usingarray.sort(() => Math.random() - 0.5)is known to produce biased results and doesn't guarantee a uniform distribution. While this isn't critical for visual animations, a Fisher-Yates shuffle would be more correct.Apply this diff if you want more uniform randomization:
-const sortRandomly = (array: any[]) => array.sort(() => Math.random() - 0.5); +const sortRandomly = <T,>(array: T[]): T[] => { + const shuffled = [...array]; + for (let i = shuffled.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; + } + return shuffled; +};
158-158: Replace function name with stable key.Using
Particle.nameas the key is fragile because function names can be mangled or lost during minification, and it doesn't guarantee uniqueness if shapes repeat (though currently they don't after shuffling).Use the index as the key since the particle list is stable:
- return <Particle key={Particle.name} style={style} color={colors[index]} />; + return <Particle key={index} style={style} color={colors[index]} />;code/core/src/components/components/Optional/Optional.tsx (2)
26-31: Simplify ref initialization.Lines 30-31 initialize width refs with
contentRef.current?.offsetWidth ?? 0, butcontentRef.currentis alwaysnullduring the initial render, making this equivalent touseRef(0).Apply this diff to simplify:
- const contentWidth = useRef(contentRef.current?.offsetWidth ?? 0); - const wrapperWidth = useRef(wrapperRef.current?.offsetWidth ?? 0); + const contentWidth = useRef(0); + const wrapperWidth = useRef(0);
33-43: Consider observing content element for complete overflow detection.The ResizeObserver only observes
wrapperRef, so dynamic content size changes (e.g., text updates, child element changes) won't trigger recalculation of overflow. If the content can change size independently of the wrapper, consider observing both refs.Apply this diff to observe both elements:
useEffect(() => { if (contentRef.current && wrapperRef.current) { const resizeObserver = new ResizeObserver(() => { wrapperWidth.current = wrapperRef.current?.offsetWidth || wrapperWidth.current; contentWidth.current = contentRef.current?.offsetWidth || contentWidth.current; setHidden(contentWidth.current > wrapperWidth.current); }); resizeObserver.observe(wrapperRef.current); + resizeObserver.observe(contentRef.current); return () => resizeObserver.disconnect(); } }, []);code/core/src/manager/components/sidebar/useChecklist.ts (2)
140-142: Handle edge case: division by zero in progress calculation.If
availableItems.length === 0, the progress calculation returnsNaN. Consider returning0or100in this case.Apply this diff:
const progress = Math.round( - ((availableItems.length - openItems.length) / availableItems.length) * 100 + availableItems.length > 0 + ? ((availableItems.length - openItems.length) / availableItems.length) * 100 + : 0 );
53-67: Consider adding cycle detection for robustness, though circular dependencies are unlikely given the static configuration nature ofchecklistData.The
checkAvailablefunction lacks cycle detection and could theoretically infinite-loop if circularafterdependencies exist. However, sincechecklistDatais a static configuration constant with no observed circular patterns, this risk is low in practice. The function also silently ignores non-existent dependency IDs (line 62 guard), which is by design but could mask configuration errors.For improved robustness and maintainability, consider:
- Adding a
visitedSet parameter to detect and prevent infinite recursion- Logging a warning when
item.afterreferences a non-existent item ID (useful for catching configuration mistakes)code/addons/onboarding/example-stories/Button.tsx (1)
28-36: Consider restricting props spreading to prevent attribute override.The props spread on line 32 (
{...props}) could allow callers to override thetype="button"attribute, potentially breaking the button's intended behavior. Consider explicitly allowing only safe props.Apply this diff to restrict props spreading:
export const Button = ({ primary = false, size = 'medium', backgroundColor, label, + onClick, ...props }: ButtonProps) => { const mode = primary ? 'storybook-button--primary' : 'storybook-button--secondary'; return ( <button type="button" className={['storybook-button', `storybook-button--${size}`, mode].join(' ')} - style={{ backgroundColor }} - {...props} + {...(backgroundColor && { style: { backgroundColor } })} + onClick={onClick} > {label} </button> ); };code/addons/onboarding/example-stories/button.css (1)
1-30: Add focus and hover states for accessibility and UX best practices.The button CSS is an educational example in the onboarding addon. Color contrast verification confirms both variants meet WCAG AA standards:
- Primary button: 5.92:1 contrast ✓
- Secondary button: 12.63:1 contrast ✓
However, adding focus and hover states would improve the example to showcase accessibility and interactive feedback best practices. Consider adding:
:focus-visibleindicator for keyboard navigation:hoverstate for visual feedbackcode/core/src/core-server/utils/checklist.ts (3)
12-20: Makeequals’s object branch symmetric to avoid surprising results if reusedFor the current checklist state (booleans and string arrays), this
equalsworks fine, but the generic object branch only iteratesObject.keys(a)and ignores extra keys onb. If this helper ever gets reused for richer objects,equals(a, b)could incorrectly returntruewhenbhas additional properties.Consider tightening the object path so both sides’ keys are compared:
- if (a && b && typeof a === 'object' && typeof b === 'object') { - return Object.keys(a).every((key) => equals(a[key as keyof T], b[key as keyof T])); - } + if (a && b && typeof a === 'object' && typeof b === 'object') { + const aKeys = Object.keys(a as any); + const bKeys = Object.keys(b as any); + if (aKeys.length !== bKeys.length) return false; + + return aKeys.every((key) => + equals( + (a as any)[key], + (b as any)[key] + ) + ); + }
33-56: Avoid floating Promises when persisting user/project checklist stateBoth persistence helpers are likely async:
settings.save()usually returns aPromise.cache.set('state', { done })also tends to be async for filesystem caches.Calling them without
awaitor at leastvoidrisks floating Promises and silent failures if they ever reject. Sincestore.onStateChangeexpects a sync callback, explicitly discarding the Promise is clearer and keeps linters happy:- const setState = ({ + const setState = ({ muted = false, accepted = [], skipped = [], }: NonNullable<typeof settings.value.checklist>) => { settings.value.checklist = { muted, accepted, skipped }; - settings.save(); + void settings.save(); }; ... - const setState = ({ done }: Pick<StoreState, 'done'>) => cache.set('state', { done }); + const setState = ({ done }: Pick<StoreState, 'done'>) => { + void cache.set('state', { done }); + };This keeps the fire-and-forget behavior but makes the intent explicit.
60-70: Guard telemetry emission and explicitly ignore its Promise
store.onStateChangecurrently:
- Always calls
telemetry('onboarding-checklist', ...), even if none ofmuted/accepted/skipped/donechanged.- Calls an async function without awaiting or voiding it.
To avoid unnecessary telemetry calls and floating Promises:
const { muted, accepted, skipped, done } = state; - const changedProperties = Object.entries({ muted, accepted, skipped, done }).filter( + const changedProperties = Object.entries({ muted, accepted, skipped, done }).filter( ([key, value]) => !equals(value, previousState[key as keyof StoreState]) ); - telemetry('onboarding-checklist', Object.fromEntries(changedProperties)); + if (changedProperties.length > 0) { + void telemetry('onboarding-checklist', Object.fromEntries(changedProperties)); + }That keeps behavior the same when something actually changes, but reduces noise and clarifies that telemetry is intentionally fire-and-forget.
code/core/src/components/components/TourGuide/HighlightElement.tsx (1)
69-147: Confirm assumptions about when the target element exists
HighlightElementlooks uptargetSelectoronce on mount:const element = document.querySelector<HTMLElement>(targetSelector); if (!element || !element.parentElement) { return; }If the target is rendered asynchronously (or in a different frame) after this effect runs, the highlight will never appear and there’s no retry.
If the intended usage always mounts
HighlightElementafter the target is in the DOM, this is fine; otherwise, consider a small retry loop or aMutationObserver/polling approach so late-arriving targets can still be highlighted.As a minor nit, the pulsating keyframes
<style>is intentionally global and guarded byHIGHLIGHT_KEYFRAMES_ID, so it’s injected once and never removed. That’s OK, but if you expect many mount/unmount cycles you might want to mirror the TourTooltip pattern and clean it up on unmount.code/core/src/manager/components/sidebar/Sidebar.stories.tsx (1)
13-16: Seeded checklist store looks good; consider giving API mocks sensible defaultsThe changes here look coherent:
internal_universalChecklistStore.setState({ loaded, muted, accepted, done, skipped })seeds the checklist store with a completeStoreStatebefore each story.managerContext.apinow includesgetData,getIndex, andnavigatemocks, which should prevent undefined-function errors as Sidebar gains more dependencies on the manager API.One thing to double‑check: if
Sidebarreads values fromapi.getIndex()orapi.getData(), the current mocks will returnundefined. If it expects anIndexHashor similar, consider returningindex(or{}) here instead of a bare Jest mock, just to keep the stories closer to real runtime behavior.Also applies to: 36-56, 117-126
code/core/src/components/components/TourGuide/TourTooltip.tsx (1)
104-115: Avoid multiple<style>elements with the same onboarding arrow IDThe arrow styling effect now uses a fixed ID:
const ONBOARDING_ARROW_STYLE_ID = 'storybook-onboarding-arrow-style'; useEffect(() => { const style = document.createElement('style'); style.id = ONBOARDING_ARROW_STYLE_ID; style.innerHTML = `...`; document.head.appendChild(style); return () => document.getElementById(ONBOARDING_ARROW_STYLE_ID)?.remove(); }, []);If multiple
TourTooltipinstances are mounted concurrently, each will append a<style>with the sameid, and the cleanup of one will only remove the first one found. It’s unlikely you’ll have overlapping tours, but it’s easy to guard:useEffect(() => { - const style = document.createElement('style'); - style.id = ONBOARDING_ARROW_STYLE_ID; - style.innerHTML = `...`; - document.head.appendChild(style); + if (!document.getElementById(ONBOARDING_ARROW_STYLE_ID)) { + const style = document.createElement('style'); + style.id = ONBOARDING_ARROW_STYLE_ID; + style.innerHTML = `...`; + document.head.appendChild(style); + } return () => document.getElementById(ONBOARDING_ARROW_STYLE_ID)?.remove(); }, []);That keeps the behavior (single global style) while avoiding duplicated nodes with the same ID.
Also applies to: 132-134
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (2)
8-16: Verify mocked manager API methods return whatChecklistModuleexpectsThe manager context for these stories now stubs several APIs as bare Jest mocks:
const managerContext: any = { state: {}, api: { getData: fn().mockName('api::getData'), getIndex: fn().mockName('api::getIndex'), getUrlState: fn().mockName('api::getUrlState'), navigate: fn().mockName('api::navigate'), on: fn().mockName('api::on'), }, };This is fine if
ChecklistModuleonly needs these functions to exist (e.g., for event wiring), but if it reads their return values (particularlygetIndex/getUrlState), returningundefinedmay lead to subtle runtime issues in stories.It’s worth double‑checking usage and, if needed, swapping some of these to simple implementations that return reasonable defaults (e.g.,
{}or a minimalIndexHash) to better mirror real manager behavior.
19-27: Narrow story double‑wraps the module with width constraintsMeta‑level decorator:
decorators: [ (Story) => ( <ManagerContext.Provider value={managerContext}> <div style={{ maxWidth: 300 }}>{Story()}</div> </ManagerContext.Provider> ), ],Narrow story:
export const Narrow = meta.story({ decorators: [(Story) => <div style={{ maxWidth: 200 }}>{Story()}</div>], });This results in a 200px container nested inside a 300px container. That’s harmless but slightly redundant. If you care about keeping the DOM clean, you could alternatively adjust the meta decorator for Narrow via args/parameters or replace the outer width constraint in the Narrow story instead of stacking them.
Also applies to: 62-64
code/addons/onboarding/example-stories/Button.stories.tsx (1)
28-75: Tidy up extra story variants with non‑descriptive namesThe
Ad,Df, andGdfstories have identical args and non‑descriptive names, so they’ll show up as confusing entries in the onboarding “Example/Button” group without adding signal.Unless they’re meant to represent distinct scenarios, consider removing them or renaming and differentiating them (e.g., specific visual states) so the onboarding template stays focused and easy to scan.
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
12-32: Verify arg/store overlap and arg precedence foravailableItemsThe story locally derives
availableItems(including section metadata and all theis*flags) and then passes:args: { availableItems, ...checklistStore }Two points to double‑check:
- If
checklistStoreexposes any state keys that overlap with these args (especiallyavailableItemsor related flags), the spread order here means the store’s values will override the local story defaults. If you want the story’savailableItemsto win, the order should be{ ...checklistStore, availableItems }instead.- Longer term, if the store is also responsible for shaping
availableItems, consider deriving the story’s list through the same store API to avoid type/shape drift between this story and the runtime logic.If the store currently only contains methods and unrelated state, this is fine as‑is; just worth confirming the intended precedence.
Also applies to: 41-46, 62-64
code/core/src/components/components/TourGuide/TourGuide.tsx (1)
56-75: Add cleanup fortimeoutRefto avoid setState after unmount
updateStepIndexuses a timeout to briefly hide the tooltip when switching steps:timeoutRef.current = setTimeout(setStepIndex, 300, index);but the component never clears that timeout on unmount. If the component unmounts during the 300ms window, React can emit “setState on unmounted component” warnings.
A small cleanup effect keeps this tight:
const timeoutRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); const updateStepIndex = useCallback((index: number) => { clearTimeout(timeoutRef.current); setStepIndex((current) => { @@ // Briefly hide the tour tooltip while switching steps timeoutRef.current = setTimeout(setStepIndex, 300, index); return null; }); }); -}, []); +}, []); + +useEffect( + () => () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }, + [] +);Not critical for correctness, but it avoids warnings and tightens lifecycle management.
code/addons/onboarding/src/Onboarding.tsx (1)
190-234: Null‑guard thedocument.querySelectorcalls incontrolsTour
controlsTour[0].onNextandcontrolsTour[1].onNextassume the queried elements always exist; if DOM structure changes,input.click()/button.click()could throw.You can make this defensive with minimal change:
- onNext: () => { - const input = document.querySelector('#control-primary') as HTMLInputElement; - input.click(); - }, + onNext: () => { + const input = document.querySelector<HTMLInputElement>('#control-primary'); + input?.click(); + }, ... - onNext: () => { - const button = document.querySelector( - 'button[aria-label="Create new story with these settings"]' - ) as HTMLButtonElement; - button.click(); - }, + onNext: () => { + const button = document.querySelector<HTMLButtonElement>( + 'button[aria-label="Create new story with these settings"]' + ); + button?.click(); + },This keeps behavior identical when elements are present and avoids hard crashes otherwise.
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
154-174: Initial checklist items are delayed by the 2s timeoutThe
useEffectthat re‑mapsitemsand thensetTimeout(setItems, 2000, itemsWithRef)means that on first loaditemsstays[]for ~2s beforeitemsWithRefis applied. GivenhasTasksis derived fromitems.length, the checklist module remains collapsed/hidden briefly even whennextItemsis already available.If you want new tasks to show as soon as they exist while still delaying replacement after completion, you could special‑case the initial empty state, e.g.:
useEffect(() => { - setItems((current) => - current.map((item) => ({ - ...item, - isCompleted: accepted.includes(item.id) || done.includes(item.id), - isSkipped: skipped.includes(item.id), - })) - ); + setItems((current) => { + if (current.length === 0) { + return itemsWithRef; + } + return current.map((item) => ({ + ...item, + isCompleted: accepted.includes(item.id) || done.includes(item.id), + isSkipped: skipped.includes(item.id), + })); + }); const timeout = setTimeout(setItems, 2000, itemsWithRef); return () => clearTimeout(timeout); }, [accepted, done, skipped, itemsWithRef]);Please confirm this matches the intended UX before changing.
code/core/src/manager/settings/Checklist/checklistData.tsx (4)
301-309: Diff syntax may not render with highlighting.The code snippet uses
language="jsx"but contains diff-style markers (-and+). SyntaxHighlighter won't apply diff highlighting with the jsx language.Consider changing to
language="diff"for proper diff highlighting:- <CodeSnippet language="jsx"> + <CodeSnippet language="diff">Or remove the diff markers and use comments instead if jsx highlighting is preferred.
567-569: Semantically incorrect HTML: inline<pre>tag.The
<pre>tag is a block-level element and shouldn't be used inline within a paragraph. Use<code>for inline code instead.Apply this diff:
<p> You can automate all of Storybook's tests by using Chromatic or by running the - <pre>vitest --project storybook</pre> command in your CI scripts. + <code>vitest --project storybook</code> command in your CI scripts. </p>
625-628: Semantically incorrect HTML: inline<pre>tag.The
<pre>tag is a block-level element and shouldn't be used inline within a paragraph. Use<code>for inline code instead.Apply this diff:
<p> - That tag can also be applied in <pre>.storybook/preview.js</pre>, to generate + That tag can also be applied in <code>.storybook/preview.js</code>, to generate documentation for all components. </p>
666-669: Semantically incorrect HTML: inline<pre>tag.The
<pre>tag is a block-level element and shouldn't be used inline within a paragraph. Use<code>for inline code instead.Apply this diff:
<p> - Run <pre>npx storybook build</pre> in CI and deploy it using services like + Run <code>npx storybook build</code> in CI and deploy it using services like Chromatic, Vercel, or Netlify. </p>
| const hasCompletedSurvey = await new Promise<boolean>((resolve) => { | ||
| const unsubscribe = checklistStore.onStateChange(({ loaded, accepted }) => { | ||
| if (loaded) { | ||
| unsubscribe(); | ||
| resolve(accepted.includes('onboarding-survey')); | ||
| } | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Potential hang while awaiting hasCompletedSurvey
hasCompletedSurvey is resolved only inside the onStateChange handler when { loaded } becomes true. If checklistStore is already loaded before this code subscribes, or if onStateChange does not replay the current state, this await may never resolve and block rendering Survey/Onboarding for that session.
Consider making this resilient to “already loaded” and failure cases, for example:
const hasCompletedSurvey = await new Promise<boolean>((resolve) => {
// If available, check current state first
const state = checklistStore.getState?.();
if (state?.loaded) {
resolve(state.accepted.includes('onboarding-survey'));
return;
}
const unsubscribe = checklistStore.onStateChange(({ loaded, accepted }) => {
if (!loaded) return;
unsubscribe();
resolve(accepted.includes('onboarding-survey'));
});
// Optional: add a timeout fallback if `loaded` never becomes true.
});Please verify checklistStore exposes a getState (or equivalent) and that onStateChange semantics match this usage.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine manager.tsx to understand checklistStore usage and imports
cat code/addons/onboarding/src/manager.tsx
Length of output: 2464
🏁 Script executed:
#!/bin/bash
# Search for checklistStore definition or import statements
rg -n "checklistStore" code/addons/onboarding/src/manager.tsx -B2 -A5Length of output: 633
🏁 Script executed:
#!/bin/bash
# Find where checklistStore is defined or exported
fd -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "checklistStore\s*=" -g "!node_modules" | head -20Length of output: 261
🏁 Script executed:
#!/bin/bash
# Search for store definitions or exports related to checklist
rg -n "export.*checklistStore|export.*Checklist.*[Ss]tore" code/addons/onboarding/src/ -A10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for store implementation patterns with onStateChange and getState
rg -n "onStateChange|getState" code/addons/onboarding/src/ -B2 -A3 --type ts --type tsxLength of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the actual checklist store implementation
cat code/core/src/manager-api/stores/checklist.ts
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Check the mock file for store interface hints
cat code/core/src/manager/manager-stores.mock.tsLength of output: 2203
🏁 Script executed:
#!/bin/bash
# Search for where internal_universalChecklistStore is created/defined
rg -n "internal_universalChecklistStore" code/core/src/ -A5 -B5Length of output: 11167
🏁 Script executed:
#!/bin/bash
# Look for universal store or store factory patterns
rg -n "onStateChange|UniversalChecklist" code/core/src/manager-api/ -lLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and read the createChecklistStore function
cat code/core/src/shared/checklist-store.ts
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Search for UniversalStore definition
rg -n "class UniversalStore|interface UniversalStore|export.*UniversalStore" code/core/src/shared/ -A15Length of output: 7323
🏁 Script executed:
#!/bin/bash
# Find experimental_UniversalStore implementation
rg -n "experimental_UniversalStore|class.*UniversalStore" code/core/src/ -l | head -10Length of output: 367
🏁 Script executed:
#!/bin/bash
# Check for onStateChange implementation in universal store
rg -n "onStateChange" code/core/src/shared/ -B3 -A8Length of output: 11868
---
🏁 Script executed:
```shell
#!/bin/bash
# Find getState method in UniversalStore
rg -n "getState" code/core/src/shared/universal-store/index.ts -B3 -A8 | head -50
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Look for UNIVERSAL_CHECKLIST_STORE_OPTIONS definition
fd -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "UNIVERSAL_CHECKLIST_STORE_OPTIONS"Length of output: 352
🏁 Script executed:
#!/bin/bash
# Check subscribe method - does it support replay?
rg -n "public subscribe|private subscribe" code/core/src/shared/universal-store/index.ts -A15 | head -80Length of output: 856
---
🏁 Script executed:
```shell
#!/bin/bash
# Find UNIVERSAL_CHECKLIST_STORE_OPTIONS definition
cat code/core/src/shared/checklist-store/index.ts
Length of output: 2403
🏁 Script executed:
#!/bin/bash
# Look for subscribe method complete implementation to confirm no replay
rg -n "this.listeners" code/core/src/shared/universal-store/index.ts -B2 -A2 | head -40Length of output: 792
🏁 Script executed:
#!/bin/bash
# Check if there's any state-on-subscribe logic in subscribe
rg -n "subscribe.*replay|onStateChange.*initial|subscribe.*current" code/core/src/shared/universal-store/ -iLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where loaded state is set to true
rg -n "loaded.*true|setState.*loaded" code/core/src/ -B3 -A3 | grep -A5 -B5 "loaded.*true"
Length of output: 5743
🏁 Script executed:
#!/bin/bash
# Look for initialization patterns in manager or checklist stores
rg -n "loaded.*:.*true" code/ -g "!node_modules" -g "!dist" --type ts --type tsx 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's server-side initialization that sets loaded=true
rg -n "loaded" code/core/src/core-server/utils/checklist.ts -B2 -A2Length of output: 186
Potential hang while awaiting hasCompletedSurvey — confirmed
The issue identified in the review is valid. The store initialization in code/core/src/core-server/utils/checklist.ts sets loaded: true, which can occur before the addon's addons.register() callback executes. Since onStateChange only emits on future state changes and does not replay the current state, if the store is already in the loaded: true state when the subscription occurs, the listener will never fire and the promise will hang indefinitely.
The fix should check the current state before subscribing. The suggested solution is appropriate: use getState() to verify if the store is already loaded, and fall back to subscription only if needed. This will prevent the promise from blocking rendering.
🤖 Prompt for AI Agents
In code/addons/onboarding/src/manager.tsx around lines 33 to 41, the Promise
awaiting hasCompletedSurvey can hang because onStateChange only emits future
updates and the store may already be loaded; fix by first calling
checklistStore.getState() and if loaded resolve immediately with whether
accepted includes 'onboarding-survey', otherwise subscribe via onStateChange,
and still unsubscribe when fired or on cleanup; ensure the Promise resolves in
both paths so it never blocks.
| const disableOnboarding = useCallback(() => { | ||
| // remove onboarding query parameter from current url | ||
| const url = new URL(window.location.href); | ||
| // @ts-expect-error (not strict) | ||
| const path = decodeURIComponent(url.searchParams.get('path')); | ||
| url.search = `?path=${path}&onboarding=false`; | ||
| history.replaceState({}, '', url.href); | ||
| api.setQueryParams({ onboarding: 'false' }); | ||
| }, [api]); |
There was a problem hiding this comment.
Fix unsafe URL manipulation and remove @ts-expect-error.
Lines 18-20 have a critical issue: if the path query parameter is missing, searchParams.get('path') returns null, which would cause decodeURIComponent(null) to throw a TypeError. The @ts-expect-error is suppressing this legitimate type error.
Apply this diff to handle the missing path parameter safely:
const disableOnboarding = useCallback(() => {
// remove onboarding query parameter from current url
const url = new URL(window.location.href);
- // @ts-expect-error (not strict)
- const path = decodeURIComponent(url.searchParams.get('path'));
- url.search = `?path=${path}&onboarding=false`;
+ const pathParam = url.searchParams.get('path');
+ if (pathParam) {
+ const path = decodeURIComponent(pathParam);
+ url.search = `?path=${path}&onboarding=false`;
+ } else {
+ url.searchParams.set('onboarding', 'false');
+ }
history.replaceState({}, '', url.href);
api.setQueryParams({ onboarding: 'false' });
}, [api]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const disableOnboarding = useCallback(() => { | |
| // remove onboarding query parameter from current url | |
| const url = new URL(window.location.href); | |
| // @ts-expect-error (not strict) | |
| const path = decodeURIComponent(url.searchParams.get('path')); | |
| url.search = `?path=${path}&onboarding=false`; | |
| history.replaceState({}, '', url.href); | |
| api.setQueryParams({ onboarding: 'false' }); | |
| }, [api]); | |
| const disableOnboarding = useCallback(() => { | |
| // remove onboarding query parameter from current url | |
| const url = new URL(window.location.href); | |
| const pathParam = url.searchParams.get('path'); | |
| if (pathParam) { | |
| const path = decodeURIComponent(pathParam); | |
| url.search = `?path=${path}&onboarding=false`; | |
| } else { | |
| url.searchParams.set('onboarding', 'false'); | |
| } | |
| history.replaceState({}, '', url.href); | |
| api.setQueryParams({ onboarding: 'false' }); | |
| }, [api]); |
| export const Header = ({ | ||
| hasClose = true, | ||
| onClose, | ||
| ...props | ||
| }: React.ComponentProps<typeof Col> & { hasClose?: boolean }) => ( | ||
| }: React.ComponentProps<typeof Col> & { hasClose?: boolean; onClose?: () => void }) => ( | ||
| <Row> | ||
| <Col {...props} /> | ||
| {hasClose && <Close />} | ||
| {hasClose && <Close onClick={onClose} />} | ||
| </Row> | ||
| ); |
There was a problem hiding this comment.
Critical: onClose callback is never invoked.
The Header component passes onClose as onClick to the Close component (line 244), but the Close component ignores the onClick prop in its default rendering path (line 192). The button only calls close from context, so the onClose callback will never be invoked.
Apply this diff to fix the Close component to honor the onClick prop:
export const Close = ({ asChild, children, onClick, ...props }: CloseProps) => {
const { close } = useContext(ModalContext);
if (asChild && React.isValidElement(children)) {
const handleClick = (event: React.MouseEvent) => {
onClick?.(event);
children.props.onClick?.(event);
close?.();
};
return React.cloneElement(children, {
...props,
onClick: handleClick,
});
}
+ const handleClick = (event: React.MouseEvent) => {
+ onClick?.(event);
+ close?.();
+ };
+
return (
<Button
padding="small"
ariaLabel="Close modal"
variant="ghost"
shortcut={['Escape']}
- onClick={close}
+ onClick={handleClick}
>
<CrossIcon />
</Button>
);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/components/components/Modal/Modal.styled.tsx around lines 237
to 246, the Header passes onClose to the Close component but Close ignores that
prop; update the Close component implementation (the default rendering path
around line ~192) to accept an optional onClick prop and invoke it when clicked
(in addition to calling the context close function) and ensure the onClick prop
is forwarded to the rendered button element so the passed onClose callback is
actually executed.
| const Donut = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 90 90" xmlns="http://www.w3.org/2000/svg" color="red" {...props}> | ||
| <path d="M45 0c24.853 0 45 20.147 45 45S69.853 90 45 90 0 69.853 0 45 20.147 0 45 0zm.5 27C35.283 27 27 35.283 27 45.5S35.283 64 45.5 64 64 55.717 64 45.5 55.717 27 45.5 27z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const L = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 55 89" xmlns="http://www.w3.org/2000/svg" color="#66BF3C" {...props}> | ||
| <path d="M55 3v83a3 3 0 01-3 3H3a3 3 0 01-3-3V64a3 3 0 013-3h21a3 3 0 003-3V3a3 3 0 013-3h22a3 3 0 013 3z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const Slice = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 92 92" xmlns="http://www.w3.org/2000/svg" color="#FF4785" {...props}> | ||
| <path d="M92 89V3c0-3-2.056-3-3-3C39.294 0 0 39.294 0 89c0 0 0 3 3 3h86a3 3 0 003-3z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const Square = ({ style, ...props }: ComponentProps<typeof Shape>) => ( | ||
| <Shape | ||
| viewBox="0 0 90 90" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| color="#1EA7FD" | ||
| {...props} | ||
| style={{ borderRadius: 5, ...style }} | ||
| > | ||
| <path d="M0 0h90v90H0z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const Triangle = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 96 88" xmlns="http://www.w3.org/2000/svg" color="#FFAE00" {...props}> | ||
| <path d="M50.63 1.785l44.928 81.77A3 3 0 0192.928 88H3.072a3 3 0 01-2.629-4.445l44.929-81.77a3 3 0 015.258 0z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const T = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 92 62" xmlns="http://www.w3.org/2000/svg" color="#FC521F" {...props}> | ||
| <path d="M63 3v25a3 3 0 003 3h23a3 3 0 013 3v25a3 3 0 01-3 3H3a3 3 0 01-3-3V34a3 3 0 013-3h24a3 3 0 003-3V3a3 3 0 013-3h27a3 3 0 013 3z" /> | ||
| </Shape> | ||
| ); | ||
|
|
||
| const Z = (props: ComponentProps<typeof Shape>) => ( | ||
| <Shape viewBox="0 0 56 90" xmlns="http://www.w3.org/2000/svg" color="#6F2CAC" {...props}> | ||
| <path d="M28 3v25a3 3 0 003 3h22a3 3 0 013 3v53a3 3 0 01-3 3H31a3 3 0 01-3-3V62a3 3 0 00-3-3H3a3 3 0 01-3-3V3a3 3 0 013-3h22a3 3 0 013 3z" /> | ||
| </Shape> | ||
| ); |
There was a problem hiding this comment.
Fix the hardcoded colors and shape count mismatch.
Two issues here:
-
Hardcoded colors are overridden: Each shape component has a hardcoded
colorprop (e.g.,color="red"on line 18,color="#66BF3C"on line 24), but these are always overridden by thecolorsarray passed from the parentParticlescomponent (line 158:color={colors[index]}). This creates confusion about which colors are actually used. -
Shape count mismatch: Only 7 shapes are defined here, but
NUM_OF_PARTICLESis set to 8 (line 105). This will causeshapes[7]to beundefined, leading to a runtime error when rendering the 8th particle.
Apply this diff to fix both issues:
const Donut = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 90 90" xmlns="http://www.w3.org/2000/svg" color="red" {...props}>
+ <Shape viewBox="0 0 90 90" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M45 0c24.853 0 45 20.147 45 45S69.853 90 45 90 0 69.853 0 45 20.147 0 45 0zm.5 27C35.283 27 27 35.283 27 45.5S35.283 64 45.5 64 64 55.717 64 45.5 55.717 27 45.5 27z" />
</Shape>
);
const L = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 55 89" xmlns="http://www.w3.org/2000/svg" color="#66BF3C" {...props}>
+ <Shape viewBox="0 0 55 89" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M55 3v83a3 3 0 01-3 3H3a3 3 0 01-3-3V64a3 3 0 013-3h21a3 3 0 003-3V3a3 3 0 013-3h22a3 3 0 013 3z" />
</Shape>
);
const Slice = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 92 92" xmlns="http://www.w3.org/2000/svg" color="#FF4785" {...props}>
+ <Shape viewBox="0 0 92 92" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M92 89V3c0-3-2.056-3-3-3C39.294 0 0 39.294 0 89c0 0 0 3 3 3h86a3 3 0 003-3z" />
</Shape>
);
const Square = ({ style, ...props }: ComponentProps<typeof Shape>) => (
<Shape
viewBox="0 0 90 90"
xmlns="http://www.w3.org/2000/svg"
- color="#1EA7FD"
{...props}
style={{ borderRadius: 5, ...style }}
>
<path d="M0 0h90v90H0z" />
</Shape>
);
const Triangle = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 96 88" xmlns="http://www.w3.org/2000/svg" color="#FFAE00" {...props}>
+ <Shape viewBox="0 0 96 88" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M50.63 1.785l44.928 81.77A3 3 0 0192.928 88H3.072a3 3 0 01-2.629-4.445l44.929-81.77a3 3 0 015.258 0z" />
</Shape>
);
const T = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 92 62" xmlns="http://www.w3.org/2000/svg" color="#FC521F" {...props}>
+ <Shape viewBox="0 0 92 62" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M63 3v25a3 3 0 003 3h23a3 3 0 013 3v25a3 3 0 01-3 3H3a3 3 0 01-3-3V34a3 3 0 013-3h24a3 3 0 003-3V3a3 3 0 013-3h27a3 3 0 013 3z" />
</Shape>
);
const Z = (props: ComponentProps<typeof Shape>) => (
- <Shape viewBox="0 0 56 90" xmlns="http://www.w3.org/2000/svg" color="#6F2CAC" {...props}>
+ <Shape viewBox="0 0 56 90" xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M28 3v25a3 3 0 003 3h22a3 3 0 013 3v53a3 3 0 01-3 3H31a3 3 0 01-3-3V62a3 3 0 00-3-3H3a3 3 0 01-3-3V3a3 3 0 013-3h22a3 3 0 013 3z" />
</Shape>
);
+
+const Circle = (props: ComponentProps<typeof Shape>) => (
+ <Shape viewBox="0 0 90 90" xmlns="http://www.w3.org/2000/svg" {...props}>
+ <circle cx="45" cy="45" r="45" />
+ </Shape>
+);Then update line 122 to include the new shape:
- const shapes = sortRandomly([Donut, L, Slice, Square, Triangle, T, Z]);
+ const shapes = sortRandomly([Donut, L, Slice, Square, Triangle, T, Z, Circle]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/components/components/Particles.tsx around lines 17–63, several
Shape components include hardcoded color props which are always overridden by
the parent colors array, and only 7 shape components are defined while
NUM_OF_PARTICLES is 8 causing an out-of-bounds access. Remove the hardcoded
color=... attributes from each Shape declaration so the parent-provided color is
authoritative, add a new eighth Shape component (e.g., Diamond or Hexagon)
matching the same pattern, and then update the shapes array at line 122 to
include the new component so its length equals NUM_OF_PARTICLES (8).
| const shapes = sortRandomly([Donut, L, Slice, Square, Triangle, T, Z]); | ||
| const colors = sortRandomly([ | ||
| '#FF0000', | ||
| '#FF4787', | ||
| '#66BF3C', | ||
| '#1EA7FD', | ||
| '#FC521F', | ||
| '#6F2CAC', | ||
| '#FFAE00', | ||
| ]); |
There was a problem hiding this comment.
Move array initialization outside render or use useMemo.
The shapes and colors arrays are created and randomized on every render, which defeats the purpose of using memo on this component. This causes unnecessary work and can lead to animation inconsistencies if the component re-renders.
Apply this diff to fix the performance issue:
+const shapes = sortRandomly([Donut, L, Slice, Square, Triangle, T, Z, Circle]);
+const colors = sortRandomly([
+ '#FF0000',
+ '#FF4785',
+ '#66BF3C',
+ '#1EA7FD',
+ '#FC521F',
+ '#6F2CAC',
+ '#FFAE00',
+ '#37D5D3',
+]);
+
export const Particles = memo(function Particles({
anchor: Anchor,
}: {
anchor: React.ElementType;
}) {
const anchorRef = useRef<HTMLDivElement>(null);
const [left, setLeft] = useState(0);
const [top, setTop] = useState(0);
- const shapes = sortRandomly([Donut, L, Slice, Square, Triangle, T, Z]);
- const colors = sortRandomly([
- '#FF0000',
- '#FF4787',
- '#66BF3C',
- '#1EA7FD',
- '#FC521F',
- '#6F2CAC',
- '#FFAE00',
- ]);
-
useLayoutEffect(() => {Note: I've also added an 8th color ('#37D5D3' - seafoam from the theme palette) to match the 8 particles after adding the Circle shape.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/components/components/Particles.tsx around lines 122 to 131,
the shapes and colors arrays are being created and randomized on every render;
move their initialization out of the render path by wrapping the arrays in
useMemo with an empty dependency array so they are computed once (or hoist them
to module scope if hooks are unsuitable), include the new Circle shape in the
shapes list to match particle count, and add the 8th color '#37D5D3' to the
colors array; ensure you call sortRandomly inside useMemo and keep the
dependency array empty to avoid re-randomizing on re-renders.
| const { itemsById, sectionsById } = useMemo( | ||
| () => | ||
| Object.fromEntries( | ||
| sections.map(({ items, ...section }) => { | ||
| const progress = | ||
| (items.reduce((a, b) => (isCompleted(b.id) ? a + 1 : a), 0) / items.length) * 100; | ||
| const itemIds = items.map(({ id }) => id); | ||
| return [section.id, { ...section, itemIds, progress }]; | ||
| }) | ||
| availableItems.reduce<{ | ||
| itemsById: Record<ChecklistItem['id'], ChecklistItem>; | ||
| sectionsById: Record<ChecklistSection['id'], ChecklistSection>; | ||
| }>( | ||
| (acc, item) => { | ||
| acc.itemsById[item.id] = item; | ||
| const { sectionId: id, sectionTitle: title } = item; | ||
| acc.sectionsById[id] = acc.sectionsById[id] ?? { id, title, itemIds: [] }; | ||
| acc.sectionsById[id].itemIds.push(item.id); | ||
| return acc; | ||
| }, | ||
| { itemsById: {}, sectionsById: {} } | ||
| ), | ||
| [sections, isCompleted] | ||
| [availableItems] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Guard itemsById access when building locked‑item tooltip text
The locked tooltip builds its message using:
note={`Complete ${isLockedBy
.map((id) => `“${itemsById[id].label}”`)
.join(', ')} first`}If isLockedBy ever contains an id that is not present in itemsById (e.g., a dependency that isn’t in availableItems), this will throw at runtime.
You can make this safer while preserving behavior:
- note={`Complete ${isLockedBy.map((id) => `“${itemsById[id].label}”`).join(', ')} first`}
+ note={`Complete ${isLockedBy
+ .map((id) => itemsById[id]?.label)
+ .filter((label): label is string => !!label)
+ .map((label) => `“${label}”`)
+ .join(', ')} first`}If your data model guarantees all isLockedBy ids are always in availableItems, this will be a no‑op in practice but protects against future changes.
Also applies to: 370-373
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the Checklist.tsx file and examine the problematic lines
find . -name "Checklist.tsx" -type f | head -5
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Get content around lines 242-258 and 370-373 from Checklist.tsx
if [ -f "code/core/src/manager/settings/Checklist/Checklist.tsx" ]; then
echo "=== Lines 240-260 (useMemo block) ==="
sed -n '240,260p' "code/core/src/manager/settings/Checklist/Checklist.tsx"
echo ""
echo "=== Lines 365-375 (tooltip area) ==="
sed -n '365,375p' "code/core/src/manager/settings/Checklist/Checklist.tsx"
fiLength of output: 1413
🏁 Script executed:
#!/bin/bash
# Search for isLockedBy definition and how it's used
rg -n 'isLockedBy' code/core/src/manager/settings/Checklist/Checklist.tsx -B2 -A2Length of output: 827
🏁 Script executed:
#!/bin/bash
# Search for itemsById access patterns to see if there are already guards elsewhere
rg -n 'itemsById\[' code/core/src/manager/settings/Checklist/Checklist.tsx -B1 -A1Length of output: 551
---
🏁 Script executed:
```shell
#!/bin/bash
# Get more context around the destructuring on line 321 to see where isLockedBy comes from
sed -n '310,335p' "code/core/src/manager/settings/Checklist/Checklist.tsx"
Length of output: 956
🏁 Script executed:
#!/bin/bash
# Look for the function signature and parameters to understand where data flows from
sed -n '1,50p' "code/core/src/manager/settings/Checklist/Checklist.tsx"Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Search for where ChecklistItemWithDependencies or similar type is used (might have isLockedBy)
rg -n 'isLockedBy' code/core/src/manager/settings/Checklist/ -A5 -B5 | head -80Length of output: 4179
🏁 Script executed:
#!/bin/bash
# Check if there's any filtering or validation of isLockedBy before line 371
sed -n '300,375p' "code/core/src/manager/settings/Checklist/Checklist.tsx"Length of output: 3758
---
🏁 Script executed:
```shell
#!/bin/bash
# Find useChecklist hook to see how isLockedBy is populated
find code/core/src -name "*useChecklist*" -type f
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Look for the component signature to see where items/availableItems come from
rg -n "export.*Checklist.*=" code/core/src/manager/settings/Checklist/Checklist.tsx | head -10Length of output: 31
🏁 Script executed:
#!/bin/bash
# Get the function definition and its parameters
sed -n '80,150p' code/core/src/manager/settings/Checklist/Checklist.tsxLength of output: 1888
---
🏁 Script executed:
```shell
#!/bin/bash
# Get component signature starting at line 230
sed -n '228,240p' code/core/src/manager/settings/Checklist/Checklist.tsx
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Examine useChecklist hook to understand data structure
cat code/core/src/manager/components/sidebar/useChecklist.tsLength of output: 5811
🏁 Script executed:
#!/bin/bash
# Search for where isLockedBy gets assigned/populated in useChecklist
rg -n "isLockedBy" code/core/src/manager/components/sidebar/useChecklist.ts -B3 -A3Length of output: 866
Guard itemsById access when building locked-item tooltip text
The vulnerability is confirmed. isLockedBy is populated from item.after (a static array in checklistData), while itemsById is built only from availableItems (which are filtered by availability checks). An item can exist in item.after but be unavailable due to its own available() check or dependencies, causing it to be excluded from availableItems and thus missing from itemsById. This leads to a runtime error at line 371 when accessing itemsById[id].label.
The suggested fix is appropriate:
- note={`Complete ${isLockedBy.map((id) => `"${itemsById[id].label}"`).join(', ')} first`}
+ note={`Complete ${isLockedBy
+ .map((id) => itemsById[id]?.label)
+ .filter((label): label is string => !!label)
+ .map((label) => `"${label}"`)
+ .join(', ')} first`}Also applies to: 370-373
🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/Checklist.tsx around lines 242-258,
the itemsById map is built only from availableItems but elsewhere (lines
~370-373) code assumes every id in item.after exists in itemsById; this can be
false and causes a runtime crash when accessing itemsById[id].label. Fix by
guarding lookups: when iterating item.after to build the locked-item tooltip,
check that itemsById[id] exists before accessing its properties (skip or provide
a safe fallback for missing entries), and update any similar access sites
(around lines 370-373) to use the same existence check or default label.
| const subscribeToIndex: ( | ||
| condition: (entries: Record<string, API_PreparedIndexEntry>) => boolean | ||
| ) => ChecklistData['sections'][number]['items'][number]['subscribe'] = | ||
| (condition) => | ||
| ({ api, done }) => { | ||
| const check = () => condition(api.getIndex()?.entries || {}); | ||
| if (check()) { | ||
| done(); | ||
| } else { | ||
| return api.on(STORY_INDEX_INVALIDATED, () => check() && done()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Memory leak: Event listener not cleaned up after completion.
The event handler continues to fire even after done() is called, as there's no unsubscribe mechanism. This can cause memory leaks and unnecessary event processing.
Apply this diff to fix the memory leak:
const subscribeToIndex: (
condition: (entries: Record<string, API_PreparedIndexEntry>) => boolean
) => ChecklistData['sections'][number]['items'][number]['subscribe'] =
(condition) =>
({ api, done }) => {
const check = () => condition(api.getIndex()?.entries || {});
if (check()) {
done();
} else {
- return api.on(STORY_INDEX_INVALIDATED, () => check() && done());
+ const unsubscribe = api.on(STORY_INDEX_INVALIDATED, () => {
+ if (check()) {
+ done();
+ unsubscribe();
+ }
+ });
+ return unsubscribe;
}
};🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/checklistData.tsx around lines 98 to
109, the subscribeToIndex callback registers an event handler but never
unsubscribes after calling done(), causing a memory leak; update the function to
capture the unsubscribe return value from api.on, and when the condition is
already true or when the handler observes the condition become true, call the
unsubscribe function before invoking done(); return the unsubscribe function
when the check is deferred so callers can also cancel if needed.
| <p> | ||
| Storybook gets better as you add more components. Start with the easy ones, like | ||
| Button or Avatar, and work your way up to more complex components, like Select, | ||
| Autocomplete, or even full pages. | ||
| </p> | ||
| ), |
There was a problem hiding this comment.
Incorrect content copy-pasted from another item.
The content for the "controls" item describes adding more components, which is copy-pasted from the more-components item. This should describe how Controls work instead.
Apply this diff to fix the content:
content: () => (
<p>
- Storybook gets better as you add more components. Start with the easy ones, like
- Button or Avatar, and work your way up to more complex components, like Select,
- Autocomplete, or even full pages.
+ Controls gives you a graphical UI to interact with a component's arguments
+ dynamically, without needing to code. It creates an addon panel that allows you
+ to quickly edit your component's args (props, slots, attributes, etc.) and see
+ the changes reflected in the story.
</p>
),📝 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.
| <p> | |
| Storybook gets better as you add more components. Start with the easy ones, like | |
| Button or Avatar, and work your way up to more complex components, like Select, | |
| Autocomplete, or even full pages. | |
| </p> | |
| ), | |
| <p> | |
| Controls gives you a graphical UI to interact with a component's arguments | |
| dynamically, without needing to code. It creates an addon panel that allows you | |
| to quickly edit your component's args (props, slots, attributes, etc.) and see | |
| the changes reflected in the story. | |
| </p> | |
| ), |
🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/checklistData.tsx around lines 254
to 259, the "controls" item's content is incorrectly copy-pasted from the
"more-components" item; replace the paragraph with a description of Storybook
Controls explaining they let you edit component props dynamically in the UI
(knobs-like), how to configure argTypes/defaultArgs for common prop types, and a
brief note to use controls to prototype and test variations interactively.
Ensure the text specifically references Controls (props/args, argTypes/defaults)
rather than adding more components.
| TourGuide.render({ | ||
| steps: [ | ||
| { | ||
| title: 'Testing widget', | ||
| content: | ||
| 'Run tests right from your Storybook sidebar using the testing widget.', | ||
| placement: 'right-end', | ||
| target: '#storybook-testing-module', | ||
| highlight: '#storybook-testing-module', | ||
| onNext: ({ next }: { next: () => void }) => { | ||
| const toggle = document.getElementById('testing-module-collapse-toggle'); | ||
| if (toggle?.getAttribute('aria-label') === 'Expand testing module') { | ||
| toggle.click(); | ||
| setTimeout(next, 300); | ||
| } else { | ||
| next(); | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| title: 'Start a test run', | ||
| content: 'Start a test run at the click of a button using Vitest.', | ||
| placement: 'right', | ||
| target: | ||
| '[data-module-id="storybook/test/test-provider"] button[aria-label="Start test run"]', | ||
| highlight: `[data-module-id="storybook/test/test-provider"] button[aria-label="Start test run"]`, | ||
| hideNextButton: true, | ||
| }, | ||
| ], | ||
| }), |
There was a problem hiding this comment.
Hardcoded DOM selectors may cause tour to fail.
The TourGuide tour uses hardcoded element selectors that may not exist in all Storybook configurations or versions. If these elements are missing, the tour will fail silently or show in incorrect positions.
Consider adding defensive checks or using more resilient selectors:
onClick: () =>
TourGuide.render({
steps: [
{
title: 'Testing widget',
content:
'Run tests right from your Storybook sidebar using the testing widget.',
placement: 'right-end',
target: '#storybook-testing-module',
highlight: '#storybook-testing-module',
+ skipIfNotFound: true,
onNext: ({ next }: { next: () => void }) => {
const toggle = document.getElementById('testing-module-collapse-toggle');
if (toggle?.getAttribute('aria-label') === 'Expand testing module') {
toggle.click();
setTimeout(next, 300);
} else {
next();
}
},
},
{
title: 'Start a test run',
content: 'Start a test run at the click of a button using Vitest.',
placement: 'right',
target:
'[data-module-id="storybook/test/test-provider"] button[aria-label="Start test run"]',
highlight: `[data-module-id="storybook/test/test-provider"] button[aria-label="Start test run"]`,
hideNextButton: true,
+ skipIfNotFound: true,
},
],
}),Note: Verify that TourGuide supports a skipIfNotFound option or similar resilience feature.
Committable suggestion skipped: line range outside the PR's diff.
| after: ['install-a11y'], | ||
| label: 'Run accessibility tests', | ||
| criteria: 'Accessibility tests are run from the test widget in the sidebar', | ||
| subscribe: ({ api, done }) => api.on('storybook/a11y/result', done), // TODO check test widget state |
There was a problem hiding this comment.
TODO comment indicates incomplete implementation.
The TODO comment suggests that checking the test widget state is incomplete. Consider implementing the proper state check or clarifying whether the current event-based approach is sufficient.
🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/checklistData.tsx around line 474,
the subscribe handler currently listens only for the 'storybook/a11y/result'
event and contains a TODO about checking the test widget state; implement a
proper state check by reading the widget's current status (e.g., via the API or
store used elsewhere in this file) before calling done, or explicitly
document/guard that the event implies a completed state — update the subscribe
to verify the widget is in the expected "completed" or "ready" state and only
call done when that condition is met; if no state-check is needed, remove the
TODO and add a brief comment explaining why the event alone is sufficient.
See #32644
Telescopes on #32874
What I did
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-32799-sha-64c5d96a. 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-32799-sha-64c5d96achecklist-tasks64c5d96a1763049775)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=32799Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor