14 KiB
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 | 28 |
|
|
issues_found |
Phase 3: Code Review Report
Reviewed: 2026-06-09T00:00:00Z Depth: standard Files Reviewed: 28 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 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.
Narrative Findings (AI reviewer)
Critical Issues
CR-01: All-day event grows by one day on every edit (cumulative DTEND drift)
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.
Trace the round-trip for a single-day all-day event:
- Create "Birthday" 2026-06-15. Form sends
start='2026-06-15', end='2026-06-15'.buildVeventStringwritesDTSTART:20260615,DTEND:20260616(exclusive). Correct. expandOccurrences(apps/api/src/broker/expand.ts:240) serializes the occurrence asstart='2026-06-15', end='2026-06-16'— it returns the raw exclusive DTEND.- User opens the edit form.
parseDateTime(occurrence.end)(EventForm.tsx:145) yieldsendDate='2026-06-16'. User changes nothing and saves; the form sendsend='2026-06-16'. buildVeventStringadvances it again toDTEND:20260617.
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.
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:
const freshEtagRows = await db
.select({ etag: calendarEvents.etag })
.from(calendarEvents)
.where(eq(calendarEvents.uid, row.uid))
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.
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.
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
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.
Info
IN-01: resolveUserId parameter typed any, defeating type safety at the auth boundary
File: apps/api/src/routes/events.ts:59
Issue: async function resolveUserId(c: any) uses any with an eslint-disable; every call site loses Hono context typing. The helper only needs c.get and getAuth(c).
Fix: Type as Context from hono (or a narrow interface exposing get), removing the any and the disable.
IN-02: 403 for a non-existent calendar conflates not-found with forbidden
File: apps/api/src/routes/events.ts:254-256
Issue: POST /create returns 403 "Calendar not found or access denied" whether the URL does not exist or exists-but-unauthorized. Conflating is a defensible hardening choice, but the file is inconsistent (404 for missing event in PATCH/DELETE, 403 here, 422 for "no writable calendar"), suggesting the conflation is incidental.
Fix: If intentional, add a comment stating the existence-oracle avoidance; otherwise align with the 404 used elsewhere.
IN-03: parseDateTime regex-tests the un-cleaned string
File: apps/pwa/src/components/EventForm.tsx:88-90
Issue: Line 89 computes clean (bracket stripped) but line 90 tests the original iso against the all-day YYYY-MM-DD regex. Harmless for current inputs (all-day strings never carry a bracket), but the dead clean value in the all-day branch is misleading about intent.
Fix: Test clean, or move the clean computation below the all-day early-return.
IN-04: resolveDefaultView is a no-op wrapper
File: apps/pwa/src/components/CalendarShell.tsx:64-67
Issue: resolveDefaultView returns 'month-grid' only in the (unreachable here) SSR branch and otherwise returns its argument unchanged — it adds no behavior over reading selectedView directly.
Fix: Inline selectedView at the call site, or have the helper actually resolve the breakpoint default.
Reviewed: 2026-06-09T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard