[codex] Add v2 workspace delete hotkey support#4673
Conversation
|
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 (1)
📝 WalkthroughWalkthroughAdds a helper to detect plain Enter key presses, wires window keydown listeners into delete dialog panes to trigger confirm/force-delete, extends dashboard layout to support v2 workspace deletion via a DeleteTarget and live query, and shows the close-workspace hotkey in workspace menus when active. ChangesV2 workspace deletion with keyboard confirmation
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)
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 |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 5786e9b on May 17, 2026 6:19pm UTC. |
Greptile SummaryThis PR wires the existing
Confidence Score: 3/5The hotkey and dialog wiring is correct, but the window-level Enter listener in both confirmation panes does not check the focused element, so a user who tabs to the Cancel button and presses Enter will trigger workspace deletion instead of dismissal. The core routing and state logic in layout.tsx is sound and the shouldConfirmDeleteDialogKey predicate and its tests are well-written. However, the window.addEventListener keydown approach in DestroyConfirmPane and TeardownFailedPane intercepts Enter unconditionally — including when a Cancel button is focused — calling event.preventDefault() which suppresses the button's native Enter→click, then immediately invoking the destructive action. This is a real, reproducible way for a keyboard user to accidentally delete a workspace when trying to cancel. DestroyConfirmPane.tsx and TeardownFailedPane.tsx need an event.target instanceof HTMLButtonElement guard before the confirm logic fires.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx | Adds v2 workspace delete hotkey support via a new DeleteTarget union type and a useLiveQuery for the active v2 workspace; the v2 dialog dismiss handler keeps the component mounted rather than clearing state to null as v1 does. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx | Adds a window-level keydown listener to confirm deletion on Enter; the listener does not filter button targets, so pressing Enter while the Cancel button is focused calls onConfirm() instead of dismissing the dialog. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/TeardownFailedPane/TeardownFailedPane.tsx | Adds the same window-level Enter keydown listener to trigger force-delete; inherits the same Cancel-button focus issue as DestroyConfirmPane. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.ts | New pure predicate that correctly guards for unmodified Enter, composition events (isComposing / keyCode 229), and all modifier keys. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts | Focused unit tests covering the predicate for plain Enter, all modifier combinations, IME composition, and non-Enter keys — all cases correctly verified. |
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/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsx:45-56
**Enter on Cancel button triggers accidental deletion**
The `window` keydown listener fires during the bubble phase for every keydown in the document, including when focus is on the Cancel button. Pressing Enter with the Cancel button focused calls `event.preventDefault()` (suppressing the button's native Enter→click activation) and then calls `onConfirm()`, which deletes the workspace. The user intended to cancel but instead confirms a destructive action.
The same pattern applies to `TeardownFailedPane` — pressing Enter on its Cancel button calls `onForceDelete()`.
The fix is to exit early when the event target is any button element, letting normal button click-from-Enter behavior proceed unimpeded: add `if (event.target instanceof HTMLButtonElement) return;` before the `shouldConfirmDeleteDialogKey` call in both components.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx:222-227
**V2 dismiss leaks a mounted-but-hidden dialog**
When the user dismisses the v2 delete dialog (Escape or clicking outside), `onOpenChange(false)` sets `deleteTarget.open = false` but leaves `DashboardSidebarDeleteDialog` mounted indefinitely. The v1 path sets `deleteTarget` to `null` on close, which unmounts the component. Because `useDestroyDialogState` already resets both `error` and `inspectState` to their initial values when `open` goes `false`, there is no state worth preserving across a dismiss — setting `deleteTarget` to `null` here, as v1 does, would match that behavior and avoid keeping the component alive unnecessarily.
Reviews (1): Last reviewed commit: "Add v2 workspace delete hotkey support" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx (1)
218-227: ⚡ Quick winClear the v2 delete target when the dialog closes.
Unlike the v1 path, closing the v2 dialog keeps the component mounted with
deleteTarget.version === "v2". That is a risky fit with the new document-level Enter listeners added to the v2 delete panes in this PR. Mirror the v1 behavior and reset the state tonullwhenopenbecomesfalse.Suggested change
{deleteTarget?.version === "v2" && ( <DashboardSidebarDeleteDialog workspaceId={deleteTarget.workspaceId} workspaceName={deleteTarget.workspaceName} open={deleteTarget.open} onOpenChange={(open) => { - setDeleteTarget((target) => - target?.version === "v2" ? { ...target, open } : target, - ); + setDeleteTarget((target) => { + if (target?.version !== "v2") return target; + return open ? { ...target, open: true } : null; + }); }} onDeleted={() => { removeWorkspaceFromSidebar(deleteTarget.workspaceId); setDeleteTarget(null); }} /> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx` around lines 218 - 227, The v2 delete dialog handler leaves deleteTarget.version === "v2" mounted when closed; update the onOpenChange for DashboardSidebarDeleteDialog so that when open becomes false you clear the v2 target (call setDeleteTarget(null)) and when open is true preserve/update the target (e.g., set the { ...target, open } only if target?.version === "v2"); reference: deleteTarget, DashboardSidebarDeleteDialog, setDeleteTarget.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts (1)
17-24: ⚡ Quick winConsider adding test coverage for ctrlKey and altKey modifiers.
The test verifies rejection of
metaKeyandshiftKey, but the implementation also rejectsctrlKeyandaltKey(source lines 19-20). Adding cases for those modifiers would complete the coverage.🧪 Proposed test additions
test("rejects modified Enter", () => { expect(shouldConfirmDeleteDialogKey({ ...plainEnter, metaKey: true })).toBe( false, ); expect( shouldConfirmDeleteDialogKey({ ...plainEnter, shiftKey: true }), ).toBe(false); + expect( + shouldConfirmDeleteDialogKey({ ...plainEnter, ctrlKey: true }), + ).toBe(false); + expect( + shouldConfirmDeleteDialogKey({ ...plainEnter, altKey: true }), + ).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts` around lines 17 - 24, The test file only checks that shouldConfirmDeleteDialogKey rejects modified Enter for metaKey and shiftKey but omits ctrlKey and altKey; add assertions in the "rejects modified Enter" test to also expect shouldConfirmDeleteDialogKey({ ...plainEnter, ctrlKey: true }) and shouldConfirmDeleteDialogKey({ ...plainEnter, altKey: true }) to be false so all modifier branches in shouldConfirmDeleteDialogKey are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts`:
- Around line 17-24: The test file only checks that shouldConfirmDeleteDialogKey
rejects modified Enter for metaKey and shiftKey but omits ctrlKey and altKey;
add assertions in the "rejects modified Enter" test to also expect
shouldConfirmDeleteDialogKey({ ...plainEnter, ctrlKey: true }) and
shouldConfirmDeleteDialogKey({ ...plainEnter, altKey: true }) to be false so all
modifier branches in shouldConfirmDeleteDialogKey are covered.
In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx`:
- Around line 218-227: The v2 delete dialog handler leaves deleteTarget.version
=== "v2" mounted when closed; update the onOpenChange for
DashboardSidebarDeleteDialog so that when open becomes false you clear the v2
target (call setDeleteTarget(null)) and when open is true preserve/update the
target (e.g., set the { ...target, open } only if target?.version === "v2");
reference: deleteTarget, DashboardSidebarDeleteDialog, setDeleteTarget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14219042-a1dc-4596-a594-3ed037a8e170
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/DestroyConfirmPane/DestroyConfirmPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/components/TeardownFailedPane/TeardownFailedPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!open || !canConfirm) return; | ||
|
|
||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (!shouldConfirmDeleteDialogKey(event)) return; | ||
| event.preventDefault(); | ||
| onConfirm(); | ||
| }; | ||
|
|
||
| window.addEventListener("keydown", handleKeyDown); | ||
| return () => window.removeEventListener("keydown", handleKeyDown); | ||
| }, [canConfirm, onConfirm, open]); |
There was a problem hiding this comment.
The window keydown listener fires during the bubble phase for every keydown in the document, including when focus is on the Cancel button. Pressing Enter with the Cancel button focused calls event.preventDefault() (suppressing the button's native Enter→click activation) and then calls onConfirm(), which deletes the workspace. The user intended to cancel but instead confirms a destructive action.
The same pattern applies to TeardownFailedPane — pressing Enter on its Cancel button calls onForceDelete().
The fix is to exit early when the event target is any button element, letting normal button click-from-Enter behavior proceed unimpeded: add if (event.target instanceof HTMLButtonElement) return; before the shouldConfirmDeleteDialogKey call in both components.
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: 45-56
Comment:
**Enter on Cancel button triggers accidental deletion**
The `window` keydown listener fires during the bubble phase for every keydown in the document, including when focus is on the Cancel button. Pressing Enter with the Cancel button focused calls `event.preventDefault()` (suppressing the button's native Enter→click activation) and then calls `onConfirm()`, which deletes the workspace. The user intended to cancel but instead confirms a destructive action.
The same pattern applies to `TeardownFailedPane` — pressing Enter on its Cancel button calls `onForceDelete()`.
The fix is to exit early when the event target is any button element, letting normal button click-from-Enter behavior proceed unimpeded: add `if (event.target instanceof HTMLButtonElement) return;` before the `shouldConfirmDeleteDialogKey` call in both components.
How can I resolve this? If you propose a fix, please make it concise.| open={deleteTarget.open} | ||
| onOpenChange={(open) => { | ||
| setDeleteTarget((target) => | ||
| target?.version === "v2" ? { ...target, open } : target, | ||
| ); | ||
| }} |
There was a problem hiding this comment.
When the user dismisses the v2 delete dialog (Escape or clicking outside), onOpenChange(false) sets deleteTarget.open = false but leaves DashboardSidebarDeleteDialog mounted indefinitely. The v1 path sets deleteTarget to null on close, which unmounts the component. Because useDestroyDialogState already resets both error and inspectState to their initial values when open goes false, there is no state worth preserving across a dismiss — setting deleteTarget to null here, as v1 does, would match that behavior and avoid keeping the component alive unnecessarily.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
Line: 222-227
Comment:
**V2 dismiss leaks a mounted-but-hidden dialog**
When the user dismisses the v2 delete dialog (Escape or clicking outside), `onOpenChange(false)` sets `deleteTarget.open = false` but leaves `DashboardSidebarDeleteDialog` mounted indefinitely. The v1 path sets `deleteTarget` to `null` on close, which unmounts the component. Because `useDestroyDialogState` already resets both `error` and `inspectState` to their initial values when `open` goes `false`, there is no state worth preserving across a dismiss — setting `deleteTarget` to `null` here, as v1 does, would match that behavior and avoid keeping the component alive unnecessarily.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts (1)
4-10: ⚡ Quick winAdd explicit type annotation to the mock object.
The
plainEnterobject lacks a type annotation. Explicitly typing it asConfirmDeleteDialogKeyEvent(or the expected event type) would improve type safety and make the test more self-documenting. As per coding guidelines, TypeScript code should prioritize type safety.🔧 Suggested typing improvement
+import type { ConfirmDeleteDialogKeyEvent } from "./shouldConfirmDeleteDialogKey"; + -const plainEnter = { +const plainEnter: ConfirmDeleteDialogKeyEvent = { key: "Enter", shiftKey: false, metaKey: false, ctrlKey: false, altKey: false, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts` around lines 4 - 10, Add an explicit type annotation to the mock event object `plainEnter`: declare it as `ConfirmDeleteDialogKeyEvent` (or the exact event type used by `shouldConfirmDeleteDialogKey`) so the test gains type safety and self-documentation; update the `plainEnter` variable declaration to use that type to satisfy TypeScript checks and align with coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.ts`:
- Around line 4-10: Add an explicit type annotation to the mock event object
`plainEnter`: declare it as `ConfirmDeleteDialogKeyEvent` (or the exact event
type used by `shouldConfirmDeleteDialogKey`) so the test gains type safety and
self-documentation; update the `plainEnter` variable declaration to use that
type to satisfy TypeScript checks and align with coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79976492-d6af-4c45-aca5-8db3009ab24e
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
* Add v2 workspace delete hotkey support * Show workspace delete hotkey in context menus * Address delete dialog review feedback * Keep Enter as delete dialog confirm * Simplify v2 delete dialog state
* Add v2 workspace delete hotkey support * Show workspace delete hotkey in context menus * Address delete dialog review feedback * Keep Enter as delete dialog confirm * Simplify v2 delete dialog state
1 participant
Summary
Adds v2 support for the existing current-workspace delete hotkey. The dashboard-level
CLOSE_WORKSPACEbinding now resolves the active v2 workspace and opens the v2 workspace destroy dialog for non-main workspaces, while preserving the existing v1 dialog path.The v2 delete confirmation flow now accepts plain Enter to confirm in both the normal delete confirmation pane and the teardown-failed "Delete anyway" pane. A shared predicate keeps modifier/composition handling consistent and covered by a focused test.
Impact
Users can delete the current v2 workspace with the same keyboard shortcut already shown in the sidebar, then confirm from the keyboard without reaching for the mouse.
Root Cause
The hotkey was registered globally as
CLOSE_WORKSPACE, but the dashboard layout only had v1 workspace data available when deciding what dialog to open. The v2 destroy dialog also only confirmed through click handlers.Validation
bun test apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarDeleteDialog/utils/shouldConfirmDeleteDialogKey.test.tsbun run lintbun run --cwd apps/desktop typecheckSummary by cubic
Adds v2 support for the delete-current-workspace hotkey and keeps Enter-to-confirm in the v2 delete dialog. Pressing the
CLOSE_WORKSPACEshortcut on a non‑main v2 workspace opens the v2 delete dialog; the v1 flow is unchanged.New Features
CLOSE_WORKSPACEopens the v2 delete dialog when on a v2 (non‑main) workspace; v1 flow unchanged.shouldConfirmDeleteDialogKey(with tests).CLOSE_WORKSPACEshortcut on the active workspace in both the dashboard and workspace sidebars when assigned.Refactors
Written for commit 5786e9b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests