Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,96 @@ describe('ChangeDetectionService', () => {
await service.dispose();
});

it('calls onInvalidate with the affected story files when a dependency changes', async () => {
// Button.tsx is imported by Button.stories.tsx (distance 1).
const reverseIndex = buildReverseIndex([
['/repo/src/Button.tsx', '/repo/src/Button.stories.tsx', 1],
['/repo/src/Button.stories.tsx', '/repo/src/Button.stories.tsx', 0],
]);
const { buildSpy } = installDependencyGraphMocks(reverseIndex);
buildSpy.mockResolvedValue({ reverseIndex, graph: new Map() });

const storyIndex = createStoryIndex([
{ storyId: 'button--primary', importPath: './src/Button.stories.tsx', title: 'Button' },
]);
const { getStatusStoreByTypeId } = createStatusStore({
universalStatusStore: new MockUniversalStore(UNIVERSAL_STATUS_STORE_OPTIONS),
environment: 'server',
});
const onInvalidate = vi.fn();
const { adapter, emitFileChange } = createMockAdapter();
const service = new ChangeDetectionService({
storyIndexGeneratorPromise: Promise.resolve({
getIndex: vi.fn().mockResolvedValue(storyIndex),
} as never),
statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID),
gitDiffProvider: createMockGitDiffProvider(),
indexBaselineService: createMockStoryIndexBaselineService(),
workingDir,
onInvalidate,
});

service.start(adapter, true);
await vi.runAllTimersAsync();

emitFileChange({ kind: 'change', path: '/repo/src/Button.tsx' });
await vi.runAllTimersAsync();

expect(onInvalidate).toHaveBeenCalledWith(['/repo/src/Button.stories.tsx']);

await service.dispose();
});

it('graph-only mode (publishStatuses:false) emits invalidations without publishing statuses or calling git', async () => {
const reverseIndex = buildReverseIndex([
['/repo/src/Button.tsx', '/repo/src/Button.stories.tsx', 1],
]);
const { buildSpy } = installDependencyGraphMocks(reverseIndex);
buildSpy.mockResolvedValue({ reverseIndex, graph: new Map() });

const storyIndex = createStoryIndex([
{ storyId: 'button--primary', importPath: './src/Button.stories.tsx', title: 'Button' },
]);
const { getStatusStoreByTypeId } = createStatusStore({
universalStatusStore: new MockUniversalStore(UNIVERSAL_STATUS_STORE_OPTIONS),
environment: 'server',
});
const gitDiffProvider = createMockGitDiffProvider((provider) => {
provider.getChangedFilesMock.mockResolvedValue({
changed: new Set(['src/Button.stories.tsx']),
new: new Set(),
});
});
const onInvalidate = vi.fn();
const { adapter, emitFileChange } = createMockAdapter();
const service = new ChangeDetectionService({
storyIndexGeneratorPromise: Promise.resolve({
getIndex: vi.fn().mockResolvedValue(storyIndex),
} as never),
statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID),
gitDiffProvider,
indexBaselineService: createMockStoryIndexBaselineService(),
workingDir,
publishStatuses: false,
onInvalidate,
});

service.start(adapter, true);
await vi.runAllTimersAsync();

emitFileChange({ kind: 'change', path: '/repo/src/Button.tsx' });
await vi.runAllTimersAsync();

// Invalidations still fire, but no statuses are published and git is never consulted.
expect(onInvalidate).toHaveBeenCalledWith(['/repo/src/Button.stories.tsx']);
expect(getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID).getAll()).toEqual({});
expect(gitDiffProvider.getChangedFilesMock).not.toHaveBeenCalled();
expect(gitDiffProvider.onGitStateChangeMock).not.toHaveBeenCalled();
expect(await getChangeDetectionReadiness()).toEqual({ status: 'ready' });

await service.dispose();
});

it('scan waits for the current patch to settle before reading reverseIndex', async () => {
// Without the patchSnapshot await in scan(), a git-state-change that fires scheduleScan
// while a patch is mid-rewalk (reverseIndex transiently empty) reads the empty index
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ export class ChangeDetectionService {
private indexBaselineService: IndexBaselineService | undefined;
private readonly workingDir: string;
private readonly debounceMs: number;
private readonly publishStatuses: boolean;
private readonly onInvalidate?: (affectedStoryFiles: string[]) => void;
private adapter: ChangeDetectionAdapter | undefined;
private dependencyGraphBuilder: DependencyGraphBuilder | undefined;
private incrementalPatcher: IncrementalPatcher | undefined;
Expand Down Expand Up @@ -181,12 +183,27 @@ export class ChangeDetectionService {
debounceMs?: number;
/** Presets instance used to resolve `experimental_importParsers` contributions from framework/renderer plugins. */
presets?: Presets;
/**
* When `false`, run the dependency graph + file watching + {@link onInvalidate} emit only,
* without computing or publishing git-diff statuses to the status store. Lets the graph power
* other consumers (e.g. the docgen service) without surfacing "review changes" sidebar
* statuses. Defaults to `true` (the existing full change-detection behavior).
*/
publishStatuses?: boolean;
/**
* Called after each file-change batch with the absolute, normalized story files affected by
* the change (the changed file's reverse-index dependents, plus the file itself when it is a
* story). Independent of {@link publishStatuses}; fires in both modes.
*/
onInvalidate?: (affectedStoryFiles: string[]) => void;
}
) {
this.gitDiffProvider = options.gitDiffProvider;
this.indexBaselineService = options.indexBaselineService;
this.workingDir = options.workingDir ?? process.cwd();
this.debounceMs = options.debounceMs ?? CHANGE_DETECTION_DEBOUNCE_MS;
this.publishStatuses = options.publishStatuses ?? true;
this.onInvalidate = options.onInvalidate;
resetChangeDetectionReadiness();
}

Expand Down Expand Up @@ -345,20 +362,25 @@ export class ChangeDetectionService {
});
}

void this.getIndexBaselineService().start();
// Baseline + git-state subscriptions are status-only concerns. In graph-only mode (docgen)
// we skip them so the graph never depends on git and never publishes statuses.
if (this.publishStatuses) {
void this.getIndexBaselineService().start();

this.getGitDiffProvider().onGitStateChange(() => {
if (this.disposed) {
return;
}
this.getGitDiffProvider().onGitStateChange(() => {
if (this.disposed) {
return;
}

this.scheduleScan(this.debounceMs);
void this.getIndexBaselineService()
.handleGitStateChange()
.catch(() => undefined);
});
this.scheduleScan(this.debounceMs);
void this.getIndexBaselineService()
.handleGitStateChange()
.catch(() => undefined);
});
}

// Initial scan surfaces git-pending diffs immediately.
// Initial scan surfaces git-pending diffs immediately (and resolves readiness in graph-only
// mode).
this.scheduleScan(0);
}

Expand Down Expand Up @@ -514,6 +536,22 @@ export class ChangeDetectionService {
if (this.disposed || !this.incrementalPatcher) {
return;
}

const path = normalize(event.path);
const affectedStoryFiles = new Set<string>();
const collectAffected = () => {
if (!this.reverseIndex) {
return;
}
for (const story of this.reverseIndex.lookup(path).keys()) {
affectedStoryFiles.add(story);
}
};
// Snapshot dependents before the patch so `unlink` (which prunes the reverse index) is still
// captured. The patcher may also early-return on a content-only edit with an unchanged import
// set, but the existing edges still resolve here.
collectAffected();

try {
await this.incrementalPatcher.patch(event);
} catch (error) {
Expand All @@ -524,6 +562,17 @@ export class ChangeDetectionService {
if (this.disposed) {
return;
}

// Snapshot again to capture dependents introduced by `add`/`change`, then include the file
// itself when it is a story root.
collectAffected();
if (this.storyFiles.has(path)) {
affectedStoryFiles.add(path);
}
if (this.onInvalidate && affectedStoryFiles.size > 0) {
this.onInvalidate([...affectedStoryFiles]);
}

this.scheduleScan(this.debounceMs);
Comment on lines +572 to 576

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

Guard onInvalidate so callback failures cannot skip the rescan.

If onInvalidate throws synchronously here, handleFileChange() exits before Line 576. The surrounding patchQueue.catch(() => undefined) then swallows the rejection, so that file change is neither rescanned nor logged. Wrap the callback in its own try/catch and keep scheduleScan() in a finally so invalidation stays best-effort.

Suggested fix
-    if (this.onInvalidate && affectedStoryFiles.size > 0) {
-      this.onInvalidate([...affectedStoryFiles]);
-    }
-
-    this.scheduleScan(this.debounceMs);
+    try {
+      if (this.onInvalidate && affectedStoryFiles.size > 0) {
+        this.onInvalidate([...affectedStoryFiles]);
+      }
+    } catch (error) {
+      logger.warn(
+        `Change detection: onInvalidate failed for ${path}: ${error instanceof Error ? error.message : String(error)}`
+      );
+    } finally {
+      this.scheduleScan(this.debounceMs);
+    }
🤖 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 572 - 576, The onInvalidate callback invocation can throw synchronously
and prevent scheduleScan from running, so wrap the call to
this.onInvalidate([...affectedStoryFiles]) in a try/catch and ensure
this.scheduleScan(this.debounceMs) runs in a finally block; specifically update
the block that checks if (this.onInvalidate && affectedStoryFiles.size > 0)
inside handleFileChange to catch any error from this.onInvalidate (log or ignore
the error) and then always call this.scheduleScan(this.debounceMs) in finally so
the rescan remains best-effort even if onInvalidate fails.

}

Expand Down Expand Up @@ -561,6 +610,13 @@ export class ChangeDetectionService {
this.scanInFlight = true;

try {
// Graph-only mode: the dependency graph + invalidation emit are the only work; skip all
// git-diff status computation and publishing.
if (!this.publishStatuses) {
this.resolveReadiness({ status: 'ready' });
return;
}

const nextStatuses = await this.buildStatuses(this.reverseIndex);
if (this.disposed) {
return;
Expand Down
46 changes: 44 additions & 2 deletions code/core/src/core-server/dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type { Options } from 'storybook/internal/types';
import compression from '@polka/compression';
import polka from 'polka';

import { getService } from '../shared/open-service/server.ts';
import type { moduleGraphServiceDef } from '../shared/open-service/services/module-graph/definition.ts';
import { isTelemetryModuleEnabled, telemetry } from '../telemetry/index.ts';
import type { ChangeDetectionAdapter } from './change-detection/index.ts';
import { ChangeDetectionService } from './change-detection/index.ts';
Expand Down Expand Up @@ -48,11 +50,47 @@ export async function storybookDevServer(
const storyIndexGeneratorPromise =
options.presets.apply<StoryIndexGenerator>('storyIndexGenerator');

// The docgen service (registered in the `services` preset) needs the change-detection dependency
// graph to know which components a file change affects, even when the change-detection *status*
// feature is off — so we run the graph (without publishing statuses) whenever docgen is enabled.
const experimentalDocgenServer = !!features?.experimentalDocgenServer && !options.ignorePreview;

/**
* Feeds affected story files into the `core/module-graph` service.
*
* This is the one imperative edge from the (non-open-service) change detector into the service
* world: it records which components changed into module-graph state. The docgen service reacts
* to that state change through an open-service subscription wired in the `services` preset — the
* dev server intentionally knows nothing about docgen here. Best-effort: failures are logged at
* debug and never break the watcher.
*/
async function recordAffectedStoryFiles(affectedStoryFiles: string[]): Promise<void> {
if (affectedStoryFiles.length === 0) {
return;
}
try {
const moduleGraph = getService<typeof moduleGraphServiceDef>('core/module-graph');
await moduleGraph.commands.resolveAffectedComponents({ storyFiles: affectedStoryFiles });
} catch (error) {
logger.debug(
`Module-graph invalidation skipped: ${error instanceof Error ? error.message : String(error)}`
);
}
}

const changeDetectionService = new ChangeDetectionService({
storyIndexGeneratorPromise,
statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID),
workingDir,
presets: options.presets,
// Only publish "review changes" sidebar statuses when the change-detection feature is on.
publishStatuses: !!features.changeDetection,
// When docgen is enabled, feed file changes into the module-graph service; docgen reacts to it.
onInvalidate: experimentalDocgenServer
? (affectedStoryFiles) => {
void recordAffectedStoryFiles(affectedStoryFiles);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
: undefined,
});

app.use(compression({ level: 1 }));
Expand Down Expand Up @@ -124,7 +162,11 @@ export async function storybookDevServer(
await Promise.resolve();

if (!options.ignorePreview) {
if (!features.changeDetection) {
// Run the dependency graph when either the change-detection status feature or the docgen
// service needs it. `publishStatuses` (set above) decides whether statuses are surfaced.
const needsChangeDetectionGraph = !!features.changeDetection || experimentalDocgenServer;

if (!needsChangeDetectionGraph) {
changeDetectionService.start(undefined, false);
}

Expand Down Expand Up @@ -153,7 +195,7 @@ export async function storybookDevServer(
throw e;
});

if (features.changeDetection) {
if (needsChangeDetectionGraph) {
let adapter: ChangeDetectionAdapter | undefined;
try {
adapter = previewBuilder.changeDetectionAdapter?.();
Expand Down
21 changes: 19 additions & 2 deletions code/core/src/core-server/presets/common-preset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ import type {
StorybookConfigRaw,
} from 'storybook/internal/types';

import { registerDocgenService } from '../../shared/open-service/services/docgen/server.ts';
import {
connectDocgenToModuleGraph,
registerDocgenService,
} from '../../shared/open-service/services/docgen/server.ts';
import { registerModuleGraphService } from '../../shared/open-service/services/module-graph/server.ts';

import { isAbsolute, join } from 'pathe';
import * as pathe from 'pathe';
Expand Down Expand Up @@ -345,10 +349,23 @@ export const services = async (_value: void, options: Options): Promise<void> =>
async () => undefined
);

registerDocgenService({
// The module-graph service translates change-detection's affected story files into component
// ids. It owns no file watching itself; it is fed by the change-detection graph (the dev server
// calls its `resolveAffectedComponents` command on file changes).
const moduleGraph = registerModuleGraphService({
getIndex: () => generator.getIndex(),
workingDir: process.cwd(),
});

const docgen = registerDocgenService({
getIndex: () => generator.getIndex(),
provider,
});

// Connect the two services with an open-service subscription: docgen re-extracts whenever the
// module graph reports an invalidation. The dev server only feeds the module graph; it never
// talks to docgen directly.
connectDocgenToModuleGraph(docgen, moduleGraph);
}
};

Expand Down
11 changes: 11 additions & 0 deletions code/core/src/shared/open-service/services/docgen/definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import type { DocgenPayload } from './types.ts';
/** Caller-facing input to the `getDocgen` query and the `extractDocgen` command. */
export const docgenInputSchema = v.object({ componentId: v.string() });

/** Input to the `handleSourceChange` command: component ids whose source may have changed. */
export const handleSourceChangeInputSchema = v.object({ componentIds: v.array(v.string()) });

/**
* Phase-1 docgen payload schema.
*
Expand Down Expand Up @@ -73,5 +76,13 @@ export const docgenServiceDef = defineService({
// Handler is supplied at registration time so it can close over the story index and the
// composed experimental_docgenProvider chain.
},
handleSourceChange: {
description:
'Re-extracts docgen for the given componentIds that are already present in state (leaving absent ones for lazy load on next read). Used to keep docgen fresh when source files change.',
input: handleSourceChangeInputSchema,
output: v.void(),
// Handler is supplied at registration time so it can log keep-last-good extraction failures
// without pulling a node-only logger into the environment-agnostic definition.
},
},
});
Loading