diff --git a/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md index 25603f8..062530f 100644 --- a/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md +++ b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md @@ -2,7 +2,7 @@ phase: 03-event-write-back-pwa-install reviewed: 2026-06-09T00:00:00Z depth: standard -files_reviewed: 28 +files_reviewed: 29 files_reviewed_list: - apps/api/src/broker/outboxWorker.ts - apps/api/src/broker/sync.ts @@ -36,134 +36,146 @@ files_reviewed_list: findings: critical: 3 warning: 6 - info: 4 - total: 13 + info: 5 + total: 14 status: issues_found --- # Phase 3: Code Review Report -**Reviewed:** 2026-06-09T00:00:00Z +**Reviewed:** 2026-06-09 **Depth:** standard -**Files Reviewed:** 28 +**Files Reviewed:** 29 **Status:** issues_found ## Summary -Reviewed the event write-back path (outbox worker, VEVENT builder, CalDAV write wrappers, events router) plus the PWA write UI (EventForm, EventDetailPopover, DeleteConfirmationDialog, SyncStateToast, InstallPrompt) and supporting config. +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. -The code is heavily annotated with prior fix references (CR-xx, WR-xx, BUG x) and the obvious surface defects have been addressed. However, tracing the all-day round-trip and the shared-Fastmail-account model (D-16) surfaces three correctness defects that ship incorrect data or pick the wrong member's row. The most serious is a cumulative one-day drift on every edit of an all-day event, caused by an exclusive-DTEND value being re-advanced each write cycle. +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: All-day event grows by one day on every edit (cumulative DTEND drift) +### CR-01: Event edit/delete ownership check resolves an arbitrary member's row (uid-only lookup) -**File:** `apps/api/src/broker/vevent.ts:83-90`, `apps/pwa/src/components/EventForm.tsx:144-152,278-287` -**Issue:** `buildVeventString` unconditionally advances the all-day `dtend` by +1 calendar day to convert an inclusive user end into RFC-5545's exclusive DTEND. That is correct for a *fresh create* where the form supplies an inclusive end. It is wrong on *edit*, because the value fed back into the form is already the exclusive DTEND. +**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. -Trace the round-trip for a single-day all-day event: -1. Create "Birthday" 2026-06-15. Form sends `start='2026-06-15', end='2026-06-15'`. `buildVeventString` writes `DTSTART:20260615`, `DTEND:20260616` (exclusive). Correct. -2. `expandOccurrences` (`apps/api/src/broker/expand.ts:240`) serializes the occurrence as `start='2026-06-15', end='2026-06-16'` — it returns the raw exclusive DTEND. -3. User opens the edit form. `parseDateTime(occurrence.end)` (`EventForm.tsx:145`) yields `endDate='2026-06-16'`. User changes nothing and saves; the form sends `end='2026-06-16'`. -4. `buildVeventString` advances it again to `DTEND:20260617`. +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. -Every subsequent edit adds another day. This is silent data corruption of the user's calendar on Fastmail. The same exclusive/inclusive mismatch also means a freshly-created single-day all-day event, when re-opened in the edit form before any change, already displays an end date one day later than the user entered. +### CR-02: Outbox WR-02 "freshest etag" re-read also queries uid-only — can pick the wrong member's etag -**Fix:** Make the inclusive→exclusive conversion idempotent across the round trip. Either (a) have the edit form convert the cached exclusive end back to an inclusive end before populating `endDate` (subtract one day for all-day events when initializing the form), or (b) move the +1-day exclusive-DTEND conversion out of `buildVeventString` and have the form always emit an exclusive end on both create and edit. Pick one boundary as the owner of the convention and apply it consistently. Add a round-trip test: create all-day → expand → edit (no change) → assert DTEND unchanged. - -### CR-02: `update` etag re-read selects an arbitrary member's row on a shared Fastmail account - -**File:** `apps/api/src/broker/outboxWorker.ts:204-211` -**Issue:** Before a PUT, the worker re-reads the freshest 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) - .where(eq(calendarEvents.uid, row.uid)) + .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) ``` -The lookup is keyed on `uid` alone. Under the shared-Fastmail-account model documented throughout this phase (D-16, `schema.ts:78-84`, `sync.ts:60-65`, `poller.ts:50-55`), the *same* VEVENT UID is cached once per member — two `calendar_events` rows with identical `uid` but different `calendarId`/etag. There is no `orderBy` and no `.limit(1)`, so `freshEtagRows[0]` is whichever row the DB returns first — potentially the *other* member's etag. Sending another member's etag as `If-Match` produces a spurious 412 conflict, which the worker marks `failed` (no retry) and surfaces the "this event changed elsewhere" toast for a write that never actually conflicted. -The whole point of BUG B (scoping calendar lookups by `(userId, url)`) is undermined here because the etag re-read drops back to a uid-only predicate. The same risk exists for the edit/delete enqueue lookups in `events.ts:311-322` and `:411-422`, which also match `calendarEvents.uid` without scoping by the resolved calendar/user and take the first row. +### CR-03: All-day end date is exclusive on write but inclusive on edit pre-fill — span grows one day per re-edit -**Fix:** Scope the etag re-read to the same calendar the outbox row targets. Join `calendar_events → calendars` and filter on `calendars.url = row.calendarUrl AND calendars.userId = row.userId` (or carry `calendarId` on the outbox row and filter on it). Apply the same scoping to the PATCH/DELETE handler lookups. - -### CR-03: Cached events for the other member's mirror row are never pruned after a write - -**File:** `apps/api/src/broker/sync.ts:141-154`, `apps/api/src/broker/outboxWorker.ts:407,419` -**Issue:** After a successful write or a 412, `triggerTargetedResync` re-syncs only the writing member's calendar row (it loads *that* member's credential, matches `davCal.url`, then `syncCalendar` prunes scoped to `cal.id`). Because each member has a separate `calendars` row for the same shared collection URL (D-16), a delete performed by member A removes the event from A's cached rows but leaves member B's mirror row in `calendar_events` until B's 5-minute poller runs. `GET /api/events` for member B (`events.ts:163-198`) selects from shared/owned calendars and keeps returning the deleted event as a live occurrence — the "ghost event that won't delete" failure this phase set out to fix, reintroduced for the *non-acting* member. - -For a two-person household: A deletes a shared event, B continues to see it (and can act on it) for up to 5 minutes with no live correction. For a delete this is a correctness/data-integrity gap, not merely staleness. - -**Fix:** On a successful shared-calendar write, re-sync every member's `calendars` row mapping to the same collection URL (iterate `calendars WHERE url = row.calendarUrl`), or key the event cache by `(url, uid)` rather than `(calendarId, uid)` so one prune covers both members. If Phase 4 SSE live-sync is intended to close this, document it explicitly — as written, delete propagation to the other member is bounded only by the poller. +**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: `create` default-calendar selection is non-deterministic and may target the shared calendar - -**File:** `apps/api/src/routes/events.ts:259-268` -**Issue:** When no `calendarUrl` is supplied, the handler picks `calendars WHERE userId = currentUserId` with no `orderBy` and no `isShared` filter, then takes the first row. The comment says "first personal calendar", but nothing restricts the result to personal calendars, and without ordering the chosen calendar can vary between requests. A user creating an event with the picker hidden (single-writable case) could have it land on an unintended collection. -**Fix:** `.where(and(eq(calendars.userId, currentUserId), eq(calendars.isShared, false))).orderBy(calendars.id).limit(1)` if "personal" is the intended default. - -### WR-02: PATCH edit-as-move does not authorize the *destination* calendar - -**File:** `apps/api/src/routes/events.ts:341-371` -**Issue:** On a calendar move, ownership is asserted only against the *source* event's calendar. `newCalendarUrl` comes straight from `payload.calendarUrl` and is enqueued as the create target with no check that the destination is owned-or-shared by `currentUserId`. POST /create performs this destination check (`:242-256`); the edit-move path does not. A client can move an event onto a calendar URL it is not authorized to write — the worker then PUTs to it with the requester's credentials. This violates the D-03 writable-set contract the route claims to enforce (T-03-06/T-03-11). -**Fix:** Before enqueuing the move, look up `newCalendarUrl` and assert `userId = currentUserId OR isShared = true`, mirroring POST /create. Return 403 otherwise. - -### WR-03: `triggerTargetedResync` swallows all errors, so a re-sync failure leaves stale cache while marking the row `done` - -**File:** `apps/api/src/broker/outboxWorker.ts:108-136,412-423` -**Issue:** The success path deliberately re-syncs before marking `done` so the cache is fresh when the toast invalidates `['events']`. But `triggerTargetedResync` catches and logs *all* errors and returns normally. If the re-sync fails (network blip, credential decrypt error in that window), the row is still marked `done`, the toast flips to "Saved", invalidates, and the refetch returns the *stale* pre-write cache — exactly the race the ordering was meant to prevent, now silent. Cache and UI disagree until the next poller cycle. -**Fix:** Distinguish "write succeeded but local re-sync failed" from full success: leave the row `pending` (so the next drain retries the resync) or mark `done` without letting the toast assert freshness. At minimum log at error level with the row id and trigger an immediate sync retry. - -### WR-04: SyncStateToast cannot surface a failed delete on an edit-as-move - -**File:** `apps/pwa/src/components/SyncStateToast.tsx:39-65`, `apps/api/src/routes/events.ts:373` -**Issue:** `sync-status` resolves a uid to the single most-recent outbox row for that member. On an edit-as-move the API returns the *new* uid; the delete row carries the *old* uid. The toast tracks only the new uid, so if the create succeeds (`done`, auto-dismiss "Saved") but the paired delete later fails, the user gets no signal — the original event remains on the source calendar, producing a silent duplicate. (The reverse, create-fails-delete-skipped, is handled by the worker preserving the original; this inverse is not surfaced.) -**Fix:** Report an aggregate status for the move `groupId`, or return enough from the edit-move response for the toast to watch both rows. - -### WR-05: Unknown HTTP status (incl. 404/410) is retried five times then dead-lettered - -**File:** `apps/api/src/broker/outboxWorker.ts:288-295` -**Issue:** Any status not in the transient/hard-fail/conflict sets is classified transient, retried with backoff, then dead-lettered with copy "Not saved. Check your connection." A 404/410 on an update/delete means the object is already gone — five wasted retries and a terminal `dead` state that misdescribes the cause. For a delete, 404/410 is success-equivalent. -**Fix:** Add explicit 404/410 handling: for delete treat as success (already gone); for update treat as conflict/needs-resync. Keep the transient default only for genuinely unknown codes. - -### WR-06: Edit silently drops recurrence on a recurring event +### 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:** As documented in-line, `occurrence.recurrence` is not part of the `CalendarOccurrence` contract, so editing a recurring event defaults the recurrence picker to `'none'`. Saving an edit then omits the RRULE from the payload, downgrading a recurring series to a single event on Fastmail. This is data-affecting edit behavior, not just a display gap. -**Fix:** Until the occurrence/expand contract carries recurrence, disable the recurrence control in edit mode (or warn the user) rather than defaulting to `'none'` and silently dropping the rule on save. +**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 `