fix(desktop): use getAllGrouped as single source of truth for workspace queries#715
fix(desktop): use getAllGrouped as single source of truth for workspace queries#715
Conversation
📝 WalkthroughWalkthroughThis PR removes the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Client Hook
participant Cache as React Query Cache (getAllGrouped)
participant TRPC as trpc.mutation -> Server
participant Server
User->>UI: trigger close/delete workspace
UI->>Cache: optimistic update (update getAllGrouped)
UI->>TRPC: send mutation request
TRPC->>Server: apply change
Server-->>TRPC: mutation result
TRPC-->>UI: onSuccess callback
UI->>Cache: invalidate getAllGrouped (and getActive if needed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
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
🧹 Nitpick comments (2)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (2)
61-72: Hardcoded worktree metadata may cause brief UI inconsistency.The optimistic update uses placeholder values (
baseBranch: null,needsRebase: false) that may not match the actual worktree state. While this is acceptable for a brief optimistic window before the cache is invalidated inonSuccess, be aware that UI components relying on these fields could display stale data momentarily.
48-98: Consider extracting duplicated next-workspace selection logic.This block (~50 lines) is nearly identical to
useDeleteWorkspace.ts. Extracting a shared helper would reduce duplication and ensure consistent behavior when selecting the next active workspace.♻️ Suggested helper function
Create a shared utility, e.g., in a
utilsfile:// workspaceOptimisticHelpers.ts export function selectNextWorkspace({ previousGrouped, excludeId, }: { previousGrouped: GroupedWorkspaces | undefined; excludeId: string; }) { const remainingWorkspaces = previousGrouped ?.flatMap((g) => g.workspaces .filter((w) => w.id !== excludeId) .map((w) => ({ workspace: w, project: g.project })), ) .sort((a, b) => b.workspace.lastOpenedAt - a.workspace.lastOpenedAt); if (!remainingWorkspaces?.length) return null; const { workspace: nextWorkspace, project } = remainingWorkspaces[0]; const worktreeData = nextWorkspace.type === "worktree" ? { branch: nextWorkspace.branch, baseBranch: null, gitStatus: { branch: nextWorkspace.branch, needsRebase: false, lastRefreshed: Date.now(), }, } : null; return { id: nextWorkspace.id, projectId: nextWorkspace.projectId, worktreeId: nextWorkspace.worktreeId, branch: nextWorkspace.branch, name: nextWorkspace.name, tabOrder: nextWorkspace.tabOrder, createdAt: nextWorkspace.createdAt, updatedAt: nextWorkspace.updatedAt, lastOpenedAt: nextWorkspace.lastOpenedAt, isUnread: nextWorkspace.isUnread, type: nextWorkspace.type, worktreePath: nextWorkspace.worktreePath, deletingAt: null, project: { id: project.id, name: project.name, mainRepoPath: project.mainRepoPath, }, worktree: worktreeData, }; }Then both hooks can call:
const nextActiveData = selectNextWorkspace({ previousGrouped, excludeId: id }); utils.workspaces.getActive.setData(undefined, nextActiveData);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
- apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (1)
34-38: LGTM!The removal of
getAllinvalidation aligns with the PR goal of usinggetAllGroupedas the single source of truth. The parallel invalidation ofgetActiveandgetAllGroupedis appropriate.apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (1)
102-118: LGTM!Error handling correctly restores both
previousGroupedandpreviousActivecaches. TheonSuccessinvalidation strategy ensures data consistency after the mutation completes.apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (1)
102-121: LGTM!The
onSettledpattern ensures cache invalidation happens regardless of success/error, which is appropriate for delete operations. Error handling correctly restores both caches.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
16-16: LGTM!Documentation correctly reflects the removal of
getAllfrom the query procedures.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (1)
17-22: LGTM!The
useMemoderivation is efficient and correctly flattens grouped workspaces. The default empty array handles the undefined case appropriately, and the comment clearly explains the rationale for usinggetAllGrouped.
…ce queries Root cause: The optimistic updates in useDeleteWorkspace and useCloseWorkspace relied on `getAll` query cache to find the next workspace, but `getAll` was rarely queried (only used by PortsList). When the cache was empty, the optimistic update would incorrectly set activeWorkspace to null, causing StartView to flash. Additionally, `utils.workspaces.invalidate()` was invalidating ALL queries including `getActive`, causing the optimistically-set data to be refetched and briefly showing undefined during the refetch. Changes: - Use `getAllGrouped` as the single source of truth (always cached by sidebar) - Remove all `getAll` query usage from frontend mutation hooks - Update `usePortsData` to derive workspace list from `getAllGrouped` - Remove unused `getAll` procedure from backend - Only invalidate `getAllGrouped` after delete/close, not `getActive` (since we already set it optimistically and don't want a refetch flash) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Kitenite
force-pushed
the
fix/desktop-workspace-cache-single-source-of-truth
branch
from
12bb1dc to
31f0ac0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (1)
48-98: Consider extracting shared next-workspace logic.The logic for computing the next active workspace (lines 50-94) is nearly identical to
useDeleteWorkspace.ts. This includes theremainingWorkspacesderivation,worktreeDataconstruction, and the fullsetDatapayload.Consider extracting a shared helper function (e.g.,
buildNextActiveWorkspacePayload) to reduce duplication and ensure both hooks stay in sync.♻️ Example shared helper
// In a shared utils file, e.g., renderer/react-query/workspaces/utils.ts export function computeNextActiveWorkspace( groupedWorkspaces: GroupedWorkspaces | undefined, excludeId: string, ) { const remaining = groupedWorkspaces ?.flatMap((g) => g.workspaces .filter((w) => w.id !== excludeId) .map((w) => ({ workspace: w, project: g.project })), ) .sort((a, b) => b.workspace.lastOpenedAt - a.workspace.lastOpenedAt); if (!remaining || remaining.length === 0) return null; const { workspace, project } = remaining[0]; const worktreeData = workspace.type === "worktree" ? { branch: workspace.branch, baseBranch: null, gitStatus: { branch: workspace.branch, needsRebase: false, lastRefreshed: Date.now(), }, } : null; return { id: workspace.id, projectId: workspace.projectId, // ... rest of payload worktree: worktreeData, }; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.tsapps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
- apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
- apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build
🔇 Additional comments (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts (1)
17-22: LGTM! Clean derivation from grouped data.The change correctly uses
getAllGroupedas the source of truth and derives a flattened workspace list viauseMemo. The empty array fallback handles the undefined case gracefully, and the dependency array is correct.apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (2)
27-46: LGTM! Proper optimistic update pattern.The cancellation of in-flight queries before snapshotting, followed by optimistic cache updates, follows the correct react-query mutation pattern. Filtering out the closed workspace from grouped data and removing empty groups is handled correctly.
113-120: LGTM! Intentional invalidation strategy to prevent flash.The comment clearly explains the rationale for not invalidating
getActive. The optimistic update provides the UI state immediately, and onlygetAllGroupedneeds server reconciliation.apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (3)
27-46: LGTM! Consistent optimistic update pattern.The cancellation and optimistic update logic correctly mirrors
useCloseWorkspace, maintaining consistency across the codebase for workspace mutations.
102-107: Verify: Inconsistent invalidation timing between hooks.
useDeleteWorkspaceinvalidates inonSettled(runs on both success and error), whileuseCloseWorkspaceinvalidates inonSuccess(runs only on success).If this is intentional (e.g., delete operations need server reconciliation even on failure), consider adding a comment explaining the difference. Otherwise, consider aligning both hooks to use
onSuccessfor consistency, since the rollback inonErroralready restores the cache state.
111-123: LGTM! Proper rollback on error.The error handler correctly restores both
previousGroupedandpreviousActivecaches before forwarding to the caller's error handler.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
getAllGroupedas the single source of truth for workspace data (always cached by sidebar)getAllquery from frontend and backendRoot Cause
The optimistic updates in
useDeleteWorkspaceanduseCloseWorkspacerelied on thegetAllquery cache to find the next workspace. However,getAllwas rarely queried (only used by PortsList), so its cache was usually empty. When empty, the optimistic update would incorrectly setactiveWorkspaceto null, causing StartView to flash briefly.Changes
Frontend:
useDeleteWorkspace.ts/useCloseWorkspace.ts: UsegetAllGroupedas source of truth instead ofgetAllusePortsData.ts: Derive workspace list fromgetAllGroupedinstead of queryinggetAlluseSetActiveWorkspace.ts/useReorderWorkspaces.ts: Remove deadgetAll.invalidate()callsBackend:
procedures/query.ts: Remove unusedgetAllprocedureTest plan
🤖 Generated with Claude Code
Note
Consolidates workspace data fetching to a single query and improves optimistic updates to eliminate UI flicker on workspace changes.
getAllprocedure; keepget,getAllGrouped, andgetActive.useCloseWorkspaceanduseDeleteWorkspaceto rely ongetAllGroupedfor optimistic next-workspace selection and avoid invalidatinggetActiveto prevent flashes.useReorderWorkspacesanduseSetActiveWorkspace(dropgetAll); keepgetAllGrouped/getActivewhere needed.usePortsDataderives flat workspace list fromgetAllGroupedinstead of queryinggetAll.Written by Cursor Bugbot for commit 31f0ac0. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.