ModuleGraph: Stop bumping the graph revision on story-index invalidation#35178
Conversation
A story-index invalidation fired an untargeted `_bumpGraphRevision` that advanced the global graph revision and reset `latestChangedStoryFiles` to empty. That coarse bump is redundant: a story-file content edit already arrives as a builder file-change event, and a story entering/leaving the index is replayed as an add/unlink by `refreshStoryFiles` — both produce a precise `_applyGraphUpdate`. The blanket bump only races those targeted updates and can clobber the latest changed-story set, so `getLatestStoryChanges` subscribers (e.g. docgen) intermittently miss a refresh after a story save. Keep the index-invalidation -> `refreshStoryFiles` reconciliation; remove the `_bumpGraphRevision` command and the engine `onStoryIndexInvalidated` callback. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves the ChangesRemove direct revision-bump path from story-index invalidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts (1)
257-271:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuffer invalidations that arrive before
incrementalPatcheris ready.Line 264 drops
STORY_INDEX_INVALIDATEDevents when startup is still building (this.incrementalPatcheris unset). With the callback path removed, that invalidation has no deferred replay, so add/unlink reconciliation can be missed until a later invalidation.💡 Proposed fix
export class ModuleGraphEngine { + private pendingStoryIndexInvalidation = false; private readonly workingDir: string; private adapter: ChangeDetectionAdapter | undefined; @@ onStoryIndexInvalidated(): void { + if (!this.incrementalPatcher) { + this.pendingStoryIndexInvalidation = true; + return; + } + if (!this.refreshInFlight && this.incrementalPatcher) { this.refreshInFlight = true; this.refreshSettled = this.refreshStoryFiles() @@ private async startInternal(): Promise<void> { @@ this.incrementalPatcher = new IncrementalPatcher({ @@ }); + + if (this.pendingStoryIndexInvalidation) { + this.pendingStoryIndexInvalidation = false; + this.onStoryIndexInvalidated(); + }🤖 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 257 - 271, The onStoryIndexInvalidated() method currently skips all processing when this.incrementalPatcher is not yet initialized during startup, causing invalidation events to be dropped with no deferred replay mechanism. Add a buffer or queue to store invalidations that arrive before this.incrementalPatcher is ready, then process any buffered invalidations once this.incrementalPatcher becomes available. Modify the guard condition to allow early invalidations to be queued even when this.incrementalPatcher is unset, and ensure the initialization of this.incrementalPatcher also triggers processing of any buffered invalidations to prevent missed add/unlink reconciliation.
🤖 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.
Outside diff comments:
In
`@code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts`:
- Around line 257-271: The onStoryIndexInvalidated() method currently skips all
processing when this.incrementalPatcher is not yet initialized during startup,
causing invalidation events to be dropped with no deferred replay mechanism. Add
a buffer or queue to store invalidations that arrive before
this.incrementalPatcher is ready, then process any buffered invalidations once
this.incrementalPatcher becomes available. Modify the guard condition to allow
early invalidations to be queued even when this.incrementalPatcher is unset, and
ensure the initialization of this.incrementalPatcher also triggers processing of
any buffered invalidations to prevent missed add/unlink reconciliation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b624929-c0bd-415c-b640-b7ed16ec2d3a
📒 Files selected for processing (7)
code/core/src/core-server/change-detection/change-detection-service.test.tscode/core/src/core-server/change-detection/change-detection.test-helpers.tscode/core/src/shared/open-service/services/module-graph/definition.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/server.test.tscode/core/src/shared/open-service/services/module-graph/server.ts
💤 Files with no reviewable changes (3)
- code/core/src/core-server/change-detection/change-detection.test-helpers.ts
- code/core/src/shared/open-service/services/module-graph/server.ts
- code/core/src/shared/open-service/services/module-graph/definition.ts
Closes #
What I did
A story-index invalidation (any story file save, story add/remove/rename, or a
preview.*config change) fired an untargeted_bumpGraphRevisionon thecore/module-graphservice. That coarse bump advanced the global graph revision and resetlatestChangedStoryFilesto empty.That blanket bump is redundant with the precise change paths the engine already has:
_applyGraphUpdatewith the affected story files.add/unlinkbyrefreshStoryFiles→_applyGraphUpdatewith the affected story files.Because the coarse bump runs concurrently with those targeted updates, it races them and can clobber the latest changed-story set back to empty.
getLatestStoryChangessubscribers (e.g. docgen) that key off that set then intermittently miss a refresh after a story save.This PR:
_bumpGraphRevisioncommand and the engineonStoryIndexInvalidatedcallback (the coarse bump).STORY_INDEX_INVALIDATED→refreshStoryFilesreconciliation, so newly added / removed story roots are still walked and reported as targeted updates.I verified the targeting empirically in a live
react-vite/default-tssandbox (temporary engine instrumentation): editing a story file, a directly-imported component, and a deep transitive dependency each produced the correct targetedbumpedStoryFilesthrough the builder file-change path alone, whileSTORY_INDEX_INVALIDATEDonly ever fired for story-file saves.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task sandbox --template react-vite/default-ts --start-from auto(the default sandbox already setsfeatures.experimentalDocgenServerandfeatures.changeDetection).*.stories.tsxfile; it should appear in the sidebar and its docgen/snippets should populate.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 pull request has been released as version
0.0.0-pr-35178-sha-a7b9e5e7. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35178-sha-a7b9e5e7 sandboxor in an existing project withnpx storybook@0.0.0-pr-35178-sha-a7b9e5e7 upgrade.More information
0.0.0-pr-35178-sha-a7b9e5e7jeppe-cursor/module-graph-drop-coarse-bumpa7b9e5e71781682398)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=35178Made with Cursor
Summary by CodeRabbit