luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Milestone v1.0: FamilySync MVP #1

Merged
luckberg merged 376 commits from gsd/v1.0-milestone into main 2026-06-10 17:39:19 -04:00
Conversation 0 Commits 376 Files Changed 338
+66 -293
1 changed files with 66 additions and 293 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e805585770 - Show all commits
.planning/phases/06-ux-polish/06-REVIEW.md
+66 -293
View File
@@ -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-<sanitized-uid>-<epochMs>` 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-<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.
**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.
---