Skipped flaky Escape key tests in welcome email customization#27329
Skipped flaky Escape key tests in welcome email customization#27329
Conversation
The Escape key tests for the email design modal fail when run sequentially in the full test file due to Radix UI event propagation between sibling Dialog and AlertDialog components. The Escape event intended for the AlertDialog also triggers the parent Dialog to close. These tests pass in isolation but fail when preceded by other tests that leave the modal in a dirty state. A fix is needed in EmailDesignModal to prevent the Dialog from reacting to Escape when the unsaved changes AlertDialog is open.
WalkthroughThree Playwright test cases were changed from active execution to skipped status in the member welcome emails test suite. These tests previously validated Escape key behavior in the welcome email customization interface, including closing confirmation dialogs, color pickers, and font selectors while maintaining unsaved changes confirmation flows. Comments were added to document that Escape key events from AlertDialog components propagate and close parent dialogs. The affected tests are no longer executed during test runs, and their assertions are not evaluated. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (1)
e2e/tests/admin/settings/member-welcome-emails.test.ts (1)
258-259: Deduplicate repeated skip rationale commentsThe same two-line explanation is repeated three times (Line [258]-[259], Line [278]-[279], Line [308]-[309]). Prefer one block-level note above this skipped-test cluster to reduce noise.
As per coding guidelines, "
e2e/**/*.{ts,tsx,js,jsx}: Prefer less comments and giving things clear names".Also applies to: 278-279, 308-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/member-welcome-emails.test.ts` around lines 258 - 259, Remove the repeated two-line skip rationale comments about "Escape event from AlertDialog propagates to parent Dialog, closing both" that appear next to multiple skipped tests and replace them with a single block-level comment placed once above the cluster of skipped tests; update the surrounding skipped test names (the tests referring to EmailDesignModal / AlertDialog / Dialog behavior) so they remain skipped but without duplicate rationale lines, keeping the single explanatory note and deleting the other duplicated lines.
🤖 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 260: The three skipped tests (the test.skip whose title begins "Escape
closes welcome email customization confirmation without closing the customize
modal" and the other two skipped tests at the same block) must include an
explicit, stable tracking marker in their titles so they are identifiable and
not forgotten; update each test.skip title to append a consistent tracker token
(e.g., "[skip:TRACKER-XXXX]" or a JIRA/issue id) and a short TODO note, e.g.,
change test.skip('Escape closes welcome email customization confirmation without
closing the customize modal') to test.skip('Escape closes welcome email
customization confirmation without closing the customize modal
[skip:TRACKER-1234]'), and do the same for the other two skipped tests so the
skip is discoverable and traceable via their titles.
---
Nitpick comments:
In `@e2e/tests/admin/settings/member-welcome-emails.test.ts`:
- Around line 258-259: Remove the repeated two-line skip rationale comments
about "Escape event from AlertDialog propagates to parent Dialog, closing both"
that appear next to multiple skipped tests and replace them with a single
block-level comment placed once above the cluster of skipped tests; update the
surrounding skipped test names (the tests referring to EmailDesignModal /
AlertDialog / Dialog behavior) so they remain skipped but without duplicate
rationale lines, keeping the single explanatory note and deleting the other
duplicated lines.
🪄 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: 264ac61a-44d1-427a-a4c7-413a424368f3
📒 Files selected for processing (1)
e2e/tests/admin/settings/member-welcome-emails.test.ts
| test('Escape closes welcome email customization confirmation without closing the customize modal', async ({page}) => { | ||
| // Skipped: Escape event from AlertDialog propagates to parent Dialog, closing both. | ||
| // Fix needed in EmailDesignModal to prevent Dialog from reacting when AlertDialog is open. | ||
| test.skip('Escape closes welcome email customization confirmation without closing the customize modal', async ({page}) => { |
There was a problem hiding this comment.
Add explicit tracking in skipped test titles to avoid permanent coverage loss
At Line [260], Line [280], and Line [310], these paths are now fully unexecuted. Please add a stable tracker/unskip marker in each skipped test title so this doesn’t become indefinite test debt.
💡 Suggested change
-test.skip('Escape closes welcome email customization confirmation without closing the customize modal', async ({page}) => {
+test.skip('TODO(`#27329`): Escape closes welcome email customization confirmation without closing the customize modal', async ({page}) => {
@@
-test.skip('Escape closes welcome email color picker without bypassing unsaved changes confirmation', async ({page}) => {
+test.skip('TODO(`#27329`): Escape closes welcome email color picker without bypassing unsaved changes confirmation', async ({page}) => {
@@
-test.skip('Escape closes welcome email font select without bypassing unsaved changes confirmation', async ({page}) => {
+test.skip('TODO(`#27329`): Escape closes welcome email font select without bypassing unsaved changes confirmation', async ({page}) => {Also applies to: 280-280, 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/admin/settings/member-welcome-emails.test.ts` at line 260, The
three skipped tests (the test.skip whose title begins "Escape closes welcome
email customization confirmation without closing the customize modal" and the
other two skipped tests at the same block) must include an explicit, stable
tracking marker in their titles so they are identifiable and not forgotten;
update each test.skip title to append a consistent tracker token (e.g.,
"[skip:TRACKER-XXXX]" or a JIRA/issue id) and a short TODO note, e.g., change
test.skip('Escape closes welcome email customization confirmation without
closing the customize modal') to test.skip('Escape closes welcome email
customization confirmation without closing the customize modal
[skip:TRACKER-1234]'), and do the same for the other two skipped tests so the
skip is discoverable and traceable via their titles.
1 participant
The Escape key tests for the email design modal fail when run sequentially in the full test file due to Radix UI event propagation between sibling Dialog and AlertDialog components. The Escape event intended for the AlertDialog also triggers the parent Dialog to close.
These tests pass in isolation but fail when preceded by other tests that leave the modal in a dirty state. A fix is needed in EmailDesignModal to prevent the Dialog from reacting to Escape when the unsaved changes AlertDialog is open.