[codex] Enable Enter on destructive alert dialogs#2995
Merged
Kitenite merged 3 commits intoMar 29, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughAn Enter-enabled alert-dialog flow was added: a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog as EnterEnabledAlertDialogContent
participant Utility as focusEnterEnabledAlertDialogPrimaryAction
participant DOM as DOM Query
participant Button as Primary Action Button
User->>Dialog: Dialog opens (onOpenAutoFocus triggered)
Dialog->>Utility: invoke onOpenAutoFocus handler with event
Utility->>Utility: if event.defaultPrevented? (exit if true)
Utility->>DOM: query currentTarget for [data-slot='alert-dialog-action']:not([disabled])
alt Primary action found
DOM-->>Utility: return button element
Utility->>Utility: call event.preventDefault()
Utility->>Button: call focus()
else No primary action found
DOM-->>Utility: return null
end
Utility-->>Dialog: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts (1)
36-50: Consider adding a disabled-primary regression test.Since the selector explicitly excludes disabled actions, a test with a marked-but-disabled button would lock in that contract.
🧪 Suggested test addition
describe("focusEnterEnabledAlertDialogPrimaryAction", () => { + test("does nothing when only a disabled primary action exists", () => { + let prevented = false; + let focused = false; + + focusEnterEnabledAlertDialogPrimaryAction({ + currentTarget: { + querySelector: (selector: string) => { + expect(selector).toBe(alertDialogPrimaryActionSelector); + return null; // simulates selector excluding disabled primary + }, + }, + defaultPrevented: false, + preventDefault: () => { + prevented = true; + }, + }); + + expect(prevented).toBe(false); + expect(focused).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts` around lines 36 - 50, Add a regression test in focus-enter-enabled-alert-dialog-primary-action.test.ts that verifies a marked-but-disabled primary action does not trigger or prevent default: call focusEnterEnabledAlertDialogPrimaryAction with an event whose currentTarget.querySelector returns an element that matches the primary-action marker used in the implementation (e.g., an element with the same data attribute or class the function searches for) but has the disabled property true, and assert that the event.preventDefault() was not called (prevented remains false); this ensures the function's exclusion of disabled buttons is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts`:
- Around line 36-50: Add a regression test in
focus-enter-enabled-alert-dialog-primary-action.test.ts that verifies a
marked-but-disabled primary action does not trigger or prevent default: call
focusEnterEnabledAlertDialogPrimaryAction with an event whose
currentTarget.querySelector returns an element that matches the primary-action
marker used in the implementation (e.g., an element with the same data attribute
or class the function searches for) but has the disabled property true, and
assert that the event.preventDefault() was not called (prevented remains false);
this ensures the function's exclusion of disabled buttons is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a90d725-5bfe-4255-ab2d-465a5b43ae00
📒 Files selected for processing (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/CloseProjectDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/UnsavedChangesDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/DiscardConfirmDialog/DiscardConfirmDialog.tsxpackages/ui/src/components/ui/alert-dialog.tsxpackages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.tspackages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/src/components/ui/alert-dialog.tsx (2)
7-7: Co-locate the focus helper with this component.Line 7 pulls an alert-dialog-specific helper from
packages/ui/src/lib, which makes the selector and the consuming markup easier to evolve separately. Keeping it next toalert-dialog.tsxwould better match the ownership boundary here.As per coding guidelines, "
**/components/**/*.{ts,tsx}: Co-locate utils, hooks, constants, config, tests, and stories next to the file using them`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ui/alert-dialog.tsx` at line 7, Move the alert-dialog-specific helper focusEnterEnabledAlertDialogPrimaryAction out of packages/ui/src/lib and co-locate it with alert-dialog.tsx (e.g., create a sibling file like focus-enter-enabled-alert-dialog-primary-action.ts or inline it), update the import in alert-dialog.tsx to the new relative path, and remove or refactor the original exported helper from the lib area (or re-export it from the new location) so the selector and consuming markup remain owned by the component; ensure any other consumers update their imports to the new module.
144-155: Expose an explicit Enter target instead of matching every action.Line 153 makes every
AlertDialogActioneligible for the helper selector, andfocusEnterEnabledAlertDialogPrimaryActioncurrently takes the first enabled match. That is fine for single-action dialogs, but reusable dialogs with two confirm actions would end up relying on DOM order instead of an explicit primary target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ui/alert-dialog.tsx` around lines 144 - 155, The helper currently matches every AlertDialogAction and picks the first enabled one; update AlertDialogAction to accept an explicit enter-target prop (e.g., enterTarget?: boolean or enterKeyTarget?: 'primary') and when set render a distinct data attribute (e.g., data-enter-target="primary") on the AlertDialogPrimitive.Action element; then change the helper selector focusEnterEnabledAlertDialogPrimaryAction to prefer elements with that attribute ([data-enter-target="primary"]) so dialogs with multiple confirm actions can opt one explicit primary enter target instead of relying on DOM order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/ui/alert-dialog.tsx`:
- Line 7: Move the alert-dialog-specific helper
focusEnterEnabledAlertDialogPrimaryAction out of packages/ui/src/lib and
co-locate it with alert-dialog.tsx (e.g., create a sibling file like
focus-enter-enabled-alert-dialog-primary-action.ts or inline it), update the
import in alert-dialog.tsx to the new relative path, and remove or refactor the
original exported helper from the lib area (or re-export it from the new
location) so the selector and consuming markup remain owned by the component;
ensure any other consumers update their imports to the new module.
- Around line 144-155: The helper currently matches every AlertDialogAction and
picks the first enabled one; update AlertDialogAction to accept an explicit
enter-target prop (e.g., enterTarget?: boolean or enterKeyTarget?: 'primary')
and when set render a distinct data attribute (e.g.,
data-enter-target="primary") on the AlertDialogPrimitive.Action element; then
change the helper selector focusEnterEnabledAlertDialogPrimaryAction to prefer
elements with that attribute ([data-enter-target="primary"]) so dialogs with
multiple confirm actions can opt one explicit primary enter target instead of
relying on DOM order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c3b95bd-638c-4120-8f0d-b659935fee74
📒 Files selected for processing (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/CloseProjectDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/UnsavedChangesDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/DiscardConfirmDialog/DiscardConfirmDialog.tsxpackages/ui/src/components/ui/alert-dialog.tsxpackages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.tspackages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/UnsavedChangesDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/DiscardConfirmDialog/DiscardConfirmDialog.tsx
- packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx
…to-be-triggerable-by-enter-ke # Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
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
EnterEnabledAlertDialogContentinpackages/uithat focuses the dialog'sAlertDialogActionwhen the dialog opensAlertDialogActionto carry the shared action slot and support buttonvariantandsizepropsAlertDialogActionimmediatelyWhy
The discard and close confirmation dialogs did not consistently route Enter to the intended destructive action. The behavior was being handled locally instead of through the shared alert-dialog primitives.
Impact
Desktop discard and close confirmations now respond to Enter by activating the primary destructive
AlertDialogActionwithout changing unrelated alert dialogs globally.Validation
bun test packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.test.tsbun run --cwd packages/ui typecheckbunx biome check --write packages/ui/src/components/ui/alert-dialog.tsx packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.ts packages/ui/src/lib/focus-enter-enabled-alert-dialog-primary-action.test.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/UnsavedChangesDialog.tsx apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/DiscardConfirmDialog/DiscardConfirmDialog.tsx apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/CloseProjectDialog.tsx apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxSummary by CodeRabbit