---
phase: 03-event-write-back-pwa-install
fixed_at: 2026-06-09T00:00:00Z
review_path: .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md
iteration: 1
findings_in_scope: 14
fixed: 13
skipped: 1
status: partial
---

# Phase 3: Code Review Fix Report

**Fixed at:** 2026-06-09
**Source review:** .planning/phases/03-event-write-back-pwa-install/03-REVIEW.md
**Iteration:** 1

**Summary:**
- Findings in scope: 14 (fix_scope: all — Critical + Warning + Info)
- Fixed: 13
- Skipped: 1

**Note on recovery:** a prior `--fix` run was interrupted (orphan worktree
`/tmp/sv-03-reviewfix-uxjhc1` + branch `gsd-reviewfix/03-53993` + recovery sentinel).
That run's 3 commits had mismatched finding labels and its branch had diverged from the
current branch tip (which had advanced with docs commits, making a fast-forward
impossible). Per the recovery protocol the orphan worktree/branch/sentinel were cleaned
up and all fixes were re-applied fresh from the current branch tip. All 13 commits below
are new.

**Verification environment:** the isolated worktree had no `node_modules` (gitignored,
not carried into a fresh worktree). `node_modules` from the main repo were symlinked in
so `tsc --noEmit` could resolve dependencies for Tier-2 syntax/type checks. The symlinks
are gitignored and were never committed. Every fix was Tier-2 verified (full
`tsc --noEmit` per affected package, clean).

## Fixed Issues

### CR-01: Event edit/delete ownership check resolves an arbitrary member's row (uid-only lookup)

**Files modified:** `apps/api/src/routes/events.ts`
**Commit:** 54addb1
**Status:** fixed: requires human verification (ownership/authorization logic)
**Applied fix:** Both the PATCH `/:uid/edit` and DELETE `/:uid` lookups now scope the
`calendarEvents` → `calendars` join to the acting member's writable set
(`or(calendars.userId = currentUserId, calendars.isShared)`), add
`orderBy(sql\`(calendars.userId = currentUserId) desc\`)` so the user's own row ranks
ahead of a shared/other copy, and `limit(1)` for determinism. This stops `[0]` from
resolving to another member's calendar row for a shared-account uid (D-16).

### CR-02: Outbox WR-02 "freshest etag" re-read also queries uid-only

**Files modified:** `apps/api/src/broker/outboxWorker.ts`
**Commit:** a596f52
**Status:** fixed: requires human verification (etag-selection logic)
**Applied fix:** The pre-PUT freshest-etag re-read now joins through `calendars` and
filters on the outbox row's own `userId` + `calendarUrl` with `limit(1)`, so the etag
used in `If-Match` belongs to the writing member's calendar instead of an arbitrary
shared-account row. `calendars` added to the schema import.

### CR-03: All-day end date exclusive on write but inclusive on edit pre-fill

**Files modified:** `apps/pwa/src/components/EventForm.tsx`
**Commit:** f645644
**Status:** fixed: requires human verification (date-arithmetic / data-correctness)
**Applied fix:** Added `exclusiveEndToInclusiveDate()` (DST-safe UTC-component
subtraction) and apply it when pre-filling the end-date input for all-day occurrences —
both in the initial `useState` and the open/reset effect. Keeps `occurrence.end`
exclusive everywhere (reviewer option a); `buildVeventString` still rolls forward to
exclusive at the ICS boundary, so a re-edit no longer grows the span by a day.
**Note:** the reviewer also suggested a regression test (edit an all-day multi-day event
twice, assert the span is stable). Not added — flagged for the developer.

### WR-01: Recurrence silently reset to `none` on every edit — data loss

**Files modified:** `apps/api/src/broker/outboxWorker.ts`, `apps/api/src/broker/vevent.ts`, `apps/pwa/src/api/client.ts`, `apps/pwa/src/components/EventForm.tsx`
**Commit:** 02aa407
**Status:** fixed: requires human verification (data-loss-prevention logic)
**Applied fix:** Coordinated change so an edit no longer strips a recurring series:
- `vevent.ts`: new `extractRruleString()` parses the existing RRULE from a stored VEVENT.
- `outboxWorker.ts` (update path): when the payload carries no explicit `recurrence`, the
  freshest-etag query also reads `rawVevent` and preserves the existing RRULE; an explicit
  recurrence value (including `'none'`) still overrides.
- `client.ts`: `CreateEventPayload.recurrence` made optional (matches the API Zod schema,
  which already had it optional).
- `EventForm.tsx`: on edit, `recurrence` is omitted from the payload (signals "unchanged")
  and the recurrence `<select>` is disabled — editing recurrence is deferred until the
  occurrence contract exposes it.

### WR-02: Default-calendar selection on create is non-deterministic

**Files modified:** `apps/api/src/routes/events.ts`
**Commit:** 5499f83
**Applied fix:** Added `.orderBy(calendars.id).limit(1)` to the default-calendar query in
POST `/create`, giving a stable insertion-order default instead of an arbitrary `[0]`.

### WR-03: `parseDateTime` all-day check uses the raw `iso`, not the cleaned string

**Files modified:** `apps/pwa/src/components/EventForm.tsx`
**Commit:** d34edec
**Applied fix:** The all-day regex test and early return now use `clean` (IANA-suffix
stripped) instead of the raw `iso`, matching the documented strip intent.

### WR-04: Worker cron schedules start on bare module import — pollutes the test process

**Files modified:** `apps/api/src/index.ts`
**Commit:** 7bc129f
**Applied fix:** `startBrokerPoller()` and `startOutboxWorker()` moved out of top level
into the `isMainModule()` entrypoint guard, so importing `./index.js` in route tests no
longer registers real `node-cron` schedules or leaks open handles.

### WR-05: `index.ts` direct-run guard is fragile and can mis-fire

**Files modified:** `apps/api/src/index.ts`
**Commit:** 22d1bc2
**Applied fix:** Replaced the basename-tail `endsWith` heuristic with
`isMainModule()` comparing `fileURLToPath(import.meta.url)` against
`realpathSync(process.argv[1])` (symlink-resolved), guarded by try/catch.

### WR-06: Edit-as-move create-412 dead-ends the move with no retry path

**Files modified:** `apps/api/src/broker/outboxWorker.ts`, `apps/pwa/src/components/SyncStateToast.tsx`
**Commit:** 1c71f8c
**Status:** fixed: requires human verification (UX/conflict-flow logic)
**Applied fix:** When a create row carrying a `groupId` (edit-as-move) hits 412, the
worker now writes a distinct `move-failed:` `lastError` (no `'412'` substring).
`SyncStateToast` detects it (`error.startsWith('move-failed')`), routes it away from the
etag-conflict copy, and shows "Couldn't move the event. Open it and save again." No
contract change — surfaced via the existing `sync-status` `error` field.

### IN-01: `triggerTargetedResync` re-loads and re-decrypts the credential per row

**Files modified:** `apps/api/src/broker/outboxWorker.ts`
**Commit:** 95f9d8c
**Applied fix:** `triggerTargetedResync` accepts an optional per-drain-cycle
`Map<number, FastmailClient>` cache; `runOutboxDrain` creates one per cycle and passes it
to both call sites, so each member's credential is decrypted at most once per cycle
(narrows the decrypted-password-in-memory window, T-03-13). Cache is discarded when the
drain returns.

### IN-02: Unknown-status responses retried for the full backoff window before giving up

**Files modified:** `apps/api/src/broker/outboxWorker.ts`
**Commit:** e29d6c1
**Status:** fixed: requires human verification (error-classification logic)
**Applied fix:** `dispatchRow` now classifies any unmapped 4xx (status 400–499, after the
explicit 408/429 transient set and 400/401/403 hard-fail set are handled) as a hard fail,
so permanent client errors (405/409/422) settle immediately instead of burning the retry
budget. 5xx, network, and truly unknown statuses still fall through to transient.

### IN-03: `InstallPrompt` reads `localStorage` synchronously without try/catch

**Files modified:** `apps/pwa/src/components/InstallPrompt.tsx`
**Commit:** 7e4ea71
**Applied fix:** Added guarded `readDismissed()` / `persistDismissed()` helpers
(try/catch, mirroring `calendarStore.ts`) used by the `useState` initializer and
`dismiss()`, so a throwing `localStorage` (private mode / SSR) degrades to "not dismissed"
instead of crashing the component on mount.

### IN-04: `resolveUserId` typed as `any`

**Files modified:** `apps/api/src/routes/events.ts`
**Commit:** 6d2fd79
**Applied fix:** Parameter typed as Hono's `Context` (imported as a type) instead of
`any`, removing the eslint-disable. `c.get('user')` resolves through the existing
`ContextVariableMap` augmentation in `auth/devBypass.ts` and `getAuth(c)` accepts a
`Context`. Used `Context` rather than the reviewer's literal
`Context<{ Variables: { user?: { id: number } } }>` because the latter would conflict
with the global `ContextVariableMap` augmentation (which types `user` non-optionally as
the DEV_USER shape).

## Skipped Issues

### IN-05: `deleteCalendarEvent` relies on tsdav ignoring `data: ''`

**File:** `apps/api/src/broker/write.ts:93`
**Reason:** skipped: reviewer specifies "None required for v1; note on the tsdav upgrade
checklist." No source change is warranted — the finding asks for a process/checklist note,
not a code fix. The existing inline comment already documents the dependency on tsdav
internals. Flagged here so the developer can add a tsdav-upgrade-checklist entry.
**Original 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.

---

_Fixed: 2026-06-09_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 1_