Skip to content

Csf-tools: Prevent false positive ObjectExpression warning on valid default exports #34993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
surendravarikallu wants to merge 1 commit into
base: next
Choose a base branch
from
+39 −4
Open

Csf-tools: Prevent false positive ObjectExpression warning on valid default exports #34993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions code/core/src/csf-tools/ConfigFile.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, it } from 'vitest';
import { describe, expect, it, vi } from 'vitest';

import { babelPrint } from 'storybook/internal/babel';
import { babelPrint, types as t } from 'storybook/internal/babel';
import { logger } from 'storybook/internal/node-logger';

Comment on lines +1 to 5

@coderabbitai coderabbitai Bot May 31, 2026

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate and print the relevant sections of the test file.
FILE="code/core/src/csf-tools/ConfigFile.test.ts"
echo "=== File: $FILE ==="
wc -l "$FILE"
rg -n "warn|spyOn\\(logger|warning" "$FILE" || true
# Show imports
sed -n '1,80p' "$FILE"
# Show around the referenced lines (~257-279)
sed -n '220,340p' "$FILE"

# Read the spy mocking rule file for exact constraints.
RULES_FILE=".cursor/rules/spy-mocking.mdc"
echo "=== Rules: $RULES_FILE ==="
ls -la "$RULES_FILE" || true
sed -n '1,200p' "$RULES_FILE" || true

Repository: storybookjs/storybook

Length of output: 7744

Align ConfigFile.test.ts logger warning suppression with Vitest spy-mocking rules.

In code/core/src/csf-tools/ConfigFile.test.ts (around lines 257-279), logger.warn is spied and mockImplementation is set inline inside the test body, which violates .cursor/rules/spy-mocking.mdc (mock implementations must be in beforeEach, and inline mock implementations in test cases should be avoided). It also doesn’t use the vi.mock(..., { spy: true }) + vi.mocked() pattern for module mocking.

Suggested update 
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
 
 import { babelPrint, types as t } from 'storybook/internal/babel';
 import { logger } from 'storybook/internal/node-logger';
 
 import { dedent } from 'ts-dedent';
 
 import { loadConfig, printConfig } from './ConfigFile.ts';
+
+vi.mock('storybook/internal/node-logger', { spy: true });
+
+const mockedWarn = vi.mocked(logger.warn);
+
+beforeEach(() => {
+  mockedWarn.mockImplementation(() => {});
+});
@@
-      it('should not log warning when default export is Identifier, CallExpression, or MemberExpression', () => {
-        const warnSpy = vi.spyOn(logger, 'warn').mockImplementation(() => {});
+      it('should not log warning when default export is Identifier, CallExpression, or MemberExpression', () => {
@@
-        expect(warnSpy).not.toHaveBeenCalled();
-        warnSpy.mockRestore();
+        expect(mockedWarn).not.toHaveBeenCalled();
       });
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/csf-tools/ConfigFile.test.ts` around lines 1 - 5, The test
currently calls logger.warn.mockImplementation inline inside a test; instead,
add a module-level vi.mock('storybook/internal/node-logger', { spy: true }) and
use vi.mocked(logger) to obtain the mocked module, then move the warn mock setup
into a beforeEach (e.g., const mockedLogger = vi.mocked(logger); beforeEach(()
=> mockedLogger.warn.mockImplementation(() => {}))); also add afterEach(() =>
vi.restoreAllMocks()) to reset spies and remove any inline mockImplementation in
individual it(...) blocks; update references to logger.warn in tests to use the
mockedLogger where needed.

import { dedent } from 'ts-dedent';

Expand Down Expand Up @@ -252,6 +253,30 @@ describe('ConfigFile', () => {
)
).toEqual('bar');
});

it('should not log warning when default export is Identifier, CallExpression, or MemberExpression', () => {
const warnSpy = vi.spyOn(logger, 'warn').mockImplementation(() => {});

// Identifier
loadConfig(dedent`
import config from './config';
export default config;
`).parse();

// CallExpression
loadConfig(dedent`
export default createConfig({ foo: 'bar' });
`).parse();

// MemberExpression
loadConfig(dedent`
const myConfig = { val: {} };
export default myConfig.val;
`).parse();

expect(warnSpy).not.toHaveBeenCalled();
warnSpy.mockRestore();
});
});

describe('factory config', () => {
Expand Down
14 changes: 12 additions & 2 deletions code/core/src/csf-tools/ConfigFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,12 @@ export class ConfigFile {

if (t.isObjectExpression(decl)) {
self._parseExportsObject(decl);
} else {
} else if (
decl &&
!t.isIdentifier(decl) &&
!t.isCallExpression(decl) &&
!t.isMemberExpression(decl)
) {
logger.warn(
getCsfParsingErrorMessage({
expectedType: 'ObjectExpression',
Expand Down Expand Up @@ -304,7 +309,12 @@ export class ConfigFile {
self._exports[exportName] = exportVal as t.Expression;
}
});
} else {
} else if (
exportObject &&
!t.isIdentifier(exportObject) &&
!t.isCallExpression(exportObject) &&
!t.isMemberExpression(exportObject)
) {
Comment on lines +312 to +317

@coderabbitai coderabbitai Bot May 31, 2026

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add regression coverage for module.exports unresolved expressions.

This guard changed behavior for module.exports too, but the new tests only assert the default-export path. Please add a counterpart test ensuring no warning for module.exports = config, module.exports = createConfig(...), and module.exports = obj.value.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/csf-tools/ConfigFile.ts` around lines 312 - 317, Tests
currently only cover default-export paths; add regression tests ensuring no
warning is emitted for module.exports when assigned an identifier, a call
expression, or a member expression. In the test suite for ConfigFile handling
(tests that exercise the exportObject guard around exportObject and checks using
t.isIdentifier/t.isCallExpression/t.isMemberExpression), add three assertions
verifying that "module.exports = config", "module.exports = createConfig(...)",
and "module.exports = obj.value" are treated the same as the default-export
cases (i.e., no warning produced). Target the code path that evaluates
exportObject in ConfigFile (the guard using exportObject and the t.is* helpers)
so the tests fail if the guard regresses for module.exports assignments.

logger.warn(
getCsfParsingErrorMessage({
expectedType: 'ObjectExpression',
Expand Down
Loading