Skip to content

Conversation

@kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Jan 15, 2026

Closes #33549

What I did

Previously, CSF factory files would throw a MixedFactoryError if any named export was not a factory story. This caused issues when files had helper exports or utility functions alongside stories.

Now, non-factory exports in factory files are simply skipped during indexing. This means:

  • Helper exports no longer need excludeStories to avoid errors
  • excludeStories/includeStories still work for factory stories
  • Only actual factory stories (meta.story() / meta.extend()) are indexed

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

  1. Run yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Add non-factory exports to a story file:
    // In Button.stories.ts, add at the end:
    export const mockData = { foo: 'bar' };
    export const helperFunction = () => console.log('helper');
  3. Run yarn storybook
  4. Before fix: Would fail with CSF: expected factory story error
  5. After fix: Storybook starts successfully, only factory stories are indexed

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 pull request has been released as version 0.0.0-pr-33550-sha-bd642744. Try it out in a new sandbox by running npx storybook@0.0.0-pr-33550-sha-bd642744 sandbox or in an existing project with npx storybook@0.0.0-pr-33550-sha-bd642744 upgrade.

More information
Published version 0.0.0-pr-33550-sha-bd642744
Triggered by @kasperpeulen
Repository storybookjs/storybook
Branch kasper/fix-csf-factories-exclude-stories
Commit bd642744
Datetime Thu Jan 15 13:53:17 UTC 2026 (1768485197)
Workflow run 21033721817

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=33550

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

CSF parsing now skips non-factory exports when a file contains factory stories instead of throwing MixedFactoryError. Tests updated to assert non-factory exports are ignored and that excludeStories continues to exclude specified factory exports.

Changes

Cohort / File(s) Change Summary
CsfFile implementation
code/core/src/csf-tools/CsfFile.ts
Reworked ExportNamedDeclaration handling: defer story registration until factory/non-factory validation, detect factory stories by unwrapping TS expressions, skip non-factory exports early in factory files (no exception), preserve annotation handling and __namedExportsOrder logic.
CsfFile tests
code/core/src/csf-tools/CsfFile.test.ts
Replaced MixedFactoryError expectation with assertions that non-factory exports are ignored; added test ensuring excludeStories excludes specified factory exports.

Sequence Diagram(s)

(omitted — changes are internal to CSF parsing and do not introduce a multi-component sequential flow that requires diagramming)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
code/core/src/csf-tools/CsfFile.ts (1)

586-595: Consider defensive check for empty arguments.

If meta.story() is called without arguments, storyNode.arguments[0] would be undefined. The code handles this gracefully (the t.isObjectExpression check at line 633 returns false for undefined), but an explicit guard could make the intent clearer.

Suggested defensive check
 if (
   t.isCallExpression(storyNode) &&
   t.isMemberExpression(storyNode.callee) &&
   t.isIdentifier(storyNode.callee.property) &&
   (storyNode.callee.property.name === 'story' ||
     storyNode.callee.property.name === 'extend')
 ) {
   storyIsFactory = true;
-  storyNode = storyNode.arguments[0];
+  storyNode = storyNode.arguments[0] ?? t.objectExpression([]);
 }
code/core/src/csf-tools/CsfFile.test.ts (1)

3005-3032: Consider moving success-case tests out of 'errors' block.

The "skips non-factory exports in factory files" test validates successful skip behavior rather than error handling. For consistency with the surrounding test structure, these tests might fit better in the describe('normal', ...) block at line 2545 or a new dedicated block.


📜 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 02dfa84 and eb657fd.

📒 Files selected for processing (2)
  • code/core/src/csf-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
🧰 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.mdc for consistent mocking patterns with Vitest

Files:

  • code/core/src/csf-tools/CsfFile.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 the mocked functions in Vitest tests
Implement mock behaviors in beforeEach blocks 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 without vi.mocked() in Vitest tests
Avoid mock implementations outside of beforeEach blocks in Vitest tests
Avoid mocking without the spy: true option 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 with vi.mocked() in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

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

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}

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

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
**/*.{ts,tsx}

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

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
**/*.{ts,tsx,js,jsx}

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

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
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-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

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

**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Use vi.mock() to mock file system, loggers, and other external dependencies in tests

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

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

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/csf-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
🧠 Learnings (4)
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns

Applied to files:

  • code/core/src/csf-tools/CsfFile.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Export functions from modules if they need to be tested

Applied to files:

  • code/core/src/csf-tools/CsfFile.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-tools/CsfFile.test.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-tools/CsfFile.test.ts
  • code/core/src/csf-tools/CsfFile.ts
🧬 Code graph analysis (1)
code/core/src/csf-tools/CsfFile.test.ts (2)
code/core/src/csf-tools/CsfFile.ts (1)
  • loadCsf (1074-1078)
scripts/utils/tools.ts (1)
  • dedent (119-119)
🔇 Additional comments (3)
code/core/src/csf-tools/CsfFile.ts (1)

596-616: Core fix looks correct - non-factory exports are now skipped.

The early return at lines 598-600 correctly implements the PR objective: when _metaIsFactory is true and the export is not a factory story, it skips silently instead of throwing MixedFactoryError. The error handling for the inverse case (factory story in non-factory file) is preserved.

code/core/src/csf-tools/CsfFile.test.ts (2)

3005-3018: Test correctly validates the skip behavior.

The test properly verifies that non-factory exports (B = {} and someHelper) are skipped while factory stories (A = meta.story({})) are indexed. Using Object.keys(parsed._stories) is appropriate for verifying the internal registration state.


3020-3032: Good coverage for excludeStories compatibility.

This test ensures backward compatibility: excludeStories continues to work for filtering factory stories. The assertion correctly verifies that B is excluded even though it's a valid factory story.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

Previously, CSF factory files would throw a `MixedFactoryError` if any
named export was not a factory story (e.g., `meta.story()`). This caused
issues when:
- Files had helper exports that were excluded via `excludeStories`
- Files had utility functions or constants exported alongside stories

Now, non-factory exports in factory files are simply skipped during
indexing. This means:
- Helper exports no longer need `excludeStories` to avoid errors
- `excludeStories`/`includeStories` still work for factory stories
- Only actual factory stories (`meta.story()` / `meta.extend()`) are indexed

Fixes #33549

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
@kasperpeulen kasperpeulen force-pushed the kasper/fix-csf-factories-exclude-stories branch from 02dfa84 to eb657fd Compare January 15, 2026 13:40
@kasperpeulen kasperpeulen changed the title CSF Factories: Skip non-factory exports instead of throwing error Jan 15, 2026
@nx-cloud
Copy link

nx-cloud bot commented Jan 15, 2026

View your CI Pipeline Execution ↗ for commit bd64274

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

☁️ Nx Cloud last updated this comment at 2026-01-15 13:59:11 UTC

)
).toThrowErrorMatchingInlineSnapshot(`
[MixedFactoryError: CSF: expected factory story (line 4, col 17)
export const someHelper = () => {}
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this is a CSF1 story, e.g.:

export const MyStory = () => <Component />

I suspect this means the story gets lost in the void (no longer indexed)?

Probably we can't fix this without considering includeStories/excludeStories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CSF4 this must be:
export const MyStory = meta.story(() => );

@kasperpeulen kasperpeulen merged commit 8f5c777 into next Jan 16, 2026
10 of 13 checks passed
@kasperpeulen kasperpeulen deleted the kasper/fix-csf-factories-exclude-stories branch January 16, 2026 16:41
@github-actions github-actions bot mentioned this pull request Jan 16, 2026
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment