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

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
06-ux-polish	2026-06-10T20:56:11Z	.planning/phases/06-ux-polish/06-REVIEW.md	1	15	13	2	partial

Phase 6: Code Review Fix Report

Fixed at: 2026-06-10T20:56:11Z Source review: .planning/phases/06-ux-polish/06-REVIEW.md Iteration: 1

Summary:

• Findings in scope: 15 (fix_scope=all — CR + WR + IN)
• Fixed: 13
• Skipped: 2

Two findings (WR-06, WR-08) are committed but flagged requires human verification — they change runtime behavior (a timeout race / a parse-acceptance predicate) that syntax checks cannot confirm semantically.

Verification note: the isolated worktree has no node_modules, so a full tsc --noEmit was not possible. Each edited TS/TSX file was syntax-validated with the TypeScript compiler API (ts.transpileModule, transpile-only) using the main repo's typescript@5.9.3. The ICS fixture (IN-06) was validated by parsing it through ical.js@2.2.1 and confirming the unfolded DESCRIPTION matches the original and the RRULE still parses.

Fixed Issues

CR-01: recurrenceUntil not validated as a date before RRULE splice

Files modified: apps/api/src/routes/events.ts, apps/api/src/broker/outboxWorker.ts Commit: d101aa8 Applied fix: Replaced z.string().max(10).optional() with z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional() in both eventFieldsSchema (route ingress) and outboxPayloadSchema (re-parse from stored JSON). With the regex enforced, until.replace(/-/g,'') is guaranteed digits-only, closing the ;-delimited RRULE-part injection vector. Defense-in-depth applied at both boundaries.

WR-01: All-day non-recurring events over-selected by the date-window SQL filter

Files modified: apps/api/src/routes/events.ts Commit: eb00ec7 Applied fix: Added sql\${calendarEvents.hasRrule} = 0`to the all-dayand(...)branch so a recurring all-day master whosedtstartDate` lands in the window is no longer matched twice (it is already carried by the recurring branch), eliminating duplicate on-the-wire occurrences.

WR-02: EventForm silently creates an unbounded series when "On date" is selected but blank / WR-07: fragile string comparison

Files modified: apps/pwa/src/components/EventForm.tsx Commit: 5724fe8 Applied fix: Restructured the recurrenceBound === 'until' validation: a blank recurrenceUntil now sets newErrors.recurrenceBound = 'Choose an end date' (WR-02), and the bound-before-start lexicographic compare is now guarded on a non-empty startDate so recurrenceUntil < '' can no longer silently skip the check (WR-07). Both findings live in the same conditional, so they were fixed and committed together.

WR-03: recurrenceCount number input can produce NaN/0 state

Files modified: apps/pwa/src/components/EventForm.tsx Commit: ac0f8d2 Applied fix: onChange now uses parseInt(value, 10) + Number.isFinite guard, coercing non-finite intermediates to 0. The validate guard was hardened to !Number.isInteger(recurrenceCount) || recurrenceCount < 1 so a NaN count is caught instead of bypassing both the validation gate and the payload spread.

WR-04: calendarStore uses the banned toISOString().slice(0,10) UTC-slice anti-pattern

Files modified: apps/pwa/src/lib/eventDateTime.ts, apps/pwa/src/store/calendarStore.ts Commit: 746c3c7 Applied fix: Exported the existing private localDateISO(d) helper from eventDateTime.ts and used it in initialCalendarRange() and todayIso() in place of .toISOString().slice(0,10). eventDateTime.ts is a leaf module (no imports), so no circular dependency is introduced.

WR-05: SessionExpiredError instanceof check fragile across module-reload boundaries

Files modified: apps/pwa/src/main.tsx Commit: 9f88068 Applied fix: onGlobalError now also matches (error as { name?: string })?.name === 'SessionExpiredError' so the session-expiry interstitial still arms when client.ts is loaded through two module graphs (the class carries a fixed name precisely for identity stability).

WR-06: triggerTargetedResync runs before marking a row done — a hang stalls the outbox (requires human verification)

Files modified: apps/api/src/broker/outboxWorker.ts Commit: 0511a23 Applied fix: Wrapped the success-path re-sync in Promise.race([triggerTargetedResync(...), timeout(RESYNC_TIMEOUT_MS=10s)]). On timeout the worker proceeds to mark done and lets the PWA's next poll reconcile. triggerTargetedResync already swallows its own errors, so the timed-out promise running on in the background is safe. Human verification needed: confirm 10s is the right cap, and that letting the row reach done after a timed-out re-sync (relying on the PWA refetch to reconcile) is acceptable for the deletion/edit stale-cache case the eager re-sync was originally protecting against.

WR-07: recurrenceUntil < startDate string comparison

See WR-02 above — fixed in the same commit (5724fe8).

WR-08: parseDateTime cannot distinguish all-day DATE from malformed partial dates (requires human verification)

Files modified: apps/pwa/src/components/EventForm.tsx Commit: d4a0ed7 Applied fix: Added a /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}/ prefix guard before new Date(clean) in the timed branch, so a truncated value like '2026-06' (which V8 parses as a valid UTC instant) now throws and is reported ok:false instead of silently resolving to an unintended day. Verified the existing test inputs ('2026-06-15T10:00:00-04:00', '2026-06-10T23:30:00-04:00') still match the regex and that '2026-06' / '2026-13-45' / 'garbage' are rejected. Human verification needed: confirm no legitimate cached start/end shape feeding the EDIT path lacks the T HH:MM prefix (e.g. a stored bare-second or comma-separated variant) that this would now reject and surface as a blank edit field.

IN-01: Stale CalendarOccurrence.id doc comment in client.ts

Files modified: apps/pwa/src/api/client.ts Commit: 8b79d49 Applied fix: Updated the comment from `${uid}::${dtstart_iso}` to ev-<sanitized-uid>-<epochMs> to match the server's makeOccurrenceId (expand.ts).

IN-02: resolveDefaultView indirection

Files modified: apps/pwa/src/components/CalendarShell.tsx Commit: a570135 Applied fix: Documented that the function is purely an SSR guard (returns a stable 'month-grid' when window is undefined) and that the D-05 breakpoint default actually lives in the store's readPersistedView(). Behavior preserved; the SSR guard was intentionally kept rather than inlined away.

IN-05: Duplicated focus-trap implementation across two dialogs

Files modified: apps/pwa/src/hooks/useFocusTrap.ts (new), apps/pwa/src/components/EventForm.tsx, apps/pwa/src/components/SeriesEditPrompt.tsx Commit: 1ab9710 Applied fix: Created useFocusTrap(dialogRef) hook returning the keydown handler; replaced the verbatim-duplicated handleDialogKeyDown in both components with a call to the hook. New file created because the fix explicitly requires shared extraction.

IN-06: weekly-count3.ics DESCRIPTION line exceeds 75 octets without folding

Files modified: apps/api/tests/fixtures/weekly-count3.ics Commit: 7ac4c29 Applied fix: Folded the DESCRIPTION onto a continuation line (RFC 5545 §3.1, leading-space continuation). Verified via ical.js@2.2.1 that the unfolded value exactly matches the original (single space and multibyte —/→ preserved) and the RRULE still parses to FREQ=WEEKLY;COUNT=3.

Skipped Issues

IN-03: members list in CalendarShell is always length-1

File: apps/pwa/src/components/CalendarShell.tsx:129-138 Reason: skipped — scope-confirmation question, not an actionable defect. The reviewer explicitly says "This may be intended for the current milestone... Confirm scope." Rendering the other member's color band requires sourcing the other member's identity/colour (a feature/data-flow decision, not a localized fix) and would be speculative. Flagged for a human scope decision.

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

File: apps/pwa/src/components/EventForm.tsx:216 Reason: skipped — the reviewer states "Not a bug, but a confusing default." Changing the default to empty/placeholder is a UX-design choice that interacts with the WR-03 sanitisation just landed (an empty field now coerces to 0, which validate() rejects with "Must be at least 1 occurrence"). Left as-is to avoid coupling a cosmetic default change to the validation fix; flagged for a human UX decision.

---

Fixed: 2026-06-10T20:56:11Z Fixer: Claude (gsd-code-fixer) Iteration: 1