HMR: Fix race conditions causing stale play functions to fire on re-rendered stories#33930
Conversation
…STORY_HOT_UPDATED timing Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
valentinpalkovic
changed the title
|
View your CI Pipeline Execution ↗ for commit a49d9e1
☁️ Nx Cloud last updated this comment at |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughModifies hot module replacement (HMR) flows to emit Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
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
🤖 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/core-server/utils/index-json.test.ts`:
- Around line 663-691: Move the debounce mock setup out of the test body into
the test suite's beforeEach so mocking rules run in setup (replace the inline
vi.mocked(debounce).mockImplementation(...) currently in the test "only emits
once per file change..." with the same mock in beforeEach); also replace the
hardcoded 200ms sleep after triggering onChange(`${workingDir}/...`) with a wait
derived from the DEBOUNCE constant (e.g., use DEBOUNCE * 2 or similar) so the
assertion on mockServerChannel.emit (used by registerIndexJsonRoute and the
Watchpack onChange handler) waits an appropriate, self-documented duration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/builders/builder-vite/src/codegen-modern-iframe-script.test.tscode/builders/builder-vite/src/codegen-modern-iframe-script.tscode/builders/builder-vite/src/codegen-project-annotations.tscode/builders/builder-webpack5/templates/virtualModuleModernEntry.jscode/core/src/core-server/utils/index-json.test.tscode/core/src/core-server/utils/index-json.ts
| it('only emits once per file change (no double-fire from leading+trailing edges)', async () => { | ||
| vi.mocked(debounce).mockImplementation( | ||
| (await vi.importActual<typeof import('es-toolkit/function')>('es-toolkit/function')) | ||
| .debounce | ||
| ); | ||
|
|
||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| registerIndexJsonRoute({ | ||
| app, | ||
| channel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| storyIndexGeneratorPromise: getStoryIndexGeneratorPromise(), | ||
| }); | ||
|
|
||
| const watcher = Watchpack.mock.instances[0]; | ||
| const onChange = watcher.on.mock.calls[0][1]; | ||
|
|
||
| // Fire a single change event | ||
| onChange(`${workingDir}/src/nested/Button.stories.ts`); | ||
|
|
||
| // Wait for the trailing debounce to fire | ||
| await vi.waitFor(() => { | ||
| expect(mockServerChannel.emit).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| // Ensure it was only called once (no double-fire from both leading and trailing edges) | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
| expect(mockServerChannel.emit).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check the exact code at lines 663-691
echo "=== Code at lines 663-691 ==="
sed -n '663,691p' code/core/src/core-server/utils/index-json.test.ts
echo -e "\n=== DEBOUNCE constant definition ==="
rg -n "const DEBOUNCE" code/core/src/core-server/utils/index-json.test.tsRepository: storybookjs/storybook
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# 2. Check for other mockImplementation calls in the test file
echo "=== All mockImplementation calls ==="
rg -n "mockImplementation\(" code/core/src/core-server/utils/index-json.test.tsRepository: storybookjs/storybook
Length of output: 267
🏁 Script executed:
#!/bin/bash
# 3. Check the structure of beforeEach blocks in the file
echo "=== beforeEach blocks structure ==="
rg -n "beforeEach" code/core/src/core-server/utils/index-json.test.ts -A 5Repository: storybookjs/storybook
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Search for DEBOUNCE more broadly (could be imported)
echo "=== Search for DEBOUNCE usage ==="
rg -n "DEBOUNCE" code/core/src/core-server/utils/index-json.test.ts
echo -e "\n=== Also check the main implementation file for debounce timing ==="
rg -n "DEBOUNCE|debounce.*\d+" code/core/src/core-server/utils/index-json.ts | head -20Repository: storybookjs/storybook
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Check context at line 605 for mockImplementation
echo "=== Context around line 605 ==="
sed -n '600,620p' code/core/src/core-server/utils/index-json.test.tsRepository: storybookjs/storybook
Length of output: 790
Move mock setup to beforeEach and derive debounce wait time from DEBOUNCE constant.
Lines 664–667 set mock behavior inside the test body, violating Vitest guidelines to implement mock behaviors in beforeEach blocks. Additionally, the hardcoded 200ms sleep at line 690 is arbitrary; it should derive from the DEBOUNCE constant (100ms) to keep the test self-documenting and maintainable.
Suggested adjustment
- it('only emits once per file change (no double-fire from leading+trailing edges)', async () => {
- vi.mocked(debounce).mockImplementation(
- (await vi.importActual<typeof import('es-toolkit/function')>('es-toolkit/function'))
- .debounce
- );
+ describe('single-emission behavior', () => {
+ beforeEach(async () => {
+ vi.mocked(debounce).mockImplementation(
+ (await vi.importActual<typeof import('es-toolkit/function')>('es-toolkit/function'))
+ .debounce
+ );
+ });
+
+ it('only emits once per file change (no double-fire from leading+trailing edges)', async () => {
// ...
- await new Promise((resolve) => setTimeout(resolve, 200));
+ await new Promise((resolve) => setTimeout(resolve, DEBOUNCE * 2));
expect(mockServerChannel.emit).toHaveBeenCalledTimes(1);
+ });
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/utils/index-json.test.ts` around lines 663 - 691,
Move the debounce mock setup out of the test body into the test suite's
beforeEach so mocking rules run in setup (replace the inline
vi.mocked(debounce).mockImplementation(...) currently in the test "only emits
once per file change..." with the same mock in beforeEach); also replace the
hardcoded 200ms sleep after triggering onChange(`${workingDir}/...`) with a wait
derived from the DEBOUNCE constant (e.g., use DEBOUNCE * 2 or similar) so the
assertion on mockServerChannel.emit (used by registerIndexJsonRoute and the
Watchpack onChange handler) waits an appropriate, self-documented duration.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.19 MB | 20.42 MB | 🚨 +228 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 🎉 -84 B 🎉 |
| Dependency size | 67.37 MB | 67.60 MB | 🚨 +228 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 65.89 MB | 66.12 MB | 🚨 +228 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1.04 MB | 0 B |
| Dependency size | 36.72 MB | 36.94 MB | 🚨 +228 KB 🚨 |
| Bundle Size Analyzer | node | node |
valentinpalkovic
added
the
patch:yes
valentinpalkovic
removed
the
patch:yes
|
@tmeasday @ghengeveld I think you might want to take a look at this fix. Let us know if there's something to keep in mind with the change! |
|
|
||
| import.meta.hot.accept('${SB_VIRTUAL_FILES.VIRTUAL_STORIES_FILE}', (newModule) => { | ||
| // Cancel any running play function before patching in the new importFn | ||
| window.__STORYBOOK_PREVIEW__.channel.emit('${STORY_HOT_UPDATED}'); |
There was a problem hiding this comment.
This looks broken, since it's not a template string but a regular string.
tmeasday
left a comment
tmeasday
left a comment
There was a problem hiding this comment.
I'm not aware of the STORY_HOT_UPDATED event - it sort of seems redundant to me (after this change especially) -- shouldn't whatever code handles it and cancels the running play function just trigger whenever the index or project annotations change? I thought we already did that @ghengeveld?
I would say a few things about both the existing code and the change:
-
Why emit an event and not just call a method on the preview like the other things we do in these HMR handlers?
-
Where are the integration tests (
PreviewWeb.test.ts) for the scenarios we are trying to test for here? We had attempted to document all the various race conditions that we deal with in the preview in that file -- it sounds like there are more that weren't covered that way, given this PR changed the behaviour but didn't need to touch the tests. For something as difficult to QA for as race conditions that makes me concerned. -
I'm not opposed to the simplification that this PR does (assuming we are OK with losing the leading invalidation, slowing down all HMR by 100ms potentially?). But in general trying to avoid races by tweaking the order things are emitted doesn't seem entirely reliable. For instance what happens if the second action (
onStoriesChanged) in the block triggers before the firststoryHotUpdatedis done passing through the channel? Can we guarantee it won't? Again putting them in the one method and simplyawait-ing the first effect before doing the second seems a better approach.
yannbf
added
the
needs qa
On every file save, up to three independent re-render triggers fired simultaneously — the HMR accept callback, the leading-edge
STORY_INDEX_INVALIDATED, and the trailing-edgeSTORY_INDEX_INVALIDATED. Concurrently,STORY_HOT_UPDATEDwas emitted afteronStoriesChangedhad already started a new render, meaning it cancelled the new play function instead of the old one, causing user-event interactions to bleed into freshly-mounted components.Changes
index-json.ts— ChangeSTORY_INDEX_INVALIDATEDdebounce fromedges: ['leading', 'trailing']toedges: ['trailing']only. Eliminates the redundant immediate-fire on every save; the trailing emit (100ms after last change) aligns with when the index is fully regenerated, and theSTORY_UNCHANGEDcheck inrenderSelectionskips the re-render when nothing structurally changed.codegen-modern-iframe-script.ts(Vite) — Remove thevite:afterUpdatelistener forSTORY_HOT_UPDATED(fired for all updates, after accept callbacks) and emit it at the top of theVIRTUAL_STORIES_FILEaccept callback, beforeonStoriesChanged:codegen-project-annotations.ts(Vite) — Same fix applied to the project annotations accept callbacks.virtualModuleModernEntry.js(Webpack) — Remove theaddStatusHandler-basedSTORY_HOT_UPDATEDemit (fired after all HMR isidle) and move it beforeonStoriesChanged/onGetProjectAnnotationsChangedin each accept callback.Together these changes ensure: (1) at most one
STORY_INDEX_INVALIDATEDfires per save; (2) the old play function is always cancelled before the new render starts, not after.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores