From e805585770bbb00a73f5abc895a31122002591be Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 10 Jun 2026 17:00:52 -0400 Subject: [PATCH] docs(06): refresh code review report to clean after auto-fix --- .planning/phases/06-ux-polish/06-REVIEW.md | 359 ++++----------------- 1 file changed, 66 insertions(+), 293 deletions(-) diff --git a/.planning/phases/06-ux-polish/06-REVIEW.md b/.planning/phases/06-ux-polish/06-REVIEW.md index c4022e5..827cf59 100644 --- a/.planning/phases/06-ux-polish/06-REVIEW.md +++ b/.planning/phases/06-ux-polish/06-REVIEW.md @@ -1,6 +1,6 @@ --- phase: 06-ux-polish -reviewed: 2026-06-10T00:00:00Z +reviewed: 2026-06-10T17:05:00Z depth: standard files_reviewed: 22 files_reviewed_list: @@ -25,316 +25,89 @@ files_reviewed_list: - apps/pwa/src/store/calendarStore.ts - apps/pwa/src/styles/index.css - apps/pwa/src/styles/tokens.css + - apps/pwa/src/hooks/useFocusTrap.ts findings: - critical: 1 - warning: 8 - info: 6 - total: 15 -status: issues_found + critical: 0 + warning: 0 + info: 1 + total: 1 +status: clean --- -# Phase 6: Code Review Report +# Phase 6: Code Review Report (re-review) **Reviewed:** 2026-06-10 **Depth:** standard **Files Reviewed:** 22 -**Status:** issues_found +**Status:** clean ## 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. +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. -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. +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`). -## Critical Issues +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. -### CR-01: `recurrenceUntil` is not validated as a date before being spliced into an RRULE and written to Fastmail +### Fix verification detail -**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)`. +- **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--` 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-01: Dead/misleading interface doc comment in `client.ts` CalendarOccurrence +### IN-04: `recurrenceCount` default of `1` is send-eligible the instant bound flips to "count" -**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. +**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. ---