From d89eb47483e5c2f0d286318bc799618cf839ca00 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Tue, 9 Jun 2026 10:19:01 -0400 Subject: [PATCH] docs(03): add code review report --- .../03-REVIEW.md | 228 ++++++++---------- 1 file changed, 94 insertions(+), 134 deletions(-) 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 275eb4c..25603f8 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 @@ -1,8 +1,8 @@ --- phase: 03-event-write-back-pwa-install -reviewed: 2026-06-05T00:00:00Z +reviewed: 2026-06-09T00:00:00Z depth: standard -files_reviewed: 18 +files_reviewed: 28 files_reviewed_list: - apps/api/src/broker/outboxWorker.ts - apps/api/src/broker/sync.ts @@ -11,199 +11,159 @@ files_reviewed_list: - 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/package.json + - apps/pwa/vitest.config.ts findings: - critical: 6 - warning: 8 - info: 5 - total: 19 + critical: 3 + warning: 6 + info: 4 + total: 13 status: issues_found --- # Phase 3: Code Review Report -**Reviewed:** 2026-06-05 +**Reviewed:** 2026-06-09T00:00:00Z **Depth:** standard -**Files Reviewed:** 18 +**Files Reviewed:** 28 **Status:** issues_found ## Summary -This phase is the CalDAV write-back + outbox + PWA-install slice, about to undergo its first live Gate 2 write test. The review found that **the write path is fundamentally broken end-to-end** and will fail at multiple independent points before any event ever reaches Fastmail: +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. -1. The PWA sends `title/start/end`, but the server's zod schema requires `summary/dtstart/dtend` — **every create and update is rejected with 400 at the route boundary.** The write path cannot succeed today. -2. Even if a row reaches the outbox, the worker PUTs the **raw JSON form payload** to Fastmail as if it were an iCalendar string. `buildVeventString` (the entire `vevent.ts` file, the D-13 DATE/DATETIME contract) is **never called** — it is dead code. Fastmail will reject the body or store a corrupt object. -3. The edit-as-move create row and the `update` row reuse the form-fields JSON as `payload`, so recurrence, all-day handling, and field mapping are all lost regardless. +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. -Because of (1) and (2), the core acceptance criterion of this phase (write an event to Fastmail) cannot pass. These must be fixed before Gate 2. - -Secondary but serious: a production fallback in `dispatchRow` can silently authenticate with empty credentials on a transient DB error; the backoff schedule skips its first delay; and several outbox/ordering edge cases can drop or double-apply writes across drain batches. +## Narrative Findings (AI reviewer) ## Critical Issues -### CR-01: API contract mismatch — every create/update rejected with 400 +### CR-01: All-day event grows by one day on every edit (cumulative DTEND drift) -**File:** `apps/api/src/routes/events.ts:68-77`, `apps/pwa/src/api/client.ts:119-128`, `apps/pwa/src/components/EventForm.tsx:247-256` -**Issue:** The server `eventFieldsSchema` requires `summary`, `dtstart`, `dtend`. The client `CreateEventPayload` and `EventForm.handleSubmit` send `title`, `start`, `end`. `@hono/zod-validator` rejects the body before the handler runs, so `POST /api/events/create` and `PATCH /api/events/:uid/edit` return 400 for every well-formed client request. The write path is dead on arrival. (`recurrence` is also `required` on the client type but `.optional()` on the server — a lesser instance of the same drift.) -**Fix:** Make the two ends agree on one field contract. Either rename the client payload to `summary/dtstart/dtend`, or accept `title/start/end` server-side and map internally: +**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 eventFieldsSchema = z.object({ - title: z.string().min(1).max(255), - allDay: z.boolean(), - start: z.string().min(1).max(64), - end: z.string().min(1).max(64), - location: z.string().max(2000).optional(), - description: z.string().max(2000).optional(), - recurrence: z.enum(['none','daily','weekly','monthly','yearly']).optional(), - calendarUrl: z.string().url().max(1024).optional(), -}) +const freshEtagRows = await db + .select({ etag: calendarEvents.etag }) + .from(calendarEvents) + .where(eq(calendarEvents.uid, row.uid)) ``` -Add a contract test that round-trips the exact `CreateEventPayload` shape through the schema. +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. -### CR-02: Outbox worker PUTs raw form JSON to Fastmail — `buildVeventString` is dead code +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. -**File:** `apps/api/src/broker/outboxWorker.ts:168-187`, `apps/api/src/routes/events.ts:250`, `apps/api/src/broker/vevent.ts` (entire file) -**Issue:** The route stores `payload: JSON.stringify(payload)` — the raw form fields. The worker passes `row.payload` directly as the `iCalString`/`data` to `createCalendarEvent`/`updateCalendarEvent`. `buildVeventString` is never imported or called anywhere in the codebase (`grep` confirms zero call sites outside its own file). The body sent to Fastmail is therefore `{"title":"...","start":"..."}` — not a VCALENDAR. Fastmail will reject it (or, worse, store a corrupt object). The entire D-13 DATE-vs-DATETIME contract, RRULE serialization, escaping, and DTSTAMP logic in `vevent.ts` is bypassed. -**Fix:** The worker must parse the stored form JSON and build the ICS before PUT: -```ts -// in dispatchRow, for create/update: -const fields = JSON.parse(row.payload) as NewEventParams-equivalent -const { icsString } = buildVeventString({ - uid: row.uid, - summary: fields.title, - allDay: fields.allDay, - dtstart: fields.allDay ? fields.start : new Date(fields.start), - dtend: fields.allDay ? fields.end : new Date(fields.end), - location: fields.location, - description: fields.description, - rruleString: fields.recurrence && fields.recurrence !== 'none' - ? RRULE_PRESETS[fields.recurrence] : undefined, -}) -response = await createCalendarEvent(client, davCalendar, row.uid, icsString) -``` -Wrap `JSON.parse` in try/catch and treat a parse failure as a hard fail (no retry). Add an integration test asserting the PUT body begins with `BEGIN:VCALENDAR`. +**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: Production fallback authenticates to Fastmail with empty credentials +### CR-03: Cached events for the other member's mirror row are never pruned after a write -**File:** `apps/api/src/broker/outboxWorker.ts:135-143` -**Issue:** `dispatchRow` wraps `loadClientForUser` in a `try/catch` and, on **any** throw, falls back to `createFastmailClient('', '')`. The comment claims this branch is "never taken" in production, but `loadClientForUser` can throw in production for real reasons: a transient DB error on the `memberCredentials` select, a decryption failure (`decryptPassword` throws on a tampered/rotated key), or a missing credential row. When that happens in production, the worker proceeds to issue a real PUT/DELETE to `caldav.fastmail.com` with empty Basic-auth credentials. At best this is a 401 (correctly classified hard-fail, marking the row `failed` and dropping the write permanently — no retry); at worst it masks a recoverable transient DB error as a permanent failure. Either way a write is silently lost on a condition that should have been retried. -**Fix:** Remove the production fallback. Let `loadClientForUser` failures propagate to the per-row `catch` in `runOutboxDrain` (line 343), which logs and leaves the row `pending` for the next drain — the correct transient behavior. Gate the empty-credential client strictly behind a test-only flag, never on a generic `catch`: -```ts -const client = await loadClientForUser(row.userId) // let it throw → outer catch retries -``` +**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. -### CR-04: 412 conflict on a `create` move-pair does not block the paired `delete` reliably across batches +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. -**File:** `apps/api/src/broker/outboxWorker.ts:247-281`, `292-296` -**Issue:** `failedCreateGroups` is a `Set` local to a single `runOutboxDrain` call. The create-before-delete guarantee only holds when both rows are fetched in the **same** drain batch. The fetch is capped at "up to 10" pending rows (and ordered only by the default DB order, not by `groupId`/`createdAt`). If a move pair straddles a batch boundary — or the create is retried into a later cycle (transient) while the delete is already eligible — the delete can run in a cycle where the create is absent from `sorted`, so `failedCreateGroups` is empty and the delete proceeds. Result: the original event is deleted before the new copy is confirmed created — exactly the "lost event" D-04 is meant to prevent. The in-memory set cannot enforce a cross-batch ordering invariant. -**Fix:** Make the dependency durable. Options: (a) do not enqueue the `delete` row as `pending`; enqueue it `blocked` and have the worker flip it to `pending` only after the paired create row reaches `done`; or (b) when dispatching a `delete` with a `groupId`, query the DB for the paired create row's status and skip/defer unless it is `done`. Do not rely on both rows co-occurring in one in-memory batch. - -### CR-05: No drain concurrency guard — overlapping cycles double-dispatch the same row - -**File:** `apps/api/src/broker/outboxWorker.ts:247-259`, `359-365` -**Issue:** The scheduler fires `runOutboxDrain` every 15s. A drain that issues several network PUTs plus targeted re-syncs (each `triggerTargetedResync` does a full `fetchCalendars` + `syncCalendar`) can easily exceed 15s. The selection query reads `status='pending'` but nothing marks a row "in-flight" before dispatch, and the row is only updated to `done`/`failed` **after** the network call returns. Two overlapping cycles will both select the same still-`pending` row and both issue the write. For a `create` (PUT with `If-None-Match: *`) the second attempt may 412 and get marked `failed`, masking a successful first write; for a `delete` the second DELETE re-runs the If-Match against a now-changed etag and can mis-classify. This is a double-apply / lost-confirmation hazard. -**Fix:** Add a concurrency guard. Simplest: a module-level `isDraining` boolean that the scheduler checks and skips if a drain is still running. More robust: atomically claim rows by updating `status='pending' → status='processing'` (with a `WHERE status='pending'` guard and `LIMIT`) inside a transaction before dispatch, and only the claiming cycle processes them. - -### CR-06: OIDC write path is a hard 401 stub — authenticated users cannot write in production - -**File:** `apps/api/src/routes/events.ts:191-201`, `268-274`, `372-378`, `437-443`, `490-496` -**Issue:** Every write/status/writable-calendars handler resolves the user via `resolveUserId`, which only returns the dev-bypass `c.get('user')`. When that is null (the real OIDC production path), the code calls `getAuth(c)` and then **unconditionally returns 401** even when `auth` is truthy ("For now return 401 if OIDC auth is not backed by a DB user here."). In production (`devBypassActive=false`), `resolveUserId` is always null, so authenticated Authelia users get 401 on every write and on `sync-status`/`writable-calendars`. The phase is described as about to undergo its first live external-auth write test (Gate 2) — this path is a stub that cannot pass. (The OIDC guard mounting in `index.ts` is correct; the issue is the unimplemented iss/sub → user-row lookup.) -**Fix:** Implement the OIDC→user resolution: from `getAuth(c)` extract `iss`+`sub`, look up the `users` row by the `uniq_oidc_identity` key, and use that `id` as `currentUserId`. Return 401 only when no session exists; return 403/422 (not 401) when a valid session has no provisioned user row. +**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: Backoff schedule skips its first (15s) delay; off-by-one on dead-letter count +### WR-01: `create` default-calendar selection is non-deterministic and may target the shared calendar -**File:** `apps/api/src/broker/outboxWorker.ts:38-44, 316-341` -**Issue:** `attemptCount` starts at 0. On the first transient failure `nextAttemptCount = 1`, and the delay is read as `BACKOFF_SECONDS[1]` = 60s — so `BACKOFF_SECONDS[0]` (15s) is never used; the documented "15+60+300+600+1800 ≈ 30 min" window is actually 60+300+600+1800. Also `MAX_ATTEMPTS=5` with `nextAttemptCount >= MAX_ATTEMPTS` dead-letters after the 5th attempt's increment reaches 5 — the comment "~30 min" and the indexing should be reconciled. -**Fix:** Index by `row.attemptCount` (the attempt that just failed) rather than `nextAttemptCount`: `const backoffMs = (BACKOFF_SECONDS[row.attemptCount] ?? 1800) * 1000`. Add a unit test asserting the exact delay sequence. +**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: Edit (same-calendar update) reuses old etag but a successful prior update changes it — guaranteed 412 on second edit +### WR-02: PATCH edit-as-move does not authorize the *destination* calendar -**File:** `apps/api/src/routes/events.ts:348-357`, `apps/api/src/broker/outboxWorker.ts:297-303` -**Issue:** On a successful update the worker marks the row `done` and triggers a re-sync, which updates `calendarEvents.etag`. That is correct. But the route reads `eventRow.etag` from the cache at enqueue time. If a user edits twice in quick succession (or the poller has not yet refreshed the etag), the second update row carries a stale etag and will 412 even though the user is the only editor. The conflict toast then fires spuriously. This is an interaction between optimistic-accept latency and If-Match. -**Fix:** Accept that rapid successive edits to the same object should coalesce or chain: either collapse pending update rows for the same uid before enqueueing, or re-read the freshest etag in the worker just before PUT rather than trusting the enqueue-time snapshot. +**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: Stale `occurrence` snapshot in EventForm — edits open with empty/old fields +### WR-03: `triggerTargetedResync` swallows all errors, so a re-sync failure leaves stale cache while marking the row `done` -**File:** `apps/pwa/src/components/EventForm.tsx:113-181` -**Issue:** `occurrence` is resolved once via an IIFE during render from the TanStack cache. The initial `useState` values capture it, but the reset `useEffect` depends on `[eventFormOpen, eventFormMode, eventFormUid]` — not on `occurrence`. If the form opens (in edit mode) before the `['events']` query has the occurrence cached, `occurrence` is null at mount and the effect never re-runs when the data later arrives, so the form stays blank. `recurrence` is also hard-reset to `'none'` on every open, so editing a recurring event silently drops its recurrence. -**Fix:** Include `occurrence` (or `occurrence?.uid`) in the reset effect deps, and derive the initial `recurrence` from the occurrence instead of always `'none'`. Guard against opening edit mode before the cache is populated. +**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: All-day end-date is exclusive in iCalendar but UI treats it as inclusive +### WR-04: SyncStateToast cannot surface a failed delete on an edit-as-move -**File:** `apps/pwa/src/components/EventForm.tsx:215-238, 250-251`, `apps/api/src/broker/vevent.ts:75-88` -**Issue:** For all-day events the form sends `end = endDate` and validates `endDate < startDate` as the only error (so a single-day event has `start === end`). iCalendar DTEND for a DATE value is **exclusive** — a one-day all-day event must have DTEND = start + 1 day. As written, a single-day all-day event would produce DTSTART=DTEND, which is invalid/zero-length per RFC 5545. (Currently moot because CR-02 means no VEVENT is built at all, but it must be fixed alongside CR-02.) -**Fix:** When building the all-day VEVENT, add one day to the end DATE (or normalize in the form). Add a test for the single-day all-day case. +**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: `parseDateTime` uses local-time getters on a UTC-parsed Date — wrong time in edit form +### WR-05: Unknown HTTP status (incl. 404/410) is retried five times then dead-lettered -**File:** `apps/pwa/src/components/EventForm.tsx:84-101` -**Issue:** For timed events `new Date(clean)` parses the offset-aware ISO into an instant, then `d.toISOString().slice(0,10)` takes the **UTC** date while `d.getHours()/getMinutes()` take the **local** time. Mixing UTC date with local clock components can yield a date/time pair that is off by a day at the edges, and the time shown will be the viewer's local wall-clock rather than the event's original zone. For a family spanning Toronto/Edmonton zones this misrepresents the edited event. -**Fix:** Derive both date and time consistently in one zone (use the same Temporal-based conversion the calendar render path uses, or compute local date with `getFullYear/getMonth/getDate`). +**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: `triggerTargetedResync` calls `fetchCalendars` on every successful row — N+1 round-trips and re-load of credentials +### WR-06: Edit silently drops recurrence on a recurring event -**File:** `apps/api/src/broker/outboxWorker.ts:89-117, 297-303` -**Issue:** Each successful/conflicted row triggers a full `loadClientForUser` (DB select + decrypt) plus `fetchCalendars()` (network) plus `syncCalendar` (REPORT of the whole collection). A drain of 10 rows for one user does this 10 times against the same calendar. Beyond load, this re-decrypts the app password 10x and multiplies the window during which overlapping drains (CR-05) can interfere. Out of strict v1 perf scope, but it is also a correctness amplifier for the concurrency and etag-staleness issues above. -**Fix:** Batch re-syncs: collect the distinct `(userId, calendarUrl)` pairs touched during a drain and re-sync each once at the end of the cycle. - -### WR-07: `EventForm` and dialogs implement no real focus trap despite claiming one - -**File:** `apps/pwa/src/components/EventForm.tsx:274-278`, `DeleteConfirmationDialog.tsx:42-46`, `EventDetailPopover.tsx:157-161` -**Issue:** The docblocks state "Focus trap while open," but the implementation only calls `.focus()` once on open. Tab can move focus out of the modal to background content (the calendar grid, FAB). For a modal `aria-modal="true"` dialog this is an accessibility defect and, combined with the always-mounted backdrop, lets keyboard users interact with obscured controls. -**Fix:** Implement an actual focus trap (cycle Tab/Shift+Tab within the dialog) or use a vetted primitive. At minimum, document honestly that it is focus-on-open only. - -### WR-08: `crypto.randomUUID()` used without importing `crypto` in the route module - -**File:** `apps/api/src/routes/events.ts:241, 317, 318` -**Issue:** The route relies on a global `crypto.randomUUID()`. This is available on Node 20+/22 globals, so it likely works at runtime, but there is no `import { randomUUID } from 'crypto'` and no explicit reference to `globalThis.crypto` — it depends entirely on the ambient global being present and typed. `vevent.ts` imports `randomUUID` from `'crypto'` explicitly; the inconsistency is a latent footgun if the runtime or tsconfig `lib` changes. -**Fix:** Import explicitly (`import { randomUUID } from 'node:crypto'`) and use `randomUUID()` for consistency with `vevent.ts`. +**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: `vevent.ts` is entirely unreachable dead code +### IN-01: `resolveUserId` parameter typed `any`, defeating type safety at the auth boundary -**File:** `apps/api/src/broker/vevent.ts:1-119` -**Issue:** As established in CR-02, nothing imports `buildVeventString` or `RRULE_PRESETS`. Once CR-02 is fixed this becomes live; until then the whole file (and its D-13 logic) is untested dead weight that gives false confidence the contract is honored. -**Fix:** Wire it in per CR-02; add a unit test so it cannot silently fall out of the call graph again. +**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: `resolveDefaultView` ignores its own SSR guard return value +### IN-02: 403 for a non-existent calendar conflates not-found with forbidden -**File:** `apps/pwa/src/components/CalendarShell.tsx:63-66` -**Issue:** `resolveDefaultView` returns `'month-grid'` when `window === undefined` else `persistedView` — it never uses a phone/desktop branch, so the JSDoc ("phone defaults to month-agenda") is misleading; the actual default already comes from the Zustand store. The helper is effectively an identity function on the client. -**Fix:** Remove the redundant helper or align the comment with what it does. +**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: `getDefaultStartDate`/`getDefaultEndDate` are identical +### IN-03: `parseDateTime` regex-tests the un-cleaned string -**File:** `apps/pwa/src/components/EventForm.tsx:47-53` -**Issue:** Both return today's date; the naming implies different defaults (the time defaults differ, but those are hardcoded separately at the call sites as `'09:00'`/`'10:00'`). Two functions with identical bodies invite drift. -**Fix:** Collapse to a single `todayIso()` helper (one already exists in `calendarStore.ts`). +**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: `manifest.json`/icons referenced but `apple-touch-icon.png` and PWA icons not verified in scope +### IN-04: `resolveDefaultView` is a no-op wrapper -**File:** `apps/pwa/index.html:7`, `apps/pwa/vite.config.ts:33-37` -**Issue:** The manifest references `/icon-192.png`, `/icon-512.png`, and `index.html` references `/apple-touch-icon.png`. These assets are outside the reviewed file set; if missing, the iOS Add-to-Home-Screen flow (the other half of this phase) will install with a broken icon. Flagging for Gate 2 verification, not a code defect in the reviewed files. -**Fix:** Confirm the icon assets exist in `public/` before the install test. - -### IN-05: Outbox `done`/`failed`/`dead` rows are never pruned - -**File:** `apps/api/src/db/schema.ts:125-154`, `apps/api/src/broker/outboxWorker.ts` -**Issue:** Terminal rows accumulate indefinitely. `sync-status` reads the latest row per uid (ordered by `createdAt desc`), so correctness is preserved, but the table grows unbounded and the `idx_outbox_next_attempt` scan includes ever-more terminal rows over time (the `WHERE status='pending'` filters them, but only after index narrowing). Low urgency for a two-user household. -**Fix:** Add a periodic prune of `done` rows older than N days; keep `failed`/`dead` for audit or prune separately. +**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-05_ +_Reviewed: 2026-06-09T00:00:00Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_