---
phase: 07-mobile-test-harness
reviewed: 2026-06-11T12:30:00Z
depth: deep
files_reviewed: 9
files_reviewed_list:
  - apps/pwa/playwright.config.ts
  - apps/pwa/e2e/global-setup.ts
  - apps/pwa/e2e/layout.spec.ts
  - apps/pwa/e2e/calendar.spec.ts
  - apps/pwa/e2e/lists.spec.ts
  - apps/pwa/e2e/README.md
  - apps/pwa/tsconfig.e2e.json
  - apps/pwa/vitest.config.ts
  - apps/pwa/package.json
findings:
  critical: 0
  critical_resolved: 1
  blocker: 0
  blocker_resolved: 2
  warning: 0
  warning_resolved: 7
  info: 0
  info_bydesign: 5
  total: 0
status: clean
---

# Phase 7: Code Review Report (DEEP) — Iteration 2 (--auto re-review)

**Reviewed:** 2026-06-11
**Depth:** deep (cross-file call-chain analysis + live-stack verification)
**Files Reviewed:** 9 (harness)
**Status:** clean — zero open actionable findings

## Summary

This is the iteration-2 re-review after the fixer applied 5 changes (commits `c564fc6` WR-07,
`9c38dd3` WR-01+WR-02, `5322cfc` WR-06, `2b745ad` WR-05). The prior pass had resolved CR-01,
BL-01, and BL-02; those resolution records are preserved below.

**Verification performed this pass:**
- Ran the full suite against the live dev stack (MariaDB :3306, API :3000 `DEV_AUTH_BYPASS=true`,
  Vite auto-started by `webServer`): **58 passed (55s)**.
- Typecheck (`tsc --noEmit` + `tsc --project tsconfig.e2e.json --noEmit`): **exit 0**.
- Probed `navigator.serviceWorker` availability on **both** engines to confirm the WR-07 fix is
  non-vacuous (see WR-07 below).
- Probed `redirect:'manual'` response semantics to confirm the WR-01 gate distinguishes a
  dev-bypass 200 from an Authelia redirect.

**Result:** all 7 prior warnings are resolved by the fixes (5 actionable + WR-03/WR-04 by-design),
no fix introduced a regression or new defect, and no new cross-file issue was exposed.
Setting `status: clean`. The 5 IN-* items remain advisory/by-design and are listed under
"Resolved / By-design"; none are actionable.

---

## Critical Issues (resolved — record preserved)

### CR-01 (RESOLVED in commit `fcc680e`): global-setup TRUNCATE had no fail-closed guard

**File:** `apps/pwa/e2e/global-setup.ts:26-44`
**Status:** RESOLVED — re-verified sound this pass.

`globalSetup` TRUNCATEs `list_items`, `list_shares`, `lists`, `calendar_events` against whatever
`DB_*` points at. The fix throws **before** opening any DB connection:
1. `NODE_ENV === 'production'` → throw (checked first).
2. `DEV_AUTH_BYPASS !== 'true'` → throw.

This mirrors the API guard (`apps/api/src/auth/devBypass.ts`) ordering exactly and is coupled to
the same switch that makes the API serve Dev User 1 without OIDC (`index.ts:24-25, 51-55`).
Residual scope note carried as IN-05 (guard protects production, not "the wrong dev DB" — by design).

---

## Blocker Findings (resolved — record preserved)

### BL-01 (RESOLVED in commit `53c3ca5`): calendar populated-state assertions were vacuous

**File:** `apps/pwa/e2e/calendar.spec.ts`
**Status:** RESOLVED — re-verified non-vacuous this pass.

The dead-`EmptyState` / always-rendered-wrapper assertions were replaced with a real DB→UI proof:
`getByText('Seeded Test Event').first()` must be visible in the grid (`calendar.spec.ts:90-97`).
Verified live: passes on both `iphone` (WebKit) and `pixel` (Chromium). With `/api/events`
mocked empty the title is absent, so the assertion genuinely tracks the seed flowing
DB → API → query → grid. The wrapper-visibility test (`:80-88`) was kept but its docstring now
correctly states it only proves the grid mounts, not that the seed reached the UI.

### BL-02 (RESOLVED in commit `53c3ca5`): seed↔view month-boundary fragility

**File:** `apps/pwa/e2e/global-setup.ts:119-154`
**Status:** RESOLVED — re-verified deterministic this pass.

The seed event is re-anchored to **noon-today (UTC)** (`global-setup.ts:127-129`) — always today's
local calendar date, always inside the current-month view both phone-width profiles render. The
prior `now+24h` could roll into the next month on a month's last day, making any "seeded event is
visible" assertion date-fragile. The seed shape (`all_day=false`, `dtstart_utc` set, recurring
flags false) matches the API's non-recurring-timed WHERE branch. Verified live on both engines.

---

## Resolved this iteration (fixer commits — verified, no regression)

### WR-01 (RESOLVED in `9c38dd3`): `/api/me` dev-bypass reachability gate

**File:** `apps/pwa/e2e/global-setup.ts:75-90`

The gate now probes `fetch(${baseURL}/api/me, { redirect: 'manual' })` after the `/health` poll
and throws unless `res.ok`. Verified correct end-to-end:
- **Dev-bypass-reachable API → 200.** `me.ts:30-42` short-circuits on `c.get('user')` (DEV_USER)
  with no DB round-trip, so the gate passes regardless of seed state and regardless of ordering
  (the probe runs before the seed — confirmed safe because `/api/me` has no DB dependency under
  bypass). The full suite passed with this gate live.
- **Authelia-redirecting API → fails loudly.** With `redirect:'manual'`, a cross-origin 302 to
  Authelia surfaces as `type:'opaqueredirect'`, `status:0`, `ok:false` → gate throws. A
  same-origin redirect (e.g. `c.redirect('/')`) surfaces as `type:'basic'`, `status:302`,
  `ok:false` → also throws. Confirmed empirically against `/api/login` (302, `ok=false`).
- **No false-fail in the supported setup:** in the dev-bypass stack the OIDC middleware is not
  mounted (`index.ts:51`), so `/api/me` always returns 200. No regression.

The error message string contains `opaqueredirect` with no space — cosmetic only (it is the exact
`Response.type` token undici emits); not actionable.

### WR-02 (RESOLVED in `9c38dd3`): explicit readiness flag

**File:** `apps/pwa/e2e/global-setup.ts:54-73`

The loop now uses an explicit `let ready = false` set inside the `res.ok` branch, and the
post-loop check is `if (!ready) throw` — success is no longer inferred from `Date.now() >= deadline`.
This removes both the false-positive-timeout (a success arriving in the final second can no longer
be misreported as a timeout) and any false-positive-ready (the flag is only set on an actual
`res.ok`). Timeout logic verified correct by reading; the gate ran green in the live suite.

### WR-05 (RESOLVED in `2b745ad`): dropped redundant `unroute` calls

**Files:** `apps/pwa/e2e/calendar.spec.ts:133-135, 154, 176`; `apps/pwa/e2e/lists.spec.ts:90-92, 118`

The trailing `page.unroute(...)` calls were removed and replaced with comments explaining that
per-test context isolation handles cleanup. Verified this is correct, not a leak risk:
- Every `page.route(...)` is registered **inside an individual test body**, never in a shared
  `beforeEach`/`beforeAll`. Playwright assigns each test a fresh `page`/`BrowserContext`, and route
  handlers are scoped to that page/context — they cannot leak into sibling tests.
- The suite runs under `fullyParallel: true` with no `describe.serial`, so there is no shared-page
  path that could carry a route forward.
- Cross-test isolation confirmed empirically: the populated-state calendar/lists tests (no mock)
  and the error/empty-state tests (with mock) all pass in the same run with no interference.

The removed `unroute` calls were genuinely dead — they never ran when an `expect` threw (the whole
point of those tests), so they had guaranteed nothing. Dropping them is strictly an improvement.

### WR-06 (RESOLVED in `5322cfc`): self-validation comment corrected

**File:** `apps/pwa/e2e/layout.spec.ts:209-211, 250-252`

The misleading "remove by reload" comments now read "REMOVE the injected style by deleting the
`<style>` element via evaluate (`styleHandle.evaluate(el => el.remove())` — no page reload)", which
matches the actual code (`styleHandle.evaluate((el) => (el as Element).remove())`). Comment matches
code. Trivial, confirmed.

### WR-07 (RESOLVED in `c564fc6`): SW-block test is now non-vacuous and honestly skips

**File:** `apps/pwa/e2e/calendar.spec.ts:41-68`

The test now (a) computes `swAvailable = 'serviceWorker' in navigator`, (b) `test.skip(!swAvailable, ...)`
when absent, and (c) otherwise asserts `getRegistration()` resolves to `undefined`. Verified all three
concerns live:

- **(a) Not vacuous on Chromium/pixel — AND not vacuous on WebKit/iphone either.** I probed both
  engines directly: `swAvailable=true` and `getRegistration()=undefined` on **both** `iphone`
  (WebKit) and `pixel` (Chromium) over `http://localhost`. So the genuine assertion runs on both
  profiles in this environment — `getRegistration()` is available and returns `undefined` under
  `serviceWorkers:'block'`. The SW test shows `✓ passed` (not `skipped`) on iphone, confirming the
  real assertion executed rather than being silently skipped.
- **(b) `test.skip` is honest.** It is a real `test.skip(condition, reason)` that, when
  `serviceWorker` is genuinely absent (e.g. a future WebKit/runner where http://localhost is not a
  secure context), marks the test **skipped/visible** in the reporter — it does not let an
  unavailable API masquerade as a pass. In the current stack the skip branch is never taken, so it
  is correct dead-fallback, not a silent pass.
- **(c) `getRegistration()` is the right probe under `serviceWorkers:'block'`.** With the block in
  effect no registration is ever created, so the promise resolves to `undefined`; if the block were
  lifted and the app registered `sw.js`, this would become a `ServiceWorkerRegistration` and the
  `toBeUndefined()` assertion would fail. This is a real, regression-sensitive signal (unlike the
  old `controller === null`, which was null on any first uncontrolled load regardless of the block).

No regression. The fix strictly strengthens the assertion.

---

## Resolved / By-design (advisory — NOT actionable)

These were never code defects; they are design notes carried for traceability. None block shipping.

- **WR-03 (by-design):** `webServer` manages Vite only; the API/DB/Redis are compose-managed per
  D-10. Playwright considers the server ready when Vite answers, before `globalSetup` polls
  `/health`; a missing API is deferred to the `/health` gate (now also the `/api/me` gate, WR-01).
  This is the intended D-09 contract. Documentation-coupling only.
- **WR-04 (by-design):** `page.route('/api/lists')` exact-match is correct — `fetchLists()` requests
  the bare path with no query string, and the narrow matcher intentionally avoids swallowing
  `/api/lists/:id/items`. A glob would be brittle. No change.
- **IN-01 (advisory):** `mysql2@3.22.4` is a PWA `devDependency` used only by the seed; correct
  placement (never bundled). Note: pinned independently from `apps/api`'s copy — keep in lockstep.
- **IN-02 (advisory):** `tsconfig.e2e.json` `types:["node"]` + `lib:["DOM",...]` correctly types the
  Node seed while still typing `page.evaluate` DOM callbacks. `@playwright/test` types come via
  direct import. Sound.
- **IN-03 (advisory):** vitest `exclude:['e2e/**']` and Playwright `testDir:'./e2e'` cleanly
  partition the two runners. Sound.
- **IN-04 (advisory):** `typecheck` covers both tsconfigs (re-verified exit 0 this pass). Good.
- **IN-05 (advisory):** the CR-01 guard protects *production*, not "the wrong dev DB" — pointing
  `DB_*` at a populated dev DB with `DEV_AUTH_BYPASS=true` will still TRUNCATE it. By design (D-06
  deterministic reseed) and documented. A defense-in-depth `E2E_ALLOW_TRUNCATE`/DB-name-pattern
  opt-in remains an optional hardening, not a defect.

---

## Live-run evidence (iteration 2)

| Check | Result |
|---|---|
| Full suite (both profiles) | 58 passed (55.0s) |
| `iphone` SW-block test | ✓ passed (real assertion ran; not skipped) |
| `pixel` SW-block test | ✓ passed |
| Seeded-event DB→UI proof (iphone + pixel) | ✓ passed both |
| `swAvailable` probe (both engines) | `true` / `getRegistration()=undefined` |
| `redirect:'manual'` on a 302 | `ok=false` (gate throws — correct) |
| `tsc --noEmit` + e2e tsconfig | exit 0 |

---

_Reviewed: 2026-06-11T12:30:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: deep (iteration 2 — --auto re-review)_
