Skip to content

Module Graph: Refactor from StoryDependencyService to open service-based ModuleGraphService#35048

Open
JReinhold wants to merge 11 commits into
nextfrom
jeppe-cursor/4b72cdff
Open

Module Graph: Refactor from StoryDependencyService to open service-based ModuleGraphService#35048
JReinhold wants to merge 11 commits into
nextfrom
jeppe-cursor/4b72cdff

Conversation

@JReinhold
Copy link
Copy Markdown
Contributor

@JReinhold JReinhold commented Jun 3, 2026

Closes #

What I did

This PR moves Storybook's story module graph out of change detection and into a new core/module-graph open service.

Concretely, it:

  • Moves the module graph engine plus its parser registry, resolver, dependency graph, and adapter types into shared/open-service/services/module-graph.
  • Registers core/module-graph as the single service boundary for module graph consumers.
  • Stores JSON-serializable module graph state using story-index-style relative paths such as ./src/Button.stories.tsx, so future static snapshots do not leak absolute filesystem paths.
  • Exposes open-service queries for:
    • getStatus — lifecycle state (booting, ready, error, unavailable) with serializable ErrorLike errors.
    • getStoriesForFiles — maps source files to affected story files and graph distance.
    • getGraphRevision — monotonic graph/index revision for consumers that need to rescan derived state.
    • getLatestStoryChanges — latest changed story files plus revision, used by DocGen reactivity.
  • Removes the legacy StoryDependencyGraphService/active-registry compatibility path; current consumers must use the open service.
  • Makes ChangeDetectionService consume module graph data through open-service queries/subscriptions instead of engine callbacks.
  • Adds DocGen reactivity by subscribing to getLatestStoryChanges and proactively refreshing already-extracted docgen for affected component ids.
  • Preserves the base branch's newer DocGen entry-selection/provider behavior while layering module graph reactivity on top.
Follow-up prompt: migrate addon-mcp to core/module-graph
You are working in the storybookjs/mcp repository. Update addon-mcp to stop using the removed Storybook dependency graph compatibility API and migrate it to Storybook's new `core/module-graph` open service API.

Context:
- Storybook PR storybookjs/storybook#35048 removes `StoryDependencyGraphService`, `experimental_getDependencyGraphService`, and the active dependency graph registry.
- The replacement is the `core/module-graph` open service.
- The old addon-mcp usage is in `packages/addon-mcp/src/utils/resolve-component-stories.ts`.
- Today that file imports `getDependencyGraphService` from `./change-detection.ts`, awaits it, checks `service.hasGraph()`, and then calls `service.lookup(target)` for each component/barrel target.

New Storybook module graph API to use:
- Resolve the open service runtime for service id `core/module-graph` instead of using `getDependencyGraphService`.
- Use `getStatus` to determine availability/readiness:
  - `{ value: 'booting' }`: graph is still building; return an unavailable response with a helpful "hasn't built yet" reason.
  - `{ value: 'ready' }`: graph is ready; continue lookup.
  - `{ value: 'unavailable', reason, error? }`: return unavailable with the provided reason.
  - `{ value: 'error', error }`: return unavailable/failure with the serialized error message.
- Use `getStoriesForFiles.loaded({ files })` instead of `lookup(target)`.
  - Input accepts absolute paths, `./`-prefixed relative paths, or relative paths without `./`, with POSIX or Windows separators.
  - Output is positional: result `i` corresponds to input file `i`.
  - Output story files are story-index-style relative paths such as `./src/Button.stories.tsx`, not absolute paths.
  - Each hit has `{ storyFile, depth }`, where `depth` is the breadth-first-search import distance.

Migration requirements:
1. Remove the dependency on the old `getDependencyGraphService` helper and any old `StoryDependencyGraphService` typing.
2. Add a small helper for accessing the open service in the addon environment. Prefer whatever addon-mcp already uses for Storybook open services; if no helper exists, create one that mirrors Storybook's open-service access pattern for `core/module-graph`.
3. In `resolveComponentStories`:
   - Get the module graph service.
   - Await `moduleGraph.queries.getStatus.loaded(undefined)` before resolving component stories.
   - If status is not `ready`, return `{ available: false, reason: ... }` with behavior equivalent to the old unavailable/hasn't-built branches.
   - Keep the existing path normalization, canonicalisation, and barrel expansion behavior (`expandBarrelTargets`, `canonicalise`) so caller ergonomics do not regress.
   - Dedupe all expanded target paths per request, call `getStoriesForFiles.loaded({ files: targets })` once for the batch if practical, and merge hits back per original component path.
   - Since returned `storyFile` values are now relative story-index paths, map story ids from the story index using `entry.importPath` directly (or normalize to the same `./...` format) instead of absolute story-file keys.
   - Preserve minimum-depth merging across barrel-expanded targets and stable sorting by `depth` then `storyId`.
4. Update tests for `resolve-component-stories.ts`:
   - ready status + direct source file hit
   - booting status returns unavailable
   - unavailable/error status returns helpful unavailable reason
   - returned relative story paths map to story ids correctly
   - barrel target expansion still merges minimum depth
   - path-not-found behavior is unchanged
5. Run the addon-mcp test/lint/typecheck commands used in that repo and fix any failures.

Keep the migration focused. Do not reintroduce Storybook's removed dependency graph compatibility API.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Focused validation run locally after resolving conflicts with claude/docgen-phase-3-rcm-provider:

  • npx vitest run --config code/core/vitest.config.ts code/core/src/shared/open-service/services/module-graph code/core/src/shared/open-service/services/docgen/server.test.ts code/core/src/core-server/change-detection/change-detection-service.test.ts (12 files, 150 tests)
  • FORCE_COLOR=0 yarn exec jiti ./scripts/check/check-package.ts --cwd code/core
  • yarn --cwd code lint:js:cmd core/src/shared/open-service/services/docgen/server.ts core/src/shared/open-service/services/docgen/server.test.ts core/src/shared/open-service/services/module-graph/definition.ts core/src/shared/open-service/services/module-graph/server.ts core/src/shared/open-service/services/module-graph/server.test.ts core/src/shared/open-service/services/module-graph/types.ts core/src/core-server/change-detection/change-detection-service.ts core/src/core-server/change-detection/change-detection.test-helpers.ts core/src/core-server/dev-server.ts --fix

Manual testing

Please QA the change detection path in a Storybook project using a builder that supports change detection, such as a React Vite Storybook.

  1. Start from a clean git working tree in a Storybook project with at least one component story, for example a Button.tsx component and Button.stories.tsx story.

  2. Enable the change detection feature flag in .storybook/main.ts:

    export default {
      features: {
        changeDetection: true,
      },
    };

    If the project already has a features object, add changeDetection: true to it.

  3. Start Storybook in dev mode.

  4. Open the manager UI and confirm the initial sidebar has no unexpected change-detection statuses for the clean working tree.

  5. Edit the component file imported by the story, for example Button.tsx, without committing the change.

  6. Confirm the related story receives the expected changed/modified status in the Storybook sidebar.

  7. Edit a transitive dependency of that story, for example a helper/module imported by Button.tsx, and confirm the dependent story receives the expected affected status.

  8. Add or rename a story file, then trigger story index regeneration by saving the file or requesting index.json; confirm the story list and change-detection status update without restarting Storybook.

  9. Revert the edited files back to the clean git state and confirm the change-detection statuses clear after Storybook processes the file changes.

  10. While Storybook is still running, make one more source-file edit and confirm the status updates again, verifying the module graph continues to process incremental changes after the initial build.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • Make 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-35048-sha-7c1a3349. Try it out in a new sandbox by running npx storybook@0.0.0-pr-35048-sha-7c1a3349 sandbox or in an existing project with npx storybook@0.0.0-pr-35048-sha-7c1a3349 upgrade.

More information
Published version 0.0.0-pr-35048-sha-7c1a3349
Triggered by @yannbf
Repository storybookjs/storybook
Branch jeppe-cursor/4b72cdff
Commit 7c1a3349
Datetime Thu Jun 4 11:50:59 UTC 2026 (1780573859)
Workflow run 26949928422

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=35048

Made with Cursor

Summary by CodeRabbit

  • Refactor

    • Change-detection reworked to use a shared module-graph service for more reliable lifecycle and startup behavior.
    • Start API simplified: change-detection now starts using a single enabled flag.
  • New Features

    • Added a module-graph service that exposes status, revisions, and file→story mappings for more accurate change tracking.
  • Bug Fixes

    • Docgen now proactively refreshes when dependent story files change, reducing stale documentation.

JReinhold and others added 2 commits June 3, 2026 23:23
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@JReinhold JReinhold self-assigned this Jun 4, 2026
@JReinhold JReinhold added maintenance User-facing maintenance tasks core ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. labels Jun 4, 2026
@JReinhold JReinhold changed the title Refactor module graph into open service Module Graph: Refactor from StoryDependencyService to open service-based ModuleGraphService Jun 4, 2026
@JReinhold JReinhold changed the base branch from next to claude/docgen-phase-3-rcm-provider June 4, 2026 05:23
Co-authored-by: Cursor <cursoragent@cursor.com>
@storybook-app-bot
Copy link
Copy Markdown

storybook-app-bot Bot commented Jun 4, 2026

Package Benchmarks

Commit: 3930029, ran on 5 June 2026 at 18:29:16 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 74 74 0
Self size 20.43 MB 20.44 MB 🚨 +13 KB 🚨
Dependency size 36.65 MB 36.65 MB 0 B
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 205 205 0
Self size 908 KB 908 KB 🎉 -144 B 🎉
Dependency size 89.16 MB 89.17 MB 🚨 +13 KB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 198 198 0
Self size 32 KB 32 KB 🚨 +36 B 🚨
Dependency size 87.65 MB 87.66 MB 🚨 +13 KB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 75 75 0
Self size 1.08 MB 1.08 MB 0 B
Dependency size 57.08 MB 57.09 MB 🚨 +13 KB 🚨
Bundle Size Analyzer node node

JReinhold and others added 7 commits June 4, 2026 08:41
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@JReinhold JReinhold marked this pull request as ready for review June 4, 2026 11:43
@JReinhold JReinhold requested review from ghengeveld and yannbf June 4, 2026 11:46
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR refactors change-detection to use a new core/module-graph open-service API instead of direct StoryDependencyGraphService dependency. The StoryDependencyGraphService is renamed to ModuleGraphEngine with updated lifecycle callbacks, ChangeDetectionService integrates with async module-graph queries, and the global dependency-graph service registry is removed in favor of deferred adapter resolution and service registration patterns.

Changes

Module-Graph Open-Service Implementation

Layer / File(s) Summary
Service types, error class, and shared utilities
code/core/src/shared/open-service/services/module-graph/errors.ts, types.ts
Define ErrorLike, ModuleGraphStatus, ModuleGraphServiceState, path normalization utilities (toStoryIndexPath, storyIndexPathToAbsolutePath, reverseIndexToStoriesByFile), and ModuleGraphFailureError class for lifecycle errors.
Service definition with queries and commands
definition.ts
Define moduleGraphServiceDef with valibot schemas for queries (getStoriesForFiles, getStatus, getGraphRevision, getLatestStoryChanges) and commands (applyGraphSnapshot, applyGraphUpdate, bumpGraphRevision, setStatus) managing in-memory reverse index state.
Service registration and open-service wiring
server.ts, story-files.ts
Implement registerModuleGraphService, manage deferred adapter resolution via changeDetectionAdapterPromise/resolveChangeDetectionAdapter, wire ModuleGraphEngine to service commands, and subscribe to story-index invalidation events.
Service query/command contract and wiring tests
server.test.ts
Verify service state transitions, path normalization, query result contracts, subscription notifications, and full registerModuleGraphService wiring with mocked dependency-graph components.
Module-graph test utilities
module-graph.test-helpers.ts
Provide createDeferred, createStoryIndex, createMockAdapter, buildReverseIndex, and installDependencyGraphMocks helpers for test setup across the module-graph subsystem.

ModuleGraphEngine Refactor

Layer / File(s) Summary
ModuleGraphEngine core implementation and lifecycle
engine/module-graph-engine.ts
Rename and refactor engine from StoryDependencyGraphService to ModuleGraphEngine with ModuleGraphEngineOptions (replacing storyIndexGeneratorPromise with getIndex() function and updating callbacks to onSnapshot/onUpdate/onStoryIndexInvalidated); rewrite startup flow, file-change handling, and story-index reconciliation.
ModuleGraphEngine test suite
engine/module-graph-engine.test.ts
Refactor tests to validate initial build snapshot callbacks, update propagation post-patch, story-index invalidation handling, and error/unavailability lifecycle with new callback contract.

ChangeDetectionService Refactor

Layer / File(s) Summary
ChangeDetectionService module-graph integration
code/core/src/core-server/change-detection/change-detection-service.ts
Add module-graph open-service subscriptions in constructor and start(); implement errorLikeToError helper for open-service error conversion; update status publishing lifecycle to route module-graph status callbacks; rewrite scan() and buildStatuses() to use module-graph queries instead of local lookups; update dispose() to unsubscribe from module-graph subscriptions.
ChangeDetectionService test setup with module-graph mocking
change-detection.test-helpers.ts
Introduce installModuleGraphQueryMock() for mocking core/module-graph open-service in tests, refactor createWiredChangeDetection() to use ModuleGraphEngine with engine-callback wiring, and re-export shared test helpers from module-graph test-helpers.
ChangeDetectionService test suite updates
change-detection-service.test.ts
Update all test cases to use service.start(enabled) signature (removing adapter parameter), remove explicit graph disposal, and wire module-graph mock for invalidation/status/query testing across 20+ test scenarios.

Remove Barrel Exports and Consolidate Module Structure

Layer / File(s) Summary
Remove change-detection barrel exports
code/core/src/core-server/change-detection/index.ts, adapters/index.ts, parser-registry/index.ts, dependency-graph/index.ts
Remove all re-exports from change-detection module barrel files to eliminate aggregated export surfaces and consolidate types into shared module-graph locations.
Refactor core-server index re-exports
code/core/src/core-server/index.ts
Split aggregated change-detection re-exports into targeted blocks sourcing from dedicated change-detection modules (errors.ts, readiness.ts) and shared module-graph engine type modules; remove StoryDependencyGraphService and experimental_getDependencyGraphService re-exports.

Dev-Server, Preset, and Docgen Integration

Layer / File(s) Summary
Dev-server change-detection wiring refactor
code/core/src/core-server/dev-server.ts
Remove explicit StoryDependencyGraphService construction; construct ChangeDetectionService directly; resolve adapter via resolveChangeDetectionAdapter(); pass only feature-gated enabled boolean to start(); remove onStoryIndexInvalidated callback from registerIndexJsonRoute.
Preset services registration for module-graph
code/core/src/core-server/presets/common-preset.ts
Register registerModuleGraphService in services preset with resolved storyIndexGenerator.getIndex(); reuse resolved generator for docgen service registration.
Docgen service module-graph subscription and refresh
code/core/src/shared/open-service/services/docgen/server.ts
Add workingDir parameter to service options; subscribe to core/module-graph latest story changes; map bumped story files to story-index entries and proactively refresh extracted docgen outputs for components with existing state.
Docgen and index-json API updates
code/core/src/shared/open-service/services/docgen/server.test.ts, core-server/utils/index-json.ts, index-json.test.ts
Register moduleGraphServiceDef in docgen tests; remove onStoryIndexInvalidated callback parameter from registerIndexJsonRoute() since story-index invalidation is now handled via module-graph service subscriptions.

Normalize Import Paths and Documentation

Layer / File(s) Summary
Dependency-graph module import path updates
engine/dependency-graph/*.ts, engine/parser-registry/*.ts (multiple files)
Update imports throughout dependency-graph modules to reference direct source files and align filename casing for case-sensitive module resolution; update error imports to use shared change-detection errors module.
Type reference updates
code/core/src/types/modules/core-common.ts
Update Builder.changeDetectionAdapter and StorybookConfigRaw.experimental_importParsers type imports to source from shared module-graph/engine type modules instead of core-server/change-detection equivalents.

Documentation Update

Layer / File(s) Summary
Cross-service composition guidelines
code/core/src/shared/open-service/README.md
Add clarification that operational cross-service reads should use await service.queries.<query>.loaded(input) form (which waits for data) rather than synchronous query(input) form (which returns immediately with potential staleness).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/change-detection.test-helpers.ts (1)

41-48: 💤 Low value

Hardcoded /repo path in test helper may cause path mismatch.

The toStoryIndexPath(storyFile, '/repo') uses a hardcoded working directory. If a test provides a different workingDir to createWiredChangeDetection, the mock's path conversion won't match the service's expectations.

♻️ Consider parameterizing the workingDir

The installModuleGraphQueryMock could accept an optional workingDir parameter to align with the test's working directory, though the current approach works for existing test fixtures using /repo.

🤖 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/core-server/change-detection/change-detection.test-helpers.ts`
around lines 41 - 48, getStoriesForFiles uses a hardcoded workingDir when
calling toStoryIndexPath, causing mismatches if tests call
createWiredChangeDetection with a different workingDir; update the
installModuleGraphQueryMock (or the helper that provides getStoriesForFiles) to
accept an optional workingDir parameter and pass that through to
getStoriesForFiles so toStoryIndexPath(storyFile, workingDir) is used instead of
'/repo', and update any callers of installModuleGraphQueryMock in tests to
forward their createWiredChangeDetection workingDir where needed.
🤖 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/definition.ts`:
- Around line 171-176: The handlers that set or bump graphRevision (e.g., the
anonymous handler shown, applyGraphSnapshot, and bumpGraphRevision) update
state.graphRevision but do not clear state.latestChangedStoryFiles, causing
getLatestStoryChanges to return stale files; update each place that increments
or replaces the revision (the handler that sets status='ready',
applyGraphSnapshot, and bumpGraphRevision) to also reset
state.latestChangedStoryFiles to an empty collection (e.g., [] or {}) inside the
same ctx.self.setState callback so the newest revision has no leftover
changed-file entries.

In
`@code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts`:
- Around line 99-117: mirrorUpdate currently only uses the post-patch
reverseIndex lookup and can miss stories whose edges were removed by the patch;
change mirrorUpdate to accept (or obtain) the pre-patch dependents and union
them with the post-patch lookup results before computing bumpedStoryFiles.
Specifically, update the mirrorUpdate signature to accept an extra
prePatchDependents (Set<string> or string[]), or capture lookup(normalized)
before applying the patch in the caller and pass it in; then merge that set with
the current this.reverseIndex.lookup(normalized) results plus this.storyFiles
into bumpedStoryFiles, and keep the rest of the logic (options.onUpdate,
reverseIndexToStoriesByFile, toStoryIndexPath) unchanged so both pre- and
post-patch dependents are included.
- Around line 237-239: Register the adapter.onStartupFailure handler before
performing any async startup work so early failures aren't missed: move the
attachment of adapter.onStartupFailure?.(...) to occur before calling
getResolveConfig(), getIndex(), or build(), and ensure it forwards event.reason
and event.error to this.options.onUnavailable so any startup error during those
async calls will be emitted.

---

Nitpick comments:
In `@code/core/src/core-server/change-detection/change-detection.test-helpers.ts`:
- Around line 41-48: getStoriesForFiles uses a hardcoded workingDir when calling
toStoryIndexPath, causing mismatches if tests call createWiredChangeDetection
with a different workingDir; update the installModuleGraphQueryMock (or the
helper that provides getStoriesForFiles) to accept an optional workingDir
parameter and pass that through to getStoriesForFiles so
toStoryIndexPath(storyFile, workingDir) is used instead of '/repo', and update
any callers of installModuleGraphQueryMock in tests to forward their
createWiredChangeDetection workingDir where needed.
🪄 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: f503b29f-7eea-4d79-b10d-fd9b95ebf43f

📥 Commits

Reviewing files that changed from the base of the PR and between 58b8104 and 7c1a334.

📒 Files selected for processing (47)
  • code/core/src/core-server/change-detection/active-service-registry.ts
  • code/core/src/core-server/change-detection/adapters/index.ts
  • code/core/src/core-server/change-detection/change-detection-service.test.ts
  • code/core/src/core-server/change-detection/change-detection-service.ts
  • code/core/src/core-server/change-detection/change-detection.test-helpers.ts
  • code/core/src/core-server/change-detection/dependency-graph/index.ts
  • code/core/src/core-server/change-detection/index.ts
  • code/core/src/core-server/change-detection/parser-registry/index.ts
  • code/core/src/core-server/dev-server.ts
  • code/core/src/core-server/index.ts
  • code/core/src/core-server/presets/common-preset.ts
  • code/core/src/core-server/utils/index-json.test.ts
  • code/core/src/core-server/utils/index-json.ts
  • code/core/src/shared/open-service/README.md
  • code/core/src/shared/open-service/services/docgen/server.test.ts
  • code/core/src/shared/open-service/services/docgen/server.ts
  • code/core/src/shared/open-service/services/module-graph/definition.ts
  • code/core/src/shared/open-service/services/module-graph/engine/adapters/types.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/adapters/types.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/dependency-graph-builder.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/dependency-graph-builder.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patch.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patcher.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/parse-resolve-cache.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/parse-resolve-cache.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/resolver-factory.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/resolver-factory.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/reverse-index.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/reverse-index.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/scope.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/types.ts
  • code/core/src/shared/open-service/services/module-graph/engine/dependency-graph/walk-from-story.ts
  • code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/builtins.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/builtins.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/mdx-parse.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/parser-registry.test.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/parser-registry.ts
  • code/core/src/shared/open-service/services/module-graph/engine/parser-registry/types.ts
  • code/core/src/shared/open-service/services/module-graph/errors.ts
  • code/core/src/shared/open-service/services/module-graph/module-graph.test-helpers.ts
  • code/core/src/shared/open-service/services/module-graph/server.test.ts
  • code/core/src/shared/open-service/services/module-graph/server.ts
  • code/core/src/shared/open-service/services/module-graph/story-files.ts
  • code/core/src/shared/open-service/services/module-graph/types.ts
  • code/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (7)
  • code/core/src/core-server/utils/index-json.ts
  • code/core/src/core-server/change-detection/adapters/index.ts
  • code/core/src/core-server/change-detection/parser-registry/index.ts
  • code/core/src/core-server/change-detection/active-service-registry.ts
  • code/core/src/core-server/utils/index-json.test.ts
  • code/core/src/core-server/change-detection/dependency-graph/index.ts
  • code/core/src/core-server/change-detection/index.ts

Comment on lines +171 to +176
handler: async (input, ctx) => {
ctx.self.setState((state) => {
state.status = { value: 'ready' };
state.storiesByFile = input.storiesByFile;
state.graphRevision += 1;
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear stale latestChangedStoryFiles when emitting non-update revisions.

applyGraphSnapshot and bumpGraphRevision increase graphRevision without resetting latestChangedStoryFiles, so getLatestStoryChanges can return old files for a newer revision.

🔧 Proposed fix
      handler: async (input, ctx) => {
        ctx.self.setState((state) => {
          state.status = { value: 'ready' };
          state.storiesByFile = input.storiesByFile;
          state.graphRevision += 1;
+          state.latestChangedStoryFiles = [];
        });
      },
...
      handler: async (_input, ctx) => {
        ctx.self.setState((state) => {
          state.graphRevision += 1;
+          state.latestChangedStoryFiles = [];
        });
      },

Also applies to: 210-213

🤖 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/definition.ts` around
lines 171 - 176, The handlers that set or bump graphRevision (e.g., the
anonymous handler shown, applyGraphSnapshot, and bumpGraphRevision) update
state.graphRevision but do not clear state.latestChangedStoryFiles, causing
getLatestStoryChanges to return stale files; update each place that increments
or replaces the revision (the handler that sets status='ready',
applyGraphSnapshot, and bumpGraphRevision) to also reset
state.latestChangedStoryFiles to an empty collection (e.g., [] or {}) inside the
same ctx.self.setState callback so the newest revision has no leftover
changed-file entries.

Comment on lines +99 to +117
private mirrorUpdate(changedFile: string): void {
if (!this.reverseIndex) {
return;
}
const normalized = normalize(changedFile);
const bumpedStoryFiles = new Set<string>();
for (const [storyFile] of this.reverseIndex.lookup(normalized)) {
bumpedStoryFiles.add(storyFile);
}
if (this.storyFiles.has(normalized)) {
bumpedStoryFiles.add(normalized);
}
this.options.onUpdate?.({
storiesByFile: reverseIndexToStoriesByFile(this.reverseIndex.asMap(), this.workingDir),
bumpedStoryFiles: Array.from(bumpedStoryFiles, (storyFile) =>
toStoryIndexPath(storyFile, this.workingDir)
),
});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Include pre-patch dependents when computing bumpedStoryFiles.

mirrorUpdate currently derives bumps from the post-patch index only. When a patch removes edges for event.path, affected stories can disappear from lookup and never get bumped.

💡 Proposed fix
@@
-  private mirrorUpdate(changedFile: string): void {
+  private mirrorUpdate(changedFile: string, prePatchBumped = new Set<string>()): void {
@@
-    const bumpedStoryFiles = new Set<string>();
+    const bumpedStoryFiles = new Set<string>(prePatchBumped);
@@
   private async handleFileChange(event: FileChangeEvent): Promise<void> {
     if (!this.incrementalPatcher) {
       return;
     }
+    const normalized = normalize(event.path);
+    const prePatchBumped = new Set<string>();
+    if (this.reverseIndex) {
+      for (const [storyFile] of this.reverseIndex.lookup(normalized)) {
+        prePatchBumped.add(storyFile);
+      }
+    }
+    if (this.storyFiles.has(normalized)) {
+      prePatchBumped.add(normalized);
+    }
     try {
       await this.incrementalPatcher.patch(event);
     } catch (error) {
@@
     }
-    this.mirrorUpdate(event.path);
+    this.mirrorUpdate(event.path, prePatchBumped);
   }

Also applies to: 370-382

🤖 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 99 - 117, mirrorUpdate currently only uses the post-patch
reverseIndex lookup and can miss stories whose edges were removed by the patch;
change mirrorUpdate to accept (or obtain) the pre-patch dependents and union
them with the post-patch lookup results before computing bumpedStoryFiles.
Specifically, update the mirrorUpdate signature to accept an extra
prePatchDependents (Set<string> or string[]), or capture lookup(normalized)
before applying the patch in the caller and pass it in; then merge that set with
the current this.reverseIndex.lookup(normalized) results plus this.storyFiles
into bumpedStoryFiles, and keep the rest of the logic (options.onUpdate,
reverseIndexToStoriesByFile, toStoryIndexPath) unchanged so both pre- and
post-patch dependents are included.

Comment on lines +237 to +239
adapter.onStartupFailure?.((event) => {
this.options.onUnavailable?.(event.reason, event.error);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Register onStartupFailure before async startup to avoid missing early failure events.

The handler is attached only after getResolveConfig(), getIndex(), and build(). If startup failure fires during that window, onUnavailable is never emitted.

💡 Proposed fix
@@
-    // Subscribe BEFORE build — buffer events until patcher is ready
+    // Subscribe BEFORE build — buffer events until patcher is ready
     const eventBuffer: FileChangeEvent[] = [];
     const unsubscribeBuffer = adapter.onFileChange((event) => {
       eventBuffer.push(event);
     });
+    adapter.onStartupFailure?.((event) => {
+      this.options.onUnavailable?.(event.reason, event.error);
+    });
@@
-    adapter.onStartupFailure?.((event) => {
-      this.options.onUnavailable?.(event.reason, event.error);
-    });
-
     this.mirrorSnapshot();
🤖 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 237 - 239, Register the adapter.onStartupFailure handler before
performing any async startup work so early failures aren't missed: move the
attachment of adapter.onStartupFailure?.(...) to occur before calling
getResolveConfig(), getIndex(), or build(), and ensure it forwards event.reason
and event.error to this.options.onUnavailable so any startup error during those
async calls will be emitted.

Keep selectComponentEntriesByComponentId from the docgen phase-3 base
while preserving module-graph subscription reactivity in docgen.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/core-server/presets/common-preset.ts (1)

333-367: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep story-index resolution lazy in services.

options.presets.apply('storyIndexGenerator') initializes the full generator immediately, so manager-only / ignorePreview runs still build the story index before the guard on Lines 345-348 is checked. That defeats the skip documented there and can bring back unnecessary work or preview-side failures in flows that intentionally avoid preview output.

Suggested change
-  // `presets.apply` flattens the generator preset's returned promise, so this is the resolved
-  // generator, not a promise.
-  const storyIndexGenerator =
-    await options.presets.apply<StoryIndexGenerator>('storyIndexGenerator');
+  const getIndex = async () => {
+    const storyIndexGenerator =
+      await options.presets.apply<StoryIndexGenerator>('storyIndexGenerator');
+    return storyIndexGenerator.getIndex();
+  };

   registerModuleGraphService({
     channel: options.channel,
-    getIndex: () => storyIndexGenerator.getIndex(),
+    getIndex,
     workingDir: process.cwd(),
     presets: options.presets,
   });
@@
     registerDocgenService({
-      getIndex: () => storyIndexGenerator.getIndex(),
+      getIndex,
       provider,
       workingDir: process.cwd(),
     });
🤖 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/core-server/presets/common-preset.ts` around lines 333 - 367,
The code eagerly calls options.presets.apply('storyIndexGenerator') (symbol:
storyIndexGenerator) which forces full story-index init even when preview is
skipped; instead make resolution lazy: remove the top-level await and replace it
with a lazily-resolving accessor (e.g. a getStoryIndex or
storyIndexGeneratorPromise resolver) that only calls
options.presets.apply('storyIndexGenerator') the first time getIndex is invoked,
cache the resolved generator, and then have registerModuleGraphService and
registerDocgenService use that accessor (replace getIndex: () =>
storyIndexGenerator.getIndex() with getIndex: () => getStoryIndex().then(g =>
g.getIndex()) or a sync wrapper that awaits internally) so the generator is only
created when needed and the features/ignorePreview guard can skip creating it
entirely.
🤖 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/core-server/presets/common-preset.ts`:
- Around line 333-367: The code eagerly calls
options.presets.apply('storyIndexGenerator') (symbol: storyIndexGenerator) which
forces full story-index init even when preview is skipped; instead make
resolution lazy: remove the top-level await and replace it with a
lazily-resolving accessor (e.g. a getStoryIndex or storyIndexGeneratorPromise
resolver) that only calls options.presets.apply('storyIndexGenerator') the first
time getIndex is invoked, cache the resolved generator, and then have
registerModuleGraphService and registerDocgenService use that accessor (replace
getIndex: () => storyIndexGenerator.getIndex() with getIndex: () =>
getStoryIndex().then(g => g.getIndex()) or a sync wrapper that awaits
internally) so the generator is only created when needed and the
features/ignorePreview guard can skip creating it entirely.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 19e86824-5cc1-46b4-8e11-ed112a32806c

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1a334 and 3930029.

📒 Files selected for processing (4)
  • code/core/src/core-server/presets/common-preset.ts
  • code/core/src/shared/open-service/services/docgen/server.test.ts
  • code/core/src/shared/open-service/services/docgen/server.ts
  • code/core/src/types/modules/core-common.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • code/core/src/shared/open-service/services/docgen/server.ts
  • code/core/src/shared/open-service/services/docgen/server.test.ts
  • code/core/src/types/modules/core-common.ts

Base automatically changed from claude/docgen-phase-3-rcm-provider to next June 5, 2026 18:32
@JReinhold
Copy link
Copy Markdown
Contributor Author

JReinhold commented Jun 6, 2026

Manual QA — Change Detection (internal UI Storybook)

QA run against the internal UI Storybook (code/.storybook, port 6006) on branch jeppe-cursor/4b72cdff after merging claude/docgen-phase-3-rcm-provider. Sandboxes were not used (gitignored / won't reflect this branch).

Environment

Field Value
Storybook URL http://localhost:6006
Primary story controls-controlspanel--ref-story-controls-load
Test files ControlsPanel.tsx, ControlsPanel.stories.tsx, SaveStory.tsx (transitive)
Config features.changeDetection: true, core.changeDetection: true in code/.storybook/main.ts

Overall verdict: PASS

Per-step results

Step Result Notes
1. Clean git tree PASS Clean at start and end
2. Change detection enabled PASS Already configured in internal UI
3. Start dev server PASS yarn storybook:ui
4. Clean sidebar (no unexpected statuses) PASS No change-detection badges on clean tree
5. Edit component → Modified PASS Modified dots on Controls Panel + Ref Story Controls Load after expanding sidebar + activating review filter
6. Edit transitive dep → Affected PASS* Status store shows status-value:affected on dependent story; Affected icon is not rendered in sidebar (Tree.tsx hides it by design)
7. Add story file → index update PASS New entry in index.json within ~1–3s; New status on component + story
8. Revert edits → statuses clear PASS Statuses cleared; index entry removed
9. Second edit (incremental) PASS Modified status returns without restart

Step 4

step4-clean-sidebar

Step 5
step5-modified

Step 5 expanded
step5-modified-expanded

Step 6
step6-affected-store

Step 7
step7-new-story

Step 8
step8-clean-revert

Step 9
step9-incremental

QA tip for reviewers

Status icons are only visible when the sidebar tree is expanded (controlsControls Panel) and, for Modified, the Review … stories switch is active.

Screenshots

Screenshots could not be embedded automatically (GitHub gist/API does not accept binary uploads via CLI). Please attach the files from the local qa-screenshots/ folder on the PR branch worktree:

Step File
4 — clean sidebar qa-screenshots/step4-clean-sidebar.png
5 — Modified qa-screenshots/step5-modified.png
6 — transitive edit (SaveStory) qa-screenshots/step6-affected-store.png
7 — new story qa-screenshots/step7-new-story.png
8 — reverted / clean qa-screenshots/step8-clean-revert.png
9 — incremental update qa-screenshots/step9-incremental.png

Local path: qa-screenshots/ at the repo root of the PR worktree.

Observations

  1. Affected status is computed and stored correctly but the sidebar does not render an Affected badge (intentional UI behavior in Tree.tsx).
  2. Modified badges require the review filter to be active.
  3. Brief HMR error if you delete a story while the browser is still on its URL — resolves after navigating to a valid story.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal Run our default set of CI jobs (choose this for most PRs). core maintenance User-facing maintenance tasks qa:needed Pull Requests that will need manual QA prior to release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant