---
phase: 03-event-write-back-pwa-install
reviewed: 2026-06-09T00:00:00Z
depth: standard
files_reviewed: 29
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/api/tests/broker/outboxWorker.test.ts
  - apps/api/tests/broker/vevent.test.ts
  - apps/api/tests/broker/write.test.ts
  - apps/api/tests/routes/events.test.ts
  - apps/pwa/index.html
  - apps/pwa/package.json
  - apps/pwa/src/api/client.test.ts
  - apps/pwa/src/api/client.ts
  - apps/pwa/src/components/CalendarShell.tsx
  - apps/pwa/src/components/DeleteConfirmationDialog.test.tsx
  - apps/pwa/src/components/DeleteConfirmationDialog.tsx
  - apps/pwa/src/components/EventDetailPopover.test.tsx
  - apps/pwa/src/components/EventDetailPopover.tsx
  - apps/pwa/src/components/EventForm.test.tsx
  - apps/pwa/src/components/EventForm.tsx
  - apps/pwa/src/components/InstallPrompt.test.tsx
  - apps/pwa/src/components/InstallPrompt.tsx
  - apps/pwa/src/components/SyncStateToast.test.tsx
  - apps/pwa/src/components/SyncStateToast.tsx
  - apps/pwa/src/store/calendarStore.ts
  - apps/pwa/vite.config.ts
  - apps/pwa/vitest.config.ts
findings:
  critical: 3
  warning: 6
  info: 5
  total: 14
status: issues_found
---

# Phase 3: Code Review Report

**Reviewed:** 2026-06-09
**Depth:** standard
**Files Reviewed:** 29
**Status:** issues_found

## Summary

The phase-3 write-back path (events router → outbox → outboxWorker → CalDAV write wrappers) and the PWA write/install UI are generally well-structured, with thorough comments documenting prior fixes (BUG A/B, CR-xx, WR-xx). However the adversarial pass surfaced a recurring class of defect the comments missed: **`calendar_events` is keyed `(calendarId, uid)`, not `uid` alone, yet several lookups query by `uid` only.** Because both household members share one Fastmail account (D-16) and each member gets their own `calendars`/`calendar_events` rows for the same collection URL, a single UID exists in MULTIPLE rows. Three query sites take an arbitrary `[0]` row from that set, producing wrong-member ownership checks, wrong etag selection, and cross-member writes. This is the same `(userId, url)` scoping bug class that schema.ts comment "BUG B" already documents for `calendars` — it was not propagated to the event-row lookups.

Additional findings: an all-day end-date inclusivity inconsistency that compounds on re-edit, a recurrence silently reset to `none` on every edit (data loss), a non-deterministic default-calendar pick, and worker cron schedules that fire on bare module import.

## Narrative Findings (AI reviewer)

## Critical Issues

### CR-01: Event edit/delete ownership check resolves an arbitrary member's row (uid-only lookup)

**File:** `apps/api/src/routes/events.ts:311-322` (edit) and `:411-422` (delete)
**Issue:** Both handlers look up the event with `.where(eq(calendarEvents.uid, uid))` and destructure `const [eventRow]`. The unique key is `(calendarId, uid)` (`schema.ts:121`), and with a shared Fastmail account (D-16) the SAME uid is cached once per member's calendar — so this query returns 2+ rows and `[0]` is whichever the DB returns first (lowest id = typically the OTHER member). Consequences:
- The ownership check `eventRow.userId !== currentUserId` can compare against the wrong member's calendar row, then fall through to the `isShared` branch and either wrongly 403 a legitimate owner or wrongly authorize against a different calendar.
- The enqueued outbox row carries `eventRow.calendarUrl / objectUrl / etag` from the arbitrary row, so the write can target the wrong member's object URL / etag.

The `GET /` handler correctly scopes by `or(eq(calendars.userId, currentUserId), eq(calendars.isShared, true))`; the write lookups do not. This is the exact bug class schema.ts "BUG B" warns about, un-propagated to the event lookups.
**Fix:** Scope the lookup to the current user's writable set and disambiguate deterministically:
```ts
const [eventRow] = await db
  .select({ /* …same cols… */ })
  .from(calendarEvents)
  .innerJoin(calendars, eq(calendarEvents.calendarId, calendars.id))
  .where(and(
    eq(calendarEvents.uid, uid),
    or(eq(calendars.userId, currentUserId), eq(calendars.isShared, true)),
  ))
  .limit(1)
```
Prefer the current user's own row over a shared/other row if both match (e.g. order so `calendars.userId = currentUserId` ranks first), so the etag/objectUrl chosen belongs to the acting member.

### CR-02: Outbox WR-02 "freshest etag" re-read also queries uid-only — can pick the wrong member's etag

**File:** `apps/api/src/broker/outboxWorker.ts:205-211`
**Issue:** Before a PUT, the worker re-reads the freshest etag with `db.select({ etag }).from(calendarEvents).where(eq(calendarEvents.uid, row.uid))` and takes `freshEtagRows[0].etag`. Same uid-collision problem as CR-01: for a shared-account uid this returns multiple rows and `[0]` may be the OTHER member's etag. Using a foreign etag in `If-Match` will either spuriously 412 (false conflict → the edit is marked `failed` with no retry, D-08, user sees the conflict toast and the edit is dropped) or, worse, match by coincidence and overwrite. The intended WR-02 behavior (avoid stale-etag 412 on rapid edits) is undermined.
**Fix:** Scope the re-read to the row's own calendar. The outbox row knows `calendarUrl` and `userId`; join through `calendars`:
```ts
const freshEtagRows = await db
  .select({ etag: calendarEvents.etag })
  .from(calendarEvents)
  .innerJoin(calendars, eq(calendarEvents.calendarId, calendars.id))
  .where(and(
    eq(calendarEvents.uid, row.uid),
    eq(calendars.userId, row.userId),
    eq(calendars.url, row.calendarUrl),
  ))
  .limit(1)
```

### CR-03: All-day end date is exclusive on write but inclusive on edit pre-fill — span grows one day per re-edit

**File:** `apps/api/src/broker/vevent.ts:83-90` vs `apps/pwa/src/components/EventForm.tsx:178-187` / `apps/api/src/broker/expand.ts`
**Issue:** `buildVeventString` advances the all-day DTEND by one calendar day to satisfy RFC-5545's exclusive-end rule (`vevent.ts:86-87`), treating the form's `end` as the inclusive last day. But on **edit**, the form pre-populates `endDate` from `occurrence.end` (`EventForm.tsx:181,187`), and `occurrence.end` for an all-day event coming back from sync/expand is the **exclusive** DTEND ('YYYY-MM-DD') that Fastmail stored. Round-tripping an edit therefore re-advances the already-exclusive end by another day on each save, silently growing multi-day all-day events by one day per edit. Even a no-op title edit corrupts the date span.
**Fix:** Make the inclusive/exclusive contract explicit and symmetric. Either (a) keep `occurrence.end` exclusive everywhere and subtract one day before pre-filling the all-day end-date input in `EventForm`, or (b) expose an inclusive end on the occurrence and convert to exclusive only at the ICS boundary. Add a regression test that edits an all-day multi-day event twice and asserts the span is stable.

## Warnings

### WR-01: Recurrence is silently reset to `none` on every edit — data loss on recurring events

**File:** `apps/pwa/src/components/EventForm.tsx:188-196`
**Issue:** `occurrence.recurrence` is not part of the `CalendarOccurrence` contract, so the edit form casts to `any`, reads `undefined`, and defaults `recurrence` to `'none'` (comment acknowledges this). Saving an edit to a recurring event then enqueues `recurrence: 'none'`, and `outboxWorker` builds a VEVENT with no RRULE — converting a weekly series into a single event on Fastmail. Any edit to a recurring event (e.g. fixing a typo) destroys the recurrence. Flagged WARNING only because v1 may not yet expose editing recurring events through this surface — confirm; otherwise promote to BLOCKER.
**Fix:** Either expose recurrence on the occurrence/expand contract and pre-fill it, or disable the recurrence `<select>` and omit `recurrence` from the update payload (so the worker preserves the existing RRULE) when editing a known-recurring event.

### WR-02: Default-calendar selection on create is non-deterministic (no ORDER BY)

**File:** `apps/api/src/routes/events.ts:260-268`
**Issue:** When `calendarUrl` is omitted, the handler picks `const [calRow] = await db.select(...).where(eq(calendars.userId, currentUserId))` with no `orderBy` and no `limit(1)`. A member with multiple personal calendars gets an arbitrary "first" calendar that can change between requests. D-01 intends a stable default. The PWA mitigates by sending `calendarUrl` when `writableCalendars.length > 1`, but the result is undefined-ordered whenever this path is reached.
**Fix:** Add deterministic order and limit: `.orderBy(calendars.id).limit(1)`, or prefer a calendar flagged as default.

### WR-03: `parseDateTime` all-day check uses the raw `iso`, not the cleaned string

**File:** `apps/pwa/src/components/EventForm.tsx:88-93`
**Issue:** `clean` strips the `[IANA]` suffix, but the all-day regex test runs against the original `iso` and the early return returns `{ date: iso }` (raw). For a true all-day 'YYYY-MM-DD' this is fine, but a date-only value carrying a bracket suffix would skip the all-day branch and fall through to `new Date(clean)`. The variable used contradicts the "Strip IANA bracket suffix" intent documented one line above.
**Fix:** Test and return `clean`: `if (/^\d{4}-\d{2}-\d{2}$/.test(clean)) return { date: clean, time: '09:00' }`.

### WR-04: Worker cron schedules start on bare module import — pollutes the test process

**File:** `apps/api/src/index.ts:63-67`
**Issue:** `startBrokerPoller()` and `startOutboxWorker()` are called at top level, so importing `./index.js` (the route tests import `app` from here) registers real `node-cron` schedules. They will fire drains/polls during the test run, touch the mocked DB/CalDAV layers nondeterministically, and keep open handles that prevent clean process exit.
**Fix:** Move worker startup inside the direct-run guard (see WR-05) or gate it behind `if (process.env.NODE_ENV !== 'test')`.

### WR-05: `index.ts` direct-run guard is fragile and can mis-fire

**File:** `apps/api/src/index.ts:79`
**Issue:** `import.meta.url.endsWith(process.argv[1].replace(/^.*\//, ''))` compares the module URL tail to the basename of argv[1]. A symlinked entrypoint or a differently-located file with the same basename can make this either fail to start the server in production or start it during an unrelated import.
**Fix:** Use a robust check, e.g. `fileURLToPath(import.meta.url) === realpathSync(process.argv[1])`.

### WR-06: Edit-as-move create-412 dead-ends the move with no retry path

**File:** `apps/api/src/broker/outboxWorker.ts:401-411` + `:361-396`
**Issue:** For edit-as-move the create runs first; on 412 it is marked `failed`, the durable gate later marks the paired delete `failed` ("original preserved"). No data is lost (original event survives), but the PWA set `lastSyncedUid` to the NEW uid (`EventForm.tsx:373`), whose only outbox row is `failed` — so the toast shows a conflict and there is no path to retry the move; the move is silently abandoned.
**Fix:** Surface that the move did not apply (distinct from a same-calendar conflict) and guide the user to re-open and re-save.

## Info

### IN-01: `triggerTargetedResync` re-loads and re-decrypts the credential per row

**File:** `apps/api/src/broker/outboxWorker.ts:108-111`
**Issue:** Each successful/conflicted row independently calls `loadClientForUser` (DB read + AES-GCM decrypt) inside the drain loop, widening the window the decrypted password is held in memory.
**Fix:** Optionally cache the client per userId within a single drain cycle.

### IN-02: Unknown-status responses retried for the full backoff window before giving up

**File:** `apps/api/src/broker/outboxWorker.ts:288-295`
**Issue:** Any unmapped non-ok status (e.g. 405, 409, 422) is classified `transient` and retried to MAX_ATTEMPTS then dead-lettered. Safe (no data loss) but slow to settle for a permanent 4xx.
**Fix:** Treat unmapped 4xx (except 408/429) as hard fail; keep transient only for 5xx/network/unknown.

### IN-03: `InstallPrompt` reads `localStorage` synchronously in `useState` initializer without try/catch

**File:** `apps/pwa/src/components/InstallPrompt.tsx:282-284`
**Issue:** Unlike `calendarStore.ts`, this access is unguarded; in private-mode/SSR contexts where `localStorage` throws it crashes the component on mount. `dismiss()` (`:298`) is likewise unguarded.
**Fix:** Wrap in try/catch returning `false`, mirroring the store's pattern.

### IN-04: `resolveUserId` typed as `any`

**File:** `apps/api/src/routes/events.ts:59`
**Issue:** The Hono context is `any` (eslint-disabled), losing type safety on `c.get('user')` and `getAuth`.
**Fix:** Type as `Context<{ Variables: { user?: { id: number } } }>`.

### IN-05: `deleteCalendarEvent` relies on tsdav ignoring `data: ''`

**File:** `apps/api/src/broker/write.ts:93`
**Issue:** Passes an empty `data` placeholder because tsdav requires the `DAVCalendarObject` shape. Relies on tsdav internals; a future version validating `data` would break this silently.
**Fix:** None required for v1; note on the tsdav upgrade checklist.

---

_Reviewed: 2026-06-09_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_