luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(03): add code review report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-09 10:32:20 -04:00
parent 197e138e3b
commit 8b519460d9
1 changed files with 100 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md
+100 -88
View File
@@ -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 `<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: `resolveUserId` parameter typed `any`, defeating type safety at the auth boundary
### 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:** `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.
**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-02: 403 for a non-existent calendar conflates not-found with forbidden
### IN-05: `deleteCalendarEvent` relies on tsdav ignoring `data: ''`
**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.
**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-09T00:00:00Z_
_Reviewed: 2026-06-09_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_