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

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

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

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:

// 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 breaks 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:

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