luckberg/familysync

Fork 0

Files

T

Lucas Berger 4303a1b680 docs(07): code-review --fix complete — 5 warnings fixed, re-review status clean

2026-06-11 08:00:55 -04:00

12 KiB

Raw Blame History

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase

reviewed

depth

files_reviewed

files_reviewed_list

findings

status

07-mobile-test-harness

2026-06-11T12:30:00Z

deep

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

critical	critical_resolved	blocker	blocker_resolved	warning	warning_resolved	info	info_bydesign	total
0	1	0	2	0	7	0	5	0

clean

Phase 7: Code Review Report (DEEP) — Iteration 2 (--auto re-review)

Reviewed: 2026-06-11 Depth: deep (cross-file call-chain analysis + live-stack verification) Files Reviewed: 9 (harness) Status: clean — zero open actionable findings

Summary

This is the iteration-2 re-review after the fixer applied 5 changes (commits c564fc6 WR-07, 9c38dd3 WR-01+WR-02, 5322cfc WR-06, 2b745ad WR-05). The prior pass had resolved CR-01, BL-01, and BL-02; those resolution records are preserved below.

Verification performed this pass:

Ran the full suite against the live dev stack (MariaDB :3306, API :3000 DEV_AUTH_BYPASS=true, Vite auto-started by webServer): 58 passed (55s).
Typecheck (tsc --noEmit + tsc --project tsconfig.e2e.json --noEmit): exit 0.
Probed navigator.serviceWorker availability on both engines to confirm the WR-07 fix is non-vacuous (see WR-07 below).
Probed redirect:'manual' response semantics to confirm the WR-01 gate distinguishes a dev-bypass 200 from an Authelia redirect.

Result: all 7 prior warnings are resolved by the fixes (5 actionable + WR-03/WR-04 by-design), no fix introduced a regression or new defect, and no new cross-file issue was exposed. Setting status: clean. The 5 IN-* items remain advisory/by-design and are listed under "Resolved / By-design"; none are actionable.

Critical Issues (resolved — 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 — re-verified sound this pass.

globalSetup TRUNCATEs list_items, list_shares, lists, calendar_events against whatever DB_* points at. The fix throws before opening any DB connection:

NODE_ENV === 'production' → throw (checked first).
DEV_AUTH_BYPASS !== 'true' → throw.

This mirrors the API guard (apps/api/src/auth/devBypass.ts) ordering exactly and is coupled to the same switch that makes the API serve Dev User 1 without OIDC (index.ts:24-25, 51-55). Residual scope note carried as IN-05 (guard protects production, not "the wrong dev DB" — by design).

Blocker Findings (resolved — record preserved)

BL-01 (RESOLVED in commit `53c3ca5`): calendar populated-state assertions were vacuous

File: apps/pwa/e2e/calendar.spec.ts Status: RESOLVED — re-verified non-vacuous this pass.

The dead-EmptyState / always-rendered-wrapper assertions were replaced with a real DB→UI proof: getByText('Seeded Test Event').first() must be visible in the grid (calendar.spec.ts:90-97). Verified live: passes on both iphone (WebKit) and pixel (Chromium). With /api/events mocked empty the title is absent, so the assertion genuinely tracks the seed flowing DB → API → query → grid. The wrapper-visibility test (:80-88) was kept but its docstring now correctly states it only proves the grid mounts, not that the seed reached the UI.

BL-02 (RESOLVED in commit `53c3ca5`): seed↔view month-boundary fragility

File: apps/pwa/e2e/global-setup.ts:119-154 Status: RESOLVED — re-verified deterministic this pass.

The seed event is re-anchored to noon-today (UTC) (global-setup.ts:127-129) — always today's local calendar date, always inside the current-month view both phone-width profiles render. The prior now+24h could roll into the next month on a month's last day, making any "seeded event is visible" assertion date-fragile. The seed shape (all_day=false, dtstart_utc set, recurring flags false) matches the API's non-recurring-timed WHERE branch. Verified live on both engines.

Resolved this iteration (fixer commits — verified, no regression)

WR-01 (RESOLVED in `9c38dd3`): `/api/me` dev-bypass reachability gate

File: apps/pwa/e2e/global-setup.ts:75-90

The gate now probes fetch(${baseURL}/api/me, { redirect: 'manual' }) after the /health poll and throws unless res.ok. Verified correct end-to-end:

Dev-bypass-reachable API → 200. me.ts:30-42 short-circuits on c.get('user') (DEV_USER) with no DB round-trip, so the gate passes regardless of seed state and regardless of ordering (the probe runs before the seed — confirmed safe because /api/me has no DB dependency under bypass). The full suite passed with this gate live.
Authelia-redirecting API → fails loudly. With redirect:'manual', a cross-origin 302 to Authelia surfaces as type:'opaqueredirect', status:0, ok:false → gate throws. A same-origin redirect (e.g. c.redirect('/')) surfaces as type:'basic', status:302, ok:false → also throws. Confirmed empirically against /api/login (302, ok=false).
No false-fail in the supported setup: in the dev-bypass stack the OIDC middleware is not mounted (index.ts:51), so /api/me always returns 200. No regression.

The error message string contains opaqueredirect with no space — cosmetic only (it is the exact Response.type token undici emits); not actionable.

WR-02 (RESOLVED in `9c38dd3`): explicit readiness flag

File: apps/pwa/e2e/global-setup.ts:54-73

The loop now uses an explicit let ready = false set inside the res.ok branch, and the post-loop check is if (!ready) throw — success is no longer inferred from Date.now() >= deadline. This removes both the false-positive-timeout (a success arriving in the final second can no longer be misreported as a timeout) and any false-positive-ready (the flag is only set on an actual res.ok). Timeout logic verified correct by reading; the gate ran green in the live suite.

WR-05 (RESOLVED in `2b745ad`): dropped redundant `unroute` calls

Files: apps/pwa/e2e/calendar.spec.ts:133-135, 154, 176; apps/pwa/e2e/lists.spec.ts:90-92, 118

The trailing page.unroute(...) calls were removed and replaced with comments explaining that per-test context isolation handles cleanup. Verified this is correct, not a leak risk:

Every page.route(...) is registered inside an individual test body, never in a shared beforeEach/beforeAll. Playwright assigns each test a fresh page/BrowserContext, and route handlers are scoped to that page/context — they cannot leak into sibling tests.
The suite runs under fullyParallel: true with no describe.serial, so there is no shared-page path that could carry a route forward.
Cross-test isolation confirmed empirically: the populated-state calendar/lists tests (no mock) and the error/empty-state tests (with mock) all pass in the same run with no interference.

The removed unroute calls were genuinely dead — they never ran when an expect threw (the whole point of those tests), so they had guaranteed nothing. Dropping them is strictly an improvement.

WR-06 (RESOLVED in `5322cfc`): self-validation comment corrected

File: apps/pwa/e2e/layout.spec.ts:209-211, 250-252

The misleading "remove by reload" comments now read "REMOVE the injected style by deleting the <style> element via evaluate (styleHandle.evaluate(el => el.remove()) — no page reload)", which matches the actual code (styleHandle.evaluate((el) => (el as Element).remove())). Comment matches code. Trivial, confirmed.

WR-07 (RESOLVED in `c564fc6`): SW-block test is now non-vacuous and honestly skips

File: apps/pwa/e2e/calendar.spec.ts:41-68

The test now (a) computes swAvailable = 'serviceWorker' in navigator, (b) test.skip(!swAvailable, ...) when absent, and (c) otherwise asserts getRegistration() resolves to undefined. Verified all three concerns live:

(a) Not vacuous on Chromium/pixel — AND not vacuous on WebKit/iphone either. I probed both engines directly: swAvailable=true and getRegistration()=undefined on both iphone (WebKit) and pixel (Chromium) over http://localhost. So the genuine assertion runs on both profiles in this environment — getRegistration() is available and returns undefined under serviceWorkers:'block'. The SW test shows ✓ passed (not skipped) on iphone, confirming the real assertion executed rather than being silently skipped.
(b) test.skip is honest. It is a real test.skip(condition, reason) that, when serviceWorker is genuinely absent (e.g. a future WebKit/runner where http://localhost is not a secure context), marks the test skipped/visible in the reporter — it does not let an unavailable API masquerade as a pass. In the current stack the skip branch is never taken, so it is correct dead-fallback, not a silent pass.
(c) getRegistration() is the right probe under serviceWorkers:'block'. With the block in effect no registration is ever created, so the promise resolves to undefined; if the block were lifted and the app registered sw.js, this would become a ServiceWorkerRegistration and the toBeUndefined() assertion would fail. This is a real, regression-sensitive signal (unlike the old controller === null, which was null on any first uncontrolled load regardless of the block).

No regression. The fix strictly strengthens the assertion.

Resolved / By-design (advisory — NOT actionable)

These were never code defects; they are design notes carried for traceability. None block shipping.

WR-03 (by-design): webServer manages Vite only; the API/DB/Redis are compose-managed per D-10. Playwright considers the server ready when Vite answers, before globalSetup polls /health; a missing API is deferred to the /health gate (now also the /api/me gate, WR-01). This is the intended D-09 contract. Documentation-coupling only.
WR-04 (by-design): page.route('/api/lists') exact-match is correct — fetchLists() requests the bare path with no query string, and the narrow matcher intentionally avoids swallowing /api/lists/:id/items. A glob would be brittle. No change.
IN-01 (advisory): mysql2@3.22.4 is a PWA devDependency used only by the seed; correct placement (never bundled). Note: pinned independently from apps/api's copy — keep in lockstep.
IN-02 (advisory): tsconfig.e2e.json types:["node"] + lib:["DOM",...] correctly types the Node seed while still typing page.evaluate DOM callbacks. @playwright/test types come via direct import. Sound.
IN-03 (advisory): vitest exclude:['e2e/**'] and Playwright testDir:'./e2e' cleanly partition the two runners. Sound.
IN-04 (advisory): typecheck covers both tsconfigs (re-verified exit 0 this pass). Good.
IN-05 (advisory): the CR-01 guard protects production, not "the wrong dev DB" — pointing DB_* at a populated dev DB with DEV_AUTH_BYPASS=true will still TRUNCATE it. By design (D-06 deterministic reseed) and documented. A defense-in-depth E2E_ALLOW_TRUNCATE/DB-name-pattern opt-in remains an optional hardening, not a defect.

Live-run evidence (iteration 2)

Check	Result
Full suite (both profiles)	58 passed (55.0s)
`iphone` SW-block test	✓ passed (real assertion ran; not skipped)
`pixel` SW-block test	✓ passed
Seeded-event DB→UI proof (iphone + pixel)	✓ passed both
`swAvailable` probe (both engines)	`true` / `getRegistration()=undefined`
`redirect:'manual'` on a 302	`ok=false` (gate throws — correct)
`tsc --noEmit` + e2e tsconfig	exit 0

Reviewed: 2026-06-11T12:30:00Z Reviewer: Claude (gsd-code-reviewer) Depth: deep (iteration 2 — --auto re-review)

12 KiB Raw Blame History

Phase 7: Code Review Report (DEEP) — Iteration 2 (--auto re-review)

Summary

Critical Issues (resolved — record preserved)

CR-01 (RESOLVED in commit fcc680e): global-setup TRUNCATE had no fail-closed guard

Blocker Findings (resolved — record preserved)

BL-01 (RESOLVED in commit 53c3ca5): calendar populated-state assertions were vacuous

BL-02 (RESOLVED in commit 53c3ca5): seed↔view month-boundary fragility