Lucas BergerandClaude Opus 4.8
c7955a46b9
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
95 lines
8.8 KiB
Markdown
95 lines
8.8 KiB
Markdown
---
|
||
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_
|