16 KiB
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 |
|
|
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/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():
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