familysync/.planning/milestones/v1.0-phases/03-event-write-back-pwa-install/03-REVIEW.iter3.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
03-event-write-back-pwa-install	2026-06-09T00:00:00Z	standard	29	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	critical warning info total 1 4 3 8	issues_found

Phase 3: Code Review Report (Re-Review, Iteration 2)

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

Summary

This is a re-review of the event write-back + PWA-install phase after a 13-item fix pass. I verified each of the previously flagged fixes the orchestrator called out:

• CR-01 / CR-02 member-scoped lookups — VERIFIED FIXED. events.ts PATCH/DELETE now scope the calendarEvents lookup to the acting member's writable set and add a deterministic ORDER BY (calendars.userId = currentUserId) DESC LIMIT 1 (events.ts:340-347, 453-460). outboxWorker.ts's fresh-etag re-read now joins calendars and filters on calendars.userId = row.userId AND calendars.url = row.calendarUrl (outboxWorker.ts:228-239), so a shared-account duplicate uid can no longer resolve to the wrong member's etag.
• CR-03 all-day inclusive/exclusive DTEND — VERIFIED FIXED and now symmetric. vevent.ts:106-118 advances the inclusive end by one UTC day on write; EventForm.tsx:86-96 exclusiveEndToInclusiveDate() rolls it back on pre-fill. The round-trip no longer grows multi-day all-day spans. vevent.test.ts:140-160 asserts DTEND = DTSTART + 1.
• WR-01 RRULE preserve-on-edit — PARTIALLY FIXED. The same-calendar update path correctly preserves the stored RRULE (outboxWorker.ts:244-248 reads rawVevent, extracts the RRULE, re-applies when the payload omits recurrence). The edit-as-move path (D-04) still silently strips recurrence — see CR-01. This is a real, demonstrable correctness regression of exactly the class WR-01 set out to prevent, so it is filed as a BLOCKER.

Other fixes (backoff index outboxWorker.ts:533-535, fail-closed credentials outboxWorker.ts:163-167, durable create-before-delete outboxWorker.ts:427-464, move-failed toast copy SyncStateToast.tsx:53-58, localStorage guards InstallPrompt.tsx:284-298) are present and correct.

Critical Issues

CR-01: Edit-as-move silently strips a recurring series' RRULE

File: apps/api/src/broker/outboxWorker.ts:267-297, apps/api/src/routes/events.ts:371-401

Issue: WR-01 was fixed only for the same-calendar update branch. When a recurring event is edited and moved to a different calendar, the PATCH handler (events.ts:371-398) enqueues a delete of the old object plus a create with a brand-new newUid and the edit payload. The edit payload omits recurrence by design (EventForm.tsx:323; the recurrence picker is disabled in edit mode). The worker's create branch then builds the VEVENT with:

rruleString: fields.recurrence && fields.recurrence !== 'none'
  ? RRULE_PRESETS[fields.recurrence as string]
  : undefined,   // ← recurrence absent → undefined → no RRULE

Unlike the update branch, the create branch performs no rawVevent read and no extractRruleString fallback. The original event's RRULE lives in calendar_events under the OLD uid/calendar; the create uses newUid and never reads it. Net effect: moving any recurring event to another calendar converts the whole series into a single one-off occurrence on Fastmail — silent data loss — and the original series is deleted once the paired delete runs. This is the identical failure mode WR-01 was meant to eliminate, on a different code path.

Fix: Carry the existing RRULE through the move. Two viable approaches:

1. In events.ts, have the edit lookup also select rawVevent, extract the RRULE, and stash it on the create outbox row so the worker re-applies it:

// events.ts — add rawVevent to the eventRow select, then in the move branch:
const preservedRrule = extractRruleString(eventRow.rawVevent ?? '');
await tx.insert(calendarOutbox).values({
  /* ...create row... */
  payload: JSON.stringify({ ...payload, _preservedRrule: preservedRrule }),
  groupId,
});

…and in the worker create branch, fall back to fields._preservedRrule when recurrence is absent.

2. Or, in the worker create branch, when the row has a groupId (move) and the payload lacks recurrence, look up the RRULE from the sibling delete row's original uid/calendar via calendarEvents.rawVevent and feed it to buildVeventString, mirroring outboxWorker.ts:244-248.

Add a regression test: move a recurring event → assert the created ICS contains RRULE:.

Warnings

WR-01: Edit form cannot edit recurrence and provides no way to remove an RRULE

File: apps/pwa/src/components/EventForm.tsx:311-327, 715-742

Issue: The recurrence <select> is hard-disabled in edit mode and the payload always omits recurrence on edit. Combined with server-side preservation, a user can never (a) change a recurring event's frequency, nor (b) intentionally make a recurring event non-recurring — the worker treats "no recurrence field" as "keep the existing RRULE," so there is no way to express "remove the RRULE." For v1 this is an accepted scope cut (documented in comments), but it is a silent usability trap: a user who opens a weekly event, changes the title, and saves gets no indication the schedule is locked. The disabled control has opacity: 0.6 and no explanatory text.

Fix: Acceptable to defer full edit-recurrence, but surface the constraint: when eventFormMode === 'edit', render helper text near the disabled select (e.g. "Repeat can't be changed yet — edits keep the existing schedule"). Additive copy only; no logic change.

WR-02: handleAllDayToggle can leave end-date inconsistent with the discarded time inputs

File: apps/pwa/src/components/EventForm.tsx:259-271, 287-289

Issue: validate() for all-day uses strict endDate < startDate. handleAllDayToggle only advances endDate to startDate when toggling all-day ON and endDate < startDate. When a timed event spans midnight (start 2026-06-10 23:00, end 2026-06-11 01:00) and the user toggles all-day ON, the time inputs are discarded but endDate is left at 06-11, producing a 2-day all-day event the user likely did not intend; conversely, toggle paths that leave endDate === startDate validate as a 1-day event silently. Not data loss, but the toggle can change the event span without a clear signal.

Fix: On toggle-on, clamp endDate to max(startDate, endDate) deterministically and clear time errors. Add a test covering toggle-on across a midnight-spanning timed event.

WR-03: Missing cached etag becomes an unconditional PUT/DELETE, defeating D-08 conflict detection

File: apps/api/src/routes/events.ts:385, 411, 486; apps/api/src/broker/write.ts:62-75, 85-97

Issue: When eventRow.etag is null (event cached before an etag was captured, or Fastmail omitted it), the outbox row's etag is undefined, and write.ts maps null/'' to "no If-Match header" — an unconditional PUT/DELETE. That defeats conflict detection for exactly the rows most likely to be stale: a concurrent external edit is silently overwritten with no 412. Only triggers when the cached etag is missing, so Warning rather than Blocker.

Fix: Make the no-etag policy explicit. Safer: when no etag is available, fetch the current etag (REPORT/GET) before writing, or skip the write and force a re-sync. At minimum, log a warning when an update/delete dispatches with an empty If-Match so the unconditional-write path is observable.

WR-04: sync-status reports only the newest outbox row per uid, masking an earlier failure

File: apps/api/src/routes/events.ts:511-532; apps/pwa/src/components/SyncStateToast.tsx:39-70

Issue: sync-status selects ORDER BY createdAt DESC LIMIT 1 for (userId, uid). For rapid successive same-uid edits (two update rows enqueued before the worker drains), the toast reports only the newest row's status. If the newest succeeds but an older row dead-letters, the user sees "Saved" while a queued write silently failed. Window is small (single-process 15s drain) but real under burst edits.

Fix: Prefer a non-terminal/failed/dead row over a done row when reporting status for a uid (order so pending/failed/dead outranks done), or report failed/dead if ANY row for the uid is in that state. Add a test with two update rows where the older is dead.

Info

IN-01: RRULE_PRESETS round-trip is lossy for any parameterized RRULE

File: apps/api/src/broker/vevent.ts:39-44, 53-67; apps/api/src/broker/outboxWorker.ts:208-211

Issue: RRULE_PRESETS maps only to bare FREQ=DAILY|WEEKLY|MONTHLY|YEARLY. extractRruleString returns the full stored RECUR (which may include BYDAY, INTERVAL, COUNT, UNTIL). The preserve path keeps the rich rule (good), but if a recurrence value is ever set on a previously-rich rule, it collapses to the bare preset — dropping BYDAY/UNTIL. Acceptable for v1 (picker offers only the four bare presets and is disabled on edit), but a latent foot-gun once recurrence editing ships.

Fix: Document the v1 limitation at the RRULE_PRESETS definition; when recurrence editing lands, modify the parsed RECUR rather than replacing it with a preset.

IN-02: parseDateTime silently rewrites a malformed edit value to today/09:00

File: apps/pwa/src/components/EventForm.tsx:107-131

Issue: On an unparseable occurrence start/end the form falls back to todayIso()/09:00 with no user signal. In edit mode a corrupt cached value silently rewrites the event to today at 09:00 if the user saves without noticing. Low probability (the API produces well-formed ISO), but a silent data-changing default in an edit form is worth a guard.

Fix: In edit mode, on parse failure, leave the field blank and block submit rather than substituting today/09:00.

IN-03: Unchecked as casts on JSON-parsed outbox payload fields

File: apps/api/src/broker/outboxWorker.ts:251-258, 285-293

Issue: fields.title as string, fields.allDay as boolean, fields.start as string, etc. are unchecked casts on a Record<string, unknown> parsed from stored JSON. The payload is zod-validated at enqueue, so low-risk, but schema drift or a manually-inserted row would pass undefined/wrong types into buildVeventString, producing SUMMARY:undefined or an Invalid Date.

Fix: Re-validate the parsed payload with eventFieldsSchema (or a worker-local zod schema) before building the VEVENT, and hard-fail the row on validation error (it can never succeed). Cheap insurance against enqueue→drain schema drift.

---

Reviewed: 2026-06-09 Reviewer: Claude (gsd-code-reviewer) Depth: standard