diff --git a/.planning/phases/11-per-event-reminders/11-05-PLAN.md b/.planning/phases/11-per-event-reminders/11-05-PLAN.md new file mode 100644 index 0000000..f7bb6cc --- /dev/null +++ b/.planning/phases/11-per-event-reminders/11-05-PLAN.md @@ -0,0 +1,137 @@ +--- +phase: 11-per-event-reminders +plan: 05 +type: tdd +wave: 4 +gap_closure: true +depends_on: [11-01, 11-02, 11-03, 11-04] +files_modified: + - apps/api/src/broker/vevent.ts + - apps/api/src/broker/expand.ts + - apps/api/src/broker/reminderScheduler.ts + - apps/api/src/routes/events.ts + - apps/api/src/broker/outboxWorker.ts + - apps/pwa/src/api/client.ts + - apps/pwa/src/components/EventForm.tsx + - apps/api/tests/broker/vevent.test.ts + - apps/api/tests/broker/expand.test.ts + - apps/api/tests/broker/reminderScheduler.test.ts + - apps/api/tests/broker/outboxWorker.test.ts + - apps/pwa/src/components/EventForm.test.tsx +autonomous: true +requirements: [CAL-14, NOTIF-05, NOTIF-06] +must_haves: + truths: + - "Editing an event whose only reminder is a custom/absolute/multi-VALARM alarm set in another client preserves that VALARM (the edit payload OMITS reminderLeadMinutes; the outbox preserve path runs)" + - "An occurrence carrying a custom alarm surfaces a distinct custom signal so the edit form initializes the read-only 'Custom (kept)' option rather than 'None'" + - "A same-day all-day reminder push body does NOT read 'Starts in 0 min'" + - "A post-event DURATION trigger (TRIGGER:+PT15M) classifies as custom, not as a 15-min-before lead" + - "reminderLeadMinutes above the UI cap (10080) is rejected by the server schema" + - "The off-list synthetic option + helper text gate on the active (allDay-vs-timed) preset set" + artifacts: + - path: "apps/api/src/broker/expand.ts" + provides: "reminderIsCustom on CalendarOccurrence, derived from classifyValarms kind==='custom'" + contains: "reminderIsCustom" + - path: "apps/pwa/src/components/EventForm.tsx" + provides: "deriveReminderValue returns __custom__ when the occurrence is custom; __custom__ omits the field on save" + contains: "__custom__" +--- + + +Gap-closure for Phase 11 from the code review (.planning/phases/11-per-event-reminders/11-REVIEW.md). Fix the two confirmed blockers (CR-01 silent strip of other-client reminders on edit; CR-02 "Starts in 0 min" all-day push body) and three warnings (WR-01 post-event trigger sign; WR-02 missing server max bound; WR-03 helper-text gating). TDD: write the failing test first for each behavior change, then the fix. + +Read .planning/phases/11-per-event-reminders/11-REVIEW.md for the full findings with file:line. The fixes below are the agreed scope. + + + + + + Task 1 — CR-01: preserve custom/absolute reminders on edit (surface custom signal end-to-end) + apps/api/src/broker/expand.ts, apps/pwa/src/api/client.ts, apps/pwa/src/components/EventForm.tsx, apps/api/tests/broker/expand.test.ts, apps/pwa/src/components/EventForm.test.tsx + + - apps/api/src/broker/expand.ts: CalendarOccurrence interface (~line 78-94) and the derivation at ~line 244-246 where `alarmClass = classifyValarms(rawVevent)` already computes `.kind`. NOTE: the GET handler (routes/events.ts:231) calls expandOccurrences(rawVevent, ...) and does NOT pass the DB column — expand.ts is the form-facing source, so the fix lives here, no DB migration. + - apps/pwa/src/components/EventForm.tsx: deriveReminderValue (~line 83), the edit-load setReminderValue (~line 321), the payload mapping (~line 467-477) — the `__custom__` → omit branch already exists but is dead because the occurrence never signals custom. + - apps/pwa/src/api/client.ts: CalendarOccurrence (the reminderLeadMinutes mirror added in 11-04). + + + Surface the custom-alarm signal so the form can preserve it: + 1. expand.ts — add `reminderIsCustom: boolean` to the CalendarOccurrence interface (document: true when the master event's alarm is custom/absolute/multi-VALARM — not reducible to a single lead). Set it from the already-computed classification: `reminderIsCustom = alarmClass.kind === 'custom'`. Propagate it to EVERY occurrence branch alongside reminderLeadMinutes (series-level, D-10) — same propagation sites as reminderLeadMinutes. + 2. client.ts — mirror `reminderIsCustom: boolean` on CalendarOccurrence (atomic mirror of expand.ts, like reminderLeadMinutes). + 3. EventForm.tsx — extend deriveReminderValue to accept the custom flag and return `'__custom__'` when it is true (precedence: custom → '__custom__'; else null → '__none__'; else preset/synthetic). Pass `occurrence?.reminderIsCustom ?? false` at the edit-load call site. This makes the existing `__custom__` → omit-field branch live: editing a custom-alarm event now leaves the field ABSENT from the payload, so the outboxWorker preserve path (extractValarms) keeps the original VALARM (D-08). + Do NOT change the outboxWorker preserve logic — it already preserves on absent field; the bug was that the form never produced an absent field for custom alarms. + + + cd apps/api && pnpm exec vitest run tests/broker/expand.test.ts; cd ../pwa && pnpm exec vitest run src/components/EventForm.test.tsx; pnpm --filter @familysync/api exec tsc --noEmit; pnpm --filter @familysync/pwa exec tsc --noEmit + + + - RED first: expand.test.ts asserts a rawVevent with an absolute (DATE-TIME) trigger OR two VALARMs yields `reminderIsCustom: true` and `reminderLeadMinutes: null`; a single relative preset yields `reminderIsCustom: false`. + - RED first: EventForm.test.tsx asserts edit mode with `occurrence.reminderIsCustom=true` initializes the picker to the read-only "Custom (kept)" option (value `__custom__`) and the submitted payload OMITS reminderLeadMinutes (`Object.prototype.hasOwnProperty.call(payload,'reminderLeadMinutes') === false`). + - Both typechecks clean; existing tests still green. + + A custom reminder set in Apple Calendar/Fastmail survives an edit-and-save round-trip from the PWA (CAL-14 / Pitfall 1). + + + + Task 2 — CR-02: all-day-aware push body (no "Starts in 0 min") + apps/api/src/broker/reminderScheduler.ts, apps/api/tests/broker/reminderScheduler.test.ts + reminderScheduler.ts humanizeLeadMinutes (~line 86) and its call site (~line 292) — the scheduler already knows allDay vs timed (two-query split). + + Make the push body allDay-aware. Extend humanizeLeadMinutes to take an `isAllDay` flag (or branch at the call site). For all-day: lead 0 → "Today"; 1440 → "Tomorrow"; 2880 → "In 2 days"; 10080 → "In 1 week"; other → `In ${Math.round(lead/1440)} days`. For timed: keep the existing wording. Pass the correct flag from the all-day vs timed scan branch at line 292. + + cd apps/api && pnpm exec vitest run tests/broker/reminderScheduler.test.ts + + - RED first: a test asserts the all-day same-day (lead 0) push body is NOT "Starts in 0 min" (e.g. equals "Today"); all-day 1440 → "Tomorrow". + - Timed-event bodies unchanged (existing assertions still pass). + + All-day reminder pushes read sensibly; no "Starts in 0 min". + + + + Task 3 — WR-01: post-event triggers classify as custom (drop Math.abs sign-flip) + apps/api/src/broker/vevent.ts, apps/api/tests/broker/vevent.test.ts + vevent.ts classifyValarms — the `leadMinutes = Math.round(Math.abs(dur.toSeconds()) / 60)` line. + + In classifyValarms, a positive-duration trigger means the alarm fires AFTER the event (e.g. TRIGGER:+PT15M, valid in Apple/Outlook) and is not a before-lead. If `dur.toSeconds() > 0`, return `{ kind: 'custom' }`. Otherwise compute `leadMinutes = Math.round(-dur.toSeconds() / 60)` (before-event lead; 0 stays 0). Keep preset-vs-offlist classification for the non-positive case. + + cd apps/api && pnpm exec vitest run tests/broker/vevent.test.ts + + - RED first: a VEVENT with TRIGGER:+PT15M (or VALUE-relative positive) classifies as `{kind:'custom'}`, NOT preset/offlist 15. + - Existing negative-trigger preset/offlist tests still pass. + + Post-event alarms are treated as custom and preserved, not mis-rendered as a 15-min-before lead. + + + + Task 4 — WR-02: server-side upper bound on reminderLeadMinutes + apps/api/src/routes/events.ts, apps/api/src/broker/outboxWorker.ts, apps/api/tests/broker/outboxWorker.test.ts + eventFieldsSchema in events.ts (the `reminderLeadMinutes: z.number().int().min(0).nullable().optional()` line ~125) and outboxPayloadSchema in outboxWorker.ts (~line 106). + Add `.max(10080)` to the reminderLeadMinutes zod field in BOTH eventFieldsSchema and outboxPayloadSchema (keep min(0), nullable, optional). 10080 = 1 week, the UI cap. + cd apps/api && pnpm exec vitest run tests/broker/outboxWorker.test.ts; cd apps/api && pnpm exec vitest run + + - RED first: a payload with reminderLeadMinutes=10081 fails schema validation (both schemas); 10080 passes; null/absent still valid. + + The server (the real trust boundary) bounds reminderLeadMinutes to the UI range. + + + + Task 5 — WR-03: gate off-list option + helper text on the active preset set + apps/pwa/src/components/EventForm.tsx, apps/pwa/src/components/EventForm.test.tsx + EventForm.tsx synthetic-option blocks (~line 971-975 timed, ~999-1004 all-day) and the helper-text condition (~line 1018) — they reference ALLDAY_REMINDER_PRESETS / TIMED_REMINDER_PRESETS; the gating must use the set matching the current `allDay` mode, not a fixed/opposite set. + Use the active preset set (`allDay ? ALLDAY_REMINDER_PRESETS : TIMED_REMINDER_PRESETS`) consistently when deciding whether the current reminderValue is off-list (synthetic option) and whether to show the helper text. Ensure a timed event with a native-client 10080-min lead renders the synthetic option AND its helper text. + cd apps/pwa && pnpm exec vitest run src/components/EventForm.test.tsx; pnpm --filter @familysync/pwa exec tsc --noEmit + + - RED first: a test asserts a timed event with reminderLeadMinutes=10080 shows the synthetic off-list option AND the helper text (not suppressed by the all-day preset set). + + Off-list synthetic option and helper text gate correctly per allDay vs timed. + + + + + +- Full CI fast-check parity green: `pnpm -r typecheck`, `pnpm test` (API, with DB env), `pnpm --filter @familysync/pwa exec vitest run`, pwa eslint, `pnpm format:check`, `pnpm md:lint`. +- CR-01 is the headline: the new RED test must prove a custom alarm survives the edit round-trip (payload omits the field). + + + +Create .planning/phases/11-per-event-reminders/11-05-SUMMARY.md when done. Record each fix, the new tests (RED→GREEN), and confirm the full gate is green. +