fix(desktop): fix git state mapping to wrong workspace in sidebar#1453
Conversation
📝 WalkthroughWalkthroughRemoved implicit local-DB branch synchronization from the changes status flow, added an explicit Changes
Sequence DiagramsequenceDiagram
actor User
participant Hook as "useBranchSyncInvalidation\n(Hook)"
participant Client as "TRPC Client"
participant Server as "syncBranch RPC\n(Server)"
participant DB as "Database"
User->>Hook: Git branch changes
Hook->>Hook: Detect branch mismatch (gitBranch ≠ workspaceBranch)
alt Guard active or branches match
Hook->>Hook: Skip sync
else Proceed
Hook->>Hook: Set syncingRef
Hook->>Client: mutate(workspaceId, branch)
Client->>Server: syncBranch request
Server->>Server: Validate branch
alt Invalid branch
Server-->>Client: { success: false, reason: "invalid-branch" }
else Valid branch
Server->>DB: Fetch workspace
alt Not found
Server-->>Client: { success: false, reason: "not-found" }
else Found
alt Branch unchanged
Server-->>Client: { success: true, changed: false }
else Branch differs
Server->>DB: Update workspace.branch
opt workspace has worktreeId
Server->>DB: Update worktree.branch
end
Server-->>Client: { success: true, changed: true }
end
end
end
Client->>Hook: Mutation settled
alt changed = true
Hook->>Hook: Update cached workspace groups
Hook->>Hook: Invalidate related queries
end
Hook->>Hook: Clear syncingRef
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
saddlepaddle
force-pushed
the
kitenite/sidebar-not-mapping-correctly
branch
from
90b8951 to
e938e44
Compare
![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
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/hooks/useBranchSyncInvalidation/useBranchSyncInvalidation.ts`:
- Around line 14-56: The syncingRef used in useBranchSyncInvalidation
(syncingRef.current) is never cleared if
electronTrpc.workspaces.syncBranch.useMutation().mutate fails, which blocks
future retries; update the mutate call inside doSync to provide an onSettled (or
onError) callback that sets syncingRef.current = null so a subsequent effect run
can retry, and keep the existing success logic intact in onSuccess; reference
the mutate call inside doSync and the syncingRef variable to locate where to add
the onSettled/onError handler.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/changes/status.ts (1)
149-164: Emptycatch {}silently swallows errors.Per coding guidelines, errors should not be swallowed silently. While skipping individual file failures is reasonable here (files may vanish between status check and read), a debug-level log would help troubleshoot issues.
Optional: add minimal logging
- } catch {} + } catch (err) { + console.debug(`[changes/status] Failed to read untracked file: ${file.path}`, err); + }As per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.ts (2)
147-150: Inconsistent error handling: return-value vs throwing.Other mutations in this file (
update,setUnread,setActive) thrownew Error(...)when a workspace is not found, butsyncBranchreturns{ success: false, reason: "not-found" }. This is defensible for a polling-driven sync (not-found isn't exceptional), but the inconsistency means callers need two different handling strategies.Consider documenting the rationale or aligning the pattern. If the return-value approach is preferred for sync operations, that's fine — just worth being intentional about the distinction.
156-160: UsetouchWorkspaceto bumpupdatedAtwhen changing branch.The local-db workspaces schema doesn't have an
$onUpdatehook forupdatedAt. The direct.set({ branch })update won't update the timestamp. Other mutations in this file (e.g., patch updates) usetouchWorkspace, which handles both the field change and timestamp updates. SincetouchWorkspaceacceptsbranchinadditionalFields, it should be used here for consistency:Use touchWorkspace for branch updates
- localDb - .update(workspaces) - .set({ branch }) - .where(eq(workspaces.id, workspaceId)) - .run(); + touchWorkspace(workspaceId, { branch });
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
- Move branch sync from path-based lookup in changes/status to workspace-aware syncBranch mutation called from renderer with correct workspaceId - Fix flickering by extracting stable mutate ref and clearing sync guard only when workspaceBranch catches up - Fix setup.sh local DB source path (~/.superset instead of ~/.superset-dev)
saddlepaddle
force-pushed
the
kitenite/sidebar-not-mapping-correctly
branch
from
e938e44 to
04a600b
Compare
2 participants
Summary
syncWorkspaceBranchinside thegetStatusquery used path-based worktree lookup instead of workspace ID, which could match the wrong DB record (no unique constraint onworktrees.path). This ran as a side effect on every 2.5s poll and every hover.useBranchSyncInvalidationto callinvalidate(getAllGrouped), re-rendering all sidebar items and triggering accumulated queries in a loop — causing flickering diffs and wrong branch labels.syncBranchmutation using workspace ID, and replaced nuclear cache invalidation with in-place cache patching.Changes
changes/status.ts: RemovedsyncWorkspaceBranchfunction and its call fromgetStatus— query is now a pure read with no DB side effectsworkspaces/procedures/status.ts: AddedsyncBranchmutation that looks up workspace by ID and updates bothworkspaces.branchandworktrees.branchuseBranchSyncInvalidation.ts: Rewrote to callsyncBranchmutation, patchgetAllGroupedcache in-place viasetData, and only invalidate the specific workspace's queries (with dedup guard viasyncingRef)Test Plan
git switch <branch>in one worktree — sidebar branch label should update correctlySummary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores