familysync/.planning/milestones/v1.0-phases/06-ux-polish/06-REVIEW.md

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

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
06-ux-polish	2026-06-10T17:05:00Z	standard	22	apps/api/src/broker/expand.ts apps/api/src/broker/outboxWorker.ts apps/api/src/routes/events.ts apps/api/tests/broker/expand.test.ts apps/api/tests/broker/outboxWorker.test.ts apps/api/tests/broker/vevent.test.ts apps/api/tests/fixtures/weekly-count3.ics apps/pwa/src/api/client.test.ts apps/pwa/src/api/client.ts apps/pwa/src/components/AuthSplash.tsx apps/pwa/src/components/CalendarShell.tsx apps/pwa/src/components/EventForm.test.tsx apps/pwa/src/components/EventForm.tsx apps/pwa/src/components/PushPermissionPrompt.tsx apps/pwa/src/components/SeriesEditPrompt.tsx apps/pwa/src/lib/eventDateTime.test.ts apps/pwa/src/lib/eventDateTime.ts apps/pwa/src/main.tsx apps/pwa/src/store/calendarStore.ts apps/pwa/src/styles/index.css apps/pwa/src/styles/tokens.css apps/pwa/src/hooks/useFocusTrap.ts	critical warning info total 0 0 1 1	clean

Phase 6: Code Review Report (re-review)

Reviewed: 2026-06-10 Depth: standard Files Reviewed: 22 Status: clean

Summary

This is the second adversarial pass over the Phase 6 UX-polish source set, focused on verifying the 12-commit fix batch (CR-01, WR-01–WR-08, IN-01/02/05/06) landed correctly and did not introduce regressions. IN-03 (single-member ColorLegend) and IN-04 (recurrenceCount default of 1) were intentionally deferred as scope questions and are out of remediation scope.

Verification result: all previously-flagged fixes are correctly applied, covered by tests, and introduce no regressions. The full Phase 6 test surface is green — 49 API broker tests (expand/outboxWorker/vevent) and 95 PWA tests (client/EventForm/eventDateTime).

No Critical or Warning findings on this pass. One Info note is carried forward unchanged (IN-04, the deferred count-default UX), recorded here only so it is not lost.

Fix verification detail

• CR-01 (RRULE UNTIL injection): Closed. recurrenceUntil is now z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional() at BOTH boundaries — eventFieldsSchema (events.ts:114) on ingress and outboxPayloadSchema (outboxWorker.ts:86) on re-parse. until.replace(/-/g,'') is now guaranteed digits-only, and recurrenceCount keeps its int().min(1) bound. The edit-as-move _preservedRrule carry-through (route → .passthrough() schema → create branch) is the server's own stored RECUR, not attacker-controlled, and is unit-tested (outboxWorker.test.ts:311, 337). No new vector introduced.
• WR-01 (all-day SQL over-select): Fixed. The all-day non-recurring branch now gates hasRrule = 0 (events.ts:207), so a recurring all-day master in the window is carried only by the recurring branch — no double expansion.
• WR-02 (blank "On date" → unbounded series): Fixed. validate() now errors on recurrenceBound === 'until' && !recurrenceUntil (EventForm.tsx:375).
• WR-03 (NaN/0 recurrenceCount): Fixed. onChange coerces via parseInt + Number.isFinite → 0 (EventForm.tsx:929-930), and validate() requires Number.isInteger(recurrenceCount) && >= 1 (EventForm.tsx:364).
• WR-04 (UTC-slice anti-pattern): Fixed. calendarStore imports and uses the exported localDateISO for both initialCalendarRange() and todayIso() (calendarStore.ts:27, 134-135, 142); no toISOString().slice(0,10) remains.
• WR-05 (fragile instanceof): Fixed. onGlobalError now also matches (error)?.name === 'SessionExpiredError' (main.tsx:35-38).
• WR-06 (unbounded resync before commit): Fixed. Success path wraps triggerTargetedResync in Promise.race with a 10s cap (outboxWorker.ts:711-714); the detached resync swallows its own errors and does not touch the isDraining guard, so no post-finally shared-state hazard.
• WR-07 (string compare with blank start): Fixed. The bound-before-start compare is now guarded on a non-empty startDate (EventForm.tsx:377).
• WR-08 (permissive new Date parse): Fixed. parseDateTime requires a ^\d{4}-\d{2}-\d{2}T\d{2}:\d{2} shape on clean before trusting new Date (EventForm.tsx:129); a partial value like 2026-06 now returns ok:false.
• IN-01 (stale id doc comment): Fixed. client.ts:107 now documents the ev-<sanitized-uid>-<epochMs> form matching expand.ts makeOccurrenceId.
• IN-02 (resolveDefaultView indirection): Addressed via clarifying doc comment (CalendarShell.tsx:61-68) documenting the SSR-only intent.
• IN-05 (duplicated focus trap): Fixed. Both EventForm and SeriesEditPrompt now consume the shared useFocusTrap hook (hooks/useFocusTrap.ts).
• IN-06 (unfolded ICS DESCRIPTION): Fixed. The fixture DESCRIPTION is now folded across two lines with a leading-space continuation (weekly-count3.ics:10-11).

Info

IN-04: recurrenceCount default of 1 is send-eligible the instant bound flips to "count"

File: apps/pwa/src/components/EventForm.tsx:225 Issue: Carried forward unchanged and explicitly deferred as a scope question. recurrenceCount still defaults to 1, so selecting "After N times" and submitting without touching the field creates a 1-occurrence "recurring" event (effectively non-recurring). Not a bug; a confusing default. If addressed later, prefer an empty initial value with the existing "e.g. 10" placeholder. No action required this pass.

---

Reviewed: 2026-06-10 Reviewer: Claude (gsd-code-reviewer) Depth: standard