Skip to content

Fixed Escape key closing both AlertDialog and Dialog in EmailDesignModal#27331

Open
cmraible wants to merge 1 commit intomainfrom
fix-escape-key-modal-flakiness
Open

Fixed Escape key closing both AlertDialog and Dialog in EmailDesignModal#27331
cmraible wants to merge 1 commit intomainfrom
fix-escape-key-modal-flakiness

Conversation

@cmraible
Copy link
Copy Markdown
Collaborator

@cmraible cmraible commented Apr 10, 2026

The Bug

EmailDesignModal renders two Radix UI components as siblings:

<>
    <Dialog>...</Dialog>           // the customize modal
    <AlertDialog>...</AlertDialog> // the "unsaved changes" confirmation
</>

Both are portaled to the document body independently. Radix registers Escape key listeners at the document level for each one.

When the user has unsaved changes and presses Escape:

  1. The Dialog's onEscapeKeyDown fires, calls handleClose(), sees dirty === true, and opens the AlertDialog.
  2. The AlertDialog is now visible. User presses Escape again.
  3. Both the AlertDialog's and Dialog's Escape handlers fire (they're independent document-level listeners, so stopPropagation between them has no effect).
  4. The AlertDialog closes (correct) — but the Dialog's handler also calls handleClose() again.
  5. handleClose() checks dirty, still true, calls setShowDirtyConfirm(true).
  6. React batches state updates, so setShowDirtyConfirm(false) (from step 4) and setShowDirtyConfirm(true) (from step 5) race — and in practice the Dialog just closes entirely.

The net result: pressing Escape on the "Are you sure?" dialog closes everything instead of just the dialog.

The Fix

A useRef (dirtyConfirmOpenRef) tracks synchronously whether the AlertDialog is open. Unlike React state, refs update immediately in the same tick with no batching delay.

  • When handleClose() opens the AlertDialog, it sets dirtyConfirmOpenRef.current = true.
  • When the AlertDialog's onOpenChange fires (open or close), it syncs the ref: dirtyConfirmOpenRef.current = isOpen.
  • The Dialog's onEscapeKeyDown and onOpenChange both check the ref before acting — if the confirm dialog is open, they do nothing.

This way, when Escape fires on the AlertDialog, the Dialog's handler sees dirtyConfirmOpenRef.current === true and skips, even though the state update from the AlertDialog closing hasn't committed yet.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8f528555-3844-423d-89ef-221c050ecbbb

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd1217 and bf035ff.

📒 Files selected for processing (3)
  • apps/admin-x-settings/src/components/settings/email-design/dirty-confirm-modal.tsx
  • apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx
  • e2e/tests/admin/settings/member-welcome-emails.test.ts

Walkthrough

This change extracts the unsaved changes confirmation modal logic into a new reusable DirtyConfirmModal component, which uses NiceModal for state management. The email-design-modal component is refactored to replace its local dirty-confirm state with calls to the new modal. The modal provides customizable title and description props, handles user intent through a close function, manages lifecycle cleanup, and intercepts escape-key event propagation. Three previously skipped E2E tests are activated to verify escape-key handling during the welcome email customization flow.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main fix: moving the dirty confirmation dialog to a separate NiceModal to resolve Escape key handling issues between nested Radix components.
Description check ✅ Passed The description comprehensively explains the bug, the root cause of the Escape key issue with batched React state updates, and the solution using a ref to track AlertDialog visibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-escape-key-modal-flakiness

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cmraible cmraible force-pushed the fix-escape-key-modal-flakiness branch from a4970bb to ecd2ff0 Compare April 10, 2026 04:36
The welcome email customize flow was flaky because the unsaved-changes confirmation lived as a nested Radix dismissable layer inside the parent customize dialog. When Escape dismissed the child confirmation, the parent dialog could also react to the same keypress and close unexpectedly, which caused the skipped e2e tests to fail intermittently.

Fixed it by moving the dirty-confirm flow into a separate NiceModal-managed Shade alert dialog instead of rendering it inside the customize dialog. That keeps the confirmation as a sibling modal instance, preserves the expected alertdialog semantics for the tests, and isolates Escape handling so the topmost layer closes without dismissing the parent modal.

Also re-enabled the skipped welcome email e2e tests that cover the confirmation, color picker, and font select Escape flows.
@cmraible cmraible force-pushed the fix-escape-key-modal-flakiness branch from ecd2ff0 to bf035ff Compare April 10, 2026 05:06
@sonarqubecloud
Copy link
Copy Markdown

@cmraible cmraible marked this pull request as ready for review April 10, 2026 05:41
@cmraible cmraible requested a review from 9larsons as a code owner April 10, 2026 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant