Skip to content

ESLint: bail out config setup if eslint-plugin-storybook is already imported#34089

Merged
valentinpalkovic merged 3 commits intonextfrom
copilot/fix-eslint-plugin-setup
Mar 11, 2026
Merged

ESLint: bail out config setup if eslint-plugin-storybook is already imported#34089
valentinpalkovic merged 3 commits intonextfrom
copilot/fix-eslint-plugin-setup

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 10, 2026

When configureFlatConfig ran against a config that already imported eslint-plugin-storybook under a custom alias, it skipped adding a duplicate import but still appended ...storybook.configs["flat/recommended"] — referencing an undefined storybook variable.

Example of broken output:

// user's existing config
import sb from 'eslint-plugin-storybook';
export default [
  ...sb.configs['flat/recommended'],
  ...storybook.configs["flat/recommended"], // ← `storybook` is undefined!
];

Changes

  • configureFlatConfig: Bails out immediately (returns code unchanged) by walking the full AST via traverse to detect if eslint-plugin-storybook is already referenced — covering both static import declarations and dynamic import() expressions (used by tools like eslint-flat-config-utils).
  • configureEslintPlugin: Skips writing the flat config file when configureFlatConfig returns unchanged code — consistent with the existing early-return behavior in the .eslintrc.json and .eslintrc.js paths.
  • Tests: Added test cases covering the "already statically imported" and "already dynamically imported" scenarios to verify no write occurs.

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Summary by CodeRabbit

  • Bug Fixes

    • Improved ESLint plugin configuration to skip modification when the Storybook plugin is already imported.
    • Added validation to prevent unnecessary file writes when no configuration changes are required.
  • Tests

    • Added test coverage for the already-imported plugin scenario in flat config.

…eady imported

Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix eslint setup to avoid duplicate storybook plugin Fix: bail out ESLint flat config setup if eslint-plugin-storybook is already imported Mar 10, 2026
@valentinpalkovic valentinpalkovic marked this pull request as ready for review March 10, 2026 09:06
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 10, 2026

Fails
🚫 PR title must be in the format of "Area: Summary", With both Area and Summary starting with a capital letter Good examples: - "Docs: Describe Canvas Doc Block" - "Svelte: Support Svelte v4" Bad examples: - "add new api docs" - "fix: Svelte 4 support" - "Vue: improve docs"
🚫 PR description is missing the mandatory "#### Manual testing" section. Please add it so that reviewers know how to manually test your changes.

Generated by 🚫 dangerJS against fc1e789

@valentinpalkovic valentinpalkovic changed the title Fix: bail out ESLint flat config setup if eslint-plugin-storybook is already imported ESLint: bail out config setup if eslint-plugin-storybook is already imported Mar 10, 2026
@valentinpalkovic valentinpalkovic moved this to In Progress in Core Team Projects Mar 10, 2026
@nx-cloud
Copy link
Copy Markdown

nx-cloud bot commented Mar 10, 2026

View your CI Pipeline Execution ↗ for commit fc1e789

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

☁️ Nx Cloud last updated this comment at 2026-03-10 13:33:34 UTC

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 10, 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: aec82a9a-085b-454f-97c2-f6550c402b1e

📥 Commits

Reviewing files that changed from the base of the PR and between 282c049 and 944b91a.

📒 Files selected for processing (2)
  • code/core/src/cli/eslintPlugin.test.ts
  • code/core/src/cli/eslintPlugin.ts

📝 Walkthrough

Walkthrough

The changes add early-return guards to the ESLint configuration setup functions to skip modification when eslint-plugin-storybook is already imported. Corresponding test coverage is added to verify this behavior prevents unnecessary file writes.

Changes

Cohort / File(s) Summary
ESLint Plugin Implementation
code/core/src/cli/eslintPlugin.ts
Adds bailout logic in configureFlatConfig to return unchanged code if eslint-plugin-storybook is already imported, and adds a guard in configureEslintPlugin to skip writing when no changes occur.
Test Coverage
code/core/src/cli/eslintPlugin.test.ts
Adds test case asserting that writeFile is not called when eslint-plugin-storybook is already present in the flat-config ESLint setup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • storybookjs/storybook#32878: Modifies the same configureEslintPlugin and configureFlatConfig functions and related flat-config test coverage.
✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)

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


// Bail out if eslint-plugin-storybook is already imported to avoid referencing
// an undefined variable or duplicating the config spread.
const alreadyHasStorybookImport = ast.program.body.some(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we instead walk through the AST and look for ImportDeclaration?
Sometimes we can have import expressions.
Quite a lot of projects uses https://github.com/antfu/eslint-flat-config-utils that accepts import(...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch! @copilot See above comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated in fc1e789 to use traverse and walk the full AST. The check now covers both static ImportDeclaration nodes and dynamic import() expressions (represented in Babel's AST as a CallExpression with an Import callee). Added a test case for the dynamic import scenario using the eslint-flat-config-utils pattern.

…nd dynamic)

Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
@valentinpalkovic valentinpalkovic merged commit 16239f5 into next Mar 11, 2026
114 of 121 checks passed
@valentinpalkovic valentinpalkovic deleted the copilot/fix-eslint-plugin-setup branch March 11, 2026 05:47
@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects Mar 11, 2026
@github-actions github-actions bot mentioned this pull request Mar 11, 2026
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants