fix(worktree): self-heal stale git-repo detection in reconciler#346
Open
willy-scr wants to merge 1 commit into
Open
fix(worktree): self-heal stale git-repo detection in reconciler#346willy-scr wants to merge 1 commit into
willy-scr wants to merge 1 commit into
Conversation
The active worktree reconciler skipped any project flagged isGitRepo=false, so a project detected as non-git at startup (e.g. git missing from the GUI app's PATH, or the repo initialised afterwards) could never be re-detected. The label showed "New Worktree (no git repo)" even for a valid repository. - Re-run reconcileProjectWorktreesNow when a project is marked non-git so the flag is re-checked and flipped back to true. - Heal the reverse case: flip to false when git now reports NOT_A_GIT_REPO (e.g. .git was removed). - Remove a dead-code useWorktreeReconciler duplicate (same export name) in use-projects-persistence.ts that masked the live hook and was never mounted. - Add tests for both self-heal directions. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR refactors worktree reconciliation by removing automatic hook triggering from the persistence module and enhancing the reconciliation hook with self-healing logic that detects when projects are no longer git repos and triggers full re-detection. ChangesWorktree Reconciliation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/use-worktree-reconciler.test.ts (1)
166-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd assertion for the reverse heal behavior.
With the new reverse heal logic (implementation lines 51-52), when
worktreeApi.listreturnsNOT_A_GIT_REPO, the hook callsupdateProject(projectId, { isGitRepo: false }). This test should verify that behavior in addition to checking thatremoveWorktreeandaddWorktreeare not called.🧪 Recommended assertion addition
await vi.waitFor(() => { expect(mocks.removeWorktree).not.toHaveBeenCalled() expect(mocks.addWorktree).not.toHaveBeenCalled() + expect(mocks.updateProject).toHaveBeenCalledWith('proj-1', { isGitRepo: false }) }) })🤖 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 `@src/renderer/hooks/use-worktree-reconciler.test.ts` around lines 166 - 180, Update the test for useWorktreeReconciler to also assert the reverse-heal call: after mocking worktreeApi.list to return NOT_A_GIT_REPO and rendering the hook, add an expectation that updateProject was called with the project id and { isGitRepo: false } (e.g. expect(mocks.updateProject).toHaveBeenCalledWith('proj-1', { isGitRepo: false })). Keep the existing assertions that removeWorktree and addWorktree were not called and wrap these expectations in the same async waitFor to ensure the async logic in useWorktreeReconciler runs first.
🧹 Nitpick comments (2)
src/renderer/hooks/use-worktree-reconciler.test.ts (1)
182-196: 💤 Low valueConsider mocking
worktreeApi.listfor the post-heal reconciliation attempt.After
reconcileNowflipsisGitRepoto true, the hook re-reads the project and proceeds to callworktreeApi.list(implementation line 48). Without an explicit mock, the test relies on the default undefined return, which triggers the error path. While this doesn't break the test, explicitly mocking the success case would make the test's intent clearer and more robust.🧪 Suggested mock addition
mocks.state.isGitRepo = false // The shared reconciler re-runs `git worktree list`; simulate the heal side-effect. mocks.reconcileNow.mockImplementation(async () => { mocks.state.isGitRepo = true }) + // After heal, the hook will call worktreeApi.list again; mock a successful response. + vi.mocked(worktreeApi.list).mockResolvedValue({ + success: true, + data: [ + { + name: 'feat-1', + branch: 'feat-1', + path: '/test/project/.termul/worktrees/feat-1', + headCommit: 'abc' + } + ] + }) renderHook(() => useWorktreeReconciler('proj-1'))🤖 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 `@src/renderer/hooks/use-worktree-reconciler.test.ts` around lines 182 - 196, The test should explicitly mock worktreeApi.list so the post-heal reconciliation path succeeds: in the test that flips mocks.state.isGitRepo to true via mocks.reconcileNow, add a mock implementation or return value for worktreeApi.list (the symbol referenced in useWorktreeReconciler) that returns the expected worktree data (or a resolved Promise) after the heal; keep the existing mocks.reconcileNow behavior and assertions (reconcileNow and proj-1) but ensure worktreeApi.list is stubbed to avoid relying on undefined behavior and to exercise the success branch.src/renderer/hooks/use-worktree-reconciler.ts (1)
30-45: ⚡ Quick winConsider clarifying the double state read pattern.
The code reads project state twice: once at line 30 to decide whether to self-heal, then again at line 42 to proceed with reconciliation. While this is intentional (covering both the heal and concurrent updates), a brief inline comment explaining the re-read purpose would improve maintainability.
📝 Suggested comment addition
if (!initial.isGitRepo) { await reconcileProjectWorktreesNow(projectId) } - // Re-read the latest state (covers the heal above and any concurrent store writes). + // Re-read the latest state. This covers two cases: + // 1. The self-heal above may have flipped isGitRepo from false to true. + // 2. Concurrent store updates during the await above. const project = useProjectStore.getState().projects.find((p) => p.id === projectId)🤖 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 `@src/renderer/hooks/use-worktree-reconciler.ts` around lines 30 - 45, The two separate reads of useProjectStore.getState() (into initial and later project) are intentional but unclear; add a concise inline comment above the second read (before const project = useProjectStore.getState()...) explaining that the first read is used to decide whether to self-heal via reconcileProjectWorktreesNow(projectId) and the second read deliberately re-reads state to pick up the heal or any concurrent store updates before proceeding with reconciliation (mention useProjectStore, initial, project, and reconcileProjectWorktreesNow by name to make intent obvious).
🤖 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 `@src/renderer/hooks/use-worktree-reconciler.ts`:
- Around line 37-39: The call to reconcileProjectWorktreesNow(projectId) can
throw and must be covered by the existing error handling: move or wrap the await
reconcileProjectWorktreesNow(projectId) into the same try-catch used by the
reconciler (the callback invoked by reconcile()), i.e., ensure
reconcileProjectWorktreesNow is awaited inside a try block and any errors are
caught and handled (log or suppress) in the catch so the promise rejection
doesn't escape the reconcile callback.
---
Outside diff comments:
In `@src/renderer/hooks/use-worktree-reconciler.test.ts`:
- Around line 166-180: Update the test for useWorktreeReconciler to also assert
the reverse-heal call: after mocking worktreeApi.list to return NOT_A_GIT_REPO
and rendering the hook, add an expectation that updateProject was called with
the project id and { isGitRepo: false } (e.g.
expect(mocks.updateProject).toHaveBeenCalledWith('proj-1', { isGitRepo: false
})). Keep the existing assertions that removeWorktree and addWorktree were not
called and wrap these expectations in the same async waitFor to ensure the async
logic in useWorktreeReconciler runs first.
---
Nitpick comments:
In `@src/renderer/hooks/use-worktree-reconciler.test.ts`:
- Around line 182-196: The test should explicitly mock worktreeApi.list so the
post-heal reconciliation path succeeds: in the test that flips
mocks.state.isGitRepo to true via mocks.reconcileNow, add a mock implementation
or return value for worktreeApi.list (the symbol referenced in
useWorktreeReconciler) that returns the expected worktree data (or a resolved
Promise) after the heal; keep the existing mocks.reconcileNow behavior and
assertions (reconcileNow and proj-1) but ensure worktreeApi.list is stubbed to
avoid relying on undefined behavior and to exercise the success branch.
In `@src/renderer/hooks/use-worktree-reconciler.ts`:
- Around line 30-45: The two separate reads of useProjectStore.getState() (into
initial and later project) are intentional but unclear; add a concise inline
comment above the second read (before const project =
useProjectStore.getState()...) explaining that the first read is used to decide
whether to self-heal via reconcileProjectWorktreesNow(projectId) and the second
read deliberately re-reads state to pick up the heal or any concurrent store
updates before proceeding with reconciliation (mention useProjectStore, initial,
project, and reconcileProjectWorktreesNow by name to make intent obvious).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b7e062d-636b-4bd7-a590-c555f9692c30
📒 Files selected for processing (3)
src/renderer/hooks/use-projects-persistence.tssrc/renderer/hooks/use-worktree-reconciler.test.tssrc/renderer/hooks/use-worktree-reconciler.ts
💤 Files with no reviewable changes (1)
- src/renderer/hooks/use-projects-persistence.ts
Comment on lines
+37
to
+39
| if (!initial.isGitRepo) { | ||
| await reconcileProjectWorktreesNow(projectId) | ||
| } |
Contributor
There was a problem hiding this comment.
Wrap the self-heal call in try-catch to prevent unhandled rejections.
reconcileProjectWorktreesNow is awaited outside the try-catch block that starts at line 47. If it throws (e.g., due to I/O errors, store exceptions, or API failures), the exception will propagate as an unhandled promise rejection because the reconcile callback is invoked with void reconcile() (lines 111, 114).
🛡️ Recommended fix: extend try-catch coverage
// Self-heal stale git-repo detection. A project can be marked non-git if git
// was not available when it was first detected (e.g. the GUI app's PATH differs
// from the shell at startup, or the repo was initialised afterwards). Re-run the
// shared reconciler, which executes `git worktree list` and flips `isGitRepo`.
- if (!initial.isGitRepo) {
- await reconcileProjectWorktreesNow(projectId)
- }
-
- // Re-read the latest state (covers the heal above and any concurrent store writes).
- const project = useProjectStore.getState().projects.find((p) => p.id === projectId)
- if (!project?.isGitRepo || !project.path) return
- // Allow empty worktrees array (still reconcile to discover newly created worktrees)
- if (project.worktrees === undefined) return
-
- try {
+ try {
+ if (!initial.isGitRepo) {
+ await reconcileProjectWorktreesNow(projectId)
+ }
+
+ // Re-read the latest state (covers the heal above and any concurrent store writes).
+ const project = useProjectStore.getState().projects.find((p) => p.id === projectId)
+ if (!project?.isGitRepo || !project.path) return
+ // Allow empty worktrees array (still reconcile to discover newly created worktrees)
+ if (project.worktrees === undefined) return
+
const result = await worktreeApi.list(project.path)📝 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
| if (!initial.isGitRepo) { | |
| await reconcileProjectWorktreesNow(projectId) | |
| } | |
| try { | |
| if (!initial.isGitRepo) { | |
| await reconcileProjectWorktreesNow(projectId) | |
| } | |
| // Re-read the latest state (covers the heal above and any concurrent store writes). | |
| const project = useProjectStore.getState().projects.find((p) => p.id === projectId) | |
| if (!project?.isGitRepo || !project.path) return | |
| // Allow empty worktrees array (still reconcile to discover newly created worktrees) | |
| if (project.worktrees === undefined) return | |
| const result = await worktreeApi.list(project.path) |
🤖 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 `@src/renderer/hooks/use-worktree-reconciler.ts` around lines 37 - 39, The call
to reconcileProjectWorktreesNow(projectId) can throw and must be covered by the
existing error handling: move or wrap the await
reconcileProjectWorktreesNow(projectId) into the same try-catch used by the
reconciler (the callback invoked by reconcile()), i.e., ensure
reconcileProjectWorktreesNow is awaited inside a try block and any errors are
caught and handled (log or suppress) in the catch so the promise rejection
doesn't escape the reconcile callback.
Owner
|
Hey @willy-scr — triage update 🔁 CI is green and this is mergeable ✅. One open CodeRabbit thread is worth a look before merging: wrap the |
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
2 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.
Problem
A project that is a valid git repository can be permanently misdetected as "no git repo" in the project context menu (the entry reads
New Worktree (no git repo)and is disabled), even though the repo is perfectly valid:Root cause
The live
useWorktreeReconcilerhook (use-worktree-reconciler.ts) reconciled worktrees with this guard:Once
isGitRepowas ever set tofalse, the periodic (60s) reconciler short-circuited and never re-checked, so a stale flag could never be corrected — a detection deadlock.A project gets flagged
isGitRepo: falseat startup whengit worktree listfails transiently — most commonly because a GUI app's PATH differs from the shell, sogitisn't found at first detection (or the repo was initialised after the project was added).There was also a second
useWorktreeReconcilerexport inuse-projects-persistence.ts(same name, no guard, could self-heal) that was dead code — never mounted — which masked the bug and made the duplication confusing.Fix
false → true: when a project is marked non-git, re-runreconcileProjectWorktreesNow(which executesgit worktree listand flipsisGitRepo) before skipping reconciliation.true → false: flip tofalsewhenworktree_listnow returnsNOT_A_GIT_REPO/GIT_NOT_FOUND(e.g..gitwas removed).useWorktreeReconcilerinuse-projects-persistence.ts.Verification
bun run typecheck✅ (node + web)bun run vitest run use-worktree-reconciler→ 6/6 pass (incl. 2 new self-heal tests)bun run vitest run use-projects-persistence→ 7/7 passbiome check✅ on changed filesBehaviour change
After this change, a project that was wrongly flagged non-git will correct itself on the next reconciliation tick (≤60s) or when it becomes the active project, instead of being stuck forever.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes