170 lines
14 KiB
Markdown
170 lines
14 KiB
Markdown
---
|
|
phase: 03-event-write-back-pwa-install
|
|
reviewed: 2026-06-09T00:00:00Z
|
|
depth: standard
|
|
files_reviewed: 28
|
|
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: 4
|
|
total: 13
|
|
status: 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:
|
|
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`.
|
|
|
|
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:
|
|
```ts
|
|
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_
|