Conversation
Destroy can take 10–20s; showing no feedback and leaving the user on the workspace being torn down felt broken. Show a loading toast that resolves to success/error, and navigate to a sibling workspace (or home) the moment destroy kicks off instead of waiting for it to finish.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional onDeleting callback and immediate "Deleting…" toast when a workspace deletion is initiated; updates dialog props and hook usage, and refactors workspace-item actions to separate navigation-away, deleting, and sidebar removal steps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceItem
participant DeleteDialog
participant DestroyHook
participant SidebarStore
participant Router
participant Toast
User->>WorkspaceItem: open menu & choose Delete
WorkspaceItem->>DeleteDialog: show dialog (passes onDeleting)
User->>DeleteDialog: confirm
DeleteDialog->>DestroyHook: close dialog, mark deleting
DestroyHook->>Toast: show "Deleting…" toast
DestroyHook->>WorkspaceItem: call onDeleting (handleDeleting)
WorkspaceItem->>Router: navigateAwayIfActive
DestroyHook->>SidebarStore: optimistic close/remove UI
DestroyHook->>DestroyHook: call async destroy()
alt destroy succeeds
DestroyHook->>SidebarStore: handleDeleted (finalize removal)
else destroy conflicts/errors
DestroyHook->>DeleteDialog: reopen dialog / show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx (1)
203-211:⚠️ Potential issue | 🔴 CriticalMissing
onDeletingwire-up in the expanded (default) sidebar branch — breaks the PR’s primary behavior.The collapsed branch at line 143 correctly forwards
onDeleting={handleDeleting}, but this expanded render — which is the default, non-collapsed sidebar and therefore the common path — only passesonDeleted. As a result, deleting the active workspace from the expanded sidebar won’t navigate the user off the workspace that’s being torn down, which is the main UX fix described in the PR objectives.Proposed fix
{!isPending && ( <DashboardSidebarDeleteDialog workspaceId={id} workspaceName={name || branch} open={isDeleteDialogOpen} onOpenChange={setIsDeleteDialogOpen} + onDeleting={handleDeleting} onDeleted={handleDeleted} /> )}🤖 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx` around lines 203 - 211, The expanded (default) branch rendering DashboardSidebarDeleteDialog is missing the onDeleting prop which the collapsed branch passes; update the expanded render of DashboardSidebarDeleteDialog to include onDeleting={handleDeleting} (alongside workspaceId={id}, workspaceName={name || branch}, open={isDeleteDialogOpen}, onOpenChange={setIsDeleteDialogOpen}, and onDeleted={handleDeleted}) so deleting the active workspace triggers the handleDeleting flow and navigates away as intended.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts (1)
80-93: Minor: inconsistent quoting ofworkspaceNamebetween success and error toasts.The loading and success toasts wrap the name in quotes (
Deleting "name"...,Deleted "name"), but the error toast does not (Failed to delete name: ...). Worth aligning for consistency.Proposed tweak
- toast.error(`Failed to delete ${workspaceName}: ${e.message}`, { + toast.error(`Failed to delete "${workspaceName}": ${e.message}`, { id: toastId, });🤖 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 80 - 93, The error toast message in useDestroyDialogState is inconsistent with the success toast: update the toast.error call (in the catch block where err is cast to DestroyWorkspaceError and handled) to wrap workspaceName in quotes just like the success and loading toasts (e.g., `Failed to delete "workspaceName": ${e.message}`), preserving the existing toastId and error message usage; ensure this change is applied in the catch branch that currently calls toast.error and does not affect the conflict/teardown-failed branch that uses setError/onOpenChange.
🤖 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 203-211: The expanded (default) branch rendering
DashboardSidebarDeleteDialog is missing the onDeleting prop which the collapsed
branch passes; update the expanded render of DashboardSidebarDeleteDialog to
include onDeleting={handleDeleting} (alongside workspaceId={id},
workspaceName={name || branch}, open={isDeleteDialogOpen},
onOpenChange={setIsDeleteDialogOpen}, and onDeleted={handleDeleted}) so deleting
the active workspace triggers the handleDeleting flow and navigates away as
intended.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts`:
- Around line 80-93: The error toast message in useDestroyDialogState is
inconsistent with the success toast: update the toast.error call (in the catch
block where err is cast to DestroyWorkspaceError and handled) to wrap
workspaceName in quotes just like the success and loading toasts (e.g., `Failed
to delete "workspaceName": ${e.message}`), preserving the existing toastId and
error message usage; ensure this change is applied in the catch branch that
currently calls toast.error and does not affect the conflict/teardown-failed
branch that uses setError/onOpenChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a63c32d4-5b93-411b-a0dc-a9e50ba70917
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
Greptile SummaryThis PR improves the v2 sidebar workspace-delete UX by (1) showing a loading → success/error toast instead of destroying silently over 10–20 s, and (2) navigating the user off the active workspace the moment deletion kicks off rather than waiting for the async destroy to finish. It achieves this by splitting the old Issues found:
Confidence Score: 2/5Not safe to merge — the primary (expanded) delete path is missing the new The overall design — loading toast, early navigation, split callbacks — is correct and well-structured. However, the P0 regression in
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx | Correctly wires onDeleting={handleDeleting} in the collapsed branch but omits it in the expanded (default) branch — leaving the primary delete path without navigation-on-delete behavior. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts | Cleanly introduces loading toast lifecycle (loading → success/error/dismiss) and onDeleting callback; minor inconsistency in error toast quoting vs. success toast. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts | Correctly splits the old handleDeleted into handleDeleting (navigate-immediately) and handleDeleted (remove-row-on-success); logic is sound. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsx | Correctly threads the new onDeleting prop through to useDestroyDialogState; no issues. |
Sequence Diagram
sequenceDiagram
participant U as User
participant D as DeleteDialog
participant H as useDestroyDialogState
participant A as useSidebarItemActions
participant T as Toast (global)
participant S as Sidebar State
U->>D: Confirm delete
D->>H: run(force=false)
H->>H: onOpenChange(false) — close dialog
H->>S: markDeleting(workspaceId) — hide row
H->>A: onDeleting() — handleDeleting()
alt isActive workspace
A->>A: navigate to sibling or /
end
H->>T: toast.loading("Deleting …")
H->>H: await destroy()
alt success
H->>T: toast.success("Deleted …")
H->>A: onDeleted() → removeWorkspaceFromSidebar()
else conflict / teardown-failed
H->>T: toast.dismiss(toastId)
H->>D: onOpenChange(true) — reopen in error pane
H->>S: clearDeleting() — row reappears
else unknown error
H->>T: toast.error("Failed to delete …", {id})
H->>S: clearDeleting() — row reappears
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx, line 203-211 (link)onDeletingmissing in the expanded (default) sidebar branchThe collapsed branch at line 143 correctly passes
onDeleting={handleDeleting}, but the expanded branch rendered here omits it. Since the expanded layout is the default sidebar state, deleting an active workspace in the normal view will never callhandleDeleting, meaning the user is never navigated away and will be left staring at the workspace being torn down for 10–20 seconds — exactly the regression this PR was written to fix.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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx Line: 203-211 Comment: **`onDeleting` missing in the expanded (default) sidebar branch** The collapsed branch at line 143 correctly passes `onDeleting={handleDeleting}`, but the expanded branch rendered here omits it. Since the expanded layout is the default sidebar state, deleting an active workspace in the normal view will never call `handleDeleting`, meaning the user is never navigated away and will be left staring at the workspace being torn down for 10–20 seconds — exactly the regression this PR was written to fix. 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
Line: 203-211
Comment:
**`onDeleting` missing in the expanded (default) sidebar branch**
The collapsed branch at line 143 correctly passes `onDeleting={handleDeleting}`, but the expanded branch rendered here omits it. Since the expanded layout is the default sidebar state, deleting an active workspace in the normal view will never call `handleDeleting`, meaning the user is never navigated away and will be left staring at the workspace being torn down for 10–20 seconds — exactly the regression this PR was written to fix.
```suggestion
{!isPending && (
<DashboardSidebarDeleteDialog
workspaceId={id}
workspaceName={name || branch}
open={isDeleteDialogOpen}
onOpenChange={setIsDeleteDialogOpen}
onDeleting={handleDeleting}
onDeleted={handleDeleted}
/>
)}
```
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: 91-93
Comment:
**Inconsistent quoting in error toast vs. success toast**
The success toast quotes the workspace name (`Deleted "${workspaceName}"`), but the error toast does not (`Failed to delete ${workspaceName}: ...`). Minor, but the inconsistency is visible to users when an error occurs.
```suggestion
toast.error(`Failed to delete "${workspaceName}": ${e.message}`, {
id: toastId,
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): toast and switch workspace..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+91
to
+93
| toast.error(`Failed to delete ${workspaceName}: ${e.message}`, { | ||
| id: toastId, | ||
| }); |
There was a problem hiding this comment.
The success toast quotes the workspace name (Deleted "${workspaceName}"), but the error toast does not (Failed to delete ${workspaceName}: ...). Minor, but the inconsistency is visible to users when an error occurs.
Suggested change
| toast.error(`Failed to delete ${workspaceName}: ${e.message}`, { | |
| id: toastId, | |
| }); | |
| toast.error(`Failed to delete "${workspaceName}": ${e.message}`, { | |
| id: toastId, | |
| }); |
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: 91-93
Comment:
**Inconsistent quoting in error toast vs. success toast**
The success toast quotes the workspace name (`Deleted "${workspaceName}"`), but the error toast does not (`Failed to delete ${workspaceName}: ...`). Minor, but the inconsistency is visible to users when an error occurs.
```suggestion
toast.error(`Failed to delete "${workspaceName}": ${e.message}`, {
id: toastId,
});
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 4 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx:143">
P1: The new `onDeleting` flow is wired only for collapsed items; expanded sidebar items still skip it, so deleting the active workspace won’t navigate away immediately in the default layout.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
A loading toast that persists for 10–20s and then resolves to success is noisier than the problem warrants. Just fire an info toast at start; success is already conveyed by the row disappearing.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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:74">
P2: The delete flow no longer uses a loading toast lifecycle, so users can lose in-progress status and never receive a success confirmation toast after deletion completes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
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">
P2: The new UX comment is incorrect: it claims there is no loading toast, but the code still shows a delete-start toast.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts">
<violation number="1">
P2: Navigation for active workspace deletion is now delayed until destroy success because it runs in `onDeleted`. Move route switching to a start-of-delete callback so users are redirected immediately.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Delete takes 10–20s; hide is immediate. In both cases we don't want to leave the user staring at a row that's being torn down or hidden. Fires `onDeleting` at the start of destroy (before teardown completes) and wraps `removeWorkspaceFromSidebar` to nav first, then remove.
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 (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx (1)
203-210:⚠️ Potential issue | 🟠 MajorPass
onDeletingin the expanded delete dialog too.The collapsed dialog wires
onDeleting={handleDeleting}, but the expanded dialog does not. Deleting from the expanded sidebar therefore hides/deletes without navigating off the active workspace.🐛 Proposed fix
{!isPending && ( <DashboardSidebarDeleteDialog workspaceId={id} workspaceName={name || branch} open={isDeleteDialogOpen} onOpenChange={setIsDeleteDialogOpen} + onDeleting={handleDeleting} onDeleted={handleDeleted} /> )}🤖 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx` around lines 203 - 210, The expanded delete dialog instance of DashboardSidebarDeleteDialog is missing the onDeleting prop, so when deleting from the expanded sidebar handleDeleting isn't called; add onDeleting={handleDeleting} to the DashboardSidebarDeleteDialog invocation (the one rendered inside the {!isPending && ( ... )} block) so both collapsed and expanded dialogs call the same handleDeleting handler.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts (1)
68-88:⚠️ Potential issue | 🟠 MajorTrack the loading toast ID and settle it on every outcome.
Line 74 emits a plain toast, so it never resolves to success/error. Conflict/teardown paths reopen the dialog while the "Deleting…" toast remains visible. Store the toast ID from
toast.loading(), update it on success/unknown failure, and dismiss it before reopening the dialog for decision-required errors.🐛 Proposed toast lifecycle fix
setError(null); onOpenChange(false); markDeleting(workspaceId); onDeleting?.(); - toast(`Deleting "${workspaceName}"...`); + const deletingToastId = toast.loading(`Deleting "${workspaceName}"...`); try { const result = await destroy({ deleteBranch, force }); for (const warning of result.warnings) toast.warning(warning); setDeleteBranch(false); + toast.success(`Deleted "${workspaceName}"`, { id: deletingToastId }); onDeleted?.(); } catch (err) { const e = err as DestroyWorkspaceError; if (e.kind === "conflict" || e.kind === "teardown-failed") { + toast.dismiss(deletingToastId); setError(e); onOpenChange(true); } else { - toast.error(`Failed to delete ${workspaceName}: ${e.message}`); + toast.error(`Failed to delete ${workspaceName}: ${e.message}`, { + id: deletingToastId, + }); } } finally {🤖 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 68 - 88, The "Deleting…" toast is emitted with toast(...) and never settled; change the flow in the delete handler (the block using setError, onOpenChange, markDeleting, onDeleting, destroy, setDeleteBranch, onDeleted) to call toast.loading(...) and capture its returned toastId, then on success call toast.update/toast.success (or toast.dismiss then toast.success) with that id, on unknown failure call toast.update/toast.error with the id, and on decision-required errors first dismiss/update the loading toast before calling setError and onOpenChange(true) so the stale "Deleting…" toast is removed when the dialog is reopened; ensure you reference the same toastId for all toast updates/ dismissals.
🤖 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 68-88: The "Deleting…" toast is emitted with toast(...) and never
settled; change the flow in the delete handler (the block using setError,
onOpenChange, markDeleting, onDeleting, destroy, setDeleteBranch, onDeleted) to
call toast.loading(...) and capture its returned toastId, then on success call
toast.update/toast.success (or toast.dismiss then toast.success) with that id,
on unknown failure call toast.update/toast.error with the id, and on
decision-required errors first dismiss/update the loading toast before calling
setError and onOpenChange(true) so the stale "Deleting…" toast is removed when
the dialog is reopened; ensure you reference the same toastId for all toast
updates/ dismissals.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 203-210: The expanded delete dialog instance of
DashboardSidebarDeleteDialog is missing the onDeleting prop, so when deleting
from the expanded sidebar handleDeleting isn't called; add
onDeleting={handleDeleting} to the DashboardSidebarDeleteDialog invocation (the
one rendered inside the {!isPending && ( ... )} block) so both collapsed and
expanded dialogs call the same handleDeleting handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06d0b66e-00c4-40e7-92ef-1f34ee83af82
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/DashboardSidebarDeleteDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
matchRoute with params + fuzzy was returning false for this item's hook when the user was viewing it, so navigateAwayIfActive no-op'd. useParams matches v1's pattern (useDeleteWorkspace) which is known to work.
Previous attempts threaded an onDeleting callback through dialog props and relied on the sidebar item's isActive closure, which wasn't firing. Replace all that with useNavigateAwayFromWorkspace: reads the current URL's workspaceId via useParams, reads the sidebar list from collections, picks the first sibling that isn't the one being removed. Delete and Hide both call it directly — no callback plumbing.
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts:78">
P2: Navigation-away now targets the first remaining workspace, not an adjacent sibling, which can redirect users to an unrelated workspace after hide/delete.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Drops the useNavigateAwayFromWorkspace hook. Nav logic stays inline in the sidebar item hook (where it already lived for the post-destroy path) and is shared between handleDeleting and handleRemoveFromSidebar. The dialog gets the nav via a new onDeleting callback mirroring onDeleted — same pattern, no hook extraction, zero new files.
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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:73">
P1: `onDeleting` is called outside the `try/finally`, so a thrown callback can leave deletion state stuck (`inFlight` never resets and `clearDeleting` never runs).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
matchRoute with params + fuzzy returns false for this hook's isActive check in the early-nav callback path, even when the user is viewing the workspace. useParams is what v1's useDeleteWorkspace uses and is known to work.
Nav was never landing — dialog close and markDeleting state thrash were swallowing it. Move onDeleting to be the first thing run() does so the navigate call goes out before any other state update can interfere.
The onDeleting callback route was silently dropping the navigation. The hook-extracted version was the only one that actually worked — call navigateAway(id) directly inside useDestroyDialogState and the sidebar item hook, no prop plumbing.
- Refresh useDestroyDialogState JSDoc: it now navigates first and fires a one-shot toast (was "run destroy silently, no toast"). - Restore the "deleteBranch preserved on optimistic close" comment that was accidentally dropped. - Revert the isActive matchRoute→useParams swap in the sidebar item hook — isActive is only used for styling now, so the change was noise.
Kitenite
deleted the
when-deleting-a-workspace-in-v2-please-toast-that-its-deleting-and-choose-a-different-workspace-to-m
branch
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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
handleDeletedinuseDashboardSidebarWorkspaceItemActionsinto a localnavigateAway, a trimmedhandleDeleted(just removes the row on destroy success), and a newhandleRemoveFromSidebar(nav + remove, used by the "Remove from Sidebar" context menu). Delete wires nav via a newonDeletingcallback onDashboardSidebarDeleteDialog, mirroring the existingonDeleted.Test plan
Summary by cubic
Show a one-time "Deleting ''..." toast when a v2 workspace delete starts. Immediately navigate off the active workspace on delete or hide to the next visible workspace or home.
useNavigateAwayFromWorkspace(reads URLworkspaceIdviauseParams) and runs before dialog close/state updates, so it lands reliably.Written for commit 1c343e3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes