familysync/.planning/milestones/v1.0-phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter2.md

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
03-event-write-back-pwa-install	2026-06-09T00:00:00Z	.planning/phases/03-event-write-back-pwa-install/03-REVIEW.md	1	14	13	1	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, andlimit(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