fix(desktop): quickfix restore workspace deletion from disk#2908
Merged
Conversation
Fixes bug introduced in PR #2573 where workspace deletion only removed database records but left worktree files on disk. **Root cause:** - `listExternalWorktrees()` was misnamed and returned ALL git worktrees - Delete logic used this for safety checks, finding the worktree being deleted in the "external" list - This caused disk deletion to be skipped incorrectly **Changes:** 1. Renamed `listExternalWorktrees` → `listAllGitWorktrees` (accurate name) 2. Created proper `listExternalWorktrees(mainRepoPath, projectId)`: - Queries database for tracked worktrees - Returns only git worktrees NOT in database - Provides true "external worktrees" list 3. Removed `createdBySuperset` flag gating from deletion logic: - Users can now delete imported external worktrees - Safety check via `listExternalWorktrees` prevents deleting worktrees that haven't been imported 4. Updated all call sites to pass `projectId` parameter 5. Extracted `normalizePath` helper to git.ts for reuse 6. Simplified git-status.ts to use new implementation **Testing:** - All typechecks pass - Existing tests updated (external-worktree-import.test.ts) - Logic verified: tracked worktrees deleted, external preserved
Contributor
📝 WalkthroughWalkthroughRefactors worktree discovery and matching: splits listing into all-vs-external functions, adds path normalization, changes callers to pass Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Procedure as Workspaces Procedure
participant GitUtils as git utils
participant DB as localDb
participant FS as Filesystem
Client->>Procedure: request (list/import/delete)
Procedure->>GitUtils: listAllGitWorktrees(mainRepoPath)
GitUtils->>FS: run `git worktree list --porcelain`
GitUtils-->>Procedure: allWorktrees
Procedure->>DB: query worktrees where projectId = project.id
DB-->>Procedure: trackedWorktreePaths
Procedure->>GitUtils: listExternalWorktrees(mainRepoPath, project.id)
GitUtils-->>Procedure: externalWorktrees (allWorktrees - trackedPaths, normalized)
alt Deletion requested
Procedure->>FS: remove path (if isActuallyExternal && exists)
FS-->>Procedure: success / error
Procedure-->>Client: { success: bool, error? }
else Import/list requested
Procedure-->>Client: filtered externalWorktrees
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
AviPeltz
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts:259">
P1: Handle `listExternalWorktrees` failures here; otherwise a thrown git error can abort deletion after `markWorkspaceAsDeleting`, leaving the workspace stuck as "deleting".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/workspaces/procedures/external-worktree-import.test.ts (1)
115-132:⚠️ Potential issue | 🟠 MajorAdd test coverage for
listExternalWorktreeswith database filtering logic.The new
listExternalWorktrees(mainRepoPath, projectId)function—which filters worktrees by project ID and usesnormalizePathfor path comparison—lacks test coverage. This function contains the core DB-filtering logic introduced in this PR and should be tested to verify:
- It correctly returns only worktrees NOT in the database for the given projectId
- Path normalization handles edge cases (symlinks, relative paths)
- It filters external worktrees appropriately when some are tracked in the database
Additionally, consider using a static import in the test at line 120 instead of dynamic import to be consistent with other tests in
git.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts` around lines 115 - 132, Add unit tests for listExternalWorktrees to cover the DB-filtering and path-normalization behavior: write a test that uses the existing test helper createExternalWorktree to create multiple external worktrees, seed the test DB with one tracked worktree for the target projectId, then call listExternalWorktrees(mainRepoPath, projectId) and assert it returns only worktrees not present in the DB; add another test to exercise normalizePath edge cases (symlink/relative path variants) to ensure comparisons treat equivalent paths as equal; also replace the dynamic import of listAllGitWorktrees with a static import (consistent with other tests) and import listExternalWorktrees and listAllGitWorktrees from ../utils/git so tests verify both listing and filtered results.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts (1)
866-880: RedundanttrackedPathscheck after API update.
listExternalWorktreesnow filters out tracked worktrees internally (by querying the DB forprojectId). The localtrackedPaths.has(wt.path)check at line 878 is now redundant since external worktrees returned by the function are guaranteed not to be in the DB.However, since the filtering uses
normalizePathinternally but this check compares raw paths, keeping it provides a secondary safety net for edge cases where path formats differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around lines 866 - 880, The local trackedPaths.has(wt.path) check in the externalWorktrees filter is redundant because listExternalWorktrees already excludes DB-tracked worktrees; remove the trackedPaths set and the trackedPaths.has(wt.path) branch from the filter in the block that builds externalWorktrees (referencing listExternalWorktrees, projectWorktrees, externalWorktrees) so you rely on the updated API behavior and avoid duplicate logic—if you want an extra safety net instead, compare normalized paths using the same normalizePath logic used inside listExternalWorktrees rather than raw wt.path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts`:
- Around line 115-132: Add unit tests for listExternalWorktrees to cover the
DB-filtering and path-normalization behavior: write a test that uses the
existing test helper createExternalWorktree to create multiple external
worktrees, seed the test DB with one tracked worktree for the target projectId,
then call listExternalWorktrees(mainRepoPath, projectId) and assert it returns
only worktrees not present in the DB; add another test to exercise normalizePath
edge cases (symlink/relative path variants) to ensure comparisons treat
equivalent paths as equal; also replace the dynamic import of
listAllGitWorktrees with a static import (consistent with other tests) and
import listExternalWorktrees and listAllGitWorktrees from ../utils/git so tests
verify both listing and filtered results.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 866-880: The local trackedPaths.has(wt.path) check in the
externalWorktrees filter is redundant because listExternalWorktrees already
excludes DB-tracked worktrees; remove the trackedPaths set and the
trackedPaths.has(wt.path) branch from the filter in the block that builds
externalWorktrees (referencing listExternalWorktrees, projectWorktrees,
externalWorktrees) so you rely on the updated API behavior and avoid duplicate
logic—if you want an extra safety net instead, compare normalized paths using
the same normalizePath logic used inside listExternalWorktrees rather than raw
wt.path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae3e4b6b-26b2-4ee9-b3c9-e1cb8d5a4ed3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts
Wraps worktree deletion logic in try-catch to ensure workspace deleting status is properly cleared if listExternalWorktrees() or other operations fail. **Problem:** - If listExternalWorktrees() throws (git error, DB error, etc.), the workspace remains stuck in 'deleting' status - User cannot retry deletion or interact with workspace - Requires manual DB cleanup or app restart to recover **Solution:** - Wrap deletion section in outer try-catch - Catch any errors from listExternalWorktrees and related operations - Always call clearWorkspaceDeletingStatus() on error - Return proper error response instead of crashing **Changes:** - workspace delete procedure: added error boundary around disk deletion - deleteWorktree procedure: added error boundary for consistency **Impact:** - Prevents workspace from getting stuck in deleting state - Provides clear error messages to user - Allows user to retry deletion after fixing issues
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
546-560: Consider more granular error handling for DB operations.If
deleteWorktreeRecordorhideProjectIfNoWorkspacesthrows after successful disk deletion, the catch block returns"Failed to delete worktree"which could be misleading—the worktree may already be removed from disk.This is a minor edge case, but you could consider separating disk and DB error handling for clearer diagnostics:
💡 Optional: More specific error handling
} finally { workspaceInitManager.releaseProjectLock(project.id); } - deleteWorktreeRecord(input.worktreeId); - hideProjectIfNoWorkspaces(worktree.projectId); - - track("worktree_deleted", { worktree_id: input.worktreeId }); - - return { success: true }; + try { + deleteWorktreeRecord(input.worktreeId); + hideProjectIfNoWorkspaces(worktree.projectId); + track("worktree_deleted", { worktree_id: input.worktreeId }); + return { success: true }; + } catch (dbError) { + console.error( + `[worktree/delete] DB cleanup failed after disk deletion:`, + dbError instanceof Error ? dbError.message : String(dbError), + ); + // Return success since disk deletion worked, but log the DB issue + return { success: true, warning: "Worktree deleted but database cleanup encountered an error" }; + } } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts` around lines 546 - 560, Separate disk and DB error handling so failures after disk deletion are reported accurately: keep the existing disk-removal logic and track call as-is, then wrap deleteWorktreeRecord and hideProjectIfNoWorkspaces in their own try/catch (or check their results) so DB errors are logged and returned with a distinct message (e.g., "disk_deleted_db_failed") instead of "Failed to delete worktree"; ensure track("worktree_deleted", ...) is only called after disk removal, and return success:true when disk removal succeeded but include a field or error string describing DB-only failures from deleteWorktreeRecord / hideProjectIfNoWorkspaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Around line 546-560: Separate disk and DB error handling so failures after
disk deletion are reported accurately: keep the existing disk-removal logic and
track call as-is, then wrap deleteWorktreeRecord and hideProjectIfNoWorkspaces
in their own try/catch (or check their results) so DB errors are logged and
returned with a distinct message (e.g., "disk_deleted_db_failed") instead of
"Failed to delete worktree"; ensure track("worktree_deleted", ...) is only
called after disk removal, and return success:true when disk removal succeeded
but include a field or error string describing DB-only failures from
deleteWorktreeRecord / hideProjectIfNoWorkspaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e88349-47c6-4cfd-b58e-3636977a4751
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
3 tasks
AviPeltz
added a commit
that referenced
this pull request
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes critical bug introduced in PR #2573 where workspace deletion only removed database records but left worktree files on disk, causing:
Root Cause
listExternalWorktrees()was misnamed and returned ALL git worktrees instead of only external ones. The delete safety check found the worktree being deleted in this list and incorrectly skipped disk deletion.Changes
listExternalWorktrees→listAllGitWorktrees(accurate name)listExternalWorktrees(mainRepoPath, projectId):createdBySupersetflag gating:listExternalWorktreesprevents deleting untracked worktreesprojectIdparameternormalizePathhelper to git.ts for reuse across modulesTesting
Files Changed
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts- Core changesapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts- Deletion logic fixedapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts- Updated callerapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts- Simplifiedapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts- Updated callerapps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts- Test updatedSummary by cubic
Restores on-disk deletion for Desktop workspaces and adds error handling to avoid stuck “deleting” states. Fix prevents leftover worktree files, frees disk space, and lets users recreate workspaces with the same branch.
listAllGitWorktrees(mainRepoPath)and a properlistExternalWorktrees(mainRepoPath, projectId)that excludes DB-tracked paths.createdBySupersetgating so DB-tracked (including imported) worktrees are deleted from disk; untracked externals are preserved with analytics logging.normalizePathtogit.ts, update call sites to passprojectId, and simplifygit-statusto the new API. Tests updated.Written for commit 4a21fb2. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor