Skip to content

Conversation

@yannbf
Copy link
Member

@yannbf yannbf commented Oct 30, 2025

Closes #32890

What I did

This PR enhances ConfigFile (the AST helper) to include two utilities: _unwrap and _resolveDeclaration. They are used to support new use cases, such as expressions that use type syntaxes like:

export default { framework: "foo" } satisfies StorybookConfig;
export default { framework: "foo" } as StorybookConfig;
export default { framework: "foo" } as unknown as StorybookConfig;

This should make it so that when the ConfigFile wants to detect named or default exports, it will try to unwrap and find the variable initialization and therefore extract the correct node.

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

  • Tests

    • Added comprehensive tests for various TypeScript export and annotation syntaxes to ensure correct config detection and wrapping.
  • Bug Fixes

    • Improved resolution of default and named exports when TypeScript assertions/annotations are present.
  • Refactor

    • Centralized and simplified internal logic for unwrapping and resolving exported values to make parsing more robust.

@yannbf yannbf self-assigned this Oct 30, 2025
@yannbf yannbf added cli ci:docs Run the CI jobs for documentation checks only. labels Oct 30, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

📝 Walkthrough

Walkthrough

Adds recursive unwrapping and declaration-resolution helpers to ConfigFile and replaces ad-hoc unwrap logic across parsing and a codemod, plus new tests verifying various TypeScript export syntaxes (satisfies/as casts/annotations) are correctly detected and handled.

Changes

Cohort / File(s) Summary
Tests — export syntax detection
code/core/src/csf-tools/ConfigFile.test.ts, code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.ts
New/expanded tests covering default and named exports using TypeScript satisfies, as, and double-cast (as unknown as) forms to ensure parsers/codemods detect and resolve underlying objects/identifiers.
ConfigFile helpers & integration
code/core/src/csf-tools/ConfigFile.ts
Removed top-level unwrap helper; added class methods _unwrap(node) and _resolveDeclaration(node, parent) to recursively remove TS assertion/satisfies wrappers and follow Identifier initializers. Replaced inline unwrap/resolve logic across export/default handling with these methods.
Codemod: unwrap usage
code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts
Replaced direct Identifier checks with an unwrapping step using config._unwrap(...) before identifying default-exported identifiers; applied in both default-export scenarios to consistently handle wrapped/annotated identifiers.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Source as Source AST
  participant ConfigFile as ConfigFile
  participant Codemod as config-to-csf-factory

  rect rgb(240,248,255)
    Note over ConfigFile: New helpers
    ConfigFile->>ConfigFile: _unwrap(node) — strip TS assertions/satisfies/as
    ConfigFile->>ConfigFile: _resolveDeclaration(node, parent) — unwrap + follow Identifier init
  end

  Source->>ConfigFile: export / default-export AST node
  ConfigFile->>ConfigFile: _resolveDeclaration(node, parent)
  alt resolves to ObjectExpression
    ConfigFile->>ConfigFile: return concrete object node
  else resolves to Identifier -> VariableDeclarator
    ConfigFile->>ConfigFile: follow init, unwrap, return resolved node
  end

  Source->>Codemod: default-export AST node
  Codemod->>ConfigFile: config._unwrap(node)
  Codemod->>ConfigFile: get identifier name / resolved node
  Codemod->>Codemod: wrap with defineMain(...) if applicable
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

  • Areas needing extra attention:
    • _resolveDeclaration() correctness: ensure it handles identifiers that resolve outside Program scope, non-variable declarations, and deeply nested TS wrappers without infinite recursion.
    • Places where inline logic was removed: verify behavioral parity across all export-resolution code paths (named exports, default exports, exports object).
    • Test coverage: confirm tests exercise both named and default export resolution and edge cases (re-exports, imported identifiers).

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 34f6ff0 and 95e9bc6.

📒 Files selected for processing (2)
  • code/core/src/csf-tools/ConfigFile.test.ts (2 hunks)
  • code/core/src/csf-tools/ConfigFile.ts (5 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 (use yarn lint:js:cmd <file>)

Files:

  • code/core/src/csf-tools/ConfigFile.ts
  • code/core/src/csf-tools/ConfigFile.test.ts
**/*.{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/csf-tools/ConfigFile.ts
  • code/core/src/csf-tools/ConfigFile.test.ts
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/csf-tools/ConfigFile.ts
  • code/core/src/csf-tools/ConfigFile.test.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}

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

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/ConfigFile.ts
  • code/core/src/csf-tools/ConfigFile.test.ts
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/csf-tools/ConfigFile.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/csf-tools/ConfigFile.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 using vi.mock() (e.g., filesystem, loggers)

Files:

  • code/core/src/csf-tools/ConfigFile.test.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mjs} : Export functions from modules when they need to be unit-tested
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mjs} : Export functions from modules when they need to be unit-tested

Applied to files:

  • code/core/src/csf-tools/ConfigFile.ts
  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Unit tests should import and execute the functions under test rather than only asserting on syntax patterns

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:04.287Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.287Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx} : Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses

Applied to files:

  • code/core/src/csf-tools/ConfigFile.test.ts
🧬 Code graph analysis (1)
code/core/src/csf-tools/ConfigFile.test.ts (1)
code/core/src/csf-tools/ConfigFile.ts (1)
  • loadConfig (1150-1153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: daily
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/csf-tools/ConfigFile.test.ts (2)

102-112: LGTM! Good test coverage for TypeScript type syntaxes.

The test verifies that the new unwrapping logic correctly handles various TypeScript type assertion and satisfies expressions, covering common patterns used in Storybook config files.


892-908: LGTM! Comprehensive coverage for export detection.

The test verifies that the parser correctly identifies the exports object across various TypeScript export syntaxes, ensuring the unwrapping logic works in both inline and variable-based export scenarios.

code/core/src/csf-tools/ConfigFile.ts (3)

177-183: LGTM! Clean recursive unwrapping implementation.

The method correctly handles TypeScript's satisfies and as expressions by recursively unwrapping them to reach the underlying node. The implementation is concise and handles nested type assertions properly.


185-196: LGTM! Well-designed declaration resolution helper.

The method elegantly combines unwrapping TypeScript assertions with identifier resolution, handling the common pattern where exports reference typed variables. The default parent parameter and the two-stage unwrapping ensure both the reference and the declaration are properly resolved.


171-171: LGTM! Consistent refactoring across all export resolution paths.

The new _resolveDeclaration helper has been correctly integrated across all export handling logic (default exports, named exports, module.exports), replacing inline unwrapping code with a centralized, maintainable implementation.

Also applies to: 205-205, 232-232, 258-258, 286-286, 293-293, 560-562


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

@yannbf yannbf added bug ci:daily Run the CI jobs that normally run in the daily job. and removed ci:docs Run the CI jobs for documentation checks only. labels Oct 30, 2025
@nx-cloud
Copy link

nx-cloud bot commented Oct 30, 2025

View your CI Pipeline Execution ↗ for commit 95e9bc6

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 50s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-30 11:17:51 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:daily Run the CI jobs that normally run in the daily job. cli

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: CSF factories automigration doesn't modify main.ts when it uses satisfies/as type syntax

3 participants