Vertex: feature & RBAC e2e suites, RBAC gating fixes, e2e in CI#3803
Merged
Conversation
Covers the Import External Device wizard on /devices/my-devices in
personal context, using hybrid interception: real auth, navigation, and
data GETs against staging, with the two write endpoints (POST
/devices/soft and cohort assignment) intercepted so runs are
deterministic and create no backend records.
- happy path: fills Device Details, asserts cohort options are scoped to
the user's own cohort IDs (cross-org leakage contract), verifies the
Confirmation summary echoes wizard state, and asserts the captured
mutation payload, cohort assignment target, success banner, and
myDevices refetch after invalidation
- gate: completing without a cohort is blocked on non-admin pages and
the mutation is never called
Supporting changes:
- e2e/support/device-mocks.ts: reusable route interceptions for device
write endpoints
- e2e/support/env-guard.ts: refuses to run mutation specs unless
NEXT_PUBLIC_API_URL looks like staging/localhost
- playwright.config.ts: pin NEXTAUTH_URL/NEXTAUTH_COOKIE_DOMAIN to the
local server for the e2e webServer run — a .env.local pointing these
at the deployed staging host issues session cookies for .airqo.net,
which the browser rejects on localhost, breaking login
- auth.setup.ts: retry login when the app's short post-login session
poll times out on a cold dev server ("Could not confirm session")
The login page and OAuth token-handoff handler each polled getSession() for only 8 x 150ms = 1.2s after a successful signIn, then surfaced "Could not confirm session. Please try again." — on slow connections or devices the sign-in had actually succeeded but the session cookie had not propagated yet. Found via e2e cold-start runs, where the failure reproduced on every fresh dev server. Extracted the duplicated inline pollers into core/auth/waitForSession.ts: deadline-based (15s budget, 250ms interval), resolving on the first successful poll so the happy path is exactly as fast as before. Added co-located unit tests including a regression guard that a session appearing after the old 1.2s cutoff is still picked up.
… guard Bulk import UX: the wizard now offers a downloadable CSV template (header row of expected-field labels plus an example row), closing the gap where users had to guess the file format. Building it exposed that the auto-mapper had no alias for "Device Connection URL" (api_code), so even a correctly-labelled column needed manual mapping — alias added, and a template round-trip e2e test proves every column now auto-maps. Mapping selects also gained aria-labels (previously unlabelled). New e2e spec for the bulk/CSV flow (hybrid interception, same as the single-device spec): template round-trip, a 2-device import asserting the captured /devices/soft/bulk payload and cohort assignment, and a partial-failure path asserting the per-row results view. Login fix: TokenHandoffHandler redirected to /home even when no session was established after signIn (e.g. cookie issued for .airqo.net being rejected on localhost), causing an endless home -> signout -> login loop. It now routes to /auth-error instead. .env.example documents the NEXTAUTH_COOKIE_DOMAIN localhost trap. Harness robustness: auth.setup treats an auto-redirect into the app as success and budgets for cold compiles; specs clear the persisted React Query cache before page load so response-based assertions cannot be starved by cache hydration from storageState.
… platform Org-scope sidebar gains an Organization section linking to <analytics-url>/org/<slug>/members and /org/<slug>/roles in a new tab, slug derived from grp_title (lowercase, spaces/underscores to dashes). NavItem gains an external flag; ANALYTICS_BASE_URL exported from core/urls. Covered by co-located unit tests.
ReusableDialog default header gains a '?' button opening the feedback dialog, with the source dialog title carried through the open event and submitted as sourceDialog metadata. Feedback dialog opts out to avoid recursion. Fixes stacked-dialog Escape handling (topmost wins) and scroll-lock release. Covered by co-located unit tests.
Adds e2e coverage proving each device action entry point (claim, import, deploy, recall, maintain) is disabled for a user whose role lacks exactly that permission, with a sibling-action control per test. Infrastructure: rbac-mocks.ts intercepts the user-details GET and transforms the real response (strip/grant role permissions, always neutralize SUPER_ADMIN) so each test boots a real session with exactly the permissions the scenario needs — no second account or seeding. device-mocks.ts gains a device-details fixture so deployment state (Deploy vs Recall visibility) is deterministic. Also wires the missing UI gating the epic specifies: the My Devices claim button now disables without DEVICE_CLAIM, and the device-details Deploy/Recall/Maintenance buttons disable without their respective permissions (they previously only carried the permission tooltip prop, matching the pattern already used on /devices/overview).
Covers both denial modes of RouteGuard against a real session: in-place forbidden UI on /devices/overview (missing DEVICE_VIEW) and /admin/networks (missing all ADMIN_PANEL_PERMISSIONS, via AdminRouteGuard), and the showError=false redirect of /sites/my-sites to /home (missing SITE_VIEW). A positive control proves the guarded page renders normally when the permission is granted.
Proves personal-only pages deny an external-org session even when the org role grants every permission the guard asks for: /sites/my-sites redirects to /home and /admin/networks shows the forbidden UI. Personal- context controls with identical grants render both pages, isolating the context check as the only denial cause. Boots into external-org context by injecting a synthetic org into the intercepted user-details response and seeding the last-active-group preference with the real user id — no org membership seeding required on the backend.
Covers the subtlest RBAC check: a user who genuinely holds a permission — but on a different organization than the one the action targets. With DEVICE_UPDATE granted on the AirQo group only, device actions on /devices/overview are disabled while an external org without that permission is active, and enabled in the personal scope that holds it (control). A route-guard variant proves DEVICE_VIEW held on AirQo does not open /devices/overview from an org whose role lacks it.
Three flake sources surfaced by full-suite runs (7 workers sharing one dev server + staging backend): - On pages whose buttons render before permissions load, assert the enabled control action first — it proves RBAC state has settled, so the disabled assertion reflects the missing permission rather than a snapshot of the loading state. - Tolerate 'Test ended' in the user-details route handler: React Query background refetches can still be in flight when a test finishes. - Give the external-org scenarios (two full boots per test) budgets that survive worker contention.
Settles the device-action permission policy: claim gates on DEVICE_CLAIM and import on DEVICE_UPDATE, everywhere. - /devices/overview: claim button gated on DEVICE_UPDATE while its tooltip cited DEVICE_CLAIM — now checks DEVICE_CLAIM; import tooltip corrected to cite DEVICE_UPDATE (its actual check). - /devices/my-devices: import button was ungated — now disables without DEVICE_UPDATE, matching overview. - useHasAnyPermission/useHasAllPermissions now honor the NEXT_PUBLIC_MOCK_PERMISSIONS_ENABLED dev flag like usePermission does; previously mocking permissions changed action buttons but not route guards (RouteGuard resolves through useHasAnyPermission). Unit test proves the hooks agree. - RBAC e2e specs updated to pin the settled policy per action, with cross-permission controls (claim disabled while import enabled, and vice versa). - Bulk-import partial-failure spec: raise a default-5s assertion to the 30s budget its sibling assertions already use (flaked under 7-worker load, passes in isolation).
Production builds exposed a race the dev server had been hiding: two independent layers deny guarded routes — RouteGuard's in-place forbidden UI and useContextAwareRouting's replace to /home for routes whose sidebar entry is permission-hidden. The dev server is slow enough that the forbidden UI wins; a production build redirects first, so assertions pinned to the forbidden UI fail under next start. New expectRouteDenied helper polls for either outcome (both are denials); the four affected tests now pass under both build modes. Unifying the two layers into one canonical denial UX is flagged as app-level follow-up.
…lytics New parallel e2e-vertex job in vertex-ci.yml: continue-on-error for now (same philosophy as coverage — a visible signal, not a merge gate, since runs depend on the staging backend), promoted once flake rate is known. Skips itself with a notice when the VERTEX_E2E_* secrets are absent (fork PRs). CI serves a production build (next build && next start) instead of the dev server — faster, more stable page loads and closer to what ships; local runs keep using npm run dev. Playwright emits JUnit in CI and the job uploads it to Codecov Test Analytics (flag vertex-e2e), closing the flaky-test tracking gap TESTING.md listed as deferred; coverage flags are untouched. HTML report uploaded as artifact on failure. TESTING.md CI-status section and changelog updated.
…gation
Two independent layers denied guarded routes and raced each other:
RouteGuard's in-place forbidden UI and useContextAwareRouting's
replace('/home') for routes whose sidebar entry is permission-hidden.
The winner depended on build mode — the slow dev server usually
rendered the forbidden page, a production build silently redirected,
so users never saw the forbidden page for mapped routes. Surfaced by
running the e2e suite against a production build (the CI path).
Canonical behavior: RouteGuard owns direct-navigation denial (forbidden
UI, or its configured redirectTo); useContextAwareRouting now only
redirects when the active context switches underneath a route the new
context doesn't offer. Every route in its map is independently guarded
(page-level RouteGuard or AdminRouteGuard), so narrowing it opens no
gap.
Hook unit tests pin the switch-only contract, and the e2e denial
assertions (expectRouteDenied) are strict again — forbidden UI, URL
unchanged — passing identically under dev and production builds.
Contributor
📝 WalkthroughWalkthroughThe PR adds informational Playwright CI execution, deterministic device-import and RBAC E2E coverage, permission-based device action gating, shared session polling, context-aware routing fixes, CSV template downloads, feedback source tracking, stacked-dialog behavior, and external organization navigation. ChangesAuthentication and routing
Device import and E2E execution
Shared UI
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI as e2e-vertex CI job
participant Playwright
participant Vertex as Vertex production server
participant Codecov
CI->>Vertex: Build and start application
CI->>Playwright: Run Playwright tests with staging environment
Playwright->>Vertex: Authenticate and execute E2E flows
Playwright->>Codecov: Upload JUnit test results
CI->>CI: Upload HTML report when tests fail
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## staging #3803 +/- ##
===========================================
- Coverage 64.74% 56.59% -8.15%
===========================================
Files 158 118 -40
Lines 7930 4569 -3361
Branches 1923 1519 -404
===========================================
- Hits 5134 2586 -2548
+ Misses 2734 1983 -751
+ Partials 62 0 -62 🚀 New features to boost your workflow:
|
Contributor
|
New azure vertex changes available for preview here |
![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: 3
🧹 Nitpick comments (4)
src/vertex/e2e/support/app-state.ts (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! Clean implementation. The
addInitScriptapproach correctly ensures the cache is cleared before any page load, and the reverse iteration is the safe pattern for removal during iteration.The hardcoded
QUERY_CACHE_KEY_PREFIXon line 4 is a known coupling risk — if the prefix incore/providers/query-provider.tsxchanges, this will silently stop matching. The comment documents it well, but consider adding a compile-time assertion or sharing the constant if feasible.🤖 Prompt for 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. In `@src/vertex/e2e/support/app-state.ts` around lines 1 - 25, Reduce the coupling between QUERY_CACHE_KEY_PREFIX in clearPersistedQueryCache and the matching prefix in query-provider.tsx by reusing a shared exported constant where feasible. Update the query-provider reference and this helper’s import accordingly, while preserving the existing addInitScript clearing behavior.src/vertex/e2e/tests/devices/import-external-device-bulk.spec.ts (1)
35-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildCsvdoesn't escape commas or quotes in field values.The current test data (
twoDeviceCsv) doesn't contain commas, so this is safe today. If a future test row includes a comma in a field value, the CSV will silently break and the auto-mapper will see extra columns. Consider adding"quoting or a note that values must be comma-free.🤖 Prompt for 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. In `@src/vertex/e2e/tests/devices/import-external-device-bulk.spec.ts` around lines 35 - 40, Update buildCsv to escape CSV field values before joining them, wrapping values containing commas or quotes in double quotes and doubling embedded quotes. Apply this to every row field while preserving the existing header and newline output.src/vertex/e2e/tests/rbac/context-gating.spec.ts (1)
34-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
bootInExternalOrgtorbac-mocks.tsto avoid duplication.This helper is identical to the one in
resource-scoped.spec.ts(lines 30–40). All its dependencies (interceptUserDetails,resetAppBootState,seedActiveGroup,E2E_EXTERNAL_ORG_ID) are already exported fromrbac-mocks.ts, so the extraction is clean.♻️ Proposed extraction
Add to
rbac-mocks.ts:+export async function bootInExternalOrg( + page: Page, + options: Parameters<typeof rbacUser>[0] +): Promise<void> { + const intercept = await interceptUserDetails(page, rbacUser(options)); + await resetAppBootState(page); + + await page.goto("/home"); + const userId = await intercept.userId(); + await seedActiveGroup(page, userId, E2E_EXTERNAL_ORG_ID); +}Then in both
context-gating.spec.tsandresource-scoped.spec.ts, importbootInExternalOrgand remove the local definition:-async function bootInExternalOrg( - page: Page, - options: Parameters<typeof rbacUser>[0] -): Promise<void> { - const intercept = await interceptUserDetails(page, rbacUser(options)); - await resetAppBootState(page); - - await page.goto("/home"); - const userId = await intercept.userId(); - await seedActiveGroup(page, userId, E2E_EXTERNAL_ORG_ID); -}import { interceptUserDetails, resetAppBootState, seedActiveGroup, rbacUser, E2E_EXTERNAL_ORG_ID, expectRouteDenied, FORBIDDEN_TEXT, + bootInExternalOrg, } from "../../support/rbac-mocks";🤖 Prompt for 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. In `@src/vertex/e2e/tests/rbac/context-gating.spec.ts` around lines 34 - 44, Extract the duplicated bootInExternalOrg helper into rbac-mocks.ts, preserving its existing behavior and dependencies. Export it there, then import and use that shared helper in both context-gating.spec.ts and resource-scoped.spec.ts, removing each local definition.src/vertex/e2e/tests/rbac/action-visibility.spec.ts (1)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
bootWithtorbac-mocks.tsto avoid duplication.This helper is identical to the one in
route-guard.spec.ts(lines 29–35). Exporting it fromrbac-mocks.tskeeps the boot sequence in one place and prevents the two copies from drifting.♻️ Proposed extraction
Add to
rbac-mocks.ts:+export async function bootWith( + page: Page, + options: Parameters<typeof rbacUser>[0] +): Promise<void> { + await interceptUserDetails(page, rbacUser(options)); + await resetAppBootState(page); +}Then in both
action-visibility.spec.tsandroute-guard.spec.ts, importbootWithand remove the local definition:-async function bootWith( - page: Page, - options: Parameters<typeof rbacUser>[0] -): Promise<void> { - await interceptUserDetails(page, rbacUser(options)); - await resetAppBootState(page); -}import { interceptUserDetails, resetAppBootState, rbacUser, + bootWith, } from "../../support/rbac-mocks";🤖 Prompt for 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. In `@src/vertex/e2e/tests/rbac/action-visibility.spec.ts` around lines 30 - 36, Move the shared bootWith helper into rbac-mocks.ts and export it, preserving its existing interceptUserDetails and resetAppBootState sequence. Update action-visibility.spec.ts and route-guard.spec.ts to import the shared bootWith and remove their local definitions.
🤖 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 `@src/vertex/components/layout/secondary-sidebar.tsx`:
- Around line 209-227: Update the Members NavItem to render or remain disabled
according to contextPermissions.canViewUserManagement, and the Roles &
Permissions NavItem according to contextPermissions.canViewAccessControl. Follow
the existing permission-handling policy used by nearby navigation items and add
coverage for denied permissions.
In `@src/vertex/components/shared/dialog/ReusableDialog.tsx`:
- Around line 151-176: Update the Escape-key effect in ReusableDialog so
openDialogStack registration and cleanup depend only on isOpen. Store onClose
and preventBackdropClose in refs, keep those refs current across renders, and
have the stable handler read from them so callback changes cannot reorder lower
dialogs in the stack.
In `@src/vertex/core/auth/waitForSession.ts`:
- Around line 11-20: Update the polling function in waitForSession to catch
rejections from getSession() and return null, preserving the documented “never
throws” contract while retaining the existing successful-session and timeout
behavior.
---
Nitpick comments:
In `@src/vertex/e2e/support/app-state.ts`:
- Around line 1-25: Reduce the coupling between QUERY_CACHE_KEY_PREFIX in
clearPersistedQueryCache and the matching prefix in query-provider.tsx by
reusing a shared exported constant where feasible. Update the query-provider
reference and this helper’s import accordingly, while preserving the existing
addInitScript clearing behavior.
In `@src/vertex/e2e/tests/devices/import-external-device-bulk.spec.ts`:
- Around line 35-40: Update buildCsv to escape CSV field values before joining
them, wrapping values containing commas or quotes in double quotes and doubling
embedded quotes. Apply this to every row field while preserving the existing
header and newline output.
In `@src/vertex/e2e/tests/rbac/action-visibility.spec.ts`:
- Around line 30-36: Move the shared bootWith helper into rbac-mocks.ts and
export it, preserving its existing interceptUserDetails and resetAppBootState
sequence. Update action-visibility.spec.ts and route-guard.spec.ts to import the
shared bootWith and remove their local definitions.
In `@src/vertex/e2e/tests/rbac/context-gating.spec.ts`:
- Around line 34-44: Extract the duplicated bootInExternalOrg helper into
rbac-mocks.ts, preserving its existing behavior and dependencies. Export it
there, then import and use that shared helper in both context-gating.spec.ts and
resource-scoped.spec.ts, removing each local definition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 325b05c0-27c3-420a-9b89-b17312e66b50
📒 Files selected for processing (42)
.github/workflows/vertex-ci.ymlsrc/vertex/.env.e2e.examplesrc/vertex/.env.examplesrc/vertex/app/(authenticated)/devices/my-devices/page.tsxsrc/vertex/app/(authenticated)/devices/overview/page.tsxsrc/vertex/app/_docs/internal/TESTING.mdsrc/vertex/app/changelog.mdsrc/vertex/app/login/page.tsxsrc/vertex/components/features/devices/device-details-layout.tsxsrc/vertex/components/features/devices/import-device-modal.tsxsrc/vertex/components/features/devices/import-steps/BulkImportForm.tsxsrc/vertex/components/features/devices/import-steps/FieldMappingStep.tsxsrc/vertex/components/features/devices/import-steps/csvTemplate.test.tssrc/vertex/components/features/devices/import-steps/csvTemplate.tssrc/vertex/components/features/feedback/feedback-dialog.tssrc/vertex/components/features/feedback/feedback-launcher.tsxsrc/vertex/components/layout/NavItem.tsxsrc/vertex/components/layout/secondary-sidebar.test.tsxsrc/vertex/components/layout/secondary-sidebar.tsxsrc/vertex/components/shared/dialog/ReusableDialog.test.tsxsrc/vertex/components/shared/dialog/ReusableDialog.tsxsrc/vertex/core/auth/authProvider.tsxsrc/vertex/core/auth/waitForSession.test.tssrc/vertex/core/auth/waitForSession.tssrc/vertex/core/hooks/useContextAwareRouting.test.tssrc/vertex/core/hooks/useContextAwareRouting.tssrc/vertex/core/hooks/usePermissions.test.tsxsrc/vertex/core/hooks/usePermissions.tssrc/vertex/core/urls.tsxsrc/vertex/e2e/README.mdsrc/vertex/e2e/setup/auth.setup.tssrc/vertex/e2e/support/app-state.tssrc/vertex/e2e/support/device-mocks.tssrc/vertex/e2e/support/env-guard.tssrc/vertex/e2e/support/rbac-mocks.tssrc/vertex/e2e/tests/devices/import-external-device-bulk.spec.tssrc/vertex/e2e/tests/devices/import-external-device.spec.tssrc/vertex/e2e/tests/rbac/action-visibility.spec.tssrc/vertex/e2e/tests/rbac/context-gating.spec.tssrc/vertex/e2e/tests/rbac/resource-scoped.spec.tssrc/vertex/e2e/tests/rbac/route-guard.spec.tssrc/vertex/playwright.config.ts
Contributor
|
New azure vertex changes available for preview here |
- Gate the org sidebar links on their permissions: Members renders only with MEMBER_VIEW, Roles & Permissions only with ROLE_VIEW, and the Organization section hides when neither is held. Component tests cover each denied state. - ReusableDialog: keep callback identity changes out of the dialog-stack registration effect (onClose/preventBackdropClose now read via refs, effect keyed on isOpen only) — previously a rerender of a lower dialog re-pushed its id to the top of the stack, so Escape could close the wrong dialog. - waitForSession: honor the documented never-throws contract — a getSession() rejection (network error) is treated as "no session yet" and polling continues; two new unit cases pin it.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vertex/components/layout/secondary-sidebar.test.tsx (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider importing
PERMISSIONSconstants instead of hardcoding permission strings.The mock compares the permission string passed by the source code (
PERMISSIONS.MEMBER.VIEW,PERMISSIONS.ROLE.VIEW) against hardcoded'MEMBER_VIEW'/'ROLE_VIEW'values in the Set. If those constant values ever change, the mock silently returnsfalsefor all checks, causing confusing test failures rather than a clear import error. Using the actual constants keeps the test contract explicit.♻️ Suggested refactor
+import { PERMISSIONS } from '`@/core/permissions`'; // adjust path as needed + -const grantedPermissions = new Set<string>(['MEMBER_VIEW', 'ROLE_VIEW']); +const grantedPermissions = new Set<string>([ + PERMISSIONS.MEMBER.VIEW, + PERMISSIONS.ROLE.VIEW, +]); vi.mock('`@/core/hooks/usePermissions`', () => ({ usePermission: (permission: string) => grantedPermissions.has(permission), }));Then update the
delete/addcalls in the test bodies to use the same constants.🤖 Prompt for 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. In `@src/vertex/components/layout/secondary-sidebar.test.tsx` around lines 18 - 24, Update secondary-sidebar.test.tsx to import and use the PERMISSIONS.MEMBER.VIEW and PERMISSIONS.ROLE.VIEW constants in grantedPermissions, replacing the hardcoded permission strings. Also update all corresponding grantedPermissions delete/add calls in the test bodies to use these constants.
🤖 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.
Nitpick comments:
In `@src/vertex/components/layout/secondary-sidebar.test.tsx`:
- Around line 18-24: Update secondary-sidebar.test.tsx to import and use the
PERMISSIONS.MEMBER.VIEW and PERMISSIONS.ROLE.VIEW constants in
grantedPermissions, replacing the hardcoded permission strings. Also update all
corresponding grantedPermissions delete/add calls in the test bodies to use
these constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57afedc6-a3ab-4a17-9aa3-f052468261d6
📒 Files selected for processing (5)
src/vertex/components/layout/secondary-sidebar.test.tsxsrc/vertex/components/layout/secondary-sidebar.tsxsrc/vertex/components/shared/dialog/ReusableDialog.tsxsrc/vertex/core/auth/waitForSession.test.tssrc/vertex/core/auth/waitForSession.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/vertex/core/auth/waitForSession.ts
- src/vertex/components/layout/secondary-sidebar.tsx
- src/vertex/core/auth/waitForSession.test.ts
- src/vertex/components/shared/dialog/ReusableDialog.tsx
Contributor
|
New azure vertex changes available for preview here |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the Vertex Playwright e2e test suite to cover key feature flows and RBAC enforcement, and then hardens/fixes the underlying RBAC gating, routing-denial behavior, and login/session confirmation logic that the new coverage surfaced. It also wires Vertex e2e into GitHub Actions as an informational CI job and adds supporting documentation and environment examples.
Changes:
- Add/expand Playwright e2e coverage for device import (single + bulk) and RBAC (action visibility, route guards, context gating, resource scoping), including shared mocking/interception helpers.
- Fix RBAC permission gating gaps and denial-surface consistency (RouteGuard vs context-aware redirect), plus align mock-permissions behavior across permission hooks.
- Improve login/session reliability (shared
waitForSessionutil, OAuth handoff guard) and integrate e2e into CI with JUnit output for Codecov Test Analytics.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/vertex/playwright.config.ts | Adds CI JUnit reporting and runs e2e against a production Next build; pins auth env for localhost runs. |
| src/vertex/e2e/tests/rbac/route-guard.spec.ts | New e2e coverage for page-level route-guard denial modes. |
| src/vertex/e2e/tests/rbac/resource-scoped.spec.ts | New e2e coverage validating org/resource-scoped permission enforcement. |
| src/vertex/e2e/tests/rbac/context-gating.spec.ts | New e2e coverage for allowedContexts enforcement independent of permissions. |
| src/vertex/e2e/tests/rbac/action-visibility.spec.ts | New e2e coverage for per-permission action button disabling across key device actions. |
| src/vertex/e2e/tests/devices/import-external-device.spec.ts | New e2e coverage for single-device import wizard with hybrid interception and payload assertions. |
| src/vertex/e2e/tests/devices/import-external-device-bulk.spec.ts | New e2e coverage for bulk CSV import, template round-trip, and partial failure results. |
| src/vertex/e2e/support/rbac-mocks.ts | Adds RBAC test infrastructure by transforming user-details in-flight and seeding active group. |
| src/vertex/e2e/support/env-guard.ts | Prevents mutation e2e specs from running against non-staging/non-local backends. |
| src/vertex/e2e/support/device-mocks.ts | Adds deterministic interceptors for device import/bulk import/cohort assignment and device-details fixtures. |
| src/vertex/e2e/support/app-state.ts | Clears persisted React Query cache to make response-based assertions deterministic. |
| src/vertex/e2e/setup/auth.setup.ts | Hardens auth setup (timeout, retries) to reduce cold-start/session-confirmation flakiness. |
| src/vertex/e2e/README.md | Documents e2e structure and hybrid interception approach. |
| src/vertex/core/urls.tsx | Exports ANALYTICS_BASE_URL for reuse in sidebar external links. |
| src/vertex/core/hooks/usePermissions.ts | Extends mock-permissions flag behavior to useHasAnyPermission/useHasAllPermissions. |
| src/vertex/core/hooks/usePermissions.test.tsx | Adds unit coverage ensuring permission hooks behave consistently under mock-permissions flag. |
| src/vertex/core/hooks/useContextAwareRouting.ts | Restricts redirects to context switches to avoid denial-surface races with RouteGuard. |
| src/vertex/core/hooks/useContextAwareRouting.test.ts | Adds unit tests pinning the new “switch-only” redirect contract. |
| src/vertex/core/auth/waitForSession.ts | Introduces shared session-polling helper with a real time budget. |
| src/vertex/core/auth/waitForSession.test.ts | Adds unit tests for waitForSession behavior and regressions. |
| src/vertex/core/auth/authProvider.tsx | Uses shared waitForSession and guards against OAuth handoff redirect loops when no session materializes. |
| src/vertex/app/login/page.tsx | Replaces inline session polling with shared waitForSession. |
| src/vertex/components/shared/dialog/ReusableDialog.tsx | Adds feedback shortcut button and fixes stacked-dialog Escape/scroll-lock behavior. |
| src/vertex/components/shared/dialog/ReusableDialog.test.tsx | Adds unit tests for feedback button behavior and stacked Escape handling. |
| src/vertex/components/layout/secondary-sidebar.tsx | Adds analytics “Organization” section with Members / Roles links gated by permissions. |
| src/vertex/components/layout/secondary-sidebar.test.tsx | Adds unit tests for slugging, permission-gated visibility, and new-tab behavior. |
| src/vertex/components/layout/NavItem.tsx | Adds external link support (new-tab attrs, no active-state highlighting). |
| src/vertex/components/features/feedback/feedback-launcher.tsx | Captures dialog source metadata and opts out of feedback button recursion inside the feedback dialog. |
| src/vertex/components/features/feedback/feedback-dialog.ts | Changes feedback open event to CustomEvent with typed detail.source. |
| src/vertex/components/features/devices/import-steps/FieldMappingStep.tsx | Improves accessibility by labeling mapping selects for screen readers/tests. |
| src/vertex/components/features/devices/import-steps/csvTemplate.ts | Adds CSV template builder + download helper for bulk import. |
| src/vertex/components/features/devices/import-steps/csvTemplate.test.ts | Adds unit tests for CSV template contract and download behavior. |
| src/vertex/components/features/devices/import-steps/BulkImportForm.tsx | Adds “Download CSV template” UI affordance to bulk import step. |
| src/vertex/components/features/devices/import-device-modal.tsx | Fixes CSV auto-mapping aliases for api_code / “Device Connection URL”. |
| src/vertex/components/features/devices/device-details-layout.tsx | Enforces permission-based disabling of deploy/recall/maintenance actions. |
| src/vertex/app/(authenticated)/devices/overview/page.tsx | Aligns claim/import permission gating and tooltips on overview page. |
| src/vertex/app/(authenticated)/devices/my-devices/page.tsx | Adds missing permission-based disabling for claim/import actions on My Devices. |
| src/vertex/app/_docs/internal/TESTING.md | Updates internal docs to reflect new informational CI e2e job behavior. |
| src/vertex/.env.example | Documents localhost cookie-domain pitfall for NextAuth. |
| src/vertex/.env.e2e.example | Documents additional staging seeding needs for the new e2e specs. |
| src/vertex/app/changelog.md | Captures release notes for the new e2e coverage, fixes, and CI wiring. |
| .github/workflows/vertex-ci.yml | Adds informational e2e-vertex job and uploads JUnit results to Codecov Test Analytics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Summary
Builds out the Vertex Playwright e2e suite from its foundation into full feature + RBAC coverage (27 tests), fixes the real RBAC bugs the tests surfaced, and wires the suite into CI as an informational check with Codecov Test Analytics.
Feature e2e (earlier commits on this branch)
waitForSession), and an OAuth token-handoff redirect-loop guard.RBAC e2e (four epics, one spec each under
e2e/tests/rbac/)Infrastructure (
e2e/support/rbac-mocks.ts): real session, but the user-details GET is intercepted and its real response transformed in flight — strip/grant role permissions (legacy names resolved via the app's own mapping),SUPER_ADMINalways neutralized, optional synthetic external org. No second test account, no backend seeding.allowedContextsdenies an external-org session even when the org role holds every required permission.RBAC bugs found and fixed by this coverage
DEVICE_CLAIM/DEVICE_DEPLOY/DEVICE_RECALL/DEVICE_MAINTAIN.DEVICE_CLAIM, import →DEVICE_UPDATEeverywhere (the overview claim button previously checkedDEVICE_UPDATEwhile its tooltip citedDEVICE_CLAIM; the My Devices import button was ungated).useHasAnyPermission/useHasAllPermissions(RouteGuard's resolution path), not justusePermission.RouteGuard's forbidden UI anduseContextAwareRouting's/homeredirect raced on every guarded route — dev showed the forbidden page, production silently redirected. NowRouteGuardowns direct-navigation denial anduseContextAwareRoutingonly redirects on an actual context switch (all mapped routes are independently guarded, so no gap opens). Surfaced by running the suite against a production build; hook unit tests pin the new contract.CI & Codecov
e2e-vertexjob (parallel withcheck-vertex,continue-on-error— informational like coverage, since runs depend on the staging backend; promote once the flake rate is known). Skips gracefully on fork PRs without secrets.next build && next start) — full suite in ~46s vs ~3-4 min on the dev server, and it's what exposed the denial-race bug above.vertex-e2e) for flaky-test tracking; coverage flags untouched.Add repository secrets
VERTEX_E2E_USER_EMAIL/VERTEX_E2E_USER_PASSWORD(the staging e2e account; seeding documented in.env.e2e.example). Optional overrides:VERTEX_E2E_NEXTAUTH_SECRETsecret andVERTEX_E2E_API_URL/VERTEX_E2E_API_BASE_URL/VERTEX_E2E_ANALYTICS_URLrepo variables (staging defaults are baked in).Test plan
npm run test— 385 passednpx tsc --noEmitnpm run test:e2e(dev server) — 27 passedCI=1 npx playwright test(production build, the CI path) — 27 passed, JUnit emitted🤖 Generated with Claude Code
Summary by CodeRabbit