fix(desktop): sync workspace branch names and hover card branch display#1198
Conversation
📝 WalkthroughWalkthroughAdds DB-side synchronization to align worktrees and branch-type workspaces with the active Git branch; surfaces Changes
Sequence DiagramsequenceDiagram
participant UI as Renderer/UI
participant Status as getStatus Handler
participant Git as Git Ops
participant DB as Local DB
participant Cache as Query Cache
UI->>Status: request worktree status
Status->>Git: run git status / parse branch
Git-->>Status: return parsed.branch
Status->>DB: syncWorkspaceBranch(worktreePath, parsed.branch)
DB->>DB: check worktree entry & project workspaces
alt worktree exists
DB->>DB: update worktrees.branch (if mismatched)
DB->>DB: update related workspaces.branch (if mismatched)
else worktree not found
DB->>DB: find project by mainRepoPath
DB->>DB: update project's branch-type workspaces (if mismatched)
end
DB-->>Status: sync complete (errors logged)
Status-->>UI: return status (includes branch)
UI->>Cache: hook detects branch divergence
Cache->>Cache: invalidate grouped workspaces, workspace query, worktree info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
253-253:⚠️ Potential issue | 🟡 MinorEmpty catch block silently swallows errors.
The
getBranchComparisonfunction has an empty catch block that swallows all errors without any logging. This violates the guideline to never silently swallow errors. At minimum, log the error with context.🛡️ Proposed fix
- } catch {} + } catch (error) { + console.warn("[changes/status] Failed to get branch comparison:", error); + }As per coding guidelines: "Never silently swallow errors with
catch(() => {})orcatch(e) { return null }" and "Log errors at minimum if not re-throwing or explicitly handling them".
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx (1)
73-79: Consider narrowing theworkspacedependency to specific fields.Using the entire
workspaceobject as a dependency means this effect will re-run whenever any workspace property changes, not just when the branch comparison is relevant. While the early return guards prevent unnecessary invalidations, the effect still executes on every workspace update.♻️ Suggested improvement
useEffect(() => { if (!workspace || !status?.branch || status.branch === "HEAD") return; if (workspace.branch !== status.branch) { utils.workspaces.getAllGrouped.invalidate(); utils.workspaces.get.invalidate({ id: workspace.id }); } -}, [status?.branch, utils, workspace]); +}, [status?.branch, utils, workspace?.id, workspace?.branch]);apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
36-39: Synchronous DB operations in async query handler.
syncWorkspaceBranchperforms synchronous database operations (.run()) within an async tRPC query. While this works correctly with better-sqlite3's synchronous API, be aware this blocks the Node.js event loop briefly during each DB write. For the current use case with small, fast updates this is acceptable, but monitor if status queries become slow under load.
saddlepaddle
left a comment
saddlepaddle
left a comment
There was a problem hiding this comment.
Code Review: fix(desktop): sync workspace branch names and hover card branch display
Summary
The PR addresses a real UX bug — stale branch labels after external git switch. The approach is sound: syncWorkspaceBranch runs synchronously inside the getStatus tRPC query to update the local DB, and React effects on the client detect the mismatch to trigger cache invalidation.
Overall this is clean and well-structured. A few items to address below.
Key Observations
syncWorkspaceBranch is synchronous, not fire-and-forget. Since better-sqlite3 operations (.get(), .run(), .all()) are synchronous, the function executes fully before the subsequent Promise.all. This is correct — no await is needed and there's no race condition with the response.
Detached HEAD guard is good. The early return for currentBranch === "HEAD" correctly avoids clobbering stored branch names during detached HEAD states.
Error handling is appropriate. The try/catch with console.warn ensures a failed sync never breaks the status query. Correct tradeoff for a best-effort sync.
Issues
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Medium | WorkspaceListItem.tsx, ChangesView.tsx |
Duplicate cache invalidation logic — extract to shared hook |
| 2 | Low | ChangesView.tsx:81 |
Over-broad workspace object in useEffect dependency array |
| 3 | Nit | status.ts |
Read-before-update pattern queries workspaces twice (once to check, once to update) — can be simplified |
See inline comments for details.
| utils.workspaces.getWorktreeInfo.invalidate({ | ||
| workspaceId: workspace.id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Medium: Duplicate invalidation logic + over-broad dependency
Two issues here:
1. Duplicate logic (Medium): This useEffect is nearly identical to the one in WorkspaceListItem.tsx:138-147. Both detect branch mismatches and invalidate the same set of queries. Consider extracting a shared hook:
// e.g., hooks/useBranchSyncInvalidation.ts
function useBranchSyncInvalidation({
gitBranch,
workspaceBranch,
workspaceId,
}: {
gitBranch: string | undefined;
workspaceBranch: string | undefined;
workspaceId: string;
}) {
const utils = electronTrpc.useUtils();
useEffect(() => {
if (!gitBranch || gitBranch === "HEAD" || !workspaceBranch) return;
if (gitBranch !== workspaceBranch) {
utils.workspaces.getAllGrouped.invalidate();
utils.workspaces.get.invalidate({ id: workspaceId });
utils.workspaces.getWorktreeInfo.invalidate({ workspaceId });
}
}, [gitBranch, workspaceBranch, workspaceId, utils]);
}2. Over-broad dependency (Low): Using the full workspace object as a dependency means this effect re-evaluates on any workspace property change (e.g., updatedAt, lastOpenedAt). The guards prevent unnecessary invalidations, but narrowing prevents unnecessary effect executions:
-}, [status?.branch, utils, workspace]);
+}, [status?.branch, utils, workspace?.id, workspace?.branch]);|
|
||
| const hasWorkspaceMismatch = workspacesForWorktree.some( | ||
| (ws) => ws.branch !== currentBranch, | ||
| ); |
There was a problem hiding this comment.
Nit: Redundant read-before-update pattern
The function reads workspaces to check for mismatches, then runs the same WHERE clause again in the UPDATE. Since the UPDATE is a no-op when there are no matching rows, you could skip the read step:
// Instead of read-check-update, just update directly:
localDb
.update(workspaces)
.set({ branch: currentBranch })
.where(
and(
eq(workspaces.worktreeId, worktree.id),
isNull(workspaces.deletingAt),
not(eq(workspaces.branch, currentBranch)),
),
)
.run();This avoids the extra SELECT on every status poll. The same pattern applies to the project-level branch workspace update below (lines 178-202).
Not blocking — the current approach is clear and correct, this is just a performance nit since getStatus is polled every 2.5s.
| utils.workspaces.get.invalidate({ id }); | ||
| utils.workspaces.getWorktreeInfo.invalidate({ workspaceId: id }); | ||
| } | ||
| }, [localChanges?.branch, branch, id, utils]); |
There was a problem hiding this comment.
Note: This effect only fires after hover (because localChanges depends on hasHovered), so it handles the case where the user externally switches branches in a non-active workspace. For the active workspace, ChangesView.tsx handles it via its 2.5s polling.
See inline comment on ChangesView.tsx regarding extracting a shared hook to deduplicate this logic.
- Extract duplicate useEffect cache invalidation logic from WorkspaceListItem and ChangesView into shared useBranchSyncInvalidation hook - Narrow useEffect dependencies to specific fields instead of full objects - Simplify syncWorkspaceBranch by using direct UPDATE with not(eq()) clause instead of SELECT-then-conditionally-UPDATE pattern, reducing DB queries on every 2.5s status poll
saddlepaddle
force-pushed
the
codex/branch-sync
branch
from
b0bebd5 to
09f944f
Compare
2 participants
Description
Syncs workspace branch names from
git statusto avoid stale branch labels after externalgit switch, and fixes hover card to show the actual branch (separate from worktree name).Related Issues
Type of Change
Testing
cd apps/desktop && SKIP_ENV_VALIDATION=1 bun run devgit switch -c test-new-branchin the terminal tabScreenshots (if applicable)
Additional Notes
branchNamefromgetWorktreeInfo;worktreeNameremains the path segment for compatibility.changes.getStatus.Summary by CodeRabbit
New Features
Bug Fixes / Stability