CSF-Factories: Handle meta.play and type errors#33353
Conversation
|
I am fixing this here: The solution there is based on using |
valentinpalkovic
changed the title
valentinpalkovic
added
the
ci:normal
📝 WalkthroughWalkthroughThis PR exports factory functions Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/csf/csf-factories.ts (1)
121-137:Story.composedreturns aplayfield that the type doesn’t declare
Story['composed']is declared as:composed: Pick<ComposedStoryFn<TRenderer>, 'argTypes' | 'parameters' | 'id' | 'tags' | 'globals'> & { args: TRenderer['args']; name: string; };but the getter now returns
{ ..., play: composed.play }and tests accessStory.composed.play. That means at runtimeplayexists, but TypeScript still thinks.playis not part of the type, which defeats the goal of avoiding type errors aroundmeta.play/ composed play.Consider updating the type to include
play, for example:composed: Pick< ComposedStoryFn<TRenderer>, 'argTypes' | 'parameters' | 'id' | 'tags' | 'globals' | 'play' > & { args: TRenderer['args']; name: string; };(or a similar addition using
ComposedStoryFn<TRenderer>['play']), soStory.composed.playis both valid at runtime and correctly typed.Also applies to: 179-195
♻️ Duplicate comments (2)
code/core/src/csf/csf-factories.spec.ts (1)
1-15: This play-inheritance test duplicates coverage already added elsewhereThe spec here re-tests exactly the same scenario as the new
CSF Next play inheritancetests in the other csf-factories test files (same meta definition, same assertions onStory.playandStory.composed.play).To avoid redundant maintenance and slower test runs, I’d consolidate this into a single test module and remove the duplicates across the suite.
code/core/src/csf/csf-factories.play-inheritance.test.ts (1)
1-15: Another duplicate of the same play-inheritance scenarioThis file repeats the same
CSF Next play inheritancetest already present in other csf-factories test modules. Keeping multiple identical copies makes it harder to evolve the behavior later.Once you pick a canonical location for this scenario, consider removing the extras (including this file) to keep tests DRY.
🧹 Nitpick comments (6)
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.ts (1)
1-15: Test covers the right behavior but duplicates other suitesThis test correctly asserts that both
Story.playandStory.composed.playinherit frommeta.play. However, the exact same scenario is also tested in other files (csf-factories.spec.ts,csf-factories.play-inheritance.test.ts, and incsf-factories.test.ts), which is redundant and will run the same assertions multiple times.Consider keeping this scenario in a single canonical place and removing the duplicates to keep the test suite lean and easier to maintain.
code/core/src/csf/csf-factories.test.ts (1)
7-20: New play-inheritance test is correct but adds yet another duplicateThe added
CSF Next play inheritancetest here is functionally identical to the tests introduced in:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.play-inheritance.test.tsIt’s good coverage, but having four copies of the same assertions is overkill. I’d keep this scenario in one well-chosen place (e.g., this main
.test.tsfile) and delete the others.code/src/stories/Page.tsx (1)
18-22: Addtype="button"to prevent unintended form submission.The buttons lack an explicit
typeattribute. If this component is ever nested within a form, these buttons could trigger form submission instead of invoking their handlers.Apply this diff:
- <button onClick={onLogout}>Log out</button> + <button type="button" onClick={onLogout}>Log out</button> ) : ( <> - <button onClick={onLogin}>Log in</button> - <button onClick={onCreateAccount}>Sign up</button> + <button type="button" onClick={onLogin}>Log in</button> + <button type="button" onClick={onCreateAccount}>Sign up</button>code/core/src/stories/Page.tsx (2)
1-37: Code duplication: identical Page component exists in two locations.This file is identical to
code/src/stories/Page.tsx. Consider consolidating to a single shared location and importing from both story files, or document if this duplication is intentional for demonstration purposes.
18-22: Addtype="button"to prevent unintended form submission.The buttons lack an explicit
typeattribute. If this component is ever nested within a form, these buttons could trigger form submission instead of invoking their handlers.Apply this diff:
- <button onClick={onLogout}>Log out</button> + <button type="button" onClick={onLogout}>Log out</button> ) : ( <> - <button onClick={onLogin}>Log in</button> - <button onClick={onCreateAccount}>Sign up</button> + <button type="button" onClick={onLogin}>Log in</button> + <button type="button" onClick={onCreateAccount}>Sign up</button>code/src/stories/Page.stories.ts (1)
18-18: Consider adding args to LoggedOut story for consistency.While an empty story object works, explicitly setting
args: { user: undefined }as incode/core/src/stories/Page.stories.ts(line 19) would make the intent clearer and match the pattern in the core stories file.-export const LoggedOut: Story = {}; +export const LoggedOut: Story = { + args: { + user: undefined, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const loginButton = canvas.getByRole('button', { name: /Log in/i }); + await expect(loginButton).toBeInTheDocument(); + }, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
code/.nx/workspace-data/lockfile-dependencies.hash(1 hunks)code/.nx/workspace-data/lockfile-nodes.hash(1 hunks)code/.nx/workspace-data/vite-1375197718504038785.hash(1 hunks)code/.storybook/main.ts(1 hunks)code/core/src/csf/__tests__/csf-factories.play-inheritance.test.ts(1 hunks)code/core/src/csf/csf-factories.play-inheritance.test.ts(1 hunks)code/core/src/csf/csf-factories.spec.ts(1 hunks)code/core/src/csf/csf-factories.test.ts(1 hunks)code/core/src/csf/csf-factories.ts(6 hunks)code/core/src/csf/story.ts(4 hunks)code/core/src/stories/Page.stories.ts(1 hunks)code/core/src/stories/Page.tsx(1 hunks)code/package.json(1 hunks)code/src/stories/Page.stories.ts(1 hunks)code/src/stories/Page.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/story.tscode/package.jsoncode/src/stories/Page.tsxcode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.tsxcode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/story.tscode/src/stories/Page.tsxcode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.tsxcode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/story.tscode/src/stories/Page.tsxcode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.tsxcode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/story.tscode/src/stories/Page.tsxcode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.tsxcode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/story.tscode/package.jsoncode/src/stories/Page.tsxcode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.tsxcode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
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.
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
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.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/stories/Page.stories.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.play-inheritance.test.tscode/core/src/csf/csf-factories.spec.tscode/core/src/csf/csf-factories.test.ts
📚 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/csf/story.ts
📚 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/csf/story.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/csf/story.tscode/.storybook/main.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/package.json
📚 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/src/stories/Page.tsxcode/core/src/stories/Page.tsxcode/.storybook/main.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
code/core/src/stories/Page.stories.tscode/.storybook/main.tscode/src/stories/Page.stories.tscode/core/src/csf/csf-factories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/core/src/csf/csf-factories.ts
🧬 Code graph analysis (5)
code/core/src/csf/__tests__/csf-factories.play-inheritance.test.ts (1)
code/core/src/csf/csf-factories.ts (4)
meta(55-58)defineMeta(102-120)Story(122-154)defineStory(160-266)
code/core/src/stories/Page.stories.ts (4)
code/core/src/csf/csf-factories.ts (3)
meta(55-58)Meta(77-96)Story(122-154)code/core/src/stories/Page.tsx (1)
Page(12-37)code/src/stories/Page.tsx (1)
Page(12-37)code/src/stories/Page.stories.ts (2)
LoggedOut(18-18)LoggedIn(20-31)
code/core/src/csf/csf-factories.spec.ts (1)
code/core/src/csf/csf-factories.ts (4)
meta(55-58)defineMeta(102-120)Story(122-154)defineStory(160-266)
code/src/stories/Page.stories.ts (3)
code/core/src/stories/Page.tsx (1)
Page(12-37)code/src/stories/Page.tsx (1)
Page(12-37)code/core/src/stories/Page.stories.ts (2)
LoggedOut(18-27)LoggedIn(30-39)
code/core/src/csf/csf-factories.test.ts (1)
code/core/src/csf/csf-factories.ts (5)
test(201-229)meta(55-58)defineMeta(102-120)Story(122-154)defineStory(160-266)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (11)
code/.nx/workspace-data/vite-1375197718504038785.hash (1)
1-1: Auto-generated workspace metadata file.This is an Nx workspace cache file and does not require functional review. These files are conventionally excluded from detailed code reviews as they are auto-generated during the build/dev process.
code/.nx/workspace-data/lockfile-dependencies.hash (1)
1-1: Auto-generated workspace metadata file.This is an Nx workspace cache file and does not require functional review. These files are conventionally excluded from detailed code reviews as they are auto-generated during the build/dev process.
code/.nx/workspace-data/lockfile-nodes.hash (1)
1-1: Auto-generated workspace metadata file.This is an Nx workspace cache file and does not require functional review. These files are conventionally excluded from detailed code reviews as they are auto-generated during the build/dev process.
code/package.json (1)
251-255: Adding jsdom as a dev dependency is appropriateUsing
jsdom(plus@types/jsdom) as devDependencies matches the Vitestjsdomenvironment change and avoids shipping it at runtime. No issues here.code/core/src/csf/story.ts (1)
219-221: WideningglobalstoRecord<string, any>looks consistentSwitching various
globalsproperties toRecord<string, any>(inStoryContextUpdate,ProjectAnnotations.initialGlobals,ComponentAnnotations.globals, andStoryAnnotations.globals) is consistent and better matches real-world usage where globals are arbitrary bags. This should also reduce type-friction around meta/preview globals.Also applies to: 419-420, 512-513, 531-532
code/core/src/csf/csf-factories.ts (3)
102-120: PublicdefineMetashape looks goodExposing
defineMetawith a typed(input, preview) => Metareturn value matches howPreview.meta()delegates and keeps the generics wired correctly. The internalstory()helper forwarding todefineStorypreserves existing behavior.
195-226: Play inheritance andtest/extendwiring look correct
get play()now consistently prefersinput.play, thenmeta.input?.play, then a noop, which ensuresStory.playis always callable for tests and composed usage.- The
test()helper wraps the providedtestFunctionso thatstory.play(including meta-level play) always runs before the test, and still respectsmountDestructuredwhen needed.extend()now merges fromstory.input(notthis), combining args, argTypes, lifecycle hooks, decorators, globals, loaders, parameters, and tags viacombineParameters/combineTagsandnormalizeArrays, which is the right direction for inheritance.No functional issues spotted here.
Also applies to: 231-256
262-264: Deadplayinheritance check is misleadingThis block:
// Inherit play from meta if not defined if (!('play' in story) && meta.input?.play) { story.play = meta.input.play; }never runs, because
storyalready has aplaygetter, so'play' in storyis always true. The actual inheritance is handled by the getter (input.play ?? meta.input?.play), so this check is redundant and potentially confusing.Either remove this block or, if you intended to materialize
input.play, change the condition/target accordingly (e.g., operate oninputinstead ofstory).⛔ Skipped due to learnings
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.code/.storybook/main.ts (1)
19-22: Including../core/src/storiesinstoriesconfig looks correctAdding the
directory: '../core/src/stories'withtitlePrefix: 'example'follows existing patterns in this file and should surface the new example stories as intended.code/core/src/csf/csf-factories.test.ts (1)
2-3: Removejsdomenvironment; these tests are not DOM-centricThe test suite (
CSF Next play inheritance, addon parameters, and test function registration) contains only pure JavaScript factory function tests with no DOM operations. Using// @vitest-environment jsdomadds unnecessary overhead. Change to the defaultnodeenvironment since there are no DOM interactions, event handling, focus management, or timers being tested.Likely an incorrect or invalid review comment.
code/core/src/stories/Page.stories.ts (1)
1-39: LGTM! Correct story implementation with proper args and assertions.Both stories correctly provide the necessary
argsto set component state and use play functions to assert the expected UI elements are rendered. This is the proper pattern for testing presentational components.
|
Thanks for the PR anway, will merge the other one today |
3 participants
Fixes #33024
What was fixed
meta.playis handled correctly and no longer throws at runtimemeta.input.playHow it was tested
Screenshots
Screenshots attached showing failing interactions before the fix and passing
interactions after the fix.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.