familysync/.planning/phases/07-mobile-test-harness/07-REVIEW.md

---
phase: 07-mobile-test-harness
reviewed: 2026-06-11T03:30:00Z
depth: deep
files_reviewed: 11
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
  - package.json
  - .gitignore
findings:
  critical: 0
  critical_resolved: 1
  blocker: 0
  blocker_resolved: 2
  warning: 7
  info: 5
  total: 14
status: blockers_resolved
---

# Phase 7: Code Review Report (DEEP)

**Reviewed:** 2026-06-11
**Depth:** deep (cross-file call-chain analysis)
**Files Reviewed:** 11 (harness) + 12 cross-referenced app/API source files
**Status:** issues_found

## Summary

This is a deep re-review of the Phase 7 Playwright mobile E2E harness, tracing every spec
assertion through to the PWA component / API route it claims to exercise. The standard-depth
pass found CR-01 (data-loss guard — resolved in `fcc680e`) plus 7 warnings and 4 info items.

The deep pass **confirms CR-01 is soundly fixed end-to-end** but escalates two findings to
**BLOCKER** that only cross-file analysis surfaces:

1. **BL-01** — Two of the three `calendar.spec.ts` "populated state" assertions are **vacuous**:
   they assert against a code path (`EmptyState` / "Nothing here") that `CalendarShell`
   **never renders**, and against a wrapper element (`.sx-react-calendar-wrapper`) that is
   **always rendered on success regardless of whether the seed produced any events**. Neither
   assertion would fail if the seed broke or the events query returned `[]`. The harness's
   central claim — "validates the populated calendar state" — is not met.

2. **BL-02** — The seeded calendar event is placed at **`Date.now() + 24h`** while the PWA's
   initial fetch window is **current-month −7d … +7d** (`calendarStore.initialCalendarRange`).
   When the suite runs in the last 7 days of a month, "tomorrow" falls in the *next* month,
   outside the initial window, so the seeded event is never fetched. This is latent today only
   because BL-01's assertions don't actually check for the event — but it means the seed↔window
   contract is broken and any future "seeded event is visible" assertion will be date-dependent
   and flaky.

The auth/security call-chain (DEV_AUTH_BYPASS) is sound. The route-mock URL patterns match the
real request URLs. The vitest/e2e glob isolation is correct. The remaining issues are
determinism and config robustness (warnings) carried forward with deeper evidence.

---

## Critical Issues (resolved this phase — 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 — verified sound end-to-end in this deep pass.

`globalSetup` runs `TRUNCATE TABLE` against `list_items`, `list_shares`, `lists`,
`calendar_events` using whatever `DB_*` env points at. The fix adds a two-part guard that
throws **before** opening any DB connection:

1. `NODE_ENV === 'production'` → throw (first check, before reading any other env var).
2. `DEV_AUTH_BYPASS !== 'true'` → throw.

**Deep-pass verification (call-chain consistency with the API guard):**

- `apps/api/src/auth/devBypass.ts:61-66` uses the *identical* ordering: `NODE_ENV==='production'`
  checked first, then `DEV_AUTH_BYPASS !== 'true'`. The harness guard mirrors it exactly.
- `apps/api/src/index.ts:24-25` computes `devBypassActive = NODE_ENV !== 'production' &&
  DEV_AUTH_BYPASS === 'true'`, and **only mounts the OIDC guard when `!devBypassActive`**
  (lines 51-55). So the harness's required precondition (`DEV_AUTH_BYPASS=true`) is the same flag
  that makes the API serve Dev User 1 without OIDC — the guards are coupled to the same switch.
- No production path serves authed data: in production `NODE_ENV==='production'` forces
  `devBypassActive=false`, the OIDC middleware is unconditionally mounted, and `devAuthBypass()`
  returns a pure passthrough. The harness guard additionally refuses to even *run* there.

**Residual note (see IN-05):** the guard couples a *data-mutation* safety check to an *auth-mode*
flag. It is correct for this harness, but `DEV_AUTH_BYPASS=true` with `DB_*` pointed at a
populated **dev** DB will still wipe that dev DB — the guard protects production, not "the wrong
non-prod DB." This is acceptable for the stated design (D-06 deterministic reseed) and documented
in README §"What globalSetup Does"; flagged only so it is not mistaken for broader protection.

---

## Blocker Findings (NEW — surfaced by call-chain analysis)

> **BOTH BLOCKERS RESOLVED in commit `53c3ca5`.**
> - **BL-01:** the dead-`EmptyState` / always-rendered-wrapper assertions were replaced with a real
>   DB→UI proof — `getByText('Seeded Test Event')` must be visible in the grid. Verified non-vacuous:
>   passes with the seed on both profiles; with `/api/events` mocked to `[]` the title is absent
>   (the assertion would fail). The old `'Nothing here'` check was empirically confirmed dead
>   (count 0 even with zero events).
> - **BL-02:** the reviewer's stated *mechanism* was inaccurate — the fetch window
>   `[monthStart−7d, monthEnd+7d]` (verified in `calendarStore.initialCalendarRange`) **does** include
>   `now+24h`, so the API window never excludes it. The real fragility is the rendered **month-agenda
>   view of the current month** (both profiles are phone-width): on a month's last day "tomorrow" is in
>   the next month and not displayed. The *conclusion* (a date-fragile visibility assertion) was correct.
>   Fixed by re-anchoring the seed to **noon-today (UTC)** — always today's local date, always in the
>   current-month view.

### BL-01: `calendar.spec.ts` populated-state assertions are vacuous — they cannot fail if the seed regresses

**Files:**
- `apps/pwa/e2e/calendar.spec.ts:66-72` (`Schedule-X calendar grid is visible after seeding`)
- `apps/pwa/e2e/calendar.spec.ts:74-78` (`EmptyState "Nothing here" is NOT present when events are seeded`)
- Ground truth: `apps/pwa/src/components/CalendarShell.tsx:383-389`, `apps/pwa/src/components/EmptyState.tsx:45`

**Issue:**

The spec docstring (`calendar.spec.ts:74-77`) asserts: *"CalendarShell renders EmptyState when the
events query succeeds with zero occurrences."* **This is factually wrong.** `CalendarShell` does
the opposite — its success branch (`CalendarShell.tsx:383-389`) **always** renders
`<ScheduleXCalendar>` and its inline comment states explicitly:

```
// ALWAYS render the calendar even when the window has no events — its built-in
// header carries the navigation, so swapping in an empty-state would strand the
// user ...
```

Consequences traced through source:

1. **`EmptyState` / the string "Nothing here" is dead code on `/calendar`.** A repo-wide search
   confirms `EmptyState.tsx` is imported by **nothing** (only `ListsEmptyState` is imported, by
   `ListsIndex`). So `page.getByText('Nothing here')` matches **zero** elements in every calendar
   state — seeded, empty, error, or loading. `toHaveCount(0)` is therefore **permanently green
   and independent of the seed**. If the seed inserted nothing, or the events query returned `[]`,
   or the API 500'd, this assertion would still pass. It validates nothing.

2. **`.sx-react-calendar-wrapper` is rendered on every successful auth**, with or without events
   (`CalendarShell.tsx:388` is in the non-error, non-loading branch which fires for *any*
   successful `eventsQuery`, including zero occurrences). So
   `expect(page.locator('.sx-react-calendar-wrapper')).toBeVisible()` passes whenever auth +
   the events fetch resolve — it does **not** prove the seeded event reached the grid. The test
   name "...visible **after seeding**" overclaims; it would stay green if the seed were deleted.

**Why this matters (adversarial):** TEST-01/TEST-02's acceptance bar is "the harness measures real
rendered state and fails on regression." These two tests measure *auth + render-of-an-empty-grid*,
not *populated state*. A regression that silently drops all events (broken seed, broken
`/api/events` join, broken `hydrateEvents`) would ship green. The only test in the suite that
actually proves data flows from DB → UI is on the **lists** side (`lists.spec.ts:42-47`,
`getByRole('listitem')` not count 0), which IS sound. The calendar side has no equivalent.

**Fix:** Assert on something only present when the seed's event is actually rendered. Schedule-X
renders an event element carrying the title. Add a positive assertion, e.g.:

```ts
test('seeded event "Seeded Test Event" is rendered in the grid', async ({ page }) => {
  await page.goto('/calendar')
  await expect(page.getByRole('navigation', { name: 'Main navigation' })).toBeVisible()
  // Schedule-X renders the SUMMARY text inside the time/month grid.
  await expect(page.getByText('Seeded Test Event').first()).toBeVisible()
})
```

and **delete** the "Nothing here" assertion (it targets a non-existent render path) — or, if an
empty-state proof is wanted, network-mock `/api/events*` to `{ occurrences: [] }` and assert the
grid renders with **no** event elements (the app's real empty behaviour), not the dead
`EmptyState`. Also correct the false docstring at `calendar.spec.ts:74-77`.

---

### BL-02: Seed event start (`now + 24h`) can fall outside the PWA's initial fetch window (month-boundary flake)

**Files:**
- `apps/pwa/e2e/global-setup.ts:97` (`futureStart = new Date(Date.now() + 24*60*60*1000)`)
- Ground truth: `apps/pwa/src/store/calendarStore.ts:124-137` (`initialCalendarRange`)
- API window contract: `apps/api/src/routes/events.ts:133-214`

**Issue:**

`initialCalendarRange()` builds the first TanStack-Query window as **first-of-current-month −7d**
to **last-of-current-month +7d** (`calendarStore.ts:126-129`). `CalendarShell` issues the initial
`fetchEvents(start, end)` with exactly this range (`CalendarShell.tsx:121-127`). The seed places
the only event at `Date.now() + 24h` (`global-setup.ts:97`).

For ~23 days of the month "tomorrow" is inside `[monthStart−7, monthEnd+7)`. But when the suite
runs on the **last 7 days of a month**, "tomorrow" rolls into the next month and lands **after**
`monthEnd+7` → the seeded row is excluded by the API's date-window pre-filter
(`events.ts:195-200`, the non-recurring timed branch requires `dtstartUtc < windowEnd`) → the
`/api/events` response is `{ occurrences: [] }` → the grid renders empty.

Today this only *masks* itself because BL-01's assertions don't check for the event. But:
- It is a real seed↔window contract violation: the deterministic seed is **not** deterministically
  inside the view the app fetches.
- The moment BL-01 is fixed with a positive "seeded event is visible" assertion (as it must be),
  that assertion becomes **calendar-date-dependent** and will fail on roughly the last week of
  every month, plus any month-length edge (28/29/30/31). This is exactly the kind of
  "coincidentally-green / occasionally-red" flake the matrix is meant to eliminate.

**Secondary correctness note:** the seed sets `all_day=false`, `dtstart_utc=<ts>`, and leaves
`dtstart_date` NULL — this correctly matches the API's "non-recurring timed" WHERE branch
(`events.ts:195-200`), so the *shape* is right. The problem is purely the *position in time*.

**Fix:** Seed the event at a position guaranteed inside `initialCalendarRange()` independent of the
run date — e.g. anchor it to "today at noon UTC" (today is always within the current-month ±7
window) rather than +24h:

```ts
// Anchor inside the PWA's initial window (current month ± 7d) on every run date.
const seedStart = new Date()
seedStart.setUTCHours(12, 0, 0, 0) // noon today UTC — always inside the initial range
```

Document the coupling at the seed site: *"dtstart MUST stay inside
calendarStore.initialCalendarRange() (current month ±7d) or calendar.spec populated assertions
go dark."* This makes the seed↔window invariant explicit so it cannot drift silently (review item 2).

---

## Warnings

### WR-01 (CONFIRMED, deeper evidence): readiness gate accepts the SPA shell, not a working API/DB

**File:** `apps/pwa/e2e/global-setup.ts:50-65`; vite proxy `apps/pwa/vite.config.ts:server.proxy`

The gate polls `${baseURL}/health`. `baseURL` is the **Vite** origin (`:5173`), and
`vite.config.ts` proxies `/health → http://localhost:3000`. So a 200 here *does* prove the API +
DB round-trip (`apps/api/src/routes/health.ts` runs `SELECT 1`, returns 503 on DB failure) **and**
that Vite is up and proxying — this is actually stronger than standard-depth credited. **However**,
the gate does **not** prove `DEV_AUTH_BYPASS=true` is set *in the API process*. If the API was
started without it, `/health` (unauthenticated, mounted before the guard — `index.ts:38`) still
returns 200, the gate passes, the seed runs, then **every spec fails** at the first
`/api/me`/`/api/events` (302 to Authelia). README §Prerequisites warns about this in prose but the
harness cannot detect it. Low-cost hardening: after the seed, the gate could `fetch(baseURL +
'/api/me', {redirect:'manual'})` and assert a 200 (dev-bypass) rather than an opaqueredirect, so a
mis-started API fails *in setup* with a clear message instead of 40 confusing spec failures.

### WR-02 (CONFIRMED): readiness-gate success can be misreported as timeout near the deadline

**File:** `apps/pwa/e2e/global-setup.ts:50-65`

The loop `break`s on `res.ok`, then line 60 re-checks `if (Date.now() >= deadline) throw`. If the
successful `/health` response arrives in the final second (the `await fetch` itself can consume
time), `Date.now()` may have crossed `deadline` by the time control reaches line 60 — throwing a
false "health check never returned 200" **after a successful** health check. Use an explicit
success flag instead of inferring success from the clock:

```ts
let ready = false
while (Date.now() < deadline) {
  try { const res = await fetch(`${baseURL}/health`); if (res.ok) { ready = true; break } }
  catch { /* keep polling */ }
  await new Promise((r) => setTimeout(r, 1_000))
}
if (!ready) throw new Error(`health check never returned 200 ...`)
```

### WR-03 (CONFIRMED): `webServer` starts Vite but the proxy target (API:3000) is not managed → readiness gate is the only thing standing between "Vite up" and "specs fail"

**File:** `apps/pwa/playwright.config.ts:59-64`

`webServer` runs `pnpm --filter @familysync/pwa dev` (Vite only) — correct per D-10 (API + DB +
Redis are compose-managed). But `webServer.url` is `:5173`; Playwright considers the server "ready"
when Vite answers, **before** `globalSetup` polls `/health`. If the operator forgets the API,
Playwright still launches; the failure is deferred to `globalSetup`'s 60s `/health` timeout. That
is the intended contract (D-09: "you bring up the stack; the harness waits"), and the deferral is
clean — keeping this as a WARNING only because `reuseExistingServer: !CI` (line 62) means in CI a
fresh Vite is spawned that *also* needs the proxied API already up; the README §CI section relies on
the CI job ordering the API before the runner. No code defect; documentation-coupling risk.

### WR-04 (DOWNGRADE → resolved-correct): `page.route('/api/lists')` exact match is correct, not too-narrow

**File:** `apps/pwa/e2e/lists.spec.ts:74, 96`

Standard-depth flagged the exact string `/api/lists` (no glob) as possibly missing the real request.
Deep trace: `listsClient.fetchLists()` calls `apiFetch('/lists')` → `fetch('/api/lists')` with **no
query string** (`listsClient.ts:12-21, 56-58`). Playwright resolves the bare path against `baseURL`
→ `http://localhost:5173/api/lists`, which the exact matcher matches. There is no
`/api/lists?foo` variant. The exact match is correct and *intentionally* narrow so it does **not**
swallow `/api/lists/:id/items` (which would break if a glob were used). **No change needed** —
recording the downgrade so it is not "fixed" into a brittle glob.

### WR-05 (CONFIRMED): `page.unroute` is not in a `finally` → a failing assertion leaks the mock to later tests

**Files:** `apps/pwa/e2e/calendar.spec.ts:116, 136, 159`; `apps/pwa/e2e/lists.spec.ts:92, 119`

Each error/empty-state test registers `page.route(...)` then calls `page.unroute(...)` at the end of
the body. If any `expect` between them throws (the whole point of the test), `unroute` never runs.
Playwright gives each test a **fresh page/context** by default, so route handlers do **not** leak
across tests in practice — which is why this hasn't bitten. But the explicit `unroute` calls signal
an *intent* to isolate that the code doesn't actually guarantee; under `fullyParallel` +
`test.describe.serial` refactors, or if someone moves a `route` into `beforeEach`/`beforeAll`, the
leak becomes real. Either drop the now-redundant `unroute` calls (rely on per-test context
isolation) or wrap them in `try/finally`. Keeping as WARNING: dead-but-misleading cleanup code.

### WR-06 (CONFIRMED): self-validation "remove style by reload" comment is wrong; the test removes it via `evaluate`

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

Comment says *"REMOVE the injected style by navigating (page.reload drops inline style tags)"* but
the code removes it with `styleHandle.evaluate((el) => el.remove())` — no reload occurs. The code is
correct; the comment is misleading and will send a maintainer down the wrong path. Fix the comment
to describe the actual `evaluate(... .remove())` removal.

### WR-07 (CONFIRMED, deeper evidence): SW-controller assertion is near-vacuous on the WebKit (iPhone) profile

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

`serviceWorkers: 'block'` is set on **both** profiles (`playwright.config.ts:44, 52`). The test reads
`navigator.serviceWorker.controller` and asserts it is null. Two reasons it's weak:

1. On **WebKit over plain `http://localhost`**, `navigator.serviceWorker` is frequently
   **undefined** (SW requires a secure context; WebKit is stricter than Chromium about treating
   `localhost` as secure in emulation). The test's own guard (`if (!('serviceWorker' in navigator))
   return null`, line 50) then returns null and the assertion passes **without ever proving the
   block worked** — it passes because SW isn't available at all, not because it was blocked.
2. Even on Chromium, `controller` is null on a first, uncontrolled load *regardless* of the
   `block` setting (a freshly-loaded page with a not-yet-activated SW also has null controller).

So this asserts "no controlling SW," which is true in the negative cases for reasons unrelated to
`serviceWorkers:'block'`. To actually prove the config blocks registration, assert that
`navigator.serviceWorker.getRegistration()` (where defined) resolves to `undefined`, and skip the
test where `serviceWorker` is absent so an unavailable API doesn't masquerade as a passing block.

---

## Info

### IN-01: `mysql2` placed as a PWA `devDependency` — acceptable, with a caveat

**File:** `apps/pwa/package.json:38`

`mysql2@3.22.4` is a `devDependency` of `@familysync/pwa`, used only by `e2e/global-setup.ts`
(`import mysql from 'mysql2/promise'`). It is also an `apps/api` dependency
(`apps/api/package.json:24`) at the same pinned version. Placement is correct (the seed is
dev/test-only and never bundled into the PWA — vitest excludes `e2e/**`, and Vite never imports it).
Caveat: the version is pinned independently in two packages; if the API bumps `mysql2` and the
harness doesn't, the seed could connect with a driver version skewed from the app's. Low risk
(MariaDB wire protocol is stable) but worth a note to keep the two pins in lockstep.

### IN-02: `tsconfig.e2e.json` `types: ["node"]` correctly re-adds DOM via `lib`, but narrows ambient types

**File:** `apps/pwa/tsconfig.e2e.json:4-5`

The base `tsconfig.json` has **no** `types` field, so it includes all `@types/*` ambiently. The e2e
config sets `types: ["node"]`, which *restricts* ambient `@types` to node only — intentional so the
seed (Node `fetch`, `setTimeout`, `mysql2`) typechecks. DOM globals used in `page.evaluate`
callbacks come from `lib: ["DOM", "DOM.Iterable"]` (line 5), which is correct because those
callbacks are type-checked as DOM code. This is sound. Noting only that `@playwright/test` brings its
own types via direct import (not ambient), so narrowing `types` doesn't break the specs. No action.

### IN-03: vitest `exclude: ['e2e/**']` correctly isolates Playwright specs from jsdom

**File:** `apps/pwa/vitest.config.ts:17`

Confirmed the exclusion prevents vitest from loading `e2e/*.spec.ts` (which import `@playwright/test`
`devices`, unavailable in jsdom). The complementary direction is also covered: `playwright.config.ts`
`testDir: './e2e'` + `testMatch: '**/*.spec.ts'` scopes Playwright to `e2e/` only, so it never picks
up `src/**/*.test.tsx` vitest files. The two runners are cleanly partitioned. No action.

### IN-04: `typecheck` script covers the e2e tsconfig — good

**File:** `apps/pwa/package.json:10`

`typecheck` runs both `tsc --noEmit` and `tsc --project tsconfig.e2e.json --noEmit`, so the harness
files are type-checked in CI (addresses the project-memory note "Vitest passes while tsc fails").
The root `typecheck` (`package.json:12`, `pnpm -r typecheck`) fans this out. No action.

### IN-05: CR-01 guard protects *production*, not "the wrong dev DB"

**File:** `apps/pwa/e2e/global-setup.ts:34-44`

(See CR-01 residual note.) The guard refuses to run unless `DEV_AUTH_BYPASS=true` and
`NODE_ENV!=='production'`. It does **not** distinguish a developer's *populated* local/staging
MariaDB from a throwaway test DB — both satisfy the guard and both get TRUNCATEd. This is by design
(D-06 reseed) and documented, but a defense-in-depth improvement would be to additionally require an
explicit opt-in like `E2E_ALLOW_TRUNCATE=true` or assert the DB name matches a `*_test`/`*_e2e`
pattern before truncating, so pointing `DB_*` at a real dev DB by accident doesn't silently wipe it.

---

## Cross-File Soundness Matrix (assertion → real code traced)

| Spec assertion | Targets | Sound? | Notes |
|---|---|---|---|
| `getByRole('navigation', {name:'Main navigation'})` | `BottomTabBar.tsx:61-62` (`<nav aria-label>`) | YES | Only one nav landmark on mobile; DesktopNav nav hidden ≥768px |
| Calendar/Lists tab ≥44px | `BottomTabBar.tsx` NavLink `minHeight:44px` + 56px bar | YES | Measures rendered geometry; self-validation proof present |
| Settings button ≥44px, name `/open settings/i` | `AppNav.tsx:90-92` (`aria-label="${name} — open settings"`) | YES | matches |
| `getByText('FamilySync', {exact:true})` | `AppNav.tsx:87` PhoneNav header text | YES | exact avoids "Install FamilySync" |
| New Event FAB ≥56px, name `New Event` | `CalendarShell.tsx:467-470` (phone FAB) | YES | matches |
| Error heading `Couldn't load events` + Retry ≥44px | `CalendarShell.tsx:354, 365-381` | YES | route-mock `/api/events*` → 500; matches `fetchEvents` URL `/api/events?...` |
| `.sx-react-calendar-wrapper` visible "after seeding" | `CalendarShell.tsx:388` | **NO (BL-01)** | always rendered on success; not seed-dependent |
| `Nothing here` count 0 when seeded | `EmptyState.tsx:45` (dead code) | **NO (BL-01)** | never rendered on /calendar; permanently green |
| `Open list: E2E Grocery List` visible | `ListCard.tsx:60` (`aria-label`) | YES | seed list name matches; sound |
| ≥1 `listitem` when seeded | `ListCard.tsx:52` (`role="listitem"`) | YES | the ONE real DB→UI proof in the suite |
| `No lists yet` count 0 when seeded / visible when `[]` | `ListsEmptyState.tsx:46` via `ListsIndex.tsx:218` | YES | route-mock `/api/lists` → `{lists:[]}`; URL matches `fetchLists` |
| `Tap + to create` visible | `ListsEmptyState.tsx:58` | YES | partial regex matches |
| SW controller null | `serviceWorkers:'block'` | WEAK (WR-07) | near-vacuous on WebKit/http |
| DEV_AUTH_BYPASS reached authed PWA | `devBypass.ts` + `index.ts:51-55` | YES | hostname stays localhost; nav appears post-auth |

---

_Reviewed: 2026-06-11T03:30:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: deep_