Repository navigation
Core: Stop recomputing and broadcasting the whole module graph on every file change - #35814
Closed
valentinpalkovic wants to merge 1 commit into
Closed
valentinpalkovic wants to merge 1 commit into
valentinpalkovic wants to merge 1 commit into
Conversation
…ry file change The module graph engine re-serialized its entire reverse index on every file-watcher event and pushed the result through schema validation, a deep clone and a websocket broadcast - even when the patch had changed nothing, and even for files the graph has never seen. Two changes: - IncrementalPatcher.patch() now reports whether the reverse index actually moved. The engine skips the mirror entirely when nothing moved and no story is affected, and omits storiesByFile when only revisions need bumping. - A service definition can declare localStateKeys. Those keys are stripped from every snapshot before it is cloned for the channel and are preserved when a peer snapshot arrives. core/module-graph declares storiesByFile, which is server-only and the largest thing in its state. On a 400-story / 1200-module project, a story save drops from 1.10s to 0.06s of dev-server CPU and stops pushing 2 x 17.36 MB over the websocket. A write to an unrelated file drops from 0.84s to 0.01s.
Contributor
Contributor
WalkthroughThe change adds ChangesOpen Service local state
Module graph synchronization
Sequence Diagram(s)sequenceDiagram
participant FileWatcher
participant IncrementalPatcher
participant ModuleGraphEngine
participant OpenServiceChannel
FileWatcher->>IncrementalPatcher: patch file change
IncrementalPatcher-->>ModuleGraphEngine: report indexChanged
ModuleGraphEngine->>ModuleGraphEngine: calculate affected stories
ModuleGraphEngine->>OpenServiceChannel: emit revision and optional storiesByFile
OpenServiceChannel->>OpenServiceChannel: preserve local storiesByFile
Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
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
🧹 Nitpick comments (1)
code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.test.ts (1)
177-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
patchSpybehavior intobeforeEach.Line 184 and Line 205 configure mock behavior inside test cases. Put these scenarios in nested
describeblocks. Configurevi.mocked(patchSpy).mockResolvedValue(false)in each nestedbeforeEach.As per coding guidelines, “Implement mock behaviors in
beforeEachblocks in Vitest tests” and “Usevi.mocked()to type and access the mocked functions in Vitest tests.”🤖 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/shared/open-service/services/module-graph/engine/module-graph-engine.test.ts` around lines 177 - 219, Move the two patchSpy=false scenarios into separate nested describe blocks and remove their inline mockResolvedValue calls. Add a beforeEach in each nested block that applies vi.mocked(patchSpy).mockResolvedValue(false), while keeping each test’s existing setup and assertions unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@code/core/src/shared/open-service/service-runtime.ts`:
- Around line 109-114: Update the getStateSnapshot overload in the service
runtime interface so calls without arguments still return TState, while calls
supplying omitKeys return Partial<TState>. Preserve the existing snapshot
behavior and use overloads to distinguish the two call signatures.
---
Nitpick comments:
In
`@code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.test.ts`:
- Around line 177-219: Move the two patchSpy=false scenarios into separate
nested describe blocks and remove their inline mockResolvedValue calls. Add a
beforeEach in each nested block that applies
vi.mocked(patchSpy).mockResolvedValue(false), while keeping each test’s existing
setup and assertions unchanged.
🪄 Autofix
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: 77a6b38f-1dea-44d3-9a7a-c4193c8e22af
📒 Files selected for processing (14)
code/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/service-definition.tscode/core/src/shared/open-service/service-registry.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/service-sync.test.tscode/core/src/shared/open-service/service-sync.tscode/core/src/shared/open-service/services/module-graph/definition.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patch.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patcher.tscode/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.test.tscode/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.tscode/core/src/shared/open-service/services/module-graph/module-graph.test-helpers.tscode/core/src/shared/open-service/services/module-graph/server.test.tscode/core/src/shared/open-service/types.ts
Comment on lines
+109
to
+114
| /** | ||
| * Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves | ||
| * the named top-level keys out without cloning them, which is how a definition's | ||
| * `localStateKeys` stay off the channel. | ||
| */ | ||
| getStateSnapshot(omitKeys?: readonly string[]): TState; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\.getStateSnapshot\s*\(' code/core/srcRepository: storybookjs/storybook
Length of output: 2066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- service-runtime declarations and implementation ---'
sed -n '80,135p' code/core/src/shared/open-service/service-runtime.ts
sed -n '280,335p' code/core/src/shared/open-service/service-runtime.ts
printf '%s\n' '--- getStateSnapshot call sites with context ---'
rg -n -C 8 '\.getStateSnapshot\s*\(' code/core/src/shared/open-service
printf '%s\n' '--- related state and localStateKeys declarations ---'
rg -n -C 5 'localStateKeys|ServiceRuntime|TState' code/core/src/shared/open-service/service-runtime.ts code/core/src/shared/open-service/service-registry.ts code/core/src/shared/open-service/*.tsRepository: storybookjs/storybook
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ServiceDefinition and localStateKeys types ---'
rg -n -C 8 'localStateKeys' code/core/src/shared/open-service/types.ts code/core/src/shared/open-service/service-definition.ts code/core/src/shared/open-service
printf '%s\n' '--- exact snapshot implementation and call-site declarations ---'
sed -n '300,325p' code/core/src/shared/open-service/service-runtime.ts
sed -n '255,276p' code/core/src/shared/open-service/service-registry.ts
sed -n '100,112p' code/core/src/shared/open-service/server.ts
printf '%s\n' '--- all non-test snapshot calls ---'
rg -n '\.getStateSnapshot\s*\(' code/core/src/shared/open-service -g '*.ts' -g '!*.test.ts'Repository: storybookjs/storybook
Length of output: 30852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository-wide snapshot API usage ---'
rg -n -C 2 '\b(getStateSnapshot|ServiceRuntime)\b' . -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' -g '!node_modules'
printf '%s\n' '--- runtime behavior for omitted keys ---'
node - <<'JS'
const rawState = {
requiredValue: 1,
localValue: { large: true },
};
const getStateSnapshot = (omitKeys) => {
if (!omitKeys?.length) {
return structuredClone(rawState);
}
const kept = {};
for (const [key, value] of Object.entries(rawState)) {
if (!omitKeys.includes(key)) kept[key] = value;
}
return structuredClone(kept);
};
const full = getStateSnapshot();
const omitted = getStateSnapshot(['localValue']);
if (full.requiredValue !== 1 || full.localValue?.large !== true) {
throw new Error('No-argument snapshot did not preserve the full state');
}
if ('localValue' in omitted || omitted.requiredValue !== 1) {
throw new Error('Omitted-key snapshot did not remove only the requested key');
}
console.log(JSON.stringify({ full, omitted }));
JS
printf '%s\n' '--- compiler availability for an isolated overload assignability check ---'
if command -v tsc >/dev/null 2>&1; then
tsc --version
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/check.ts" <<'TS'
type State = { requiredValue: number; localValue: object };
type Runtime = {
getStateSnapshot(): State;
getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<State>;
};
const implementation = (omitKeys?: readonly string[]): State => ({
requiredValue: 1,
localValue: {},
});
const runtime: Runtime = { getStateSnapshot: implementation };
const full: State = runtime.getStateSnapshot();
const partial: Partial<State> = runtime.getStateSnapshot(['localValue']);
void [full, partial];
TS
tsc --strict --noEmit --skipLibCheck "$tmpdir/check.ts"
else
echo 'tsc unavailable'
fiRepository: storybookjs/storybook
Length of output: 5698
Correct the getStateSnapshot return type.
A non-empty omitKeys list can remove a required top-level key. Keep the no-argument overload as TState, and return Partial<TState> for calls that provide the optional omission list.
Proposed type fix
- getStateSnapshot(omitKeys?: readonly string[]): TState;
+ getStateSnapshot(): TState;
+ getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<TState>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves | |
| * the named top-level keys out without cloning them, which is how a definition's | |
| * `localStateKeys` stay off the channel. | |
| */ | |
| getStateSnapshot(omitKeys?: readonly string[]): TState; | |
| /** | |
| * Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves | |
| * the named top-level keys out without cloning them, which is how a definition's | |
| * `localStateKeys` stay off the channel. | |
| */ | |
| getStateSnapshot(): TState; | |
| getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<TState>; |
🤖 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/shared/open-service/service-runtime.ts` around lines 109 - 114,
Update the getStateSnapshot overload in the service runtime interface so calls
without arguments still return TState, while calls supplying omitKeys return
Partial<TState>. Preserve the existing snapshot behavior and use overloads to
distinguish the two call signatures.
valentinpalkovic
added
the
ci:normal
valentinpalkovic
marked this pull request as draft
August 8, 2026 18:00
1 of 9 tasks
Member
|
Closing in favor of #35825, which takes the engine half of this PR (patch change signal + skip/omit reverse-index mirror when nothing changed). We are intentionally not landing Huge thanks for the diagnosis, measurements, and guide PR — the analysis in #35810 / this draft is what made the engine fix straightforward. Credited via |
Member
|
Superseded by #35825 (engine-only fix; no OSA localStateKeys). |
1 of 9 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
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.
Closes #35810
What I did
Since 10.5, every file the watcher reports causes the module graph engine to re-serialize its entire reverse index, run it through schema validation, deep-clone it, and telejson it onto the websocket. None of that depends on what actually changed. A comment-only edit pays it. Appending a line to a
.logfile the graph has never seen pays it too.This PR makes the work proportional to the change: the patcher now says whether the reverse index actually moved, and the index itself no longer travels over the channel at all.
Thanks a lot to @vivshaw for the write-up in #35810 and the draft in #35809 - the analysis there was spot on and is what this PR builds on. 👏
The pipeline today
patch()already knows when it did nothing - it returns early for a comment-only edit and for a file outside the graph. It just threw that knowledge away, and nothing downstream could recover it.After
The two changes
patch()returns whether the reverse index movedengine/dependency-graph/incremental-patcher.tsstoriesByFileengine/module-graph-engine.ts_applyGraphUpdate.storiesByFilebecomes optionalservices/module-graph/definition.tslocalStateKeyson a service definitiontypes.ts,service-registry.ts,service-runtime.ts,service-sync.tsThe decision that matters is one guard:
Side findings
Three things turned up while measuring that the issue does not mention. The first is the big one.
1. The reverse index was being broadcast to browsers that cannot use it.
core/module-graphlives inserverCoreServiceDefsonly. No manager or preview runtime registers it - I checked everyregisterServicecall site and everygetService('core/module-graph')consumer, and all five are node-side. ButwrapCommandsForBroadcastemits the full post-mutation snapshot for every command regardless, so the whole index crossed the wire on every save. The browsers telejson-parsed it and dropped it on aserviceIdmismatch.Captured on
next, instrumentingServerChannelTransport.send:17.36 MB per send on a 400-story repro. The reporter's graph is ~3.2M pairs, roughly 6.7x - so on the order of 116 MB per file save, twice. That is very likely the "disconnects" they described, not just slowness.
Hence
localStateKeys. A definition can now name top-level state keys that stay in the process that owns them:Listed keys are dropped from the snapshot before it is cloned, and
applyStatePatchneither deletes nor overwrites them when a peer snapshot arrives. Revisions and status keep syncing normally.2. Two broadcasts per save, not one. Every command is broadcast-wrapped, including
_waitForSettledEngine- which is a read barrier called from thestatusquery'sload. So waiting for the engine to settle also shipped the full state. Fixed by the same change, but worth knowing: in this system a read can cost a full-state broadcast.3.
ServerChannelTransport.sendserializes before checking for clients. Note the0 client(s)above: 66-86 ms of telejson with nothing connected. I deliberately left this alone - it is a separate concern in the channel layer, and withlocalStateKeysin place the payload is small enough that it stops mattering here. Happy to do it as a follow-up if you think it is worth it, though.Performance
Same machine, same repro, same build (
10.6.0-alpha.4) with and without the patch, so this is not a version artifact.rebuildscounts fullreverseIndexToStoriesByFilecalls; CPU is the dev-server process delta fromps -o time=.Before:
After:
nextThis scales with graph size, so the numbers should be quite a bit more dramatic on the reporter's ~2.1K story files / ~3.2M edges.
Where the old ~1.0 s went, measured separately at repro scale:
reverseIndexToStoriesByFilestructuredCloneof the whole stateCorrectness
The obvious worry with a change like this is that it goes fast by skipping work it should have done. I verified the graph still tracks edges by adding and removing a real import against a running dev server, with
_applyGraphUpdateinstrumented:Step 3 is the one that matters: the new edge was recorded, so editing that module correctly reports the importing story as affected - while still not re-sending an index that did not change. Step 5 confirms the edge is pruned again.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
New coverage:
patch()'s return contract for each no-op path (incremental-patch.test.ts), the engine's skip and omit behaviour (module-graph-engine.test.ts),_applyGraphUpdatewithoutstoriesByFileplus an assertion that the index never reaches the channel (server.test.ts), andlocalStateKeyshandling inapplyStatePatch(service-sync.test.ts).Manual testing
Generate a sandbox:
yarn task sandbox --template react-vite/default-ts --start-from auto cd ../storybook-sandboxes/react-vite-default-tsGrow the module graph so the cost is observable (400 stories x 1200 modules):
Start Storybook and wait until it goes quiet:
Note the dev-server process's cumulative CPU with
ps -o time= -p <pid>, then trigger each of these and re-check after each one:Both should be near-zero. On
nexteach costs roughly a second of CPU.Confirm the graph still updates when it should. Add a real import to
Comp0.stories.jsx(e.g. a newsrc/newdep.js), save, and check that editingsrc/newdep.jsafterwards still marks the story as modified in the sidebar. Remove the import and confirm editingsrc/newdep.jsno longer affects it.Alternatively - and this is a lot less setup - @vivshaw's reproduction repo already contains all of the above plus the instrumentation, so swapping this branch in there is the fastest path.
Documentation
MIGRATION.MD
code/core/src/shared/open-service/README.mdgains alocalStateKeyssection covering when to reach for it and whycore/module-graphis the motivating case.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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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>