fix(desktop): hide v2 workspace rows while destroy is in flight#3621
Merged
Kitenite merged 1 commit intoApr 21, 2026
Merged
Conversation
`workspaceCleanup.destroy` can take 10–20s; the old loading toast sat there the whole time and the row stayed in the sidebar. Now the row hides optimistically on confirm and the toast is gone. On error (conflict / teardown-failed) the row reappears and the dialog reopens into the matching error pane for force-retry.
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 (5)
📝 WalkthroughWalkthroughA new context-based provider system tracks workspace deletion states across the authenticated layout. The Changes
Sequence DiagramsequenceDiagram
participant User
participant DeleteDialog as Delete Dialog
participant Provider as DeletingWorkspacesProvider
participant Sidebar as Sidebar Component
User->>DeleteDialog: Confirm delete workspace
DeleteDialog->>Provider: markDeleting(workspaceId)
Provider->>Provider: Add ID to deletion set
DeleteDialog->>DeleteDialog: Close dialog
Sidebar->>Provider: Poll isDeleting(workspaceId)
Provider-->>Sidebar: true
Sidebar->>Sidebar: Hide item UI
DeleteDialog->>DeleteDialog: Execute destroy operation
alt Success
DeleteDialog->>DeleteDialog: Call onDeleted()
else Error (conflict/teardown)
DeleteDialog->>DeleteDialog: Reopen with error state
else Other Error
DeleteDialog->>DeleteDialog: Show error toast
end
DeleteDialog->>Provider: clearDeleting(workspaceId) in finally
Provider->>Provider: Remove ID from deletion set
Sidebar->>Provider: Poll isDeleting(workspaceId)
Provider-->>Sidebar: false
Sidebar->>Sidebar: Show item UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 improves the UX of v2 workspace deletion by replacing the long-lived Key changes:
Confidence Score: 5/5Safe to merge — all error paths restore the row and the implementation is correct under React 18 automatic batching. The provider is implemented correctly (functional setState, stable callbacks, proper memoization). The No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/providers/DeletingWorkspacesProvider/DeletingWorkspacesProvider.tsx | New context provider tracking in-flight workspace deletions with a Set; correctly uses functional setState updates and memoization. |
| apps/desktop/src/renderer/routes/_authenticated/providers/DeletingWorkspacesProvider/index.ts | Barrel export for the new provider — no issues. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/hooks/useDestroyDialogState/useDestroyDialogState.ts | Replaces loading-toast pattern with optimistic hide; success-path clearDeleting in finally could theoretically cause a single-frame flash if onDeleted is async, but React 18 batching makes this safe in practice. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx | Wraps visible content in <div hidden={isDeleting}>; correctly keeps DashboardSidebarDeleteDialog outside the hidden wrapper so error panes can still reopen. |
| apps/desktop/src/renderer/routes/_authenticated/layout.tsx | Wraps WorkerPoolContextProvider and descendants in the new DeletingWorkspacesProvider — correct placement in the component tree. |
Sequence Diagram
sequenceDiagram
actor User
participant Dialog as DeleteDialog
participant Hook as useDestroyDialogState
participant Provider as DeletingWorkspacesProvider
participant Row as SidebarWorkspaceItem
participant API as workspaceCleanup.destroy
User->>Dialog: Click Delete → Confirm
Dialog->>Hook: run(force=false)
Hook->>Provider: markDeleting(workspaceId)
Provider-->>Row: isDeleting → true → hidden
Hook->>Dialog: onOpenChange(false)
Hook->>API: destroy()
alt Success
API-->>Hook: result
Hook->>Row: onDeleted() → unmount
Hook->>Provider: clearDeleting() [finally]
else Conflict / Teardown-failed
API-->>Hook: throw error
Hook->>Provider: clearDeleting() [finally]
Provider-->>Row: row reappears
Hook->>Dialog: reopen error pane
else Unknown error
API-->>Hook: throw error
Hook->>Hook: toast.error
Hook->>Provider: clearDeleting() [finally]
Provider-->>Row: row reappears
end
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/hooks/useDestroyDialogState/useDestroyDialogState.ts
Line: 85-88
Comment:
**`clearDeleting` on success path may briefly flash the row**
On the success path, `onDeleted?.()` (which removes the workspace from sidebar state) is called in the `try` block and `clearDeleting(workspaceId)` runs afterwards in `finally`. In React 18 with automatic batching these two state updates will almost always be coalesced into one render — so the row would unmount without ever becoming visible again.
However, if `onDeleted` performs an asynchronous step before actually updating sidebar state (e.g. an async removal from a store), the `clearDeleting` call could land first in a separate render tick. At that instant `isDeleting(id)` returns `false` while the workspace is still present in the sidebar list, causing the row to flash visible for one frame before the sidebar update unmounts it.
If `onDeleted` is always synchronous this is a non-issue, but it may be worth moving `clearDeleting` inside the `try` block (before `onDeleted`) so the sequence is deterministically: row hidden → mark not-deleting → unmount due to onDeleted. The current code is safe as long as `onDeleted` only calls synchronous state setters.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): hide v2 workspace rows whi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
85
to
88
| } finally { | ||
| clearDeleting(workspaceId); | ||
| inFlight.current = false; | ||
| } |
There was a problem hiding this comment.
clearDeleting on success path may briefly flash the row
On the success path, onDeleted?.() (which removes the workspace from sidebar state) is called in the try block and clearDeleting(workspaceId) runs afterwards in finally. In React 18 with automatic batching these two state updates will almost always be coalesced into one render — so the row would unmount without ever becoming visible again.
However, if onDeleted performs an asynchronous step before actually updating sidebar state (e.g. an async removal from a store), the clearDeleting call could land first in a separate render tick. At that instant isDeleting(id) returns false while the workspace is still present in the sidebar list, causing the row to flash visible for one frame before the sidebar update unmounts it.
If onDeleted is always synchronous this is a non-issue, but it may be worth moving clearDeleting inside the try block (before onDeleted) so the sequence is deterministically: row hidden → mark not-deleting → unmount due to onDeleted. The current code is safe as long as onDeleted only calls synchronous state setters.
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: 85-88
Comment:
**`clearDeleting` on success path may briefly flash the row**
On the success path, `onDeleted?.()` (which removes the workspace from sidebar state) is called in the `try` block and `clearDeleting(workspaceId)` runs afterwards in `finally`. In React 18 with automatic batching these two state updates will almost always be coalesced into one render — so the row would unmount without ever becoming visible again.
However, if `onDeleted` performs an asynchronous step before actually updating sidebar state (e.g. an async removal from a store), the `clearDeleting` call could land first in a separate render tick. At that instant `isDeleting(id)` returns `false` while the workspace is still present in the sidebar list, causing the row to flash visible for one frame before the sidebar update unmounts it.
If `onDeleted` is always synchronous this is a non-issue, but it may be worth moving `clearDeleting` inside the `try` block (before `onDeleted`) so the sequence is deterministically: row hidden → mark not-deleting → unmount due to onDeleted. The current code is safe as long as `onDeleted` only calls synchronous state setters.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
5 tasks
13 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 23, 2026
upstream commit 3902e3b (superset-sh#3621) が layout.tsx から MainWindowEffects wrapper を外して AgentHooks / ScheduleFireToasts / WorkspaceInitEffects を直接 mount するように変更していた。この変更は fork の tearoff window 前提を壊す: tearoff でも authenticated tree が描画されるため、これら singleton エフェクト (useDevicePresence / useCommandWatcher / tRPC subscription / workspace init subscription) が main window と tearoff で二重に走り、agent orchestration および workspace init が多重起動するリスクがある。 MainWindowEffects コンポーネント自体はリポジトリに残っており (isTearoffWindow + window.shouldOwnSingletonEffects でガードする実装)、 単に layout.tsx の呼び出しだけが置き換えられていた。直接 mount を <MainWindowEffects /> に戻すことで upstream 変更を巻き戻す。 Refs: CodeRabbit on PR #388 (Major).
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
workspaceCleanup.destroycan take 10–20s. The previoustoast.loading → successsat onscreen the whole time and the sidebar row stayed put, so clicking Delete felt like nothing was happening.onDeletedremoves thev2WorkspaceLocalStateentry and the row unmounts naturally. On error the row reappears.Mechanism: a new
DeletingWorkspacesProvidertracks in-flight deletions with{ isDeleting, markDeleting, clearDeleting }.useDestroyDialogStatemarks on confirm and clears infinally;DashboardSidebarWorkspaceItemwraps its visible content in<div hidden={isDeleting}>.Test plan
handleDeletedflow, unchanged).toast.warnings.Summary by cubic
Optimistically hide v2 workspace rows during deletion and remove the loading toast for faster, clearer feedback. Rows reappear on error, and the delete dialog reopens for conflict/teardown cases.
DeletingWorkspacesProviderwithisDeleting,markDeleting, andclearDeletingto track in-flight destroys.DashboardSidebarWorkspaceItemto hide visible content withhidden={isDeleting}; keep the delete dialog outside the wrapper so errors can reopen correctly.useDestroyDialogStateto mark/clear deleting, droptoast.loading, keeptoast.warningfor destroy warnings, and showtoast.errorfor unknown errors; on success, the row unmounts via existing state.DeletingWorkspacesProviderin_authenticated/layout.tsx.Written for commit bd0edda. Summary will update on new commits.
Summary by CodeRabbit
New Features
User Experience