Core: Onboarding checklist #32795
Core: Onboarding checklist #32795
Conversation
…e whole UI up and showing empty space below)
📝 WalkthroughWalkthroughAdds a checklist/guided-tour feature: shared checklist store and persistence via global settings, server initialization, new UI primitives (Card, Collapsible, FocusProxy, Listbox), manager UI (ChecklistModule, Checklist, GuidedTourPage), story entries, and small layout/transition and UrlStore adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server
participant Settings
participant Store
participant Manager
participant Explorer
Server->>Settings: load global settings
Settings-->>Server: { checklist?: { completed, skipped } }
Server->>Store: initializeChecklist() (creates UniversalStore, seed state)
Store-->>Server: ready
Note over Store,Settings: on Store state change
Store->>Settings: settings.value.checklist = state
Settings->>Settings: save()
Manager->>Explorer: render
Explorer->>Manager: mount ChecklistModule
Manager->>Store: checklistStore.complete(id) / skip(id)
Store-->>Manager: emits state change
Manager->>Manager: re-render Checklist UI (progress update)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b91e701
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://cdn.statically.io/img/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.
| @@ -0,0 +1,98 @@ | |||
| import React, { type ComponentProps } from 'react'; | |||
|
|
|||
| import type { CSSObject, color } from 'storybook/theming'; | |||
There was a problem hiding this comment.
Fix TS typing: typeof color on a type‑only import will fail
import type { ... color } plus keyof typeof color is invalid; typeof requires a value import. Use the theme type instead.
Apply:
-import type { CSSObject, color } from 'storybook/theming';
+import type { CSSObject, Theme } from 'storybook/theming';
@@
- outlineColor?: keyof typeof color;
+ outlineColor?: keyof Theme['color'];
@@
- color?: keyof typeof color;
+ color?: keyof Theme['color'];Optionally rename the Outline prop color to outlineColor to avoid confusion with CSS color and theme.color keys.
Also applies to: 35-37, 52-54
🤖 Prompt for AI Agents
In code/core/src/components/components/Card/Card.tsx around lines 3 (and
similarly at 35-37 and 52-54), the code uses a type-only import "color" with
"keyof typeof color", which fails because "typeof" requires a value import;
replace the type usage with the Storybook theme type (import type { Theme } from
'storybook/theming') and change typings to use keyof Theme['color'] (or
Theme['color'] directly where appropriate) instead of keyof typeof color; update
the Outline prop name from color to outlineColor (and update usages/props
destructuring) to avoid confusion with CSS color and theme.color keys; ensure
imports remain type-only and run TypeScript to verify no remaining typeof value
usages.
| outline: `2px solid transparent`, | ||
| outlineOffset, | ||
|
|
||
| [`&:focus, &:has(#${htmlFor}:focus-visible)`]: { |
There was a problem hiding this comment.
Potential CSS selector injection risk with unsanitized htmlFor prop.
The htmlFor prop is directly interpolated into a CSS selector without sanitization: [`&:focus, &:has(#${htmlFor}:focus-visible)`]. If htmlFor contains special CSS characters (e.g., , ], ), ;), it could break the selector or potentially lead to CSS injection attacks.
Consider one of these solutions:
Solution 1 (Recommended): Add validation to ensure htmlFor is a valid ID
+const isValidId = (id: string) => /^[a-zA-Z][\w-]*$/.test(id);
+
export const FocusProxy = styled.div<{ htmlFor: string; outlineOffset?: number }>(
({ theme, htmlFor, outlineOffset = 0 }) => ({
+ ...(isValidId(htmlFor) ? {} : { display: 'none' }), // or throw error
width: '100%',
borderRadius: 'inherit',
transition: 'outline-color var(--transition-duration, 0.2s)',
outline: `2px solid transparent`,
outlineOffset,
[`&:focus, &:has(#${htmlFor}:focus-visible)`]: {
outlineColor: theme.color.secondary,
},
})
);Solution 2: Use CSS.escape() if available in your environment
export const FocusProxy = styled.div<{ htmlFor: string; outlineOffset?: number }>(
({ theme, htmlFor, outlineOffset = 0 }) => ({
width: '100%',
borderRadius: 'inherit',
transition: 'outline-color var(--transition-duration, 0.2s)',
outline: `2px solid transparent`,
outlineOffset,
- [`&:focus, &:has(#${htmlFor}:focus-visible)`]: {
+ [`&:focus, &:has(#${CSS.escape(htmlFor)}:focus-visible)`]: {
outlineColor: theme.color.secondary,
},
})
);📝 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.
| [`&:focus, &:has(#${htmlFor}:focus-visible)`]: { | |
| [`&:focus, &:has(#${CSS.escape(htmlFor)}:focus-visible)`]: { |
🤖 Prompt for AI Agents
In code/core/src/components/components/FocusProxy/FocusProxy.tsx around line 11,
the template literal injects htmlFor directly into a CSS selector which can
break the selector or enable CSS injection; validate that htmlFor is a safe ID
(e.g. match /^[A-Za-z][A-Za-z0-9\-_:\.]*$/ or your app's ID policy) and refuse
or sanitize values that don't match, or alternatively escape the value with
CSS.escape() (and provide a small fallback escape routine if CSS.escape is
unavailable) before interpolating into the selector.
| {task.start && ( | ||
| <ListboxButton | ||
| onClick={() => { | ||
| checklistStore.complete(task.id); | ||
| task.start?.({ api }); | ||
| }} | ||
| > | ||
| Start | ||
| </ListboxButton> | ||
| )} |
There was a problem hiding this comment.
Don’t mark tasks complete on “Start” click.
This records completion before the task actually completes. Let predicates or the destination UI mark completion, or do it after success.
Apply:
- <ListboxButton
- onClick={() => {
- checklistStore.complete(task.id);
- task.start?.({ api });
- }}
- >
+ <ListboxButton
+ onClick={() => {
+ // Navigate/trigger start; completion should occur via predicate or explicit action later
+ task.start?.({ api });
+ }}
+ >
Start
</ListboxButton>If you need “started” state, consider adding a separate flag in the store instead of conflating with “completed”.
🤖 Prompt for AI Agents
In code/core/src/manager/components/sidebar/ChecklistModule.tsx around lines
62–71, the Start button handler incorrectly calls
checklistStore.complete(task.id) and thus marks tasks complete before they
actually finish; remove that complete call from the onClick so Start only
triggers task.start({ api }) (or sets a separate "started" flag). If you need to
track started state, add a started boolean and a
checklistStore.setStarted(task.id, true) action, update the store type and
consumers to read that flag, and ensure completion is only invoked by the task's
success callback or the destination UI.
| export const Checklist = ({ data }: { data: ChecklistData }) => { | ||
| const api = useStorybookApi(); | ||
| const [checklistState] = experimental_useUniversalStore(universalChecklistStore); | ||
| const { completed, skipped } = checklistState; | ||
| const [hash, setHash] = useState(globalThis.window.location.hash ?? ''); | ||
|
|
||
| useEffect(() => { | ||
| const updateHash = () => setHash(globalThis.window.location.hash ?? ''); | ||
| const interval = setInterval(updateHash, 100); | ||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
Guard window access and replace 100ms polling with hashchange
Avoid SSR/test crashes and reduce work by listening to hashchange.
- const [hash, setHash] = useState(globalThis.window.location.hash ?? '');
+ const [hash, setHash] = useState(
+ typeof window !== 'undefined' ? window.location.hash ?? '' : ''
+ );
@@
- useEffect(() => {
- const updateHash = () => setHash(globalThis.window.location.hash ?? '');
- const interval = setInterval(updateHash, 100);
- return () => clearInterval(interval);
- }, []);
+ useEffect(() => {
+ if (typeof window === 'undefined') return;
+ const onHashChange = () => setHash(window.location.hash ?? '');
+ window.addEventListener('hashchange', onHashChange, { passive: true });
+ return () => window.removeEventListener('hashchange', onHashChange);
+ }, []);🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/Checklist.tsx around lines 217 to
227, the component directly accesses globalThis.window and uses a 100ms polling
interval; change this to safely guard against server/test environments and use
the native "hashchange" event instead. Initialize hash using a check like typeof
window !== 'undefined' ? window.location.hash : '' and in useEffect,
early-return if window is undefined; otherwise add a
window.addEventListener('hashchange', handler) that updates state and remove the
listener in the cleanup; remove the setInterval usage and any associated
clearInterval to eliminate polling.
| // Focus the target element when the hash changes | ||
| useEffect(() => { | ||
| const timeout = setTimeout(() => { | ||
| const target = | ||
| targetHash && globalThis.document.querySelector(`[for="toggle-${targetHash}"]`); | ||
| if (target instanceof HTMLElement) { | ||
| target.focus({ preventScroll: true }); | ||
| target.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| }, 200); | ||
| return () => clearTimeout(timeout); | ||
| }, [targetHash]); |
There was a problem hiding this comment.
Fix deep‑link focus: wrong selector attribute
[for="toggle-…"] doesn’t match anything here. Target the button by id.
- useEffect(() => {
- const timeout = setTimeout(() => {
- const target =
- targetHash && globalThis.document.querySelector(`[for="toggle-${targetHash}"]`);
- if (target instanceof HTMLElement) {
- target.focus({ preventScroll: true });
- target.scrollIntoView({ behavior: 'smooth', block: 'center' });
- }
- }, 200);
- return () => clearTimeout(timeout);
- }, [targetHash]);
+ useEffect(() => {
+ const timeout = setTimeout(() => {
+ if (typeof document === 'undefined' || !targetHash) return;
+ const el = document.getElementById(`toggle-${targetHash}`);
+ if (el instanceof HTMLElement) {
+ el.focus({ preventScroll: true });
+ el.scrollIntoView({ behavior: 'smooth', block: 'center' });
+ }
+ }, 200);
+ return () => clearTimeout(timeout);
+ }, [targetHash]);📝 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.
| // Focus the target element when the hash changes | |
| useEffect(() => { | |
| const timeout = setTimeout(() => { | |
| const target = | |
| targetHash && globalThis.document.querySelector(`[for="toggle-${targetHash}"]`); | |
| if (target instanceof HTMLElement) { | |
| target.focus({ preventScroll: true }); | |
| target.scrollIntoView({ behavior: 'smooth', block: 'center' }); | |
| } | |
| }, 200); | |
| return () => clearTimeout(timeout); | |
| }, [targetHash]); | |
| // Focus the target element when the hash changes | |
| useEffect(() => { | |
| const timeout = setTimeout(() => { | |
| if (typeof document === 'undefined' || !targetHash) return; | |
| const el = document.getElementById(`toggle-${targetHash}`); | |
| if (el instanceof HTMLElement) { | |
| el.focus({ preventScroll: true }); | |
| el.scrollIntoView({ behavior: 'smooth', block: 'center' }); | |
| } | |
| }, 200); | |
| return () => clearTimeout(timeout); | |
| }, [targetHash]); |
🤖 Prompt for AI Agents
In code/core/src/manager/settings/Checklist/Checklist.tsx around lines 264 to
275, the effect tries to find the target using `[for="toggle-${targetHash}"]`
which matches no element; change the lookup to target the button by id instead
(e.g. use document.getElementById(`toggle-${targetHash}`) or
document.querySelector(`#toggle-${targetHash}`)), keep the instanceof
HTMLElement check, then call focus({preventScroll: true}) and scrollIntoView as
before and preserve the timeout/cleanup logic.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 30.14 MB | 30.19 MB | 🚨 +47 KB 🚨 |
| Dependency size | 17.36 MB | 17.36 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 927 KB | 927 KB | 0 B |
| Dependency size | 79.92 MB | 79.97 MB | 🚨 +46 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -42 B 🎉 |
| Dependency size | 76.35 MB | 76.39 MB | 🚨 +47 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 🎉 -30 B 🎉 |
| Dependency size | 47.50 MB | 47.55 MB | 🚨 +47 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.
| debug: true, | ||
| }, | ||
| testUtils | ||
| ) as unknown as UniversalStore<StoreState, StoreEvent>; |
There was a problem hiding this comment.
Can we fix typings so that this is not necessary? cc @JReinhold
See #32644
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Improvements