diff --git a/.planning/phases/07-mobile-test-harness/07-REVIEW-FIX.md b/.planning/phases/07-mobile-test-harness/07-REVIEW-FIX.md new file mode 100644 index 0000000..adfa98e --- /dev/null +++ b/.planning/phases/07-mobile-test-harness/07-REVIEW-FIX.md @@ -0,0 +1,104 @@ +--- +phase: 07-mobile-test-harness +fixed_at: 2026-06-11T08:05:00Z +review_path: .planning/phases/07-mobile-test-harness/07-REVIEW.md +iteration: 1 +findings_in_scope: 12 +fixed: 6 +skipped: 6 +status: partial +--- + +# Phase 7: Code Review Fix Report + +**Fixed at:** 2026-06-11T08:05:00Z +**Source review:** .planning/phases/07-mobile-test-harness/07-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope (fix_scope=all): 12 open/actionable + info; CR-01/BL-01/BL-02 already resolved (left intact) +- Fixed: 6 (WR-01, WR-02, WR-05, WR-06, WR-07 — and WR-02/WR-01 share one commit) +- Skipped: 6 (WR-03, WR-04, IN-01..IN-05) — by-design / positive notes, no net-positive edit available + +**Verification evidence (all fixes):** +- Full E2E suite (both profiles, iphone/WebKit + pixel/Chromium): **58 passed** (29.4s), suite exit 0. +- `pnpm --filter @familysync/pwa typecheck` (both `tsconfig.json` and `tsconfig.e2e.json`): **exit 0**. +- SW test (WR-07) confirmed passing on BOTH iphone(WebKit) and pixel(Chromium) — re-run in isolation: 2 passed. +- Suite was run from the isolated worktree with `.env` sourced from the main repo (worktree `.env` is gitignored/absent) + `DEV_AUTH_BYPASS=true DB_HOST=127.0.0.1 DB_PORT=3306`. + +## Fixed Issues + +### WR-01: readiness gate accepts the SPA shell, not a working DEV_AUTH_BYPASS API + +**Files modified:** `apps/pwa/e2e/global-setup.ts` +**Commit:** 9c38dd3 (shared with WR-02) +**Applied fix:** Added a Step 1b probe after the `/health` gate: `fetch(baseURL + '/api/me', { redirect: 'manual' })` and throw with a clear, actionable message unless it returns 200. If the API was started without `DEV_AUTH_BYPASS=true`, `/api/me` redirects (302) to Authelia; the gate now fails loudly in setup instead of producing ~40 confusing spec failures. Verified: the seed ran and all 58 specs passed, proving the new gate does not false-positive against the correctly-configured dev stack. + +### WR-02: readiness-gate success misreported as timeout near the deadline + +**Files modified:** `apps/pwa/e2e/global-setup.ts` +**Commit:** 9c38dd3 (shared with WR-01) +**Applied fix:** Replaced the post-loop `if (Date.now() >= deadline) throw` (which can misclassify a success that arrived in the final second as a timeout, because `await fetch` itself consumes time) with an explicit `let ready = false` flag set inside the loop on `res.ok`; throw only `if (!ready)`. Removes the clock-inference race. Verified by full green suite (globalSetup executes once at suite start). + +> Note: WR-01 and WR-02 are committed together because both edits live in the same contiguous readiness-gate hunk in `global-setup.ts` (no `gsd-tools` / interactive hunk-split available to separate one hunk into two commits). Both are readiness-gate robustness changes. + +### WR-05: `page.unroute` not in `finally` — misleading dead cleanup + +**Files modified:** `apps/pwa/e2e/calendar.spec.ts`, `apps/pwa/e2e/lists.spec.ts` +**Commit:** 2b745ad +**Applied fix:** Removed the 5 trailing `page.unroute(...)` calls (calendar: error-heading, retry-44px, error-overflow tests; lists: empty-state, empty-overflow tests) and replaced each with a one-line comment explaining that Playwright gives each test a fresh page/context, so route handlers do not leak across tests — and that a trailing unroute never runs anyway if an `expect` above throws. Chose "drop redundant calls" over "wrap in try/finally" per the reviewer's stated options; it is the lower-noise option and matches real per-test isolation. Verified: all route-mocked error/empty-state tests still pass on both profiles. + +### WR-06: self-validation "remove style by reload" comment is wrong + +**Files modified:** `apps/pwa/e2e/layout.spec.ts` +**Commit:** 5322cfc +**Applied fix:** Corrected both misleading comments (Rule 1 proof ~L209, Rule 2 proof ~L250) that claimed the injected `