docs(11): code review (2 blockers, 3 warnings) + goal verification (5/5, 1 live-check deferred)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Lucas Berger
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
213f2547cf
commit
89132be3be
@@ -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_
|
||||||
@@ -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 `<select id="event-reminder">`, allDay swap, edit pre-population, payload mapping | VERIFIED | `id="event-reminder"` at line 953; allDay conditional option set swap at lines 962–1014; `handleAllDayToggle` resets `setReminderValue('__none__')` at line 359; `reminderPayload` assembled and spread into `payload` at line 494; `deriveReminderValue` drives edit pre-population from `occurrence.reminderLeadMinutes` |
|
||||||
|
| `apps/pwa/src/components/EventForm.test.tsx` | Component tests: default None, allDay swap + reset, edit pre-population, payload mapping | VERIFIED | 54 PWA tests pass including Phase 11 describe block: D-01 default None, D-02/D-03 allDay swap + reset, edit pre-population (30 → "30 minutes before", 1440 all-day → "1 day before (9 AM)", off-list 45 → synthetic), payload mapping (None→null, preset→integer, Custom-kept→field absent) |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Key Link Verification
|
||||||
|
|
||||||
|
| From | To | Via | Status | Details |
|
||||||
|
|---|---|---|---|---|
|
||||||
|
| EventForm reminder select | `CreateEventPayload.reminderLeadMinutes` | submit handler maps `reminderValue` → `reminderPayload` → spread into `payload` | WIRED | `reminderPayload = { reminderLeadMinutes: null \| parsed }` assembled at lines 467–477; spread at line 494 |
|
||||||
|
| `edit-mode load` | `occurrence.reminderLeadMinutes` | `deriveReminderValue` called on mount at line 321 | WIRED | `setReminderValue(deriveReminderValue(occurrence?.reminderLeadMinutes ?? null, occAllDay))` |
|
||||||
|
| `outboxWorker` UPDATE branch | `extractValarms(rawVevent)` | `hasExplicitReminder` gate at line 493 | WIRED | `if (!hasExplicitReminder && freshEtagRows.length > 0 && freshEtagRows[0].rawVevent) { valarmsToPreserve = extractValarms(...) }` |
|
||||||
|
| `sync.ts` upsert | `classifyValarms(rawVevent)` | `reminderLeadMinutesValue` derivation at line 137 | WIRED | `const alarmClass = classifyValarms(obj.data as string)` → written to both values() and onDuplicateKeyUpdate() |
|
||||||
|
| `GET /api/events` select | `expandOccurrences → CalendarOccurrence.reminderLeadMinutes` | `calendarEvents.reminderLeadMinutes` in select + `classifyValarms(rawVevent)` in expandOccurrences | WIRED | Select at events.ts line 181; derivation in expand.ts line 245 |
|
||||||
|
| `runReminderCheck` timed query | `calendarEvents.reminderLeadMinutes` | SQL `WHERE reminder_lead_minutes IS NOT NULL` | WIRED | `sql\`${calendarEvents.reminderLeadMinutes} IS NOT NULL\`` at reminderScheduler.ts line 143 |
|
||||||
|
| `notification.body` | `humanizeLeadMinutes` | Function call replacing hardcoded string | WIRED | `body: humanizeLeadMinutes(event.reminderLeadMinutes)` at reminderScheduler.ts line 292 |
|
||||||
|
| `startReminderScheduler` | `index.ts` server startup | Import + call inside `isMainModule` guard | WIRED | `import { startReminderScheduler }` at index.ts:18; `startReminderScheduler()` at index.ts:149 |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Data-Flow Trace (Level 4)
|
||||||
|
|
||||||
|
| Artifact | Data Variable | Source | Produces Real Data | Status |
|
||||||
|
|---|---|---|---|---|
|
||||||
|
| `EventForm.tsx` reminder select | `reminderValue` (state) | `deriveReminderValue(occurrence.reminderLeadMinutes)` on mount; user interaction | Yes — from DB-backed occurrence or user selection | FLOWING |
|
||||||
|
| `reminderScheduler.ts` `runReminderCheck` | `calendarEvents.reminderLeadMinutes` | DB column (populated by sync.ts upsert from native VALARMs, or set by outboxWorker on create/edit) | Yes — real DB query with `IS NOT NULL` filter | FLOWING |
|
||||||
|
| `outboxWorker.ts` preserve path | `valarmsToPreserve` | `extractValarms(freshEtagRows[0].rawVevent)` — reads live rawVevent from CalDAV GET | Yes — live VALARM components re-attached verbatim | FLOWING |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Behavioral Spot-Checks
|
||||||
|
|
||||||
|
| Behavior | Command | Result | Status |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `buildTimedValarm(30)` produces `TRIGGER:-PT30M`, no `VALUE=TEXT` | Asserted in vevent.test.ts line 208–212 (vitest run 37/37) | PASS | PASS |
|
||||||
|
| `classifyValarms` returns `{kind:'offlist', leadMinutes:45}` for `TRIGGER:-PT45M` | Asserted in vevent.test.ts line 348 (vitest run) | PASS | PASS |
|
||||||
|
| `computeAlertInstantUtc('2026-06-15', 0, 'America/New_York')` → `2026-06-15T13:00:00.000Z` | Asserted in vevent.test.ts DST tests (vitest run) | PASS | PASS |
|
||||||
|
| Full API suite (327 tests) | `DB_HOST=127.0.0.1 pnpm --filter @familysync/api exec vitest run` | 327/327 PASS | PASS |
|
||||||
|
| Full PWA suite (201 tests) | `pnpm --filter @familysync/pwa exec vitest run` | 201/201 PASS | PASS |
|
||||||
|
| API typecheck | `pnpm --filter @familysync/api exec tsc --noEmit` | 0 errors | PASS |
|
||||||
|
| PWA typecheck | `pnpm --filter @familysync/pwa exec tsc --noEmit` | 0 errors | PASS |
|
||||||
|
| No `node-cron` in reminderScheduler | `grep node-cron reminderScheduler.ts` | no match | PASS |
|
||||||
|
| `startReminderScheduler` wired in index.ts | `grep startReminderScheduler apps/api/src/index.ts` | lines 18, 149 | PASS |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Probe Execution
|
||||||
|
|
||||||
|
No probe scripts declared or applicable for this phase.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Requirements Coverage
|
||||||
|
|
||||||
|
| REQ-ID | Source Plan | Description | Status | Evidence |
|
||||||
|
|---|---|---|---|---|
|
||||||
|
| CAL-13 | 11-01, 11-03, 11-04 | User picks reminder lead; choice serialized as VALARM on event written to Fastmail | SATISFIED | `eventFieldsSchema` field; outboxWorker CREATE/UPDATE wiring; `buildTimedValarm`/`buildAllDayValarm`; EventForm picker; 327 tests pass |
|
||||||
|
| CAL-14 | 11-01, 11-03, 11-04 | Editing an event preserves existing VALARM — never silently stripped | SATISFIED | `hasExplicitReminder` absent-vs-null sentinel; `extractValarms` preserve path; CAL-14 test passes; `__custom__` → field omitted from payload → server preserves |
|
||||||
|
| NOTIF-04 | 11-02 | Reminder fires at event's chosen lead time, not hardcoded 15-min | SATISFIED | Per-event `fireTime = dtstartUtc - lead * 60s`; `NOTIF-04` test passes; `humanizeLeadMinutes` body from DB lead |
|
||||||
|
| NOTIF-05 | 11-02 | No reminder set → no push | SATISFIED | `IS NOT NULL` SQL filter; timed-0 skip `if (lead === 0) continue`; `NOTIF-05` NULL and timed-0 tests pass; personal calendar restriction dropped |
|
||||||
|
| NOTIF-06 | 11-01, 11-02 | All-day fires at 9 AM local; exactly-once across catch-up and reschedule | SATISFIED | `computeAlertInstantUtc` (DST-correct); `uid:dtstartMs` dedup; `pruneMs = start-of-next-day` for all-day; all-day 9 AM and reschedule tests pass |
|
||||||
|
|
||||||
|
No orphaned requirements for Phase 11. REQUIREMENTS.md traceability table shows CAL-13, CAL-14, NOTIF-04, NOTIF-05, NOTIF-06 all mapped to Phase 11; all plans' `requirements` fields cover these IDs completely with no gaps or extras.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Anti-Patterns Found
|
||||||
|
|
||||||
|
| File | Line | Pattern | Severity | Impact |
|
||||||
|
|---|---|---|---|---|
|
||||||
|
| `apps/pwa/src/components/EventForm.tsx` | 701, 1086, 1121, 1135 | `placeholder=` | Info | HTML input placeholder attributes (not stub indicators). Normal UI copy. No impact. |
|
||||||
|
|
||||||
|
No `TBD`, `FIXME`, `XXX`, `return null`, empty handlers, or stub patterns found in any Phase 11 modified file.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Human Verification Required
|
||||||
|
|
||||||
|
#### 1. Live end-to-end reminder round-trip (Fastmail + push)
|
||||||
|
|
||||||
|
**Test:** Using a household member account with a connected Fastmail provider (not the dev-bypass user 1), create a timed event with a 30-minute reminder. Open the event in the PWA to confirm the reminder shows "30 minutes before". Wait for the push notification to fire at T-30. Then open the same event in Fastmail Web or Apple Calendar and confirm the VALARM is present.
|
||||||
|
|
||||||
|
**Expected:** (a) PWA edit form shows "30 minutes before" pre-populated. (b) A push notification with body "Starts in 30 min" arrives ~30 minutes before event start. (c) Fastmail / Apple Calendar shows a reminder on the event.
|
||||||
|
|
||||||
|
**Why human:** Dev-bypass user (id 1) has no Fastmail provider configured (`needsProviderSetup=true`, empty `member_credentials`/`calendars`). A live CalDAV PUT (outbox → Fastmail) and a real VAPID push to a subscribed device cannot be exercised without a provisioned provider. All server-side paths are validated by 327 automated tests and a route-mocked Playwright smoke. Only the live Fastmail round-trip and real device push require a human + live device. Tracked in backlog 999.19.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Gaps Summary
|
||||||
|
|
||||||
|
No gaps. All 5 ROADMAP success criteria are verified (1 with an authorized override for the deliberate all-day behavior evolution from "disabled" to "preset swap"). All 327 API tests and 201 PWA tests pass. All typechecks clean. No debt markers. The one item in the `human_verification` section is a dev-environment caveat (no live Fastmail provider in dev), not an implementation gap.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
_Verified: 2026-06-14T07:38:00Z_
|
||||||
|
_Verifier: Claude (gsd-verifier)_
|
||||||
Reference in New Issue
Block a user