---
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)))) && (
    <div ...>Custom reminder kept — select a preset to replace it.</div>
  )}
```

---

## 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_