fix(desktop): show v1 uncommitted-changes banner instead of second delete prompt#3688
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe workspace deletion dialog now runs a preflight Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog as DashboardSidebarDeleteDialog
participant Hook as useDestroyDialogState
participant TRPC as TRPC API
participant Confirm as DestroyConfirmPane
User->>Dialog: open dialog
Dialog->>Hook: init(open=true)
Hook->>TRPC: query workspaces.canDelete
TRPC-->>Hook: {hasChanges, hasUnpushedCommits}
Hook-->>Dialog: expose flags + run()
Dialog->>Confirm: render(hasChanges, hasUnpushedCommits, isCheckingStatus)
Confirm->>Confirm: compute hasWarnings, show inline banner if needed
User->>Confirm: confirm delete
Confirm->>Hook: run(hasWarnings)
Hook->>TRPC: mutation destroy(force=false)
alt conflict & force=false
TRPC-->>Hook: conflict error
Hook->>TRPC: mutation destroy(force=true)
TRPC-->>Hook: success
Hook-->>Dialog: close
else teardown-failed
TRPC-->>Hook: teardown-failed
Hook-->>Dialog: open error pane
else success
TRPC-->>Hook: success
Hook-->>Dialog: close
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. Comment |
Greptile SummaryThis PR eliminates the double-prompt UX bug when deleting a workspace with uncommitted changes. Instead of showing a generic confirm dialog followed by a separate Confidence Score: 5/5Safe to merge — correctness is preserved by the silent conflict retry, and all remaining findings are P2 suggestions. No P0/P1 issues found. The two open items are a button label cosmetic (P2) and an optional loading-guard UX improvement (P2); neither affects data integrity or correctness. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts | Adds canDelete preflight query (enabled when dialog is open) and silent conflict retry; removes ConflictPane routing — logic is sound with good race handling |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx | Removes ConflictPane dispatch, passes open and warning flags through; run(hasWarnings) correctly passes force: true when dirty state is known |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx | Adds yellow inline warning banner for dirty/unpushed state; Delete button label unchanged despite warnings being shown — minor UX inconsistency |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/ConflictPane.tsx | File deleted — ConflictPane is fully superseded by the inline banner approach |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/index.ts | Barrel export deleted along with ConflictPane |
Sequence Diagram
sequenceDiagram
participant U as User
participant D as DeleteDialog
participant Q as canDelete query
participant W as destroy (IPC)
U->>D: Click "Delete workspace"
D->>Q: useQuery({ enabled: true })
Q-->>D: { hasChanges, hasUnpushedCommits }
Note over D: Show yellow banner if dirty
U->>D: Click "Delete" (force = hasWarnings)
D->>W: destroy({ force })
alt worktree clean OR force=true
W-->>D: DestroyWorkspaceSuccess
D->>U: toast warnings (if any)
else conflict race (force=false, dirty)
W-->>D: conflict error
D->>W: destroy({ force: true }) [silent retry]
W-->>D: DestroyWorkspaceSuccess
else teardown-failed
W-->>D: teardown-failed error
D->>U: Reopen dialog → TeardownFailedPane
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx, line 86-91 (link)Button label unchanged when warnings are shown
When the yellow banner is visible, the "Delete" button label doesn't update to reflect that the user is overriding the warning. Without a label change (e.g. "Delete anyway"), it's easy to miss that clicking Delete will discard uncommitted changes.
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx Line: 86-91 Comment: **Button label unchanged when warnings are shown** When the yellow banner is visible, the "Delete" button label doesn't update to reflect that the user is overriding the warning. Without a label change (e.g. "Delete anyway"), it's easy to miss that clicking Delete will discard uncommitted changes. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx
Line: 86-91
Comment:
**Button label unchanged when warnings are shown**
When the yellow banner is visible, the "Delete" button label doesn't update to reflect that the user is overriding the warning. Without a label change (e.g. "Delete anyway"), it's easy to miss that clicking Delete will discard uncommitted changes.
```suggestion
Delete{hasWarnings ? " anyway" : ""}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
Line: 34-43
Comment:
**No pending/loading guard while `canDelete` query resolves**
`hasChanges` and `hasUnpushedCommits` both default to `false` while the query is in-flight. On a slow IPC call the banner never renders before the user clicks Delete — they skip the warning entirely. The silent conflict-retry means correctness is preserved, but if this latency is common, consider using the `isPending` / `isLoading` flag from the query result to disable the Delete button (or show a spinner) until the status is confirmed.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): show v1 uncommitted-change..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+34
to
+43
| { id: workspaceId }, | ||
| { | ||
| enabled: open, | ||
| staleTime: STATUS_STALE_TIME_MS, | ||
| refetchOnWindowFocus: false, | ||
| }, | ||
| ); | ||
| const hasChanges = canDeleteData?.hasChanges ?? false; | ||
| const hasUnpushedCommits = canDeleteData?.hasUnpushedCommits ?? false; | ||
|
|
There was a problem hiding this comment.
canDelete query resolves
hasChanges and hasUnpushedCommits both default to false while the query is in-flight. On a slow IPC call the banner never renders before the user clicks Delete — they skip the warning entirely. The silent conflict-retry means correctness is preserved, but if this latency is common, consider using the isPending / isLoading flag from the query result to disable the Delete button (or show a spinner) until the status is confirmed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts
Line: 34-43
Comment:
**No pending/loading guard while `canDelete` query resolves**
`hasChanges` and `hasUnpushedCommits` both default to `false` while the query is in-flight. On a slow IPC call the banner never renders before the user clicks Delete — they skip the warning entirely. The silent conflict-retry means correctness is preserved, but if this latency is common, consider using the `isPending` / `isLoading` flag from the query result to disable the Delete button (or show a spinner) until the status is confirmed.
How can I resolve this? If you propose a fix, please make it concise.![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/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts (1)
93-100:⚠️ Potential issue | 🟡 MinorNarrowing
setErrorto onlyteardown-failedleavesUnknownErrorPaneunreachable.
DashboardSidebarDeleteDialog.tsxstill rendersUnknownErrorPanewhenerror?.kind === "unknown"(lines 56–65), but this branch now only callssetErrorforteardown-failed. Anunknownerror only produces a toast, soerror.kindcan never be"unknown"and that pane is dead code.Either keep surfacing unknown errors via the pane, or drop the pane + its import in the parent. Preferring the latter matches the intent described in the PR (teardown failures still surface
TeardownFailedPane; everything else toasts), but make it explicit so the UI isn't carrying an unreachable branch.♻️ Option A — keep UnknownErrorPane reachable
- if (e.kind === "teardown-failed") { + if (e.kind === "teardown-failed" || e.kind === "unknown") { setError(e); onOpenChange(true); } else { toast.error(`Failed to delete ${workspaceName}: ${e.message}`); }♻️ Option B — drop the unreachable pane in the parent
In
DashboardSidebarDeleteDialog.tsx:-import { UnknownErrorPane } from "./components/UnknownErrorPane"; ... - if (error?.kind === "unknown") { - return ( - <UnknownErrorPane - open={open} - onOpenChange={handleOpenChange} - message={error.message} - onRetry={clearError} - /> - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts` around lines 93 - 100, The current catch narrows setError to only teardown-failed, making UnknownErrorPane in DashboardSidebarDeleteDialog.tsx unreachable; remove the unused UnknownErrorPane import and its JSX branch (the conditional that checks error?.kind === "unknown") from DashboardSidebarDeleteDialog.tsx so the UI matches the handler in useDestroyDialogState (keep TeardownFailedPane and the toast behavior for other errors), and verify there are no leftover references to UnknownErrorPane or its prop usage.
🤖 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/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts`:
- Around line 93-100: The current catch narrows setError to only
teardown-failed, making UnknownErrorPane in DashboardSidebarDeleteDialog.tsx
unreachable; remove the unused UnknownErrorPane import and its JSX branch (the
conditional that checks error?.kind === "unknown") from
DashboardSidebarDeleteDialog.tsx so the UI matches the handler in
useDestroyDialogState (keep TeardownFailedPane and the toast behavior for other
errors), and verify there are no leftover references to UnknownErrorPane or its
prop usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ce2a836-8105-4fc4-8b3d-bc1fce35509c
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/ConflictPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/index.tsapps/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.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/ConflictPane/ConflictPane.tsx
![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 5 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/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts:41">
P2: While the `canDelete` query is in-flight, `hasChanges` and `hasUnpushedCommits` default to `false`, so the warning banner won't render and `run(false)` is called on confirm. On a slow IPC call the user can delete without ever seeing the dirty-worktree warning. Consider destructuring `isPending` from the query and disabling the Delete button (or passing a loading state) until the status is confirmed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…lete prompt The v2 workspace delete flow showed two warnings: a generic confirm pane, then a "Uncommitted changes in worktree" pane after destroy hit a conflict. Surface the v1 yellow banner inline on the confirm pane via the existing canDelete preflight, force when warnings are shown, and silently retry on the rare race so the user only ever sees one prompt.
saddlepaddle
force-pushed
the
galvanized-tv
branch
from
b5956c5 to
d31cd51
Compare
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
11 tasks
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
Has uncommitted changes / unpushed commitsbanner inline on the confirm pane (sourced fromelectronTrpc.workspaces.canDelete) so the user sees the warning upfrontforce: truefrom the confirm when warnings are shown, and silently retry withforceon the rare conflict race, so the second prompt is never reached.ConflictPaneis removed.Test plan
TeardownFailedPanestill shows and "Delete anyway" still forcesSummary by cubic
Show a single inline warning banner for uncommitted changes or unpushed commits on the delete confirm pane and remove the second prompt, keeping workspace deletion a one-click flow with an automatic force retry on conflict races.
electronTrpc.workspaces.canDeletereportshasChangesorhasUnpushedCommits; disable Delete while the status is loading.force: true; on rare conflict races, silently retry withforce. Removed theConflictPane.TeardownFailedPane. Unknown errors now toast only (no dialog).useDestroyDialogStatefetches can-delete status on open (5s stale) and exposeshasChanges/hasUnpushedCommits.Written for commit d31cd51. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Bug Fixes