test(notification-channel): align channels e2e with alerts settings tabs - #3593
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe notification channel UI test now expects the alerts root redirect after creation. It then reopens the Notification Channels tab. The page object waits for the tab to become selected before verification. ChangesNotification channel navigation
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to This PR updates notification-channel end-to-end tests for the Alerts tab navigation and redirect behavior without changing product runtime behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: ✨ 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
apps/deploy-web/tests/ui/pages/AlertsPage.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3593 +/- ##
==========================================
- Coverage 77.00% 76.18% -0.83%
==========================================
Files 1150 1057 -93
Lines 30026 27589 -2437
Branches 7492 7023 -469
==========================================
- Hits 23121 21018 -2103
+ Misses 6076 5772 -304
+ Partials 829 799 -30
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. Small, self-contained e2e test fix that realigns the spec with the tab-based Alerts page from #3583; the one nit noted inline (duplicated tab locator regex) is a minor style suggestion, not a correctness issue.
Extended reasoning...
Overview
This PR only touches two Playwright e2e test files: managed-wallet-notification-channels.spec.ts and the AlertsPage page-object helper. It updates the spec to wait for the new /alerts tab-based navigation (introduced in #3583) instead of the old /alerts/notification-channels URL, and re-opens the Notification Channels tab after save since the app now lands back on the default Alerts tab. No application/production code is modified.
Security risks
None. This is test-only code with no auth, crypto, data handling, or user input changes.
Level of scrutiny
Low. The change is confined to e2e test helpers and a single spec file, is small (size: XS), and does exactly what the PR description claims — verified the diff matches the stated behavior change (URL wait removed for the tab click, URL regex updated to /\/alerts\/?$/ after save, and an extra openNotificationChannelsTab() call added before the list-verification step). This is squarely a mechanical fix following the existing page-object pattern.
Other factors
The one issue surfaced (duplicated locator regex in openNotificationChannelsTab) is a minor readability nit with no functional impact and is already captured as an inline comment. No outstanding unaddressed reviewer comments exist on this PR.
There was a problem hiding this comment.
LGTM — the prior nit (duplicate tab locator) has been fixed in a33b860, and no new issues were found.
What was reviewed: the updated wait-for-URL/tab-selected flow in the spec and AlertsPage.openNotificationChannelsTab(), confirming the assertions now match the post-#3583 Alerts-tab redirect behavior described in the PR description.
Extended reasoning...
Overview
This PR touches only two Playwright e2e test files: the notification-channels spec and its AlertsPage page object. It updates stale URL-based waits to match the new tab-based Alerts UI introduced in #3583 (notification channels are now a local tab rather than a separate route).
Security risks
None. This is test-only code with no changes to application logic, auth, or data handling.
Level of scrutiny
Low. Test-only, mechanical alignment fix for a known-stale e2e assertion, in a non-critical path (e2e tooling, not production code). The change is well-scoped and its rationale (URL/tab behavior changed in #3583) is clearly explained and verifiable against that PR.
Other factors
The bug-hunting system's earlier nit about duplicated locator construction in openNotificationChannelsTab() was addressed by the author in a33b860, which is already reflected in the current diff. No outstanding review comments remain, and no new issues were found on this pass.
1 participant
Why
The beta e2e gate for console-web v3.164.0 failed on
managed-wallet-notification-channels.spec.ts(run). #3583 folded Notification Channels into the Alerts page as local tabs: switching tabs no longer changes the URL,/alerts/notification-channelsnow redirects to/alerts, and saving a new channel navigates back to/alertswith the default Alerts tab selected. The spec still waited on the old URLs, so it timed out. Beta itself is healthy; only the spec was stale.Part of CON-733.
What
AlertsPage.openNotificationChannelsTab()waits for the tab to become selected instead of relying on a URL change/alerts, where the app actually landsBeta e2e checks out the release tag, so this takes effect starting with the next console-web release; the failed v3.164.0 run cannot be re-greened by this PR.
Summary by CodeRabbit