Core: Extract StoryDependencyGraphService from ChangeDetectionService#35009
Merged
Conversation
…ionService ChangeDetectionService fused two independent responsibilities: the module dependency graph (build a reverse index from story files, watch builder file events, patch incrementally) and the status pipeline (git diff -> affected stories -> status store). This splits the graph half into a standalone StoryDependencyGraphService that ChangeDetectionService now composes. The two halves shared no state; the only coupling was a single reverseIndex.lookup() call plus a patch-settle drain. The graph is now independently constructible and testable, and "graph-only" usage is expressed by constructing the tracker without a status publisher rather than via an internal mode flag. - StoryDependencyGraphService owns the adapter subscription, graph build, incremental patcher, patchQueue serialization, story-file reconciliation, and the OXC parse-pool teardown. It exposes lookup(), whenSettled() (a tail-snapshot patch-settle barrier), onStoryIndexInvalidated(), and onReady/onChange/onError/onUnavailable lifecycle callbacks. It does not touch the change-detection readiness signal. - ChangeDetectionService keeps its public surface (start/onStoryIndexInvalidated/ dispose) and stays the sole owner of git diffing, the status store, and readiness. dispose() is now idempotent and tears down the graph last. - Shared story-file mapping moved to story-files.ts; shared test scaffolding moved to change-detection.test-helpers.ts. - Graph-internal behavior is now tested directly in StoryDependencyGraphService.test.ts; ChangeDetectionService.test.ts retains the status / integration / readiness coverage. No behavior change: dev-server wiring and the public barrel are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
valentinpalkovic
added
the
cleanup
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:
📝 WalkthroughWalkthroughExtracts graph/patching into StoryDependencyGraphService, composes it into ChangeDetectionService (which now awaits graph readiness/whenSettled), centralizes test helpers and story-file utilities, updates dev-server wiring, and adjusts tests to use the new service and helpers. ChangesGraph Service Extraction and Change Detection Refactoring
Sequence Diagram(s)sequenceDiagram
participant DevServer as DevServer
participant CDS as ChangeDetectionService
participant Graph as StoryDependencyGraphService
participant Adapter as ChangeDetectionAdapter
participant GitDiff as GitDiffProvider
DevServer->>Graph: construct(options) and start(adapter)
Graph->>Adapter: subscribe file-change & getResolveConfig
Graph->>Graph: build dependency graph & reverse index
Graph->>CDS: onReady()
CDS->>CDS: startStatusPipeline() (once)
Graph->>CDS: onChange() (debounced)
CDS->>Graph: whenSettled()
Graph->>Graph: await patch queue tail
CDS->>GitDiff: getChangedFiles()
CDS->>Graph: lookup(changedFile)
CDS->>CDS: buildStatuses()
CDS->>CDS: publish statuses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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/core-server/change-detection/ChangeDetectionService.ts`:
- Around line 259-267: The current await this.graph.whenSettled() in scan() runs
too early and does not cover the awaits inside buildStatuses(), so a new patch
can start before the first graph.lookup() and produce inconsistent statuses; fix
this by ensuring the graph is settled immediately before any graph read—either
move or add an await this.graph.whenSettled() right before the first
graph.lookup() (or at the start of buildStatuses()), and keep references to the
same storyIndex/graph snapshot used for subsequent operations (e.g., ensure
scan() calls buildStatuses() only after awaiting whenSettled() so
buildStatuses() uses a stable graph and storyIndex).
- Around line 184-190: The onUnavailable handler currently calls this.dispose()
without observing rejections; change that call to handle promise rejection so
disposal failures don't become unhandled rejections. Replace the bare void
this.dispose() with a handled call such as this.dispose().catch(err =>
logger.warn('Error during dispose after unavailable:', err)) (or await in a
try/catch if you make onUnavailable async) so any thrown/rejected errors from
dispose/this.graph.dispose() are logged and swallowed.
In `@code/core/src/core-server/change-detection/StoryDependencyGraphService.ts`:
- Around line 165-217: Subscribe to adapter.onStartupFailure before awaiting
dependencyGraphBuilder.build to ensure startup failures emitted during the
initial build window are handled; move the onStartupFailure wiring (creating
unsubscribeStartupFailure and invoking options.onUnavailable and dispose()) to
occur alongside the initial adapter.onFileChange buffering setup (before calling
this.dependencyGraphBuilder.build(this.storyFiles)), and keep the existing
disposal checks and the unsubscribe semantics so the handler is removed after
the build if 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: 1994da2d-c70f-4d49-b1d3-dcfbfb2ac46a
📒 Files selected for processing (6)
code/core/src/core-server/change-detection/ChangeDetectionService.test.tscode/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/StoryDependencyGraphService.test.tscode/core/src/core-server/change-detection/StoryDependencyGraphService.tscode/core/src/core-server/change-detection/change-detection.test-helpers.tscode/core/src/core-server/change-detection/story-files.ts
Comment on lines
+184
to
+190
| onUnavailable: (reason, error) => { | ||
| if (this.disposed) { | ||
| return; | ||
| } | ||
| logger.warn(`Change detection unavailable: ${reason}`); | ||
| this.resolveReadiness({ status: 'unavailable', reason, error }); | ||
| void this.dispose(); |
Contributor
There was a problem hiding this comment.
Catch dispose() failures on the unavailable path.
Line 190 fires async cleanup without observing rejection. If this.graph.dispose() throws here, this path turns an already-handled “unavailable” condition into an unhandled rejection.
Suggested fix
onUnavailable: (reason, error) => {
if (this.disposed) {
return;
}
logger.warn(`Change detection unavailable: ${reason}`);
this.resolveReadiness({ status: 'unavailable', reason, error });
- void this.dispose();
+ void this.dispose().catch(() => undefined);
},🤖 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/ChangeDetectionService.ts` around
lines 184 - 190, The onUnavailable handler currently calls this.dispose()
without observing rejections; change that call to handle promise rejection so
disposal failures don't become unhandled rejections. Replace the bare void
this.dispose() with a handled call such as this.dispose().catch(err =>
logger.warn('Error during dispose after unavailable:', err)) (or await in a
try/catch if you make onUnavailable async) so any thrown/rejected errors from
dispose/this.graph.dispose() are logged and swallowed.
Comment on lines
+165
to
+217
| // Subscribe BEFORE build — buffer events until patcher is ready | ||
| const eventBuffer: FileChangeEvent[] = []; | ||
| this.unsubscribeFileChange = adapter.onFileChange((event) => { | ||
| if (this.disposed) { | ||
| return; | ||
| } | ||
| eventBuffer.push(event); | ||
| }); | ||
|
|
||
| const { reverseIndex, graph } = await this.dependencyGraphBuilder.build(this.storyFiles); | ||
| if (this.disposed) { | ||
| return; | ||
| } | ||
| this.reverseIndex = reverseIndex; | ||
| void this.dumpDebugSnapshot(reverseIndex, graph, projectRoot, workspaceRoots, cache); | ||
|
|
||
| this.incrementalPatcher = new IncrementalPatcher({ | ||
| reverseIndex, | ||
| graph, | ||
| registry, | ||
| resolver, | ||
| workspaceRoots, | ||
| projectRoot, | ||
| cache, | ||
| isStoryFile: (path: string) => this.storyFiles.has(normalize(path)), | ||
| }); | ||
|
|
||
| // Drain buffered events into patchQueue, then switch to live handler | ||
| this.unsubscribeFileChange?.(); | ||
| for (const event of eventBuffer) { | ||
| this.patchQueue = this.patchQueue | ||
| .then(() => this.handleFileChange(event)) | ||
| .catch(() => undefined); | ||
| } | ||
|
|
||
| this.unsubscribeFileChange = adapter.onFileChange((event) => { | ||
| if (this.disposed) { | ||
| return; | ||
| } | ||
| this.patchQueue = this.patchQueue | ||
| .then(() => this.handleFileChange(event)) | ||
| .catch(() => undefined); | ||
| }); | ||
|
|
||
| if (adapter.onStartupFailure) { | ||
| this.unsubscribeStartupFailure = adapter.onStartupFailure((event) => { | ||
| if (this.disposed) { | ||
| return; | ||
| } | ||
| this.options.onUnavailable?.(event.reason, event.error); | ||
| void this.dispose(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect prior placement of onStartupFailure relative to the eager build in ChangeDetectionService history.
fd -t f 'ChangeDetectionService.ts' --exec sh -c 'echo "== {} =="; git log -n 3 --oneline -- {}'
fd -t f 'ChangeDetectionService.ts' --exec rg -n -C2 'onStartupFailure|\.build\(' {}Repository: storybookjs/storybook
Length of output: 233
Subscribe to adapter.onStartupFailure before the eager dependency-graph build.
StoryDependencyGraphService buffers onFileChange before await this.dependencyGraphBuilder.build(...), but wires adapter.onStartupFailure only after the build resolves; if the adapter can emit startup failures during that build window, options.onUnavailable/dispose() won’t run.
♻️ Suggested reordering
this.unsubscribeFileChange = adapter.onFileChange((event) => {
if (this.disposed) {
return;
}
eventBuffer.push(event);
});
+ if (adapter.onStartupFailure) {
+ this.unsubscribeStartupFailure = adapter.onStartupFailure((event) => {
+ if (this.disposed) {
+ return;
+ }
+ this.options.onUnavailable?.(event.reason, event.error);
+ void this.dispose();
+ });
+ }
+
const { reverseIndex, graph } = await this.dependencyGraphBuilder.build(this.storyFiles);🤖 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/StoryDependencyGraphService.ts`
around lines 165 - 217, Subscribe to adapter.onStartupFailure before awaiting
dependencyGraphBuilder.build to ensure startup failures emitted during the
initial build window are handled; move the onStartupFailure wiring (creating
unsubscribeStartupFailure and invoking options.onUnavailable and dispose()) to
occur alongside the initial adapter.onFileChange buffering setup (before calling
this.dependencyGraphBuilder.build(this.storyFiles)), and keep the existing
disposal checks and the unsubscribe semantics so the handler is removed after
the build if needed.
…ed contract Make `whenSettled()` a two-phase barrier that first awaits any in-flight story-index reconciliation (tracked via a new `refreshSettled` field) before snapshotting the patch tail, so a consumer that does `whenSettled()` -> `lookup()` no longer observes a pre-reconciliation graph. The single-flight guard moves from `refreshStoryFiles` to `onStoryIndexInvalidated` so a dropped second invalidation cannot overwrite `refreshSettled` with a resolved no-op and let the barrier skip the real reconciliation. `onChange` timing is unchanged and `getIndex()` stays off the patch queue, so this is behavior-preserving for ChangeDetectionService (which already self-heals via its debounced rescan). Also rewrite the `onChange`/`whenSettled` JSDoc to describe the honest contract (onChange is an edge-triggered coalesce signal, not a settled read; whenSettled is a point-in-time barrier, not a freeze), drop a cross-file line reference and a vitest-mock implementation transcript per AGENTS.md comment rules, and fix a typo. Adds a concurrency test asserting whenSettled() blocks until an in-flight reconciliation settles. All 142 change-detection tests pass; core typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Tracked follow-up (not in this PR): OXC parse-pool ownershipOne item from review is intentionally deferred rather than fixed here.
It becomes a real bug only once a second consumer drives the graph independently — i.e. the server-side docgen re-extraction (#34998) this extraction is built for. At that point two Because the fix should be designed against the real second consumer (ref-counted pool ownership, or a single owning coordinator) and |
valentinpalkovic
changed the title
Contributor
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/core-server/change-detection/StoryDependencyGraphService.ts (1)
250-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuffer pre-ready story-index invalidations instead of dropping them.
If
onStoryIndexInvalidated()fires while startup is still building the graph,this.incrementalPatcheris undefined and this guard skips reconciliation permanently. A story file added/removed during that window can leavestoryFilesandreverseIndexstale until a second invalidation happens. Please persist a pending invalidation and replay it once the patcher is ready, before signaling readiness.🤖 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/StoryDependencyGraphService.ts` around lines 250 - 267, onStoryIndexInvalidated currently drops invalidations when incrementalPatcher is undefined during startup; change it to record a pending invalidation flag (e.g. this.pendingStoryIndexInvalidation) when incrementalPatcher is not yet ready, and return; when incrementalPatcher is later initialized (where it is assigned), check that flag and invoke the same single-flight path (i.e. call the logic that sets refreshInFlight, assigns refreshSettled to refreshStoryFiles().catch(() => undefined).finally(() => { refreshInFlight = false; }) or simply call onStoryIndexInvalidated()) before signaling readiness so the buffered invalidation is replayed; keep the existing single-flight guards (refreshInFlight, refreshSettled, refreshStoryFiles, incrementalPatcher) so multiple rapid invalidations still coalesce.
🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/StoryDependencyGraphService.test.ts (1)
301-321: ⚡ Quick winMove this test-specific mock behavior into
beforeEach.This regression case configures
patchSpyandgetIndexinline inside the test body, which diverges from the suite's Vitest mocking rules. Please move those implementations into abeforeEachin a nesteddescribeand keep the test focused on the concurrency assertions.As per coding guidelines, "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases 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/core-server/change-detection/StoryDependencyGraphService.test.ts` around lines 301 - 321, Move the test-specific mock implementations out of the test body into a nested describe's beforeEach: extract the patchSpy.mockImplementation block (which checks event.kind/path and calls reverseIndex.record('/repo/src/B.stories.tsx',...)) and the getIndex mock sequence (the vi.fn() that .mockResolvedValueOnce(initialIndex) and .mockImplementationOnce(async () => { await getIndexDeferred.promise; return updatedIndex; })) and set them up in beforeEach so the test itself only performs the concurrency assertions; keep references to patchSpy, getIndex, getIndexDeferred, initialIndex, and updatedIndex when moving the behavior.
🤖 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/change-detection/StoryDependencyGraphService.ts`:
- Around line 250-267: onStoryIndexInvalidated currently drops invalidations
when incrementalPatcher is undefined during startup; change it to record a
pending invalidation flag (e.g. this.pendingStoryIndexInvalidation) when
incrementalPatcher is not yet ready, and return; when incrementalPatcher is
later initialized (where it is assigned), check that flag and invoke the same
single-flight path (i.e. call the logic that sets refreshInFlight, assigns
refreshSettled to refreshStoryFiles().catch(() => undefined).finally(() => {
refreshInFlight = false; }) or simply call onStoryIndexInvalidated()) before
signaling readiness so the buffered invalidation is replayed; keep the existing
single-flight guards (refreshInFlight, refreshSettled, refreshStoryFiles,
incrementalPatcher) so multiple rapid invalidations still coalesce.
---
Nitpick comments:
In
`@code/core/src/core-server/change-detection/StoryDependencyGraphService.test.ts`:
- Around line 301-321: Move the test-specific mock implementations out of the
test body into a nested describe's beforeEach: extract the
patchSpy.mockImplementation block (which checks event.kind/path and calls
reverseIndex.record('/repo/src/B.stories.tsx',...)) and the getIndex mock
sequence (the vi.fn() that .mockResolvedValueOnce(initialIndex) and
.mockImplementationOnce(async () => { await getIndexDeferred.promise; return
updatedIndex; })) and set them up in beforeEach so the test itself only performs
the concurrency assertions; keep references to patchSpy, getIndex,
getIndexDeferred, initialIndex, and updatedIndex when moving the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd36ade2-e398-4000-8e65-dbf83c994787
📒 Files selected for processing (5)
code/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/StoryDependencyGraphService.test.tscode/core/src/core-server/change-detection/StoryDependencyGraphService.tscode/core/src/core-server/change-detection/change-detection.test-helpers.tscode/core/src/core-server/change-detection/story-files.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/core-server/change-detection/story-files.ts
- code/core/src/core-server/change-detection/change-detection.test-helpers.ts
- code/core/src/core-server/change-detection/ChangeDetectionService.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 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/core-server/change-detection/index.ts`:
- Around line 11-14: The barrel export incorrectly re-exports
setActiveStoryDependencyGraphService which doesn't exist; update the export list
to re-export the actual symbol exported from active-service-registry.ts by
replacing setActiveStoryDependencyGraphService with setDependencyGraphService so
the exported names (getDependencyGraphService and setDependencyGraphService)
match the implementations in active-service-registry.ts.
🪄 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: 1a9caae5-e8e9-49c5-856e-c07e72d7ed28
📒 Files selected for processing (4)
code/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/active-service-registry.tscode/core/src/core-server/change-detection/index.tscode/core/src/core-server/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/core-server/change-detection/ChangeDetectionService.ts
Move StoryDependencyGraphService lifecycle to dev-server so the graph stays available independently of the change-detection feature flag, while keeping status publishing explicitly feature-gated. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
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/core-server/change-detection/ChangeDetectionService.ts (1)
128-154:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShared
graphinstances never notify this service after startup.When
options.graphis passed, the lifecycle callbacks in Lines 143-154 are skipped, and Lines 213-218 only do a one-offonGraphReady(). That means later graphonChange/onError/onUnavailableevents from a shared instance can never reachChangeDetectionService, so injected-graph mode can miss rescans and readiness/error transitions. Please register listeners for externally owned graphs too, or enforce/document that an injected graph must already be fully ready and externally bridged to this service.Also applies to: 213-218
🤖 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/ChangeDetectionService.ts` around lines 128 - 154, When an external StoryDependencyGraphService is injected via options.graph, ChangeDetectionService currently skips wiring lifecycle callbacks so later onChange/onError/onUnavailable events never reach it; update the constructor to always attach the same lifecycle handlers (onGraphReady, onGraphChange, onGraphError, onGraphUnavailable) to this.graph even when options.graph is provided (and keep ownsGraph = false), or alternatively validate/document that an injected graph is already bridged; reference the ChangeDetectionService constructor, the this.graph assignment, and the lifecycle handler methods (onGraphReady, onGraphChange, onGraphError, onGraphUnavailable) and ensure you register those handlers on the external graph instance after assignment.
♻️ Duplicate comments (1)
code/core/src/core-server/change-detection/ChangeDetectionService.ts (1)
183-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch
dispose()failures on both unavailable paths.Lines 190 and 317 still trigger cleanup without handling a rejected
dispose(). Ifthis.graph.dispose()throws, an already-handled unavailable state turns into an unhandled rejection.Suggested fix
onGraphUnavailable(reason: string, error?: Error): void { if (this.disposed || !this.changeDetectionEnabled) { return; } logger.warn(`Change detection unavailable: ${reason}`); this.resolveReadiness({ status: 'unavailable', reason, error }); - void this.dispose(); + void this.dispose().catch((disposeError) => { + logger.warn('Change detection dispose failed after unavailable', disposeError); + }); }if (error instanceof ChangeDetectionUnavailableError) { logger.warn(`Change detection unavailable: ${error.message}`); this.resolveReadiness({ status: 'unavailable', reason: error.message, error, }); - await this.dispose(); + await this.dispose().catch((disposeError) => { + logger.warn('Change detection dispose failed after unavailable', disposeError); + }); } else if (error instanceof ChangeDetectionFailureError) {Also applies to: 310-317
🤖 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/ChangeDetectionService.ts` around lines 183 - 190, The onGraphUnavailable path (onGraphUnavailable) and the other unavailable branch that calls this.graph.dispose() must not let a rejected dispose() turn a handled unavailable state into an unhandled rejection; change the void this.dispose() call and the this.graph.dispose() call to awaitable calls with try/catch (or use .catch(...)) so any errors from dispose() are caught and logged (e.g., via logger.error) and do not propagate; ensure resolveReadiness({ status: 'unavailable', ... }) remains invoked before swallowing dispose errors.
🧹 Nitpick comments (1)
code/core/src/core-server/dev-server.ts (1)
78-82: ⚡ Quick winLog disposal errors for observability.
The
disposeChangeDetectionRuntimehelper swallows disposal errors with.catch(() => undefined), which reduces observability and could mask shutdown issues. Consider logging failures to help diagnose disposal problems.📊 Proposed fix to log disposal errors
const disposeChangeDetectionRuntime = async () => { - await changeDetectionService.dispose().catch(() => undefined); + await changeDetectionService.dispose().catch((err) => { + logger.error('Failed to dispose change detection service', err); + }); setDependencyGraphService(undefined); - await storyDependencyGraphService.dispose().catch(() => undefined); + await storyDependencyGraphService.dispose().catch((err) => { + logger.error('Failed to dispose story dependency graph service', err); + }); };As per coding guidelines, use
storybook/internal/node-loggerfor server-side logging instead of rawconsole.*.🤖 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/dev-server.ts` around lines 78 - 82, The disposeChangeDetectionRuntime helper currently swallows errors from changeDetectionService.dispose() and storyDependencyGraphService.dispose(); update it to catch and log those errors using the storybook/internal/node-logger instead of ignoring them. Locate disposeChangeDetectionRuntime and replace the .catch(() => undefined) handlers with error handlers that call the logger (importing the logger at the top) and include contextual messages like "error disposing changeDetectionService" and "error disposing storyDependencyGraphService"; keep the call to setDependencyGraphService(undefined) as-is and ensure the disposal awaits still resolve even after logging.
🤖 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/change-detection/ChangeDetectionService.ts`:
- Around line 128-154: When an external StoryDependencyGraphService is injected
via options.graph, ChangeDetectionService currently skips wiring lifecycle
callbacks so later onChange/onError/onUnavailable events never reach it; update
the constructor to always attach the same lifecycle handlers (onGraphReady,
onGraphChange, onGraphError, onGraphUnavailable) to this.graph even when
options.graph is provided (and keep ownsGraph = false), or alternatively
validate/document that an injected graph is already bridged; reference the
ChangeDetectionService constructor, the this.graph assignment, and the lifecycle
handler methods (onGraphReady, onGraphChange, onGraphError, onGraphUnavailable)
and ensure you register those handlers on the external graph instance after
assignment.
---
Duplicate comments:
In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts`:
- Around line 183-190: The onGraphUnavailable path (onGraphUnavailable) and the
other unavailable branch that calls this.graph.dispose() must not let a rejected
dispose() turn a handled unavailable state into an unhandled rejection; change
the void this.dispose() call and the this.graph.dispose() call to awaitable
calls with try/catch (or use .catch(...)) so any errors from dispose() are
caught and logged (e.g., via logger.error) and do not propagate; ensure
resolveReadiness({ status: 'unavailable', ... }) remains invoked before
swallowing dispose errors.
---
Nitpick comments:
In `@code/core/src/core-server/dev-server.ts`:
- Around line 78-82: The disposeChangeDetectionRuntime helper currently swallows
errors from changeDetectionService.dispose() and
storyDependencyGraphService.dispose(); update it to catch and log those errors
using the storybook/internal/node-logger instead of ignoring them. Locate
disposeChangeDetectionRuntime and replace the .catch(() => undefined) handlers
with error handlers that call the logger (importing the logger at the top) and
include contextual messages like "error disposing changeDetectionService" and
"error disposing storyDependencyGraphService"; keep the call to
setDependencyGraphService(undefined) as-is and ensure the disposal awaits still
resolve even after logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dde2d1d4-dd49-4e9a-a877-2b9b424196e7
📒 Files selected for processing (4)
code/core/src/core-server/change-detection/ChangeDetectionService.test.tscode/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/active-service-registry.tscode/core/src/core-server/dev-server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/core-server/change-detection/active-service-registry.ts
- code/core/src/core-server/change-detection/ChangeDetectionService.test.ts
yannbf
added
feature request
ci:normal
valentinpalkovic
commented
Jun 2, 2026
valentinpalkovic
commented
Jun 2, 2026
JReinhold
approved these changes
Jun 2, 2026
This was referenced Jun 2, 2026
This was referenced Jun 2, 2026
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
3 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.
What I did
ChangeDetectionServicehad fused two independent responsibilities into one class: the module dependency graph (build a reverse index from story files, watch builder file events, patch incrementally) and the status pipeline (git diff → affected stories → status store). This splits the graph half into a standaloneStoryDependencyGraphServicethatChangeDetectionServicenow composes.The two halves shared no state — the only coupling was a single
reverseIndex.lookup()call plus a patch-settle drain — so this is a mechanical, behavior-preserving move:StoryDependencyGraphService(new) owns the builder-adapter subscription, the eager graph build, theIncrementalPatcher,patchQueueserialization, story-file reconciliation, and the OXC parse-pool teardown. Its public surface is intentionally narrow:lookup(),whenSettled()(a two-phase settle barrier — see below),onStoryIndexInvalidated(),start/dispose, andonReady/onChange/onError/onUnavailablelifecycle callbacks. It does not touch the change-detection readiness signal.ChangeDetectionServicekeeps its public surface (start/onStoryIndexInvalidated/dispose) and remains the sole owner of git diffing, the status store, and readiness.dispose()is now idempotent and tears down the graph last (drain patches → dispose pool).story-files.tsandchange-detection.test-helpers.ts.dev-server.tswiring and the publiccore-serverbarrel are untouched, so there is no behavior change. This decoupling is a precursor that lets a future consumer (server-side docgen re-extraction, #34998) drive the dependency graph independently of the status feature.Follow-up commit:
onChange/whenSettledcontract hardeningThe first review surfaced two latent issues in the tracker's read contract — harmless for
ChangeDetectionServicetoday (it self-heals via its debounced rescan), but foot-guns for the future independent consumer this extraction is built for. Both are addressed in a behavior-preserving follow-up commit:whenSettled()is now a two-phase barrier.onStoryIndexInvalidated()kicks off an async reconciliation (getIndex→ add/unlink patches) and firesonChangesynchronously, before those patches are enqueued. Previously a consumer doingwhenSettled()→lookup()could snapshot a patch tail that didn't yet contain the reconciliation and read a pre-reconciliation graph.whenSettled()now first awaits any in-flight reconciliation (tracked via arefreshSettledfield), then drains the patch tail. To avoid a coalescing bug, the single-flight guard moved fromrefreshStoryFilestoonStoryIndexInvalidated, so a dropped second invalidation can't overwriterefreshSettledwith a resolved no-op.onChangetiming is unchanged andgetIndex()stays off the patch queue (no head-of-line blocking), soChangeDetectionServicebehavior is preserved.onChangeis documented as an edge-triggered "recompute soon" coalesce signal (not a settled read);whenSettled()as a point-in-time barrier (not a freeze — alookup()taken after furtherawaits may observe a newer, still non-mid-patch graph). This replaces an over-stated "non-mid-patch consistent" guarantee.AGENTS.md: dropped a cross-file line reference and a vitest-mock implementation transcript, fixed a typo.A new concurrency test asserts
whenSettled()blocks until an in-flight reconciliation settles and that the subsequentlookup()is post-reconciliation. A heavier consumer-side convenience (lookupSettled()) was intentionally not added — no caller exists yet; it belongs with the docgen PR (#34998) that would consume it.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
No manual testing is necessary. This is a behavior-preserving internal refactor of code that is gated behind the experimental
features.changeDetectionflag, with no user-facing or public-API change. Correctness is verified by:ChangeDetectionServicetest suite passing unchanged — it exercises the composed behavior through the same public surface, so it acts as a regression gate proving the extraction preserves behavior;StoryDependencyGraphServiceunit suite covering the extracted graph contract directly (build/lookup, patch serialization, thewhenSettledbarrier including the in-flight-reconciliation case, buffering during build, index-invalidation replay, and build/startup-failure + disposal paths).All 142 change-detection tests pass,
yarn nx run core:checkis clean (0 type errors), and eslint/oxfmt pass.Documentation
MIGRATION.MD
N/A — internal refactor; no documented or public API is affected.
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:
cleanupAvailable 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-35009-sha-8c89ad44. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35009-sha-8c89ad44 sandboxor in an existing project withnpx storybook@0.0.0-pr-35009-sha-8c89ad44 upgrade.More information
0.0.0-pr-35009-sha-8c89ad44valentin/extract-story-dependency-graph-service8c89ad441780401055)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=35009🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Tests