182 lines
13 KiB
Markdown
182 lines
13 KiB
Markdown
---
|
|
phase: 03-event-write-back-pwa-install
|
|
reviewed: 2026-06-09T00:00:00Z
|
|
depth: standard
|
|
files_reviewed: 29
|
|
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: 5
|
|
total: 14
|
|
status: issues_found
|
|
---
|
|
|
|
# Phase 3: Code Review Report
|
|
|
|
**Reviewed:** 2026-06-09
|
|
**Depth:** standard
|
|
**Files Reviewed:** 29
|
|
**Status:** issues_found
|
|
|
|
## Summary
|
|
|
|
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.
|
|
|
|
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: Event edit/delete ownership check resolves an arbitrary member's row (uid-only lookup)
|
|
|
|
**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.
|
|
|
|
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.
|
|
|
|
### CR-02: Outbox WR-02 "freshest etag" re-read also queries uid-only — can pick the wrong member's 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)
|
|
.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)
|
|
```
|
|
|
|
### CR-03: All-day end date is exclusive on write but inclusive on edit pre-fill — span grows one day per re-edit
|
|
|
|
**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: Recurrence is silently reset to `none` on every edit — data loss on recurring events
|
|
|
|
**File:** `apps/pwa/src/components/EventForm.tsx:188-196`
|
|
**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: `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:** 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-05: `deleteCalendarEvent` relies on tsdav ignoring `data: ''`
|
|
|
|
**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-09_
|
|
_Reviewer: Claude (gsd-code-reviewer)_
|
|
_Depth: standard_
|