Conversation
|
View your CI Pipeline Execution ↗ for commit 1580101
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a global spy registry system for module mocks created with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/.storybook/preview.tsx (1)
40-40: Minor formatting inconsistency.For consistency with line 38 (
{ spy: true }with spaces), consider adding spaces inside the braces.Proposed fix
-sb.mock('../core/template/stories/test/ClearModuleMocks.api.ts', {spy: true}); +sb.mock('../core/template/stories/test/ClearModuleMocks.api.ts', { spy: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/.storybook/preview.tsx` at line 40, The sb.mock call uses inconsistent brace spacing; update the mock invocation in preview.tsx (sb.mock('../core/template/stories/test/ClearModuleMocks.api.ts', {spy: true})) to match the style used on line 38 by adding spaces inside the object braces (i.e., change {spy: true} to { spy: true }) so formatting is consistent.code/core/template/stories/test/ClearModuleMocks.tsx (1)
1-2: Minor: Missing semicolons.Import statements are missing trailing semicolons, inconsistent with the rest of the codebase style.
Proposed fix
-import { useEffect, useState } from "react"; -import { Data, fetchData } from "./ClearModuleMocks.api" +import { useEffect, useState } from 'react'; +import { Data, fetchData } from './ClearModuleMocks.api';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/template/stories/test/ClearModuleMocks.tsx` around lines 1 - 2, The import lines in ClearModuleMocks.tsx (the statements importing useEffect, useState and importing Data, fetchData) are missing trailing semicolons; update those import statements to add semicolons at the end of each import line to match project style (e.g., terminate both "import { useEffect, useState } from \"react\";" and "import { Data, fetchData } from \"./ClearModuleMocks.api\";").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tasks/sandbox-parts.ts`:
- Line 858: The sandbox mock call
"sb.mock('../template-stories/core/test/ClearModuleMocks.api.ts');" is missing
the { spy: true } option and must match the mock in code/.storybook/preview.tsx
to properly test clearAllMocks() on spy mocks; update the sb.mock invocation in
scripts/tasks/sandbox-parts.ts to include the options object (e.g.,
sb.mock('../template-stories/core/test/ClearModuleMocks.api.ts', { spy: true }))
so the sandbox uses a spy mock.
---
Nitpick comments:
In `@code/.storybook/preview.tsx`:
- Line 40: The sb.mock call uses inconsistent brace spacing; update the mock
invocation in preview.tsx
(sb.mock('../core/template/stories/test/ClearModuleMocks.api.ts', {spy: true}))
to match the style used on line 38 by adding spaces inside the object braces
(i.e., change {spy: true} to { spy: true }) so formatting is consistent.
In `@code/core/template/stories/test/ClearModuleMocks.tsx`:
- Around line 1-2: The import lines in ClearModuleMocks.tsx (the statements
importing useEffect, useState and importing Data, fetchData) are missing
trailing semicolons; update those import statements to add semicolons at the end
of each import line to match project style (e.g., terminate both "import {
useEffect, useState } from \"react\";" and "import { Data, fetchData } from
\"./ClearModuleMocks.api\";").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a250b2d-69c1-4fc2-b0d3-d216332f87ad
📒 Files selected for processing (8)
code/.storybook/main.tscode/.storybook/preview.tsxcode/core/src/mocking-utils/automock.tscode/core/src/test/spy.tscode/core/template/stories/test/ClearModuleMocks.api.tscode/core/template/stories/test/ClearModuleMocks.stories.tsxcode/core/template/stories/test/ClearModuleMocks.tsxscripts/tasks/sandbox-parts.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/core/template/stories/test/ClearModuleMocks.tsx (1)
15-16: Use the API item'sidas the React key.
Dataalready has a stableid, so usingidxmakes DOM reuse depend on array order and can hide list-update bugs if the mocked payload changes later.♻️ Proposed fix
- {data.map((elm, idx) => ( - <li key={idx}>{elm.title}</li> + {data.map((elm) => ( + <li key={elm.id}>{elm.title}</li> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/template/stories/test/ClearModuleMocks.tsx` around lines 15 - 16, The list mapping in ClearModuleMocks.tsx uses the array index (key={idx}) which is unstable; change the React key to the item's stable id (use key={elm.id}) in the data.map callback so DOM reuse is based on the API item's id; ensure the mapped object property name (elm.id) exists and is unique (or fall back to a stable unique value) when updating the key in the map expression.
🤖 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/test/spy.ts`:
- Line 79: The arrow callbacks with concise bodies return the mock instance and
trigger Biome's iterable-callback-return rule; update all usages of
moduleMockSpies.forEach((spy) => spy.mockClear()) (and the similar forEach calls
around the same area) to use block-bodied arrow callbacks that call
spy.mockClear(); with no return (e.g., moduleMockSpies.forEach((spy) => {
spy.mockClear(); })), ensuring the callback returns void.
- Around line 98-104: The restoreAllMocks() implementation currently only calls
mockClear() on moduleMockSpies which preserves any
mockImplementation/mockResolvedValue between stories; change the moduleMockSpies
cleanup to call mockReset() (not mockRestore()) so each module spy's
implementation and call state are fully reset between stories, while leaving the
existing mocks.forEach((spy) => spy.mockRestore()) behavior for non-module spies
intact.
---
Nitpick comments:
In `@code/core/template/stories/test/ClearModuleMocks.tsx`:
- Around line 15-16: The list mapping in ClearModuleMocks.tsx uses the array
index (key={idx}) which is unstable; change the React key to the item's stable
id (use key={elm.id}) in the data.map callback so DOM reuse is based on the API
item's id; ensure the mapped object property name (elm.id) exists and is unique
(or fall back to a stable unique value) when updating the key in the map
expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e010e144-304d-4940-8dc7-ab03d69e36d2
📒 Files selected for processing (5)
code/.storybook/preview.tsxcode/core/src/test/spy.tscode/core/template/stories/test/ClearModuleMocks.stories.tsxcode/core/template/stories/test/ClearModuleMocks.tsxscripts/tasks/sandbox-parts.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/tasks/sandbox-parts.ts
| */ | ||
| export function clearAllMocks() { | ||
| mocks.forEach((spy) => spy.mockClear()); | ||
| moduleMockSpies.forEach((spy) => spy.mockClear()); |
There was a problem hiding this comment.
These forEach callbacks trip Biome's iterable-callback-return rule.
Biome is already flagging these new lines because the concise arrows return the mock instance expression. Use block bodies so the callbacks return void.
🧹 Proposed fix
- moduleMockSpies.forEach((spy) => spy.mockClear());
+ moduleMockSpies.forEach((spy) => {
+ spy.mockClear();
+ });
...
- moduleMockSpies.forEach((spy) => spy.mockReset());
+ moduleMockSpies.forEach((spy) => {
+ spy.mockReset();
+ });
...
- moduleMockSpies.forEach((spy) => spy.mockClear());
+ moduleMockSpies.forEach((spy) => {
+ spy.mockClear();
+ });Also applies to: 91-91, 104-104
🧰 Tools
🪛 Biome (2.4.4)
[error] 79-79: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/test/spy.ts` at line 79, The arrow callbacks with concise
bodies return the mock instance and trigger Biome's iterable-callback-return
rule; update all usages of moduleMockSpies.forEach((spy) => spy.mockClear())
(and the similar forEach calls around the same area) to use block-bodied arrow
callbacks that call spy.mockClear(); with no return (e.g.,
moduleMockSpies.forEach((spy) => { spy.mockClear(); })), ensuring the callback
returns void.
| export function restoreAllMocks() { | ||
| mocks.forEach((spy) => spy.mockRestore()); | ||
| // For module mock spies, we only clear call history (not restore), because: | ||
| // - mockRestore() would try to undo the spyOn on the module export object, which is not | ||
| // meaningful for automocked modules where the spy reference is captured at module load time | ||
| // - The spy needs to remain active for subsequent stories | ||
| moduleMockSpies.forEach((spy) => spy.mockClear()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest/@vitest/spy, how does mockRestore()behave for spies created viaspyOnorvi.mock(..., { spy: true })? Is there a supported way to restore the original implementation while preserving the same spy function reference?
💡 Result:
-
vi.spyOn(obj, 'method')spies:spy.mockRestore()restores the original property onobj(original descriptor / function) and clears spy state. After that,obj.methodis no longer the spy (the reference changes), so the oldspyobject is effectively “detached” from future calls. (vitest.dev) -
vi.mock(path, { spy: true })(“spied modules”): Vitest replaces exports with spy wrappers that still call the original implementation by default (so you can assert calls/returns without changing behavior). (vitest.dev)
If you then override an export (e.g.vi.mocked(mod.fn).mockReturnValue(...)), calling.mockRestore()on that spy follows the same rule: it restores the original implementation, which implies dropping the spy wrapper rather than keeping the same spy function identity. (Vitest’s “restore” concept is “put the original back”, not “keep spying but behave like original”.) (vitest.dev) -
Supported way to restore original implementation while preserving the same spy function reference?
No. “Restore” in Vitest means restoring the original method/export (descriptor), which necessarily breaks identity with the spy reference; Vitest explicitly notes that once mocks are restored, you can’t keep using the old spy handle to affect the restored method—you must spy again. (vitest.dev)
Practical workaround (keeps the same spy reference, but does not truly restore):
- Capture the original implementation first, then make the spy call through to it:
const original = obj.methodconst spy = vi.spyOn(obj, 'method')- later:
spy.mockImplementation(original)(optionallyspy.mockClear()to drop history)
This preserves the spy function reference and behavior, but it’s not a “restore” (the property remains a spy). (vitest.dev)
Citations:
- 1: https://vitest.dev/api/mock
- 2: https://vitest.dev/api/vi
- 3: https://vitest.dev/api/vi
- 4: https://vitest.dev/api/vi
- 5: https://vitest.dev/guide/mocking/modules.html
🏁 Script executed:
cd code/core/src && find . -name "spy.ts" -o -name "spy.js" | head -5Repository: storybookjs/storybook
Length of output: 78
🏁 Script executed:
cat -n code/core/src/test/spy.ts | head -120Repository: storybookjs/storybook
Length of output: 5438
🏁 Script executed:
cat -n code/core/src/test/spy.ts | sed -n '85,115p'Repository: storybookjs/storybook
Length of output: 1579
🏁 Script executed:
cd code && rg '__STORYBOOK_MODULE_MOCK_SPIES__' --type ts --type js -B2 -A2Repository: storybookjs/storybook
Length of output: 1189
🏁 Script executed:
cd code && rg "moduleMockSpies" --type ts --type js -B2 -A2Repository: storybookjs/storybook
Length of output: 1118
🏁 Script executed:
cd code && rg "vi\.mock\(" --type ts --type js --max-count=10 | head -20Repository: storybookjs/storybook
Length of output: 1854
🏁 Script executed:
cd code/core/src/mocking-utils && cat -n automock.ts | head -80Repository: storybookjs/storybook
Length of output: 3883
🏁 Script executed:
cd code/core/src/mocking-utils && cat -n automock.ts | sed -n '100,180p'Repository: storybookjs/storybook
Length of output: 4023
🏁 Script executed:
cd code/core/src/mocking-utils && cat -n automock.ts | sed -n '175,200p'Repository: storybookjs/storybook
Length of output: 1377
🏁 Script executed:
cd code && rg "mockImplementation\|mockResolvedValue" --type ts --type tsx -c | head -10Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
cd code && rg "mockImplementation|mockResolvedValue" --type ts -c | head -10Repository: storybookjs/storybook
Length of output: 593
🏁 Script executed:
cd code && rg "mockReset" --type ts -A3 -B1 | head -40Repository: storybookjs/storybook
Length of output: 2448
restoreAllMocks() leaks overridden module-spy implementations between stories.
Calling mockClear() on module mock spies only clears call history but preserves any mockImplementation or mockResolvedValue overrides set during a story. This causes the previous story's mock implementation to persist into the next story, breaking story isolation.
Consider using mockReset() instead for moduleMockSpies in restoreAllMocks(), since mockReset() clears state and resets implementations (whereas mockRestore() would incorrectly attempt to drop the spy wrapper entirely for automocked modules).
🧰 Tools
🪛 Biome (2.4.4)
[error] 99-99: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
[error] 104-104: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/test/spy.ts` around lines 98 - 104, The restoreAllMocks()
implementation currently only calls mockClear() on moduleMockSpies which
preserves any mockImplementation/mockResolvedValue between stories; change the
moduleMockSpies cleanup to call mockReset() (not mockRestore()) so each module
spy's implementation and call state are fully reset between stories, while
leaving the existing mocks.forEach((spy) => spy.mockRestore()) behavior for
non-module spies intact.
|
Code wise I think the PR is ok, I wish we could fix the module duplication though, rather than using this shared set between the implementations which seems brittle. Are there other solutions for the issue? Can we deduplicate @vitest/spy? |
|
@yannbf I've investigated using the globalization plug-in also for addon-vitest, but without any success. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/template/stories/test/ClearModuleMocks.stories.ts (2)
33-33: Replaceanywith proper type forcanvasparameter.Using
anybypasses TypeScript's type checking and violates strict mode. The play function context should be properly typed.♻️ Suggested fix
- play: async ({ canvas }: any) => { + play: async ({ canvas }) => {Remove the explicit
anytype annotation on both lines 33 and 44. TypeScript will infer the correct type from Storybook's CSF typing, or you can explicitly type it using Storybook'sPlayFunctionorStoryObjtypes if stricter typing is needed.As per coding guidelines: "TypeScript strict mode is enabled and must be followed".
Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/template/stories/test/ClearModuleMocks.stories.ts` at line 33, The play function parameter is currently typed as `any` (play: async ({ canvas }: any) => ...) which breaks strict TypeScript rules; remove the explicit `any` from the `canvas` parameter on both occurrences (lines where `play` is declared) so the correct Storybook CSF types are inferred, or alternatively annotate the play function with Storybook's official types (e.g., `PlayFunction`/`StoryObj`) to provide a proper type for `canvas` instead of `any`.
2-2: Avoid shadowing the nativeglobalThis.The import alias
globalThisshadows the built-in global property. Consider using a different alias name to satisfy the linter and avoid confusion.♻️ Suggested fix
-import { global as globalThis } from '@storybook/global'; +import { global as storybookGlobal } from '@storybook/global';And update the usage on line 17:
- component: globalThis.__TEMPLATE_COMPONENTS__.Button, + component: storybookGlobal.__TEMPLATE_COMPONENTS__.Button,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/template/stories/test/ClearModuleMocks.stories.ts` at line 2, The import currently aliases Storybook's global to the native name (import { global as globalThis } from '@storybook/global'), which shadows the built-in globalThis; change the alias (for example to storybookGlobal or sbGlobal) in the import and update all usages (e.g., the reference at the line that uses globalThis around the ClearModuleMocks story) to the new alias so the linter and runtime no longer confuse the identifier with the native globalThis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/template/stories/test/ClearModuleMocks.stories.ts`:
- Line 33: The play function parameter is currently typed as `any` (play: async
({ canvas }: any) => ...) which breaks strict TypeScript rules; remove the
explicit `any` from the `canvas` parameter on both occurrences (lines where
`play` is declared) so the correct Storybook CSF types are inferred, or
alternatively annotate the play function with Storybook's official types (e.g.,
`PlayFunction`/`StoryObj`) to provide a proper type for `canvas` instead of
`any`.
- Line 2: The import currently aliases Storybook's global to the native name
(import { global as globalThis } from '@storybook/global'), which shadows the
built-in globalThis; change the alias (for example to storybookGlobal or
sbGlobal) in the import and update all usages (e.g., the reference at the line
that uses globalThis around the ClearModuleMocks story) to the new alias so the
linter and runtime no longer confuse the identifier with the native globalThis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fe43c69-9fc6-458b-bf3e-f87552253abf
📒 Files selected for processing (2)
code/core/template/stories/test/ClearModuleMocks.stories.tscode/core/template/stories/test/ModuleAutoMocking.stories.ts
💤 Files with no reviewable changes (1)
- code/core/template/stories/test/ModuleAutoMocking.stories.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/template/stories/test/ClearModuleMocksMocking.stories.ts (1)
2-2: Consider renaming the import alias to avoid shadowingglobalThis.Biome flags this as shadowing a restricted global name. While this is a common pattern in Storybook templates, consider using a different alias like
storybookGlobalto avoid the linter warning and potential confusion.Proposed fix
-import { global as globalThis } from '@storybook/global'; +import { global as storybookGlobal } from '@storybook/global';And update line 17:
- component: globalThis.__TEMPLATE_COMPONENTS__.Button, + component: storybookGlobal.__TEMPLATE_COMPONENTS__.Button,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/template/stories/test/ClearModuleMocksMocking.stories.ts` at line 2, The import alias shadows the restricted global name; change the import "global as globalThis" to a non-conflicting alias such as "storybookGlobal" and update all usages of the former alias (e.g., where the code references globalThis around the story setup, such as the call/site on line 17) to use the new alias (storybookGlobal) so the linter warning is resolved and intent is clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/template/stories/test/ClearModuleMocksMocking.stories.ts`:
- Line 2: The import alias shadows the restricted global name; change the import
"global as globalThis" to a non-conflicting alias such as "storybookGlobal" and
update all usages of the former alias (e.g., where the code references
globalThis around the story setup, such as the call/site on line 17) to use the
new alias (storybookGlobal) so the linter warning is resolved and intent is
clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f30696c5-e66f-4a3c-9cbb-8e68bd3b6caa
📒 Files selected for processing (4)
code/.storybook/preview.tsxcode/core/template/stories/test/ClearModuleMocksMocking.api.tscode/core/template/stories/test/ClearModuleMocksMocking.stories.tsscripts/tasks/sandbox-parts.ts
2 participants
Closes #34075
What I did
Fixed
clearAllMocks()/resetAllMocks()/restoreAllMocks()not clearing spy mocks created bysb.mock('...', { spy: true })when called inbeforeEach.Root cause: The module mocker (
__vitest_mocker__) uses vitest's own@vitest/spyinstance to create spies, whileclearAllMocks()fromstorybook/testiterates over a different bundled@vitest/spyinstance'smocksSet. Since the two Sets are separate, storybook's mock-clearing functions never find the module mock spies.Fix: Introduced a shared global registry (
globalThis.__STORYBOOK_MODULE_MOCK_SPIES__) that bridges the gap:automock.ts— The generated automock code now registers each spy function in the global registry after module mock creation.spy.ts—clearAllMocks,resetAllMocks, andrestoreAllMocksnow also iterate over this registry to clear/reset module mock spies.This means users can now use
clearAllMocks()inbeforeEach(at project, meta, or story level) and it will properly reset spy mocks between stories — no per-story workaround needed.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn vitest run ClearModuleMocksfrom thecode/directoryFirstandSecondstories should pass — previouslySecondfailed withexpect(fetchData).toHaveBeenCalledTimes(1)receiving 2Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam 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
New Features
onMockCall()public API to register listeners for mock invocations.clearAllMocks(),resetAllMocks(), andrestoreAllMocks()now properly handle module mocks created with spy option.Tests
Chores