Skip to content

Conversation

@kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Dec 13, 2025

Closes #32620, Closes #32831, Closes #33024

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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

    • Play handlers can be composed via meta-level input, allowing story-level play to forward to meta.input.play.
  • Refactor

    • More accurate story selection and richer metadata collection, including parameter checks and inclusion of child (test) stories with IDs.
    • Project-level composed preview source now follows the selected factory story.
    • Stricter React preview typings and new overloads for more precise args handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@nx-cloud
Copy link

nx-cloud bot commented Dec 13, 2025

View your CI Pipeline Execution ↗ for commit 02804b5

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 10m 11s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-17 14:03:16 UTC

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes several CSF (Component Story Format) factory bugs related to type inference and story processing. The changes address three issues: incorrect handling of non-story exports in factory-based CSF files, type inference problems with meta args, and missing access to meta.input.play.

Key changes:

  • Fixed CSF file processing to correctly identify story exports instead of assuming the first export is a story
  • Improved TypeScript type inference for React meta factory args
  • Added new overload to support calling meta.story() without arguments when all required args are provided in meta

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
code/renderers/react/src/preview.tsx Improved TypeScript types for ReactPreview meta return type, changed story method signatures from optional to required parameters, and added new overload for parameterless story() calls
code/renderers/react/src/csf-factories.test.tsx Updated test to use meta.story() instead of meta.story({}) to match new API, and added test verifying meta.input.play accessibility
code/core/src/preview-api/modules/store/csf/processCSFFile.ts Fixed bug where first export was assumed to be a story by using find() to locate actual story exports, and added defensive check to ensure exports are Story objects in factory mode

Comment on lines +52 to +53
const factoryStory = Object.values(namedExports).find((it) => isStory<TRenderer>(it));
if (factoryStory) {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from Object.values(namedExports)[0] to Object.values(namedExports).find((it) => isStory<TRenderer>(it)) is a bug fix that ensures the code finds an actual story export rather than assuming the first export is a story. However, there appears to be no test coverage for this scenario in processCSFFile.test.ts. Consider adding a test case where the first named export is not a story (e.g., a helper function or constant) to verify this fix works correctly.

Copilot uses AI. Check for mistakes.

Object.keys(namedExports).forEach((key) => {
if (isExportStory(key, meta)) {
if (isExportStory(key, meta) && isStory<TRenderer>(namedExports[key])) {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional isStory check on line 61 ensures that only actual Story objects (created by meta.story()) are processed in factory-based CSF files, not just any export that passes the isExportStory filter. This is a defensive check that prevents processing non-Story exports as stories. Consider adding test coverage for a scenario where a factory-based CSF file has a named export that passes isExportStory but is not a Story object (e.g., a helper function with a name that looks like a story name).

Copilot uses AI. Check for mistakes.
});
// @ts-expect-error disabled not provided ❌
const Basic = meta.story({});
const Basic = meta.story();
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable Basic.

Suggested change
const Basic = meta.story();
meta.story();

Copilot uses AI. Check for mistakes.
},
});

const ExtendedInteractionsStory = meta.story({
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable ExtendedInteractionsStory.

Suggested change
const ExtendedInteractionsStory = meta.story({
meta.story({

Copilot uses AI. Check for mistakes.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 13, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Selects the first exported story that satisfies isStory as the factory, narrows exported-story processing to values passing isStory, adds parameter checks and test-child handling with __id assignment, adjusts play/mount destructuring in Story.test, and refines React preview/Meta typings including meta.input.play behavior.

Changes

Cohort / File(s) Summary
CSF processing & test-child handling
code/core/src/preview-api/modules/store/csf/processCSFFile.ts
Pick the first export satisfying isStory as the factory; normalize component annotations from that factory; only process exports whose values satisfy isStory; attach parameter checks for each story; collect and normalize story children (tests) and assign __id for test child inputs; set projectAnnotations source from factoryStory.meta.preview.composed.
Story test execution tweak
code/core/src/csf/csf-factories.ts
Generate Story.test play wrapper that destructures { mount, context } from the StoryContext parameter (adds an ESLint disable for unused mount) before invoking play/test functions.
React preview typings
code/renderers/react/src/preview.tsx
Add InferArgs alias and rework ReactPreview/ReactMeta public surface: reconstruct args via Omit<...,'args'> & { args: ... }, make primary story parameter required in overloads, add a variadic overload for optional-args scenarios, and include an internal ts-expect-error annotation.
Renderer test update
code/renderers/react/src/csf-factories.test.tsx
Add/adjust test showing meta.input.play is accessible and that a story-level play can delegate to meta.input.play.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus areas:
    • code/renderers/react/src/preview.tsx — verify conditional overloads, InferArgs correctness, and compatibility with existing public types.
    • code/core/src/preview-api/modules/store/csf/processCSFFile.ts — confirm story detection/filtering, parameter-check wiring, and __id assignment for test children.
    • code/core/src/csf/csf-factories.ts — ensure destructured { mount, context } semantics align with existing play/test expectations and test harness usage.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f817405 and 365c71d.

📒 Files selected for processing (1)
  • code/renderers/react/src/preview.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/renderers/react/src/preview.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode

Files:

  • code/renderers/react/src/preview.tsx
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/renderers/react/src/preview.tsx
code/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Export functions that need to be tested from their modules

Files:

  • code/renderers/react/src/preview.tsx
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/renderers/react/src/preview.tsx
🧠 Learnings (4)
📓 Common learnings
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.
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: 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: 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.
📚 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/renderers/react/src/preview.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/renderers/react/src/preview.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/renderers/react/src/preview.tsx
🧬 Code graph analysis (1)
code/renderers/react/src/preview.tsx (1)
code/core/src/csf/csf-factories.ts (1)
  • story (114-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/renderers/react/src/preview.tsx (4)

41-50: LGTM - internal wrapper with appropriate type suppression.

The @ts-expect-error comment now provides context for the type suppression. The TODO comment appropriately flags the open question about the Component construct.


56-60: LGTM - well-structured type inference for combined args.

The InferArgs type correctly combines component args with decorator-inferred args while using RemoveIndexSignature to avoid polluting the type with index signatures.


98-120: LGTM - required story parameter enforces compile-time arg checking.

Changing story from optional to required in these overloads correctly addresses issue #32831. When required args exist, calling meta.story() without an argument will now produce a TypeScript error, while the new third overload (lines 122-130) allows the no-argument form only when all args are already satisfied.


122-130: LGTM - clever conditional overload for zero-argument story().

The comment explains the intent well, addressing the previous reviewer's request. The [] : [never] conditional tuple pattern correctly enables meta.story() only when all required args are already provided by the meta, otherwise TypeScript will reject the call.


Comment @coderabbitai help to get the list of available commands and usage tips.

@storybook-app-bot
Copy link

storybook-app-bot bot commented Dec 13, 2025

Package Benchmarks

Commit: 02804b5, ran on 17 December 2025 at 14:04:23 UTC

No significant changes detected, all good. 👏

>;
},
{ args: Partial<TArgs> extends TMetaArgs ? {} : TMetaArgs }
Omit<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining what's going on?

/** @ts-expect-error hard */
): ReactStory<T, TInput>;

story(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here to explain what's going on?

@kasperpeulen kasperpeulen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Dec 17, 2025
@kasperpeulen kasperpeulen merged commit 0a6f8ea into next Dec 17, 2025
68 checks passed
@kasperpeulen kasperpeulen deleted the kasper/empty-story-factory branch December 17, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal patch:yes Bugfix & documentation PR that need to be picked to main branch

Projects

None yet

3 participants