Repository navigation
Conversation
vivshaw
marked this pull request as draft
August 8, 2026 00:11
Contributor
|
Contributor
WalkthroughThe module graph now reports reverse-index changes from incremental patching. The engine caches serialized reverse-index snapshots and reuses them when unchanged. Story index path conversion now memoizes results by working directory and input path. ChangesModule graph caching
Sequence Diagram(s)sequenceDiagram
participant FileChangeEvent
participant IncrementalPatcher
participant ModuleGraphEngine
participant SnapshotConsumer
FileChangeEvent->>ModuleGraphEngine: notify file change
ModuleGraphEngine->>IncrementalPatcher: patch(event)
IncrementalPatcher-->>ModuleGraphEngine: return indexChanged
ModuleGraphEngine->>ModuleGraphEngine: rebuild cached serialized index when needed
ModuleGraphEngine->>SnapshotConsumer: mirror update with cached index
Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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/services/module-graph/engine/dependency-graph/incremental-patcher.ts`:
- Line 119: Update the unlink handling around removeStory(path) so the return
value is true when either the story removal changes the graph or dependent
stories require re-walking, and false for an unknown file with neither change.
Do not base the result solely on storiesToWalk.length; preserve
removeStory(path)’s mutation result alongside dependent-story processing.
In
`@code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts`:
- Around line 137-144: Update the callback boundary in the module graph engine
around reverseIndexToStoriesByFile and onUpdate so reused storiesByFile
snapshots cannot be mutated by downstream core/module-graph handlers. Enforce a
readonly callback contract if supported by the callback types, or create a
defensive copy before passing storiesByFile to onSnapshot/onUpdate, while
preserving cache reuse.
In `@code/core/src/shared/open-service/services/module-graph/types.ts`:
- Around line 91-112: Update storyIndexPathCache and the toStoryIndexPath
caching flow to prevent unbounded retention of working directories and paths.
Prefer scoping the cache to the module-graph service lifecycle, or add bounded
eviction plus an explicit clear hook invoked during service teardown; ensure
cached path conversion remains unchanged while active.
🪄 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 Plus
Run ID: fa4d6a8b-e4ad-4327-9480-7b4b39978cd1
📒 Files selected for processing (3)
code/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.tscode/core/src/shared/open-service/services/module-graph/types.ts
| } | ||
| await Promise.all(storiesToWalk.map((story) => this.walkStory(story))); | ||
| return; | ||
| return true; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Return false for a no-op unlink.
When an unknown non-story file has no dependents, removeStory(path) is a no-op and storiesToWalk is empty. Line 119 still returns true, so ModuleGraphEngine.handleFileChange rebuilds the serialized reverse index unnecessarily.
Return a value that includes both the actual removeStory(path) result and dependent story re-walks. Do not use only storiesToWalk.length, because removing a story can change the index without re-walking a dependent story.
🤖 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/dependency-graph/incremental-patcher.ts`
at line 119, Update the unlink handling around removeStory(path) so the return
value is true when either the story removal changes the graph or dependent
stories require re-walking, and false for an unknown file with neither change.
Do not base the result solely on storiesToWalk.length; preserve
removeStory(path)’s mutation result alongside dependent-story processing.
Comment on lines
+137
to
+144
| if (indexChanged || this.lastStoriesByFile === undefined) { | ||
| this.lastStoriesByFile = reverseIndexToStoriesByFile( | ||
| this.reverseIndex.asMap(), | ||
| this.workingDir | ||
| ); | ||
| } | ||
| this.options.onUpdate?.({ | ||
| storiesByFile: reverseIndexToStoriesByFile(this.reverseIndex.asMap(), this.workingDir), | ||
| storiesByFile: this.lastStoriesByFile, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root=code/core/src/shared/open-service/services/module-graph
rg -n -C 6 '\b(onSnapshot|onUpdate)\b|\bstoriesByFile\b' "$root" --glob '*.ts'Repository: storybookjs/storybook
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== module graph engine =="
fd -a module-graph-engine.ts code/core/src/shared/open-service/services/module-graph/engine | sed 's#^\./##'
ast-grep outline code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts --view expanded || true
echo "== focused engine slice =="
sed -n '1,190p' code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts
echo "== focused server/callback construction =="
sed -n '1,110p' code/core/src/shared/open-service/services/module-graph/server.ts
sed -n '220,285p' code/core/src/shared/open-service/services/module-graph/definition.ts
echo "== broad callback mutation search in module-graph paths =="
rg -n 'storiesByFile(\[|\{)|\.onSnapshot|\.onUpdate|_applyGraphSnapshot|_applyGraphUpdate|runtime\.commands' code/core/src/shared/open-service/services/module-graph --glob '*.ts'Repository: storybookjs/storybook
Length of output: 24186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all storiesByFile handlers/assignments outside module-graph service =="
rg -n 'storiesByFile\s*[:=]|\.storiesByFile|getState\(\)\s*\.storiesByFile|setState\(' code/core/src --glob '*.ts' | rg -v 'node_modules|dist|build|storybook/dist|coverage' | head -n 200
echo "== state management usages for state properties =="
rg -n 'setState\(\(state\)|\.setState\(|getState\(\)' code/core/src/shared/open-service --glob '*.ts' | head -n 200
echo "== module graph query reads and subscribe payloads =="
sed -n '1,135p' code/core/src/shared/open-service/services/module-graph/definition.ts
rg -n 'subscribe\(|latestStoryChanges|graphRevision|storiesByFile' code/core/src/shared/open-service/services/module-graph --glob '*.ts' | head -n 200Repository: storybookjs/storybook
Length of output: 46347
Keep the cached graph snapshot immutable across cache reuse.
onSnapshot and onUpdate forward storiesByFile into internal core/module-graph commands, and those handlers assign the record to state. Since these callbacks may reuse the same nested record when the index is unchanged, guard this boundary with a readonly callback contract or copy the record before entering service state.
🤖 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.ts`
around lines 137 - 144, Update the callback boundary in the module graph engine
around reverseIndexToStoriesByFile and onUpdate so reused storiesByFile
snapshots cannot be mutated by downstream core/module-graph handlers. Enforce a
readonly callback contract if supported by the callback types, or create a
defensive copy before passing storiesByFile to onSnapshot/onUpdate, while
preserving cache reuse.
Source: Coding guidelines
Comment on lines
+91
to
+112
| const storyIndexPathCache = new Map<string, Map<string, string>>(); | ||
|
|
||
| /** | ||
| * Converts absolute or relative file paths into the same relative import-path format used by the | ||
| * story index (`./src/Button.stories.tsx`). This is the storage format for module-graph service | ||
| * state so static snapshots do not leak machine-specific filesystem roots. | ||
| */ | ||
| export function toStoryIndexPath(path: string, workingDir: string): string { | ||
| let cachedForWorkingDir = storyIndexPathCache.get(workingDir); | ||
| if (cachedForWorkingDir === undefined) { | ||
| cachedForWorkingDir = new Map(); | ||
| storyIndexPathCache.set(workingDir, cachedForWorkingDir); | ||
| } | ||
|
|
||
| const cached = cachedForWorkingDir.get(path); | ||
| if (cached !== undefined) { | ||
| return cached; | ||
| } | ||
|
|
||
| const result = computeStoryIndexPath(path, workingDir); | ||
| cachedForWorkingDir.set(path, result); | ||
| return result; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\btoStoryIndexPath\s*\(' \
code/core/src/shared/open-service/services/module-graph --glob '*.ts' || true
rg -n -C 4 'storyIndexPathCache|dispose|destroy|reset|clear' \
code/core/src/shared/open-service/services/module-graph --glob '*.ts' || trueRepository: storybookjs/storybook
Length of output: 30095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l code/core/src/shared/open-service/services/module-graph/types.ts \
code/core/src/shared/open-service/services/module-graph/server.ts \
code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts \
code/core/src/shared/open-service/services/module-graph/definition.ts || true
echo
echo "== module-graph/types.ts relevant cache code =="
sed -n '70,140p' code/core/src/shared/open-service/services/module-graph/types.ts | cat -n
echo
echo "== module-graph/server.ts registration/lifecycle relevant code =="
sed -n '1,130p' code/core/src/shared/open-service/services/module-graph/server.ts | cat -n
echo
echo "== module-graph-engine.ts relevant update/clear code =="
rg -n -C 5 'class ModuleGraphEngine|constructor|lastStoriesByFile|reverseIndex|storiesByFile|_applyGraph|clear|module-graph/types|toStoryIndexPath|workingDir' \
code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts \
code/core/src/shared/open-service/services/module-graph/server.ts \
code/core/src/shared/open-service/services/module-graph/definition.ts | head -n 240
echo
echo "== server.ts clearRegistry implementation =="
rg -n -A 25 -B 5 'function clearRegistry|clearRegistry' code/core/src/shared/open-service -g '*.ts' || trueRepository: storybookjs/storybook
Length of output: 50378
Bound the module-level story path cache.
storyIndexPathCache is keyed by workingDir and stores each distinct path without eviction, teardown, or cache clearance. registerModuleGraphService can register a single working directory, but long-running or multi-project processes that pass different directories let removed paths remain strongly referenced for the module lifetime. Scope the cache to the module-graph lifecycle or add bounded eviction and a clear hook.
🤖 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/types.ts` around
lines 91 - 112, Update storyIndexPathCache and the toStoryIndexPath caching flow
to prevent unbounded retention of working directories and paths. Prefer scoping
the cache to the module-graph service lifecycle, or add bounded eviction plus an
explicit clear hook invoked during service teardown; ensure cached path
conversion remains unchanged while active.
Closed
3 of 9 tasks
1 of 9 tasks
This branch has not been deployed
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
1 participant
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
#35810 contains a write-up of two potential problems I found with v10.5+. This PR addresses both of them by:
patch()returnfalseif the module graph didn't change, so other code paths can listen on that signallastStoriesByFileso it can be reused if the module graph did not changeBenchmarked impact on my machine, median across 10 runs:
nextChecklist 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!
You could follow these steps to manually test:
Repeat on
next. Confirm that on next, each costs seconds of CPU. With this PR, each is near-zero.Confirm the graph still updates when it ought to. Add a real import to
Comp0.stories.jsx, save, and check that stories depending on the changed module are still reported as affected. Remove it again and confirm it reverts.However! Rather than walk through all of these steps yourself, it might be easier to swap this patch into my repro repo.
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.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>