diff --git a/.planning/phases/11-per-event-reminders/11-02-SUMMARY.md b/.planning/phases/11-per-event-reminders/11-02-SUMMARY.md new file mode 100644 index 0000000..a448e45 --- /dev/null +++ b/.planning/phases/11-per-event-reminders/11-02-SUMMARY.md @@ -0,0 +1,164 @@ +--- +phase: 11-per-event-reminders +plan: "02" +subsystem: api/broker +tags: [scheduler, tdd, reminders, variable-lead, dedup, humanize, all-day, notif-04, notif-05, notif-06] +dependency_graph: + requires: + - computeAlertInstantUtc (Plan 11-01, vevent.ts) + provides: + - humanizeLeadMinutes + - runReminderCheck (variable-lead, uid:dtstartMs dedup, all-day branch) + - startReminderScheduler (unchanged) + affects: + - apps/api/src/broker/reminderScheduler.ts (Plan 11-03+ consumer if any) +tech_stack: + added: [] + patterns: + - Two-query split (timed vs all-day) to avoid mixing SQL filter semantics + - Per-event JS fire-time check within a wide pre-filter SQL window (T-11-04 cap) + - uid:dtstartMs compound dedup key — identity stable across consecutive ticks, re-fires on reschedule + - pruneMs separate from dtstartMs — all-day events need end-of-event-day as prune boundary + - humanizeLeadMinutes branch order: check < 120 before hours division (90-min = 1 hour, not 2) +key_files: + created: [] + modified: + - apps/api/src/broker/reminderScheduler.ts + - apps/api/tests/broker/reminderScheduler.test.ts +decisions: + - "D-PRUNE-SPLIT: Introduced separate pruneMs field alongside dtstartMs in the byKey map. For timed events pruneMs = dtstartUtc (prune when event starts). For all-day events pruneMs = start-of-next-day UTC (prune after event date), because UTC midnight of the event date is always before the 9 AM fire time — storing dtstartMs as the prune value caused immediate eviction after tick 1." + - "D-TWO-QUERY: Split the single DB query into timed + all-day separate queries. This avoids ambiguous WHERE predicates (e.g. reminder_lead_minutes > 0 is wrong for all-day where 0 = same-day) and keeps SQL pre-filter logic readable per event type." + - "D-WIDE-PREFILTER: SQL pre-filter uses wide window (now + MAX_LEAD_MINUTES / MAX_ALLDAY_LEAD_DAYS); per-event JS check narrows to exact 60s catch-up window. Avoids complex MariaDB timezone arithmetic for all-day, keeps correctness in JS." +metrics: + duration_minutes: 13 + completed_date: "2026-06-14" + tasks_completed: 3 + files_modified: 2 +--- + +# Phase 11 Plan 02: Variable-Lead Reminder Scheduler Summary + +Generalized the reminder scheduler from a fixed shared-event 15-min scan to a per-event variable-lead scheduler: uid:dtstartMs compound dedup, dropped isShared restriction, all-day 9 AM-local branch (via computeAlertInstantUtc from Plan 11-01), NULL-vs-0 guard, and humanized push body. + +## Tasks Completed + +| Task | Description | Commit | +|------|-------------|--------| +| RED | Failing tests: variable-lead, uid:dtstartMs dedup, NULL-vs-0, personal calendar | 9635aa9 | +| GREEN Task 1 | Variable-lead window, uid:dtstartMs dedup, drop isShared/allDay restrictions, timed-0 skip | 62d3f58 | +| Task 2 | humanizeLeadMinutes tests (8 bucket cases) + body dispatch assertion | 57f9d67 | +| Task 3 | All-day 9 AM-local tests + all-day prune-boundary fix (NOTIF-06) | 0dc227a | + +## New Exported Symbols + +| Symbol | File | Description | +|--------|------|-------------| +| `humanizeLeadMinutes(leadMinutes)` | reminderScheduler.ts | Maps minutes → human string: `< 60` → `N min`; `< 120` → `1 hour`; `< 1440` → `N hours`; `< 2880` → `1 day`; else `N days`. Branch order prevents 90-min rounding to 2 hours. | + +## Key Changes to runReminderCheck + +### SQL Query: Two queries replacing one + +**Before:** Single query with `isShared=true`, `allDay=false`, fixed `(now, now+16min]` window. + +**After (timed query):** +- Removed `eq(calendars.isShared, true)` — personal events fire (NOTIF-05) +- Removed `eq(calendarEvents.allDay, false)` — handled separately +- Added `reminderLeadMinutes IS NOT NULL` (NOTIF-05) +- Changed window to `(now, now + MAX_LEAD_MINUTES]` (2880 min) as a pre-filter + +**After (all-day query):** +- `allDay=true`, `reminderLeadMinutes IS NOT NULL`, `dtstartDate <= today + 7 days` +- Alert time computed in JS via `computeAlertInstantUtc(dtstartDate, leadDays, serverTz)` + +### JS Filter: Per-event fire-time check + +- **Timed:** `fireTime = dtstartUtc - lead * 60s`. Fire if `fireTime ∈ (now - 60s, now]`. Skip if `lead === 0` (D-06). +- **All-day:** `alertInstant = computeAlertInstantUtc(dtstartDate, lead/1440, serverTz)`. Fire if `alertInstant ∈ (now - 60s, now]`. + +### Dedup Key: uid → uid:dtstartMs + +- Key format: `` `${uid}:${dtstartMs}` `` +- Timed events: `dtstartMs = dtstartUtc.getTime()` +- All-day events: `dtstartMs = Date.UTC(y, m-1, d)` (UTC midnight of event date) +- Reschedule detection: same uid with new dtstart gets a new compound key → re-fires + +### Prune Boundary (New Field: pruneMs) + +- Timed: `pruneMs = dtstartMs` (same as before — prune when event starts) +- All-day: `pruneMs = Date.UTC(y, m-1, d+1)` (end-of-event-day) — avoids immediate prune since UTC midnight of event date is before the 9 AM fire instant + +### Body: humanizeLeadMinutes + +``` +body: humanizeLeadMinutes(event.reminderLeadMinutes) +``` +Driven by the DB-stored configured lead (D-09 ground truth), not the live minutes-to-start delta. + +## humanizeLeadMinutes Bucket Table + +| Input (min) | Output | +|-------------|--------| +| 5–59 | `Starts in N min` | +| 60–119 | `Starts in 1 hour` | +| 120–1439 | `Starts in N hours` | +| 1440–2879 | `Starts in 1 day` | +| 2880+ | `Starts in N days` | + +90 min → `Starts in 1 hour` (not 2 hours — the `< 120` check comes before the hours division). + +## Requirements Satisfied + +| Req ID | Behavior | Test | +|--------|----------|------| +| NOTIF-04 | Fires at T-lead for 30-min lead event; not-fired outside window | `NOTIF-04: dispatches a timed event when now is inside the lead-driven fire window (30-min lead)` | +| NOTIF-05 | NULL lead → no push; timed 0-lead → no push; personal → dispatch | 3 tests in `variable-lead, NULL-vs-0, personal calendar` | +| NOTIF-06 | uid:dtstartMs dedup (once/3 ticks); reschedule re-fires; all-day 9 AM | 5 tests covering dedup + all-day | +| D-09 | Humanized body: 1440-min lead → "Starts in 1 day" | `dispatched notification body is humanized from configured lead` | + +## Verification Results + +- `pnpm --filter @familysync/api exec vitest run tests/broker/reminderScheduler.test.ts`: 28/28 PASS +- `pnpm --filter @familysync/api exec vitest run` (full suite): 314/314 PASS +- `pnpm --filter @familysync/api exec tsc --noEmit`: CLEAN (0 errors) +- `grep 'setInterval' reminderScheduler.ts`: retained (no node-cron) + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] All-day dedup immediate prune via UTC-midnight dtstartMs** +- **Found during:** Task 3 GREEN — all-day dedup test failed: dispatched twice across 2 ticks +- **Issue:** `sentReminders` stored `dtstartMs = Date.UTC(y, m-1, d)` (UTC midnight of event date) as the prune value. By the time a 9 AM reminder fires, this value is already `<= now`, so the CR-01 prune loop evicted the entry in the same tick. The next tick re-entered the fire window and dispatched again. +- **Fix:** Introduced separate `pruneMs` field. For all-day events, `pruneMs = Date.UTC(y, m-1, d+1)` (start-of-next-day), ensuring the entry persists through the full event date. For timed events, `pruneMs = dtstartMs` (unchanged behavior). +- **Files modified:** apps/api/src/broker/reminderScheduler.ts +- **Commit:** 0dc227a + +**2. [Rule 3 - Blocking] Test mock needed two-query support** +- **Found during:** Task 1 GREEN — existing `makeSelectMock` assumed two `innerJoin` calls (calendars + pushSubscriptions). New implementation uses a single `innerJoin` per query but makes two queries. +- **Fix:** Replaced `vi.mocked(db.select).mockReturnValue(...)` pattern with `mockTwoQueries(db, timedRows, allDayRows)` that sequences two `mockReturnValueOnce` calls to correctly simulate the timed vs all-day query split. +- **Files modified:** apps/api/tests/broker/reminderScheduler.test.ts +- **Commit:** 62d3f58 + +## Known Stubs + +None. All implemented functions are fully wired and produce real output. No placeholder values or TODO markers. + +## Threat Flags + +None. No new network endpoints, auth paths, file access patterns, or schema changes introduced. The only behavioral expansion (personal-calendar reminders) matches T-11-03 (accepted risk per threat register — authorized requirement NOTIF-05 corollary, body carries only event title + relative time). + +## Self-Check: PASSED + +Files exist: +- FOUND: apps/api/src/broker/reminderScheduler.ts +- FOUND: apps/api/tests/broker/reminderScheduler.test.ts +- FOUND: .planning/phases/11-per-event-reminders/11-02-SUMMARY.md + +Commits exist: +- 9635aa9: RED test commit (test(11-02)) +- 62d3f58: GREEN Task 1 (feat(11-02)) +- 57f9d67: Task 2 (feat(11-02)) +- 0dc227a: Task 3 (feat(11-02)) + +Exports verified: `humanizeLeadMinutes` exported from reminderScheduler.ts, `computeAlertInstantUtc` imported from vevent.ts (Plan 11-01 artifact).