From 7c687ea413c0cf7829503ed784c6b048e3b23ede Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Tue, 9 Jun 2026 11:12:09 -0400 Subject: [PATCH] docs(03): add code review fix report (--auto, 2 fix passes) Auto-fix loop converged after 2 fix iterations + a final verifying re-review: - Pass 1: 13/14 findings fixed (3 Critical, 6 Warning, 4 Info). - Re-review surfaced 1 new Critical (move-path RRULE data loss) + 4 lower. - Pass 2: 8/8 fixed, including the move-path RRULE forwarding. - Final re-review: 0 Critical. Remaining 2 Warning / 2 Info are documented v1 scope cuts (recurrence-editing deferred), not defects. Test suites green throughout: api 108, pwa 145; both tsc --noEmit clean. Per-iteration REVIEW/REVIEW-FIX snapshots retained as audit trail. --- .../03-REVIEW-FIX.iter2.md | 189 ++++++++++++++++++ .../03-REVIEW-FIX.iter3.md | 84 ++++++++ .../03-REVIEW-FIX.md | 84 ++++++++ .../03-REVIEW.iter2.md | 181 +++++++++++++++++ .../03-REVIEW.iter3.md | 160 +++++++++++++++ .../03-REVIEW.md | 132 +++--------- 6 files changed, 728 insertions(+), 102 deletions(-) create mode 100644 .planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter2.md create mode 100644 .planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter3.md create mode 100644 .planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.md create mode 100644 .planning/phases/03-event-write-back-pwa-install/03-REVIEW.iter2.md create mode 100644 .planning/phases/03-event-write-back-pwa-install/03-REVIEW.iter3.md diff --git a/.planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter2.md b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter2.md new file mode 100644 index 0000000..f077419 --- /dev/null +++ b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW-FIX.iter2.md @@ -0,0 +1,189 @@ +--- +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 `` 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_ diff --git a/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.iter3.md b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.iter3.md new file mode 100644 index 0000000..0b2c11b --- /dev/null +++ b/.planning/phases/03-event-write-back-pwa-install/03-REVIEW.iter3.md @@ -0,0 +1,160 @@ +--- +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: 1 + warning: 4 + info: 3 + total: 8 +status: issues_found +--- + +# Phase 3: Code Review Report (Re-Review, Iteration 2) + +**Reviewed:** 2026-06-09 +**Depth:** standard +**Files Reviewed:** 29 +**Status:** issues_found + +## Summary + +This is a re-review of the event write-back + PWA-install phase after a 13-item fix pass. I verified each of the previously flagged fixes the orchestrator called out: + +- **CR-01 / CR-02 member-scoped lookups** — VERIFIED FIXED. `events.ts` PATCH/DELETE now scope the `calendarEvents` lookup to the acting member's writable set and add a deterministic `ORDER BY (calendars.userId = currentUserId) DESC LIMIT 1` (events.ts:340-347, 453-460). `outboxWorker.ts`'s fresh-etag re-read now joins `calendars` and filters on `calendars.userId = row.userId AND calendars.url = row.calendarUrl` (outboxWorker.ts:228-239), so a shared-account duplicate uid can no longer resolve to the wrong member's etag. +- **CR-03 all-day inclusive/exclusive DTEND** — VERIFIED FIXED and now symmetric. `vevent.ts:106-118` advances the inclusive end by one UTC day on write; `EventForm.tsx:86-96` `exclusiveEndToInclusiveDate()` rolls it back on pre-fill. The round-trip no longer grows multi-day all-day spans. `vevent.test.ts:140-160` asserts DTEND = DTSTART + 1. +- **WR-01 RRULE preserve-on-edit** — PARTIALLY FIXED. The same-calendar `update` path correctly preserves the stored RRULE (`outboxWorker.ts:244-248` reads `rawVevent`, extracts the RRULE, re-applies when the payload omits `recurrence`). **The edit-as-move path (D-04) still silently strips recurrence** — see CR-01. This is a real, demonstrable correctness regression of exactly the class WR-01 set out to prevent, so it is filed as a BLOCKER. + +Other fixes (backoff index `outboxWorker.ts:533-535`, fail-closed credentials `outboxWorker.ts:163-167`, durable create-before-delete `outboxWorker.ts:427-464`, move-failed toast copy `SyncStateToast.tsx:53-58`, localStorage guards `InstallPrompt.tsx:284-298`) are present and correct. + +## Critical Issues + +### CR-01: Edit-as-move silently strips a recurring series' RRULE + +**File:** `apps/api/src/broker/outboxWorker.ts:267-297`, `apps/api/src/routes/events.ts:371-401` + +**Issue:** WR-01 was fixed only for the same-calendar `update` branch. When a recurring event is edited *and moved to a different calendar*, the PATCH handler (`events.ts:371-398`) enqueues a `delete` of the old object plus a `create` with a brand-new `newUid` and the edit payload. The edit payload omits `recurrence` by design (`EventForm.tsx:323`; the recurrence picker is disabled in edit mode). The worker's `create` branch then builds the VEVENT with: + +```ts +rruleString: fields.recurrence && fields.recurrence !== 'none' + ? RRULE_PRESETS[fields.recurrence as string] + : undefined, // ← recurrence absent → undefined → no RRULE +``` + +Unlike the `update` branch, the `create` branch performs **no** `rawVevent` read and **no** `extractRruleString` fallback. The original event's RRULE lives in `calendar_events` under the OLD uid/calendar; the create uses `newUid` and never reads it. Net effect: moving any recurring event to another calendar converts the whole series into a single one-off occurrence on Fastmail — silent data loss — and the original series is deleted once the paired delete runs. This is the identical failure mode WR-01 was meant to eliminate, on a different code path. + +**Fix:** Carry the existing RRULE through the move. Two viable approaches: + +1. In `events.ts`, have the edit lookup also select `rawVevent`, extract the RRULE, and stash it on the create outbox row so the worker re-applies it: + +```ts +// events.ts — add rawVevent to the eventRow select, then in the move branch: +const preservedRrule = extractRruleString(eventRow.rawVevent ?? '') +await tx.insert(calendarOutbox).values({ + /* ...create row... */ + payload: JSON.stringify({ ...payload, _preservedRrule: preservedRrule }), + groupId, +}) +``` +…and in the worker `create` branch, fall back to `fields._preservedRrule` when `recurrence` is absent. + +2. Or, in the worker `create` branch, when the row has a `groupId` (move) and the payload lacks `recurrence`, look up the RRULE from the sibling delete row's original uid/calendar via `calendarEvents.rawVevent` and feed it to `buildVeventString`, mirroring `outboxWorker.ts:244-248`. + +Add a regression test: move a recurring event → assert the created ICS contains `RRULE:`. + +## Warnings + +### WR-01: Edit form cannot edit recurrence and provides no way to remove an RRULE + +**File:** `apps/pwa/src/components/EventForm.tsx:311-327, 715-742` + +**Issue:** The recurrence `` is disabled in edit mode with explanatory helper text (EventForm.tsx:789-800), and `handleAllDayToggle` clamps `endDate` to `max(startDate,endDate)` on toggle-on and clears stale time errors (EventForm.tsx:296-305). -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. +- **All-day inclusive/exclusive DTEND symmetry (CR-03) — STILL CORRECT.** `vevent.ts:116-123` rolls the inclusive end forward one UTC day on write; `EventForm.tsx:86-96` rolls it back on pre-fill. Symmetric; covered by vevent.test.ts:140-160. -### 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. +The two findings below are **carried-forward, deliberately-accepted v1 limitations** (documented in code), not regressions; they are recorded for completeness. There are no blockers in this phase. ## Warnings -### WR-01: Recurrence is silently reset to `none` on every edit — data loss on recurring events +### WR-01: Missing cached etag still produces an unconditional PUT/DELETE (D-08 gap) -**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 `` is hard-disabled on edit and the payload always omits `recurrence` on edit (EventForm.tsx:367). The server treats an absent `recurrence` as "preserve the stored RRULE" (both the same-calendar update and the move path). The consequence is that a user can never (a) change a recurring event's frequency, nor (b) intentionally make a recurring event non-recurring — there is no way to express "remove the RRULE" through the edit form, because "omit recurrence" is reserved to mean "unchanged." Helper text now surfaces the constraint (EventForm.tsx:789-800), which is the iteration-2 mitigation, so this is a documented v1 scope cut rather than a silent trap. Recorded because the overloaded semantics will need disentangling when recurrence editing ships (a sentinel distinct from "omitted" will be required to express "remove"). -**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. +**Fix:** When recurrence editing lands, introduce an explicit "remove recurrence" signal distinct from an omitted field (e.g. `recurrence: 'none'` already overrides — wire the edit form to send it when the user clears the schedule), and parse-and-modify the stored RECUR in place rather than replacing it with a bare preset (see IN-01). ## Info -### IN-01: `triggerTargetedResync` re-loads and re-decrypts the credential per row +### IN-01: `RRULE_PRESETS` round-trip is lossy for any parameterized RRULE -**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. +**File:** `apps/api/src/broker/vevent.ts:49-54`; `apps/api/src/broker/outboxWorker.ts:245-248, 337-340` -### IN-02: Unknown-status responses retried for the full backoff window before giving up +**Issue:** `RRULE_PRESETS` maps only to bare `FREQ=DAILY|WEEKLY|MONTHLY|YEARLY`. `extractRruleString` correctly preserves the full stored RECUR (which may carry `BYDAY`/`INTERVAL`/`COUNT`/`UNTIL`), and both preserve paths keep that rich rule. But if a `recurrence` preset value is ever applied to a previously-rich rule, it collapses the rule to the bare preset — silently dropping qualifiers. This cannot happen in v1 (the picker offers only the four bare presets and is disabled on edit), so it is latent, not active. The limitation is now documented at the `RRULE_PRESETS` definition (vevent.ts:39-48). -**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. +**Fix:** When recurrence editing ships, parse the existing RECUR and modify it in place instead of replacing it with a preset. -### IN-03: `InstallPrompt` reads `localStorage` synchronously in `useState` initializer without try/catch +### IN-02: Move-path RRULE preservation depends silently on `rawVevent` being non-empty -**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. +**File:** `apps/api/src/routes/events.ts:388-391` -### IN-04: `resolveUserId` typed as `any` +**Issue:** In the move branch, `preservedRrule = payload.recurrence === undefined ? extractRruleString(eventRow.rawVevent ?? '') : undefined`. If `eventRow.rawVevent` is ever null/empty (it is selected at events.ts:338 and `calendar_events.rawVevent` is `notNull` per schema.ts:106, so this is not currently reachable), `extractRruleString('')` returns `undefined` and the move silently drops the RRULE with no diagnostic. The schema NOT NULL constraint makes this safe today; the fragility is that the preserve path has no observability if that invariant ever changes (unlike write.ts:75-77 which logs the analogous no-etag gap). -**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. +**Fix:** Optional — log a warning when a move with no explicit recurrence finds no extractable RRULE on a recurring-looking source, so a future schema/contract change that empties `rawVevent` is diagnosable rather than silent. ---