Extract local branch sync logic and scope sync to local mode#65
Extract local branch sync logic and scope sync to local mode#65juliusmarminge merged 2 commits intomainfrom
Conversation
- move branch-sync decision into `BranchToolbar.logic.ts` - avoid auto-sync when env mode is `worktree` or thread has worktree path - add unit tests for sync and no-sync cases
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds deriveSyncedLocalBranch and its input interface, integrates it into BranchToolbar to replace inline branch-lookup logic, and adds unit tests covering local-sync and worktree bypass scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Scope branch auto-sync in
|
Greptile SummaryThis PR extracts the branch auto-sync decision logic from Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[useEffect fires\nwhen deps change] --> B[deriveSyncedLocalBranch]
B --> C{activeThreadId\ndefined?}
C -- No --> D[return null]
C -- Yes --> E{activeWorktreePath\nnon-null?}
E -- Yes --> D
E -- No --> F{envMode\n=== 'local'?}
F -- No --> D
F -- Yes --> G[find current branch\nin queryBranches]
G --> H{currentBranch\nfound?}
H -- No --> D
H -- Yes --> I{currentBranch.name\n=== activeThreadBranch?}
I -- Yes --> D
I -- No --> J[return currentBranch.name]
D --> K[return: no dispatch]
J --> L[dispatch SET_THREAD_BRANCH\nthreadId: activeThreadId\nbranch: syncedBranch\nworktreePath: null]
Last reviewed commit: 3756bbf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| dispatch({ | ||
| type: "SET_THREAD_BRANCH", | ||
| threadId: activeThreadId, | ||
| branch: current.name, | ||
| branch: syncedBranch, | ||
| worktreePath: null, |
There was a problem hiding this comment.
Unsafe threadId type passed to dispatch
activeThreadId is typed string | undefined (derived from activeThread?.id at line 47), but SET_THREAD_BRANCH requires threadId: string. This compiles only because TypeScript doesn't narrow the type across the deriveSyncedLocalBranch call boundary — it doesn't know that a non-null return from that function implies activeThreadId is defined.
The fix is to explicitly assert the non-undefined guarantee that the logic function already encodes, so the narrowing is visible to the type checker at the call site:
| dispatch({ | |
| type: "SET_THREAD_BRANCH", | |
| threadId: activeThreadId, | |
| branch: current.name, | |
| branch: syncedBranch, | |
| worktreePath: null, | |
| dispatch({ | |
| type: "SET_THREAD_BRANCH", | |
| threadId: activeThreadId!, | |
| branch: syncedBranch, | |
| worktreePath: null, | |
| }); |
Alternatively, guard with if (!activeThreadId) return; before the dispatch call (redundant at runtime, but type-safe and consistent with the pattern used elsewhere in this file at lines 115 and 120).
| describe("deriveSyncedLocalBranch", () => { | ||
| it("syncs to git current branch in local mode", () => { | ||
| const result = deriveSyncedLocalBranch({ | ||
| activeThreadId: "thread-1", | ||
| activeWorktreePath: null, | ||
| envMode: "local", | ||
| activeThreadBranch: "feature/demo", | ||
| queryBranches: branches, | ||
| }); | ||
|
|
||
| expect(result).toBe("main"); | ||
| }); | ||
|
|
||
| it("does not sync when creating a new worktree", () => { | ||
| const result = deriveSyncedLocalBranch({ | ||
| activeThreadId: "thread-1", | ||
| activeWorktreePath: null, | ||
| envMode: "worktree", | ||
| activeThreadBranch: "feature/demo", | ||
| queryBranches: branches, | ||
| }); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it("does not sync when thread already targets a worktree path", () => { | ||
| const result = deriveSyncedLocalBranch({ | ||
| activeThreadId: "thread-1", | ||
| activeWorktreePath: "/tmp/repo/worktrees/feature-demo", | ||
| envMode: "local", | ||
| activeThreadBranch: "feature/demo", | ||
| queryBranches: branches, | ||
| }); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test cases for documented guardrails
The PR description explicitly states "Keep existing guardrails: no sync when there is no active thread, when thread already has a worktree path, or when branch is already in sync." However, the test suite is missing coverage for:
- No active thread (
activeThreadId: undefined) — the first guard in the logic (!activeThreadId). - Branch already in sync (
activeThreadBranch === currentBranch.name) — the equality check guard. - No branch data (
queryBranches: undefined) — defensive path when the query hasn't resolved.
The "does not sync when thread already targets a worktree path" test covers the activeWorktreePath guard, and the "does not sync when creating a new worktree" test covers the envMode guard. The missing cases are easy to add and would fully validate the claimed guardrails:
it("does not sync when there is no active thread", () => {
const result = deriveSyncedLocalBranch({
activeThreadId: undefined,
activeWorktreePath: null,
envMode: "local",
activeThreadBranch: "feature/demo",
queryBranches: branches,
});
expect(result).toBeNull();
});
it("does not sync when branch is already in sync", () => {
const result = deriveSyncedLocalBranch({
activeThreadId: "thread-1",
activeWorktreePath: null,
envMode: "local",
activeThreadBranch: "main",
queryBranches: branches,
});
expect(result).toBeNull();
});
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/BranchToolbar.tsx`:
- Around line 90-103: The TS2322 error occurs because deriveSyncedLocalBranch
returning a value doesn't convince TypeScript that activeThreadId is a string;
before calling dispatch({ type: "SET_THREAD_BRANCH", threadId: activeThreadId,
... }) add an explicit guard that ensures activeThreadId is defined (e.g., if
(!activeThreadId) return;) so threadId is narrowed to string, then proceed to
dispatch using the already computed syncedBranch; reference
deriveSyncedLocalBranch, activeThreadId, and the dispatch call with type
"SET_THREAD_BRANCH".
- Prevent `SET_THREAD_BRANCH` dispatch unless both `activeThreadId` and `syncedBranch` exist - Avoids updating thread-branch state without an active thread context
1 participant
Summary
BranchToolbar.logic.tsviaderiveSyncedLocalBranch.localmode (skip sync inworktreemode).BranchToolbar.logic.test.tscovering local-mode sync and non-sync cases.Testing
deriveSyncedLocalBranch:Summary by CodeRabbit
Refactor
Tests
Note
Low Risk
Small refactor that narrows when auto-sync runs and adds focused unit tests; limited surface area and no security/data-impacting changes.
Overview
Thread-branch auto-sync in
BranchToolbaris refactored into a newderiveSyncedLocalBranchhelper and is now explicitly disabled inworktreemode, preventing automatic branch changes while creating/using worktrees.Adds Vitest unit coverage for the helper to ensure sync happens only when appropriate (local mode, no worktree path, and branch differs from git current).
Written by Cursor Bugbot for commit 86a2cbc. This will update automatically on new commits. Configure here.