docs(07): deep code review report (2 blockers, CR-01 verified resolved)
This commit is contained in:
Lucas Berger
@@ -1,7 +1,7 @@
|
||||
---
|
||||
phase: 07-mobile-test-harness
|
||||
reviewed: 2026-06-11T00:00:00Z
|
||||
depth: standard
|
||||
reviewed: 2026-06-11T03:30:00Z
|
||||
depth: deep
|
||||
files_reviewed: 11
|
||||
files_reviewed_list:
|
||||
- apps/pwa/playwright.config.ts
|
||||
@@ -18,152 +18,379 @@ files_reviewed_list:
|
||||
findings:
|
||||
critical: 0
|
||||
critical_resolved: 1
|
||||
blocker: 2
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 12
|
||||
info: 5
|
||||
total: 14
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 7: Code Review Report
|
||||
# Phase 7: Code Review Report (DEEP)
|
||||
|
||||
**Reviewed:** 2026-06-11
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 11
|
||||
**Depth:** deep (cross-file call-chain analysis)
|
||||
**Files Reviewed:** 11 (harness) + 12 cross-referenced app/API source files
|
||||
**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.
|
||||
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.
|
||||
|
||||
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 deep pass **confirms CR-01 is soundly fixed end-to-end** but escalates two findings to
|
||||
**BLOCKER** that only cross-file analysis surfaces:
|
||||
|
||||
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.
|
||||
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.
|
||||
|
||||
## Critical Issues
|
||||
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.
|
||||
|
||||
### CR-01: globalSetup truncates the DB with no production / test-environment guard — ✅ RESOLVED (commit fcc680e)
|
||||
|
||||
**Resolution:** Fail-closed guard added at the top of `global-setup.ts` (Step 0) — hard `NODE_ENV==='production'` check first, then `DEV_AUTH_BYPASS!=='true'` refusal, before opening any DB connection. README updated with the test-process env requirement. Verified: guard throws without `DEV_AUTH_BYPASS`; full 58-test suite passes with it.
|
||||
|
||||
**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.
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-11_
|
||||
## 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)
|
||||
|
||||
### 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: standard_
|
||||
_Depth: deep_
|
||||
|
||||
Reference in New Issue
Block a user