Core: Implement Git change detection#34420
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit d4d3de2
☁️ Nx Cloud last updated this comment at |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Git state change detection to trigger change-detection rescans when commits are created or branches are switched, keeping the module graph/statuses in sync with Git.
Changes:
- Add filesystem watchers in
GitDiffProviderforHEAD,packed-refs, and the current branch ref, with a subscription API. - Wire
ChangeDetectionServiceto rescan (debounced) when Git state changes occur. - Extend unit tests to cover watcher behavior and Git-triggered rescans.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
code/core/src/core-server/change-detection/GitDiffProvider.ts |
Implements Git state change subscription + .git watchers to emit change events. |
code/core/src/core-server/change-detection/GitDiffProvider.test.ts |
Adds unit tests validating watcher setup, reconfiguration, reuse, and cleanup. |
code/core/src/core-server/change-detection/ChangeDetectionService.ts |
Subscribes to Git state changes and schedules debounced scans. |
code/core/src/core-server/change-detection/ChangeDetectionService.test.ts |
Updates mocks and adds a test ensuring rescans happen on Git state change with debounce. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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:
📝 WalkthroughWalkthroughAdds filesystem injection and git-state subscriptions: Changes
Sequence Diagram(s)sequenceDiagram
participant Service as ChangeDetectionService
participant Provider as GitDiffProvider
participant FS as FileSystem
participant Subscriber as GitStateSubscriber
Service->>Provider: onGitStateChange(subscriber)
Provider->>Provider: store callback, ensure watchers (lazy)
alt watchers not active
Provider->>FS: readFile(".git/HEAD")
Provider->>FS: stat(".git")
Provider->>FS: watch(".git")
Provider->>FS: watch(resolved refs/heads or branch dir)
end
FS-->>Provider: watch change / error
Provider->>Subscriber: invoke callback (git-state change)
Subscriber->>Service: notify -> schedule debounced rescan
Service->>Provider: getChangedFiles()
Service->>Provider: dispose() -> unsubscribe git-state
Provider->>FS: close watchers (if no subscribers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 1
🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/GitDiffProvider.ts (1)
207-209: Consider supporting Git worktrees.
getGitDir()assumes a standard.gitdirectory structure. For Git worktrees,.gitis a file containing the path to the actual git directory. This may cause the watcher to fail silently for worktree setups.This is likely acceptable for the initial implementation since the feature is opportunistic, but worth documenting as a known limitation.
🤖 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/GitDiffProvider.ts` around lines 207 - 209, getGitDir() currently assumes a literal .git directory and will fail for worktrees where .git is a file that points to the real gitdir; update getGitDir to check if join(await this.getRepoRoot(), '.git') is a file, and if so read its contents, parse the "gitdir: <path>" line, resolve that path (relative to the repo root) and return the actual git directory path; keep the existing behavior when .git is a directory. Reference getGitDir and getRepoRoot when making the change.
🤖 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/core/src/core-server/change-detection/GitDiffProvider.ts`:
- Around line 89-100: The unsubscribe path can race with in-flight watcher setup
triggered by ensureWatchers(); update the logic so clearAllWatchers cannot run
while setup is still creating watchers: add an internal inFlightSetup Promise or
boolean flag set by ensureWatchers()/setupWatchers() and check it before
creating watchers in watchFile (or await it in clearAllWatchers), and on
unsubscribe in onGitStateChange ensure you either await the inFlightSetup to
finish before calling clearAllWatchers or flip a cancelled flag that
setupWatchers/watchFile respects (checking gitStateCallbacks.size === 0 plus the
cancelled flag) to avoid creating orphaned watchers; reference functions:
onGitStateChange, ensureWatchers, setupWatchers, watchFile, clearAllWatchers,
and the gitStateCallbacks collection.
---
Nitpick comments:
In `@code/core/src/core-server/change-detection/GitDiffProvider.ts`:
- Around line 207-209: getGitDir() currently assumes a literal .git directory
and will fail for worktrees where .git is a file that points to the real gitdir;
update getGitDir to check if join(await this.getRepoRoot(), '.git') is a file,
and if so read its contents, parse the "gitdir: <path>" line, resolve that path
(relative to the repo root) and return the actual git directory path; keep the
existing behavior when .git is a directory. Reference getGitDir and getRepoRoot
when making the change.
🪄 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: 21b80900-748c-4482-a260-20a15a41bfc0
📒 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/GitDiffProvider.test.tscode/core/src/core-server/change-detection/GitDiffProvider.ts
ghengeveld
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/GitDiffProvider.test.ts (1)
347-365: Assert the existing watchers are closed in this race test.This only proves that no extra branch watcher was registered after
HEADresolves. It does not prove the already-createdHEADandpacked-refswatchers were torn down whenunsubscribe()ran, so the original leak could still pass here.As per coding guidelines, "Test real behavior, not just syntax patterns".💡 Tighten the assertion
it('does not leave watchers behind when unsubscribing during setup', async () => { const headReadDeferred = createDeferred<void>(); - const { createProvider, getWatchedPaths } = setupGitWatchProvider({ + const { createProvider, getWatchedPaths, watchRecords } = setupGitWatchProvider({ headReadDelay: headReadDeferred.promise, }); const provider = createProvider(); @@ expect(getWatchedPaths()).toEqual(['/repo/.git/HEAD', '/repo/.git/packed-refs']); + watchRecords.forEach(({ watcher }) => { + expect(watcher.close).toHaveBeenCalledTimes(1); + }); });🤖 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/GitDiffProvider.test.ts` around lines 347 - 365, The test currently only checks no new watchers were added after unsubscribe; change it to assert the actual watchers were torn down: after calling unsubscribe() and resolving headReadDeferred.resolve(), wait for the async flushes and then assert getWatchedPaths() is empty (or does not contain '/repo/.git/HEAD' and '/repo/.git/packed-refs'). Update the assertion in the test using the existing helpers (setupGitWatchProvider, getWatchedPaths, headReadDeferred, unsubscribe) so the test verifies the HEAD and packed-refs watchers were closed rather than merely not re-registered.
🤖 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/core/src/core-server/change-detection/GitDiffProvider.ts`:
- Around line 123-130: The current file watches individual ref files (HEAD,
packed-refs, branch ref files) which breaks when Git atomically replaces files
because fs.watch can stay bound to the old inode; update GitDiffProvider to
watch parent directories instead: replace per-file watches in the block that
calls watchFile(generation, 'head', join(gitDir, 'HEAD'), ...), watchFile(...
'packedRefs', join(gitDir, 'packed-refs'), ...) and the branch-watching logic
inside configureBranchWatcher so they watch gitDir (for HEAD/packed-refs) and
dirname(branchRef) (for branch refs) and on any directory event call
emitGitStateChange() and re-read the specific ref; keep the existing fallback
behavior but prefer directory watches to avoid inode staleness while retaining
re-read semantics used by emitGitStateChange and configureBranchWatcher.
---
Nitpick comments:
In `@code/core/src/core-server/change-detection/GitDiffProvider.test.ts`:
- Around line 347-365: The test currently only checks no new watchers were added
after unsubscribe; change it to assert the actual watchers were torn down: after
calling unsubscribe() and resolving headReadDeferred.resolve(), wait for the
async flushes and then assert getWatchedPaths() is empty (or does not contain
'/repo/.git/HEAD' and '/repo/.git/packed-refs'). Update the assertion in the
test using the existing helpers (setupGitWatchProvider, getWatchedPaths,
headReadDeferred, unsubscribe) so the test verifies the HEAD and packed-refs
watchers were closed rather than merely not re-registered.
🪄 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: f9cb4b11-de71-460e-96a0-b7dace69dcfd
📒 Files selected for processing (2)
code/core/src/core-server/change-detection/GitDiffProvider.test.tscode/core/src/core-server/change-detection/GitDiffProvider.ts
…ctor test cases for clarity. Added emitError functionality and adjusted watcher setup logic to prevent indefinite retries. Updated tests to reflect changes in git state watching behavior.
…ction and update test cases for accurate file categorization. Adjusted git command filters to differentiate between staged, unstaged, and newly added files.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
code/core/src/core-server/change-detection/GitDiffProvider.test.ts (2)
393-403: Same fragile pattern for testing bounded retry behavior.Multiple
await Promise.resolve()calls are unnecessary here since the test assertsexecawas called exactly once. A single await followed by the assertion should suffice, or usevi.waitFor()for robustness.♻️ Suggested refactor
it('does not retry watcher setup indefinitely when no watchers can be installed', async () => { repoRootResult = rejected(new Error('fatal: not a git repository')); const provider = new GitDiffProvider('/repo'); provider.onGitStateChange(vi.fn()); - await Promise.resolve(); - await Promise.resolve(); - await Promise.resolve(); + await vi.waitFor(() => { + expect(vi.mocked(execa)).toHaveBeenCalledTimes(1); + }); - expect(vi.mocked(execa)).toHaveBeenCalledTimes(1); });🤖 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/GitDiffProvider.test.ts` around lines 393 - 403, The test "does not retry watcher setup indefinitely when no watchers can be installed" uses multiple unnecessary await Promise.resolve() calls; simplify to a single microtask flush or use vi.waitFor to make the assertion robust: after setting repoRootResult and instantiating GitDiffProvider('/repo') and registering provider.onGitStateChange(vi.fn()), replace the repeated await Promise.resolve() calls with one await Promise.resolve() (or await vi.waitFor(() => expect(vi.mocked(execa)).toHaveBeenCalledTimes(1))) so the mocked execa invocation is reliably observed and the test asserts it was called exactly once; keep references to repoRootResult, GitDiffProvider, provider.onGitStateChange, and vi.mocked(execa) to locate the change.
385-391: Fragile microtask queue flushing with multipleawait Promise.resolve().Using multiple consecutive
await Promise.resolve()to wait for async watcher setup to complete is timing-sensitive and may become flaky. Consider usingvi.waitFor()with an assertion on the expected final state, consistent with other tests in this file.♻️ Suggested refactor
unsubscribe(); headReadDeferred.resolve(); - await Promise.resolve(); - await Promise.resolve(); + await vi.waitFor(() => { + expect(getWatchedPaths()).toEqual(['/repo/.git', '/repo/.git']); + }); - expect(getWatchedPaths()).toEqual(['/repo/.git', '/repo/.git']); });🤖 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/GitDiffProvider.test.ts` around lines 385 - 391, The test is using fragile microtask flushes via two consecutive await Promise.resolve(); instead replace them with a robust retry waiter: after calling unsubscribe() and headReadDeferred.resolve(), use vi.waitFor() (or equivalent) to poll until getWatchedPaths() returns the expected ['/repo/.git', '/repo/.git'] value; update the assertion to be inside or after vi.waitFor() to avoid timing flakes and refer to the existing getWatchedPaths(), unsubscribe(), and headReadDeferred.resolve() symbols when making the change.code/core/src/core-server/change-detection/GitDiffProvider.ts (1)
129-136: Both 'head' and 'packedRefs' watchers target the samegitDirpath.Both calls to
watchFileat lines 129 and 133 watch the samegitDirdirectory (e.g.,/repo/.git), resulting in twofs.watchhandles on the identical path. This works correctly but is inefficient—a single directory watcher with unified callback logic could reduce resource overhead.🤖 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/GitDiffProvider.ts` around lines 129 - 136, The two watchFile calls for 'head' and 'packedRefs' both target the same gitDir and create duplicate fs.watch handles; consolidate into a single watcher for gitDir that triggers the shared logic: call emitGitStateChange() and (for the head case) kick off configureBranchWatcher(gitDir, generation). Update the code around watchFile(...) so it either accepts an array of targets or use one watchFile(generation, ['head','packedRefs'], gitDir, callback) (or create a new single watch on gitDir) and inside the callback inspect which file changed and invoke emitGitStateChange(), and only when the changed file is 'head' call void configureBranchWatcher(gitDir, generation); keep the existing function names watchFile, emitGitStateChange, and configureBranchWatcher to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/change-detection/GitDiffProvider.test.ts`:
- Around line 393-403: The test "does not retry watcher setup indefinitely when
no watchers can be installed" uses multiple unnecessary await Promise.resolve()
calls; simplify to a single microtask flush or use vi.waitFor to make the
assertion robust: after setting repoRootResult and instantiating
GitDiffProvider('/repo') and registering provider.onGitStateChange(vi.fn()),
replace the repeated await Promise.resolve() calls with one await
Promise.resolve() (or await vi.waitFor(() =>
expect(vi.mocked(execa)).toHaveBeenCalledTimes(1))) so the mocked execa
invocation is reliably observed and the test asserts it was called exactly once;
keep references to repoRootResult, GitDiffProvider, provider.onGitStateChange,
and vi.mocked(execa) to locate the change.
- Around line 385-391: The test is using fragile microtask flushes via two
consecutive await Promise.resolve(); instead replace them with a robust retry
waiter: after calling unsubscribe() and headReadDeferred.resolve(), use
vi.waitFor() (or equivalent) to poll until getWatchedPaths() returns the
expected ['/repo/.git', '/repo/.git'] value; update the assertion to be inside
or after vi.waitFor() to avoid timing flakes and refer to the existing
getWatchedPaths(), unsubscribe(), and headReadDeferred.resolve() symbols when
making the change.
In `@code/core/src/core-server/change-detection/GitDiffProvider.ts`:
- Around line 129-136: The two watchFile calls for 'head' and 'packedRefs' both
target the same gitDir and create duplicate fs.watch handles; consolidate into a
single watcher for gitDir that triggers the shared logic: call
emitGitStateChange() and (for the head case) kick off
configureBranchWatcher(gitDir, generation). Update the code around
watchFile(...) so it either accepts an array of targets or use one
watchFile(generation, ['head','packedRefs'], gitDir, callback) (or create a new
single watch on gitDir) and inside the callback inspect which file changed and
invoke emitGitStateChange(), and only when the changed file is 'head' call void
configureBranchWatcher(gitDir, generation); keep the existing function names
watchFile, emitGitStateChange, and configureBranchWatcher to locate and modify
the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18bdcff3-fc0d-4b34-816a-23feda077944
📒 Files selected for processing (2)
code/core/src/core-server/change-detection/GitDiffProvider.test.tscode/core/src/core-server/change-detection/GitDiffProvider.ts
… enhance test reliability. Updated tests to utilize waitFor for asynchronous assertions, ensuring proper cleanup of watchers and accurate git state change detection.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/GitDiffProvider.test.ts (1)
405-435: Consider documenting the timing assumption in the test.The test uses
advanceTimersByTimeAsync(60)to trigger the watcher rebuild. This assumes the exponential backoff starts at 50ms (Math.min(1000, 50 * 2 ** 0) = 50). Consider adding a brief comment to clarify this assumption:watchRecords[1].emitError(); + // First rebuild delay is 50ms (50 * 2^0), advance past it await vi.advanceTimersByTimeAsync(60);🤖 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/GitDiffProvider.test.ts` around lines 405 - 435, Add a brief comment in the test "rebuilds watchers after a watcher error while callbacks are still registered" (the test using setupGitWatchProvider(), provider.onGitStateChange, and advanceTimersByTimeAsync(60)) that documents the timing assumption: note that the rebuild relies on the backoff starting at 50ms (Math.min(1000, 50 * 2 ** 0) === 50) so advancing timers by 60ms triggers the retry; place this comment near the advanceTimersByTimeAsync(60) call to make the assumption explicit for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/change-detection/GitDiffProvider.test.ts`:
- Around line 405-435: Add a brief comment in the test "rebuilds watchers after
a watcher error while callbacks are still registered" (the test using
setupGitWatchProvider(), provider.onGitStateChange, and
advanceTimersByTimeAsync(60)) that documents the timing assumption: note that
the rebuild relies on the backoff starting at 50ms (Math.min(1000, 50 * 2 ** 0)
=== 50) so advancing timers by 60ms triggers the retry; place this comment near
the advanceTimersByTimeAsync(60) call to make the assumption explicit for future
readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15f1f731-373e-448c-85fd-7b12bdeda267
📒 Files selected for processing (2)
code/core/src/core-server/change-detection/GitDiffProvider.test.tscode/core/src/core-server/change-detection/GitDiffProvider.ts
…en change detection is enabled.
… error logging. Updated tests to reflect changes in callback handling and watcher teardown, ensuring accurate detection of git state changes and improved test reliability.
…avior. Codified the intent to avoid indefinite retries when git watching is unavailable, enhancing test documentation.
…n choices for single subscriber support and error handling during watcher failures. This improves code readability and maintains predictable behavior in change detection.
…tate change handling. Removed unnecessary unsubscribe logic and improved watcher management for better performance and clarity in change detection.
valentinpalkovic
approved these changes
Apr 2, 2026
10 tasks
This was referenced Apr 2, 2026
Merged
8 tasks
This was referenced Apr 30, 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
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
Added Git change detection for the ChangeDetectionService so that whenever a new commit is created or the branch is switched, the module graph will be reevaluated. In practice, this means that as soon as file changes are committed, components will no longer be marked "modified".
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
features.changeDetectionis enabled. Start Storybook and keep it running.git commit -am "test"); verify both statuses clear after the commit (no longer shown as changed/new).git reset HEAD~1 --hard) but be aware this will drop local changes.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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-34420-sha-fbb1632a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34420-sha-fbb1632a sandboxor in an existing project withnpx storybook@0.0.0-pr-34420-sha-fbb1632a upgrade.More information
0.0.0-pr-34420-sha-fbb1632agit-change-detectionfbb1632a1775052344)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=34420Summary by CodeRabbit
New Features
Bug Fixes
Tests