chore: archive v1.1 phase directories to milestones/v1.1-phases/
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Lucas Berger
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
a2890d1542
commit
c7955a46b9
@@ -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)))) && (
|
||||
<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_
|
||||
Reference in New Issue
Block a user