feat(desktop): align v2 delete-workspace dialog with host-service saga#3912
Merged
feat(desktop): align v2 delete-workspace dialog with host-service saga#3912
Conversation
Co-locate the v2 delete dialog's preflight on the same host-service that
runs `destroy`, so they can never disagree about what's blocked vs
warned. Fixes:
- main-workspace path comparison was raw string equality (would fail
open under symlinks / trailing slash / macOS case folding); now
realpath-normalized in a shared isMainWorkspace() helper used by
both inspect and destroy
- hasUnpushedCommits silently missed branches with no upstream;
inspect now uses `rev-list HEAD --not --remotes` which catches
never-pushed branches
- two concurrent destroys could race phase 3b; host-service now
guards with a process-local in-flight Set and surfaces a typed
DELETE_IN_PROGRESS cause distinct from dirty-worktree CONFLICT
- pending-host states (loading, local-starting) used to flicker as a
destructive banner; now render as a disabled "Checking…" button
Audit + decisions captured in plans/20260430-v2-delete-workspace-audit.md.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a host-aware delete preview via a new workspaceCleanup.inspect endpoint, refactors host discovery into a discriminated WorkspaceHostTarget union, returns hostTarget and inspect() from the destroy hook, introduces a process-local concurrent-delete guard that surfaces a typed in-progress error, and updates renderer dialog/state to surface blocking reasons and dynamic labels. Changes
Sequence DiagramsequenceDiagram
participant User as User (UI)
participant Dialog as Delete Dialog (React)
participant Hook as useDestroyWorkspace (Hook)
participant HostSvc as Host Service (tRPC)
participant DB as Database
User->>Dialog: open delete dialog
Dialog->>Hook: init(workspaceId)
Hook->>Hook: resolve hostTarget via useWorkspaceHostTarget
alt hostTarget.status != "ready"
Hook-->>Dialog: hostTarget (not ready)
Dialog->>Dialog: disable confirm / spinner
else host ready
Dialog->>Hook: call inspect()
Hook->>HostSvc: workspaceCleanup.inspect(workspaceId)
HostSvc->>DB: check workspace, git status, rev-list
HostSvc-->>Hook: preview {canDelete, reason, hasChanges, hasUnpushedCommits}
Hook-->>Dialog: show preview
User->>Dialog: confirm (Delete)
Dialog->>Hook: destroy()
Hook->>HostSvc: workspaceCleanup.destroy(workspaceId)
HostSvc->>HostSvc: check destroysInFlight
alt concurrent in-flight
HostSvc-->>Hook: TRPCError(CONFLICT) with DELETE_IN_PROGRESS
Hook-->>Dialog: show in-progress toast
else proceed
HostSvc->>DB: isMainWorkspace validation
HostSvc->>HostSvc: runDestroy phases (local/cloud)
HostSvc->>HostSvc: clear destroysInFlight
HostSvc-->>Hook: success
Hook-->>Dialog: close/update UI
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 2/8 reviews remaining, refill in 41 minutes and 47 seconds.Comment |
Greptile SummaryThis PR co-locates the v2 delete-dialog's preflight ( All remaining findings are P2: Confidence Score: 5/5Safe to merge; all findings are P2 style/dead-code concerns with no present correctness impact. The core logic is well-structured and the described bugs (symlink path comparison, no-upstream branches, concurrent destroys, pending-host UX) are each correctly fixed. All four inline comments are P2: dead code that never fires, a potential effect re-fire on reference change, a type gap that the current server never exercises, and redundant DB queries. None represent a present defect on the changed path. useDestroyDialogState.ts — dead conflictWasRaced logic and effect dependency on mutable function reference.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts | Core destroy/inspect router; adds process-local in-flight guard, extracts runDestroy, and co-locates inspect with destroy. Double DB fetch between isMainWorkspace and runDestroy is the one inefficiency. |
| packages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.ts | New shared helper for main-workspace detection; uses realpathSync normalization with resolve fallback, and checks both local path equality and cloud type. Clean and correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts | Migrates preflight from IPC to host-service inspect; dead conflictWasRaced logic, effect re-fire on hostTarget reference change, and canDelete/reason type gap noted. |
| apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.ts | Adds inspect callback and host-unavailable error variant; isDestroyWorkspaceError guard correctly short-circuits already-normalized errors before normalizeError re-wraps them. |
| apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts | Promotes string |
| packages/host-service/src/trpc/error-types.ts | Adds DeleteInProgressCause type and type-guard; clean, mirrors existing TeardownFailureCause pattern. |
| packages/host-service/src/trpc/index.ts | Wires deleteInProgress onto the TRPC error shape alongside the existing teardownFailure; straightforward extension of the formatter. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx | Adds blockingReason banner and confirmLabel prop; button correctly disabled on isCheckingStatus |
Sequence Diagram
sequenceDiagram
participant UI as DestroyConfirmPane
participant DS as useDestroyDialogState
participant DW as useDestroyWorkspace
participant HS as host-service (TRPC)
participant DB as SQLite
participant Cloud as Cloud API
UI->>DS: dialog opens
DS->>DS: hostTarget.status !== ready?
alt host loading/local-starting
DS-->>UI: isCheckingStatus=true (Checking button)
else host ready
DS->>DW: inspect()
DW->>HS: workspaceCleanup.inspect
HS->>DB: isMainWorkspace() — query workspaces+projects
HS->>DB: re-query workspaces (worktree path)
HS->>HS: git status + rev-list --not --remotes
HS-->>DW: canDelete, reason, hasChanges, hasUnpushedCommits
DW-->>DS: DestroyWorkspacePreview
DS-->>UI: blockingReason / hasChanges / hasUnpushedCommits
end
UI->>DS: user confirms (onConfirm)
DS->>DW: destroy(deleteBranch, force=hasWarnings)
DW->>HS: workspaceCleanup.destroy
HS->>HS: destroysInFlight.has(id)?
alt already in flight
HS-->>DW: CONFLICT + DELETE_IN_PROGRESS cause
DW-->>DS: kind=in-progress
DS-->>UI: toast.error already in progress
else first caller
HS->>HS: isMainWorkspace() — query again
HS->>DB: preflight git status (unless force)
alt dirty worktree
HS-->>DW: CONFLICT dirty worktree
DW-->>DS: kind=conflict
DS->>DW: silent retry destroy force=true
end
HS->>Cloud: v2Workspace.delete (commit point)
HS->>DB: remove worktree row + invalidate cache
HS-->>DW: success, warnings
DW-->>DS: DestroyWorkspaceSuccess
DS-->>UI: toast warnings + onDeleted()
end
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts:83
**`conflictWasRaced` is unreachable dead code**
`setError` is only ever called in the `e.kind === "teardown-failed"` branch (line 130). For a `conflict` error, the code either silently force-retries (inner catch, line 119–123) or falls to the `toast.error` branch (line 135) — neither path calls `setError`. So `error?.kind === "conflict"` is always `false`, and the `|| conflictWasRaced` term in `hasChanges` never activates.
If the intent was to re-surface `hasChanges: true` when a conflict race is later reported back to the user via the dialog, that path needs to route through `setError` as well. Otherwise, remove the variable to avoid misleading future readers.
### Issue 2 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts:80
**Inspect re-fires on `hostTarget` reference change without status change**
The `inspect` callback has `[hostTarget, workspaceId]` as its `useCallback` dependency. If `hostTarget` gets a new object identity while keeping `status === "ready"` (e.g., a `useLiveQuery` update changes `match` while the workspace URL is unchanged), `inspect`'s reference changes, this `useEffect` re-runs, and the dialog briefly flickers back to the "Checking…" state.
Consider narrowing the effect dependency to the stable scalar value — `hostTarget.status === "ready" ? hostTarget.url : null` — rather than the full `inspect` function reference. Or use a stable `useRef`-based callback that reads the current host target at call time.
### Issue 3 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts:162
**`canDelete: false` with `reason: null` leaves the delete button enabled**
`blockingReason` is `null` when `preview.reason` is `null`, so the button's `disabled={isCheckingStatus || !!blockingReason}` condition would not block deletion. The `DestroyWorkspacePreview` type allows `reason: string | null` regardless of `canDelete`, so a server response of `{ canDelete: false, reason: null }` (which the TypeScript type permits) would silently allow the deletion.
The current server always provides a non-null `reason` when `canDelete === false`, but the client-side contract should guard against this. Consider also disabling the button when `preview && !preview.canDelete`:
```ts
disabled={isCheckingStatus || (!!preview && !preview.canDelete) || !!blockingReason}
```
Or tighten the type on the server so `reason` is `string` when `canDelete` is `false`.
### Issue 4 of 4
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts:163-173
**Duplicate DB queries between `isMainWorkspace` and `runDestroy`**
`isMainWorkspace` (called on line 175) already queries `workspaces` and `projects` internally. `runDestroy` then immediately re-queries both tables (lines 166–173) for `local` and `project`. For the common non-main path this doubles the synchronous SQLite round-trips on every destroy call.
Consider threading the already-fetched row data into `isMainWorkspace`, or having it return the rows alongside the boolean, so `runDestroy` can reuse them.
Reviews (1): Last reviewed commit: "feat(desktop): align v2 delete-workspace..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx`:
- Around line 103-107: In DestroyConfirmPane, when rendering the confirm Button
(props: onConfirm, disabled uses isCheckingStatus || !!blockingReason), replace
the displayed confirmLabel with an explicit "Checking…" string while
isCheckingStatus is true so the button shows the pending state; keep the same
disabled logic (isCheckingStatus || !!blockingReason) and otherwise render
confirmLabel as before. Use the existing symbols Button, isCheckingStatus,
blockingReason, confirmLabel, and onConfirm to locate and update the JSX.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts`:
- Around line 50-60: The useEffect inside the useDestroyDialogState hook
currently maps every non-ready hostTarget.status to "loading", causing terminal
states like "not-found" to remain in a perpetual checking state; update the
effect so only transient statuses (e.g., "starting", "initializing", etc.)
setInspectState({ status: "loading" }) while terminal statuses (specifically
"not-found", and any other failure states) set a distinct failure state (e.g.,
setInspectState({ status: "error", reason: "not-found" }) or similar) so
isCheckingStatus becomes false and the Delete button can surface the failure
path; modify the conditional logic that checks hostTarget.status in the
useEffect (and any related isCheckingStatus derivation) to differentiate
transient vs terminal statuses, referencing open, setInspectState,
hostTarget.status, useEffect, and isCheckingStatus to locate where to change the
behavior.
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 252-255: The call to open the repo via ctx.git(project.repoPath)
can throw before the cleanup try/catch and must be treated as a best-effort
warning instead of letting it abort the mutation; wrap the repo-open and
subsequent worktree removal in the same phase-3 warning path by moving
ctx.git(project.repoPath) inside the existing try/catch (or adding a new
try/catch) that handles worktree removal, and on any error from ctx.git or
git.raw(["worktree","remove",...]) push a warning (same mechanism used for
phase-3 warnings) and continue so the cloud delete remains best-effort and the
host-sqlite cleanup that follows still runs. Ensure you reference ctx.git,
project.repoPath, local.worktreePath and git.raw in the updated try/catch so
repo-open failures don't escape.
In `@plans/20260430-v2-delete-workspace-audit.md`:
- Around line 30-31: Update the audit record text to reflect the actual
implementation: replace the "for-each-ref fallback" description with the current
unpushed-detection approach using `git rev-list HEAD --not --remotes` (no
fallback), and replace the concurrency guard description that mentions
`Map<workspaceId, Promise>` with the implemented process-local `Set<string>`
tracking in-progress workspace IDs (ensure it documents that a second caller
sees a typed "already in progress" error or awaits based on the Set semantics).
Also apply the same wording changes to the other occurrence of the old design in
the file so the decision log matches the shipped code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3df80d5-2657-4f09-b856-0a8168406ba5
📒 Files selected for processing (13)
apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/index.tsapps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.tsapps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/index.tsapps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tspackages/host-service/src/index.tspackages/host-service/src/trpc/error-types.tspackages/host-service/src/trpc/index.tspackages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.tspackages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tsplans/20260430-v2-delete-workspace-audit.md
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
… guard
15 tests across three areas:
- isMainWorkspace: realpath normalization (incl. symlink case),
cloud-type fallback, no-row safe path
- workspaceCleanup.inspect: blocked-main, no-row, hasChanges,
rev-list-aware unpushed detection, git-failure swallow
- workspaceCleanup.destroy in-flight guard: cleanup on success,
cleanup on throw, CONFLICT+DELETE_IN_PROGRESS rejection,
retry-after-failure (regression test for in-flight leak)
Exposes destroysInFlight as __testDestroysInFlight (internal-only,
explicit @internal annotation) so tests can assert cleanup state
without spinning up a full tRPC client.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts (1)
255-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ctx.git(project.repoPath)can still hard-fail after the cloud commit point.At Line 256, repo-open happens outside the phase-3 warning path. If it throws,
destroyrejects even though cloud delete already succeeded, and host sqlite cleanup at Line 287+ is skipped.Proposed fix
if (local && project) { - const git = await ctx.git(project.repoPath); - try { + let git: Awaited<ReturnType<HostServiceContext["git"]>> | null = null; + try { + git = await ctx.git(project.repoPath); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push( + `Failed to open repo at ${project.repoPath} for cleanup: ${message}`, + ); + } + + if (git) { + try { await git.raw(["worktree", "remove", "--force", local.worktreePath]); worktreeRemoved = true; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - if ( - message.includes("is not a working tree") || - message.includes("No such file or directory") || - message.includes("ENOENT") - ) { - worktreeRemoved = true; - } else { - warnings.push( - `Failed to remove worktree at ${local.worktreePath}: ${message}`, - ); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + if ( + message.includes("is not a working tree") || + message.includes("No such file or directory") || + message.includes("ENOENT") + ) { + worktreeRemoved = true; + } else { + warnings.push( + `Failed to remove worktree at ${local.worktreePath}: ${message}`, + ); + } } - } - if (input.deleteBranch && local.branch) { - try { - await git.raw(["branch", "-D", local.branch]); - branchDeleted = true; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - warnings.push(`Failed to delete branch ${local.branch}: ${message}`); + if (input.deleteBranch && local.branch) { + try { + await git.raw(["branch", "-D", local.branch]); + branchDeleted = true; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + warnings.push(`Failed to delete branch ${local.branch}: ${message}`); + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts` around lines 255 - 257, The call to ctx.git(project.repoPath) can throw after the cloud-delete phase and cause destroy to reject, skipping the host sqlite cleanup; wrap the repo-open in its own try/catch (or move it inside the phase-3 warning path) so failures there are caught and do not abort the remainder of cleanup: call ctx.git(project.repoPath) inside a guarded block, on error log a warning referencing project.repoPath and continue to run the host sqlite cleanup code (the block after Line 287) so destroy can complete even if repo-open fails.
🧹 Nitpick comments (1)
packages/host-service/test/workspace-cleanup.test.ts (1)
276-359: ⚡ Quick winAdd a regression test for repo-open failure in phase-3 cleanup.
Current destroy tests cover cloud-delete failure and in-flight cleanup, but they don’t assert behavior when
ctx.git(project.repoPath)throws after the cloud delete commit point. A dedicated case here would lock in the “return success + warning” contract for best-effort local cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/test/workspace-cleanup.test.ts` around lines 276 - 359, Add a regression test in workspace-cleanup.test.ts that simulates a failure when opening the repo in phase 3 by making makeCtx({ git: () => { throw new Error("repo open failed") } }) (or mocking ctx.git(project.repoPath) to throw), call workspaceCleanupRouter.createCaller(ctx).destroy({ workspaceId: "ws-1", deleteBranch: false, force: false }) and assert the call resolves (does not throw) — i.e., destroy returns success with only a warning — and also assert __testDestroysInFlight.has("ws-1") is false after the call; reference the existing tests for cloudDelete behavior and reuse __testDestroysInFlight and makeCtx to locate where to add the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 255-257: The call to ctx.git(project.repoPath) can throw after the
cloud-delete phase and cause destroy to reject, skipping the host sqlite
cleanup; wrap the repo-open in its own try/catch (or move it inside the phase-3
warning path) so failures there are caught and do not abort the remainder of
cleanup: call ctx.git(project.repoPath) inside a guarded block, on error log a
warning referencing project.repoPath and continue to run the host sqlite cleanup
code (the block after Line 287) so destroy can complete even if repo-open fails.
---
Nitpick comments:
In `@packages/host-service/test/workspace-cleanup.test.ts`:
- Around line 276-359: Add a regression test in workspace-cleanup.test.ts that
simulates a failure when opening the repo in phase 3 by making makeCtx({ git: ()
=> { throw new Error("repo open failed") } }) (or mocking
ctx.git(project.repoPath) to throw), call
workspaceCleanupRouter.createCaller(ctx).destroy({ workspaceId: "ws-1",
deleteBranch: false, force: false }) and assert the call resolves (does not
throw) — i.e., destroy returns success with only a warning — and also assert
__testDestroysInFlight.has("ws-1") is false after the call; reference the
existing tests for cloudDelete behavior and reuse __testDestroysInFlight and
makeCtx to locate where to add the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8822c963-3d30-4fa2-8e7a-42b1f52cf9dc
📒 Files selected for processing (2)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tspackages/host-service/test/workspace-cleanup.test.ts
Eight findings from greptile + coderabbit:
- phase 3 ctx.git(project.repoPath) was outside the per-step try
blocks; a failure there past the cloud-delete commit point would
surface as a hard error for an already-deleted workspace. Now
captured as a warning and the rest of phase 3 skips cleanly.
- DestroyConfirmPane disabled on !!blockingReason; if server ever
returned { canDelete: false, reason: null } the button would stay
enabled. Renderer now derives canConfirm from preview.canDelete
directly.
- host-target's `not-found` was collapsed into "loading"; dialog
would sit in perpetual "Checking…". Now renders a destructive
blocking banner ("Workspace is no longer available on this host").
- useDestroyWorkspace's useCallback closed over the full hostTarget
object; useLiveQuery returns a new array per tick, so identity
churn was rebuilding inspect/destroy and re-firing the dialog
effect (visible flicker). Stabilized on hostUrl + hostStatus
scalars.
- confirm button label said "Delete" during preflight; now flips to
"Checking…" while inspect is in flight.
- removed dead `conflictWasRaced` (error is only ever set to
`teardown-failed`; conflict path either silently retries or toasts).
- de-duped sqlite reads: isMainWorkspace already loads workspace +
project rows; thread them through to runDestroy and inspect
instead of re-querying.
- audit doc said `for-each-ref` and Map<workspaceId, Promise>;
reflect what shipped (`rev-list --not --remotes` and Set<string>).
Adds a test for the phase-3 git-factory warning behavior. Test count
now 16 (was 15), all green.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 237-295: The post-commit cleanup steps
(disposeSessionsByWorkspaceId, the host sqlite delete call
ctx.db.delete(workspaces).where(eq(workspaces.id, input.workspaceId)).run(), and
invalidateLabelCache(input.workspaceId)) must be executed inside the phase-3
best-effort warning path so they never cause the mutation to throw; wrap these
calls in a try/catch (the same pattern used earlier for worktree/branch
cleanup), catch any error, convert it to a message string, and push a warning
into the existing warnings array instead of letting the error propagate,
ensuring the mutation returns success even if these cleanup ops fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01cb1882-cfb4-42d0-a7c4-46a9d6c2b2ca
📒 Files selected for processing (8)
apps/desktop/src/renderer/hooks/host-service/useDestroyWorkspace/useDestroyWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tspackages/host-service/src/trpc/router/workspace-cleanup/is-main-workspace.tspackages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tspackages/host-service/test/workspace-cleanup.test.tsplans/20260430-v2-delete-workspace-audit.md
✅ Files skipped from review due to trivial changes (1)
- plans/20260430-v2-delete-workspace-audit.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
Three small wins, net -11 lines:
- drop unused `ahead` field from ContextSpec (leftover from when
inspect used status.ahead instead of rev-list)
- drop redundant afterEach (beforeEach already protects the next
test from any leak)
- replace manual try/catch in the concurrent-CONFLICT test with
`await expect(...).rejects.toMatchObject(...)`
No coverage change. 16 tests, all green.
Two more PR-review findings:
- disposeSessionsByWorkspaceId, the host sqlite delete, and
invalidateLabelCache were still outside any try/catch. Anything
that throws past phase 2 (cloud-commit point) breaks the
"phase 3 = best-effort" contract and surfaces a hard "Failed to
delete" toast for an already-deleted workspace. Each is now its
own try/catch + warning push.
- InspectResult is now a discriminated union on both server and
renderer sides. The bad combination { canDelete: false, reason:
null } is unrepresentable at the type level, so the previous
"button still enabled when reason is null" bug can't be
reintroduced even if a future refactor drops the canConfirm
consumer-side check.
Adds a sqlite-delete-throws regression test. 17 tests, all green.
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.
Co-locate the v2 delete dialog's preflight on the same host-service that runs
destroy, so they can never disagree about what's blocked vs warned. Fixes:rev-list HEAD --not --remoteswhich catches never-pushed branchesAudit + decisions captured in plans/20260430-v2-delete-workspace-audit.md.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation