Builder-Vite: Add onModuleGraphChange method#34323
Merged
Merged
Conversation
- Added `buildModuleGraph` function to create a module graph from Vite's module nodes. - Introduced `onModuleGraphChange` to allow listeners to react to changes in the module graph. - Created comprehensive tests for module graph behavior in `index.test.ts`. - Updated type definitions to include `ModuleGraph` and `ModuleNode` for better type safety.
|
View your CI Pipeline Execution ↗ for commit 76cbb12 ☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit cb10bac ☁️ Nx Cloud last updated this comment at |
Contributor
|
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:
📝 WalkthroughWalkthroughConstructs a ModuleGraph from Vite's moduleGraph, adds an onModuleGraphChange subscription API with a 100ms-debounced watcher handler and initial polling to emit the first graph, updates bail() to clean up watcher/interval/debounce/listeners, and adds Vitest coverage for graph building and notifications. Changes
Sequence DiagramsequenceDiagram
participant ViteServer as Vite Dev Server
participant Watcher as File Watcher
participant Debounce as Debounce Handler
participant GraphBuilder as buildModuleGraph()
participant Listeners as Registered Listeners
ViteServer->>Watcher: attach 'all' handler (change/add/unlink)
Note over Watcher,Debounce: file event(s) emitted
Watcher->>Debounce: emit event
Debounce->>Debounce: reset 100ms timer
Note over Debounce: timer expires
Debounce->>GraphBuilder: read fileToModulesMap -> build ModuleGraph
GraphBuilder->>GraphBuilder: dedupe nodes, wire importers/importedModules
GraphBuilder-->>Debounce: return ModuleGraph
Debounce->>Listeners: notifyListeners(moduleGraph)
par notify all callbacks
Listeners->>Listeners: callback A receives graph
Listeners->>Listeners: callback B receives graph
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 4
🧹 Nitpick comments (1)
code/builders/builder-vite/src/index.test.ts (1)
9-11: Usevi.mock()withspy: trueoption.Per coding guidelines, all package and file mocks should use the
spy: trueoption for consistent mocking patterns.Suggested fix
-vi.mock('./vite-server', () => ({ - createViteServer: vi.fn(), -})); +vi.mock('./vite-server', { spy: true });Then in
beforeEach:vi.mocked(createViteServer).mockResolvedValue(fakeViteServer);As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/index.test.ts` around lines 9 - 11, Update the test mock call to use the spy option and convert direct mock assignments to the typed mocked helper: change the vi.mock('./vite-server', () => ({ createViteServer: vi.fn(), })); to use vi.mock('./vite-server', { spy: true }, ... ) (keeping the same factory) and in the test setup (e.g., beforeEach) replace any direct assignments with vi.mocked(createViteServer).mockResolvedValue(fakeViteServer) so the createViteServer export is properly spied and typed for mocking.
🤖 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/builders/builder-vite/src/index.test.ts`:
- Around line 321-333: The test and implementation disagree on event names:
onModuleGraphChange registers a handler on the watcher's 'all' event but bail()
removes 'change' handlers; update the implementation so the same event name is
used everywhere. Locate the onModuleGraphChange function and the bail function
(symbols: onModuleGraphChange, bail) and change the registration/removal to the
same event (either use 'all' for both registration and off call, or register on
'change' and off('change', ...) consistently), and ensure watcher.off is invoked
with the exact handler reference that was passed to watcher.on so the listener
is actually removed.
In `@code/builders/builder-vite/src/index.ts`:
- Around line 120-124: The bail() function is removing watcherChangeHandler from
the wrong event ('change'); since the handler was registered on the 'all' event,
update the removal to use the same event name so the handler is detached
properly: in bail() locate the server?.watcher.off('change',
watcherChangeHandler) call and change it to off('all', watcherChangeHandler)
(keep the watcherChangeHandler guard and set watcherChangeHandler = undefined
afterwards) so the handler registered by the start/registration logic is
actually removed.
- Around line 100-109: Remove the temporary debug console output in this file:
delete the three console.log calls that print MODULE GRAPH and the
moduleGraph/Object.fromEntries output (the block referencing fileToModulesMap
and moduleGraph and mapping node.importedModules). If you need to keep
machine-readable diagnostics instead of removing them, replace the console.*
calls with the Storybook server logger from storybook/internal/node-logger and
log at an appropriate level (e.g., logger.debug) using the same data
transformation; otherwise simply remove the debug block before merge.
- Around line 163-169: The interval created by setInterval (assigned to
waitForModuleGraph) is never cleared when bail() is invoked, which can leak
memory and access server after close; hoist the interval reference (e.g., let
waitForModuleGraph: ReturnType<typeof setInterval> | null) to module scope,
assign it when you call setInterval in the index.ts block, and in the bail()
implementation clearInterval(waitForModuleGraph) and set it to null; also keep
the existing clearInterval(waitForModuleGraph) where the moduleGraph check
succeeds (and ensure watcherChangeHandler is only called after clearing).
---
Nitpick comments:
In `@code/builders/builder-vite/src/index.test.ts`:
- Around line 9-11: Update the test mock call to use the spy option and convert
direct mock assignments to the typed mocked helper: change the
vi.mock('./vite-server', () => ({ createViteServer: vi.fn(), })); to use
vi.mock('./vite-server', { spy: true }, ... ) (keeping the same factory) and in
the test setup (e.g., beforeEach) replace any direct assignments with
vi.mocked(createViteServer).mockResolvedValue(fakeViteServer) so the
createViteServer export is properly spied and typed for mocking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bab5d36-38c3-4a8a-bc89-6dac7091025b
📒 Files selected for processing (3)
code/builders/builder-vite/src/index.test.tscode/builders/builder-vite/src/index.tscode/core/src/types/modules/core-common.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a builder-agnostic module graph type to Storybook core types and adds an onModuleGraphChange subscription API to @storybook/builder-vite, intended to support dependency tracking updates for server-side change detection.
Changes:
- Extend the core
Builderinterface with an optionalonModuleGraphChangecallback API and introduce sharedModuleGraph/ModuleNodetypes. - Add Vite module graph conversion logic and a debounced watcher-based notifier in
@storybook/builder-vite. - Add unit tests covering graph conversion, listener registration/unsubscription, debounce behavior, and
bail()cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
code/core/src/types/modules/core-common.ts |
Adds onModuleGraphChange to the Builder interface and defines shared module graph/node types. |
code/builders/builder-vite/src/index.ts |
Implements module graph conversion + listener notification and wires it to the Vite watcher and startup timing. |
code/builders/builder-vite/src/index.test.ts |
Adds unit tests for the module graph conversion and change notification API. |
Comments suppressed due to low confidence (2)
code/builders/builder-vite/src/index.ts:169
- The PR description/issue scope calls for an eager startup indexing pass across story entrypoints to build a complete dependency map. This implementation only waits for
fileToModulesMap.size > 0and then emits whatever happens to be in the module graph afterwaitForRequestsIdle(), which may be incomplete for lazily loaded stories. Either add the explicit indexing pass over story entrypoints, or update the description/semantics so consumers don't assume the graph is complete.
toJson: () => {
throw new NoStatsForViteDevError();
},
},
totalTime: process.hrtime(startTime),
};
};
code/builders/builder-vite/src/index.ts:167
waitForModuleGraphusessetIntervalbut the interval handle isn't stored anywhere thatbail()can clear. If Storybook shuts down before the graph is populated (or during tests), this interval can continue running and call into a closing/closed Vite server. Store the interval id and clear it inbail()(and ensure a secondstart()doesn't leave multiple intervals running).
toJson: () => {
throw new NoStatsForViteDevError();
},
},
totalTime: process.hrtime(startTime),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ghengeveld
changed the title
onModuleGraphChange method to listen for graph updates
ghengeveld
changed the title
onModuleGraphChange method to listen for graph updatesonModuleGraphChange method
- Changed the event listener removal from 'change' to 'all' in the bail function to ensure proper cleanup. - Updated the test description to reflect the new behavior of removing the all-event watcher during bail. - Added assertions in tests to verify the listener count before and after bail operations.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/builders/builder-vite/src/index.ts (1)
152-158:⚠️ Potential issue | 🟠 Major
waitForModuleGraphinterval not cleared onbail().If
bail()is called beforefileToModulesMap.size > 0, the interval continues running. This can cause memory leaks and potential errors if the server is closed while the interval is still polling. This issue was flagged in a previous review but appears unaddressed.Proposed fix: Store interval at module level and clear in bail()
let debounce: ReturnType<typeof setTimeout> | undefined; let watcherChangeHandler: (() => void) | undefined; +let waitForModuleGraphInterval: ReturnType<typeof setInterval> | undefined;In
bail():export async function bail(): Promise<void> { + if (waitForModuleGraphInterval) { + clearInterval(waitForModuleGraphInterval); + waitForModuleGraphInterval = undefined; + } + if (watcherChangeHandler) { server?.watcher.off('all', watcherChangeHandler);In
start():- const waitForModuleGraph = setInterval(async () => { + waitForModuleGraphInterval = setInterval(async () => { if (server.moduleGraph.fileToModulesMap.size > 0) { - clearInterval(waitForModuleGraph); + clearInterval(waitForModuleGraphInterval); + waitForModuleGraphInterval = undefined; await server.waitForRequestsIdle(); watcherChangeHandler?.(); } }, 1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/index.ts` around lines 152 - 158, The interval created as waitForModuleGraph inside start() can keep running if bail() is called before server.moduleGraph.fileToModulesMap.size > 0; make the interval reference module-scoped (move declaration of waitForModuleGraph outside start or attach it to a higher-scope variable) so bail() can call clearInterval(waitForModuleGraph), and also clearInterval(waitForModuleGraph) immediately before/after you call clearInterval in the successful branch (inside the setInterval when size > 0) to ensure it’s always cleared; update bail() to check and clearInterval(waitForModuleGraph) and ensure any code that awaits server.waitForRequestsIdle() still runs after the interval is cleared (use the same waitForModuleGraph identifier in start(), in the setInterval callback, and in bail()).
🧹 Nitpick comments (1)
code/builders/builder-vite/src/index.test.ts (1)
9-11: Consider usingspy: trueoption for vi.mock.Per coding guidelines,
vi.mock()should use thespy: trueoption for file mocks. However, since this is a complete mock replacement that doesn't need to call through to the original, this is acceptable.Optional: Use spy option
-vi.mock('./vite-server', () => ({ - createViteServer: vi.fn(), -})); +vi.mock('./vite-server', { spy: true });Then in
beforeEach:vi.mocked(createViteServer).mockResolvedValue(fakeViteServer);As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/index.test.ts` around lines 9 - 11, The test currently replaces the entire module with vi.mock('./vite-server', () => ({ createViteServer: vi.fn() })), but per guidelines you should call vi.mock with the spy: true option so mocks can be treated as spies; update the mock call to use vi.mock('./vite-server', { spy: true }, /* optional factory if still replacing */) or simply vi.mock('./vite-server', { spy: true }) and then in setup use vi.mocked(createViteServer).mockResolvedValue(fakeViteServer) to set the resolved value for createViteServer; ensure you reference the createViteServer symbol from the vite-server module when switching to vi.mocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/builders/builder-vite/src/index.ts`:
- Around line 152-158: The interval created as waitForModuleGraph inside start()
can keep running if bail() is called before
server.moduleGraph.fileToModulesMap.size > 0; make the interval reference
module-scoped (move declaration of waitForModuleGraph outside start or attach it
to a higher-scope variable) so bail() can call
clearInterval(waitForModuleGraph), and also clearInterval(waitForModuleGraph)
immediately before/after you call clearInterval in the successful branch (inside
the setInterval when size > 0) to ensure it’s always cleared; update bail() to
check and clearInterval(waitForModuleGraph) and ensure any code that awaits
server.waitForRequestsIdle() still runs after the interval is cleared (use the
same waitForModuleGraph identifier in start(), in the setInterval callback, and
in bail()).
---
Nitpick comments:
In `@code/builders/builder-vite/src/index.test.ts`:
- Around line 9-11: The test currently replaces the entire module with
vi.mock('./vite-server', () => ({ createViteServer: vi.fn() })), but per
guidelines you should call vi.mock with the spy: true option so mocks can be
treated as spies; update the mock call to use vi.mock('./vite-server', { spy:
true }, /* optional factory if still replacing */) or simply
vi.mock('./vite-server', { spy: true }) and then in setup use
vi.mocked(createViteServer).mockResolvedValue(fakeViteServer) to set the
resolved value for createViteServer; ensure you reference the createViteServer
symbol from the vite-server module when switching to vi.mocked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c07e2497-f0d3-4bf1-b377-ce99b158fc44
📒 Files selected for processing (2)
code/builders/builder-vite/src/index.test.tscode/builders/builder-vite/src/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/builders/builder-vite/src/index.test.ts (1)
9-11: Usespy: trueoption for vi.mock per coding guidelines.The mock should use the
spy: trueoption to align with the project's spy mocking conventions.Suggested fix
-vi.mock('./vite-server', () => ({ - createViteServer: vi.fn(), -})); +vi.mock('./vite-server', { spy: true });The mock implementation in
beforeEachat line 202 already usesvi.mocked(createViteServer).mockResolvedValue(...), which will work correctly with this change.As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/index.test.ts` around lines 9 - 11, Update the module mock to use the Vitest spy option: change the existing vi.mock('./vite-server', () => ({ createViteServer: vi.fn() })) call to include the { spy: true } option so Vitest treats the mock as a spy; keep the same exported symbol name createViteServer since the tests (e.g., the beforeEach that calls vi.mocked(createViteServer).mockResolvedValue(...)) rely on that export.code/builders/builder-vite/src/index.ts (1)
149-165: Consider extracting the debounce delay as a constant.The 100ms debounce delay (line 151) and 1000ms polling interval (line 158) are reasonable values but are hardcoded. For maintainability, consider extracting these as named constants.
Suggested refactor
+const MODULE_GRAPH_CHANGE_DEBOUNCE_MS = 100; +const MODULE_GRAPH_POLL_INTERVAL_MS = 1000; + // ... inside start(): watcherChangeHandler = () => { clearTimeout(debounce); debounce = setTimeout(() => { notifyListeners(buildModuleGraph(server.moduleGraph.fileToModulesMap)); - }, 100); + }, MODULE_GRAPH_CHANGE_DEBOUNCE_MS); }; server.watcher.on('all', watcherChangeHandler); - waitForModuleGraph = setInterval(async () => { + waitForModuleGraph = setInterval(async () => { if (server.moduleGraph.fileToModulesMap.size > 0) { // ... } - }, 1000); + }, MODULE_GRAPH_POLL_INTERVAL_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/index.ts` around lines 149 - 165, Extract the hardcoded delays into named constants (e.g. DEBOUNCE_DELAY_MS and POLL_INTERVAL_MS) and use them in watcherChangeHandler's setTimeout and the setInterval for waitForModuleGraph; specifically replace the literal 100 used in setTimeout inside watcherChangeHandler (which debounces notifyListeners(buildModuleGraph(server.moduleGraph.fileToModulesMap))) with DEBOUNCE_DELAY_MS and replace the literal 1000 used in setInterval that checks server.moduleGraph.fileToModulesMap and calls server.waitForRequestsIdle()/watcherChangeHandler with POLL_INTERVAL_MS so the timing values are centralized and easier to adjust.
🤖 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/builders/builder-vite/src/index.test.ts`:
- Around line 9-11: Update the module mock to use the Vitest spy option: change
the existing vi.mock('./vite-server', () => ({ createViteServer: vi.fn() }))
call to include the { spy: true } option so Vitest treats the mock as a spy;
keep the same exported symbol name createViteServer since the tests (e.g., the
beforeEach that calls vi.mocked(createViteServer).mockResolvedValue(...)) rely
on that export.
In `@code/builders/builder-vite/src/index.ts`:
- Around line 149-165: Extract the hardcoded delays into named constants (e.g.
DEBOUNCE_DELAY_MS and POLL_INTERVAL_MS) and use them in watcherChangeHandler's
setTimeout and the setInterval for waitForModuleGraph; specifically replace the
literal 100 used in setTimeout inside watcherChangeHandler (which debounces
notifyListeners(buildModuleGraph(server.moduleGraph.fileToModulesMap))) with
DEBOUNCE_DELAY_MS and replace the literal 1000 used in setInterval that checks
server.moduleGraph.fileToModulesMap and calls
server.waitForRequestsIdle()/watcherChangeHandler with POLL_INTERVAL_MS so the
timing values are centralized and easier to adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08accd7e-aa09-46fb-a024-432dd4d77fa9
📒 Files selected for processing (2)
code/builders/builder-vite/src/index.test.tscode/builders/builder-vite/src/index.ts
Contributor
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/builders/builder-vite/src/utils/build-module-graph.ts`:
- Around line 49-57: When resolving neighbor nodes in buildModuleGraph, ensure
getOrCreateModuleNode uses fileToModulesMap to find a module when
viteModuleNode.file === null so edges from viteModuleNode.importers and
viteModuleNode.importedModules are not dropped: modify the neighbor resolution
in the loops that iterate over viteModuleNode.importers and
viteModuleNode.importedModules to fall back to lookup in fileToModulesMap (or
other map used later) when getOrCreateModuleNode(importer|importedModule)
returns undefined due to a null file, and then add that returned node to
moduleNode.importers/moduleNode.importedModules as before; also add a regression
test that creates a vite node with file: null discovered first via
importers/importedModules and later populated in fileToModulesMap to assert the
edge is preserved.
🪄 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: 2de6baba-584f-4b5b-b233-aafca3ba2e20
📒 Files selected for processing (2)
code/builders/builder-vite/src/index.tscode/builders/builder-vite/src/utils/build-module-graph.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/builders/builder-vite/src/index.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
ghengeveld
changed the title
onModuleGraphChange method
ghengeveld
changed the title
ghengeveld
force-pushed
the
module-graph-change-listener
branch
from
40f3955 to
af5da75
Compare
…file path is known
valentinpalkovic
approved these changes
Mar 30, 2026
18 tasks
8 tasks
This was referenced Apr 8, 2026
8 tasks
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
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.
Closes #34251
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
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 pull request has been released as version
0.0.0-pr-34323-sha-af5da75a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34323-sha-af5da75a sandboxor in an existing project withnpx storybook@0.0.0-pr-34323-sha-af5da75a upgrade.More information
0.0.0-pr-34323-sha-af5da75amodule-graph-change-listeneraf5da75a1774620372)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34323Summary by CodeRabbit
New Features
Tests