git:20260708.dbb71c9 to git:20260708.d397e29

+ description: Review of just-written or modified autotests against TypeScript + Playwright best practices (per official documentation) and your project's conventions. Use via /test-review or after writing/editing any test (UI E2E, API, UI+API, mocks, visual, mobile) or a Page Object/fixture/constants — before commit. Produces a prioritized list of findings with severity, line references, and ready-made fixes.

+ # Autotest review (TypeScript + Playwright)

+ Check just-written or modified test code against best practices and produce a prioritized list of findings with fixes. Rule source — the **official Playwright documentation** ([best-practices](https://playwright.dev/docs/best-practices), [locators](https://playwright.dev/docs/locators), [test-assertions](https://playwright.dev/docs/test-assertions)) and [TypeScript](https://playwright.dev/docs/test-typescript) + the specific project's conventions.

+ > **Source of truth for the project** — its root `CLAUDE.md` (if present) and the style of neighboring code. This skill is the **review phase**: it complements project rules, it does not replace them. Expanded `❌ before → ✅ after` examples and source links for each rule — in [`references/rules-catalog.md`](references/rules-catalog.md).

+ ## When to apply and operating mode

+ - **Default mode — diagnostics.** Read the code, run static analysis, produce a report. Do **not** edit files until the user explicitly asks to "apply / fix". Then — **iteratively**, one change at a time with a run in between (no big-bang rewrite of a working test).

+ - **Scope — only new/changed code, not the whole suite.** Default — uncommitted changes (`git status` + `git diff`). If the user specified a file/directory — review those.

+ - **Any test type:** UI E2E, API, UI+API, visual regression, mobile, mocks, plus Page Objects, fixtures, constants.

+ - Do not drift into autonomous actions beyond the review (browser reproduction, running the whole suite, edits) without confirmation — the default task is "read and assess".

+ ## Evidence discipline (no hallucinations)

+ - Every finding — from an **actually read line** (`file:line`) or from **observed output** of typecheck / lint / a run. Do not invent violations "by analogy" and do not reference lines you have not seen.

+ - A rule is checkable by a tool (tsc, ESLint, a run) → **run the tool first, then report its output**, not "probably present".

+ - Not sure it is a defect rather than a deliberate project decision → mark it **"questionable"**, do not assert. Cross-check against the project's `CLAUDE.md` and neighboring code: some "anti-patterns" may be intentional (legacy helpers, non-standard markup, deliberate rule exceptions). File names/endpoints/selectors from memory and past context are background — re-verify against live code.

+ - Found no violations in a category — say so: "clean", do not invent a finding for volume.

+ ## Process

+ 1. **Scope.** Determine the files under review: `git status --short` + `git diff --name-only` (include untracked), or the paths provided. For each spec, find the related Page Objects, constants, fixtures.

+ 2. **Context.** `Read` the changed files + related POM/constants/fixtures. `Read` a neighboring spec in the same directory as a style reference. Cross-check the directory/suite rules (smoke / regress / api etc.) against the project's `CLAUDE.md`, if present.

+ 3. **Static analysis (mandatory — cheap and evidence-based):**

+ - Typecheck: `tsc --noEmit` (or the project's typecheck script from `package.json`). Any type error in new code = 🔴 Blocker.

+ - ESLint: find the project config and **read which rules are actually enabled** (especially from `eslint-plugin-playwright`) — do not assume from memory. Lint output is the source of truth.

+ - **What lint usually does NOT catch:** `missing-playwright-await`, `@typescript-eslint/no-floating-promises`, `prefer-web-first-assertions`, `no-networkidle` are absent from many configs → check against the config and verify these categories manually (A/C): missing `await`, manual asserts instead of web-first, `networkidle`. Worth suggesting that the user add the missing rules.

+ - If needed — a formatting check (Prettier), if configured in the project.

+ 4. **Checklist.** Go through categories A–J below + K (your project's rules). For each violation — severity + `file:line` + fix. Deeper per rule — [`references/rules-catalog.md`](references/rules-catalog.md).

+ 5. **Stability verification** (only if the user asks to confirm the test works and a sandbox is available): run **only this test** in the project's native parallelism (NOT `--workers=1`):

+ Pass-on-retry or a floating result = flake = 🟠 Major; fix the cause (races/hydration/waits), do not hide it behind retries.

+ 6. **Report** — in the format from the "Report format" section.

+ | Label | Meaning | Typical examples |

+ | 🔴 **Blocker** | Test is broken, non-deterministic, or masks a bug. Do not merge. | Typecheck error; missing `await` (floating promise); `waitForTimeout`/in-page `setTimeout` pause; pass only on retry; `{ force: true }` / `dispatchEvent` / direct `setter` bypassing the real UI; test without asserts; `test.only`; conditional `expect` that may never execute. |

+ | 🟠 **Major** | Brittleness or flake on content/environment change; violation of a key project rule. | CSS/XPath chains instead of role/label; instant `count()`/`isVisible()`/`allTextContents()` as a gate; exact prices/texts/dates instead of regex; project convention violation (importing base `@playwright/test` where the project requires a custom fixture; a mandatory project check skipped — e.g. the network error monitor); `waitForLoadState('networkidle')`; a test depends on another test's state. |

+ | 🟡 **Minor** | Style/readability/maintainability; no impact on stability. | No `test.step` for business steps; inline comments instead of self-documentation; `.nth()` where `.filter()` fits; a working but non-priority locator; unused import/constant. |

+ | ⚪ **Nit** | Cosmetics. | Naming, import order, formatting (if not caught by Prettier). |

+ ## Review checklist

+ Each item — what to look for; the violation's severity in parentheses. Expanded examples and proofs — in the catalog.

+ ### A. Determinism, waits, asynchrony

+ - [ ] No `page.waitForTimeout(ms)` and no `setTimeout`/`sleep` inside `page.evaluate` (grep for both). Wait for state, not time. (🔴)

+ - [ ] No floating promises: every `expect`, `test.step`, action (`click/fill/goto`), `waitFor*` — under `await`/`return`/`void`. A missing `await` = silent flake. (🔴)

+ - [ ] Network — the **promise → action → await** pattern: `const p = page.waitForResponse(...); await click(); await p`. Declaring after the action = race. (🔴)

+ - [ ] No `waitForLoadState('networkidle')`. Navigation — `goto(url, { waitUntil: 'domcontentloaded' })`, without a duplicate `waitForLoadState` right after. (🟠)

+ - [ ] Derived/compound checks (several related conditions, measuring a collection right after it appears) — in `await expect(async () => {...}).toPass({ timeout })`, not a chain of `await`s. (🟠)

+ - [ ] SSR hydration accounted for: SSR frameworks (Nuxt/Next etc.) can remount content after hydration → an instant `count()`/`allTextContents()` right after appearance hits the emptiness window. Measure via `toPass`. (🟠)

+ ### B. Locators

+ - [ ] Priority: `getByRole({ name })` → `getByText` → `getByLabel` → `getByPlaceholder` → `getByAltText` → `getByTitle` → `getByTestId` → CSS (last resort) → XPath (almost never). (🟠 for CSS/XPath without a reason)

+ - [ ] No brittle CSS chains tied to DOM structure (`div > div > span`, `.episode-actions-later`). They break on rebranding. (🟠)

+ - [ ] Strict mode: the locator resolves to a single element; narrow via `{ name }` / `.filter({ hasText })` / `.filter({ has })`, not `.nth()`. `.nth()` — only with justification. (🟡)

+ - [ ] Floating elements (dropdowns, toasts, modals, portal content, iframes) are located **globally from `page`**, not from a section. (🟠)

+ - [ ] A long `getByText('whole sentence')` is not used as an anchor — brittle to copy edits; take a stable fragment/role. (🟡)

+ ### C. Assertions

+ - [ ] Only **web-first** (auto-retrying): `toBeVisible/toHaveText/toHaveCount/toHaveValue/toBeChecked/toHaveAttribute/toHaveURL`. No `expect(await loc.isVisible()).toBe(true)` and `expect(await loc.count()).toBe(n)` — they do not retry. (🔴/🟠)

+ - [ ] Every test verifies something (no test that only clicks without an `expect`). (🔴)

+ - [ ] A block of independent checks for one section — via `expect.soft`, to collect all failures at once. (🟡)

+ - [ ] No asserts on exact prices/numbers/dates/dynamic content — regex or a range. (🟠)

+ - [ ] A known open bug — `test.fail()` (with the single bug assert), not `test.fixme()`; the working behavior — as a separate regular test. (🟡)

+ ### D. Isolation and independence

+ - [ ] Tests are independent: state is NOT passed between tests. `let x` at `describe` level, reinitialized in `beforeEach`, is a common valid pattern; the violation is a test reading another test's result. Must pass in isolation and in any order. (🔴 if isolation is broken)

+ - [ ] `describe.configure({ mode: 'serial' })` — only for a real dependency, not "just in case". (🟠)

+ - [ ] Setup/teardown — in `beforeEach`/fixtures, no copy-paste; created entities (API) are deleted. (🟠)

+ - [ ] The test does not depend on external sites and third-party widgets — test only what you control; for the external part — mock/verify the request was made. (🟠)

+ ### E. TypeScript and lint

+ - [ ] Typecheck green for new code (`tsc --noEmit`, strict). (🔴)

+ - [ ] `any` in **POM / fixtures / utils** — undesirable, type it (`Locator`/`Page`/`Route`/`APIResponse`). Cross-check the project config: `any` may be deliberately allowed in specs (e.g. for mock data) — do not flag it there. `@ts-ignore` — only with a reason/ticket. (🟠 for POM/utils)

+ - [ ] POM fields — `readonly Locator`; fixtures typed (`base.extend<{...}>`); type the API response body explicitly if asserts rely on it. (🟡)

+ - [ ] **A missing `await` is often NOT caught by lint** (check the config: is `no-floating-promises` / `missing-playwright-await` there; `valid-expect` covers only part) → re-read by eye, see A2. (🔴)

+ - [ ] Match the module system (ESM vs CJS) and import style (relative vs aliases) against the project's actual code — follow the existing style, do not impose your own. Remove unused imports/variables/constants/POM methods. (🟡)

+ ### F. Network and mocks

+ - [ ] Mocks (`page.route`) — only for edge cases (5xx, empty response, timeout, offline). The positive happy path — against the real API. (🟠)

+ - [ ] Contract verification where it is the point of the test: `waitForResponse` (status + body) / `waitForRequest` + `postDataJSON()`. For forms — inspect the payload for `[object Object]`, empty/non-serialized fields, not just "button is enabled". (🟠)

+ - [ ] Routes are set up **before** the triggering action; scope — test/fixture, not globally on the suite. (🟠)

+ - [ ] An external host that may not respond (e.g. an external account portal, a payment gateway): do not verify the transition "in depth" — the oracle is the initiated navigation request + `route.abort()`, otherwise a pending navigation hangs teardown. (🟠)

+ ### G. Structure, readability, hygiene

+ - [ ] Logical steps are wrapped in `test.step('Imperative name', …)` (visible in Allure/HTML/trace). `return` — outside the callback. Do not split into a step per action. (🟡)

+ - [ ] **No inline comments** in tests — self-documentation (meaningful names, semantic locators, steps). Context — in the reporter description/annotation (e.g. `allure.description`), if the project uses them. (🟡)

+ - [ ] Parameterize same-shaped cases via `for...of` **outside** `test.describe`, not test copies. (🟡)

+ - [ ] No `test.only`, commented-out tests, temporary files/drafts, debug `console.log`/`page.pause()`. (🔴 for `test.only`/`page.pause`, otherwise 🟡)

+ - [ ] Test/step names are meaningful; ID/tag format (`@allure.id:N`, TC key etc.) — as in the neighboring tests in the file. (⚪)

+ ### H. Bug masking and flake

+ - [ ] No synthetic bypasses of the real UX: `{ force: true }`, `dispatchEvent`, direct React/Vue setter, manual scroll instead of auto-actionability — unless justified by a controlled input (e.g. custom `display:none` inputs — verify in the browser). The fix must catch a regression if the feature breaks, not hide it. (🔴)

+ - [ ] `retries`/`mode: serial`/an increased timeout are not used as a "cure" for flake. Quarantine is acceptable only temporarily, with a ticket link. (🟠)

+ - [ ] `try/catch` does not swallow action/assert failures (auto-waiting is built in; `.catch()` hides the bug). (🟠)

+ - [ ] A pre-release test (written before the feature ships) **fails honestly**, not hidden behind `skip`/a flag. (🟠)

+ ### I. Special cases by test type

+ - [ ] **API:** verify status AND body; request identifiers — a fresh `randomUUID()` from the built-in `crypto` per request (do not pull in the `uuid` package if the project does not have it); rate limit accounted for; cleanup of created entities. (🟠)

+ - [ ] **iframe:** `frameLocator`; content is located inside the frame. **New tab:** `context.waitForEvent('page')`. **Download:** `waitForEvent('download')` + filename check. **Upload:** `setInputFiles`. **Time:** `page.clock`. **Geolocation/permissions:** `grantPermissions`/`setGeolocation`. (🟠 for manual workarounds)

+ - [ ] **visual:** `toHaveScreenshot` with `animations:'disabled'` and `mask` on dynamic content; baselines — on the CI platform (a macOS baseline against Linux CI = guaranteed diff). Only if the test case requires a baseline. (🟠)

+ ### J. Intent conformance (the oracle actually verifies what is claimed)

+ - [ ] The test verifies what the name/description promises, not a surrogate. "Form validation" → inspect the real payload, not just "button is enabled". "Load more" → actual loading of more items and verification, not just the click. (🟠)

+ - [ ] Content binding is structural (presence, non-emptiness, `count > 0`, regex), so the test survives copy/price changes — especially for regression tests after a fix. (🟠)

+ - [ ] The oracle matches the environment's limitation: where the UI does not distinguish 404/5xx (one stub for both) — the check is network-level, not "saw the error text". (🟠)

+ ### K. Your project's rules (template — fill in for your repository)

+ > A mature test repository always has conventions no universal checklist will catch. Record them here or in the project's `CLAUDE.md` — then the review will catch their violations. Typical categories with examples:

+ - [ ] **Custom fixtures:** where to import `test` from the custom fixture (`./fixtures/custom-test`) instead of `@playwright/test`, and which mandatory checks it provides (e.g. a network error monitor invoked at the end of the test / in `afterEach`). (🟠)

+ - [ ] **Directory patterns:** how the rules of the smoke / regress / api suites differ — composition vs fixtures for POM, reporter annotations, testMatch/testIgnore, where to add new tests. (🟠)

+ - [ ] **Environments:** the test and POM are verified on all target environments, not just one (the DOM on the test environment may differ from prod); known environment quirks are recorded as a list. (🟠)

+ - [ ] **Skipped hygiene:** the test does not add permanent skips to standard runs; environment-specific exclusion goes through config (`testIgnore`/`testMatch`), runtime `test.skip` — only for dynamic conditions (feature flag, known bug with a ticket). Run outcome: passed = ok, failed = problem, skipped = requires an explanation. (🟡)

+ - [ ] **Dependencies/config:** do not bump the `@playwright/test` version and do not add dependencies without checking against CI (Docker image, lock file); do not touch `playwright.config.ts` without necessity. (🔴 if touched without being asked)

+ ## Report format

+ ## Review: <files / scope>

+ **Static analysis:** typecheck ✅/❌ · lint ✅/❌ · (run: N/N pass, --repeat-each=5)

+ 1. `path/to/spec.ts:42` — <what is wrong>.

+ Why: <link to rule/category>.

+ Fix:

+ // ❌ before / ✅ after

+ ### ✅ What is good

+ - <what matches best practice — brief>

+ ### Verdict

+ <Ready to commit / Needs rework: list of Blocker+Major> · <run command with the correct --project>

+ Rules:

+ - Sort strictly by severity (Blocker → Nit). Within a level — by file/line.

+ - Every Blocker/Major — with a concrete fix (a `❌ before → ✅ after` snippet).

+ - A category is clean — write "clean", do not invent.

+ - At the end — a one-line verdict and the run command with the correct `--project`.

+ - If asked to apply fixes — do it **iteratively** (one change → typecheck/run → the next), do not rewrite a working test wholesale.