From 43650bb65e9f67ca6a555ff9ec4b7f1c6e0dd3fb Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 10 Jun 2026 16:35:09 -0400 Subject: [PATCH] docs(06): add code review report --- .planning/phases/06-ux-polish/06-REVIEW.md | 343 +++++++++++++++++++++ 1 file changed, 343 insertions(+) create mode 100644 .planning/phases/06-ux-polish/06-REVIEW.md diff --git a/.planning/phases/06-ux-polish/06-REVIEW.md b/.planning/phases/06-ux-polish/06-REVIEW.md new file mode 100644 index 0000000..c4022e5 --- /dev/null +++ b/.planning/phases/06-ux-polish/06-REVIEW.md @@ -0,0 +1,343 @@ +--- +phase: 06-ux-polish +reviewed: 2026-06-10T00:00:00Z +depth: standard +files_reviewed: 22 +files_reviewed_list: + - 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 +findings: + critical: 1 + warning: 8 + info: 6 + total: 15 +status: 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: + +```js +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): + +```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`: + +```js +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:** +```js +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: + +```js +} 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: +```js +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: +```js +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/New_York/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()`: +```js +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:** +```js +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--`. 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_