docs(03): add code review report
This commit is contained in:
Lucas Berger
@@ -0,0 +1,209 @@
|
||||
---
|
||||
phase: 03-event-write-back-pwa-install
|
||||
reviewed: 2026-06-05T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 18
|
||||
files_reviewed_list:
|
||||
- apps/api/src/broker/outboxWorker.ts
|
||||
- apps/api/src/broker/sync.ts
|
||||
- apps/api/src/broker/vevent.ts
|
||||
- apps/api/src/broker/write.ts
|
||||
- apps/api/src/db/schema.ts
|
||||
- apps/api/src/index.ts
|
||||
- apps/api/src/routes/events.ts
|
||||
- apps/pwa/index.html
|
||||
- apps/pwa/src/api/client.ts
|
||||
- apps/pwa/src/components/CalendarShell.tsx
|
||||
- apps/pwa/src/components/DeleteConfirmationDialog.tsx
|
||||
- apps/pwa/src/components/EventDetailPopover.tsx
|
||||
- apps/pwa/src/components/EventForm.tsx
|
||||
- apps/pwa/src/components/InstallPrompt.tsx
|
||||
- apps/pwa/src/components/SyncStateToast.tsx
|
||||
- apps/pwa/src/store/calendarStore.ts
|
||||
- apps/pwa/vite.config.ts
|
||||
- apps/pwa/package.json
|
||||
findings:
|
||||
critical: 6
|
||||
warning: 8
|
||||
info: 5
|
||||
total: 19
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 3: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-05
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 18
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
This phase is the CalDAV write-back + outbox + PWA-install slice, about to undergo its first live Gate 2 write test. The review found that **the write path is fundamentally broken end-to-end** and will fail at multiple independent points before any event ever reaches Fastmail:
|
||||
|
||||
1. The PWA sends `title/start/end`, but the server's zod schema requires `summary/dtstart/dtend` — **every create and update is rejected with 400 at the route boundary.** The write path cannot succeed today.
|
||||
2. Even if a row reaches the outbox, the worker PUTs the **raw JSON form payload** to Fastmail as if it were an iCalendar string. `buildVeventString` (the entire `vevent.ts` file, the D-13 DATE/DATETIME contract) is **never called** — it is dead code. Fastmail will reject the body or store a corrupt object.
|
||||
3. The edit-as-move create row and the `update` row reuse the form-fields JSON as `payload`, so recurrence, all-day handling, and field mapping are all lost regardless.
|
||||
|
||||
Because of (1) and (2), the core acceptance criterion of this phase (write an event to Fastmail) cannot pass. These must be fixed before Gate 2.
|
||||
|
||||
Secondary but serious: a production fallback in `dispatchRow` can silently authenticate with empty credentials on a transient DB error; the backoff schedule skips its first delay; and several outbox/ordering edge cases can drop or double-apply writes across drain batches.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: API contract mismatch — every create/update rejected with 400
|
||||
|
||||
**File:** `apps/api/src/routes/events.ts:68-77`, `apps/pwa/src/api/client.ts:119-128`, `apps/pwa/src/components/EventForm.tsx:247-256`
|
||||
**Issue:** The server `eventFieldsSchema` requires `summary`, `dtstart`, `dtend`. The client `CreateEventPayload` and `EventForm.handleSubmit` send `title`, `start`, `end`. `@hono/zod-validator` rejects the body before the handler runs, so `POST /api/events/create` and `PATCH /api/events/:uid/edit` return 400 for every well-formed client request. The write path is dead on arrival. (`recurrence` is also `required` on the client type but `.optional()` on the server — a lesser instance of the same drift.)
|
||||
**Fix:** Make the two ends agree on one field contract. Either rename the client payload to `summary/dtstart/dtend`, or accept `title/start/end` server-side and map internally:
|
||||
```ts
|
||||
const eventFieldsSchema = z.object({
|
||||
title: z.string().min(1).max(255),
|
||||
allDay: z.boolean(),
|
||||
start: z.string().min(1).max(64),
|
||||
end: z.string().min(1).max(64),
|
||||
location: z.string().max(2000).optional(),
|
||||
description: z.string().max(2000).optional(),
|
||||
recurrence: z.enum(['none','daily','weekly','monthly','yearly']).optional(),
|
||||
calendarUrl: z.string().url().max(1024).optional(),
|
||||
})
|
||||
```
|
||||
Add a contract test that round-trips the exact `CreateEventPayload` shape through the schema.
|
||||
|
||||
### CR-02: Outbox worker PUTs raw form JSON to Fastmail — `buildVeventString` is dead code
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:168-187`, `apps/api/src/routes/events.ts:250`, `apps/api/src/broker/vevent.ts` (entire file)
|
||||
**Issue:** The route stores `payload: JSON.stringify(payload)` — the raw form fields. The worker passes `row.payload` directly as the `iCalString`/`data` to `createCalendarEvent`/`updateCalendarEvent`. `buildVeventString` is never imported or called anywhere in the codebase (`grep` confirms zero call sites outside its own file). The body sent to Fastmail is therefore `{"title":"...","start":"..."}` — not a VCALENDAR. Fastmail will reject it (or, worse, store a corrupt object). The entire D-13 DATE-vs-DATETIME contract, RRULE serialization, escaping, and DTSTAMP logic in `vevent.ts` is bypassed.
|
||||
**Fix:** The worker must parse the stored form JSON and build the ICS before PUT:
|
||||
```ts
|
||||
// in dispatchRow, for create/update:
|
||||
const fields = JSON.parse(row.payload) as NewEventParams-equivalent
|
||||
const { icsString } = buildVeventString({
|
||||
uid: row.uid,
|
||||
summary: fields.title,
|
||||
allDay: fields.allDay,
|
||||
dtstart: fields.allDay ? fields.start : new Date(fields.start),
|
||||
dtend: fields.allDay ? fields.end : new Date(fields.end),
|
||||
location: fields.location,
|
||||
description: fields.description,
|
||||
rruleString: fields.recurrence && fields.recurrence !== 'none'
|
||||
? RRULE_PRESETS[fields.recurrence] : undefined,
|
||||
})
|
||||
response = await createCalendarEvent(client, davCalendar, row.uid, icsString)
|
||||
```
|
||||
Wrap `JSON.parse` in try/catch and treat a parse failure as a hard fail (no retry). Add an integration test asserting the PUT body begins with `BEGIN:VCALENDAR`.
|
||||
|
||||
### CR-03: Production fallback authenticates to Fastmail with empty credentials
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:135-143`
|
||||
**Issue:** `dispatchRow` wraps `loadClientForUser` in a `try/catch` and, on **any** throw, falls back to `createFastmailClient('', '')`. The comment claims this branch is "never taken" in production, but `loadClientForUser` can throw in production for real reasons: a transient DB error on the `memberCredentials` select, a decryption failure (`decryptPassword` throws on a tampered/rotated key), or a missing credential row. When that happens in production, the worker proceeds to issue a real PUT/DELETE to `caldav.fastmail.com` with empty Basic-auth credentials. At best this is a 401 (correctly classified hard-fail, marking the row `failed` and dropping the write permanently — no retry); at worst it masks a recoverable transient DB error as a permanent failure. Either way a write is silently lost on a condition that should have been retried.
|
||||
**Fix:** Remove the production fallback. Let `loadClientForUser` failures propagate to the per-row `catch` in `runOutboxDrain` (line 343), which logs and leaves the row `pending` for the next drain — the correct transient behavior. Gate the empty-credential client strictly behind a test-only flag, never on a generic `catch`:
|
||||
```ts
|
||||
const client = await loadClientForUser(row.userId) // let it throw → outer catch retries
|
||||
```
|
||||
|
||||
### CR-04: 412 conflict on a `create` move-pair does not block the paired `delete` reliably across batches
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:247-281`, `292-296`
|
||||
**Issue:** `failedCreateGroups` is a `Set` local to a single `runOutboxDrain` call. The create-before-delete guarantee only holds when both rows are fetched in the **same** drain batch. The fetch is capped at "up to 10" pending rows (and ordered only by the default DB order, not by `groupId`/`createdAt`). If a move pair straddles a batch boundary — or the create is retried into a later cycle (transient) while the delete is already eligible — the delete can run in a cycle where the create is absent from `sorted`, so `failedCreateGroups` is empty and the delete proceeds. Result: the original event is deleted before the new copy is confirmed created — exactly the "lost event" D-04 is meant to prevent. The in-memory set cannot enforce a cross-batch ordering invariant.
|
||||
**Fix:** Make the dependency durable. Options: (a) do not enqueue the `delete` row as `pending`; enqueue it `blocked` and have the worker flip it to `pending` only after the paired create row reaches `done`; or (b) when dispatching a `delete` with a `groupId`, query the DB for the paired create row's status and skip/defer unless it is `done`. Do not rely on both rows co-occurring in one in-memory batch.
|
||||
|
||||
### CR-05: No drain concurrency guard — overlapping cycles double-dispatch the same row
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:247-259`, `359-365`
|
||||
**Issue:** The scheduler fires `runOutboxDrain` every 15s. A drain that issues several network PUTs plus targeted re-syncs (each `triggerTargetedResync` does a full `fetchCalendars` + `syncCalendar`) can easily exceed 15s. The selection query reads `status='pending'` but nothing marks a row "in-flight" before dispatch, and the row is only updated to `done`/`failed` **after** the network call returns. Two overlapping cycles will both select the same still-`pending` row and both issue the write. For a `create` (PUT with `If-None-Match: *`) the second attempt may 412 and get marked `failed`, masking a successful first write; for a `delete` the second DELETE re-runs the If-Match against a now-changed etag and can mis-classify. This is a double-apply / lost-confirmation hazard.
|
||||
**Fix:** Add a concurrency guard. Simplest: a module-level `isDraining` boolean that the scheduler checks and skips if a drain is still running. More robust: atomically claim rows by updating `status='pending' → status='processing'` (with a `WHERE status='pending'` guard and `LIMIT`) inside a transaction before dispatch, and only the claiming cycle processes them.
|
||||
|
||||
### CR-06: OIDC write path is a hard 401 stub — authenticated users cannot write in production
|
||||
|
||||
**File:** `apps/api/src/routes/events.ts:191-201`, `268-274`, `372-378`, `437-443`, `490-496`
|
||||
**Issue:** Every write/status/writable-calendars handler resolves the user via `resolveUserId`, which only returns the dev-bypass `c.get('user')`. When that is null (the real OIDC production path), the code calls `getAuth(c)` and then **unconditionally returns 401** even when `auth` is truthy ("For now return 401 if OIDC auth is not backed by a DB user here."). In production (`devBypassActive=false`), `resolveUserId` is always null, so authenticated Authelia users get 401 on every write and on `sync-status`/`writable-calendars`. The phase is described as about to undergo its first live external-auth write test (Gate 2) — this path is a stub that cannot pass. (The OIDC guard mounting in `index.ts` is correct; the issue is the unimplemented iss/sub → user-row lookup.)
|
||||
**Fix:** Implement the OIDC→user resolution: from `getAuth(c)` extract `iss`+`sub`, look up the `users` row by the `uniq_oidc_identity` key, and use that `id` as `currentUserId`. Return 401 only when no session exists; return 403/422 (not 401) when a valid session has no provisioned user row.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Backoff schedule skips its first (15s) delay; off-by-one on dead-letter count
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:38-44, 316-341`
|
||||
**Issue:** `attemptCount` starts at 0. On the first transient failure `nextAttemptCount = 1`, and the delay is read as `BACKOFF_SECONDS[1]` = 60s — so `BACKOFF_SECONDS[0]` (15s) is never used; the documented "15+60+300+600+1800 ≈ 30 min" window is actually 60+300+600+1800. Also `MAX_ATTEMPTS=5` with `nextAttemptCount >= MAX_ATTEMPTS` dead-letters after the 5th attempt's increment reaches 5 — the comment "~30 min" and the indexing should be reconciled.
|
||||
**Fix:** Index by `row.attemptCount` (the attempt that just failed) rather than `nextAttemptCount`: `const backoffMs = (BACKOFF_SECONDS[row.attemptCount] ?? 1800) * 1000`. Add a unit test asserting the exact delay sequence.
|
||||
|
||||
### WR-02: Edit (same-calendar update) reuses old etag but a successful prior update changes it — guaranteed 412 on second edit
|
||||
|
||||
**File:** `apps/api/src/routes/events.ts:348-357`, `apps/api/src/broker/outboxWorker.ts:297-303`
|
||||
**Issue:** On a successful update the worker marks the row `done` and triggers a re-sync, which updates `calendarEvents.etag`. That is correct. But the route reads `eventRow.etag` from the cache at enqueue time. If a user edits twice in quick succession (or the poller has not yet refreshed the etag), the second update row carries a stale etag and will 412 even though the user is the only editor. The conflict toast then fires spuriously. This is an interaction between optimistic-accept latency and If-Match.
|
||||
**Fix:** Accept that rapid successive edits to the same object should coalesce or chain: either collapse pending update rows for the same uid before enqueueing, or re-read the freshest etag in the worker just before PUT rather than trusting the enqueue-time snapshot.
|
||||
|
||||
### WR-03: Stale `occurrence` snapshot in EventForm — edits open with empty/old fields
|
||||
|
||||
**File:** `apps/pwa/src/components/EventForm.tsx:113-181`
|
||||
**Issue:** `occurrence` is resolved once via an IIFE during render from the TanStack cache. The initial `useState` values capture it, but the reset `useEffect` depends on `[eventFormOpen, eventFormMode, eventFormUid]` — not on `occurrence`. If the form opens (in edit mode) before the `['events']` query has the occurrence cached, `occurrence` is null at mount and the effect never re-runs when the data later arrives, so the form stays blank. `recurrence` is also hard-reset to `'none'` on every open, so editing a recurring event silently drops its recurrence.
|
||||
**Fix:** Include `occurrence` (or `occurrence?.uid`) in the reset effect deps, and derive the initial `recurrence` from the occurrence instead of always `'none'`. Guard against opening edit mode before the cache is populated.
|
||||
|
||||
### WR-04: All-day end-date is exclusive in iCalendar but UI treats it as inclusive
|
||||
|
||||
**File:** `apps/pwa/src/components/EventForm.tsx:215-238, 250-251`, `apps/api/src/broker/vevent.ts:75-88`
|
||||
**Issue:** For all-day events the form sends `end = endDate` and validates `endDate < startDate` as the only error (so a single-day event has `start === end`). iCalendar DTEND for a DATE value is **exclusive** — a one-day all-day event must have DTEND = start + 1 day. As written, a single-day all-day event would produce DTSTART=DTEND, which is invalid/zero-length per RFC 5545. (Currently moot because CR-02 means no VEVENT is built at all, but it must be fixed alongside CR-02.)
|
||||
**Fix:** When building the all-day VEVENT, add one day to the end DATE (or normalize in the form). Add a test for the single-day all-day case.
|
||||
|
||||
### WR-05: `parseDateTime` uses local-time getters on a UTC-parsed Date — wrong time in edit form
|
||||
|
||||
**File:** `apps/pwa/src/components/EventForm.tsx:84-101`
|
||||
**Issue:** For timed events `new Date(clean)` parses the offset-aware ISO into an instant, then `d.toISOString().slice(0,10)` takes the **UTC** date while `d.getHours()/getMinutes()` take the **local** time. Mixing UTC date with local clock components can yield a date/time pair that is off by a day at the edges, and the time shown will be the viewer's local wall-clock rather than the event's original zone. For a family spanning Toronto/Edmonton zones this misrepresents the edited event.
|
||||
**Fix:** Derive both date and time consistently in one zone (use the same Temporal-based conversion the calendar render path uses, or compute local date with `getFullYear/getMonth/getDate`).
|
||||
|
||||
### WR-06: `triggerTargetedResync` calls `fetchCalendars` on every successful row — N+1 round-trips and re-load of credentials
|
||||
|
||||
**File:** `apps/api/src/broker/outboxWorker.ts:89-117, 297-303`
|
||||
**Issue:** Each successful/conflicted row triggers a full `loadClientForUser` (DB select + decrypt) plus `fetchCalendars()` (network) plus `syncCalendar` (REPORT of the whole collection). A drain of 10 rows for one user does this 10 times against the same calendar. Beyond load, this re-decrypts the app password 10x and multiplies the window during which overlapping drains (CR-05) can interfere. Out of strict v1 perf scope, but it is also a correctness amplifier for the concurrency and etag-staleness issues above.
|
||||
**Fix:** Batch re-syncs: collect the distinct `(userId, calendarUrl)` pairs touched during a drain and re-sync each once at the end of the cycle.
|
||||
|
||||
### WR-07: `EventForm` and dialogs implement no real focus trap despite claiming one
|
||||
|
||||
**File:** `apps/pwa/src/components/EventForm.tsx:274-278`, `DeleteConfirmationDialog.tsx:42-46`, `EventDetailPopover.tsx:157-161`
|
||||
**Issue:** The docblocks state "Focus trap while open," but the implementation only calls `.focus()` once on open. Tab can move focus out of the modal to background content (the calendar grid, FAB). For a modal `aria-modal="true"` dialog this is an accessibility defect and, combined with the always-mounted backdrop, lets keyboard users interact with obscured controls.
|
||||
**Fix:** Implement an actual focus trap (cycle Tab/Shift+Tab within the dialog) or use a vetted primitive. At minimum, document honestly that it is focus-on-open only.
|
||||
|
||||
### WR-08: `crypto.randomUUID()` used without importing `crypto` in the route module
|
||||
|
||||
**File:** `apps/api/src/routes/events.ts:241, 317, 318`
|
||||
**Issue:** The route relies on a global `crypto.randomUUID()`. This is available on Node 20+/22 globals, so it likely works at runtime, but there is no `import { randomUUID } from 'crypto'` and no explicit reference to `globalThis.crypto` — it depends entirely on the ambient global being present and typed. `vevent.ts` imports `randomUUID` from `'crypto'` explicitly; the inconsistency is a latent footgun if the runtime or tsconfig `lib` changes.
|
||||
**Fix:** Import explicitly (`import { randomUUID } from 'node:crypto'`) and use `randomUUID()` for consistency with `vevent.ts`.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `vevent.ts` is entirely unreachable dead code
|
||||
|
||||
**File:** `apps/api/src/broker/vevent.ts:1-119`
|
||||
**Issue:** As established in CR-02, nothing imports `buildVeventString` or `RRULE_PRESETS`. Once CR-02 is fixed this becomes live; until then the whole file (and its D-13 logic) is untested dead weight that gives false confidence the contract is honored.
|
||||
**Fix:** Wire it in per CR-02; add a unit test so it cannot silently fall out of the call graph again.
|
||||
|
||||
### IN-02: `resolveDefaultView` ignores its own SSR guard return value
|
||||
|
||||
**File:** `apps/pwa/src/components/CalendarShell.tsx:63-66`
|
||||
**Issue:** `resolveDefaultView` returns `'month-grid'` when `window === undefined` else `persistedView` — it never uses a phone/desktop branch, so the JSDoc ("phone defaults to month-agenda") is misleading; the actual default already comes from the Zustand store. The helper is effectively an identity function on the client.
|
||||
**Fix:** Remove the redundant helper or align the comment with what it does.
|
||||
|
||||
### IN-03: `getDefaultStartDate`/`getDefaultEndDate` are identical
|
||||
|
||||
**File:** `apps/pwa/src/components/EventForm.tsx:47-53`
|
||||
**Issue:** Both return today's date; the naming implies different defaults (the time defaults differ, but those are hardcoded separately at the call sites as `'09:00'`/`'10:00'`). Two functions with identical bodies invite drift.
|
||||
**Fix:** Collapse to a single `todayIso()` helper (one already exists in `calendarStore.ts`).
|
||||
|
||||
### IN-04: `manifest.json`/icons referenced but `apple-touch-icon.png` and PWA icons not verified in scope
|
||||
|
||||
**File:** `apps/pwa/index.html:7`, `apps/pwa/vite.config.ts:33-37`
|
||||
**Issue:** The manifest references `/icon-192.png`, `/icon-512.png`, and `index.html` references `/apple-touch-icon.png`. These assets are outside the reviewed file set; if missing, the iOS Add-to-Home-Screen flow (the other half of this phase) will install with a broken icon. Flagging for Gate 2 verification, not a code defect in the reviewed files.
|
||||
**Fix:** Confirm the icon assets exist in `public/` before the install test.
|
||||
|
||||
### IN-05: Outbox `done`/`failed`/`dead` rows are never pruned
|
||||
|
||||
**File:** `apps/api/src/db/schema.ts:125-154`, `apps/api/src/broker/outboxWorker.ts`
|
||||
**Issue:** Terminal rows accumulate indefinitely. `sync-status` reads the latest row per uid (ordered by `createdAt desc`), so correctness is preserved, but the table grows unbounded and the `idx_outbox_next_attempt` scan includes ever-more terminal rows over time (the `WHERE status='pending'` filters them, but only after index narrowing). Low urgency for a two-user household.
|
||||
**Fix:** Add a periodic prune of `done` rows older than N days; keep `failed`/`dead` for audit or prune separately.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-05_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user