test(styling): make e2e navigation flag-agnostic for top nav - #3453
Merged
Conversation
The flag-gated top nav (ui_top_nav) replaces the sidebar with a different shell: no Deploy link, Alerts and API Keys behind a Settings dropdown, and no wallet-balance element. The Playwright suite navigated via the sidebar, so it failed wherever the flag is on (login itself broke, since loginExistingUser waited on the nav's wallet-balance label). Rename the Sidebar page object to AppNav and have each method detect the rendered shell (only the top nav exposes nav[aria-label="Primary"]) and act accordingly, so specs pass whether ui_top_nav is on or off. Switch the authenticated-shell signal from the wallet-balance label to the account-menu button, present in both shells.
Contributor
|
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 deploy-web UI tests add ChangesAppNav navigation migration
Estimated code review effort: 2 (Simple) | ~15 minutes 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
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3453 +/- ##
==========================================
- Coverage 73.05% 71.98% -1.08%
==========================================
Files 1175 1085 -90
Lines 29793 27444 -2349
Branches 7402 6936 -466
==========================================
- Hits 21765 19755 -2010
+ Misses 7068 6759 -309
+ Partials 960 930 -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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/deploy-web/tests/ui/pages/AppNav.ts`:
- Around line 55-56: Update isTopNav() to wait for the shared account-menu
button or the primary navigation element to become available before checking
which navigation is present. Do not rely solely on the immediate count()
snapshot, so openDeploy() correctly detects the top-nav shell after goto(..., {
waitUntil: "commit" }).
In `@apps/deploy-web/tests/ui/pages/ConfigureDeploymentPage.ts`:
- Around line 20-26: Update ConfigureDeploymentPage.open so it waits for the
application shell to be ready after page.goto before calling AppNav.openDeploy.
Use accountMenuButton() or the established shell-ready signal, while preserving
the existing commit navigation and deploy-opening flow.
🪄 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: b510524c-bfde-4fe5-8292-79eddec49cf6
📒 Files selected for processing (12)
apps/deploy-web/tests/ui/actions/auth.tsapps/deploy-web/tests/ui/actions/deploy.tsapps/deploy-web/tests/ui/deploy-linux.spec.tsapps/deploy-web/tests/ui/managed-wallet-alerts.spec.tsapps/deploy-web/tests/ui/managed-wallet-api-keys.spec.tsapps/deploy-web/tests/ui/managed-wallet-deployment.spec.tsapps/deploy-web/tests/ui/managed-wallet-notification-channels.spec.tsapps/deploy-web/tests/ui/onboarding-gate.spec.tsapps/deploy-web/tests/ui/pages/AppNav.tsapps/deploy-web/tests/ui/pages/ConfigureDeploymentPage.tsapps/deploy-web/tests/ui/pages/HomePage.tsapps/deploy-web/tests/ui/pages/Sidebar.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/tests/ui/pages/Sidebar.ts
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.
Why
PR #3448 introduced a flag-gated top navigation (
ui_top_nav) that replaces the sidebar shell. The Playwright e2e suite (apps/deploy-web/tests/ui) was written against the sidebar and fails wherever the flag is on. Most notably,loginExistingUserwaited on the nav's wallet-balance label (dropped from the top nav), so everyuserType: "existing"spec failed at login. Deploy/Alerts/API-Keys navigation and the "logged-in" signal also moved.Ref CON e2e stability for the top-nav rollout.
What
Makes the e2e navigation shell-agnostic so specs pass whether
ui_top_navis on (top nav) or off (sidebar):pages/Sidebar.ts→pages/AppNav.ts. Each method detects the rendered shell (only the top nav exposesnav[aria-label="Primary"]) and routes accordingly:openDeploy()— sidebar: the Deploy link; top nav: Deployments → the page's Deploy CTA (works from any page).openAlerts()/openApiKeys()— sidebar: link / account-menu; top nav: the Settings dropdown menuitem.accountMenuButton()— the authenticated-shell signal, present in both shells.actions/auth.ts,pages/HomePage.ts,onboarding-gate.spec.ts). The wallet balance now lives in the home "Available Balance" card; its removal from the nav is intended.actions/deploy.ts,ConfigureDeploymentPage.ts, and the deployment/alerts/notification-channels/api-keys specs) toAppNav.Verification:
eslint --quietclean on all changed files;tsc --noEmitshows no new errors (179, unchanged baseline). Full Playwright run requires a live env and was not executed locally.Summary by CodeRabbit
Tests
Refactor
AppNavto handle navigation across both legacy sidebar and flag-gated top navigation shells.Sidebarpage-object and migrated callers toAppNav.