From f9fc7bbda2ead6310ba5a2079d7febaeda151d1a Mon Sep 17 00:00:00 2001
From: Lucas Berger <me@lucasberger.ca>
Date: Fri, 12 Jun 2026 09:07:20 -0400
Subject: [PATCH] docs(14): add code review report

---
 .../14-desktop-e2e-coverage/14-REVIEW.md      | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 .planning/phases/14-desktop-e2e-coverage/14-REVIEW.md

diff --git a/.planning/phases/14-desktop-e2e-coverage/14-REVIEW.md b/.planning/phases/14-desktop-e2e-coverage/14-REVIEW.md
new file mode 100644
index 0000000..dfbff20
--- /dev/null
+++ b/.planning/phases/14-desktop-e2e-coverage/14-REVIEW.md
@@ -0,0 +1,94 @@
+---
+phase: 14-desktop-e2e-coverage
+reviewed: 2026-06-12T00:00:00Z
+depth: standard
+files_reviewed: 6
+files_reviewed_list:
+  - apps/pwa/playwright.config.ts
+  - apps/pwa/e2e/layout.spec.ts
+  - apps/pwa/e2e/calendar.spec.ts
+  - apps/pwa/e2e/lists.spec.ts
+  - apps/pwa/e2e/README.md
+  - .gitea/workflows/ci.yml
+findings:
+  critical: 0
+  warning: 3
+  info: 3
+  total: 6
+status: issues_found
+---
+
+# Phase 14: Code Review Report
+
+**Reviewed:** 2026-06-12
+**Depth:** standard
+**Files Reviewed:** 6
+**Status:** issues_found
+
+## Summary
+
+Phase 14 adds a third Playwright project (`desktop` — Desktop Chrome, 1280×720, no `hasTouch`) and makes the existing mobile-authored e2e specs green on desktop. The diff is test-harness-only — no runtime/app code changed. I verified the load-bearing source assumptions encoded in the new skip guards against the actual components:
+
+- **Skip guards are correct.** On desktop (≥768px) `AppNav` renders `DesktopNav` with the sole `<nav aria-label="Main navigation">` (`AppNav.tsx:168`), and `BottomTabBar` returns `null` (`BottomTabBar.tsx:56`). There is no strict-mode collision and the safe-area-inset bottom-edge assertion is genuinely meaningless for a left sidebar — the two `BottomTabBar is fully in-viewport` skips are right.
+- **FAB skip / desktop-parity test are consistent.** On desktop the only `button` with accessible name "New Event" is the toolbar button (`CalendarShell.tsx:436-457`, text "New Event", `minHeight: '44px'`). The FAB is gated behind `phone &&` (`CalendarShell.tsx:464`), so the 56×56 FAB assertion correctly skips and the new ≥44px parity test (`testInfo.project.name !== 'desktop'` inverse-guard) correctly runs only on desktop. The two guards target the exact same locator — internally consistent.
+- **No mobile assertion was weakened or deleted.** `calendar.spec.ts` and `lists.spec.ts` changes are comment-only. `layout.spec.ts` only adds guards + one new test; every prior mobile assertion is intact. The CI workflow change is comment-only (step name + comment), the `desktop` project runs automatically because the harness invokes `playwright test` with no `--project` filter.
+
+Findings below are quality/determinism concerns, not correctness blockers. The harness logic is sound.
+
+## Warnings
+
+### WR-01: Two "PhoneNav" tests silently re-target DesktopNav on desktop and no longer test what their names claim
+
+**File:** `apps/pwa/e2e/layout.spec.ts:83-96`
+**Issue:** `PhoneNav header is visible (Rule 3)` and `PhoneNav settings button meets 44×44px (Rule 1)` are NOT guarded, so they run on the `desktop` project. On desktop `PhoneNav` is never rendered (`AppNav.tsx:34` returns `DesktopNav` at ≥768px). The tests pass only by coincidence:
+- `getByText('FamilySync', { exact: true })` matches the `DesktopNav` sidebar title (`AppNav.tsx:181`) instead of the PhoneNav header.
+- `getByRole('button', { name: /open settings/i })` matches the `DesktopNav` avatar button (`AppNav.tsx:220-222`, `minHeight: '44px'`) instead of the PhoneNav settings button.
+
+This is misleading green: a future regression that breaks PhoneNav specifically would still pass on desktop, and the test name asserts a component that isn't on screen. The phase scope ("right tests skipped, no over/under-skipping") implies these mobile-named geometry tests should either be desktop-skipped (like the FAB) or renamed to reflect that on desktop they validate the DesktopNav equivalent.
+**Fix:** Either guard them mobile-only and add explicit DesktopNav counterparts, e.g.:
+```ts
+test('PhoneNav header is visible (Rule 3)', async ({ page }, testInfo) => {
+  test.skip(testInfo.project.name === 'desktop',
+    'PhoneNav is not rendered at ≥768px; DesktopNav renders the FamilySync title instead');
+  // ...
+});
+```
+or rename to "App header/title is visible" and "Settings affordance meets 44×44px" so the assertion is honest across all three profiles.
+
+### WR-02: calendar.spec "all tests pass unchanged" relies on an untested Schedule-X view (month-grid) on desktop
+
+**File:** `apps/pwa/e2e/calendar.spec.ts:16`, `apps/pwa/e2e/calendar.spec.ts:92-101`
+**Issue:** The header comment claims desktop runs "all tests pass unchanged", but the desktop default Schedule-X view differs from mobile: `readPersistedView()` returns `month-grid` for ≥768px vs `month-agenda` for ≤767px (`apps/pwa/src/store/calendarStore.ts:14-15,119-120`). The `seeded event "Seeded Test Event" is rendered (DB→UI proof)` test asserts `getByText('Seeded Test Event').first()` is visible — on desktop this now exercises month-grid rendering that the iphone/pixel profiles never covered. In month-grid, an event title can be collapsed into a "+N more" overflow affordance when a day cell is crowded. With a single seeded event today this won't trigger, so the risk is low, but the claim "unchanged" understates that desktop adds a new rendering path. If global-setup is ever extended to seed multiple same-day events, this assertion could flake/fail on desktop only.
+**Fix:** Note the view divergence in the comment, and make the assertion view-robust if multi-event seeds are anticipated — e.g. assert the event is reachable, falling back to an explicit week/day view switch, or pin the desktop test to a deterministic view. At minimum, change the comment to "all tests pass; desktop renders month-grid (mobile renders month-agenda)".
+
+### WR-03: lists.spec / calendar.spec "all tests pass unchanged" claim is unverified by the diff and contradicted by view/landmark divergence
+
+**File:** `apps/pwa/e2e/lists.spec.ts:19`, `apps/pwa/e2e/calendar.spec.ts:16`
+**Issue:** Both comment headers added by this phase assert "(all tests pass unchanged)" for the desktop profile. The lists `at least one list item is present` test (`lists.spec.ts:43-48`) and `seeded "E2E Grocery List" card` test depend on `ListsIndex.tsx:152` `role="list"` + `ListCard.tsx:52` `role="listitem"`, which are viewport-independent — those are fine. But the blanket "unchanged" wording is an unverifiable assertion baked into source comments; it papers over WR-01 (PhoneNav tests retarget) and WR-02 (month-grid). Comments that assert test outcomes drift out of date and mislead future readers into trusting desktop coverage they don't have.
+**Fix:** Replace "all tests pass unchanged" with a factual statement of what desktop exercises (e.g. "desktop renders DesktopNav sidebar + month-grid; mobile-geometry tests are skipped, see layout.spec.ts"). Keep outcome claims out of source comments — let the CI run be the source of truth.
+
+## Info
+
+### IN-01: `serviceWorkers: 'block'` is duplicated across all three projects instead of hoisted to shared `use`
+
+**File:** `apps/pwa/playwright.config.ts:45,53,62`
+**Issue:** Every project repeats `serviceWorkers: 'block'`. The config comment (`line 6`) states it applies to all profiles, so it is a global invariant, not a per-device override. Repeating it invites a future profile being added without it (silent SW-block regression).
+**Fix:** Hoist `serviceWorkers: 'block'` into the top-level `use` block (line 28-34) and drop it from each project. Per-project `use` only needs the device descriptor spread.
+
+### IN-02: New desktop parity test has no explicit auth/mount wait before locating the toolbar button
+
+**File:** `apps/pwa/e2e/layout.spec.ts:114-128`
+**Issue:** The describe-level `beforeEach` (`layout.spec.ts:32-34`) only does `page.goto('/calendar')` with no `await expect(nav).toBeVisible()` gate (unlike the calendar/lists specs which wait for the nav landmark). The new test relies entirely on `boundingBox()` auto-waiting for the toolbar button to mount after auth + CalendarShell render. This works because Playwright retries, but it's inconsistent with the explicit auth-wait pattern used in `calendar.spec.ts:79` and `lists.spec.ts:33`, and a slow desktop mount could eat into the default timeout.
+**Fix:** Add `await expect(page.getByRole('navigation', { name: 'Main navigation' })).toBeVisible();` at the top of the new test (or in a desktop-scoped beforeEach) to gate on auth before measuring, matching the established pattern.
+
+### IN-03: Desktop project lacks an explicit viewport assertion / pin
+
+**File:** `apps/pwa/playwright.config.ts:56-63`
+**Issue:** The `desktop` project relies entirely on `devices['Desktop Chrome']`'s bundled 1280×720 viewport. The spec comments hard-code "1280×720" in three files (`layout.spec.ts:13`, `calendar.spec.ts:16`, `lists.spec.ts:19`). If Playwright's `Desktop Chrome` descriptor viewport ever changes across a version bump, the comments silently lie and the ≥768px breakpoint assumptions could (in extreme cases) break with no guard. Low risk — 1280×720 is stable — but the breakpoint dependency (≥768px) is implicit.
+**Fix:** Optionally pin `viewport: { width: 1280, height: 720 }` explicitly in the desktop project `use` so the ≥768px DesktopNav assumption is self-documenting and version-stable.
+
+---
+
+_Reviewed: 2026-06-12_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_