luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(07): add code review report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-11 02:20:32 -04:00
parent 10e570b21d
commit 8458dc25eb
1 changed files with 166 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/07-mobile-test-harness/07-REVIEW.md
+166
View File
@@ -0,0 +1,166 @@
---
phase: 07-mobile-test-harness
reviewed: 2026-06-11T00:00:00Z
depth: standard
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: 1
warning: 7
info: 4
total: 12
status: issues_found
---
# Phase 7: Code Review Report
**Reviewed:** 2026-06-11
**Depth:** standard
**Files Reviewed:** 11
**Status:** issues_found
## Summary
Reviewed the Phase 7 Playwright mobile test harness: a two-profile device matrix config, a `globalSetup` that resets/seeds the dev MariaDB, and three spec files asserting mobile layout/calendar/lists state.
Overall the harness is well-constructed: credentials are env-only (no hardcoding), SQL writes are parameterized, the device matrix and SW-block decisions are correct, and the calendar/lists API mocks match the real frontend URLs (`/api/events?...`, `/api/lists`). The populated-state seed lands inside the calendar's default window (now+24h vs. month±7d ≤ 90d span) and the apostrophe in the `"Couldn't load events"` assertion is plain ASCII matching the source `<h2>`, so those assertions are not vacuous.
The dominant defect is **missing seed safety guardrails**: `global-setup.ts` issues `TRUNCATE` against whatever database the `DB_*` env vars point at, with no production/non-test guard, no confirmation that auth-bypass is the active mode, and silent fallbacks for every connection parameter including an empty password. A misconfigured `DB_HOST`/`DB_NAME` (or running this against a prod-pointed shell) wipes four tables irreversibly. Several determinism and assertion-soundness warnings follow.
## Critical Issues
### CR-01: globalSetup truncates the DB with no production / test-environment guard
**File:** `apps/pwa/e2e/global-setup.ts:50-65`
**Issue:** The setup connects using `DB_*` env vars (each defaulting silently — `DB_HOST` to `127.0.0.1`, `DB_NAME` to `familysync`, `DB_PASSWORD` to empty string) and immediately runs `SET FOREIGN_KEY_CHECKS=0` followed by four `TRUNCATE TABLE` statements. There is **no guard** that:
- `NODE_ENV !== 'production'`
- `DEV_AUTH_BYPASS === 'true'` (the README claims this is the harness's safety contract, but the seed never checks it)
- the target host/db is a recognized dev/CI database
The production app reuses these exact env-var names (`apps/api/src/db/client.ts`). If an operator runs the suite from a shell that still has production `DB_HOST`/`DB_NAME`/`DB_PASSWORD` exported (a common footgun after a prod ops session), the harness silently truncates `list_items`, `list_shares`, `lists`, and `calendar_events` in production. Unlike the API's `devBypass.ts`, which makes `NODE_ENV === 'production'` the hard first guard, the seed has no equivalent. This is a data-loss vulnerability, not a style issue.
**Fix:** Add a fail-closed guard before opening the connection or running any DDL:
```ts
if (process.env.NODE_ENV === 'production') {
throw new Error('global-setup refuses to seed: NODE_ENV=production')
}
if (process.env.DEV_AUTH_BYPASS !== 'true') {
throw new Error(
'global-setup refuses to seed: DEV_AUTH_BYPASS must be "true" (dev/CI only). ' +
'This prevents truncating a non-test database.',
)
}
// Optional belt-and-suspenders: require an explicit opt-in token
// if (process.env.E2E_ALLOW_DB_RESET !== '1') throw new Error(...)
```
Mirror the API's pattern: production check first, before reading any other connection var.
## Warnings
### WR-01: Readiness gate accepts any 2xx/3xx, including an auth-redirect login page
**File:** `apps/pwa/e2e/global-setup.ts:30-45`
**Issue:** The poll breaks on `res.ok` (200–299). If the API is running **without** `DEV_AUTH_BYPASS` and `/health` is itself behind the OIDC guard, `fetch` follows the redirect and may resolve to a 200 Authelia login page — `res.ok` is true, the gate passes, then every spec fails opaquely at `getByRole('navigation')`. The README states the harness "cannot inject DEV_AUTH_BYPASS at runtime," yet the gate does nothing to detect that the bypass is off. The readiness signal is weaker than the comment ("Poll until 200 OK") implies.
**Fix:** Assert the response is the real health payload, not just `res.ok`. If `/health` returns JSON, check a known field; at minimum disable redirect-following so an auth bounce is visible:
```ts
const res = await fetch(`${baseURL}/health`, { redirect: 'manual' })
if (res.status === 200) {
// optionally: const body = await res.json(); if (body.status === 'ok') break
break
}
```
### WR-02: Deadline-expiry detection has a false-positive race
**File:** `apps/pwa/e2e/global-setup.ts:30-45`
**Issue:** After the `while (Date.now() < deadline)` loop, success is inferred by re-checking `if (Date.now() >= deadline)`. If `/health` becomes ready on the final iteration but the 60s deadline elapses by the time control reaches line 40 (e.g. a slow final `fetch`/seed-machine contention), the code throws "health check never returned 200" **even though it did**. The loop `break` and the failure condition are not the same signal, so the success path is not authoritative.
**Fix:** Track readiness with an explicit boolean instead of inferring it 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 {}
await new Promise<void>((r) => setTimeout(r, 1_000))
}
if (!ready) throw new Error(...)
```
### WR-03: `webServer` proxy dependency makes the /health gate non-deterministic on a cold stack
**File:** `apps/pwa/playwright.config.ts:59-64` + `apps/pwa/e2e/global-setup.ts:32`
**Issue:** `globalSetup` polls `${PLAYWRIGHT_BASE_URL}/health` (the Vite origin, `:5173`), which only resolves because Vite proxies `/health` → `:3000`. Playwright starts `webServer` (Vite) and waits on its `url`, but `globalSetup` runs after that — so the gate effectively waits on the API *through* the Vite proxy. If the proxied API is down, the Vite server still answers `/health` with a proxy error (often a 5xx, sometimes a 200/HTML depending on Vite version), and combined with WR-01's `res.ok`-only check the gate can mis-fire either direction. The gate should target the dependency it actually guards.
**Fix:** Poll the API health endpoint directly (e.g. a separate `API_BASE_URL` env, defaulting to `http://127.0.0.1:3000`) rather than relying on the Vite proxy, since the API + DB are the resources the seed depends on.
### WR-04: Empty-state lists spec only mocks `/api/lists` exactly — leaks if the index ever paginates or appends a query
**File:** `apps/pwa/e2e/lists.spec.ts:74-80, 96-102`
**Issue:** `page.route('/api/lists', ...)` is an exact-string match (no glob). `fetchLists()` currently calls `/api/lists` with no query string, so it matches today. But this is brittle: the moment the client adds any query param (cursor, `?include=counts`, cache-buster) the route silently stops matching, the real seeded list is returned, and the "empty state" assertion `getByText('No lists yet')` fails with no indication that the mock missed. The calendar spec correctly uses the `/api/events*` glob for exactly this reason; the lists spec is inconsistent.
**Fix:** Use a glob to be robust to query strings, matching the calendar spec's pattern:
```ts
await page.route('/api/lists*', (route) => route.fulfill({ ... }))
await page.unroute('/api/lists*')
```
### WR-05: `unroute` is not in a `finally`, so a mid-test failure leaks the route mock across tests
**File:** `apps/pwa/e2e/calendar.spec.ts:98-117, 122-137, 140-160` and `apps/pwa/e2e/lists.spec.ts:74-93, 96-120`
**Issue:** Each error/empty-state test registers `page.route(...)` and calls `page.unroute(...)` as the **last statement**. If any preceding `await expect(...)` fails or times out, the test aborts before `unroute` runs. Playwright does reset routes per-`page`/per-test in most cases, but these specs explicitly rely on `unroute` for isolation (the inline comments say "so the mock does not leak to subsequent tests"). Relying on a trailing statement that the failure path skips defeats that stated guarantee and can produce confusing cascade failures where one broken test poisons the next within the same worker if pages are reused.
**Fix:** Register cleanup that always runs, e.g. `test.afterEach(({ page }) => page.unrouteAll())`, or wrap the body in `try { ... } finally { await page.unroute('/api/events*') }`. Prefer `afterEach` + `unrouteAll()` for the whole describe block.
### WR-06: Self-validation "removal" step contradicts its own comment and never reloads
**File:** `apps/pwa/e2e/layout.spec.ts:209-211, 250-251`
**Issue:** The comment states *"REMOVE the injected style by navigating (page.reload drops inline style tags)"* but the code does **not** reload — it calls `styleHandle.evaluate((el) => el.remove())`. The comment is misleading (a future maintainer may "fix" it by adding a reload that re-runs auth and breaks the measurement). More substantively, removing a `<style>` tag via `Element.remove()` and immediately re-measuring `boundingBox()` assumes synchronous style recalc; WebKit (iPhone profile) can defer layout, so the post-removal assertion (`height ≥ 44px`) is mildly flaky without an explicit wait for the geometry to settle.
**Fix:** Delete the stale "by navigating / page.reload" wording, and gate the re-measure on the value settling rather than measuring immediately:
```ts
await styleHandle.evaluate((el) => el.remove())
await expect.poll(async () => (await calTab.boundingBox())?.height ?? 0).toBeGreaterThanOrEqual(44)
```
### WR-07: SW-controller assertion can pass vacuously where it is least meaningful
**File:** `apps/pwa/e2e/calendar.spec.ts:48-54`
**Issue:** The `page.evaluate` returns `null` when `navigator.serviceWorker` is **undefined** ("Treat undefined as no-controller (safe)"), and `expect(controller).toBeNull()` then passes. But on the WebKit/iPhone profile over plain `http://localhost`, `navigator.serviceWorker` can legitimately be undefined regardless of the `serviceWorkers: 'block'` setting — so on that profile the test asserts nothing about whether the block actually took effect. It is green by construction, not by verification, exactly on the engine where SW behavior differs most. This is a soundness gap for a test whose stated purpose is "SW-block enforced."
**Fix:** Distinguish "SW API absent" from "SW present but not controlling." If the goal is to prove the block, register a SW in-page and assert registration is rejected/empty, or assert `navigator.serviceWorker.controller === null` only when `'serviceWorker' in navigator` and skip with an explicit `test.skip()` annotation otherwise so the vacuous case is visible rather than silently passing.
## Info
### IN-01: `mysql2` is a devDependency of the PWA package solely for the seed script
**File:** `apps/pwa/package.json:38`
**Issue:** `mysql2` (a backend DB driver) sits in the frontend PWA package because `global-setup.ts` imports it. This is acceptable but couples the PWA's dependency surface to DB internals; a careless bundler/import from `src/` could now pull a Node-only driver into the browser build.
**Fix:** Acceptable as-is for a monorepo. Optionally move the seed + its `mysql2` dependency into a small `@familysync/e2e` or `tools` workspace so the PWA's runtime deps stay UI-only. At minimum, ensure no `src/` code can import `mysql2` (lint rule / import boundary).
### IN-02: Seed-anchor values duplicated as prose contracts across four files
**File:** `apps/pwa/e2e/global-setup.ts:14-18`, `calendar.spec.ts`, `lists.spec.ts`, `e2e/README.md`
**Issue:** The anchor strings (`'Seeded Test Event'`, `'E2E Grocery List'`, `'Milk'`, `'Eggs'`, `calendar_id=10`) are hand-synchronized via comments in four places. Drift between the seed and a spec produces a silent assertion failure with no compile-time link.
**Fix:** Export the anchors from a single shared module (e.g. `e2e/fixtures.ts`) and import them in both the seed and the specs so a rename is a single edit the type system enforces.
### IN-03: Magic literal `calendar_id=10` is unexplained at the seed site
**File:** `apps/pwa/e2e/global-setup.ts:69-72, 99`
**Issue:** The shared-calendar id `10` is hardcoded with only a passing comment. Its provenance (matches dev-data memory "shared calendar id 10") is not obvious to a future maintainer and is unrelated to any auto-increment guarantee — `INSERT IGNORE ... (id, ...) VALUES (10, ...)` forces it, which is fragile if the dev DB ever assigns id 10 to a different calendar.
**Fix:** Promote to a named constant (`const E2E_SHARED_CALENDAR_ID = 10`) in the shared fixtures module with a one-line rationale, and consider keying the seed on a stable `url` instead of a hardcoded numeric id.
### IN-04: `futureStartUtc` string construction is hand-rolled and locale/format-fragile
**File:** `apps/pwa/e2e/global-setup.ts:79-82`
**Issue:** Converting the ISO string to MariaDB `TIMESTAMP` via `.replace('T',' ').replace(/\.\d+Z$/,'')` works but is brittle and duplicated logic; a future `Date` whose `toISOString()` lacks fractional seconds (it always has them today) or a column type change would break it silently. The VEVENT `DTSTART`/`DTEND` use a third, separate formatting expression.
**Fix:** Centralize UTC-timestamp formatting in one helper, or let mysql2 format the `Date` by passing the `Date` object directly to a `?` placeholder where the column is a `TIMESTAMP`/`DATETIME` (mysql2 formats JS `Date` to the correct SQL literal), removing the manual string surgery.
---
_Reviewed: 2026-06-11_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_