feat(desktop): confirm before removing workspace from sidebar#4524
Conversation
Wires every "Remove from sidebar" entry point (sidebar minus button, context menu, command palette, Workspaces page row) through the existing remove-from-sidebar intent store, which now drives an AlertDialog that reminds users the workspace can be re-added from the Workspaces page.
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
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 (5)
📝 WalkthroughWalkthroughThe PR adds workspace name to the remove-from-sidebar workflow and replaces tick-based mount logic with a visible AlertDialog confirmation. Multiple UI entry points now pass workspace name through the intent request, which the confirmation dialog displays before removal. ChangesRemove Workspace from Sidebar Intent Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 converts the "Remove from sidebar" action from an auto-executing side-effect into a confirmation
Confidence Score: 4/5Safe to merge; the change is well-scoped and all removal paths correctly funnel through the new dialog before any sidebar state is mutated. The refactor is straightforward — all entry points now call the shared intent store, and the dialog confirm/cancel paths correctly call clear() to reset state. Two minor issues exist: the tick field is still being incremented in the store but is never read after the useEffect removal, and the dialog uses plain Button components instead of Radix AlertDialogAction/AlertDialogCancel, which may affect screen-reader focus order. Neither affects functional correctness for mouse or keyboard users in the common case. The remove-workspace-from-sidebar-intent.ts store retains a tick field that no longer serves a purpose, and RemoveFromSidebarMount.tsx warrants a second look for accessibility if the team cares about screen-reader support.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/commandPalette/ui/RemoveFromSidebarMount/RemoveFromSidebarMount.tsx | Core UI change: replaces a silent useEffect executor with an AlertDialog; uses plain Button instead of AlertDialogAction/AlertDialogCancel primitives, and the tick field is no longer consumed here. |
| apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts | Adds workspaceName to the target interface; tick is still incremented on every request but is no longer read anywhere after the useEffect was removed. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsx | Delegates removal to the shared intent store; AccessibleV2Workspace.projectId is typed as string so no null-coalescing guard is needed here. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts | Correctly replaces direct sidebar mutations with a request to the shared intent store, passing workspaceName through. |
| apps/desktop/src/renderer/commandPalette/modules/workspace/commands.tsx | Minimal one-line addition of workspaceName to the intent request; already guarded by the existing if (workspace.projectId) check. |
Sequence Diagram
sequenceDiagram
participant U as User
participant E as Entry Point
participant S as useRemoveFromSidebarIntent
participant D as RemoveFromSidebarMount
participant SB as useDashboardSidebarState
U->>E: Click "Remove from sidebar"
E->>S: "request({ workspaceId, workspaceName, projectId, isMain })"
S->>S: set target (tick++)
S-->>D: target non-null → dialog opens
alt User clicks Remove
U->>D: onClick handleConfirm
D->>SB: navigateAwayFromWorkspace
alt isMain
D->>SB: hideWorkspaceInSidebar
else
D->>SB: removeWorkspaceFromSidebar
end
D->>S: clear() → dialog closes
else User clicks Cancel or Escape
U->>D: onOpenChange(false)
D->>S: clear() → dialog closes
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts:8
**`tick` is now dead state**
`tick` is incremented on every `request()` call but is no longer consumed anywhere — the old `useEffect` guard (`target.tick === lastTickRef.current`) was removed when `RemoveFromSidebarMount` became an `AlertDialog`. The dialog open/close state is driven entirely by `!!target`, so the field serves no runtime purpose. Consider removing `tick` from `RemoveFromSidebarTarget` and the `request` implementation to avoid confusion for future readers.
### Issue 2 of 2
apps/desktop/src/renderer/commandPalette/ui/RemoveFromSidebarMount/RemoveFromSidebarMount.tsx:59-75
**Plain `Button` bypasses `AlertDialog` accessibility primitives**
Radix UI's `AlertDialogAction` and `AlertDialogCancel` carry built-in accessibility semantics for the `alertdialog` role — `AlertDialogCancel` receives focus by default on open, and both handle keyboard dismiss correctly with their ARIA roles. Using plain `Button` components means those semantics are absent. The current implementation works visually, but screen readers and keyboard-only users may not get the expected cancel-first focus order or the correct element roles.
Reviews (1): Last reviewed commit: "feat(desktop): confirm before removing w..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| workspaceName: string; | ||
| projectId: string; | ||
| isMain: boolean; | ||
| tick: number; |
There was a problem hiding this comment.
tick is now dead state
tick is incremented on every request() call but is no longer consumed anywhere — the old useEffect guard (target.tick === lastTickRef.current) was removed when RemoveFromSidebarMount became an AlertDialog. The dialog open/close state is driven entirely by !!target, so the field serves no runtime purpose. Consider removing tick from RemoveFromSidebarTarget and the request implementation to avoid confusion for future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts
Line: 8
Comment:
**`tick` is now dead state**
`tick` is incremented on every `request()` call but is no longer consumed anywhere — the old `useEffect` guard (`target.tick === lastTickRef.current`) was removed when `RemoveFromSidebarMount` became an `AlertDialog`. The dialog open/close state is driven entirely by `!!target`, so the field serves no runtime purpose. Consider removing `tick` from `RemoveFromSidebarTarget` and the `request` implementation to avoid confusion for future readers.
How can I resolve this? If you propose a fix, please make it concise.| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 px-3 text-xs" | ||
| onClick={() => handleOpenChange(false)} | ||
| > | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| variant="destructive" | ||
| size="sm" | ||
| className="h-7 px-3 text-xs" | ||
| onClick={handleConfirm} | ||
| > | ||
| Remove | ||
| </Button> | ||
| </AlertDialogFooter> |
There was a problem hiding this comment.
Button bypasses AlertDialog accessibility primitives
Radix UI's AlertDialogAction and AlertDialogCancel carry built-in accessibility semantics for the alertdialog role — AlertDialogCancel receives focus by default on open, and both handle keyboard dismiss correctly with their ARIA roles. Using plain Button components means those semantics are absent. The current implementation works visually, but screen readers and keyboard-only users may not get the expected cancel-first focus order or the correct element roles.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/commandPalette/ui/RemoveFromSidebarMount/RemoveFromSidebarMount.tsx
Line: 59-75
Comment:
**Plain `Button` bypasses `AlertDialog` accessibility primitives**
Radix UI's `AlertDialogAction` and `AlertDialogCancel` carry built-in accessibility semantics for the `alertdialog` role — `AlertDialogCancel` receives focus by default on open, and both handle keyboard dismiss correctly with their ARIA roles. Using plain `Button` components means those semantics are absent. The current implementation works visually, but screen readers and keyboard-only users may not get the expected cancel-first focus order or the correct element roles.
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
useRemoveFromSidebarIntentstore.RemoveFromSidebarMountfrom an auto-executing effect into anAlertDialogthat warns the user and notes the workspace can be re-added from the Workspaces page.workspaceNameto the intent target so the dialog can name what's being removed.Test plan
⌘Kcommand palette → "Remove from sidebar" → same dialog flow.hideWorkspaceInSidebar; non-main routes throughremoveWorkspaceFromSidebar.Summary by cubic
Add a confirmation dialog before removing a workspace from the sidebar and route all remove actions through a shared intent store. This prevents accidental removals and keeps behavior consistent across the sidebar, context menu, command palette, and Workspaces page.
useRemoveFromSidebarIntent.AlertDialog(@superset/ui/alert-dialog,@superset/ui/button) that names the workspace and notes it can be re-added from the Workspaces page.workspaceName; main workspaces still callhideWorkspaceInSidebar, others callremoveWorkspaceFromSidebar.Written for commit 38646bb. Summary will update on new commits.
Summary by CodeRabbit
Release Notes