Conversation
WalkthroughThis PR implements a safe workspace deletion flow by adding backend canDelete validation to the workspaces TRPC router, creating a DeleteWorkspaceDialog React component with pre-deletion checks, and introducing reusable alert-dialog UI components. The backend validates Git worktree state before deletion; the frontend presents warnings and prevents deletion when unsafe. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Dialog as DeleteWorkspaceDialog
participant TRPC as TRPC Router
participant Git as Git/Worktree
participant DB as Database
User->>Dialog: Opens dialog
Dialog->>TRPC: canDelete (validation)
TRPC->>Git: Check worktree exists (dry-run)
alt Git check succeeds
Git-->>TRPC: Worktree status
alt Worktree found
TRPC-->>Dialog: canDelete=true
else Worktree not found
TRPC-->>Dialog: canDelete=true, warning
end
else Git check fails
Git-->>TRPC: Error
TRPC-->>Dialog: canDelete=false, reason
end
alt Deletion allowed
User->>Dialog: Confirm delete
Dialog->>TRPC: delete (mutation)
TRPC->>Git: removeWorktree
alt Removal succeeds
Git-->>TRPC: Success
TRPC->>DB: Remove workspace/worktree records
DB-->>TRPC: Success
TRPC-->>Dialog: Deletion complete
Dialog->>Dialog: Close dialog
else Removal fails
Git-->>TRPC: Error
TRPC-->>Dialog: Failure, DB unchanged
end
else Deletion not allowed
Dialog->>Dialog: Show reason, disable delete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (6)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
199-263: canDelete flow looks solid; consider tightening worktree detection and warning textThe overall control flow and error handling for
canDeletelook good and match the intended UX, but there are a couple of small polish opportunities:
worktrees.includes(worktree.path)may give false positives if another worktree path is a superset of this one. A more robust check would splitworktreesinto lines and compare the path segment explicitly (e.g.,some(line => line.startsWith(worktree.path + " "))).- In the final
return(Line 257+) you always use the warning"No associated worktree found"even when the project is missing but aworktreeIdexists, or vice versa. If you care about messaging accuracy, consider tailoring this to “No associated worktree or project found” or similar when only one side is present.
281-325: Align delete semantics with canDelete for missing/invalid worktreesThe updated
deletemutation correctly treats worktree-removal failures as fatal and avoids DB cleanup, which is a good safety improvement. One edge case to watch:
canDeletecan returncanDelete: truewith a warning when the worktree is not found in git.- If
removeWorktreelater throws for the same “not found” condition,deletewill now fail and leave the workspace + DB entries intact.To keep behavior consistent between
canDeleteanddelete, consider treating “worktree not found / already removed” errors fromremoveWorktreeas a non-fatal no-op (e.g., by inspecting the error message or by makingremoveWorktreeitself tolerant of that case) so deletion proceeds whencanDeletehas already okayed it.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
5-10: Verify Bun’smock.modulebehavior with named imports and import timingThe tests rely heavily on
mock.moduleto replace:
"./utils/git"(includingremoveWorktree) and"simple-git"(used via dynamic import insidecanDelete).Given that
createWorkspacesRouterimports{ createWorktree, removeWorktree, generateBranchName }as named exports from"./utils/git"at module load time, and those imports happen before thesemock.modulecalls run, it’s not obvious that the router’s bindings will ever see your mocks—named imports are usually fixed references.Similarly, the dynamic import of
"simple-git"insidecanDeleteshould be patchable, but it’s worth confirming that Bun’smock.module("simple-git", ...)intercepts that path as expected.Please double-check Bun’s mocking semantics here. If named imports aren’t updated by
mock.modulepost-import, you may need to:
- Apply the mocks before importing
createWorkspacesRouter, or- Change the router to use a star import (
* as gitUtils) and reference functions as properties (so swapping the module object works), or- Use dependency injection for git helpers in tests.
Also applies to: 60-66, 106-114, 131-140, 155-161, 175-181
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (2)
32-35: Guard against canDelete query errors enabling deletion by defaultRight now:
const canDelete = canDeleteData?.canDelete ?? true;- The Delete button is
disabled={!canDelete || isDeleting || isLoading}.While loading,
isLoadingkeeps the button disabled, which is fine. But if thecanDeletequery fails (e.g. network/TRPC error) and throws instead of returning data,canDeleteDatawill beundefined,isLoadingwill befalse, andcanDeletewill fall back totrue, enabling the Delete action without a validated status.To make this safer and clearer, consider:
- Defaulting
canDeletetofalsewhendatais undefined, and- Reading the query’s
error/isErrorflag to render an “Error checking workspace status” message and keep the Delete action disabled in that case.This would ensure you only allow deletion when you have a positive canDelete signal from the server.
Also applies to: 49-52, 59-79, 88-92
84-87: Avoid relying onReact.MouseEventwithout an explicit React type importThe
onClickhandler currently annotates the event asReact.MouseEvent, but this file only importsuseStatefrom"react". Depending on your TypeScript and@types/reactsetup,Reactmay not be available as a global namespace, leading to aCannot find namespace 'React'error.You can simplify and avoid this by either:
- Dropping the explicit type and letting TS infer it:
<AlertDialogAction onClick={(e) => { e.preventDefault(); handleDelete(); }} // ... >or
- Importing the type explicitly:
import { useState, type MouseEvent } from "react"; // ... onClick={(e: MouseEvent) => { /* ... */ }}apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
37-38: Update aria-label to reflect destructive delete actionThe new flow of opening
DeleteWorkspaceDialogvia the X button is a nice UX improvement, and the click handler correctly stops propagation so it doesn’t interfere with activation/drag.One small a11y nit: the button still uses
aria-label="Close workspace"even though it now initiates a delete-confirmation flow. For screen readers this can be misleading; consider changing it to something like"Delete workspace"to match the destructive semantics and styling.Also applies to: 65-67, 96-111, 114-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(3 hunks)packages/ui/package.json(3 hunks)packages/ui/src/alert-dialog.tsx(1 hunks)packages/ui/src/components/alert-dialog.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
createWorkspacesRouter(12-386)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
removeWorktree(68-81)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
packages/ui/src/components/alert-dialog.tsx (8)
AlertDialog(144-144)AlertDialogContent(148-148)AlertDialogHeader(149-149)AlertDialogTitle(151-151)AlertDialogDescription(152-152)AlertDialogFooter(150-150)AlertDialogCancel(154-154)AlertDialogAction(153-153)
packages/ui/src/components/alert-dialog.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(4-6)packages/ui/src/components/button.tsx (1)
buttonVariants(61-61)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(4-6)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
DeleteWorkspaceDialog(22-97)
🔇 Additional comments (9)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
56-124: Good coverage of delete and canDelete pathsThe tests exercise the main success and failure scenarios for
deleteandcanDelete:
- Successful deletion with worktree removal and DB cleanup.
- Failed worktree removal leaving DB untouched.
canDeletesuccess, warning, and git-error paths.Once the DB and mocking concerns above are addressed, this suite should provide solid regression coverage for the new behavior.
Also applies to: 127-190
packages/ui/src/alert-dialog.tsx (1)
1-1: Barrel re-export for alert-dialog looks goodThe top-level
export * from "./components/alert-dialog";is a clean way to expose the alert dialog primitives via@superset/ui/alert-dialogand keeps the public surface centralized.packages/ui/package.json (2)
7-7: LGTM! Export path is correctly configured.The export entry follows the established pattern and correctly points to the new alert-dialog component.
33-33: Dependencies verified as valid and secure.Both package versions are published on npm with no known security vulnerabilities: @radix-ui/react-alert-dialog@1.1.15 and @radix-ui/react-slot@1.2.4.
packages/ui/src/components/alert-dialog.tsx (5)
1-5: LGTM! Imports are clean and follow established patterns.The imports correctly reference the Radix UI primitive and local utilities.
7-27: LGTM! Simple wrapper components follow consistent patterns.These pass-through wrappers correctly add data-slot attributes while maintaining type safety and prop forwarding.
29-62: LGTM! Overlay and Content components use appropriate modal patterns.The composition in
AlertDialogContent(Portal > Overlay > Content) follows standard modal design patterns, and the animations/positioning are correctly implemented with Tailwind utilities.
64-117: LGTM! Layout and text components implement responsive, accessible patterns.The responsive design (mobile-stacked, desktop-horizontal) and semantic structure using Radix primitives ensure good UX and accessibility.
119-155: LGTM! Action buttons correctly apply variant styling and all exports are properly defined.The action components appropriately use
buttonVariants()with default and outline variants to differentiate primary/secondary actions, and all 11 components are cleanly exported.
| const mockDb = { | ||
| data: { | ||
| workspaces: [ | ||
| { | ||
| id: "workspace-1", | ||
| projectId: "project-1", | ||
| worktreeId: "worktree-1", | ||
| name: "Test Workspace", | ||
| tabOrder: 0, | ||
| createdAt: Date.now(), | ||
| updatedAt: Date.now(), | ||
| lastOpenedAt: Date.now(), | ||
| }, | ||
| ], | ||
| worktrees: [ | ||
| { | ||
| id: "worktree-1", | ||
| projectId: "project-1", | ||
| path: "/path/to/worktree", | ||
| branch: "test-branch", | ||
| createdAt: Date.now(), | ||
| }, | ||
| ], | ||
| projects: [ | ||
| { | ||
| id: "project-1", | ||
| name: "Test Project", | ||
| mainRepoPath: "/path/to/repo", | ||
| color: "#ff0000", | ||
| tabOrder: 0, | ||
| createdAt: Date.now(), | ||
| lastOpenedAt: Date.now(), | ||
| }, | ||
| ], | ||
| settings: { | ||
| lastActiveWorkspaceId: "workspace-1", | ||
| }, | ||
| }, | ||
| update: mock(async (fn: (data: typeof mockDb.data) => void) => { | ||
| fn(mockDb.data); | ||
| }), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Router under test doesn’t actually use mockDb
In these tests you construct mockDb and pass it via router.createCaller({ db: mockDb as any }), then assert against mockDb.data. However, createWorkspacesRouter imports and uses the db singleton from "main/lib/db" directly (e.g., db.data.workspaces, db.update(...)) rather than ctx.db.
Unless "main/lib/db" is being mocked or initialized elsewhere to point at mockDb, these tests will not exercise mockDb at all and may either:
- operate against the real in-memory DB (making the assertions on
mockDb.datameaningless), or - throw “Database not initialized” if
_dbisn’t set.
I’d recommend either:
- Refactoring the router to read
ctx.dbinstead of the importeddb, so the context you pass tocreateCalleris actually used; or - Mocking
"main/lib/db"in this test to export{ db: mockDb }so the router’s importeddbis your mock.
I need to verify the concerns raised in the review comment by examining the actual test file and router implementation. Let me gather the necessary information:
🏁 Script executed:
#!/bin/bash
# Find and examine the test file
find . -path "*/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" -type f | head -1Length of output: 65
🏁 Script executed:
#!/bin/bash
# Locate the workspaces router implementation
find . -path "*/workspaces.ts" -o -path "*/workspaces/index.ts" | grep -E "routers/workspaces" | grep -v test | head -5Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for createWorkspacesRouter definition
rg -n "createWorkspacesRouter|export.*workspaces" --type=ts --type=tsx | grep -v test | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check for mock.module calls targeting "main/lib/db"
rg -n "mock\.module.*main/lib/db|mock\.module.*db" --type=ts --type=tsx | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for the db singleton import/export in main/lib/db
find . -path "*/main/lib/db.ts" -o -path "*/main/lib/db/index.ts" | head -1Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for test file imports and setup
rg -n "import.*db|import.*createCaller|describe|beforeEach" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" -A 2 | head -40Length of output: 513
🏁 Script executed:
#!/bin/bash
# Get the full workspaces router test file
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || echo "File not found, attempting alternative paths..." && find . -name "workspaces.test.ts" -path "*/routers/workspaces/*" -type fLength of output: 5075
🏁 Script executed:
#!/bin/bash
# Examine the actual router implementation (workspaces router)
find . -path "*/routers/workspaces.ts" -o -path "*/routers/workspaces/index.ts" | grep -v test | xargs cat 2>/dev/null | head -100Length of output: 108
🏁 Script executed:
#!/bin/bash
# Search more broadly for createWorkspacesRouter
rg "export.*createWorkspacesRouter|function createWorkspacesRouter|const createWorkspacesRouter" --type=ts --type=tsx -B 2 -A 10 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check db usage pattern in workspaces router - look for ctx.db vs imported db
rg "ctx\.db|from.*main/lib/db" --type=ts --type=tsx | grep -i workspace | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look at db module structure to understand export pattern
find . -path "*/main/lib/db*" -type f | head -10Length of output: 81
🏁 Script executed:
#!/bin/bash
# Search for db.data pattern usage in workspaces context
rg "db\.data\." --type=ts --type=tsx | grep -i workspace | head -10Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find all test setup code including mock.module calls
find . -name "workspaces.test.ts" -path "*/routers/workspaces/*" -exec cat {} \;Length of output: 5009
🏁 Script executed:
#!/bin/bash
# More targeted search for the exact test file with full context
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || cat "./apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || rg "mockDb.*=" --type=ts -B 5 -A 50 | head -100Length of output: 5009
🏁 Script executed:
#!/bin/bash
# Check test file structure with grep
grep -n "import\|mock\.module\|describe\|it\|createCaller" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null | head -50Length of output: 2229
🏁 Script executed:
#!/bin/bash
# Find the workspace router directory and list its contents
find . -type d -path "*/routers/workspaces" | head -1 | xargs ls -laLength of output: 355
🏁 Script executed:
#!/bin/bash
# List files in the test directory
ls -la "apps/desktop/src/lib/trpc/routers/workspaces/" 2>/dev/null || ls -la "./apps/desktop/src/lib/trpc/routers/workspaces/" 2>/dev/null || echo "Attempting find..." && find . -type d -name workspaces -path "*/routers/*"Length of output: 402
🏁 Script executed:
#!/bin/bash
# Direct comprehensive check
if [ -f "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" ]; then
echo "=== TEST FILE ===" && cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts"
else
echo "Finding test file..." && find . -name "workspaces.test.ts" -type f
fiLength of output: 5027
🏁 Script executed:
#!/bin/bash
# Check the router implementation
if [ -f "apps/desktop/src/lib/trpc/routers/workspaces.ts" ]; then
echo "=== ROUTER FILE ===" && cat "apps/desktop/src/lib/trpc/routers/workspaces.ts"
else
echo "Looking for router..." && find . -path "*/routers/workspaces*" -name "*.ts" -not -name "*.test.ts" | head -5
fiLength of output: 198
🏁 Script executed:
#!/bin/bash
# Search for both files systematically
echo "Looking for test file..."
find . -name "workspaces.test.ts" 2>/dev/null | head -3
echo ""
echo "Looking for router file..."
find . -path "*/lib/trpc/routers/workspaces.ts" -o -path "*/lib/trpc/routers/workspaces/index.ts" 2>/dev/null | head -3Length of output: 174
🏁 Script executed:
#!/bin/bash
# Final comprehensive attempt - check what files exist in the target directory
find . -type f -path "*/apps/desktop/src/lib/trpc/routers/workspaces/*" 2>/dev/null | head -20Length of output: 242
🏁 Script executed:
#!/bin/bash
# Read the actual workspaces router implementation
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts"Length of output: 9955
🏁 Script executed:
#!/bin/bash
# Read the db module to understand structure
cat "apps/desktop/src/main/lib/db/index.ts"Length of output: 726
🏁 Script executed:
#!/bin/bash
# Search for ctx.db or imported db usage in workspaces.ts
grep -n "ctx\|db\|import" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts" | head -50Length of output: 1298
Router uses imported db singleton, not ctx.db—mockDb is unused
The review comment is correct. The router definitively imports and uses the db singleton from "main/lib/db" (line 2 of workspaces.ts), accessing it directly via db.data.* and db.update() throughout all resolvers. The router does not accept or use ctx.db at all.
Since mockDb is passed to createCaller({ db: mockDb as any }) but the router never reads it, the tests do not exercise the mock. Instead:
- If
_dbis uninitialized, resolvers will throw "Database not initialized. Call initDb() first." - If
_dbhappens to be set, tests will operate against the real singleton, making assertions onmockDb.datameaningless.
To fix this:
- Mock
"main/lib/db"at the test top-level to export{ db: mockDb }, so the router's import resolves to your mock; or - Refactor the router to accept and use
ctx.dbinstead of the imported singleton.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts around lines
13–54, the test constructs mockDb but the router imports and uses the singleton
from "main/lib/db" so mockDb is never used; fix by mocking the module import at
the top of the test so "main/lib/db" exports { db: mockDb } (ensure the mock
export shape matches the real db API including .data and .update) so the router
resolves to the mock during tests; alternatively, refactor the router to use
ctx.db instead of importing the singleton so tests can pass a db via
createCaller—pick one approach and implement consistent module-mocking or
refactor accordingly.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.