--- 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 `` 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=`, 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` (`