🐛 Prevented Escape keypresses in welcome emails Customize modal from exiting settings#27516
Conversation
The new unit test captures the early Escape race where the color picker has not mounted yet and the parent customize modal closes, so we have a deterministic red test before fixing the dismissal handling.
The previous test left a render tick between opening the picker and pressing Escape, which missed the race that dismisses the modal.
Immediate Escape presses could bypass the color picker, leak to the settings shell, and navigate back to Analytics. This keeps Escape scoped to the open picker and ensures the settings shell recognizes Shade dialogs as active modals.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a capturing-window 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/admin/settings/member-welcome-emails.test.ts`:
- Line 384: Rename the test title string in the test(...) call for the Escape
behavior so it follows the lowercase "what is tested - expected outcome"
convention; change the current title "Escape closes welcome email color picker
without bypassing unsaved changes confirmation" to a lowercase, dash-separated
form such as "escape closes welcome email color picker - does not bypass unsaved
changes confirmation" by updating the first argument to the test(...)
invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ec99ab7-c093-4a30-bdc4-2d1a20ac1fed
📒 Files selected for processing (4)
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsxapps/admin-x-settings/src/main-content.tsxapps/admin-x-settings/test/unit/membership/welcome-email-customize-modal.test.tsxe2e/tests/admin/settings/member-welcome-emails.test.ts
This trims unnecessary useCallback wrappers from the local reset and close helpers while keeping the global Escape listener callbacks stable for add/removeEventListener cleanup.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx (1)
113-121: Minor: redundant reset/detach on close path.When
nextOpenisfalse, this path callsresetInteractionState()anddetachEarlyEscapeListener(), whichclosePopover()already performs before drivingsetOpen(false). It's harmless (both are idempotent), but you could simplify by only resetting/attaching on the open transition:♻️ Optional simplification
onOpenChange={(nextOpen) => { - resetInteractionState(); if (nextOpen) { + resetInteractionState(); attachEarlyEscapeListener(); } else { detachEarlyEscapeListener(); } setOpen(nextOpen); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx` around lines 113 - 121, The onOpenChange handler currently calls resetInteractionState() and detachEarlyEscapeListener() for both open and close transitions even though closePopover() already resets state and detaches the listener before setOpen(false); update onOpenChange to only call resetInteractionState() and attachEarlyEscapeListener() when nextOpen is true and remove those calls from the false branch so closing relies on closePopover() and setOpen(false) as intended. Ensure you leave setOpen(nextOpen) and the existing attachEarlyEscapeListener() call for the open path intact and reference the onOpenChange, resetInteractionState, attachEarlyEscapeListener, detachEarlyEscapeListener, closePopover, and setOpen symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx`:
- Around line 113-121: The onOpenChange handler currently calls
resetInteractionState() and detachEarlyEscapeListener() for both open and close
transitions even though closePopover() already resets state and detaches the
listener before setOpen(false); update onOpenChange to only call
resetInteractionState() and attachEarlyEscapeListener() when nextOpen is true
and remove those calls from the false branch so closing relies on closePopover()
and setOpen(false) as intended. Ensure you leave setOpen(nextOpen) and the
existing attachEarlyEscapeListener() call for the open path intact and reference
the onOpenChange, resetInteractionState, attachEarlyEscapeListener,
detachEarlyEscapeListener, closePopover, and setOpen symbols when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 969d6cf2-ca48-4c66-89c9-42ca573f46f2
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx
This removes the last unnecessary useCallback wrapper so the early Escape path reads more directly while preserving the same listener cleanup behavior.
| await expect(page).toHaveURL(/\/ghost\/#\/settings\/memberemails$/); | ||
| }); | ||
|
|
||
| test.skip('Escape closes welcome email color picker without bypassing unsaved changes confirmation', async ({page}) => { |
There was a problem hiding this comment.
only changes here are un-skipping this test, and updating the names to follow the convention, as pointed out by coderabbit.
This extracts the open Shade dialog selector into a named constant and documents why settings checks both the legacy modal backdrop and newer Shade/Radix dialogs before handling Escape globally.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/main-content.tsx (1)
45-50: Honor already-handled Escape events before the page shortcut.If a nested dialog/control calls
preventDefault()but doesn’t stop propagation, this window listener can still run. Bail out first so one Escape cannot both dismiss inner UI and trigger settings navigation if modal DOM state changes during the same event.Suggested hardening
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Escape') { + if (event.defaultPrevented) { + return; + } + // Don't navigate away if a modal is open - let the modal handle ESC if (hasOpenModal()) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/main-content.tsx` around lines 45 - 50, The Escape handler can run even if a nested control already called event.preventDefault(); update handleKeyDown to bail out early by checking event.defaultPrevented (e.g., if (event.defaultPrevented) return;) before any logic, then keep the existing hasOpenModal() check so a single Escape cannot both dismiss inner UI and trigger navigation via handleKeyDown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin-x-settings/src/main-content.tsx`:
- Around line 45-50: The Escape handler can run even if a nested control already
called event.preventDefault(); update handleKeyDown to bail out early by
checking event.defaultPrevented (e.g., if (event.defaultPrevented) return;)
before any logic, then keep the existing hasOpenModal() check so a single Escape
cannot both dismiss inner UI and trigger navigation via handleKeyDown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 632f0262-8896-4e6e-8e40-43a1e0a3d2b6
📒 Files selected for processing (1)
apps/admin-x-settings/src/main-content.tsx
|
cmraible
added
the
ok to merge for me
| // Don't navigate away if a modal is open - let the modal handle ESC | ||
| const modalBackdrop = document.getElementById('modal-backdrop'); | ||
| if (modalBackdrop) { | ||
| if (hasOpenModal()) { |
There was a problem hiding this comment.
interesting TIL - i was surprised the linter wasn't yelling about hasOpenModal not being in the dep array on L63, but it's because there's no state or reactivity or anything in the function, so it doesn't need to be.
I'd always thought every function/variable got declared there but I guess not! Probably because like 98% of the time they do have React stuff going on.
troyciesco
deleted the
chris-ny-1234-flaky-member-welcome-emails-e2e-test
branch
2 participants
ref https://linear.app/ghost/issue/NY-1234/flaky-member-welcome-emails-e2e-test
Why this change?
Pressing
Escapeinside the welcome emails "Customize" flow could exit settings instead of just dismissing the active UI layer. That made the interaction flaky in E2E and, more importantly, let a nested control bypass the expected unsaved-changes flow.Reproduction
Expected behavior
Escape should close the color picker, but leave the Customize modal open
Actual behavior
If you time it just right, the Customize modal will close, then Settings will close, and you'll land on the Analytics screen (or where ever you were before opening settings to begin with).
Root cause
admin-x-settingshas a page-levelEscapehandler that navigates away from settings when no modal is open.The welcome email customize UI is built with Shade/Radix dialogs and popovers, but the page-level guard was still only checking for the legacy
#modal-backdropelement. That created two failure modes:Escape.onEscapeKeyDownhandler cannot intercept the keypress. In that gap,Escapecan bubble to the page-level handler and exit settings.Why this fix is correct
This fix closes the gap at the right layers instead of weakening the global shortcut.
MainContentnow treats open Radixdialogandalertdialogelements as active modals, not just the legacy backdrop. That keeps the page-levelEscapeshortcut disabled while the customize modal or unsaved-changes confirmation is open.ColorPickerFieldattaches an early capture-phaseEscapelistener as soon as the popover is opened. That means the firstEscapeis claimed by the color picker even if the popover content has not mounted yet. Once the popover is mounted, the normal popover escape handling still applies, and the temporary listener is removed when the popover closes or the component unmounts.Together, those changes preserve the intended dismissal order:
Escape: close the nested color picker.Escape: show or dismiss the customize modal's unsaved-changes confirmation (if dirty).Escape.Test coverage
The PR adds:
Escapecloses the color picker first and still respects the unsaved-changes confirmation flow