From 89132be3be5744685949a83a512147a97b731a37 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sun, 14 Jun 2026 08:02:08 -0400 Subject: [PATCH] docs(11): code review (2 blockers, 3 warnings) + goal verification (5/5, 1 live-check deferred) Co-Authored-By: Claude Opus 4.8 --- .../11-per-event-reminders/11-REVIEW.md | 240 ++++++++++++++++++ .../11-per-event-reminders/11-VERIFICATION.md | 162 ++++++++++++ 2 files changed, 402 insertions(+) create mode 100644 .planning/phases/11-per-event-reminders/11-REVIEW.md create mode 100644 .planning/phases/11-per-event-reminders/11-VERIFICATION.md diff --git a/.planning/phases/11-per-event-reminders/11-REVIEW.md b/.planning/phases/11-per-event-reminders/11-REVIEW.md new file mode 100644 index 0000000..575de29 --- /dev/null +++ b/.planning/phases/11-per-event-reminders/11-REVIEW.md @@ -0,0 +1,240 @@ +--- +phase: 11-per-event-reminders +reviewed: 2026-06-14T00:00:00Z +depth: deep +files_reviewed: 8 +files_reviewed_list: + - apps/api/src/broker/vevent.ts + - apps/api/src/broker/reminderScheduler.ts + - apps/api/src/broker/sync.ts + - apps/api/src/broker/expand.ts + - apps/api/src/broker/outboxWorker.ts + - apps/api/src/routes/events.ts + - apps/pwa/src/api/client.ts + - apps/pwa/src/components/EventForm.tsx +findings: + critical: 2 + warning: 3 + info: 2 + total: 7 +status: issues_found +--- + +# Phase 11: Code Review Report + +**Reviewed:** 2026-06-14 +**Depth:** deep +**Files Reviewed:** 8 +**Status:** issues_found + +## Summary + +Phase 11 introduces per-event reminders via VALARM building, classification, a variable-lead scheduler, and a reminder picker in EventForm. The core mechanics are well-structured: the `resetType('duration')` / `resetType('date-time')` approach correctly prevents Pitfall 2 (VALUE=TEXT), the `hasExplicitReminder` preserve pattern mirrors the established `hasExplicitRecurrence` pattern, the uid:dtstartMs compound dedup key correctly handles rescheduled events, and input paths use ical.js properly with no string-splicing or eval. + +Two blockers were found. The more serious one is a systemic Pitfall 1 violation for events carrying custom (absolute DATE-TIME or multi-VALARM) alarms: the API contract collapses `{kind:'custom'}` to `null` in the GET response, making the form's `__custom__` preserve path permanently unreachable. Any edit to such an event silently strips the alarm. The second blocker is that `humanizeLeadMinutes(0)` produces "Starts in 0 min" in the push notification body for all-day same-day reminders — a misleading and user-visible defect. + +Three warnings were found: positive-duration VALARM triggers (fires-after-event) are classified as before-event leads due to `Math.abs()`, a missing max bound on `reminderLeadMinutes` server-side validation, and an allDay-unaware helper text condition in EventForm that silently suppresses the "Custom reminder kept" callout for timed 10080-minute off-list values. + +--- + +## Critical Issues + +### CR-01: Custom VALARM Silently Stripped on Any Edit — Pitfall 1 for Absolute/Multi Alarms + +**File:** `apps/api/src/broker/vevent.ts:188`, `apps/api/src/broker/expand.ts:240`, `apps/pwa/src/components/EventForm.tsx:83` + +**Issue:** Both `sync.ts` and `expand.ts` map `{kind:'custom'}` (absolute DATE-TIME trigger or multiple VALARMs) to `reminderLeadMinutes = null`. The GET `/events` response therefore sends `null` for both "no alarm" and "custom alarm" events. `deriveReminderValue(null, ...)` unconditionally returns `'__none__'`, so the form initializes the picker to "None" regardless of what kind of VALARM is actually stored. + +When the user saves any field on such an event (title, time, location — anything), the form emits `{ reminderLeadMinutes: null }`. Because `hasExplicitReminder = true`, the outboxWorker skips the preserve path entirely and calls `buildVeventString({ reminderLeadMinutes: null, ... })`, which emits no VALARM. The custom alarm is gone. + +The `'__custom__'` state in EventForm, the disabled "Custom (kept)" option, and the `__custom__ → reminderPayload = {}` preserve branch are all structurally correct but permanently unreachable, because no server GET path ever produces a `'custom'` indicator to the client. The form cannot distinguish a custom alarm from no alarm. + +**Affected path:** any event whose CalDAV source carries an absolute-trigger VALARM (e.g., Apple Calendar's default all-day alarm style) or multiple VALARMs. + +**Fix:** Surface the `'custom'` classification through the API contract so the form can initialize `reminderValue` to `'__custom__'` and emit an absent field (no-change). + +Option A — add an `alarmKind` field alongside `reminderLeadMinutes`: + +```typescript +// In CalendarOccurrence (expand.ts + client.ts) +reminderLeadMinutes: number | null; +alarmKind: 'none' | 'preset' | 'offlist' | 'custom'; // NEW + +// expand.ts — replace the two-value cast: +const alarmClass = classifyValarms(rawVevent); +const reminderLeadMinutes = alarmClass.kind === 'preset' || alarmClass.kind === 'offlist' + ? alarmClass.leadMinutes : null; +const alarmKind = alarmClass.kind; // pass through verbatim + +// EventForm — deriveReminderValue now receives alarmKind: +function deriveReminderValue( + leadMinutes: number | null, + alarmKind: AlarmKind, + isAllDay: boolean, +): string { + if (alarmKind === 'custom') return '__custom__'; // <-- was unreachable, now reachable + if (leadMinutes === null) return '__none__'; + const presets = isAllDay ? ALLDAY_REMINDER_PRESETS : TIMED_REMINDER_PRESETS; + return String(leadMinutes); +} +``` + +Option B (narrower) — add a boolean `hasCustomAlarm` to the occurrence and treat it as `'__custom__'` in `deriveReminderValue`. + +--- + +### CR-02: `humanizeLeadMinutes(0)` Produces "Starts in 0 min" for All-Day Same-Day Push + +**File:** `apps/api/src/broker/reminderScheduler.ts:87,292` + +**Issue:** `humanizeLeadMinutes(leadMinutes: number)` is called for every push notification body. For all-day events with `reminderLeadMinutes = 0` (same-day, 9 AM reminder), `0 < 60` is true so the function returns `"Starts in 0 min"`. The push notification the user receives therefore reads "Starts in 0 min", which is factually wrong (the event starts later today, not in 0 minutes) and will erode trust for non-technical users — the core audience of this app. + +This is a user-visible correctness defect, not a cosmetic issue. + +**Fix:** Branch on all-day vs. timed at the call site in `runReminderCheck`, or add an optional `isAllDay` parameter to `humanizeLeadMinutes`: + +```typescript +// Option A: differentiate at call site +body: event.reminderLeadMinutes === 0 && event.isAllDay + ? 'Reminder: today' + : humanizeLeadMinutes(event.reminderLeadMinutes), + +// Option B: extend humanizeLeadMinutes +export function humanizeLeadMinutes(leadMinutes: number, isAllDay = false): string { + if (isAllDay) { + if (leadMinutes === 0) return 'Reminder: today'; + if (leadMinutes < 2880) return 'Reminder: tomorrow'; + return `Reminder: ${Math.round(leadMinutes / 1440)} days away`; + } + if (leadMinutes < 60) return `Starts in ${leadMinutes} min`; + if (leadMinutes < 120) return 'Starts in 1 hour'; + if (leadMinutes < 1440) return `Starts in ${Math.round(leadMinutes / 60)} hours`; + if (leadMinutes < 2880) return 'Starts in 1 day'; + return `Starts in ${Math.round(leadMinutes / 1440)} days`; +} +``` + +The `byKey` map would need `isAllDay` added to the stored value to carry it to the dispatch loop. + +--- + +## Warnings + +### WR-01: `Math.abs()` Misclassifies Positive-Duration VALARM Triggers (Fires-After-Event) + +**File:** `apps/api/src/broker/vevent.ts:188` + +**Issue:** RFC 5545 allows `TRIGGER:+PT15M` — a VALARM that fires 15 minutes *after* the event starts. `classifyValarms` uses `Math.abs(dur.toSeconds())` to extract the lead, discarding the sign. A `+PT15M` trigger produces `leadMinutes = 15` and is classified as `{kind:'preset', leadMinutes:15}`. Downstream, `sync.ts` stores `reminderLeadMinutes = 15` and the scheduler fires at `dtstartUtc - 15 min` — the opposite direction from the original alarm. + +Apple Calendar and some enterprise CalDAV clients (Outlook) do emit post-event alarms for task-follow-up use cases. The round-trip silently inverts the alarm direction. + +**Fix:** Check the sign of `toSeconds()` before the preset lookup. A positive value should return `{kind:'custom'}` to trigger the preserve path (no modification by this app): + +```typescript +const seconds = dur.toSeconds(); +if (seconds > 0) return { kind: 'custom' }; // positive = fires after event, preserve as-is +const leadMinutes = Math.round(Math.abs(seconds) / 60); +return PRESET_MINUTES.has(leadMinutes) + ? { kind: 'preset', leadMinutes } + : { kind: 'offlist', leadMinutes }; +``` + +--- + +### WR-02: No Upper Bound on `reminderLeadMinutes` in Server-Side Validation + +**File:** `apps/api/src/routes/events.ts:125`, `apps/api/src/broker/outboxWorker.ts:105` + +**Issue:** Both `eventFieldsSchema` and `outboxPayloadSchema` validate `reminderLeadMinutes` as `z.number().int().min(0)` with no maximum. The EventForm UI caps at 10080 (1 week), but these schemas are the server-side trust boundary. A direct API call with `reminderLeadMinutes = 99999999` would be accepted, stored, and cause: + +1. `buildTimedValarm(99999999)` → `TRIGGER:-PT99999999M` in the ICS — technically valid iCal but 190 years in the past for all-day; confusing to native CalDAV clients. +2. The all-day scheduler: `leadDays = 99999999 / 1440 ≈ 69444` → `computeAlertInstantUtc` returns a date ~190 years ago → alertInstant far outside the catch-up window → silent no-fire (safe from crash perspective, but data is corrupted). + +The server is the trust boundary. It should enforce the same maximum the UI enforces: + +```typescript +// In both eventFieldsSchema and outboxPayloadSchema: +reminderLeadMinutes: z.number().int().min(0).max(10080).nullable().optional(), +``` + +--- + +### WR-03: Helper Text Condition Not allDay-Aware — Suppresses "Custom reminder kept" for Timed 10080 + +**File:** `apps/pwa/src/components/EventForm.tsx:1017-1022` + +**Issue:** The helper text displayed in edit mode uses both preset sets conjunctively: + +```tsx +!TIMED_REMINDER_PRESETS.has(parseInt(reminderValue, 10)) && +!ALLDAY_REMINDER_PRESETS.has(parseInt(reminderValue, 10)) && +``` + +`ALLDAY_REMINDER_PRESETS` includes `10080`. If a native CalDAV client has set a 7-day (10080-minute) DURATION trigger on a timed event, `classifyValarms` returns `{kind:'preset', leadMinutes:10080}`, the DB stores 10080, and the form initializes `reminderValue = '10080'`. The timed picker correctly renders a synthetic "168 hours before" option (since 10080 is not in `TIMED_REMINDER_PRESETS`), but the helper text is suppressed because `ALLDAY_REMINDER_PRESETS.has(10080)` is `true`. The user sees an unexpected picker value with no explanation. + +**Fix:** Gate on the current `allDay` state: + +```tsx +{eventFormMode === 'edit' && + (reminderValue === '__custom__' || + (reminderValue !== '__none__' && + !(allDay ? ALLDAY_REMINDER_PRESETS : TIMED_REMINDER_PRESETS).has( + parseInt(reminderValue, 10) + ) && + Number.isFinite(parseInt(reminderValue, 10)))) && ( +
Custom reminder kept — select a preset to replace it.
+ )} +``` + +--- + +## Info + +### IN-01: All-Day DB Query Has No Lower Bound on `dtstartDate` + +**File:** `apps/api/src/broker/reminderScheduler.ts:154-177` + +**Issue:** The all-day events query filters `dtstartDate <= allDayWindowEnd` but has no lower bound. Every tick queries all past all-day events that have a non-null `reminderLeadMinutes`, regardless of how old they are. The JS-side fire-time check correctly discards them (alertInstant is far in the past), so there is no correctness defect, but the DB query returns O(all-calendar-history) rows every minute as calendar data accumulates. + +A lower bound of `dtstartDate >= (today - MAX_ALLDAY_LEAD_DAYS days)` would bound the result set to the relevant window. Not flagged as a perf bug (out of v1 scope per CLAUDE.md), but noting it here because it compounds with CR-01: until CR-01 is fixed, many past events with custom alarms (stored as `reminderLeadMinutes = null`) are excluded by the `IS NOT NULL` filter — so the current population is smaller than it will be post-fix. + +**Fix (when needed):** + +```typescript +// Add to allDayRows WHERE: +sql`${calendarEvents.dtstartDate} >= ${new Date(now.getTime() - MAX_ALLDAY_LEAD_DAYS * 24 * 60 * 60 * 1000).toISOString().slice(0, 10)}`, +``` + +--- + +### IN-02: `deriveReminderValue` Has an Unreachable Code Path (Dead Branch) + +**File:** `apps/pwa/src/components/EventForm.tsx:86-88` + +**Issue:** The `deriveReminderValue` function has two branches that produce identical output: + +```typescript +if (presets.has(leadMinutes)) return String(leadMinutes); // preset: "15" +// Off-list positive value — use the numeric string; a synthetic option will be rendered +return String(leadMinutes); // off-list: "15" +``` + +Both the `presets.has()` branch and the fallthrough return `String(leadMinutes)`. The `if` check is dead (the comment acknowledges the two cases differ semantically but the code treats them identically). This obscures that the function intentionally returns the same value for both — the picker then uses the value-set membership check externally to decide whether to render a synthetic option. + +The `if` branch should be removed or the comment should clarify why both arms return the same thing: + +```typescript +function deriveReminderValue(leadMinutes: number | null, isAllDay: boolean): string { + if (leadMinutes === null) return '__none__'; + // Both preset and off-list values are returned as their numeric string. + // The picker uses TIMED/ALLDAY_REMINDER_PRESETS.has() externally to decide + // whether to render a synthetic off-list option. + return String(leadMinutes); +} +``` + +--- + +_Reviewed: 2026-06-14_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: deep_ diff --git a/.planning/phases/11-per-event-reminders/11-VERIFICATION.md b/.planning/phases/11-per-event-reminders/11-VERIFICATION.md new file mode 100644 index 0000000..5e3ab8a --- /dev/null +++ b/.planning/phases/11-per-event-reminders/11-VERIFICATION.md @@ -0,0 +1,162 @@ +--- +phase: 11-per-event-reminders +verified: 2026-06-14T07:38:00Z +status: human_needed +score: 5/5 +overrides_applied: 1 +overrides: + - must_have: "The reminder selector is disabled/hidden for all-day events in the UI" + reason: "Deliberately superseded by decisions D-02/D-03 during discuss/UI-SPEC: the picker SWAPS to day-granularity presets (None / Same day (9 AM) / 1d / 2d / 1wk) instead of being disabled. The scheduler fires at 9 AM local on the computed alert day. Documented in 11-CONTEXT.md (roadmap_amendments section) and authorized by the user. The 9 AM local fire time (NOTIF-06) is retained and now governs day-lead choices." + accepted_by: "luc" + accepted_at: "2026-06-13T00:00:00Z" +human_verification: + - test: "Create a timed event with a 30-minute reminder on the live dev stack using a member that has a connected Fastmail provider, then verify the Fastmail calendar shows a VALARM on the event, and the push fires at T-30." + expected: "Event appears in Fastmail with BEGIN:VALARM / TRIGGER:-PT30M; a push notification arrives 30 minutes before the event." + why_human: "Dev-bypass user 1 has no Fastmail provider configured (needsProviderSetup=true, no member_credentials). End-to-end CalDAV write + VAPID push requires a live Fastmail account. Server schema acceptance and ICS generation are verified by 327 automated tests; only the live Fastmail round-trip cannot be exercised in dev. Tracked in backlog 999.19." +--- + +# Phase 11: Per-Event Reminders — Verification Report + +**Phase Goal:** A user can choose a reminder lead time per event (None / 5m / 10m / 15m / 30m / 1h / 2h / 1d / 2d, default None), serialized as a VALARM on the event, and the push scheduler fires at that exact lead — firing nothing when there is no alarm and never stripping reminders set in other clients. +**Verified:** 2026-06-14T07:38:00Z +**Status:** human_needed +**Re-verification:** No — initial verification + +--- + +## Goal Achievement + +### Observable Truths + +The ROADMAP defines 5 success criteria. SC-5 has an authorized override (all-day swap behavior vs the literal "disabled" wording). + +| # | Truth (Roadmap SC) | Status | Evidence | +|---|---|---|---| +| 1 | User can pick a reminder lead when creating/editing a timed event; choice round-trips to Fastmail as a VALARM | VERIFIED | `eventFieldsSchema` accepts `reminderLeadMinutes`; outboxWorker wires it to `buildVeventString`; 327/327 API tests pass including outboxWorker CAL-13 tests asserting `TRIGGER:-PT15M` in emitted ICS | +| 2 | Editing an event with a reminder set in another client preserves that VALARM — never silently dropped | VERIFIED | `hasExplicitReminder = Object.prototype.hasOwnProperty.call(fields, 'reminderLeadMinutes')` + `extractValarms(rawVevent)` preserve path in outboxWorker UPDATE branch; test `CAL-14 preserve: UPDATE with no reminderLeadMinutes field preserves existing VALARM from rawVevent` passes | +| 3 | A reminder push fires at the event's chosen lead time (e.g. T-30), not a hardcoded 15-min lead | VERIFIED | reminderScheduler reads `reminder_lead_minutes` from DB; per-event fire time: `fireTime = dtstartUtc - reminderLeadMinutes * 60s`; test `NOTIF-04: dispatches a timed event when now is inside the lead-driven fire window (30-min lead)` passes; `humanizeLeadMinutes` drives body from configured lead | +| 4 | An event with no reminder set produces no reminder push | VERIFIED | SQL `WHERE reminderLeadMinutes IS NOT NULL`; timed-0 guard `if (lead === 0) continue`; tests `NOTIF-05: NULL lead → zero dispatches` and `NOTIF-05: timed-0-lead → zero dispatches` pass | +| 5 | All-day event's reminder fires at 9 AM local on alert day; exactly-once across catch-up scans and rescheduled events | PASSED (override) | Override: SC-5 "disabled" wording superseded by D-02/D-03 — picker swaps to day-granularity presets, not disabled. 9 AM local fire and exactly-once are verified: `computeAlertInstantUtc` (DST-correct, 5 DST boundary tests pass); `uid:dtstartMs` dedup; all-day `pruneMs = start-of-next-day` fix; tests `NOTIF-06: all-day 9 AM-local fire` and reschedule re-fire pass. Accepted by luc on 2026-06-13. | + +**Score:** 5/5 (including 1 override) + +--- + +### Deferred Items + +None. All must-haves are verified or covered by an authorized override. + +--- + +### Required Artifacts + +| Artifact | Expected | Status | Details | +|---|---|---|---| +| `apps/api/src/broker/vevent.ts` | VALARM builders, classifier, extractor, computeAlertInstantUtc, extended NewEventParams | VERIFIED | All 5 functions exported: `buildTimedValarm`, `buildAllDayValarm`, `classifyValarms`, `extractValarms`, `computeAlertInstantUtc`; `PRESET_MINUTES`, `AlarmClassification` type; `NewEventParams` extended with `reminderLeadMinutes`, `valarms`, `allDayAlertInstantUtc` | +| `apps/api/tests/broker/vevent.test.ts` | TDD coverage for all 5 units | VERIFIED | 37 tests (per SUMMARY-01 metrics); asserts `TRIGGER:-PT30M`, no `VALUE=TEXT`, absolute `VALUE=DATE-TIME`, DST boundaries (5 cases), `classifyValarms` all 4 kinds, `extractValarms` round-trip | +| `apps/api/src/broker/reminderScheduler.ts` | Variable-lead query, uid:dtstartMs dedup, all-day 9 AM, humanizeLeadMinutes, dropped isShared | VERIFIED | Two-query split (timed + all-day); `humanizeLeadMinutes` exported; `uid:dtstartMs` compound key; `pruneMs` separate field; `setInterval` only (no node-cron); wired in `index.ts` at line 149 | +| `apps/api/tests/broker/reminderScheduler.test.ts` | TDD coverage for variable-lead, dedup, all-day, humanized body | VERIFIED | 28 tests covering NOTIF-04/05/06/D-09; `uid:dtstartMs` dedup across 3 ticks; reschedule re-fire; personal calendar dispatch | +| `apps/api/src/broker/outboxWorker.ts` | reminderLeadMinutes in outboxPayloadSchema, hasExplicitReminder preserve path, buildVeventString wiring | VERIFIED | `reminderLeadMinutes: z.number().int().min(0).nullable().optional()` in `outboxPayloadSchema`; `hasExplicitReminder` guard at line 443; `extractValarms` + `computeAlertInstantUtc` imported and called; both UPDATE and CREATE branches pass correct params to `buildVeventString` | +| `apps/api/tests/broker/outboxWorker.test.ts` | Tests for preserve path, timed VALARM, null clear, all-day DATE-TIME | VERIFIED | 4 new tests: CAL-14 preserve, CAL-13 timed (TRIGGER:-PT15M), CAL-13 clear (no VALARM), CAL-13 all-day (VALUE=DATE-TIME) | +| `apps/api/src/broker/sync.ts` | VALARM → reminderLeadMinutes upsert via classifyValarms | VERIFIED | `classifyValarms` imported; `reminderLeadMinutesValue` derived (preset/offlist → minutes, custom/none → null); written to both `.values()` and `.onDuplicateKeyUpdate()` | +| `apps/api/tests/broker/sync.test.ts` | Tests for VALARM → DB column derivation | VERIFIED | 5 tests: preset TRIGGER:-PT30M → 30, no VALARM → null, absolute DATE-TIME → null, two VALARMs → null, onDuplicateKeyUpdate column present | +| `apps/api/src/broker/expand.ts` | reminderLeadMinutes on CalendarOccurrence, series-level propagation | VERIFIED | `reminderLeadMinutes: number \| null` on `CalendarOccurrence` interface; derived via `classifyValarms(rawVevent)` once per event; set in both non-recurring and recurring occurrence branches | +| `apps/api/tests/broker/expand.test.ts` | Tests for reminderLeadMinutes propagation (D-10) | VERIFIED | 4 tests: non-recurring with 30-min, all-day 0-lead, no VALARM null, recurring series-level inheritance | +| `apps/api/src/routes/events.ts` | reminderLeadMinutes in eventFieldsSchema + GET select | VERIFIED | `reminderLeadMinutes: z.number().int().min(0).nullable().optional()` at line 125; `calendarEvents.reminderLeadMinutes` in GET select at line 181 | +| `apps/pwa/src/api/client.ts` | reminderLeadMinutes on CreateEventPayload + CalendarOccurrence | VERIFIED | `reminderLeadMinutes: number \| null` on `CalendarOccurrence` (required, line 141); `reminderLeadMinutes?: number \| null` on `CreateEventPayload` (optional, line 212) | +| `apps/pwa/src/components/EventForm.tsx` | Reminder `