familysync/.planning/milestones/v1.0-phases/06-ux-polish/06-REVIEW.iter2.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-10T00:00: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	critical warning info total 1 8 6 15	issues_found

Phase 6: Code Review Report

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

Summary

Reviewed the Phase 6 UX-polish source set: server-side recurrence expansion, the outbox worker write-path, the events route, the PWA event form / auth-splash / push-prompt components, the calendar store, and supporting tests + CSS.

The code is heavily commented and carries a clear audit trail of prior fixes. The adversarial pass focused on the gaps between those documented fixes. The one Critical finding is a security-relevant injection vector in the RRULE UNTIL assembly (the route validates the date-window query params and write-body lengths, but recurrenceUntil is NOT validated as a date before being spliced into an RRULE string and PUT to Fastmail). The remaining findings are correctness/robustness gaps: a search-query SQL filter that over-returns all-day events, an unbounded-recurrence silent fallthrough in the form, a NaN-able count input, a fragile instanceof session-error check across module-reload boundaries, and the calendar store using the exact UTC-slice anti-pattern the rest of the codebase explicitly bans.

Critical Issues

CR-01: recurrenceUntil is not validated as a date before being spliced into an RRULE and written to Fastmail

File: apps/api/src/routes/events.ts:111, apps/api/src/broker/outboxWorker.ts:124-132 Issue: The route validates recurrenceUntil only as z.string().max(10).optional() — any ≤10-char string passes. The outbox worker then does:

const dateDigits = until.replace(/-/g, '');
s += `;UNTIL=${dateDigits}`; // all-day
s += `;UNTIL=${dateDigits}T235959Z`; // timed

until.replace(/-/g,'') strips hyphens but leaves every other character. A payload of recurrenceUntil: "A;FREQ=DA" (10 chars, no hyphens) yields ;UNTIL=A;FREQ=DA — i.e. an injected extra RRULE part. The worker comment claims "The fixed ;UNTIL= template prevents injection of extra ;-delimited RRULE parts" and "ICAL.Recur.fromString rejects malformed values" — but the value itself can contain ;, and ICAL.Recur.fromString is lenient about unknown parts. Even in the benign case, a non-date string like "notadate" produces ;UNTIL=notadate, which either silently corrupts the series bound or throws deep in buildVeventString (burning the outbox attempt budget) rather than being rejected at the boundary.

This is the same class the route's own header comment claims to defend against (T-02b / T-03-08 input validation). recurrenceCount is correctly bounded (z.number().int().min(1)); recurrenceUntil is not.

Fix: Validate the format at the zod boundary in both eventFieldsSchema (events.ts) and outboxPayloadSchema (outboxWorker.ts):

recurrenceUntil: z.string().regex(/^\d{4}-\d{2}-\d{2}$/).optional(),

The existing windowed-GET schema already uses exactly this regex (events.ts:88-89) — reuse it. With the regex in place the .replace(/-/g,'') output is guaranteed digits-only and the injection vector closes.

Warnings

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

File: apps/api/src/routes/events.ts:199-203 Issue: The third or() branch selects any row with dtstartDate in [start, end) without gating on hasRrule = 0:

and(
  sql`${calendarEvents.dtstartDate} IS NOT NULL`,
  sql`${calendarEvents.dtstartDate} >= ${start}`,
  sql`${calendarEvents.dtstartDate} < ${end}`,
);

The timed branch above it explicitly gates hasRrule = 0, but this all-day branch does not. Any recurring all-day master whose dtstartDate happens to fall inside the window is matched twice (once by the recurring branch at :184, once here). Because both branches feed the same flatMap(expandOccurrences), the same master is expanded twice and every occurrence is duplicated in the response. expandOccurrences builds a stable makeOccurrenceId, so Schedule-X dedups on render — but the duplication is real on the wire and any consumer that counts occurrences (or a future view that does not dedup) sees doubles. Add the missing hasRrule = 0 gate to the all-day branch.

Fix:

and(
  sql`${calendarEvents.hasRrule} = 0`,
  sql`${calendarEvents.dtstartDate} IS NOT NULL`,
  sql`${calendarEvents.dtstartDate} >= ${start}`,
  sql`${calendarEvents.dtstartDate} < ${end}`,
);

WR-02: EventForm silently creates an unbounded series when "On date" is selected but no date entered

File: apps/pwa/src/components/EventForm.tsx:355-359, 399-401 Issue: When recurrenceBound === 'until', validate() only flags an error when recurrenceUntil is truthy AND before the start:

} else if (recurrenceBound === 'until' && recurrenceUntil) {
  if (recurrenceUntil < startDate) { ... }
}

If the user picks "On date" but leaves the date blank, validation passes. The payload builder then omits recurrenceUntil (the spread is guarded by ... && recurrenceBound === 'until' && recurrenceUntil), so the event is created as an unbounded recurring series — the opposite of the user's stated intent ("Ends: On date"). Treat a blank recurrenceUntil while bound === 'until' as a validation error.

Fix: Add to the recurrence !== 'none' block:

if (recurrenceBound === 'until' && !recurrenceUntil) {
  newErrors.recurrenceBound = 'Choose an end date';
}

WR-03: recurrenceCount number input can produce NaN / 0 state and an empty-string-driven 0

File: apps/pwa/src/components/EventForm.tsx:932 Issue: onChange={(e) => setRecurrenceCount(Number(e.target.value))}. Clearing the field yields e.target.value === '' → Number('') === 0; certain intermediate inputs ("-", "e") yield NaN. NaN < 1 is false, so the count-validation guard (recurrenceCount < 1) does NOT fire for NaN, and the payload spread (recurrenceCount >= 1 → NaN >= 1 is false) silently drops the count, again yielding an unbounded series. The 0 case is caught by validation, but the NaN case bypasses both the validation gate and the payload gate.

Fix: Sanitize on change and validate explicitly:

onChange={(e) => {
  const n = parseInt(e.target.value, 10)
  setRecurrenceCount(Number.isFinite(n) ? n : 0)
}}
// and in validate():
if (recurrenceBound === 'count' && (!Number.isInteger(recurrenceCount) || recurrenceCount < 1)) {
  newErrors.recurrenceBound = 'Must be at least 1 occurrence'
}

WR-04: calendarStore uses toISOString().slice(0,10) — the exact UTC-slice anti-pattern the codebase bans

File: apps/pwa/src/store/calendarStore.ts:130-131, 137 Issue: initialCalendarRange() and todayIso() both build date strings with .toISOString().slice(0, 10). eventDateTime.ts:64-65 and EventForm.tsx:108-110 explicitly document this as forbidden ("NEVER use toISOString().slice(0,10) — that returns the UTC date, not the local date"). For a user west of UTC (the project's primary zones are Toronto/Detroit/NewYork/Edmonton — all negative offsets) after ~20:00 local, todayIso() returns _tomorrow's date. This is the default selectedDate and seeds the initial fetch window — so a late-evening cold load can center the calendar on the wrong day and the EventForm create default (todayIso() at EventForm.tsx:153/158) pre-fills tomorrow. The fix already exists as the private localDateISO helper in eventDateTime.ts; export and reuse it.

Fix: Export localDateISO from eventDateTime.ts and use it in initialCalendarRange()/todayIso():

function localDateISO(d: Date): string {
  return `${d.getFullYear()}-${pad2(d.getMonth()+1)}-${pad2(d.getDate())}`
}

WR-05: SessionExpiredError instanceof check is fragile across the test's dynamic re-imports / module duplication

File: apps/pwa/src/main.tsx:30-34, apps/pwa/src/api/client.ts:33-39 Issue: The global error handler routes on error instanceof SessionExpiredError. The class is defined in client.ts and re-imported in main.tsx. This works in the prod bundle (single module instance), but it is a known footgun: if client.ts is ever loaded through two module graphs (Vite SSR, a duplicated chunk, or — as the tests already do — repeated await import('./client.js')), instanceof fails and the session-expiry interstitial never arms, leaving the user on a hung query. The class uses a fixed readonly name = 'SessionExpiredError' precisely to be identity-stable; the handler should defensively also check name.

Fix:

function onGlobalError(error: unknown): void {
  if (error instanceof SessionExpiredError ||
      (error as { name?: string })?.name === 'SessionExpiredError') {
    useCalendarStore.getState().setSessionExpired(true)
  }
}

WR-06: triggerTargetedResync runs before marking a row done, so a sync hang stalls the outbox cycle and the optimistic toast

File: apps/api/src/broker/outboxWorker.ts:683-694 Issue: On success the worker awaits triggerTargetedResync(...) BEFORE writing status='done'. The comment justifies this (avoid the PWA refetch racing stale cache). But triggerTargetedResync performs client.fetchCalendars() + syncCalendar() — unbounded network I/O against Fastmail with no timeout. If that hangs or is slow, the row stays pending from the DB's perspective for the full duration, the 15s isDraining guard keeps the next cycle a no-op, and the PWA polls sync-status seeing pending indefinitely. A single slow re-sync therefore blocks the entire single-process outbox. Consider bounding the re-sync with a timeout, or marking done and accepting the documented race (the PWA already re-polls). At minimum, the unbounded-I/O-before-commit tradeoff should be a deliberate, time-boxed decision rather than open-ended.

Fix: Wrap the resync in a timeout (e.g. Promise.race with a 10s cap) so a stalled Fastmail connection cannot wedge the drain loop; on timeout, proceed to mark done and let the next poll reconcile.

WR-07: recurrenceUntil < startDate string comparison is only valid for same-format DATE strings

File: apps/pwa/src/components/EventForm.tsx:356 Issue: if (recurrenceUntil < startDate) compares two strings lexicographically. recurrenceUntil comes from a type="date" input (YYYY-MM-DD) and startDate is also YYYY-MM-DD, so this works today. But it is silently coupled to both values always being zero-padded ISO dates. If startDate is ever blank (the IN-02 edit parse-failure path sets it to ''), recurrenceUntil < '' is always false, so the bound-before-start guard is skipped exactly when the start is unknown. Low impact (create-mode only shows the bound control, and create-mode start is never blank), but the implicit format coupling is fragile. Compare parsed dates or assert non-empty startDate first.

Fix: Guard on non-empty operands or compare via Date/Temporal.PlainDate.

WR-08: parseDateTime swallows all errors and cannot distinguish "all-day DATE" from "malformed" in some inputs

File: apps/pwa/src/components/EventForm.tsx:112-139 Issue: new Date(clean) for a string like '2026-13-45' returns an Invalid Date, caught and returned as ok:false — correct. But new Date('2026-06') (a partial date) is parsed as a valid UTC instant in V8, so a truncated/garbled cached value would parse "successfully" to an unintended day/time and be saved on edit without tripping the IN-02 blank-field guard. The function trusts new Date()'s permissive parsing. Tighten the accepted timed-format (e.g. require a T and : before calling new Date) so only genuinely well-formed ISO datetimes parse as ok:true.

Fix: Pre-validate the timed branch shape, e.g. if (!/T\d{2}:\d{2}/.test(clean)) return { ...today, ok: false } before new Date(clean).

Info

IN-01: Dead/misleading interface doc comment in client.ts CalendarOccurrence

File: apps/pwa/src/api/client.ts:107 Issue: The id field comment says `${uid}::${dtstart_iso}` — stable identity but the server (expand.ts:101-104 makeOccurrenceId) now emits ev-<sanitized-uid>-<epochMs>. The ::-format comment is stale and contradicts the actual wire contract (and the server-side comment that explains why :: was abandoned). Update the comment to the ev-… form to avoid misleading future readers.

IN-02: resolveDefaultView ignores its only branch's intent

File: apps/pwa/src/components/CalendarShell.tsx:65-68 Issue: resolveDefaultView(persistedView) returns 'month-grid' for SSR and otherwise returns persistedView verbatim — the function adds nothing over reading selectedView directly, and the D-05 phone/desktop default logic it appears to promise actually lives in the store's readPersistedView(). Harmless, but the indirection invites a future reader to expect breakpoint logic here that isn't present. Inline it or move the default resolution here for real.

IN-03: members list in CalendarShell is always length-1 (only the current user)

File: apps/pwa/src/components/CalendarShell.tsx:129-138 Issue: members is built solely from meQuery.data.user, so ColorLegend and buildCalendarConfig only ever see the current member. Given MEMORY notes the app is designed to be "member-count-agnostic" and to render the other member's calendar as a read-only overlay, a single-member legend will mislabel/omit the other member's color band. This may be intended for the current milestone, but it contradicts the multi-member intent and the ColorLegend's plural framing. Confirm scope.

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

File: apps/pwa/src/components/EventForm.tsx:216 Issue: recurrenceCount defaults to 1. If a user selects "After N times" and submits without touching the field, a 1-occurrence "recurring" event is created (effectively non-recurring). Not a bug, but a confusing default; consider an empty initial value with a placeholder (the placeholder "e.g. 10" already implies blank).

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

File: apps/pwa/src/components/EventForm.tsx:445-472, apps/pwa/src/components/SeriesEditPrompt.tsx:59-84 Issue: The Tab/Shift+Tab focus-trap handleDialogKeyDown is copy-pasted verbatim into both components. Extract to a shared hook (useFocusTrap(ref)) so a future fix (e.g. handling disabled/hidden elements, or radio-group focus) lands in one place.

IN-06: weekly-count3.ics fixture DESCRIPTION exceeds the typical 75-octet ICS line without folding

File: apps/api/tests/fixtures/weekly-count3.ics:10 Issue: The DESCRIPTION: line is a single long unfolded line. ICAL.parse tolerates it, so the test passes, but a hand-authored fixture that violates RFC 5545 line-folding can mask folding-related regressions. Cosmetic; fold the line if the fixture is meant to mirror real Fastmail output.

---

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