Onboarding: Guided tour checklist #32795
Conversation
…e whole UI up and showing empty space below)
📝 WalkthroughWalkthroughAdds an onboarding/checklist subsystem: settings schema, universal checklist store, server initializer, manager/store exports, client hook and UI (widget, guide, checklist), onboarding addon/survey, TourGuide/highlight primitives, many new components/stories, and Sidebar/Menu refactors plus assorted small UI/signature tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant UniversalStore as UniversalChecklistStore
participant Manager
participant Widget as ChecklistWidget/GuidePage
Server->>UniversalStore: initializeChecklist()
UniversalStore->>UniversalStore: load user & project caches, merge, mark loaded
UniversalStore->>Server: persist project/user updates & emit telemetry
Manager->>UniversalStore: subscribe to changes
Widget->>UniversalStore: dispatch accept/done/skip/mute/disable
UniversalStore->>Manager: notify subscribers
Manager->>Widget: UI re-renders with updated state
sequenceDiagram
participant User
participant Sidebar
participant Menu
participant Hook as useChecklist
participant Store as ChecklistStore
User->>Sidebar: open menu
Sidebar->>Menu: useMenu({ api, showToolbar, isPanelShown, isNavShown, enableShortcuts })
Menu->>Hook: request available/next/progress
Hook->>Store: subscribe & compute derived state
Hook-->>Menu: return collections/actions
User->>Menu: click guide item
Menu->>Store: accept(id) / navigate
Store->>Sidebar: notify -> widget progress updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas meriting focused review:
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
|
View your CI Pipeline Execution ↗ for commit 2ff8d29
☁️ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
147-154: Clear pending timeout on unmount to avoid setState after unmount
timeoutRefisn’t cleared in a cleanup, which can fire after unmount.const timeoutRef = useRef<null | ReturnType<typeof setTimeout>>(null); @@ const toggleCollapsed = useCallback((event?: SyntheticEvent, value?: boolean) => { @@ timeoutRef.current = setTimeout(() => { setChangingCollapse(false); }, 250); }, []); + + useEffect(() => { + return () => { + if (timeoutRef.current) clearTimeout(timeoutRef.current); + }; + }, []);Also applies to: 187-197
🧹 Nitpick comments (14)
code/core/src/manager/settings/whats_new.tsx (1)
34-46: LGTM! Footer styling updates align with theme tokens.The updated styling correctly uses theme tokens for typography and borders, consistent with the broader PR's theming approach. The footer height reduction from 80px to 40px is properly coordinated with the iframe height calculation.
Minor note: Line 37 uses a numeric value (
height: 40) which defaults to pixels, while other dimensions use string literals (e.g., line 101:'calc(100% - 40px)'). Considerheight: '40px'for consistency, though the current approach is valid.For consistency:
- height: 40, + height: '40px',code/core/src/preview-api/modules/preview-web/UrlStore.ts (1)
77-80: Consider using an anchored regex for exact matching.The logic correctly distinguishes between missing/empty viewMode (defaults to 'story') and invalid non-empty viewMode (returns null). However, the regex
/docs|story/could match partial strings like "stories" or "documentation".Consider using an anchored regex for more precise validation:
- } else if (!viewMode.match(/docs|story/)) { + } else if (!viewMode.match(/^(docs|story)$/)) { return null;This ensures only exact matches for "docs" or "story" are accepted.
code/core/src/manager/settings/GuidedTourPage.tsx (1)
40-43: Consider refining the description text tone.The current description has an informal, slightly dismissive tone: "This isn't your first time setting up software so get to it!" Consider whether this matches your product's voice and user experience goals for onboarding.
If a more professional tone is preferred:
<p> - Learn the basics. Set up Storybook. You know the drill. This isn't your first time setting - up software so get to it! + Learn the basics and get started with Storybook. Follow the guided tour to set up your project and explore key features. </p>code/core/src/manager/manager-stores.ts (1)
19-23: Consider making debug flag conditional on environment.The
debug: trueflag is hardcoded, which means debug logging will be active in all environments. Consider making this conditional based onCONFIG_TYPEor a separate debug environment variable to reduce noise in production.export const universalChecklistStore = experimental_UniversalStore.create<StoreState, StoreEvent>({ ...UNIVERSAL_CHECKLIST_STORE_OPTIONS, leader: globalThis.CONFIG_TYPE === 'PRODUCTION', - debug: true, + debug: globalThis.CONFIG_TYPE !== 'PRODUCTION', });code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
8-13: Consider typing the mock context more precisely.The
anytype formanagerContextweakens type safety. If you have access to the properManagerContexttype, consider using it or at least a partial type that defines the required shape.Apply this diff to improve type safety:
-const managerContext: any = { +const managerContext: { state: object; api: { navigateUrl: ReturnType<typeof fn> } } = { state: {}, api: { navigateUrl: fn().mockName('api::navigateUrl'), }, };code/core/src/core-server/utils/checklist.ts (1)
10-27: Consider error handling for settings operations.The
globalSettings()andsettings.save()calls lack error handling. If settings fail to load or save (e.g., file system permissions, disk full, corrupted JSON), the checklist initialization will silently fail, potentially leading to state loss or confusion.Consider adding error handling:
export function initializeChecklist() { const store = experimental_UniversalStore.create<StoreState, StoreEvent>({ ...UNIVERSAL_CHECKLIST_STORE_OPTIONS, leader: true, }); - globalSettings().then((settings) => { + globalSettings() + .then((settings) => { store.setState({ completed: settings.value.checklist?.completed ?? [], skipped: settings.value.checklist?.skipped ?? [], }); store.onStateChange((state) => { settings.value.checklist = state; - settings.save(); + settings.save().catch((error) => { + // Log error without throwing to avoid disrupting the store + console.error('Failed to save checklist state:', error); + }); }); + }) + .catch((error) => { + console.error('Failed to initialize checklist from settings:', error); }); }Note: Based on coding guidelines, consider using Storybook's node-logger instead of
console.errorfor server-side code.code/core/src/manager/components/sidebar/ChecklistModule.tsx (2)
29-33: Make CSSTransition nodeRefs stable across renders.Recreating refs each render can break enter/exit animations and cause warnings. Memoize per id.
Apply:
-import React, { createRef } from 'react'; +import React, { createRef, useRef } from 'react'; @@ - const nextTasks = allTasks - .filter(({ id }) => !completed.includes(id) && !skipped.includes(id)) - .map((task) => ({ ...task, nodeRef: createRef<HTMLLIElement>() })); + const refMap = useRef(new Map<string, React.RefObject<HTMLLIElement>>()); + const getRef = (id: string) => { + let ref = refMap.current.get(id); + if (!ref) { + ref = createRef<HTMLLIElement>(); + refMap.current.set(id, ref); + } + return ref; + }; + const nextTasks = allTasks + .filter(({ id }) => !completed.includes(id) && !skipped.includes(id)) + .map((task) => ({ ...task, nodeRef: getRef(task.id) }));Also applies to: 52-54
45-47: Guard progress calculation (division-by-zero) and improve clarity.Minor safety/readability tweak.
Apply:
- <ListboxButton onClick={() => api.navigateUrl('/settings/guided-tour', { plain: false })}> - {Math.round(((allTasks.length - nextTasks.length) / allTasks.length) * 100)}% - </ListboxButton> + <ListboxButton onClick={() => api.navigateUrl('/settings/guided-tour', { plain: false })}> + {allTasks.length + ? Math.round(((allTasks.length - nextTasks.length) / allTasks.length) * 100) + : 0} + % + </ListboxButton>code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
6-13: Add minimal a11y to toggle button (aria-expanded/aria-controls).Expose and pass
contentIdfrom state to the button for better semantics.Example:
-const toggle = ({ isCollapsed, toggleCollapsed }: { isCollapsed: boolean; toggleCollapsed: () => void }) => - <button onClick={toggleCollapsed}>{isCollapsed ? 'Open' : 'Close'}</button>; +const toggle = ({ isCollapsed, toggleCollapsed, contentId }: { isCollapsed: boolean; toggleCollapsed: () => void; contentId: string }) => + <button + aria-expanded={!isCollapsed} + aria-controls={contentId} + onClick={toggleCollapsed} + > + {isCollapsed ? 'Open' : 'Close'} + </button>;Also applies to: 36-46
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
29-33: Avoid arbitrary setTimeout completion in sample predicate.Auto-completing after 3s may confuse users. Consider gating completion on an actual user action or detectable condition.
code/core/src/components/components/Listbox/Listbox.tsx (1)
56-63: Forward refs through ListboxButton for parity with Button and better composability.This enables parent code to focus/measure the underlying button.
Apply:
-import React, { type ComponentProps } from 'react'; +import React, { forwardRef, type ComponentProps } from 'react'; @@ -export const ListboxButton = ({ - padding = 'small', - size = 'medium', - variant = 'ghost', - ...props -}: ComponentProps<typeof Button>) => ( - <Button {...props} variant={variant} padding={padding} size={size} /> -); +export const ListboxButton = forwardRef<HTMLButtonElement, ComponentProps<typeof Button>>( + ({ padding = 'small', size = 'medium', variant = 'ghost', ...props }, ref) => ( + <Button ref={ref} {...props} variant={variant} padding={padding} size={size} /> + ) +);code/core/src/components/components/Card/Card.tsx (1)
64-97: Prevent the animated outline from intercepting pointer eventsThe absolutely‑positioned
&:beforecan sit above content and block clicks. Make it inert to pointer events.'&:before': { content: '""', display: animation === 'none' ? 'none' : 'block', position: 'absolute', left: 0, top: 0, width: '100%', height: '100%', opacity: 1, + pointerEvents: 'none',code/core/src/manager/components/sidebar/TestingModule.tsx (1)
30-34: Avoid naming collision with exported Collapsible componentThis local styled
Collapsiblecan be confused with the exportedcomponents/Collapsible. ConsiderCollapsibleContainer.code/core/src/manager/settings/Checklist/Checklist.tsx (1)
246-249: Remove unusednodeRef(or wire it up)
nodeRefis created but never read; drop it or use it for scrolling/focus.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/components/components/Button/Button.tsx(3 hunks)code/core/src/components/components/Card/Card.stories.tsx(1 hunks)code/core/src/components/components/Card/Card.tsx(1 hunks)code/core/src/components/components/Collapsible/Collapsible.stories.tsx(1 hunks)code/core/src/components/components/Collapsible/Collapsible.tsx(1 hunks)code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx(1 hunks)code/core/src/components/components/FocusProxy/FocusProxy.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.stories.tsx(1 hunks)code/core/src/components/components/Listbox/Listbox.tsx(1 hunks)code/core/src/components/index.ts(1 hunks)code/core/src/core-server/presets/common-preset.ts(2 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager/components/Transition.tsx(1 hunks)code/core/src/manager/components/layout/Layout.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(2 hunks)code/core/src/manager/components/sidebar/TestingModule.tsx(3 hunks)code/core/src/manager/globals/exports.ts(2 hunks)code/core/src/manager/manager-stores.mock.ts(2 hunks)code/core/src/manager/manager-stores.ts(2 hunks)code/core/src/manager/settings/Checklist/Checklist.stories.tsx(1 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(1 hunks)code/core/src/manager/settings/Checklist/checklistData.tsx(1 hunks)code/core/src/manager/settings/GuidedTourPage.tsx(1 hunks)code/core/src/manager/settings/index.tsx(3 hunks)code/core/src/manager/settings/whats_new.tsx(2 hunks)code/core/src/preview-api/modules/preview-web/UrlStore.test.ts(1 hunks)code/core/src/preview-api/modules/preview-web/UrlStore.ts(1 hunks)code/core/src/shared/checklist-store/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/core-server/presets/common-preset.tscode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/components/index.tscode/core/src/manager/manager-stores.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/components/components/FocusProxy/FocusProxy.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/settings/whats_new.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.test.tscode/core/src/components/components/Button/Button.tsxcode/core/src/manager/manager-stores.mock.tscode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/settings/GuidedTourPage.tsxcode/core/src/manager/globals/exports.tscode/core/src/shared/checklist-store/index.tscode/core/src/components/components/FocusProxy/FocusProxy.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/manager/components/Transition.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.tscode/core/src/components/components/Collapsible/Collapsible.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/core-server/presets/common-preset.tscode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/components/index.tscode/core/src/manager/manager-stores.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/components/components/FocusProxy/FocusProxy.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/settings/whats_new.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.test.tscode/core/src/components/components/Button/Button.tsxcode/core/src/manager/manager-stores.mock.tscode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/settings/GuidedTourPage.tsxcode/core/src/manager/globals/exports.tscode/core/src/shared/checklist-store/index.tscode/core/src/components/components/FocusProxy/FocusProxy.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/manager/components/Transition.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.tscode/core/src/components/components/Collapsible/Collapsible.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/core-server/presets/common-preset.tscode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/components/index.tscode/core/src/manager/manager-stores.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/components/components/FocusProxy/FocusProxy.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/settings/whats_new.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.test.tscode/core/src/components/components/Button/Button.tsxcode/core/src/manager/manager-stores.mock.tscode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/settings/GuidedTourPage.tsxcode/core/src/manager/globals/exports.tscode/core/src/shared/checklist-store/index.tscode/core/src/components/components/FocusProxy/FocusProxy.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/manager/components/Transition.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.tscode/core/src/components/components/Collapsible/Collapsible.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/core-server/presets/common-preset.tscode/core/src/components/components/Card/Card.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.stories.tsxcode/core/src/manager/settings/index.tsxcode/core/src/manager/components/sidebar/ChecklistModule.tsxcode/core/src/components/index.tscode/core/src/manager/manager-stores.tscode/core/src/manager/components/sidebar/ChecklistModule.stories.tsxcode/core/src/components/components/FocusProxy/FocusProxy.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/components/components/Listbox/Listbox.stories.tsxcode/core/src/components/components/Card/Card.tsxcode/core/src/cli/globalSettings.tscode/core/src/manager/components/sidebar/Explorer.tsxcode/core/src/manager/settings/whats_new.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.test.tscode/core/src/components/components/Button/Button.tsxcode/core/src/manager/manager-stores.mock.tscode/core/src/manager/settings/Checklist/checklistData.tsxcode/core/src/components/components/Collapsible/Collapsible.stories.tsxcode/core/src/core-server/utils/checklist.tscode/core/src/manager/components/sidebar/TestingModule.tsxcode/core/src/manager/settings/GuidedTourPage.tsxcode/core/src/manager/globals/exports.tscode/core/src/shared/checklist-store/index.tscode/core/src/components/components/FocusProxy/FocusProxy.stories.tsxcode/core/src/manager/settings/Checklist/Checklist.tsxcode/core/src/components/components/Listbox/Listbox.tsxcode/core/src/manager/components/Transition.tsxcode/core/src/preview-api/modules/preview-web/UrlStore.tscode/core/src/components/components/Collapsible/Collapsible.tsx
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/core/src/preview-api/modules/preview-web/UrlStore.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/preview-api/modules/preview-web/UrlStore.test.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/core/src/preview-api/modules/preview-web/UrlStore.test.ts
🧬 Code graph analysis (22)
code/core/src/core-server/presets/common-preset.ts (1)
code/core/src/core-server/utils/checklist.ts (1)
initializeChecklist(10-27)
code/core/src/components/components/Card/Card.stories.tsx (1)
code/core/src/components/components/Card/Card.tsx (1)
Card(29-42)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (3)
code/core/src/manager/settings/Checklist/Checklist.tsx (1)
Checklist(217-409)code/core/src/manager-api/root.tsx (1)
ManagerContext(78-78)code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-173)
code/core/src/manager/settings/index.tsx (1)
code/core/src/manager/settings/GuidedTourPage.tsx (1)
GuidedTourPage(35-48)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (6)
code/core/src/manager-api/root.tsx (1)
useStorybookApi(294-297)code/core/src/manager/manager-stores.mock.ts (2)
universalChecklistStore(48-57)checklistStore(59-59)code/core/src/manager/manager-stores.ts (2)
universalChecklistStore(19-23)checklistStore(25-25)code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-173)code/core/src/components/components/Card/Card.tsx (1)
Card(29-42)code/core/src/manager/components/Transition.tsx (1)
Transition(6-12)
code/core/src/manager/manager-stores.ts (2)
code/core/src/manager/manager-stores.mock.ts (2)
universalChecklistStore(48-57)checklistStore(59-59)code/core/src/shared/checklist-store/index.ts (4)
StoreState(6-9)StoreEvent(11-14)UNIVERSAL_CHECKLIST_STORE_OPTIONS(16-19)createChecklistStore(29-50)
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (2)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
ChecklistModule(25-78)code/core/src/manager-api/root.tsx (1)
ManagerContext(78-78)
code/core/src/components/components/FocusProxy/FocusProxy.tsx (1)
code/core/src/components/index.ts (1)
FocusProxy(46-46)
code/core/src/components/components/Listbox/Listbox.stories.tsx (2)
code/core/src/components/components/Listbox/Listbox.tsx (5)
Listbox(7-15)ListboxItem(17-54)ListboxText(76-90)ListboxButton(56-63)ListboxAction(65-74)code/core/src/manager/container/Menu.tsx (1)
Shortcut(44-50)
code/core/src/components/components/Card/Card.tsx (1)
code/core/src/components/index.ts (1)
Card(45-45)
code/core/src/manager/components/sidebar/Explorer.tsx (1)
code/core/src/manager/components/sidebar/ChecklistModule.tsx (1)
ChecklistModule(25-78)
code/core/src/preview-api/modules/preview-web/UrlStore.test.ts (1)
code/core/src/preview-api/modules/preview-web/UrlStore.ts (1)
getSelectionSpecifierFromPath(69-92)
code/core/src/manager/manager-stores.mock.ts (2)
code/core/src/manager/manager-stores.ts (2)
universalChecklistStore(19-23)checklistStore(25-25)code/core/src/shared/checklist-store/index.ts (4)
StoreState(6-9)StoreEvent(11-14)UNIVERSAL_CHECKLIST_STORE_OPTIONS(16-19)createChecklistStore(29-50)
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
code/core/src/manager/settings/Checklist/Checklist.tsx (1)
ChecklistData(11-24)
code/core/src/components/components/Collapsible/Collapsible.stories.tsx (3)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
Collapsible(6-31)code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx (1)
Default(10-21)code/core/src/test/testing-library.ts (1)
userEvent(120-123)
code/core/src/core-server/utils/checklist.ts (2)
code/core/src/shared/checklist-store/index.ts (3)
StoreState(6-9)StoreEvent(11-14)UNIVERSAL_CHECKLIST_STORE_OPTIONS(16-19)code/core/src/cli/globalSettings.ts (1)
globalSettings(22-41)
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
code/core/src/components/components/Card/Card.tsx (2)
Card(29-42)Content(44-48)
code/core/src/manager/settings/GuidedTourPage.tsx (2)
code/core/src/manager/settings/Checklist/Checklist.tsx (1)
Checklist(217-409)code/core/src/manager/settings/Checklist/checklistData.tsx (1)
checklistData(7-173)
code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx (3)
code/core/src/components/components/FocusProxy/FocusProxy.tsx (1)
FocusProxy(3-15)code/core/src/components/index.ts (1)
FocusProxy(46-46)code/core/src/components/components/Collapsible/Collapsible.stories.tsx (1)
Default(24-24)
code/core/src/manager/settings/Checklist/Checklist.tsx (5)
code/core/src/manager-api/root.tsx (2)
API(94-107)useStorybookApi(294-297)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Actions(112-116)code/core/src/manager/manager-stores.mock.ts (2)
universalChecklistStore(48-57)checklistStore(59-59)code/core/src/manager/manager-stores.ts (2)
universalChecklistStore(19-23)checklistStore(25-25)code/core/src/components/components/FocusProxy/FocusProxy.tsx (1)
FocusProxy(3-15)
code/core/src/components/components/Listbox/Listbox.tsx (1)
code/core/src/components/components/Button/Button.tsx (1)
Button(20-76)
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
code/core/src/components/index.ts (1)
Collapsible(44-44)
⏰ 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: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (25)
code/core/src/manager/components/layout/Layout.tsx (1)
261-262: LGTM! Flex layout enables vertical stacking for pages content.The addition of flex properties to
PagesContaineris a straightforward CSS change that enables its children to be arranged in a vertical column. This complements the existing grid positioning properties without conflict.code/core/src/manager/settings/whats_new.tsx (1)
90-105: LGTM! Iframe height calculation correctly updated.The height calculation properly accounts for the new 40px footer height, preventing visual gaps or overlaps. This change is correctly coordinated with the Container component updates.
code/core/src/preview-api/modules/preview-web/UrlStore.test.ts (3)
75-81: LGTM! Test correctly verifies explicit viewMode=story.The test properly validates that when viewMode is explicitly set to 'story' in the URL, the function returns the expected storySpecifier and viewMode.
82-88: LGTM! Test correctly verifies explicit viewMode=docs.The test properly validates that when viewMode is explicitly set to 'docs' in the URL, the function returns the expected storySpecifier and viewMode.
89-92: LGTM! Test correctly verifies rejection of unsupported viewModes.The test properly validates that invalid viewMode values cause the function to return null, preventing navigation to stories with unsupported view modes.
code/core/src/manager/settings/index.tsx (1)
16-16: LGTM! Guided tour integration follows established patterns.The new guided tour tab and route follow the same structure as existing settings pages. The implementation is consistent with the codebase conventions.
Also applies to: 95-95, 115-117
code/core/src/manager/globals/exports.ts (1)
489-489: LGTM! Generated export additions are correct.The new component exports (Card, Collapsible, FocusProxy, Listbox family) are properly added to the public API surface. Since this file is auto-generated (as noted in the header comment), these changes reflect the intended public exports for the new checklist components.
Also applies to: 492-492, 499-499, 513-517
code/core/src/cli/globalSettings.ts (1)
18-18: LGTM! Schema extension is forward-compatible.The new
checklistfield is correctly defined as optional, following the established pattern for forward compatibility (as noted in line 14's comment). The structure withcompletedandskippedarrays aligns with the store state types used throughout the feature.code/core/src/manager/components/sidebar/Explorer.tsx (1)
4-4: LGTM! Checklist module integration is clean.The
ChecklistModuleis properly integrated into the Explorer component. Based on the implementation incode/core/src/manager/components/sidebar/ChecklistModule.tsx, the module returnsnullwhen there are no pending tasks, ensuring it doesn't affect the layout when inactive.Also applies to: 42-42
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
1-46: LGTM! Story setup properly mocks dependencies.The story correctly provides the required
ManagerContextand initializes the mock checklist store with sample data. The use offn().mockName()for the API mock and thebeforeEachhook for state setup follow Storybook best practices.code/core/src/manager/manager-stores.ts (1)
1-25: Store initialization follows established patterns.The checklist stores are properly initialized using the universal store pattern, which enables state synchronization across contexts. The conditional
leaderflag based onCONFIG_TYPEaligns with the mock setup incode/core/src/manager/manager-stores.mock.ts.code/core/src/core-server/presets/common-preset.ts (1)
256-256: The review comment contains incorrect premises and lacks supporting evidence.The stated concern assumes
initializeChecklist()can be awaited, but the function is synchronous (noasynckeyword) and therefore cannot be awaited. The async initialization viaglobalSettings().then()is fire-and-forget by design, matching the pattern used byinitializeWhatsNew()andinitializeSaveStory().Additionally, the claim that "other code attempts to access checklist state before initialization completes" lacks supporting evidence. All actual checklist store accesses found in the codebase occur in user interaction handlers (
onClick), not in synchronous startup code paths. TheUniversalStoreclass providesuntilReady()for scenarios requiring readiness guarantees, and the commented code inChecklist.tsxdemonstrates developer awareness of this pattern.Likely an incorrect or invalid review comment.
code/core/src/components/components/Card/Card.stories.tsx (1)
1-63: LGTM! Story comprehensively demonstrates Card variations.The story file is well-structured and demonstrates the Card component's various states including different outline animations (rainbow, spin) and outline colors. The grid layout provides clear visual comparison of all variants.
code/core/src/manager/components/sidebar/ChecklistModule.stories.tsx (1)
24-29: LGTM! Mock store initialization is well-structured.The
beforeEachhook properly initializes the mock store state with realistic completed and skipped items, ensuring a consistent starting state for the story.code/core/src/components/components/FocusProxy/FocusProxy.stories.tsx (1)
10-21: LGTM! FocusProxy story effectively demonstrates focus behavior.The story correctly demonstrates the FocusProxy component's focus behavior with a play function that automatically focuses the button on canvas load. The
htmlForprop properly targets the button element by id.code/core/src/components/components/Button/Button.tsx (2)
9-9: LGTM! Polymorphicasprop adds flexibility.The addition of the
asprop enables the Button to be rendered as different HTML elements (button, a, label) or as a Slot for composition. This is a useful enhancement for accessibility and flexibility.
36-36: Nice simplification of polymorphic rendering logic.The new implementation
const Comp = asChild ? Slot : asis more concise and clearer than conditional branching. It properly handles the asChild case while respecting theasprop.code/core/src/manager/manager-stores.mock.ts (1)
48-59: LGTM! Mock store setup mirrors production implementation.The mock checklist store setup correctly mirrors the production implementation from
manager-stores.ts, using the same configuration options and factory pattern. The use oftestUtils.mocked()ensures proper mock tracking for tests.code/core/src/manager/components/Transition.tsx (1)
6-12: LGTM! Excellent accessibility consideration.The Transition component properly respects the
prefers-reduced-motionmedia query by setting the transition duration to 0ms, ensuring a better experience for users with motion sensitivity. The CSS variable approach is clean and allows consuming components to reference--transition-duration.code/core/src/core-server/utils/checklist.ts (1)
10-14: This review comment is incorrect and should be dismissed.The store instance in
initializeChecklist()is properly stored in UniversalStore's global instances registry using its ID'storybook/checklist'and does not need to be returned. More importantly, the store is actively used within the function: it subscribes to state changes and persists them to global settings. The server-side store (created withleader: true) synchronizes with the manager-side store via the UniversalStore's channel mechanism.code/core/src/components/index.ts (1)
44-53: Exports look correct and consistent with existing structure.Public surface updates for Collapsible/Card/FocusProxy/Listbox* LGTM.
code/core/src/components/components/Listbox/Listbox.stories.tsx (1)
8-11: Stories read well and cover main compositions.No issues spotted.
Also applies to: 15-50, 52-81
code/core/src/manager/settings/Checklist/checklistData.tsx (1)
51-57: Open-in-new-tab links: confirm rel is set for security.If
Linkdoesn’t auto-applyrel="noopener noreferrer"whentarget="_blank", add it to avoid opener leaks.Can you confirm the internal Link component behavior?
Also applies to: 71-77, 116-122, 139-145
code/core/src/components/components/Collapsible/Collapsible.tsx (1)
6-31: LGTM: Accessible, controlled/uncontrolled API with sensible defaultsGood ARIA wiring (
aria-controls,aria-expanded,aria-hidden), reduced‑motion handling, and render‑prop support.Also applies to: 33-50, 52-70
code/core/src/manager/components/sidebar/TestingModule.tsx (1)
373-401: Code is appropriate as-is; no fallback needed.Storybook targets modern evergreen browsers (latest 1–2 stable versions, Chrome ≥100), and the
inertattribute is natively supported in all current releases of Chrome (~v102+), Firefox (~v112+), and Safari (~v15.5+). There is no browser coverage gap within Storybook's supported scope. The use ofinertwithout a fallback is safe.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 39 | 39 | 0 |
| Self size | 19.20 MB | 19.65 MB | 🚨 +448 KB 🚨 |
| Dependency size | 16.40 MB | 16.40 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 173 | 173 | 0 |
| Self size | 773 KB | 773 KB | 🎉 -84 B 🎉 |
| Dependency size | 66.17 MB | 66.62 MB | 🚨 +449 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 166 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +36 B 🚨 |
| Dependency size | 64.74 MB | 65.19 MB | 🚨 +448 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 40 | 40 | 0 |
| Self size | 999 KB | 999 KB | 🚨 +42 B 🚨 |
| Dependency size | 35.60 MB | 36.05 MB | 🚨 +448 KB 🚨 |
| Bundle Size Analyzer | node | node |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
code/core/src/manager/settings/Checklist/Checklist.tsx (2)
222-228: Guardwindowaccess and replace polling withhashchangeevent.This issue has already been flagged in previous reviews. The code directly accesses
globalThis.windowwithout checking if it exists (causing potential SSR/test crashes) and uses 100ms polling instead of the nativehashchangeevent.
265-276: Fix focus selector - wrong attribute.This issue has already been flagged in previous reviews. The
[for="toggle-${targetHash}"]selector doesn't match the button elements. Should usegetElementByIdorquerySelector('#toggle-...')instead.
🧹 Nitpick comments (3)
code/core/src/manager/settings/Checklist/Checklist.tsx (3)
230-242: Remove commented-out code.This large block of commented code should either be uncommented if needed or removed entirely. Dead code adds confusion and maintenance burden.
- // universalChecklistStore.untilReady().then(() => checklistStore.complete('whats-new-sb-9')); - - // useEffect(() => { - // // const componentTestStatusStore = experimental_getStatusStore(STATUS_TYPE_ID_COMPONENT_TEST); - // // const a11yStatusStore = experimental_getStatusStore(STATUS_TYPE_ID_A11Y); - - // data.sections.forEach((section) => { - // section.items.forEach((item) => { - // const complete = () => setCompleted((completed) => new Set([...completed, item.id])); - // item.predicate?.({ api, complete }); - // }); - // }); - // }, [data, api]); -
244-259: Optimize state computation withuseMemoand fix ref creation.The state object is recomputed on every render, and
createRefis called for each item on every render, breaking referential equality and potentially causing issues with focus management.+ const state: ChecklistState = React.useMemo(() => ({ - const state: ChecklistState = { next: 0, progress: 0, sections: data.sections.map((section) => ({ ...section, - items: section.items.map((item) => ({ ...item, nodeRef: createRef<HTMLLIElement>() })), + items: section.items.map((item) => ({ + ...item, + nodeRef: item.nodeRef ?? createRef<HTMLLIElement>() + })), open: false, progress: (section.items.reduce( (a, b) => (completed.includes(b.id) || skipped.includes(b.id) ? a + 1 : a), 0 ) / section.items.length) * 100, })), - }; + }), [data, completed, skipped]);
350-360: Consider letting the start handler control completion.The Start button marks the item as complete before calling the start handler. This assumes "starting" equals "completing," which may not be the intended behavior. Consider letting the start handler decide when to mark the item as complete.
<Button variant="solid" size="small" onClick={() => { - checklistStore.complete(item.id); - item.start?.({ api }); + item.start?.({ api }); }} > Start </Button>The start handler can then call the
completecallback passed via the predicate pattern if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/manager-stores.mock.ts(2 hunks)code/core/src/manager/settings/Checklist/Checklist.tsx(1 hunks)
🧰 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/manager-stores.mock.tscode/core/src/manager/settings/Checklist/Checklist.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/manager-stores.mock.tscode/core/src/manager/settings/Checklist/Checklist.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/manager-stores.mock.tscode/core/src/manager/settings/Checklist/Checklist.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/manager-stores.mock.tscode/core/src/manager/settings/Checklist/Checklist.tsx
🧬 Code graph analysis (2)
code/core/src/manager/manager-stores.mock.ts (2)
code/core/src/manager/manager-stores.ts (2)
universalChecklistStore(19-23)checklistStore(25-25)code/core/src/shared/checklist-store/index.ts (4)
StoreState(6-9)StoreEvent(11-14)UNIVERSAL_CHECKLIST_STORE_OPTIONS(16-19)createChecklistStore(29-50)
code/core/src/manager/settings/Checklist/Checklist.tsx (4)
code/core/src/manager-api/root.tsx (2)
API(94-107)useStorybookApi(294-297)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Actions(112-116)code/core/src/manager/manager-stores.ts (2)
universalChecklistStore(19-23)checklistStore(25-25)code/core/src/components/components/FocusProxy/FocusProxy.tsx (1)
FocusProxy(3-15)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/manager/manager-stores.mock.ts (2)
7-8: LGTM!The imports align with the checklist store integration and mirror the pattern used in the real manager-stores.ts implementation.
48-57: LGTM!The mock store implementation correctly mirrors the pattern used for other stores in this file and aligns with the real implementation in manager-stores.ts.
code/core/src/manager/settings/Checklist/Checklist.tsx (2)
11-32: LGTM!The data contracts are well-structured and provide clear interfaces for the checklist feature.
320-325: Verify keyboard navigation accessibility.The
tabIndex={-1}andonMouseDownpreventDefault remove the FocusProxy from keyboard navigation. While this appears intentional for programmatic focus via hash changes, ensure keyboard users have an alternative way to navigate to and interact with checklist items.
code/core/src/components/components/Collapsible/Collapsible.tsx
Outdated
Show resolved
Hide resolved
…first task for each section rather than first 3 open tasks
…lar set of (known) tasks
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
code/core/src/manager/components/sidebar/Menu.stories.tsx (3)
26-35: Consider providing a complete api stub in managerContext to match useMenu's requirements.The
managerContext.apionly includes five methods (getData, getIndex, getUrlState, navigate, on), but components within the ManagerContext.Provider may attempt to access other methods. While the story-specificuseMenucalls provide their ownapiobjects, any component that directly accesses the ManagerContext would receive this incomplete stub.This is related to the incomplete API surface already flagged in the previous review for the story-specific
useMenucalls.
51-62: beforeEach pattern issue already flagged in previous review.The
beforeEachhook should return a cleanup function rather than directly callingmockStore.setState, as noted in the previous review comment. This ensures proper state isolation between story renders.Per the previous review:
beforeEach: () => { return () => { mockStore.setState(/* initial state */); }; }
89-102: Incomplete api object already flagged in previous review.The
apiobject passed touseMenuonly includes read-only helper methods but omits the action methods (changeSettingsTab,toggleNav,togglePanel,toggleToolbar,jumpToComponent,jumpToStory,emit) that menu items will invoke. Clicking those menu items in the story will throw runtime errors.The previous review suggested providing a complete stub
apiwith all required methods. This issue applies to all threeExpanded*story renderers (lines 89-102, 139-165, 191-204).code/core/src/manager/components/sidebar/Sidebar.stories.tsx (1)
5-6: UseglobalThisinstead of importing from@storybook/global.This import is only used to access
CONFIG_TYPEin theSimpleInProductionstory (lines 191-192). As previously noted, usingglobalThis.CONFIG_TYPEdirectly would avoid adding the@storybook/globaldependency here.Apply this diff to remove the unnecessary import:
-import { global } from '@storybook/global'; -Then update lines 191-192:
- const configType = global.CONFIG_TYPE; - global.CONFIG_TYPE = 'PRODUCTION'; + const configType = globalThis.CONFIG_TYPE; + globalThis.CONFIG_TYPE = 'PRODUCTION';And update line 194:
- global.CONFIG_TYPE = configType; + globalThis.CONFIG_TYPE = configType;code/core/src/core-server/utils/checklist.ts (1)
80-86: Handle async telemetry failures and avoid emitting empty events.
telemetryis async and awaited internally; calling it here withoutawait/.catch()risks unhandled promise rejections if sending telemetry fails, and you also invoke it even when neither items nor the widget changed. This was called out in a previous review and remains unresolved.Consider building a payload first, skipping the call when it’s empty, and catching errors so checklist state changes can’t be disrupted by telemetry:
- const changedValues = Object.entries(state.items).filter( - ([key, value]) => value !== previousState.items[key] - ); - telemetry('onboarding-checklist', { - ...(changedValues.length > 0 ? { items: Object.fromEntries(changedValues) } : {}), - ...(!deepEqual(state.widget, previousState.widget) ? { widget: state.widget } : {}), - }); + const changedValues = Object.entries(state.items).filter( + ([key, value]) => value !== previousState.items[key] + ); + const payload: Record<string, unknown> = { + ...(changedValues.length > 0 ? { items: Object.fromEntries(changedValues) } : {}), + ...(!deepEqual(state.widget, previousState.widget) ? { widget: state.widget } : {}), + }; + if (Object.keys(payload).length > 0) { + telemetry('onboarding-checklist', payload).catch((error) => { + logger.error('Failed to send onboarding-checklist telemetry'); + logger.error(error); + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager-api/modules/versions.ts(3 hunks)code/core/src/manager/components/TourGuide/HighlightElement.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistWidget.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/ChecklistWidget.tsx(1 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(4 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- code/core/src/manager-api/modules/versions.ts
- code/core/src/manager/container/Menu.stories.tsx
- code/core/src/manager/settings/GuidePage.stories.tsx
- code/core/src/manager/components/TourGuide/HighlightElement.tsx
- code/core/src/manager/components/sidebar/ChecklistWidget.stories.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/sidebar/ChecklistWidget.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.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/Menu.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.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/Menu.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.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/Menu.stories.tsxcode/core/src/manager/components/sidebar/Sidebar.stories.tsx
🧬 Code graph analysis (4)
code/core/src/core-server/utils/checklist.ts (3)
code/core/src/shared/checklist-store/index.ts (4)
StoreState(6-10)StoreEvent(15-21)UNIVERSAL_CHECKLIST_STORE_OPTIONS(23-26)ItemState(13-13)code/core/src/cli/globalSettings.ts (1)
globalSettings(34-53)code/core/src/telemetry/index.ts (1)
telemetry(29-70)
code/core/src/manager/components/sidebar/ChecklistWidget.tsx (5)
code/core/src/manager-api/root.tsx (1)
useStorybookApi(295-298)code/core/src/manager/components/sidebar/useChecklist.ts (2)
useChecklist(109-225)ChecklistItem(28-39)code/core/src/manager/components/Optional/Optional.tsx (1)
Optional(19-54)code/core/src/manager/components/TextFlip.tsx (1)
TextFlip(63-100)code/core/src/manager/components/Particles/Particles.tsx (1)
Particles(113-165)
code/core/src/manager/components/sidebar/Menu.stories.tsx (3)
code/core/src/manager-api/root.tsx (1)
ManagerContext(79-79)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-98)code/core/src/manager/container/Menu.tsx (1)
useMenu(64-271)
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
code/core/src/manager/manager-stores.mock.ts (1)
internal_universalChecklistStore(52-61)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-98)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (3)
13-17: LGTM!The checklist-related imports are correctly integrated for test setup. The mock stores and initial state are properly used in the
beforeEachhook to initialize the checklist feature state for the Sidebar stories.
118-129: LGTM!The
beforeEachhook properly initializes the checklist store state for testing. The status overrides (accepted,done,skipped) create a realistic test scenario with a partially-completed checklist, which is appropriate for visual testing of the Sidebar component.
190-196: LGTM!The
beforeEachhook correctly implements the cleanup pattern to temporarily switch to production mode for this specific story. The use of a return function to restore the originalCONFIG_TYPEensures proper test isolation.code/core/src/manager/components/sidebar/ChecklistWidget.tsx (1)
49-76: Checklist widget state and transitions look solid.The
useTransitionArray+renderItemspattern, the 2‑second delayed swap tonextItems, and the event handling (includingstopPropagationon nested buttons) all look correct and align with the intended animations and navigation behavior. No further changes needed from my side here.Also applies to: 167-325
| const setState = ({ | ||
| items = state.items, | ||
| widget = state.widget, | ||
| }: { | ||
| items?: typeof state.items; | ||
| widget?: typeof state.widget; | ||
| }) => { | ||
| settings.value.checklist = { items: items, widget }; | ||
| settings.save(); | ||
| }; |
There was a problem hiding this comment.
Avoid unhandled rejections from settings.save() and cache.set() in persistence helpers.
Both settings.save() (awaited in globalSettings) and cache.set() from the filesystem cache are very likely async. Here they’re called without await or .catch(), so any write error (permissions, disk issues) can surface as unhandled promise rejections, even though you already protect initialization with a top-level try/catch.
You can keep the fire‑and‑forget behavior but explicitly swallow/log failures inside the persistence lambdas:
- const setState = ({
- items = state.items,
- widget = state.widget,
- }: {
- items?: typeof state.items;
- widget?: typeof state.widget;
- }) => {
- settings.value.checklist = { items: items, widget };
- settings.save();
- };
+ const setState = ({
+ items = state.items,
+ widget = state.widget,
+ }: {
+ items?: typeof state.items;
+ widget?: typeof state.widget;
+ }) => {
+ settings.value.checklist = { items, widget };
+ void settings.save().catch((error) => {
+ logger.error('Failed to persist user checklist state');
+ logger.error(error);
+ });
+ };
@@
- const setState = ({ items }: Pick<StoreState, 'items'>) => cache.set('state', { items });
+ const setState = ({ items }: Pick<StoreState, 'items'>) => {
+ void cache.set('state', { items }).catch((error: unknown) => {
+ logger.error('Failed to persist project checklist state');
+ logger.error(error);
+ });
+ };
@@
- saveProjectState({ items: projectValues });
- saveUserState({ items: userValues, widget: state.widget });
+ saveProjectState({ items: projectValues });
+ saveUserState({ items: userValues, widget: state.widget });This keeps the callback sync from the store’s perspective while making persistence failures explicit and non‑fatal.
Also applies to: 48-52, 77-78
🤖 Prompt for AI Agents
code/core/src/core-server/utils/checklist.ts lines 35-44 (also apply same change
to 48-52 and 77-78): the persistence calls (settings.save() and cache.set()) are
invoked fire-and-forget and can produce unhandled promise rejections; change
them to explicitly handle async failures by attaching a rejection handler (e.g.,
call the promise and use .catch(...) or prefix with void and .catch(...)) that
logs or swallows errors so failures are non-fatal while keeping the surrounding
API synchronous; apply the same pattern to every settings.save() and cache.set()
invocation in the listed ranges.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/shared/checklist-store/index.ts (1)
59-76:disableunconditionally clearsmutedAtfor all items; consider conditioning on the flagRight now
disablealways rewrites every item withmutedAt: undefined, regardless of whethervalueistrueorfalse. That means any per‑item mute/snooze state is discarded both when disabling and when re‑enabling the widget, which may not be what you want ifmutedAtis meant to persist user intent.If the intent is “disable = hide the widget, but keep per‑item mute state” and “re‑enable = clear mutes so everything can resurface”, you could gate the
itemsupdate on the flag and avoid touchingitemswhen simply turning the widget off:- disable: (value: boolean) => { - universalChecklistStore.setState((state) => ({ - ...state, - widget: { ...state.widget, disable: value }, - items: Object.entries(state.items).reduce( - (acc, [id, value]) => ({ ...acc, [id]: { ...value, mutedAt: undefined } }), - state.items - ), - })); - }, + disable: (disabled: boolean) => { + universalChecklistStore.setState((state) => ({ + ...state, + widget: { ...state.widget, disable: disabled }, + items: disabled + ? state.items + : Object.entries(state.items).reduce( + (acc, [id, item]) => ({ ...acc, [id]: { ...item, mutedAt: undefined } }), + state.items + ), + })); + },This keeps existing mute state when you flip the global “disable” on, and only clears
mutedAtwhen you explicitly re‑enable the widget. Please adjust the condition if your intended semantics differ.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/cli/globalSettings.ts(1 hunks)code/core/src/shared/checklist-store/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/cli/globalSettings.ts
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/shared/checklist-store/index.ts (2)
code/core/src/cli/globalSettings.ts (1)
globalSettings(57-76)code/core/src/manager-api/stores/checklist.ts (1)
universalChecklistStore(9-12)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/shared/checklist-store/index.ts (1)
6-26: Universal checklist store typing and core operations look solidDeriving
StoreStatefrom the persistedglobalSettings().value.checklistshape and exposing it viaUNIVERSAL_CHECKLIST_STORE_OPTIONSgives a single source of truth for checklist state, while thecreateChecklistStorehelpers perform focused, immutable updates foraccept/done/skip/resetand preserve all unrelated fields. This wiring matches howUniversalStore.createis used inmanager-api/stores/checklist.ts, and nothing here stands out as a correctness or layering problem.Also applies to: 30-57
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager-api/stores/__mocks__/checklist.ts (1)
1-6: Consolidate imports from the same module.Lines 1-5 and line 6 both import from
'../../../shared/checklist-store'. Consider merging them into a single import statement for better readability.Apply this diff:
-import { - type StoreEvent, - type StoreState, - createChecklistStore, -} from '../../../shared/checklist-store'; -import { UNIVERSAL_CHECKLIST_STORE_OPTIONS } from '../../../shared/checklist-store'; +import { + type StoreEvent, + type StoreState, + createChecklistStore, + UNIVERSAL_CHECKLIST_STORE_OPTIONS, +} from '../../../shared/checklist-store';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager-api/stores/__mocks__/checklist.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager-api/stores/__mocks__/checklist.ts (1)
code/core/src/shared/checklist-store/index.ts (4)
StoreState(6-10)StoreEvent(15-21)UNIVERSAL_CHECKLIST_STORE_OPTIONS(23-26)createChecklistStore(30-78)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/manager-api/stores/__mocks__/checklist.ts (1)
9-13: LGTM!The mock correctly creates a functional
UniversalStoreinstance with production options and wires it to thechecklistStorevia the factory function. This approach enables integration-style testing with real store behavior.
…ing and ensuring proper type assertions.
…Case to avoid quotes
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
code/core/src/manager/components/sidebar/Menu.stories.tsx (2)
26-35: Provide a shared, fullerapistub foruseMenuto avoid runtime errors and duplicationThe
apiobjects passed intouseMenuonly define shortcut/docs helpers, butuseMenualso builds menu items that call methods likechangeSettingsTab,toggleNav,toggleToolbar,togglePanel,jumpToComponent,jumpToStory, andemit(perMenu.tsx). Clicking those items in these stories will currently throw, and the three inlineapiliterals duplicate a lot of logic.Consider defining a single shared
managerApistub that covers the Menu-related API surface and reusing it in bothManagerContextand alluseMenucalls, overriding only what each story cares about (e.g., shortcuts or WhatsNew):-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'), - }, -}; +const managerApi = { + 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'), + changeSettingsTab: fn().mockName('api::changeSettingsTab'), + toggleNav: fn().mockName('api::toggleNav'), + togglePanel: fn().mockName('api::togglePanel'), + toggleToolbar: fn().mockName('api::toggleToolbar'), + jumpToComponent: fn().mockName('api::jumpToComponent'), + jumpToStory: fn().mockName('api::jumpToStory'), + emit: fn().mockName('api::emit'), + getShortcutKeys: () => ({}), + getAddonsShortcuts: () => ({}), + versionUpdateAvailable: () => false, + isWhatsNewUnread: () => false, + getDocsUrl: () => 'https://storybook.js.org/docs/', +}; + +const managerContext: any = { + state: {}, + api: managerApi, +};Then reuse it in the stories, overriding only what differs:
- const menu = useMenu({ - api: { - // @ts-expect-error (Converted from ts-ignore) - getShortcutKeys: () => ({}), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, - isWhatsNewUnread: () => false, - getDocsUrl: () => 'https://storybook.js.org/docs/', - }, + const menu = useMenu({ + api: managerApi, showToolbar: false, isPanelShown: false, isNavShown: false, enableShortcuts: false, });- const menu = useMenu({ - api: { - // @ts-expect-error (invalid) - getShortcutKeys: () => ({ + const menu = useMenu({ + api: { + ...managerApi, + // Override shortcuts for this story + getShortcutKeys: () => ({ shortcutsPage: ['⌘', '⇧', ','], toggleNav: ['⌥', 'S'], togglePanel: ['⌥', 'A'], toolbar: ['⌥', 'T'], panelPosition: ['⌥', 'D'], fullScreen: ['⌥', 'F'], search: ['⌥', 'K'], prevComponent: ['⌥', '↑'], nextComponent: ['⌥', '↓'], prevStory: ['⌥', '←'], nextStory: ['⌥', '→'], collapseAll: ['⌥', '⇧', '↑'], }), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, - isWhatsNewUnread: () => false, - getDocsUrl: () => 'https://storybook.js.org/docs/', - }, + }, showToolbar: false, isPanelShown: false, isNavShown: false, enableShortcuts: true, });- const menu = useMenu({ - api: { - // @ts-expect-error (invalid) - getShortcutKeys: () => ({}), - getAddonsShortcuts: () => ({}), - versionUpdateAvailable: () => false, - isWhatsNewUnread: () => true, - getDocsUrl: () => 'https://storybook.js.org/docs/', - }, + const menu = useMenu({ + api: { + ...managerApi, + isWhatsNewUnread: () => true, + }, showToolbar: false, isPanelShown: false, isNavShown: false, enableShortcuts: false, });This keeps the stories robust if
useMenustarts using more of the API, and reduces duplication.Also applies to: 89-102, 139-165, 191-204
44-62: ResetmockStorebetween story runs instead of only mutating it inbeforeEach
beforeEachmutates the sharedinternal_universalChecklistStorewithout resetting it, so checklist state can leak across story renders. Also,asyncisn’t needed here. Prefer seeding the store and returning a cleanup function that restores the initial checklist state after each run.Example adjustment (tweak the reset state if your real initial
loadedvalue differs):- beforeEach: async () => { - mockStore.setState({ - loaded: true, - widget: {}, - items: { - ...initialState.items, - controls: { status: 'accepted' }, - renderComponent: { status: 'done' }, - viewports: { status: 'skipped' }, - }, - }); - }, + beforeEach: () => { + mockStore.setState({ + loaded: true, + widget: {}, + items: { + ...initialState.items, + controls: { status: 'accepted' }, + renderComponent: { status: 'done' }, + viewports: { status: 'skipped' }, + }, + }); + + return () => { + // Reset checklist store for subsequent stories + mockStore.setState({ + loaded: false, + ...initialState, + }); + }; + },code/core/src/core-server/utils/checklist.ts (2)
84-90: Guard telemetry calls and handle rejections to avoid unhandled promise errors
telemetry('onboarding-checklist', …)is called on every state change without error handling, and it may be invoked with an empty payload when neither items nor widget actually changed. Iftelemetryrejects, this can lead to unhandled promise rejections from within the store callback.You can both avoid no-op telemetry and make failures non-fatal:
- const changedValues = Object.entries(state.items).filter( - ([key, value]) => value !== previousState.items[key as keyof typeof state.items] - ); - telemetry('onboarding-checklist', { - ...(changedValues.length > 0 ? { items: Object.fromEntries(changedValues) } : {}), - ...(!deepEqual(state.widget, previousState.widget) ? { widget: state.widget } : {}), - }); + const changedValues = Object.entries(state.items).filter( + ([key, value]) => value !== previousState.items[key as keyof typeof state.items] + ); + const payload: Record<string, unknown> = { + ...(changedValues.length > 0 ? { items: Object.fromEntries(changedValues) } : {}), + ...(!deepEqual(state.widget, previousState.widget) ? { widget: state.widget } : {}), + }; + + if (Object.keys(payload).length > 0) { + void telemetry('onboarding-checklist', payload).catch((error) => { + logger.error('Failed to send onboarding checklist telemetry'); + logger.error(error); + }); + }This keeps telemetry fully fire‑and‑forget while ensuring errors are contained and noisy no-op calls are skipped. Based on learnings
35-44: Handle async persistence errors insetStatehelpers to avoid unhandled rejections
settings.save()is async (it’s awaited inglobalSettings), andcache.set()from the filesystem cache is also very likely async. Calling them withoutawaitor.catch()inside the onStateChange path means any write failure (permissions, disk issues) can surface as an unhandled promise rejection, even though initialization itself is protected by the outer try/catch.You can keep these as fire‑and‑forget writes but explicitly swallow/log failures:
const setState = ({ items = state.items, widget = state.widget, }: { items?: typeof state.items; widget?: typeof state.widget; }) => { settings.value.checklist = { items, widget }; - settings.save(); + void settings.save().catch((error) => { + logger.error('Failed to persist user checklist state'); + logger.error(error); + }); }; @@ cache.get<Pick<ChecklistState, 'items'>>('state').then((cachedState) => { const state = { items: cachedState?.items ?? {} }; - const setState = ({ items }: Pick<ChecklistState, 'items'>) => - cache.set('state', { items }); + const setState = ({ items }: Pick<ChecklistState, 'items'>) => { + void cache.set('state', { items }).catch((error: unknown) => { + logger.error('Failed to persist project checklist state'); + logger.error(error); + }); + }; return [state, setState] as const; }),This matches the repo’s preference for
.catch()on fire‑and‑forget side effects and prevents noisy/unhandled rejections while keeping the store callback sync. Based on learningsAlso applies to: 48-52
🧹 Nitpick comments (1)
code/core/src/shared/checklist-store/checklistData.tsx (1)
34-52: Avoid recomputing the dark theme for everyCodeSnippetrender
CodeSnippetcallsconvert(themes.dark)on every render. It’s cheap, but since the theme is static you can hoist it once to avoid unnecessary work and object identity changes:-const CodeSnippet = (props: ComponentProps<typeof SyntaxHighlighter>) => ( - <ThemeProvider theme={convert(themes.dark)}> +const darkCodeTheme = convert(themes.dark); + +const CodeSnippet = (props: ComponentProps<typeof SyntaxHighlighter>) => ( + <ThemeProvider theme={darkCodeTheme}> <CodeWrapper> <SyntaxHighlighter {...props} /> </CodeWrapper> </ThemeProvider> );This keeps behavior the same while reducing redundant conversions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
code/addons/onboarding/src/manager.tsx(1 hunks)code/core/src/cli/globalSettings.ts(3 hunks)code/core/src/core-server/utils/checklist.ts(1 hunks)code/core/src/manager-api/modules/versions.ts(3 hunks)code/core/src/manager-api/tests/versions.test.js(2 hunks)code/core/src/manager/components/sidebar/ChecklistWidget.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(4 hunks)code/core/src/manager/container/Menu.stories.tsx(2 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)code/core/src/manager/settings/GuidePage.tsx(1 hunks)code/core/src/shared/checklist-store/checklistData.state.ts(1 hunks)code/core/src/shared/checklist-store/checklistData.tsx(1 hunks)code/core/src/shared/checklist-store/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- code/core/src/manager/container/Menu.stories.tsx
- code/core/src/manager/settings/GuidePage.stories.tsx
- code/core/src/shared/checklist-store/checklistData.state.ts
- code/core/src/manager/components/sidebar/ChecklistWidget.stories.tsx
- code/core/src/manager/components/sidebar/Sidebar.stories.tsx
- code/core/src/manager/settings/GuidePage.tsx
- code/core/src/cli/globalSettings.ts
- code/core/src/shared/checklist-store/index.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-17T07:32:14.512Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.
Applied to files:
code/core/src/core-server/utils/checklist.ts
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/shared/checklist-store/checklistData.tsxcode/addons/onboarding/src/manager.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/shared/checklist-store/checklistData.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/addons/onboarding/src/manager.tsxcode/core/src/manager/components/sidebar/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/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/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: 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/Menu.stories.tsx
🧬 Code graph analysis (5)
code/core/src/manager-api/tests/versions.test.js (1)
code/core/src/manager-api/modules/versions.ts (1)
init(72-186)
code/core/src/core-server/utils/checklist.ts (2)
code/core/src/shared/checklist-store/index.ts (4)
StoreState(12-15)StoreEvent(20-26)UNIVERSAL_CHECKLIST_STORE_OPTIONS(28-31)ChecklistState(7-9)code/core/src/cli/globalSettings.ts (1)
globalSettings(62-81)
code/core/src/shared/checklist-store/checklistData.tsx (7)
code/core/src/theming/convert.ts (1)
convert(78-201)code/core/src/shared/checklist-store/index.ts (1)
ItemId(17-17)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)code/core/src/manager-api/root.tsx (2)
API(95-108)API_StoryEntry(308-308)code/addons/onboarding/src/constants.ts (1)
ADDON_ONBOARDING_CHANNEL(2-2)code/core/src/manager/components/TourGuide/TourGuide.tsx (1)
TourGuide(38-171)code/addons/vitest/src/constants.ts (1)
STORYBOOK_ADDON_TEST_CHANNEL(13-13)
code/addons/onboarding/src/manager.tsx (5)
code/addons/onboarding/src/Onboarding.tsx (1)
Onboarding(47-331)code/addons/onboarding/src/Survey.tsx (1)
Survey(11-47)code/addons/onboarding/src/constants.ts (2)
ADDON_ID(1-1)ADDON_CONTROLS_ID(5-5)code/core/src/manager-api/stores/checklist.ts (1)
checklistStore(14-14)code/core/src/manager-api/index.ts (1)
checklistStore(22-22)
code/core/src/manager/components/sidebar/Menu.stories.tsx (4)
code/core/src/manager-api/root.tsx (1)
ManagerContext(79-79)code/core/src/manager/components/layout/LayoutProvider.tsx (1)
LayoutProvider(29-63)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)code/core/src/manager/container/Menu.tsx (1)
useMenu(64-271)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (18)
code/core/src/manager-api/tests/versions.test.js (3)
229-229: LGTM: Minor capitalization consistency fix.Lowercasing "url" improves consistency with other test descriptions in the suite.
256-278: LGTM: Asset path test coverage added.This test correctly verifies that
asset: trueproduces adocs-assetsURL with the major.minor version segment. The test addresses the past review comment requesting coverage for the newassetsparameter.
280-304: LGTM: Subpath handling with asset flag verified.This test correctly validates that subpaths are appended directly (without a trailing slash) when
asset: true, which is appropriate for file paths like images. The version segment (7.2) is correctly included based on the current version (7.2.5).code/core/src/manager-api/modules/versions.ts (2)
51-63: LGTM: Clear documentation added for the asset parameter.The JSDoc comment now clearly documents the
assetoption and its purpose, addressing the past review request from ndelangen. The signature change to an options object provides better extensibility and clarity.
102-127: LGTM: Asset URL logic correctly implemented.The implementation correctly handles the asset flag:
- Base URL branches to
docs-assetswhenasset: true(line 107)- Version segment appended for assets using major.minor format (lines 109-111)
- Subpath handling distinguishes between file paths (assets, no trailing slash) and directory paths (non-assets, trailing slash) (line 127)
The logic is clean, maintains backward compatibility, and the test coverage confirms correctness.
code/addons/onboarding/src/manager.tsx (3)
1-11: LGTM! Proper React 18 upgrade and modular imports.The migration to React 18's
createRootAPI, lazy-loaded components, and constants import are all appropriate improvements.
13-23: LGTM! Clean React 18 rendering implementation.The persistent root pattern and idempotent container creation are appropriate. The Suspense wrapper correctly handles the lazy-loaded components.
46-67: LGTM! Well-structured onboarding flow with appropriate guards.The flow correctly:
- Waits for stories to load before checking existence
- Provides helpful warnings when button stories are missing
- Guards against narrow screens for better UX
- Passes survey completion state to the Onboarding component
The
getElementByIdfallback at line 50 is a reasonable defensive check, though it may not always find the element depending on render timing.code/core/src/manager/components/sidebar/Menu.stories.tsx (1)
9-16: Imports for ManagerContext, test helpers, and checklist store are consistent with usageThe new imports cleanly match how ManagerContext,
storybook/testhelpers,initialState, andmockStoreare used later in the stories; no changes suggested here.code/core/src/core-server/utils/checklist.ts (3)
56-62: State merge andloadedflag wiring looks correctMerging
userStateandprojectStateinto the universal store viatoMergedand then marking{ loaded: true }is a clean way to overlay persisted state on the initial options while keeping typing honest withsatisfies StoreState. No changes needed here.
64-83: Good separation of user-local vs project-local persistenceThe split of
state.itemsintoprojectValues(status === 'done') anduserValues(accepted/skippedplusmutedAtfor any item) nicely enforces the intended persistence boundary between project and user settings. The handling ofmutedAtas user-local even for done items is especially clear. Looks solid.
17-27: Initialization error handling is appropriateWrapping the entire initialization flow in a try/catch and logging failures via
logger.errorensures that checklist setup issues don’t break server startup, while still surfacing diagnostics. This is a reasonable failure mode for an auxiliary feature like onboarding.Also applies to: 92-95
code/core/src/shared/checklist-store/checklistData.tsx (6)
54-120: Checklist data typing is robust and future-proofDeriving
ItemIdfromtypeof initialState['items']and then expressingChecklistDatain terms of those item IDs (includingafter,available, andsubscribesignatures) is a solid way to keep the data structure aligned with the store’s state shape. This should make future item additions and refactors much safer.
122-134:subscribeToIndexhelper correctly handles initial and subsequent index availabilityThe helper’s pattern of checking the condition immediately, then once on
PREVIEW_INITIALIZED, and then on eachSTORY_INDEX_INVALIDATED(via the returned unsubscribe) is a clean abstraction for index-based completion. Usingapi.getIndex()?.entries || {}also guards nicely against undefined index instances. No changes needed.
136-345: Basics section logic aligns with described criteriaThe basics items (“guided tour”, survey, initial render, more components/stories, and “What’s new”) wire their
available,subscribe, andcriteriafields consistently:
- Gating the onboarding items on both example stories and addon registration is sensible.
- The
STORY_FINISHED-based completion forrenderComponentmatches the description.- Index-based counts for components and stories correctly ignore example entries.
Content and links look coherent and framework-agnostic. No functional issues spotted here.
348-477: Development section events and content look consistentThe development items hook into the right core events:
controlslistens toSTORY_ARGS_UPDATED, which matches “Change a story with Controls”.viewportslistens toUPDATE_GLOBALSand checksglobals.viewport, correctly reflecting viewport changes.organizeStoriesusessubscribeToIndexand checks for titles containing/, which is a reasonable heuristic for grouped components.The accompanying copy and examples (including the hierarchy snippet) are accurate and nicely aligned with current Storybook patterns.
479-987: Testing section wiring matches intended behaviorsThe testing section’s items map well to their criteria:
- Install steps (
installVitest,installA11y,installChromatic,installDocs) useexperimental_getRegisteredAddons()to detect registration, which is consistent with the rest of the codebase, and the copy explicitly tells users to restart Storybook after installation.runTestsleveragesinternal_universalTestProviderStore.onStateChangeto detect a succeeded test-provider state.accessibilityTestsandcoveragesubscribe toSTORYBOOK_ADDON_TEST_CHANNELand checkpayload.config.a11y/payload.config.coverage, matching how the Vitest addon reports its config.visualTestsuses the Chromatic addon’s event channel for build starts, consistent with the install criteria and UI copy.Overall, the event wiring, addon IDs, and user-facing descriptions are coherent; no changes recommended here.
989-1209: Documentation section items correctly reflect docs and index behaviorThe documentation items:
- Detect addon-docs installation via
experimental_getRegisteredAddonsand the docs addon ID.- Use
subscribeToIndexto look forautodocstags and MDX doc entries (type === 'docs'), which matches how the index is structured.- Provide MDX and autodocs snippets plus links to the relevant docs, and “publish Storybook” guidance is consistent with current workflows.
The structure and wiring look sound; no functional or semantic HTML issues stand out in this section.
| const hasCompletedSurvey = await new Promise<boolean>((resolve) => { | ||
| const unsubscribe = checklistStore.onStateChange(({ loaded, items }) => { | ||
| if (loaded) { | ||
| unsubscribe(); | ||
| resolve(items.onboardingSurvey.status === 'accepted'); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add defensive checks and timeout for store loading.
This promise-based store check has two critical issues:
- Line 37: Direct property access to
items.onboardingSurveywithout existence check could throw a runtime error if the property is missing or undefined. - No timeout: If the store never emits a
loadedstate, this promise will hang indefinitely, blocking the addon registration.
Apply this diff to add defensive checks and a timeout:
- const hasCompletedSurvey = await new Promise<boolean>((resolve) => {
+ const hasCompletedSurvey = await Promise.race([
+ new Promise<boolean>((resolve) => {
- const unsubscribe = checklistStore.onStateChange(({ loaded, items }) => {
- if (loaded) {
- unsubscribe();
- resolve(items.onboardingSurvey.status === 'accepted');
- }
- });
+ const unsubscribe = checklistStore.onStateChange(({ loaded, items }) => {
+ if (loaded) {
+ unsubscribe();
+ resolve(items.onboardingSurvey?.status === 'accepted');
+ }
+ });
+ }),
+ new Promise<boolean>((resolve) => setTimeout(() => resolve(false), 5000))
- });
+ ]);📝 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 hasCompletedSurvey = await new Promise<boolean>((resolve) => { | |
| const unsubscribe = checklistStore.onStateChange(({ loaded, items }) => { | |
| if (loaded) { | |
| unsubscribe(); | |
| resolve(items.onboardingSurvey.status === 'accepted'); | |
| } | |
| }); | |
| }); | |
| const hasCompletedSurvey = await Promise.race([ | |
| new Promise<boolean>((resolve) => { | |
| const unsubscribe = checklistStore.onStateChange(({ loaded, items }) => { | |
| if (loaded) { | |
| unsubscribe(); | |
| resolve(items.onboardingSurvey?.status === 'accepted'); | |
| } | |
| }); | |
| }), | |
| new Promise<boolean>((resolve) => setTimeout(() => resolve(false), 5000)) | |
| ]); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
code/core/src/manager/components/sidebar/Menu.stories.tsx (4)
141-167: Incomplete api stubs will cause runtime errors when menu items are clicked.Same issue as lines 91-104: the
apiobject is missing action methods thatuseMenutries to call. This story will also throw at runtime if menu items are clicked.
53-64: beforeEach hook still doesn't follow cleanup pattern and is unnecessarily async.This duplicates the previous review's concern: the
beforeEachhook should return a cleanup function rather than directly callingsetStateto ensure proper state reset between story renders.Additionally, the
asynckeyword is unnecessary since there are noawaitcalls inside the function.Apply this diff:
- beforeEach: async () => { - mockStore.setState({ + beforeEach: () => { + return () => { + mockStore.setState({ + loaded: true, + widget: {}, + items: { + ...initialState.items, + controls: { status: 'accepted' }, + renderComponent: { status: 'done' }, + viewports: { status: 'skipped' }, + }, + }); + }; - loaded: true, - widget: {}, - items: { - ...initialState.items, - controls: { status: 'accepted' }, - renderComponent: { status: 'done' }, - viewports: { status: 'skipped' }, - }, - }); },
91-104: Incomplete api stubs will cause runtime errors when menu items are clicked.This duplicates the previous review's concern: the
apiobject passed touseMenuis missing action methods (changeSettingsTab,toggleNav,toggleToolbar,togglePanel,jumpToComponent,jumpToStory,emit) thatuseMenutries to call in menu item onClick handlers. Clicking those menu items in this story will throw at runtime.
193-206: Incomplete api stubs will cause runtime errors when menu items are clicked.Same issue as lines 91-104 and 141-167: the
apiobject is missing action methods. This story will also throw at runtime if menu items are clicked.code/core/src/manager/components/sidebar/Sidebar.stories.tsx (1)
5-5: UseglobalThisinstead of importing from@storybook/global.To avoid adding unnecessary dependencies, use
globalThisdirectly instead of importingglobalfrom@storybook/global. This import is used on lines 192-193 and 195 for accessingCONFIG_TYPE.Based on learnings.
Apply this diff:
-import { global } from '@storybook/global';Then update the usage on lines 192-193 and 195:
- const configType = global.CONFIG_TYPE; - global.CONFIG_TYPE = 'PRODUCTION'; + const configType = globalThis.CONFIG_TYPE; + globalThis.CONFIG_TYPE = 'PRODUCTION'; return () => { - global.CONFIG_TYPE = configType; + globalThis.CONFIG_TYPE = configType; };
🧹 Nitpick comments (1)
code/core/src/manager/settings/GuidePage.stories.tsx (1)
14-18: Address the TODO comment for hard-coded version.The hard-coded version "10.0" in the docs URL should be resolved to use the actual major.minor version of the latest release dynamically.
Do you want me to open a new issue to track this task, or would you like suggestions on how to resolve this dynamically?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/manager/components/sidebar/Menu.stories.tsx(6 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(4 hunks)code/core/src/manager/settings/GuidePage.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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/settings/GuidePage.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.stories.tsxcode/core/src/manager/settings/GuidePage.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.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/settings/GuidePage.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: 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/settings/GuidePage.stories.tsxcode/core/src/manager/components/sidebar/Menu.stories.tsx
🧬 Code graph analysis (3)
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
code/core/src/manager/manager-stores.mock.ts (1)
internal_universalChecklistStore(52-61)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)
code/core/src/manager/settings/GuidePage.stories.tsx (2)
code/core/src/manager/settings/GuidePage.tsx (1)
GuidePage(37-67)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)
code/core/src/manager/components/sidebar/Menu.stories.tsx (3)
code/core/src/manager/components/layout/LayoutProvider.tsx (1)
LayoutProvider(29-63)code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)code/core/src/manager/container/Menu.tsx (1)
useMenu(64-271)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/manager/components/sidebar/Menu.stories.tsx (3)
9-15: LGTM: Imports support new ManagerContext and checklist integration.The added imports are correctly used throughout the file to support the new ManagerContext decoration and mock store initialization.
26-37: LGTM: Manager context mock is appropriate for test isolation.The mock
managerContextprovides necessary API method stubs for ManagerContext.Provider and is correctly separated from theapiobjects passed directly touseMenu.
46-52: LGTM: Decorator structure correctly provides contexts.The decorator nesting provides ManagerContext and LayoutProvider in the correct order for stories to access necessary contexts.
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (2)
13-17: LGTM! Necessary imports for checklist functionality.The imports of
initialStateandinternal_universalChecklistStoreare required to support the new onboarding checklist feature in the sidebar.
119-131: LGTM! Checklist store initialization is appropriate.The
beforeEachsetup properly initializes the checklist store with test data, consistent with the pattern used in GuidePage.stories.tsx. The mock state includes realistic item statuses (accepted, done, skipped) for testing the sidebar's checklist functionality.
ghengeveld
force-pushed
the
onboarding-checklist
branch
from
5a6b7fa to
5f29a50
Compare
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/19563973710 |
ghengeveld
added
the
needs qa
yannbf
removed
the
needs qa
Closes #32644
What I did
Warning
The docs site must be redeployed after merging this PR, but before it is published in a release. Message @kylegach with any questions.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
npx storybook@<canary> initwith onboarding (when prompted)npx storybook@<canary> initwithout onboardingnpx storybook@<canary> upgrade(without onboarding)~/.storybook/settings.jsonand verify it containsmuted,acceptedandskippeditems. Remove thechecklistproperty altogether and restart Storybook.node_modules/.cache/storybook/default/storybook-*(as JSON) and confirm it containsdoneitems. Delete the cache file and restart Storybook.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-32795-sha-2ff8d295. 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-32795-sha-2ff8d295onboarding-checklist2ff8d2951763711139)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=32795Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.