Skip to content

Core: Add ChangeDetectionService and wire up builder-vite#34369

Open
ghengeveld wants to merge 2 commits intomodule-graph-change-listenerfrom
change-detection-service
Open

Core: Add ChangeDetectionService and wire up builder-vite#34369
ghengeveld wants to merge 2 commits intomodule-graph-change-listenerfrom
change-detection-service

Conversation

@ghengeveld
Copy link
Copy Markdown
Member

@ghengeveld ghengeveld commented Mar 27, 2026

Closes #34253

What I did

Implement module-graph change detection for affected stories.

Track changed files from git, trace affected story files through the builder module graph, and surface those results through the dev server status store.

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

Manual testing

Caution

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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

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

More information
Published version 0.0.0-pr-34369-sha-40f39558
Triggered by @ghengeveld
Repository storybookjs/storybook
Branch change-detection-service
Commit 40f39558
Datetime Fri Mar 27 14:06:01 UTC 2026 (1774620361)
Workflow run 23650123796

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=34369

Summary by CodeRabbit

  • New Features

    • Introduced change detection service to track modified and affected stories based on git changes and module dependencies.
    • Added readiness API to check change detection availability and status.
    • Integrated change detection with Vite builder module graph monitoring.
  • Bug Fixes

    • Updated TypeScript SDK reference path in development environment configuration.
  • Tests

    • Added comprehensive test coverage for change detection service, git diff provider, and module graph tracing.

Track changed files from git, trace affected story files through the builder module graph, and surface those results through the dev server status store.
@ghengeveld ghengeveld self-assigned this Mar 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","build","dependencies"]

🚫

PR is not labeled with one of: ["ci:normal","ci:merged","ci:daily","ci:docs"]

Generated by 🚫 dangerJS against 40f3955

@ghengeveld ghengeveld changed the title Change detection service Core: Add ChangeDetectionService and wire up builder-vite Mar 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Implements a server-side change detection pipeline that uses the builder’s module graph plus git working tree changes to compute per-story “new/modified/affected” statuses and publish them via the dev server status store.

Changes:

  • Add ChangeDetectionService (+ readiness signal) and supporting git diff + module-graph tracing utilities in core-server.
  • Wire the service into storybookDevServer() and export the experimental readiness API.
  • Update @storybook/builder-vite to warm up/poll the module graph and emit graph changes for consumers.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
code/core/src/types/modules/core-common.ts Formatting adjustment for type-only imports.
code/core/src/core-server/index.ts Export change detection readiness + service from core-server.
code/core/src/core-server/dev-server.ts Instantiate/start change detection service and dispose on preview build failure.
code/core/src/core-server/change-detection/trace-changed.ts BFS tracer from changed file → reachable story files with distances.
code/core/src/core-server/change-detection/trace-changed.test.ts Unit tests for BFS tracer behavior (distance, cycles, etc.).
code/core/src/core-server/change-detection/service.ts Core service: git diff + module-graph tracing + status store patching + readiness.
code/core/src/core-server/change-detection/service.test.ts Unit tests for service status classification and patch behavior.
code/core/src/core-server/change-detection/readiness.ts Deferred readiness signal implementation.
code/core/src/core-server/change-detection/index.ts Barrel exports for change detection module.
code/core/src/core-server/change-detection/git-diff-provider.ts git CLI provider for staged/unstaged/untracked file detection.
code/core/src/core-server/change-detection/git-diff-provider.test.ts Unit tests for git diff provider.
code/core/src/core-server/change-detection/errors.ts Typed error classes for unavailable/failure conditions.
code/builders/builder-vite/src/index.ts Add module-graph warmup/polling to reliably emit module graph changes.
code/builders/builder-vite/src/index.test.ts Update tests to account for new warmup/polling behavior and watcher hookup timing.
.vscode/settings.json Point VS Code TS SDK to node_modules/typescript.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +213 to +222
const changedFiles = Array.from(changes.changed).map((filePath) => resolve(repoRoot, filePath));
const newFiles = Array.from(changes.new).map((filePath) => resolve(repoRoot, filePath));

const storyIndex = await storyIndexGenerator.getIndex();
const workingDir = this.getWorkingDir();
const storyIdsByFile = getStoryIdsByAbsolutePath(storyIndex, workingDir);
const statuses = new Map<string, Status>();

for (const changedFile of changedFiles) {
const affectedStoryFiles = findAffectedStoryFiles(changedFile, moduleGraph, storyIdsByFile);
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

buildStatuses() only iterates over changes.changed. In the real GitDiffProvider, untracked files are returned in changes.new but are not included in changes.changed, so newly added (untracked) story files will never be scanned/marked as status-value:new unless they also appear in changed. Consider iterating over the union of changed and new (or just iterate changed ∪ new) so new story files are detected as intended.

Copilot uses AI. Check for mistakes.
Comment on lines +233 to +237
const value = newFiles.includes(storyFile)
? 'status-value:new'
: distance === lowestDistance
? 'status-value:modified'
: 'status-value:affected';
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

newFiles.includes(storyFile) is an O(n) lookup inside a nested loop over affected story files and story IDs. Since this can run on every module graph change, consider storing newFiles as a Set (or keeping changes.new as a Set of normalized absolute paths) to make the membership check O(1).

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +68
async getChangedFiles(): Promise<GitDiffResult> {
const repoRoot = await this.getRepoRoot();

try {
const [staged, unstaged, untracked] = await Promise.all([
execa('git', ['diff', '--name-only', '--diff-filter=d', '--cached'], {
cwd: repoRoot,
stdio: 'pipe',
}),
execa('git', ['diff', '--name-only', '--diff-filter=d'], {
cwd: repoRoot,
stdio: 'pipe',
}),
execa('git', ['ls-files', '--others', '--exclude-standard'], {
cwd: repoRoot,
stdio: 'pipe',
}),
]);

return {
changed: new Set([
...parseChangedFiles(staged.stdout),
...parseChangedFiles(unstaged.stdout),
]),
new: parseChangedFiles(untracked.stdout),
};
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

GitDiffProvider.getChangedFiles() only populates new from untracked files. Newly added tracked files (e.g. git add <file>) will show up in the staged diff but currently end up in changed, meaning they’ll be treated as modified/affected rather than status-value:new. Consider adding a staged-added query (e.g. git diff --name-only --cached --diff-filter=A) and unioning that with the untracked set into new.

Copilot uses AI. Check for mistakes.
}, 100);
};
// We intentionally don't await this. Cleanup happens in bail().
startChangeDetection(options);
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

startChangeDetection(options) is intentionally not awaited, but it contains awaited work (warmupRequest, getIndex, etc.). If any of that rejects, it will surface as an unhandled promise rejection (and the polling interval may never be cleared). Consider attaching a .catch(...) handler (or wrapping in void startChangeDetection(options).catch(...)) to log/debug and ensure cleanup.

Suggested change
startChangeDetection(options);
void startChangeDetection(options).catch((error) => {
// Ensure errors in change detection don't become unhandled rejections and that we clean up.
console.error('[builder-vite] startChangeDetection failed:', error);
void bail().catch((bailError) => {
console.error('[builder-vite] bail failed after startChangeDetection error:', bailError);
});
});

Copilot uses AI. Check for mistakes.
Comment on lines +113 to +123
if (listeners.size > 0) {
// Debounce handler to prevent multiple callback invocations when multiple files are edited
watcherChangeHandler = () => {
clearTimeout(debounce);
debounce = setTimeout(() => {
notifyListeners(buildModuleGraph(server.moduleGraph.fileToModulesMap));
}, 100);
};
// We intentionally don't await this. Cleanup happens in bail().
startChangeDetection(options);
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

start() only initializes watcherChangeHandler / module-graph polling when listeners.size > 0 at startup. If a consumer calls onModuleGraphChange() after the server has started, it will register into listeners but never receive events because the watcher/polling were never set up. Either document/enforce “subscribe before start” or initialize the watcher lazily when the first listener is added.

Copilot uses AI. Check for mistakes.
Comment on lines +241 to +243
const changedStoryFiles = new Set<string>(existingStatus?.data?.changedFiles ?? []);
changedStoryFiles.add(changedFile.replace(`${repoRoot}/`, ''));

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

changedFiles path trimming uses changedFile.replace(${repoRoot}/, ''), which is not robust on Windows (path separators) and can also leave absolute paths if the prefix doesn't match exactly. Prefer computing path.relative(repoRoot, changedFile) (and normalizing separators if needed) to ensure data.changedFiles is consistently repo-relative across platforms.

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +30
while (queue.length > 0) {
const current = queue.shift();
if (!current) {
continue;
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

The BFS queue uses queue.shift() in a loop, which is O(n) per pop due to array reindexing. For large module graphs this can add noticeable overhead during scans. Consider using an index-based queue (e.g. let i=0; while (i<queue.length) { const current = queue[i++]; ... }) or a small deque implementation.

Suggested change
while (queue.length > 0) {
const current = queue.shift();
if (!current) {
continue;
}
let index = 0;
while (index < queue.length) {
const current = queue[index++];

Copilot uses AI. Check for mistakes.
Comment on lines +161 to +183
it('marks new story files as new and unsets them after they are reverted', async () => {
const storyIndex = createStoryIndex([
{
storyId: 'new-button--primary',
importPath: './src/NewButton.stories.tsx',
title: 'NewButton',
},
]);
const { getStatusStoreByTypeId } = createStatusStore({
universalStatusStore: new MockUniversalStore(UNIVERSAL_STATUS_STORE_OPTIONS),
environment: 'server',
});
const gitDiffProvider = {
getChangedFiles: vi
.fn()
.mockResolvedValueOnce({
changed: new Set(['src/NewButton.stories.tsx']),
new: new Set(['src/NewButton.stories.tsx']),
})
.mockResolvedValueOnce({
changed: new Set(),
new: new Set(),
}),
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

Current tests don’t cover the real-world case where an untracked file appears only in the new set (and not in changed). Given ChangeDetectionService.buildStatuses() currently iterates changed, adding a test that sets changed: new Set() and new: new Set(['src/NewButton.stories.tsx']) would prevent regressions once the logic is fixed.

Copilot uses AI. Check for mistakes.
new: new Set(['src/NewButton.stories.tsx']),
});
});

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

Tests currently validate unioning staged+unstaged into changed and untracked into new, but don’t cover newly added tracked files (staged additions) being classified into the new set. Once GitDiffProvider is updated to query --diff-filter=A, add a unit test that mocks that call and asserts staged-added files are included in new.

Suggested change
it('includes staged-added files in the new set', async () => {
vi.mocked(execa)
.mockResolvedValueOnce({ stdout: '/repo' } as never)
// Staged files (includes both a modified and an added file)
.mockResolvedValueOnce({
stdout: 'src/ExistingComponent.tsx\nsrc/NewComponent.tsx\n',
} as never)
// Unstaged files
.mockResolvedValueOnce({
stdout: 'src/ExistingComponent.tsx\n',
} as never)
// Untracked files
.mockResolvedValueOnce({
stdout: 'src/UntrackedHelper.ts\n',
} as never)
// Staged-added files from --diff-filter=A
.mockResolvedValueOnce({
stdout: 'src/NewComponent.tsx\n',
} as never);
const provider = new GitDiffProvider('/repo');
await expect(provider.getChangedFiles()).resolves.toEqual({
changed: new Set([
'src/ExistingComponent.tsx',
'src/NewComponent.tsx',
]),
new: new Set([
'src/UntrackedHelper.ts',
'src/NewComponent.tsx',
]),
});
});

Copilot uses AI. Check for mistakes.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

Introduces a git-based change detection system for Storybook that monitors module graph changes, identifies modified and new story files, and manages readiness state. Integrates a new ChangeDetectionService with the dev-server, gated by a feature flag. Updates builder-vite polling logic and VS Code TypeScript configuration.

Changes

Cohort / File(s) Summary
Configuration
.vscode/settings.json
Updated VS Code TypeScript service to use ./node_modules/typescript/lib instead of ./typescript/lib.
Builder-Vite Module Graph Handling
code/builders/builder-vite/src/index.ts, code/builders/builder-vite/src/index.test.ts
Refactored polling and change detection initialization to conditionally start file-change detection only when listeners are registered; added startChangeDetection() helper for async module graph warmup and idle wait; updated tests with new createStartArgs() pattern and startChangeDetection() async helper for consistent test setup.
Change Detection Errors
code/core/src/core-server/change-detection/errors.ts
Added two new error classes: ChangeDetectionUnavailableError and ChangeDetectionFailureError for domain-specific error handling.
Git-Based File Change Detection
code/core/src/core-server/change-detection/git-diff-provider.ts, code/core/src/core-server/change-detection/git-diff-provider.test.ts
Implemented GitDiffProvider that executes git commands to compute staged, unstaged, and untracked file changes; converts git errors to domain errors; test suite verifies file aggregation and error handling.
Change Detection Readiness State
code/core/src/core-server/change-detection/readiness.ts
Added readiness state management with observable types ('ready', 'unavailable', 'error') and deferred-promise mechanism for lifecycle signaling via getChangeDetectionReadiness(), setChangeDetectionReadiness(), and resetChangeDetectionReadiness().
Module Graph Traversal
code/core/src/core-server/change-detection/trace-changed.ts, code/core/src/core-server/change-detection/trace-changed.test.ts
Implemented findAffectedStoryFiles() that performs breadth-first traversal of module graph to compute minimum distances from changed modules to reachable story files; test suite verifies distance computation, cycle handling, and edge cases.
Change Detection Service
code/core/src/core-server/change-detection/service.ts, code/core/src/core-server/change-detection/service.test.ts
Introduced ChangeDetectionService that listens to module graph changes, scans git diffs, traces affected stories, and updates status store with new/modified/affected markers; manages readiness state and debounces concurrent scans; comprehensive test suite covers modified/affected story detection, new file handling, error scenarios, and precedence rules.
Change Detection Public API
code/core/src/core-server/change-detection/index.ts, code/core/src/core-server/index.ts
Created barrel module exporting error classes, GitDiffProvider, readiness helpers, and service; re-exported public API from core-server index with experimental naming convention for readiness types/functions.
Dev-Server Integration
code/core/src/core-server/dev-server.ts
Initialized and wired ChangeDetectionService to builder's onModuleGraphChange, gated by features?.changeDetection; added service disposal on preview build failure; moved features preset application earlier in startup sequence.
Type System Updates
code/core/src/types/modules/core-common.ts
Changed imports of StoryIndexGenerator and CsfFile to type-only import syntax.

Sequence Diagram(s)

sequenceDiagram
    participant Builder
    participant ChangeDetectionService
    participant GitDiffProvider
    participant ModuleGraph
    participant StatusStore

    Builder->>ChangeDetectionService: onModuleGraphChange event
    activate ChangeDetectionService
    ChangeDetectionService->>ChangeDetectionService: Debounce scan (200ms)
    
    ChangeDetectionService->>GitDiffProvider: getChangedFiles()
    activate GitDiffProvider
    GitDiffProvider->>GitDiffProvider: Execute git diff commands
    GitDiffProvider-->>ChangeDetectionService: GitDiffResult {changed, new}
    deactivate GitDiffProvider
    
    ChangeDetectionService->>ModuleGraph: Retrieve changed modules
    activate ModuleGraph
    ModuleGraph-->>ChangeDetectionService: ModuleNode[] for changed files
    deactivate ModuleGraph
    
    ChangeDetectionService->>ChangeDetectionService: Traverse module graph via findAffectedStoryFiles()
    ChangeDetectionService->>ChangeDetectionService: Compute story statuses (new/modified/affected)
    
    ChangeDetectionService->>StatusStore: Patch story change-detection statuses
    activate StatusStore
    StatusStore-->>ChangeDetectionService: Status update complete
    deactivate StatusStore
    
    ChangeDetectionService->>ChangeDetectionService: setChangeDetectionReadiness('ready')
    deactivate ChangeDetectionService
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 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/git-diff-provider.ts (1)

43-72: Consider more specific error attribution for parallel git commands.

When the Promise.all rejects, the error is attributed to 'git diff' (line 70), but the failure could originate from any of the three commands including git ls-files. This could make debugging slightly harder.

Consider capturing command context or using Promise.allSettled with individual error handling if precise attribution is needed, though this is a minor concern since all commands run in the same repo context.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/core/src/core-server/change-detection/git-diff-provider.ts` around lines
43 - 72, The Promise.all in getChangedFiles currently maps any rejection to a
generic 'git diff' error via toGitError, which loses which of the three execa
calls failed; update getChangedFiles to attribute errors to the specific command
that failed (e.g., the first execa for staged diff, second for unstaged diff, or
the execa('git', ['ls-files'...]) for untracked) by either using
Promise.allSettled and checking each result for .status === 'rejected' and
calling toGitError with a descriptive command name, or by wrapping each execa
call in its own try/catch that calls this.toGitError(error, '<specific git
command>') before rethrowing so the thrown error contains the precise command
context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/builders/builder-vite/src/index.ts`:
- Around line 121-122: The fire-and-forget call to startChangeDetection(options)
can reject during preset resolution/index generation/warmup and leave change
detection half-initialized; make this explicit and observe rejections by calling
it as void startChangeDetection(options).catch(err => { /* handle/log */ });:
update the call site to prefix with void and attach a .catch that logs the error
(use the existing logger in this module or console.error) and perform any
minimal cleanup or state marking so bail() or shutdown sees the failure,
referencing startChangeDetection and bail to ensure the cleanup path remains
correct.
- Around line 88-98: The code in waitForModuleGraph registers and calls
watcherChangeHandler after awaiting server.waitForRequestsIdle(), but bail() may
clear watcherChangeHandler during that await causing server.watcher.on('all',
watcherChangeHandler) or watcherChangeHandler() to be invoked as undefined;
update the block in waitForModuleGraph to re-check that watcherChangeHandler is
still defined after the await (i.e., after server.waitForRequestsIdle()) before
calling server.watcher.on('all', watcherChangeHandler) and before invoking
watcherChangeHandler(), and bail() should continue to be allowed to clear
watcherChangeHandler as before.

In `@code/core/src/core-server/change-detection/git-diff-provider.test.ts`:
- Around line 20-38: Move the vi.mocked(execa) setup into a beforeEach and split
the "staged-new" and "untracked-new" fixtures so the same path isn't returned by
both staged and untracked commands; specifically, in beforeEach mock the
sequence of execa calls that GitDiffProvider('/repo') expects (repo root, staged
list, unstaged list, untracked list) and ensure the staged list contains the
"new" file (e.g. 'src/NewButton.stories.tsx') while the untracked list contains
a different file (e.g. 'src/Button.css'), then remove any inline
vi.mocked(execa) calls from individual it blocks so tests call
GitDiffProvider.getChangedFiles() against the shared beforeEach mocks.

---

Nitpick comments:
In `@code/core/src/core-server/change-detection/git-diff-provider.ts`:
- Around line 43-72: The Promise.all in getChangedFiles currently maps any
rejection to a generic 'git diff' error via toGitError, which loses which of the
three execa calls failed; update getChangedFiles to attribute errors to the
specific command that failed (e.g., the first execa for staged diff, second for
unstaged diff, or the execa('git', ['ls-files'...]) for untracked) by either
using Promise.allSettled and checking each result for .status === 'rejected' and
calling toGitError with a descriptive command name, or by wrapping each execa
call in its own try/catch that calls this.toGitError(error, '<specific git
command>') before rethrowing so the thrown error contains the precise command
context.
🪄 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: cfc75601-eb26-4b8f-8040-78cbf224f12c

📥 Commits

Reviewing files that changed from the base of the PR and between af5da75 and 40f3955.

📒 Files selected for processing (15)
  • .vscode/settings.json
  • code/builders/builder-vite/src/index.test.ts
  • code/builders/builder-vite/src/index.ts
  • code/core/src/core-server/change-detection/errors.ts
  • code/core/src/core-server/change-detection/git-diff-provider.test.ts
  • code/core/src/core-server/change-detection/git-diff-provider.ts
  • code/core/src/core-server/change-detection/index.ts
  • code/core/src/core-server/change-detection/readiness.ts
  • code/core/src/core-server/change-detection/service.test.ts
  • code/core/src/core-server/change-detection/service.ts
  • code/core/src/core-server/change-detection/trace-changed.test.ts
  • code/core/src/core-server/change-detection/trace-changed.ts
  • code/core/src/core-server/dev-server.ts
  • code/core/src/core-server/index.ts
  • code/core/src/types/modules/core-common.ts

Comment on lines +88 to +98
waitForModuleGraph = setInterval(async () => {
if (!watcherChangeHandler || process.hrtime(startTime)[0] > 30) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
} else if (server.moduleGraph.fileToModulesMap.size > 0) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
await server.waitForRequestsIdle();
server.watcher.on('all', watcherChangeHandler);
watcherChangeHandler();
}
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 | 🔴 Critical

Re-check teardown state after waitForRequestsIdle().

bail() can run while Line 95 is awaiting and clear watcherChangeHandler. Lines 96-97 then register and invoke undefined, which turns teardown into an unhandled rejection.

Possible fix
   } else if (server.moduleGraph.fileToModulesMap.size > 0) {
     clearInterval(waitForModuleGraph);
     waitForModuleGraph = undefined;
     await server.waitForRequestsIdle();
-    server.watcher.on('all', watcherChangeHandler);
-    watcherChangeHandler();
+    const handler = watcherChangeHandler;
+    if (!handler) {
+      return;
+    }
+    server.watcher.on('all', handler);
+    handler();
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
waitForModuleGraph = setInterval(async () => {
if (!watcherChangeHandler || process.hrtime(startTime)[0] > 30) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
} else if (server.moduleGraph.fileToModulesMap.size > 0) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
await server.waitForRequestsIdle();
server.watcher.on('all', watcherChangeHandler);
watcherChangeHandler();
}
waitForModuleGraph = setInterval(async () => {
if (!watcherChangeHandler || process.hrtime(startTime)[0] > 30) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
} else if (server.moduleGraph.fileToModulesMap.size > 0) {
clearInterval(waitForModuleGraph);
waitForModuleGraph = undefined;
await server.waitForRequestsIdle();
const handler = watcherChangeHandler;
if (!handler) {
return;
}
server.watcher.on('all', handler);
handler();
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/builders/builder-vite/src/index.ts` around lines 88 - 98, The code in
waitForModuleGraph registers and calls watcherChangeHandler after awaiting
server.waitForRequestsIdle(), but bail() may clear watcherChangeHandler during
that await causing server.watcher.on('all', watcherChangeHandler) or
watcherChangeHandler() to be invoked as undefined; update the block in
waitForModuleGraph to re-check that watcherChangeHandler is still defined after
the await (i.e., after server.waitForRequestsIdle()) before calling
server.watcher.on('all', watcherChangeHandler) and before invoking
watcherChangeHandler(), and bail() should continue to be allowed to clear
watcherChangeHandler as before.

Comment on lines +121 to +122
// We intentionally don't await this. Cleanup happens in bail().
startChangeDetection(options);
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

Handle rejected pre-poll initialization.

Line 122 fire-and-forgets startChangeDetection(), but that function still awaits preset resolution, index generation, and all warmupRequest() calls before it even installs the poll. Any rejection there is currently unobserved and leaves change detection half-initialized. If this is meant to be best-effort, make that explicit with void startChangeDetection(options).catch(...).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/builders/builder-vite/src/index.ts` around lines 121 - 122, The
fire-and-forget call to startChangeDetection(options) can reject during preset
resolution/index generation/warmup and leave change detection half-initialized;
make this explicit and observe rejections by calling it as void
startChangeDetection(options).catch(err => { /* handle/log */ });: update the
call site to prefix with void and attach a .catch that logs the error (use the
existing logger in this module or console.error) and perform any minimal cleanup
or state marking so bail() or shutdown sees the failure, referencing
startChangeDetection and bail to ensure the cleanup path remains correct.

Comment on lines +20 to +38
it('returns the union of staged, unstaged, and untracked files', async () => {
vi.mocked(execa)
.mockResolvedValueOnce({ stdout: '/repo' } as never)
.mockResolvedValueOnce({
stdout: 'src/Button.tsx\nsrc/NewButton.stories.tsx\n',
} as never)
.mockResolvedValueOnce({ stdout: 'src/Button.tsx\nsrc/Button.css\n' } as never)
.mockResolvedValueOnce({ stdout: 'src/NewButton.stories.tsx\n' } as never);

const provider = new GitDiffProvider('/repo');

await expect(provider.getChangedFiles()).resolves.toEqual({
changed: new Set(['src/Button.tsx', 'src/NewButton.stories.tsx', 'src/Button.css']),
new: new Set(['src/NewButton.stories.tsx']),
});
});

it('throws a typed unavailable error when git cannot find a repository', async () => {
vi.mocked(execa).mockRejectedValueOnce(new Error('fatal: not a git repository'));
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

Split the staged-new and untracked-new fixtures.

Line 24 and Line 27 feed the same path through both mocked branches, so this suite never proves those code paths independently. A regression that drops staged-new files from the new set can still pass as long as the untracked branch returns that file. While splitting those cases, please also move the execa behavior into beforeEach instead of configuring it inline inside each test body.

As per coding guidelines, "Implement mock behaviors in beforeEach blocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/core/src/core-server/change-detection/git-diff-provider.test.ts` around
lines 20 - 38, Move the vi.mocked(execa) setup into a beforeEach and split the
"staged-new" and "untracked-new" fixtures so the same path isn't returned by
both staged and untracked commands; specifically, in beforeEach mock the
sequence of execa calls that GitDiffProvider('/repo') expects (repo root, staged
list, unstaged list, untracked list) and ensure the staged list contains the
"new" file (e.g. 'src/NewButton.stories.tsx') while the untracked list contains
a different file (e.g. 'src/Button.css'), then remove any inline
vi.mocked(execa) calls from individual it blocks so tests call
GitDiffProvider.getChangedFiles() against the shared beforeEach mocks.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants