fix(desktop): hot fix for worktree deletion (still need to refactor)#2929
Merged
Conversation
…check Enhance the safety mechanism that prevents deletion of external worktrees by cross-referencing git worktrees with database-tracked worktrees. Previously, the check relied solely on listExternalWorktrees(), which could miss cases where a worktree was incorrectly marked as createdBySuperset. Now the logic: - Gets all git worktrees from the repository - Queries all tracked worktrees from the database for the project - Normalizes paths for accurate comparison (handles symlinks) - Determines if a worktree is external by checking if it exists in git but is NOT tracked in the database This prevents accidental deletion of external worktrees that may have been incorrectly flagged, providing an additional layer of safety. Applied to both workspace.delete and workspace.deleteWorktree procedures.
Contributor
📝 WalkthroughWalkthroughThe workspace/worktree deletion procedures now always enumerate git worktrees and load tracked worktree paths from the local DB, normalize and compare paths, and treat a path as external only if it exists in git and is not tracked in the DB. External worktrees are preserved and a safety event is emitted; otherwise disk removal proceeds. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC as TRPC Router
participant Git as Git (listExternalWorktrees)
participant DB as LocalDB
participant FS as Filesystem
participant EE as EventEmitter
Client->>TRPC: request deleteWorktree / workspace.delete
TRPC->>Git: listExternalWorktrees(project.mainRepoPath)
TRPC->>DB: select tracked worktrees for project
TRPC->>TRPC: normalize paths, compute isActuallyExternal
alt isActuallyExternal == true
TRPC->>EE: emit worktree_delete_safety_trigger(reason: "untracked_worktree_detected")
TRPC->>Client: respond preserved (no disk delete)
else isActuallyExternal == false
TRPC->>FS: remove worktree disk (and teardown path if present)
TRPC->>DB: delete worktree record
TRPC->>Client: respond success (deleted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
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.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
277-299: Extract duplicated “actually external” classification into a shared helper.The same query/normalize/classify logic now exists in two deletion flows. Consolidating it will reduce drift risk and keep future safety fixes consistent.
♻️ Proposed refactor
+const getTrackedWorktreePathSet = (projectId: string): Set<string> => { + const trackedWorktrees = localDb + .select({ path: worktrees.path }) + .from(worktrees) + .where(eq(worktrees.projectId, projectId)) + .all(); + return new Set(trackedWorktrees.map((wt) => normalizePath(wt.path))); +}; + +const isActuallyExternalWorktree = async ({ + mainRepoPath, + projectId, + worktreePath, +}: { + mainRepoPath: string; + projectId: string; + worktreePath: string; +}): Promise<boolean> => { + const allGitWorktrees = await listExternalWorktrees(mainRepoPath); + const trackedPaths = getTrackedWorktreePathSet(projectId); + const worktreePathNorm = normalizePath(worktreePath); + const existsInGit = allGitWorktrees.some( + (wt) => normalizePath(wt.path) === worktreePathNorm, + ); + return existsInGit && !trackedPaths.has(worktreePathNorm); +};-// Get all git worktrees -const allGitWorktrees = await listExternalWorktrees(project.mainRepoPath); -// Get all tracked worktree paths from database for this project -const trackedWorktrees = localDb - .select({ path: worktrees.path }) - .from(worktrees) - .where(eq(worktrees.projectId, project.id)) - .all(); -const trackedPaths = new Set( - trackedWorktrees.map((wt) => normalizePath(wt.path)), -); -const worktreePathNorm = normalizePath(worktree.path); -const existsInGit = allGitWorktrees.some( - (wt) => normalizePath(wt.path) === worktreePathNorm, -); -const isActuallyExternal = - existsInGit && !trackedPaths.has(worktreePathNorm); +const isActuallyExternal = await isActuallyExternalWorktree({ + mainRepoPath: project.mainRepoPath, + projectId: project.id, + worktreePath: worktree.path, +});Also applies to: 510-531
🤖 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 277 - 299, Duplicate logic that determines whether a worktree is "actually external" should be extracted into a single helper to avoid drift: create a function (e.g., isWorktreeActuallyExternal or classifyExternalWorktree) that accepts the repository path, the worktree path, and the DB instance, calls listExternalWorktrees(projectMainRepoPath), normalizes paths with normalizePath, queries tracked paths from the worktrees table (using localDb.select(...).from(worktrees).where(eq(worktrees.projectId, ...)).all()), and returns a boolean (or classification) matching the existing isActuallyExternal logic; then replace the duplicated blocks (including the occurrence around isActuallyExternal and the similar block at lines ~510-531) with calls to this new helper (keep references to listExternalWorktrees, normalizePath, tracked worktrees query, and isActuallyExternal semantics).
🤖 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 277-299: Duplicate logic that determines whether a worktree is
"actually external" should be extracted into a single helper to avoid drift:
create a function (e.g., isWorktreeActuallyExternal or classifyExternalWorktree)
that accepts the repository path, the worktree path, and the DB instance, calls
listExternalWorktrees(projectMainRepoPath), normalizes paths with normalizePath,
queries tracked paths from the worktrees table (using
localDb.select(...).from(worktrees).where(eq(worktrees.projectId, ...)).all()),
and returns a boolean (or classification) matching the existing
isActuallyExternal logic; then replace the duplicated blocks (including the
occurrence around isActuallyExternal and the similar block at lines ~510-531)
with calls to this new helper (keep references to listExternalWorktrees,
normalizePath, tracked worktrees query, and isActuallyExternal semantics).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6980f276-1d12-480d-aaa3-47c8b18f2c58
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
…uperset Update deletion logic to remove all worktrees from disk when deleted through Superset, regardless of whether they were created by Superset or imported as external worktrees. Previously: - Superset-created worktrees (createdBySuperset=true) → deleted from disk - External worktrees (createdBySuperset=false) → only removed from database Now: - All worktrees tracked in Superset → deleted from disk when removed - Safety check still prevents deletion of untracked worktrees This provides consistent UX: once a worktree is managed in Superset (even if originally created externally), deleting it removes it completely. The safety mechanism still protects against edge cases where a worktree exists in git but is not properly tracked in the database. Updated both workspace.delete and workspace.deleteWorktree procedures.
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 2 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:295">
P0: This external-detection condition misclassifies imported external worktrees as safe-to-delete, which can delete user-managed worktree directories from disk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+295
to
+296
| const isActuallyExternal = | ||
| existsInGit && !trackedPaths.has(worktreePathNorm); |
Contributor
There was a problem hiding this comment.
P0: This external-detection condition misclassifies imported external worktrees as safe-to-delete, which can delete user-managed worktree directories from disk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts, line 295:
<comment>This external-detection condition misclassifies imported external worktrees as safe-to-delete, which can delete user-managed worktree directories from disk.</comment>
<file context>
@@ -267,43 +270,51 @@ export const createDeleteProcedures = () => {
+ const existsInGit = allGitWorktrees.some(
+ (wt) => normalizePath(wt.path) === worktreePathNorm,
+ );
+ const isActuallyExternal =
+ existsInGit && !trackedPaths.has(worktreePathNorm);
+
</file context>
Suggested change
| const isActuallyExternal = | |
| existsInGit && !trackedPaths.has(worktreePathNorm); | |
| const isActuallyExternal = | |
| !worktree.createdBySuperset || | |
| (existsInGit && !trackedPaths.has(worktreePathNorm)); |
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
Enhances the safety mechanism that prevents accidental deletion of external worktrees by implementing a more robust detection algorithm that cross-references git worktrees with database-tracked worktrees.
Problem
The previous implementation relied solely on
listExternalWorktrees(), which could miss edge cases where a worktree was incorrectly marked ascreatedBySupersetin the database. This created a risk of accidentally deleting user-managed external worktrees.Solution
The improved detection logic now:
listExternalWorktrees()normalizePath())A worktree is considered "actually external" only if it exists in git but is not tracked in our DB, regardless of the
createdBySupersetflag.Changes
workspace.deleteprocedure to use enhanced external worktree detectionworkspace.deleteWorktreeprocedure with the same safety checkTest Plan
createdBySuperset(should preserve)Related
This addresses the workspace deletion safety concerns raised in previous issues around external worktree preservation.
Summary by cubic
Improves deletion safety by cross-checking git worktrees with DB-tracked paths, and deletes all Superset-tracked worktrees from disk on removal. Applies to both
workspace.deleteandworkspace.deleteWorktree.Bug Fixes
Dependencies
@superset/desktopto1.4.3inbun.lock.Written for commit c45408e. Summary will update on new commits.
Summary by CodeRabbit