Skip to content

Bug: Skip re-processing already transformed config files for CSF factories#34273

Merged
yannbf merged 7 commits into
nextfrom
fix/recast_windows_lf_crlf
Mar 23, 2026
Merged

Bug: Skip re-processing already transformed config files for CSF factories#34273
yannbf merged 7 commits into
nextfrom
fix/recast_windows_lf_crlf

Conversation

@huang-julien
Copy link
Copy Markdown
Contributor

@huang-julien huang-julien commented Mar 23, 2026

…orues

Fix in next release

What I did

configTOCsfFactories had no early return for already transformed code

THe failing test was for

import { defineMain } from '@storybook/react-vite/node';

export default defineMain({});

the test title state that there's nothing to change but the issue is that dedent applies \n to the code, then transform applies recast uppon the code which apply platform line break. on windows it would be \r\n . Then the test would break

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

  • check that csf factories still works

Caution

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

  • Bug Fixes
    • Better detection of already-transformed default exports so configs are preserved and redundant wrapping/merging is skipped when appropriate.
    • Ensures required framework imports are present (inserting or updating imports as needed) even when skipping other transformations, avoiding incomplete or broken configs.

@huang-julien huang-julien requested review from Copilot and yannbf March 23, 2026 11:11
@huang-julien huang-julien self-assigned this Mar 23, 2026
@huang-julien huang-julien changed the title Perf: Skip re-processing already transformed config files for CSF factories Bug: Skip re-processing already transformed config files for CSF factories Mar 23, 2026
@huang-julien huang-julien added performance issue ci:daily Run the CI jobs that normally run in the daily job. labels Mar 23, 2026
@huang-julien huang-julien added ci:normal and removed ci:daily Run the CI jobs that normally run in the daily job. labels Mar 23, 2026
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Mar 23, 2026

View your CI Pipeline Execution ↗ for commit 99d3ce0

Command Status Duration Result
nx run-many -t compile -c production --parallel=1 ✅ Succeeded 7s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-23 13:39:12 UTC

Copy link
Copy Markdown
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 optimizes the CSF factories codemod by avoiding unnecessary re-printing/re-formatting when a config file is already in the target defineMain/definePreview form, preventing platform-specific line-ending churn (notably on Windows).

Changes:

  • Adds an early-return check that detects export default defineMain(...) / export default definePreview(...) and returns the original source when no named exports need merging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts Outdated
Comment thread code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e1f82462-6eec-4a1d-b7c9-fe038e0969e9

📥 Commits

Reviewing files that changed from the base of the PR and between 13a7ea6 and 99d3ce0.

📒 Files selected for processing (2)
  • code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.ts
  • code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts

📝 Walkthrough

Walkthrough

Detects when a config file is already a CSF factory export (e.g., export default defineMain(...) / definePreview(...)) and, depending on file kind and exports, skips wrapping/merging while still ensuring the correct non-type import/specifier for the factory is present.

Changes

Cohort / File(s) Summary
Codemod helper
code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts
Adds detection of an existing export default that (optionally unwrapped) is a direct call to the CSF factory (defineMain/definePreview), computes shouldSkipTransform (different rules for main vs preview), and short-circuits wrapping/merge steps while continuing import injection/cleanup when needed.
Tests
code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.ts
Adds tests verifying injection or augmentation of the non-type import from the framework package when a file already exports defineMain(...) (cases: missing import entirely, existing import without the factory specifier).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts`:
- Around line 45-47: The early return that checks isAlreadyTransformed and
!hasNamedExports (returning info.source) can skip the import normalization/fixup
logic; change the guard in that conditional (around isAlreadyTransformed and
hasNamedExports) so it only returns early when the module's required framework
imports are already present and valid, or alternatively move/ensure the
import-normalization code (the import fixup block) runs before returning; locate
the conditional using the symbols isAlreadyTransformed, hasNamedExports, and
info.source and adjust it so files like export default defineMain({}) still go
through the import repair path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 585c4df8-5b94-4fdd-a71f-a3581c66247d

📥 Commits

Reviewing files that changed from the base of the PR and between b2fb7d8 and 8a1f355.

📒 Files selected for processing (1)
  • code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts

Comment thread code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts Outdated
@yannbf yannbf added the maintenance User-facing maintenance tasks label Mar 23, 2026
huang-julien and others added 6 commits March 23, 2026 14:08
…ry.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ry.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ry.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@yannbf yannbf merged commit da2f32e into next Mar 23, 2026
124 checks passed
@yannbf yannbf deleted the fix/recast_windows_lf_crlf branch March 23, 2026 13:55
@github-actions github-actions Bot mentioned this pull request Mar 23, 2026
17 tasks
@valentinpalkovic valentinpalkovic added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 31, 2026
@github-actions github-actions Bot mentioned this pull request Mar 31, 2026
19 tasks
valentinpalkovic pushed a commit that referenced this pull request Apr 2, 2026
Bug: Skip re-processing already transformed config files for CSF factories
(cherry picked from commit da2f32e)
@github-actions github-actions Bot mentioned this pull request Apr 2, 2026
19 tasks
@github-actions github-actions Bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch performance issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants