/e2e-test-review

name: e2e-test-review
description: Review Cypress E2E spec files for Metabase conventions, common gotchas, and flakiness/performance issues. Use when reviewing pull requests or diffs containing Cypress spec files in e2e/test/scenarios/.
allowed-tools: Read, Grep, Bash, Glob
metadata:
  metareview:
    title: E2E review
    kind: agent
    rule: Cypress specs changed
    watches: ["e2e/**/*.cy.spec.{js,ts}"]
    icon: eye
    color: orange

E2E Test Review Skill

@./../_shared/cypress-conventions.md

Review mode detection

Before starting, determine which mode to use:

1. **PR review mode** — if `mcp__github__create_pending_pull_request_review` is available, post issues as one cohesive pending review. 2. **Local review mode** — if not, output a numbered list in the conversation.

Review process

1. Detect mode. 2. Read the changed spec end-to-end first to understand intent. Don't review line-by-line cold. 3. **(Conditional)** If after reading the spec a specific assertion or selector is **ambiguous** — you can't tell if it's the right anchor, or whether the test actually verifies what its title claims — briefly grep the related component for the test ID / role / text. **Don't read components by default.** Only do this when there's a real signal of confusion in the spec. 4. Scan against the checklist + pattern table below. 5. Number all issues sequentially. Skip nits — only flag what's worth fixing. 6. **Always** finish with the honest e2e-vs-unit breakdown (see the section of that name below). It's part of every report, not an optional add-on.

When to hand off instead of review

If the spec references an issue (`metabase#NNNNN`) and the user wants to **fix** flakiness or assess whether the test still reproduces the original bug, that's outside the review skill's scope. Point them at the dedicated flake-fixing workflow, which knows to fetch the issue and the resolving PR's diff. The review skill stays focused on "is this test well-written and conformant" — it doesn't fetch external issue context by default.

Review checklist

The checklist mirrors the order of the conventions file. Items marked **(lint)** are also caught by ESLint — flag only when you see them slip through (helper-wrapped, lint-disabled, etc.).

File and naming

• [ ] Spec lives in `e2e/test/scenarios/<area>/`
• [ ] Extension is `.cy.spec.ts` (preferred) or `.cy.spec.js` — don't flag `.js` on existing files
• [ ] `describe` block names the area when relevant: `"area > sub-area > feature (#issue-number)"`
• [ ] No leftover `.only` or `.skip` (pre-commit hook should catch, but flag it if it slips through)

Helpers and constants

• [ ] Helpers accessed via `const { H } = cy;` — no direct imports from `e2e/support/helpers` **(lint)**
• [ ] Sample DB schema imported from `cypress_sample_database`
• [ ] Instance data IDs imported from `cypress_sample_instance_data`
• [ ] **No hardcoded numeric IDs anywhere** — including for entities the test creates itself. Capture from the create response or alias the intercept.
• [ ] Existing navigation helpers used (`H.openOrdersTable`, `H.visitDashboard(id)`, etc.) instead of raw `cy.visit()` chains

Shared helper changes

When a diff changes a shared helper, inspect its callers rather than reviewing the helper in isolation:

• [ ] Default behavior changes are justified for every caller; otherwise the new behavior is explicit and opt-in.
• [ ] Repeated calls with identical and different arguments are safe.
• [ ] Mandatory network waits account for cache hits, request deduplication, unchanged query arguments, and conditional fetches where no request occurs.
• [ ] Intercepts and aliases cannot accumulate, collide, or resolve against the wrong request.
• [ ] Verification covers both an affected caller and a representative caller using unchanged defaults.

Selectors

• [ ] Prefers a11y queries (`findByRole`, `findByLabelText`) over `findByText`
• [ ] `findByText` only when an a11y query doesn't fit
• [ ] `findByTestId` for `data-testid` attributes (never raw `cy.get("[data-testid='...']")`)
• [ ] No CSS class names — especially generated ones from styled-components/Mantine (`.css-1abc2d`)
• [ ] No ad-hoc CSS attribute selectors in specs or new helpers (the visualization helpers in `e2e/support/helpers/e2e-visual-tests-helpers.js` are the **only** intentional exception — see "What NOT to flag")
• [ ] No XPath
• [ ] **Positional selectors** (`.eq()`, `.first()`, `.last()`, `:nth-child`) only when the order is the assertion itself, or guarded by a length assertion immediately before. **(lint catches `.last()` and `.eq(<negative)`; the rest is on the reviewer)**
• [ ] **Text selectors are scoped** — top-level `cy.findByText(...)` / `cy.contains(...)` in `it`/`before`/`beforeEach` is forbidden. Must be scoped via `cy.contains(selector, text)`, `cy.someQuery().findByText(...)`, or `someQuery().within(...)`. **(lint catches the top-level case ONLY — it does NOT catch helper-wrapped queries; manually scan helper bodies)**

Setup and isolation

• [ ] State setup uses `cy.request` / API helpers, not the UI
• [ ] `H.restore()` and sign-in are in `beforeEach`, not `before`
• [ ] Each `it()` is independently runnable — no `it()` depends on prior `it()` state
• [ ] When both appear, `H.restore()` precedes `H.resetTestTable()` **(lint)**

Waits and timing

• [ ] No numeric `cy.wait(ms)` — even small ones
• [ ] `cy.intercept()` defined BEFORE the action that triggers the request
• [ ] No `setTimeout`, `Cypress.Promise.delay`, or other manual sleeps
• [ ] No long custom timeouts (e.g. `{ timeout: 30000 }`) papering over a race
• [ ] DOM readiness uses `.should("be.visible")`, **not** `.should("exist")`. Reserve `exist` for hidden inputs / off-screen / portal-detached cases.

Never assign return values from `cy.*` commands

• [ ] No `const x = cy.someCommand(...)` — `x` is a one-shot chainer, not the resolved value **(lint catches simple cases)**
• [ ] If a quer