fix: resolve #34468 storybook eslint defineConfig issue#34487
Open
justismailmemon wants to merge 5 commits intostorybookjs:nextfrom
Open
fix: resolve #34468 storybook eslint defineConfig issue#34487justismailmemon wants to merge 5 commits intostorybookjs:nextfrom
justismailmemon wants to merge 5 commits intostorybookjs:nextfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/cli/eslintPlugin.ts (1)
224-250:⚠️ Potential issue | 🟠 MajorHandle
export default configwhenconfigcomes fromtseslint.config(...).Case 3 only mutates raw arrays and
defineConfig([...])calls. The patternconst config = tseslint.config(...); export default config;is skipped, creating an inconsistency:export default tseslint.config(...)(Case 2) is rewritten, but the variable-assigned form is not.Fix
if (t.isIdentifier(eslintConfigExpression)) { const binding = path.scope.getBinding(eslintConfigExpression.name); if (binding && t.isVariableDeclarator(binding.path.node)) { const init = unwrapTSExpression(binding.path.node.init); if (t.isArrayExpression(init)) { init.elements.push(createStorybookConfigSpread()); + } else if ( + t.isCallExpression(init) && + t.isMemberExpression(init.callee) && + tsEslintLocalName && + t.isIdentifier(init.callee.object, { name: tsEslintLocalName }) && + t.isIdentifier(init.callee.property, { name: "config" }) + ) { + init.arguments.push(storybookConfig); } else if ( t.isCallExpression(init) && init.arguments.length > 0 && t.isIdentifier(init.callee) && eslintDefineConfigLocalName && init.callee.name === eslintDefineConfigLocalName ) {No test coverage exists for this pattern; add one as part of the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.ts` around lines 224 - 250, The code only handles variable-initialized arrays and defineConfig(...) calls but misses when the variable is initialized by a CallExpression like tseslint.config(...); update the block in eslintPlugin.ts that inspects binding.path.node.init inside the "if (t.isIdentifier(eslintConfigExpression))" branch to also detect when init is a CallExpression whose callee is a MemberExpression with object identifier "tseslint" and property name "config" (or the equivalent import identifier if you have one), then locate the array argument (unwrapTSExpression on the first argument) and push createStorybookConfigSpread({ castToAnyArray: shouldCastStorybookConfig }) into that array just like the existing defineConfig handling; add a unit test that covers "const config = tseslint.config([...]); export default config;" to ensure the transformation runs.
🤖 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/core/src/cli/eslintPlugin.ts`:
- Around line 255-275: The Program visitor currently always injects an ESM
import (constructed as importDecl) even for CommonJS flat config files because
isFlatConfig's pattern matches .cjs/.mjs and the rewrite only happens in the
ExportDefaultDeclaration path; fix this by introducing and using a boolean flag
(e.g., fileRewritten or rewrittenFlatConfig) that you set to true when the
ExportDefaultDeclaration rewrite succeeds, then change the Program(path) logic
to only insert the importDecl when alreadyImported is false AND that
rewrittenFlatConfig flag is true so CommonJS files that weren't rewritten won't
receive an ESM import.
---
Outside diff comments:
In `@code/core/src/cli/eslintPlugin.ts`:
- Around line 224-250: The code only handles variable-initialized arrays and
defineConfig(...) calls but misses when the variable is initialized by a
CallExpression like tseslint.config(...); update the block in eslintPlugin.ts
that inspects binding.path.node.init inside the "if
(t.isIdentifier(eslintConfigExpression))" branch to also detect when init is a
CallExpression whose callee is a MemberExpression with object identifier
"tseslint" and property name "config" (or the equivalent import identifier if
you have one), then locate the array argument (unwrapTSExpression on the first
argument) and push createStorybookConfigSpread({ castToAnyArray:
shouldCastStorybookConfig }) into that array just like the existing defineConfig
handling; add a unit test that covers "const config = tseslint.config([...]);
export default config;" to ensure the transformation runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b868c6c9-f3c1-4a1c-a596-bd25baafacd4
📒 Files selected for processing (1)
code/core/src/cli/eslintPlugin.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/cli/eslintPlugin.test.ts (1)
439-486:⚠️ Potential issue | 🟡 MinorMissing test coverage for TypeScript +
defineConfigscenario.The PR's primary fix addresses a TypeScript type incompatibility when using
defineConfigfromeslint/config. However, the existingdefineConfigtests use.mjsextensions (JavaScript), not TypeScript. There should be a test that verifies theas any[]cast is applied when:
- The config file is TypeScript (e.g.,
eslint.config.ts)- Using
defineConfigfrom"eslint/config"Consider adding a test like:
Suggested test case
it("should apply 'as any[]' cast for TypeScript defineConfig", async () => { const mockPackageManager = { getAllDependencies: vi.fn(), } satisfies Partial<JsPackageManager>; const mockConfigFile = dedent`import { defineConfig } from "eslint/config"; export default defineConfig([ { rules: { "no-console": "error" } }, ]);`; vi.mocked(readFile).mockResolvedValue(mockConfigFile); await configureEslintPlugin({ eslintConfigFile: "eslint.config.ts", // TypeScript config packageManager: mockPackageManager as any, isFlatConfig: true, }); const [, content] = vi.mocked(writeFile).mock.calls[0]; // Verify the cast is applied expect(content).toContain("...storybook.configs[\"flat/recommended\"] as any[]"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.test.ts` around lines 439 - 486, The test suite is missing coverage for the TypeScript + defineConfig case; add a new test in eslintPlugin.test.ts that calls configureEslintPlugin with eslintConfigFile set to "eslint.config.ts", mocks readFile to return a defineConfig TypeScript file (importing from "eslint/config"), stubs packageManager.getAllDependencies, sets isFlatConfig: true, and then asserts the written content includes the cast "...storybook.configs[\"flat/recommended\"] as any[]" (or equivalent) to verify the TypeScript-only fix is applied; reference configureEslintPlugin and the writeFile/readFile mocks when implementing the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/cli/eslintPlugin.test.ts`:
- Around line 439-486: The test suite is missing coverage for the TypeScript +
defineConfig case; add a new test in eslintPlugin.test.ts that calls
configureEslintPlugin with eslintConfigFile set to "eslint.config.ts", mocks
readFile to return a defineConfig TypeScript file (importing from
"eslint/config"), stubs packageManager.getAllDependencies, sets isFlatConfig:
true, and then asserts the written content includes the cast
"...storybook.configs[\"flat/recommended\"] as any[]" (or equivalent) to verify
the TypeScript-only fix is applied; reference configureEslintPlugin and the
writeFile/readFile mocks when implementing the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d61d272-a2fb-45b2-8dee-383e7648d455
📒 Files selected for processing (2)
code/core/src/cli/eslintPlugin.test.tscode/core/src/cli/eslintPlugin.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/cli/eslintPlugin.test.ts (1)
17-28:⚠️ Potential issue | 🟠 MajorAdd
spy: trueoption to both module mocks to comply with repository test guidelines.Lines 17-28 contain two
vi.mock()calls that lack the requiredspy: trueoption. Per the spy mocking rules defined in.cursor/rules/spy-mocking.mdc, all package and file mocks in Vitest tests must use{ spy: true }.Update both mocks:
- Line 17: Add
{ spy: true }to theempathic/findmock- Line 21: Add
{ spy: true }to thenode:fs/promisesmockSuggested fix
-vi.mock("empathic/find", () => ({ - up: vi.fn(), -})); +vi.mock("empathic/find", { spy: true }); -vi.mock(import("node:fs/promises"), async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - readFile: vi.fn(), - writeFile: vi.fn(), - }; -}); +vi.mock("node:fs/promises", { spy: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.test.ts` around lines 17 - 28, Both vi.mock calls need the repository-required spy option; update the mocks for "empathic/find" (the vi.mock("empathic/find", () => ({ up: vi.fn() })) call) and for the dynamic import mock (vi.mock(import("node:fs/promises"), async (importOriginal) => { ... })) to pass { spy: true } as the third argument to vi.mock (i.e., vi.mock(module, factory, { spy: true })) so the mocks comply with the spy-mocking rule.
🧹 Nitpick comments (2)
code/core/src/cli/eslintPlugin.test.ts (2)
201-237: Remove duplicated inline snapshot assertion in the same test.The comment-json test asserts the same inline snapshot twice. Keeping one snapshot + targeted
toContainchecks is enough and reduces churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.test.ts` around lines 201 - 237, The test contains a duplicated inline snapshot assertion for the same `content` value; remove the redundant `expect(content).toMatchInlineSnapshot(...)` (the second/duplicate occurrence) and keep the single snapshot plus the targeted `expect(content).toContain(...)` checks so the test asserts the snapshot once while still verifying specific comment and entry presence; look for the two identical `expect(content).toMatchInlineSnapshot` calls and delete the later one.
508-509: Strengthen the TS cast assertion to validate placement, not just presence.
toContain("as any[]")can pass even if the cast appears in an unrelated part of the output. Prefer asserting the spread expression is specifically casted.Example assertion
- expect(content).toContain('storybook.configs["flat/recommended"]'); - expect(content).toContain("as any[]"); + expect(content).toMatch( + /\.\.\.storybook\.configs\["flat\/recommended"\]\s+as any\[\]/, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.test.ts` around lines 508 - 509, Replace the loose existence check for "as any[]" with a targeted assertion that the spread expression for storybook.configs["flat/recommended"] is the thing being cast; update the test (where content is asserted) to use a regex match such as matching storybook.configs["flat/recommended"] followed by optional whitespace and then "as any[]" (e.g. expect(content).toMatch(/storybook\.configs\["flat\/recommended"\]\s+as any\[\]/)) so the test validates placement, not just presence.
🤖 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/core/src/cli/eslintPlugin.test.ts`:
- Around line 118-123: The tests in describe("configureEslintPlugin") rely on
precise readFile/writeFile call counts but currently define mock implementations
outside test setup, causing cross-test bleed; move any mock implementations for
readFile, writeFile and the mockPackageManager.getAllDependencies into a
beforeEach inside this describe block, and call vi.resetAllMocks() or
vi.clearAllMocks() at the start of that beforeEach so each test (including
"should not configure ESLint plugin if it is already configured") starts with
fresh mocks and predictable mock.calls[0] behavior.
---
Outside diff comments:
In `@code/core/src/cli/eslintPlugin.test.ts`:
- Around line 17-28: Both vi.mock calls need the repository-required spy option;
update the mocks for "empathic/find" (the vi.mock("empathic/find", () => ({ up:
vi.fn() })) call) and for the dynamic import mock
(vi.mock(import("node:fs/promises"), async (importOriginal) => { ... })) to pass
{ spy: true } as the third argument to vi.mock (i.e., vi.mock(module, factory, {
spy: true })) so the mocks comply with the spy-mocking rule.
---
Nitpick comments:
In `@code/core/src/cli/eslintPlugin.test.ts`:
- Around line 201-237: The test contains a duplicated inline snapshot assertion
for the same `content` value; remove the redundant
`expect(content).toMatchInlineSnapshot(...)` (the second/duplicate occurrence)
and keep the single snapshot plus the targeted `expect(content).toContain(...)`
checks so the test asserts the snapshot once while still verifying specific
comment and entry presence; look for the two identical
`expect(content).toMatchInlineSnapshot` calls and delete the later one.
- Around line 508-509: Replace the loose existence check for "as any[]" with a
targeted assertion that the spread expression for
storybook.configs["flat/recommended"] is the thing being cast; update the test
(where content is asserted) to use a regex match such as matching
storybook.configs["flat/recommended"] followed by optional whitespace and then
"as any[]" (e.g.
expect(content).toMatch(/storybook\.configs\["flat\/recommended"\]\s+as
any\[\]/)) so the test validates placement, not just presence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4fe01d7-a588-4911-ba6d-00b031ecc212
📒 Files selected for processing (1)
code/core/src/cli/eslintPlugin.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #34468
This resolves the
defineConfigtype issue when addingstorybook.configs["flat/recommended"].What changed
eslintConfigFileinto flat config transformation so TS and JS can be handled correctlyWhy
The original fix solves the TypeScript
defineConfigerror, but usingas any[]everywhere would break JS config files. This change fixes the TS error without introducing that JS regression.Result
defineConfigtype error in TS configs.js,.mjs, and.cjsESLint config filesSummary by CodeRabbit
Bug Fixes
Refactor
Tests