Skip to content

fix (desktop): delete optimistic updates#632

Merged
AviPeltz merged 3 commits intomainfrom
why-deleting-take-so-long
Jan 7, 2026
Merged

fix (desktop): delete optimistic updates#632
AviPeltz merged 3 commits intomainfrom
why-deleting-take-so-long

Conversation

@AviPeltz
Copy link
Copy Markdown
Collaborator

@AviPeltz AviPeltz commented Jan 6, 2026

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes

Summary by CodeRabbit

  • New Features
    • Workspaces now remove from the UI instantly when closed or deleted (optimistic update).
    • If the active workspace is closed/deleted, the app automatically switches to a most-recent workspace.
    • Operations run in the background and sync afterward to keep data consistent.
    • If an operation fails, the previous workspace state is restored automatically.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 6, 2026

📝 Walkthrough

Walkthrough

Adds optimistic UI updates and rollback for workspace close and delete mutations: mutations snapshot and modify React Query caches (getAllGrouped, getAll, getActive) onMutate, restore on onError, and invalidate on onSuccess; introduces CloseContext/DeleteContext to hold snapshots.

Changes

Cohort / File(s) Summary
Workspace mutation hooks
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts, apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
Both hooks now implement optimistic updates: onMutate cancels queries, snapshots getAllGrouped/getAll/getActive, and removes the target workspace from caches. If the target was active, the active workspace is switched (or cleared) based on grouped recency. onError restores snapshots via CloseContext/DeleteContext; onSuccess invalidates related queries and forwards user callbacks.
Planning / docs
plans/optimistic-workspace-delete.md
Documents optimistic deletion behavior, UX integration points (active-workspace switching, dialog timing), test scenarios, and files to review for UI implications.

Sequence Diagram

sequenceDiagram
    participant User as User / UI
    participant Mut as React Query Mutation
    participant Cache as Local Cache (React Query)
    participant API as Backend API

    User->>Mut: trigger delete/close(workspaceId)

    rect rgb(220,235,255)
    Note over Mut,Cache: onMutate — optimistic update
    Mut->>Mut: cancelQueries()
    Mut->>Cache: snapshot previousGrouped, previousAll, previousActive
    Mut->>Cache: remove workspace from getAllGrouped & getAll
    Mut->>Cache: update/clear getActive (if needed)
    Mut-->>User: UI updates immediately
    end

    Mut->>API: send delete/close request

    alt success
        rect rgb(220,255,230)
        API-->>Mut: success
        Mut->>Mut: invalidate workspace/project queries
        Mut->>User: invoke userOnSuccess (if provided)
        end
    else error
        rect rgb(255,230,230)
        API-->>Mut: error
        Mut->>Cache: restore previousGrouped, previousAll, previousActive
        Mut->>User: invoke userOnError (if provided)
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 I nudged a workspace, sent it on its way,
It vanished from view while the API went to play.
If the server frowns, I hop it back in place,
Optimistic carrots keep a smiling face!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description uses the template structure but provides no substantive details: Related Issues, Testing, Screenshots, and Additional Notes are all empty, lacking essential context about the changes. Add a clear description of the optimistic update implementation, link related issues, document testing steps for the delete and close workspace behaviors, and explain the rollback mechanism on errors.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: implementing optimistic updates for workspace deletion, clearly summarizing the primary focus of the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (2)

3-19: Consider extracting shared context type to reduce duplication.

The CloseContext type is structurally identical to DeleteContext in useDeleteWorkspace.ts. Both files also share nearly identical onMutate and onError logic.

Consider extracting the shared type and optimistic update logic into a common utility:

🔎 Suggested shared utility extraction
// workspaceOptimisticUtils.ts
export type WorkspaceOptimisticContext = {
  previousGrouped: ReturnType<
    typeof trpc.useUtils
  >["workspaces"]["getAllGrouped"]["getData"] extends () => infer R
    ? R
    : never;
  previousAll: ReturnType<
    typeof trpc.useUtils
  >["workspaces"]["getAll"]["getData"] extends () => infer R
    ? R
    : never;
  previousActive: ReturnType<
    typeof trpc.useUtils
  >["workspaces"]["getActive"]["getData"] extends () => infer R
    ? R
    : never;
};

export async function cancelWorkspaceQueries(utils: ReturnType<typeof trpc.useUtils>) {
  await Promise.all([
    utils.workspaces.getAll.cancel(),
    utils.workspaces.getAllGrouped.cancel(),
    utils.workspaces.getActive.cancel(),
  ]);
}

export function rollbackWorkspaceCache(
  utils: ReturnType<typeof trpc.useUtils>,
  context: WorkspaceOptimisticContext | undefined
) {
  if (context?.previousGrouped !== undefined) {
    utils.workspaces.getAllGrouped.setData(undefined, context.previousGrouped);
  }
  if (context?.previousAll !== undefined) {
    utils.workspaces.getAll.setData(undefined, context.previousAll);
  }
  if (context?.previousActive !== undefined) {
    utils.workspaces.getActive.setData(undefined, context.previousActive);
  }
}

125-133: Consider parallelizing invalidations for better performance.

The onSuccess handler awaits invalidations sequentially. Since these queries are independent, they can run in parallel.

🔎 Suggested parallel invalidation
 onSuccess: async (...args) => {
   // Invalidate to ensure consistency with backend state
-  await utils.workspaces.invalidate();
-  // Invalidate project queries since close updates project metadata
-  await utils.projects.getRecents.invalidate();
+  await Promise.all([
+    utils.workspaces.invalidate(),
+    // Invalidate project queries since close updates project metadata
+    utils.projects.getRecents.invalidate(),
+  ]);

   // Call user's onSuccess if provided
   await options?.onSuccess?.(...args);
 },
apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (1)

3-109: Near-duplicate optimistic update logic with useCloseWorkspace.

This file shares ~95% of its code with useCloseWorkspace.ts. The DeleteContext type, onMutate, and onError handlers are functionally identical. As suggested in the other file, consider extracting shared logic.

The implementation itself is correct - the optimistic update pattern is well-structured.

plans/optimistic-workspace-delete.md (1)

1-140: Consider removing or archiving this plan file.

This planning document has served its purpose - the implementation is complete. Keeping outdated plan files in the repository can cause confusion when they drift from the actual implementation (as already seen with the active workspace handling).

Consider:

  1. Moving to a docs/adr/ directory as an Architecture Decision Record
  2. Removing from the codebase entirely
  3. Updating to reflect the actual implementation
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7703f98 and 1f1feba.

📒 Files selected for processing (3)
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
  • plans/optimistic-workspace-delete.md
🧰 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 in src/lib/trpc
Use alias as defined in tsconfig.json when 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/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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 using any type - 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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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). (6)
  • GitHub Check: Deploy Marketing
  • GitHub Check: Deploy API
  • GitHub Check: Deploy Admin
  • GitHub Check: Deploy Web
  • GitHub Check: Deploy Docs
  • GitHub Check: Build
🔇 Additional comments (6)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (2)

33-109: Optimistic update implementation looks correct.

The logic properly:

  1. Cancels outgoing queries to prevent race conditions
  2. Snapshots previous state for rollback
  3. Filters workspace from both grouped and flat caches
  4. Handles active workspace switching with recency-based selection
  5. Returns context for rollback

One minor note: at line 95, worktree: null is set with a comment that it will be populated on invalidate. This is acceptable since the optimistic state is temporary.


110-124: Rollback logic is correct but has subtle difference from plan.

The rollback checks context?.previousGrouped !== undefined which correctly handles cases where previousGrouped might be undefined (no cached data). This is an improvement over checking just context?.previousGrouped which would fail to restore if the previous value was falsy.

apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (2)

110-124: Rollback implementation is correct.

The error handler properly restores all three cache states using the !== undefined check pattern, which handles falsy values correctly.


125-131: Add utils.projects.getRecents.invalidate() to useDeleteWorkspace onSuccess.

Workspace deletion updates project metadata (sets tabOrder: null when the last workspace is deleted, hiding the project). Since useCloseWorkspace invalidates projects.getRecents for the same reason (updating project metadata), useDeleteWorkspace should do the same for consistency.

plans/optimistic-workspace-delete.md (2)

63-66: Implementation improved upon this plan.

The actual implementation in useDeleteWorkspace.ts (lines 69-104) is more sophisticated than this plan - it selects the next workspace by recency rather than just clearing the active state. This provides a better UX by avoiding a "no workspace" flash.


83-85: Implement error toast notification for failed workspace deletion.

Section 5 of the plan ("Add error handling UI") explicitly requires showing a toast notification when deletion fails. Currently the error toast is commented out in useDeleteWorkspace.ts, and the test scenarios expect this feedback to be displayed when deletion fails. Without it, users won't understand why the workspace reappeared after attempting to delete it.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 6, 2026

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Fly.io Electric (Fly.io) View App
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

@AviPeltz AviPeltz merged commit 57bd020 into main Jan 7, 2026
11 of 12 checks passed
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (2)

3-139: Extract shared optimistic update logic to eliminate duplication.

This file duplicates ~95% of the code from useCloseWorkspace.ts. The entire optimistic update pattern (context type, onMutate, onError) is identical between close and delete operations. This violates DRY and creates maintenance burden—any bug fix or enhancement must be applied twice.

Extract a shared helper function that implements the common optimistic update logic, then call it from both hooks.

🔎 Recommended refactor to extract shared logic

Create a new file apps/desktop/src/renderer/react-query/workspaces/optimisticWorkspaceUpdate.ts:

import { trpc } from "renderer/lib/trpc";

export type WorkspaceUpdateContext = {
	previousGrouped: ReturnType<
		typeof trpc.useUtils
	>["workspaces"]["getAllGrouped"]["getData"] extends () => infer R
		? R
		: never;
	previousAll: ReturnType<
		typeof trpc.useUtils
	>["workspaces"]["getAll"]["getData"] extends () => infer R
		? R
		: never;
	previousActive: ReturnType<
		typeof trpc.useUtils
	>["workspaces"]["getActive"]["getData"] extends () => infer R
		? R
		: never;
};

export function createOptimisticWorkspaceUpdate(
	utils: ReturnType<typeof trpc.useUtils>,
) {
	return {
		async onMutate({ id }: { id: string }) {
			// Cancel outgoing refetches
			await Promise.all([
				utils.workspaces.getAll.cancel(),
				utils.workspaces.getAllGrouped.cancel(),
				utils.workspaces.getActive.cancel(),
			]);

			// Snapshot previous values
			const previousGrouped = utils.workspaces.getAllGrouped.getData();
			const previousAll = utils.workspaces.getAll.getData();
			const previousActive = utils.workspaces.getActive.getData();

			// Optimistically remove workspace from caches
			if (previousGrouped) {
				utils.workspaces.getAllGrouped.setData(
					undefined,
					previousGrouped
						.map((group) => ({
							...group,
							workspaces: group.workspaces.filter((w) => w.id !== id),
						}))
						.filter((group) => group.workspaces.length > 0),
				);
			}

			if (previousAll) {
				utils.workspaces.getAll.setData(
					undefined,
					previousAll.filter((w) => w.id !== id),
				);
			}

			// Handle active workspace switching
			if (previousActive?.id === id) {
				const remainingWorkspaces = previousAll
					?.filter((w) => w.id !== id)
					.sort((a, b) => b.lastOpenedAt - a.lastOpenedAt);

				if (remainingWorkspaces && remainingWorkspaces.length > 0) {
					const nextWorkspace = remainingWorkspaces[0];
					const projectGroup = previousGrouped?.find((g) =>
						g.workspaces.some((w) => w.id === nextWorkspace.id),
					);
					const workspaceFromGrouped = projectGroup?.workspaces.find(
						(w) => w.id === nextWorkspace.id,
					);

					if (projectGroup && workspaceFromGrouped) {
						const worktreeData =
							workspaceFromGrouped.type === "worktree"
								? {
										branch: nextWorkspace.branch,
										baseBranch: null,
										gitStatus: {
											branch: nextWorkspace.branch,
											needsRebase: false,
											lastRefreshed: Date.now(),
										},
									}
								: null;

						utils.workspaces.getActive.setData(undefined, {
							...nextWorkspace,
							type: workspaceFromGrouped.type,
							worktreePath: workspaceFromGrouped.worktreePath,
							project: {
								id: projectGroup.project.id,
								name: projectGroup.project.name,
								mainRepoPath: projectGroup.project.mainRepoPath,
							},
							worktree: worktreeData,
						});
					} else {
						utils.workspaces.getActive.setData(undefined, null);
					}
				} else {
					utils.workspaces.getActive.setData(undefined, null);
				}
			}

			return { previousGrouped, previousAll, previousActive };
		},

		onError(
			_err: Error,
			_variables: { id: string },
			context: WorkspaceUpdateContext | undefined,
		) {
			console.error("[workspaces] Optimistic update failed, rolling back:", _err);
			if (context?.previousGrouped !== undefined) {
				utils.workspaces.getAllGrouped.setData(
					undefined,
					context.previousGrouped,
				);
			}
			if (context?.previousAll !== undefined) {
				utils.workspaces.getAll.setData(undefined, context.previousAll);
			}
			if (context?.previousActive !== undefined) {
				utils.workspaces.getActive.setData(undefined, context.previousActive);
			}
		},
	};
}

Then simplify both useDeleteWorkspace.ts and useCloseWorkspace.ts:

import { trpc } from "renderer/lib/trpc";
import { createOptimisticWorkspaceUpdate } from "./optimisticWorkspaceUpdate";

export function useDeleteWorkspace(
	options?: Parameters<typeof trpc.workspaces.delete.useMutation>[0],
) {
	const utils = trpc.useUtils();
	const optimistic = createOptimisticWorkspaceUpdate(utils);

	return trpc.workspaces.delete.useMutation({
		...options,
		onMutate: async (variables) => {
			const context = await optimistic.onMutate(variables);
			await options?.onMutate?.(variables);
			return context;
		},
		onError: (err, variables, context) => {
			optimistic.onError(err, variables, context);
			options?.onError?.(err, variables, context);
		},
		onSuccess: async (...args) => {
			await utils.workspaces.invalidate();
			await options?.onSuccess?.(...args);
		},
	});
}

As per coding guidelines (DRY principle, avoid duplicate code).


140-146: Add projects.getRecents invalidation to maintain cache consistency with backend changes.

Both workspace deletion and closing modify project metadata (setting tabOrder: null when no workspaces remain). The useCloseWorkspace hook invalidates projects.getRecents for this reason, but useDeleteWorkspace does not. This inconsistency can leave stale project data in the frontend after deletion.

Update the onSuccess handler to also invalidate:

await utils.projects.getRecents.invalidate();
🧹 Nitpick comments (3)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (3)

69-120: Extract the active workspace switching logic to reduce complexity.

This 50-line block handles active workspace switching with deep nesting (4+ levels). The logic is correct but difficult to follow and maintain. Extract it to a separate helper function like getNextActiveWorkspace({ previousActive, previousAll, previousGrouped, closedId }).

🔎 Recommended refactor to extract helper function

At the module level, add:

function getNextActiveWorkspace({
	previousActive,
	previousAll,
	previousGrouped,
	closedId,
	utils,
}: {
	previousActive: CloseContext["previousActive"];
	previousAll: CloseContext["previousAll"];
	previousGrouped: CloseContext["previousGrouped"];
	closedId: string;
	utils: ReturnType<typeof trpc.useUtils>;
}) {
	if (previousActive?.id !== closedId) {
		return; // Not closing active workspace, no action needed
	}

	const remainingWorkspaces = previousAll
		?.filter((w) => w.id !== closedId)
		.sort((a, b) => b.lastOpenedAt - a.lastOpenedAt);

	if (!remainingWorkspaces || remainingWorkspaces.length === 0) {
		utils.workspaces.getActive.setData(undefined, null);
		return;
	}

	const nextWorkspace = remainingWorkspaces[0];
	const projectGroup = previousGrouped?.find((g) =>
		g.workspaces.some((w) => w.id === nextWorkspace.id),
	);
	const workspaceFromGrouped = projectGroup?.workspaces.find(
		(w) => w.id === nextWorkspace.id,
	);

	if (!projectGroup || !workspaceFromGrouped) {
		utils.workspaces.getActive.setData(undefined, null);
		return;
	}

	const worktreeData =
		workspaceFromGrouped.type === "worktree"
			? {
					branch: nextWorkspace.branch,
					baseBranch: null,
					gitStatus: {
						branch: nextWorkspace.branch,
						needsRebase: false,
						lastRefreshed: Date.now(),
					},
				}
			: null;

	utils.workspaces.getActive.setData(undefined, {
		...nextWorkspace,
		type: workspaceFromGrouped.type,
		worktreePath: workspaceFromGrouped.worktreePath,
		project: {
			id: projectGroup.project.id,
			name: projectGroup.project.name,
			mainRepoPath: projectGroup.project.mainRepoPath,
		},
		worktree: worktreeData,
	});
}

Then replace lines 69-120 with:

-			// If closing the active workspace, switch to another workspace optimistically
-			// This prevents a flash of "no workspace" state while the backend processes
-			if (previousActive?.id === id) {
-				// Find the next workspace to switch to (matches backend logic: most recently opened)
-				const remainingWorkspaces = previousAll
-					?.filter((w) => w.id !== id)
-					.sort((a, b) => b.lastOpenedAt - a.lastOpenedAt);
-
-				if (remainingWorkspaces && remainingWorkspaces.length > 0) {
-					const nextWorkspace = remainingWorkspaces[0];
-					// Find the project info for the next workspace from grouped data
-					const projectGroup = previousGrouped?.find((g) =>
-						g.workspaces.some((w) => w.id === nextWorkspace.id),
-					);
-					const workspaceFromGrouped = projectGroup?.workspaces.find(
-						(w) => w.id === nextWorkspace.id,
-					);
-
-					if (projectGroup && workspaceFromGrouped) {
-						// For worktree-type workspaces, provide minimal worktree data to prevent
-						// hasIncompleteInit from triggering the initialization view
-						const worktreeData =
-							workspaceFromGrouped.type === "worktree"
-								? {
-										branch: nextWorkspace.branch,
-										baseBranch: null,
-										gitStatus: {
-											branch: nextWorkspace.branch,
-											needsRebase: false,
-											lastRefreshed: Date.now(),
-										},
-									}
-								: null;
-
-						utils.workspaces.getActive.setData(undefined, {
-							...nextWorkspace,
-							type: workspaceFromGrouped.type,
-							worktreePath: workspaceFromGrouped.worktreePath,
-							project: {
-								id: projectGroup.project.id,
-								name: projectGroup.project.name,
-								mainRepoPath: projectGroup.project.mainRepoPath,
-							},
-							worktree: worktreeData,
-						});
-					} else {
-						// Fallback: just clear it and let invalidate handle it
-						utils.workspaces.getActive.setData(undefined, null);
-					}
-				} else {
-					// No remaining workspaces
-					utils.workspaces.getActive.setData(undefined, null);
-				}
-			}
+			// If closing the active workspace, switch to another workspace optimistically
+			getNextActiveWorkspace({
+				previousActive,
+				previousAll,
+				previousGrouped,
+				closedId: id,
+				utils,
+			});

As per coding guidelines (avoid deep nesting 4+ levels - use early returns, extract functions).


88-99: Extract magic values to named constants.

The worktree data construction contains several hardcoded values (baseBranch: null, needsRebase: false) that represent a "minimal initialization state". Extract these to named constants at the module top for clarity and maintainability.

🔎 Suggested refactor

At the module top, add:

const MINIMAL_WORKTREE_STATE = {
	baseBranch: null,
	needsRebase: false,
} as const;

Then update the worktreeData construction:

 const worktreeData =
   workspaceFromGrouped.type === "worktree"
     ? {
         branch: nextWorkspace.branch,
-        baseBranch: null,
+        baseBranch: MINIMAL_WORKTREE_STATE.baseBranch,
         gitStatus: {
           branch: nextWorkspace.branch,
-          needsRebase: false,
+          needsRebase: MINIMAL_WORKTREE_STATE.needsRebase,
           lastRefreshed: Date.now(),
         },
       }
     : null;

As per coding guidelines (extract magic numbers and hardcoded values to named constants at module top).


125-139: Add error logging for debugging.

The rollback logic is correct, but the error is not logged. This could make debugging production issues difficult. Add contextual logging to capture when optimistic updates fail.

🔎 Recommended addition
 onError: (_err, _variables, context) => {
+  console.error("[workspaces/close] Optimistic update failed, rolling back:", _err);
   // Rollback to previous state on error
   if (context?.previousGrouped !== undefined) {

As per coding guidelines (use prefixed console logging with context pattern, never swallow errors silently).

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f1feba and 8e30af5.

📒 Files selected for processing (2)
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.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 in src/lib/trpc
Use alias as defined in tsconfig.json when 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/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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 using any type - 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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Biome for formatting and linting - run at root level with bun run lint:fix or biome check --write

Files:

  • apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
  • apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.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). (6)
  • GitHub Check: Deploy Marketing
  • GitHub Check: Deploy Web
  • GitHub Check: Deploy Docs
  • GitHub Check: Deploy API
  • GitHub Check: Deploy Admin
  • GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (4)

33-44: LGTM!

The pattern of canceling in-flight queries and snapshotting previous values is correct and follows React Query best practices for optimistic updates.


140-148: LGTM!

The success handler correctly invalidates queries and chains the user's callback. The pattern is correct.


73-73: No issues found. The lastOpenedAt field is defined as integer("last_opened_at") in the workspace schema and defaults to Date.now(), which returns a number. The sort comparison b.lastOpenedAt - a.lastOpenedAt is valid.


3-19: The conditional type inference pattern is correct and properly handles undefined/null cases.

The pattern successfully extracts the return types from tRPC's getData() method. For each query:

  • getAll and getAllGrouped return arrays or undefined (cache miss)
  • getActive returns the workspace object, null (no active), or undefined (cache miss)

The code correctly handles all three possibilities through existence checks (if (previousGrouped), if (previousActive?.id), and explicit !== undefined guards in the error handler). The explicit cast to CloseContext on line 123 accurately reflects the actual types returned by the queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant