From 91feb2de3f0ea17310dd3a288f7526bfd4da1916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 17 May 2026 23:44:59 +0200 Subject: [PATCH] docs(planning): v0.5.4 server-coord report and six-axis QA findings Captures the paper trail for the iOS-coordination PR #190 admin merge plus the parallel review pass that surfaced the v1.4.38.2 hotfix bundle: - Senior-dev / security / UX / product-lead / specialist / simplifier finding reports. - V054-server-apns-mood report carried over from the iOS-coord agent's working notes. --- .../V054-QA-product-lead-findings.md | 282 +++++++++++++++ .../ios-coord/V054-QA-security-findings.md | 320 ++++++++++++++++++ .../ios-coord/V054-QA-senior-dev-findings.md | 167 +++++++++ .../ios-coord/V054-QA-simplifier-findings.md | 86 +++++ .../ios-coord/V054-QA-specialist-findings.md | 187 ++++++++++ .planning/ios-coord/V054-QA-ux-findings.md | 222 ++++++++++++ .../ios-coord/V054-server-apns-mood-report.md | 126 +++++++ 7 files changed, 1390 insertions(+) create mode 100644 .planning/ios-coord/V054-QA-product-lead-findings.md create mode 100644 .planning/ios-coord/V054-QA-security-findings.md create mode 100644 .planning/ios-coord/V054-QA-senior-dev-findings.md create mode 100644 .planning/ios-coord/V054-QA-simplifier-findings.md create mode 100644 .planning/ios-coord/V054-QA-specialist-findings.md create mode 100644 .planning/ios-coord/V054-QA-ux-findings.md create mode 100644 .planning/ios-coord/V054-server-apns-mood-report.md diff --git a/.planning/ios-coord/V054-QA-product-lead-findings.md b/.planning/ios-coord/V054-QA-product-lead-findings.md new file mode 100644 index 000000000..e6768fd32 --- /dev/null +++ b/.planning/ios-coord/V054-QA-product-lead-findings.md @@ -0,0 +1,282 @@ +# V0.5.4 — Product-Lead QA Findings (PR #190, v1.4.38.1) + +**Scope:** `4049c6c7^..c4f2e0bc` on `main` — APNs category + MOOD_REMINDER +event + CHANGELOG v1.4.38.1. +**Reviewer lens:** Product-Lead (positioning, UX coherence, Marc-Voice, +PII gate, iOS / self-hoster split, v1.5 carry-forward). +**Verdict:** GO with **2 High-severity follow-ups** scheduled into v1.5 +P1 (UI surface + locale parity). No Strategic-block. + +--- + +## Severity scale + +- **Strategic-block** — must not ship / must roll back. +- **High** — ship is OK; close inside the next release window (v1.5 P1). +- **Medium** — schedule for the next polishing slot; no urgency. +- **Low** — note, opportunistic fix. + +--- + +## Findings + +### 1. Positioning fit — Mood reminder vs. existing Coach / Insights / Briefing surfaces + +**Severity:** Low (positioning is sound). + +The MOOD_REMINDER feature is a **capture-prompt**, not an **insight**. +It nudges the user to enter raw data into the Stimmung tab so the +existing AI Coach + daily-briefing + correlations have inputs to chew +on. That's the *opposite* of duplication — without a daily capture +event the mood column on the correlations grid stays sparse, which +directly degrades the Coach's "evidence-grounded, multi-signal" +positioning (`feedback_ai_insights_differentiator.md`). + +Overlap checked against the three adjacent surfaces: + +| Surface | Trigger | Output | Overlap? | +|---|---|---|---| +| Daily briefing (`/insights`) | Request-time on page load | AI narrative | No — pulls *from* mood entries; doesn't request them | +| Med-reminder push | Per-schedule windows | Action buttons (Take / Snooze / Skip) | No — different event payload, different category | +| Compliance / anomaly pushes | Threshold-driven | Alert | No — reactive vs. the proactive 22:00 prompt | + +The Coach + Insights are unchanged. This release ships a **capture +funnel**, not a new differentiator-class feature. + +--- + +### 2. Daily 22:00 slot — conflict analysis + +**Severity:** Low. + +Existing scheduled windows reviewed in `reminder-worker.ts:118–193`: + +| Slot (Europe/Berlin) | Job | +|---|---| +| every 15 min | `medication-reminder-check` (per-user schedule windows; user's own med times decide actual fires) | +| every 60 min | Withings sync | +| 02:00 / 02:05 / 02:10 / 02:15 / 02:20 / 02:25 | Insights status jobs | +| 03:00 (Sun) | Data backup | +| 03:00 / 03:15 / 03:45 / 04:00 | Cleanup + drain + feedback aggregator | +| **22:00 local-tz (user-individual)** | **NEW** MOOD_REMINDER | + +22:00 is a clean choice: + +- **No collision** with the medication-reminder cron — that one is + schedule-driven (windows the user defined), not wall-clock-driven. + A user with a med slot at 22:00 *and* mood opt-in receives two + pushes that thread differently (`threadId = MEDICATION_REMINDER` vs. + `threadId = MOOD_REMINDER`), so iOS groups them separately. No + perceived noise. +- **No collision** with the daily briefing — briefing is request-time + on `/insights` page-mount, not push-triggered. +- **Wind-down slot** is research-aligned (Apple Health, Withings, Oura + all default mood / sleep prompts into the 21:00–22:30 window). The + inline comment captures this rationale (`mood-reminder.ts:30–35`). + +**One subtle item to flag for v1.5:** the 22:00 boundary is hard-coded +(`MOOD_REMINDER_LOCAL_HOUR = 22`). Some users will want 20:00 or +21:30. Add to v1.5 backlog as a Medium item (see §7). + +--- + +### 3. Opt-in default — "user controls everything" check + +**Severity:** none — fully respected. + +The feature is **double-gated**: + +- `EVENT_DEFAULT_ENABLED.MOOD_REMINDER = false` (per-event policy in + `types.ts:51`). +- `User.mood_reminder_enabled = false` by default (migration + `0069_v054_mood_reminder/migration.sql:16`). + +Either gate suffices to block dispatch. A fresh user gets **zero** +mood pushes until they explicitly opt in. This matches the +`PERSONAL_RECORD` pattern set in v1.4.25 and respects the +"user-controls-everything" instinct without forcing an onboarding +roadblock. + +The double-gate also means there is **no rollback risk**: if the +v1.5 UI ships broken, the worst case is "the toggle does nothing" +— the dispatcher stays silent. + +--- + +### 4. Marc-Voice review of DE/EN copy (and the four sibling locales) + +**Severity:** Low (DE/EN ship-ready; one weak FR string). + +| Locale | Title | Body | Verdict | +|---|---|---|---| +| de | "Stimmung erfassen" | "Wie geht es dir heute?" | **Marc-Voice clean** — direct, du-form, no AI mention, reads as Marc's authorship | +| en | "Log your mood" | "How are you feeling today?" | **Marc-Voice clean** — imperative title, conversational body | +| es | "Registrar el ánimo" | "¿Cómo te sientes hoy?" | OK | +| fr | "Enregistrer votre humeur" | "Comment vous sentez-vous aujourdhui ?" | **Typo: `aujourdhui` missing apostrophe** — should be `aujourd'hui`. Low-severity copy bug, fix in next release | +| it | "Registra il tuo umore" | "Come ti senti oggi?" | OK | +| pl | "Zapisz nastrój" | "Jak się dzisiaj czujesz?" | OK | + +No AI / Claude / agent / wave / phase language anywhere. No PII. No +jargon. All six locales pass the `feedback_marc_voice_english.md` +test. + +**Low-severity fix-list:** correct French apostrophe (`aujourd'hui`) +in `messages/fr.json` line 2994. + +--- + +### 5. CHANGELOG v1.4.38.1 entry + +**Severity:** Medium (one user-facing-jargon nit). + +Reviewed against `feedback_marc_voice_english.md` + +`feedback_no_pii_in_user_facing.md`: + +- **PII:** clean. No personal name, no health figures, no medication + references, no measurement counts. +- **Marc-Voice:** clean — reads as Marc's authorship, English, no + AI/agent/phase/wave language. +- **User-readability:** generally good. Two technical-jargon + spots that a self-hoster who hasn't read the schema diff won't + parse: + - `EVENT_DEFAULT_ENABLED` (line 27) — implementation symbol leaking + into release notes. Reword to "default-off at the event-policy + layer" or similar. + - `aps.mutable-content = 1` (line 14) — operator-facing detail; + fine to keep since the rest of the bullet is operator-scoped, but + flag it. +- **Self-hoster context:** the "iOS v0.5.4 push-notification + coordination" header tells self-hosters who don't run the iOS app + that this release is iOS-coordination scaffolding. The + Compatibility section adds the "older iOS builds render plain + alert" sentence which closes the loop. **Sufficient — no action + needed** for non-iOS self-hosters. + +**Medium fix:** swap the one identifier-leak in §Added for a +plain-English phrasing in the next release notes (v1.5 entry). + +--- + +### 6. iOS v0.5.4 dependency boundary + +**Severity:** Medium (documentation gap for self-hosters running +the server *without* the iOS app). + +What self-hosters need to know: + +- **Server-only operators:** the migration runs and the cron entry + registers, but with no APNs config + no iOS devices paired, every + cron tick is a no-op and the new event surface stays silent. **Zero + behavioural change.** ✅ Captured in CHANGELOG "Compatibility" + + the `loadApnsConfig` all-or-none guard already shipped in v1.4.23. +- **Mixed operators (server + iOS):** need the iOS app to register + `UNNotificationCategory` identifiers `MEDICATION_REMINDER` and + `MOOD_REMINDER` at launch. Without that, iOS renders plain alerts. + Captured in PR report (`V054-server-apns-mood-report.md:104–108`) + but **not** in CHANGELOG. +- **The `scheduledAt` ISO field** on med-reminder metadata is now + always emitted. Older iOS builds that don't read the field ignore + it. **No documentation needed** (additive metadata is the existing + iOS-coord contract). + +**Medium fix for v1.5 release notes:** add a one-liner that calls +out the iOS app must register the new category identifier to +surface the mood reminder as actionable. Self-hosters running +HealthLog headless (no iOS, only web) get no change and need no +action — that's the safe default but worth saying explicitly once. + +--- + +### 7. Strategic gap — v1.5 carry-forward + +**Severity:** **High** — must land in v1.5 P1, otherwise the feature +is invisible to end users. + +The server side ships **fully wired**, but there is **no UI to flip +`User.mood_reminder_enabled`**. Confirmed by grep: + +- `moodReminderEnabled` appears only in: `prisma/schema.prisma`, the + worker handler, the test suite, and the auto-generated Prisma + client. **Zero references in `src/app/**` (no Settings page, no + API write route, no iOS HTTP endpoint).** +- The existing notifications-preferences API + (`src/app/api/notifications/preferences/route.ts`) covers the + per-channel × per-event matrix only — it doesn't surface a + per-user boolean flag like `moodReminderEnabled`. This is the + same gap the PR report calls out at line 109–114. +- The `/notifications` page renders the per-channel matrix from + `EVENT_TYPES`, so MOOD_REMINDER will appear there as an event + row — but ticking it on a channel won't help: the worker also + requires `User.moodReminderEnabled = true`. Two-toggle confusion + is the predictable end-user failure mode. + +**v1.5 carry-forward (must-have for the iOS sprint):** + +1. **iOS-side:** Settings → Notifications toggle for "Daily mood + reminder" that PATCHes a new endpoint + (`PATCH /api/me/preferences` or extend + `/api/notifications/preferences` with a per-user-flag branch). +2. **Web parity:** mirror the same toggle on + `/settings/notifications` so non-iOS self-hosters can opt their + Telegram / ntfy / Web-Push channels into the daily nudge. The + server is channel-agnostic — withholding the toggle from web + would be a regression vs. the dispatcher's actual capability. +3. **Translation key parity:** add + `notifications.eventMoodReminder` label key in all six locales + so the `/notifications` matrix row has a human-readable label + (the function at `src/app/notifications/page.tsx:73` will derive + `notifications.eventMoodReminder` and fall through to the raw + key if missing — currently missing for MOOD_REMINDER in *all + six* `messages/*.json` files). +4. **Configurable hour:** lift `MOOD_REMINDER_LOCAL_HOUR = 22` to + a per-user preference (default 22) so users in shift-work or + different cultural wind-down rhythms can move the slot. + Medium-priority — server-side default is already opinionated; + v1.5 P2 is fine. + +--- + +## Missing-for-next-release list + +Listed in priority order for the v1.5 P1 iOS sprint: + +1. **[High] iOS Settings toggle** for `moodReminderEnabled` + the + matching server PATCH route. Without this the feature is dead + code for end users. +2. **[High] Web Settings parity toggle** at + `/settings/notifications` so self-hosters without an iPhone can + opt their existing channels into the daily nudge. Same PATCH + route as #1. +3. **[High] Translation key** `notifications.eventMoodReminder` in + all six `messages/*.json` so the matrix row on + `/notifications` shows a label rather than the camel-cased key. +4. **[Medium] Configurable reminder hour** — lift `22` to a + per-user preference. +5. **[Medium] French typo** `aujourdhui` → `aujourd'hui` in + `messages/fr.json:2994`. +6. **[Medium] CHANGELOG language clean-up** — drop the + `EVENT_DEFAULT_ENABLED` identifier leak from future entries that + touch the same surface. +7. **[Low] Operator note in v1.5 release notes** explicitly calling + out that headless self-hosters (web-only, no iOS app) need no + action — the feature is silent without paired devices. + +--- + +## Sign-off + +**Verdict:** GO. The release is strategically coherent (capture +funnel that feeds the existing differentiator), respects the opt-in +default pattern, ships Marc-Voice-clean copy, leaks no PII, and +documents the iOS dependency adequately in the PR report (with a +small CHANGELOG gap noted in §6). + +**Conditions:** + +- The four items marked **[High]** above MUST land in v1.5 P1. + Until then the user-visible footprint of v1.4.38.1 is zero on the + iOS side and exactly one un-labelled row in the web matrix that + doesn't actually flip the dispatcher. +- The migration is additive + idempotent — rollback path is "leave + the column at default false; the cron remains a no-op". No + rollback risk. diff --git a/.planning/ios-coord/V054-QA-security-findings.md b/.planning/ios-coord/V054-QA-security-findings.md new file mode 100644 index 000000000..4dc1e832c --- /dev/null +++ b/.planning/ios-coord/V054-QA-security-findings.md @@ -0,0 +1,320 @@ +# V054 IT-Security Review — PR #190 (4049c6c7 + c4f2e0bc) + +Scope: APNs `category` + `MOOD_REMINDER` event-type. iOS coord patch on `main`. + +Files inspected: + +- `messages/{de,en,es,fr,it,pl}.json` (only de/en touched; rest unchanged) +- `prisma/migrations/0069_v054_mood_reminder/migration.sql` +- `prisma/schema.prisma` (User.moodReminderEnabled + MoodReminderDispatch) +- `src/lib/jobs/mood-reminder.ts` + test +- `src/lib/jobs/reminder-worker.ts` (queue registration + scheduledAt patch) +- `src/lib/notifications/senders/apns.ts` + test +- `src/lib/notifications/types.ts` + +Method: read diff, trace dispatcher cascade for APNs delivery, search for opt-in write +paths, grep for log surfaces that touch device tokens or signing keys, manually +walk migration 0069 against the multi-writer / cascade rules. + +Severity legend: Critical (block release) / High (must fix this release) / +Medium (fix before iOS GA) / Low (cosmetic or hardening). + +--- + +## Severity counts + +- Critical: 0 +- High: 2 +- Medium: 4 +- Low: 3 + +--- + +## H-1 — MEDICATION_REMINDER APNs payload exposes medication name + IDs to Apple-servers and lock-screen + +File: `src/lib/notifications/senders/apns.ts:240-277` (push composition) + +`src/lib/jobs/reminder-worker.ts:594-608` (caller; pre-existing) + +`src/lib/notifications/senders/apns.ts:391-411` (per-device send in `sendViaApns`). + +What ships: + +- `aps.alert.title` = `getPhaseMessage(...).title` which is templated with + `{ medName }` — the medication's display name flows literally into the alert + title. Apple's APNs servers see this in cleartext at the HTTP/2 boundary + (TLS in transit, but Apple terminates) and iOS renders it on the + lock-screen unless the user has globally hidden previews. +- `aps.alert.body` = `getPhaseMessage(...).message` which is templated with + `{ medName, doseInfo, timeWindow, minutes }` — same exposure as the title, + PLUS the dose string (e.g. "Mounjaro 7.5 mg"). The body shows in the + Notification Center preview row even when notification previews are + permission-gated. +- `aps` custom payload (`data`) carries `medicationId`, `scheduleId`, + `phase`, `date`, `scheduledAt`, and `replyMarkup` (a Telegram-shaped + `inline_keyboard` object). The custom payload is not rendered on the + lock-screen, but Apple's relay still sees it, and any future iOS + Notification Service Extension (mutable-content is forced on at + `apns.ts:409`) will receive the entire object. + +Why this is High and not Critical: HealthLog has historically routed the +same content through Telegram and Web Push, and Apple's relay TLS + opaque +server contract are industry-accepted. The new exposure is "now also on +Apple's network and iOS lock-screen", not "newly cleartext on the internet". +But it crosses two HealthLog norms: + +1. The codebase comments repeatedly call out lock-screen exposure as a + sensitive surface (see `MOOD_REMINDER` rationale at types.ts:38-52). +2. `replyMarkup` is a Telegram-specific field bleeding into the iOS payload + — pure leakage with zero iOS-side consumer. + +Remediation shape: + +- Server-side: in `apns.ts:sendViaApns()` construct the per-channel payload + inside the APNs sender (don't pass the dispatcher payload `data` through + raw). Whitelist the fields APNs actually needs: `eventType`, + `medicationId`, `scheduleId`, `scheduledAt`. Drop `replyMarkup`, `phase`, + `date` from the iOS leg. +- Lock-screen: for `MEDICATION_REMINDER`, set + `aps.alert.title-loc-key` + `loc-args` so iOS can render a generic + "Medication reminder" title when the user has previews hidden, while the + app still receives the real medName via `loc-args`. (Or simpler: + render the title as the generic "Medication reminder" + put medName in + `data` only, let the iOS app expand to the full string after unlock.) +- Decision lever for Marc: keep medName on the lock-screen (current UX + parity with Telegram / Web Push) vs hide-until-unlock. Both are + defensible — but `replyMarkup` leakage is not a UX choice and should be + removed unconditionally. + +--- + +## H-2 — `users.moodReminderEnabled` has no API write path → opt-in flow is missing + +Files: PR adds the column + worker-side read, but no Next.js route, no +form, no Zod schema, no Settings UI touches the flag. + +Verified by grep: +`grep -rn "moodReminderEnabled\|mood_reminder_enabled" src/app src/components` +returns zero non-generated matches. + +Security implication: with the flag at default `false`, the feature is +functionally inert in production until someone writes a UI/PATCH endpoint. +That part is safe. But the iOS coord patch frames this as the user-facing +opt-in, and the iOS app will presumably want to flip it via the existing +auth-cookie + CSRF middleware on `/api/user/settings` or similar. Whoever +wires that endpoint MUST: + +- Authenticate (session cookie or bearer iOS API token). +- Apply the same CSRF guard as the rest of `/api/user/*` (HealthLog uses + the shared `apiContext` wrapper — verify the eventual route uses it). +- Validate the body via Zod (a single boolean). +- NOT accept mass-toggle on behalf of another userId (the route handler + must read userId from session, never from request body). + +Remediation shape: file this as a non-negotiable acceptance criterion on +the future "POST /api/user/notification-prefs" task (or wherever the toggle +lands). Add an integration test that asserts: + +1. Unauthenticated request → 401. +2. Request with a different userId in body → ignored / 403. +3. Request without CSRF token (web) → 403. +4. Value coerced to boolean (no `"true"` string injection enabling). + +Severity High because the opt-in is the GDPR + ethical anchor of an +"emotionally-loaded" surface (per the schema comment), and shipping the +write endpoint without these guards would silently let a malicious page +opt the user into a daily push. + +--- + +## M-1 — `mood_reminder_dispatches` ledger has no retention policy + +File: `prisma/migrations/0069_v054_mood_reminder/migration.sql:18-30` + +`prisma/schema.prisma:1192-1212`. + +Growth rate: ~365 rows / opted-in user / year, kept forever. Each row +holds `userId`, `date`, `dispatchedAt` — low PII surface, but it is a +behavioural footprint ("at which dates did this user fail to log a mood +before 22:00"). + +GDPR posture: the row is technically health-adjacent (it derives from +mood-logging behaviour). Storing it indefinitely for users who later +disable the reminder leaks behavioural history. + +Remediation shape: + +- Add a daily cron (slot it next to the audit-log cleanup at 03:15) that + deletes `mood_reminder_dispatches` rows older than ~90 days. The dedup + contract only needs the current day's row to be live. +- Alternatively, set up a Postgres partition + drop-by-month, but for + ~365 rows/user/year the simple `deleteMany` with a date predicate is + plenty. +- Ensure account-deletion cascade already handles this (FK + `ON DELETE CASCADE` is set at migration line 36 — confirmed safe). + +Severity Medium: privacy-relevant, not security-exploitable. + +--- + +## M-2 — Mood-reminder dispatcher aborts the whole tick on a single bad timezone + +File: `src/lib/jobs/mood-reminder.ts:151-164` + `src/lib/timezone.ts:21-32`. + +`getLocalDateParts` calls `new Intl.DateTimeFormat(..., { timeZone: tz })` +which throws `RangeError: Invalid time zone specified` for unknown IANA +strings. The mood-reminder loop has no per-user `try` wrapper — one +corrupted `users.timezone` value (typo from a manual DB write, future +import bug) will throw out of `runMoodReminderTick`, propagate to +`handleMoodReminderCheck` → `evt.setError` → `throw err`, and the entire +tick rolls back. Every opted-in user downstream of the bad row is silently +skipped for that 15-min window. + +Reliability bug, not a classic security bug, but availability is part of +the security CIA triad and this is one rogue row → cohort-wide +notification outage. + +Remediation shape: wrap the per-user body in `try { ... } catch (err) { +evt.addWarning(...); continue; }` and increment a +`summary.skippedInvalidTimezone` counter. Mirrors the pattern the +medication-reminder loop uses (see `reminder-worker.ts` per-user error +handling). + +--- + +## M-3 — APNs key file read error logs full filesystem path + +File: `src/lib/notifications/senders/apns.ts:172-179`. + +``` +signingKey = readFileSync(/* turbopackIgnore: true */ keyFile, "utf8"); +// catch +getEvent()?.addWarning(`APNs key file read failed: ${message}`); +``` + +`fs.readFileSync` errors include the literal path (`ENOENT: no such file +or directory, open '/secrets/apns/key.p8'`) in `err.message`. This lands +in the wide-event log. If logs ever surface to a less-trusted boundary +(Coolify admin UI, support export, log shipping to a SaaS), the operator +discloses their secret-mount path. + +The signing key content itself is NOT in `err.message`, only the path. + +Remediation shape: log `err.code` (`ENOENT` / `EACCES` / `EISDIR`) without +the path, e.g. +`getEvent()?.addWarning("APNs key file read failed: " + (err.code ?? "read_failed"))`. + +Severity Medium because: + +1. Path is sensitive but not health-PII. +2. The warning fires only at process boot. +3. Wide-event log is operator-only today. + +--- + +## M-4 — APNs cascade priority means iOS push fires even when user globally disabled the channel via a different transport + +File: `src/lib/notifications/dispatcher.ts:115` (`channelPriority`) + +`src/lib/notifications/dispatcher.ts:126-145` (per-channel iteration). + +APNs is cascade-priority 0 (fires first). The per-event preference check at +line 124-132 reads `EVENT_DEFAULT_ENABLED[MOOD_REMINDER] = false`, so a +user has to flip `NotificationPreference.MOOD_REMINDER.enabled = true` for +the APNs channel for the push to surface. + +The double opt-in claim in the code comment ("a user has to opt in twice +before the server starts nudging them about an emotionally-loaded +surface") is therefore real ONLY for users who already have an APNS +channel row. Two scenarios where the comment lies: + +1. A user with NO `NotificationChannel(type=APNS)` row will see the + dispatcher skip APNs naturally — fine. +2. A user who paired an iPhone (APNS channel row created) and flipped + `moodReminderEnabled = true` but NEVER touched the per-channel + `NotificationPreference.MOOD_REMINDER.enabled` — the dispatcher + honours `EVENT_DEFAULT_ENABLED[MOOD_REMINDER] = false` and skips. Good. + +So the double opt-in actually holds in code today. BUT: the iOS coord +patch presumably ships an iOS Settings toggle that flips ONLY +`User.moodReminderEnabled` (not the per-channel preference row). If the +iOS app's toggle wires straight to the user-level flag without ALSO +creating the per-channel preference row, the worker enqueues the +notification via `dispatchNotification("MOOD_REMINDER")` and the +dispatcher swallows it because of the per-event default-off. Net: the +user toggled "on" in iOS, sees no push, blames the app. + +Not a security exposure — it's a "fail-closed-but-confusingly" interaction. +Worth a contract test once the iOS-facing endpoint exists, and a docs note +on the iOS coord plan that the endpoint must upsert BOTH: + +- `users.mood_reminder_enabled = true` +- `notification_preferences (channel=APNS, event=MOOD_REMINDER, enabled=true)` + +Severity Medium because shipping the iOS toggle without this upsert sets +up the user for a silent feature failure that looks like a bug. + +--- + +## L-1 — `dispatchedAt` second index `(user_id, dispatched_at)` is unused + +File: `prisma/migrations/0069_v054_mood_reminder/migration.sql:29-30` + +`prisma/schema.prisma:1209`. + +The handler only queries by `(userId, date)` (uses the unique index). No +code path uses the `(user_id, dispatched_at)` composite index. It costs +~1 row-write CPU and ~B-tree storage per dispatch; harmless today but +also no benefit. Remediation: drop the index in a follow-up migration OR +add the retention-cron predicate that would use it (`WHERE userId = $1 +AND dispatched_at < now() - 90 days`). + +Severity Low — minor hygiene. + +--- + +## L-2 — `MOOD_REMINDER` lacks a per-event `category` registration check in tests + +File: `src/lib/notifications/senders/__tests__/apns.test.ts:449-467`. + +The test pins that `note.category === "MOOD_REMINDER"`, but there is no +contract test against the iOS-coord doc that the app actually registered +a `MOOD_REMINDER` UNNotificationCategory. Server-side this is fine +(APNs ignores unknown categories per the apns.ts comment at line 401-403); +just a documentation observation. + +Severity Low — no server-side fix needed, flag for iOS sub-team. + +--- + +## L-3 — `provider.shutdown()` in `resetApnsForTesting` is fire-and-forget + +File: `src/lib/notifications/senders/apns.ts:191-196`. + +`void provider.shutdown()` discards the returned promise. In tests this +is fine, but if any production code path ever calls +`resetApnsForTesting()` (it's exported), the in-flight HTTP/2 streams +could be torn down mid-send. Today no prod caller exists; the export is +test-only. Consider renaming to `_resetApnsForTesting` or guarding with +`if (process.env.NODE_ENV !== "test") throw ...` so future refactors +don't accidentally pick it up. + +Severity Low. + +--- + +## Top 3 issues (Marc-facing brief) + +1. **H-1** — MEDICATION_REMINDER APNs payload puts medication name + + `replyMarkup` (Telegram inline-keyboard object) into Apple's relay + and onto the iOS lock-screen. `replyMarkup` is pure leakage; medName + is a UX choice that crosses HealthLog's lock-screen-exposure norm. + +2. **H-2** — `users.moodReminderEnabled` has no API write path in this + PR. The opt-in toggle endpoint that the iOS coord patch needs must + land with auth + CSRF + userId-from-session guards; pre-flag those + acceptance criteria. + +3. **M-1** — `mood_reminder_dispatches` grows ~365 rows/user/year + forever, storing a behavioural footprint of mood-logging gaps. Add a + 90-day retention cron alongside the audit-log cleanup at 03:15. + +No queue-injection surface found (cron registration is process-internal, +no API route enqueues `mood-reminder-check`). No new key material exposed +(signing key cached in process memory only, never logged). No device +tokens in logs. diff --git a/.planning/ios-coord/V054-QA-senior-dev-findings.md b/.planning/ios-coord/V054-QA-senior-dev-findings.md new file mode 100644 index 000000000..e99a0fa0f --- /dev/null +++ b/.planning/ios-coord/V054-QA-senior-dev-findings.md @@ -0,0 +1,167 @@ +# v0.5.4 iOS-coord PR #190 — Senior Code Review + +Scope: squash merge `4049c6c7` + version bump `c4f2e0bc` on `main`. + +## Verdict at a glance + +| Severity | Count | +|---|---| +| Critical | 1 | +| High | 4 | +| Medium | 5 | +| Low | 3 | + +Headline: the implementation is well-shaped, the test coverage is the strongest part, and the integration with the existing reminder-worker is clean. There is **one Critical**: the ledger row is written **before** `dispatchNotification` is called, but the dispatcher swallows every internal error — so a transient APNs / Telegram failure leaves the ledger marked "dispatched" forever and the user silently gets no push that day, and no push the next day until the ledger key rolls over. Two High-severity issues concern the i18n contract (four supported locales fall back to English instead of using their freshly added strings) and the 15-minute cron cadence producing up to four wide-event lines per user-per-day even when the handler has nothing to do. + +--- + +## Critical + +### C-1. Ledger row is committed before delivery; lost pushes never retry +File: `src/lib/jobs/mood-reminder.ts:202-229` +Also: `src/lib/notifications/dispatcher.ts:209-211` (the catch-all that masks the failure) + +The handler does: + +``` +moodReminderDispatch.create(...) // step A — commits the dedup row +dispatchImpl({ eventType: "MOOD_REMINDER", ... }) // step B — fire-and-forget +``` + +`dispatchNotification` is a `void` returning function whose outer `try/catch` swallows every error path inside it (`dispatcher.ts:209`). The dispatcher also internally treats `APNs not configured`, `no devices`, every channel `hardReject`, every transient failure, and the case where the user simply has zero enabled channels as silent. That means: + +* If the cron tick lands at 22:00 on a day where APNs is mid-rotation, the network is wobbly, the dispatcher throws on a DB connection blip, or the user happens to have no enabled channel yet, the ledger row is **still committed**. +* The dedup guard then correctly suppresses every subsequent tick in the 22:00 window and the next day's window stays open — but the user got nothing for *this* day. +* Worse: because `dispatchImpl` cannot be observed to fail, the handler reports `summary.dispatched += 1` even when the push never reached a single channel. The wide-event will read "we nudged the user" when in fact nothing went out. + +Recommended fix shape: +1. Move the ledger write to **after** the dispatch returns. Use a `try/finally` so a thrown dispatch still records the ledger (preventing a tight retry loop within the 15-min window if dispatcher latency briefly spikes), but also gate the ledger commit on at-least-one-channel-success. The cleanest path is to give the dispatcher a return value (`{ delivered: boolean }`) and only insert the ledger when `delivered === true`. +2. Failing that, accept the at-most-once trade-off but increment `summary.dispatched` *after* dispatch returns and split `summary.delivered` vs `summary.attempted` so the wide-event is honest about what happened. +3. Either way, the dispatcher's internal swallow needs a return-value signal — `dispatcher.ts:209-211` should bubble (or surface) the outcome enough that the mood-reminder handler can branch. + +This is the only finding that materially breaks the user-visible contract; everything else below is defence-in-depth or quality. + +--- + +## High + +### H-1. Locale resolver drops four of the six supported locales +File: `src/lib/jobs/mood-reminder.ts:60-62` + +``` +function resolveLocale(locale: string | null | undefined): Locale { + return locale === "en" || locale === "de" ? locale : defaultLocale; +} +``` + +`src/lib/i18n/config.ts:1` declares the supported set as `["de", "en", "fr", "es", "it", "pl"]`, and **this same PR adds `moodReminders.dailyTitle / dailyBody` strings for all four of the additional locales** (`messages/{es,fr,it,pl}.json`). A French user with `user.locale = "fr"` will receive the English copy, even though `messages/fr.json` has the translation sitting one resolver call away. + +This is also the only place where v0.5.4 narrows the supported-locale set — the existing reminder worker uses the full set via `getServerTranslator`. + +Recommended fix shape: import `locales` from `@/lib/i18n/config` and check membership against that array (or rely on `getServerTranslator` to fall back internally and just pass through the user's locale unchanged). + +Test gap: `buildMoodReminderPayload` only has three tests (`de`, `en`, `null/unknown`) — add a parameterised case across all six maintained locales so this never regresses. + +### H-2. 15-min cron cadence produces ~96 wide-event lines per day per user (4× the necessary fan-out and 96× the necessary background-event scaffolding) +File: `src/lib/jobs/reminder-worker.ts:185, 1142-1163` + +The cron is `*/15 * * * *`. Inside the 22:00 window for any user this means up to 4 ticks per user per day where `inWindow == true`, each running a Prisma `findUnique` against the ledger (cheap, but logged as a wide-event subtree). The remaining 92 ticks per day fire `withBackgroundEvent("job.mood_reminder", ...)` and execute a `findMany` over every opted-in user even though no user is in their 22:00 hour from the worker's perspective — there is no early-exit shortcut. + +For a fleet of N opted-in users we get: +* 96 ticks/day × 1 `user.findMany` each = 96 unnecessary scans. +* 96 wide-event roots/day. +* `localConcurrency=1` means these ticks serialise against each other (fine today, but the queue could starve if N grows). + +Recommended fix shape: either run the cron at `0 * * * *` (top-of-hour only — covers every IANA-hour boundary while cutting the noise 4×) and accept a worst-case 0–59 min latency on the *first* in-window tick (acceptable since the second hour-tick still gets the user the same day), or keep the 15-min cadence but early-exit the handler with a `SELECT 1 WHERE EXISTS (... AND user.timezone such that local hour == 22)` so the no-op ticks don't pull every row across the wire. + +If 15-min cadence is kept for the iOS contract reason, at minimum suppress the `withBackgroundEvent` root when zero candidates are in-window so the observability dashboards don't flood with no-op events. + +### H-3. `note.payload = { ...input.payload.data }` can collide with `aps` +File: `src/lib/notifications/senders/apns.ts:275-277` +Caller: `src/lib/notifications/senders/apns.ts:392-410` spreads arbitrary `payload.metadata` into `data`. + +`note.payload` becomes the top-level JSON adjacent to `aps`. If any caller (now or later) puts `aps`, `apns`, `apns-push-type`, or other Apple-reserved keys inside `metadata`, node-apn will serialise them at the JSON root, potentially clobbering the alert/category/sound `aps` block that the same code path just configured. Today only known callers exist (medication reminders + the new mood reminder), but `metadata` is `Record` — a future caller has zero compile-time protection. + +Recommended fix shape: strip known-reserved root keys (`aps`, `apns-*`) from `input.payload.data` before assigning; or assert and throw in `sendApnsPush` when a reserved key is present so the regression surfaces at the call site instead of at the user's lock-screen. + +### H-4. `MOOD_REMINDER` event is gated on `EVENT_DEFAULT_ENABLED = false` AND `moodReminderEnabled`, but the user-facing UI to flip either is not in this PR +File: `src/lib/notifications/types.ts:40-52`, `src/lib/jobs/mood-reminder.ts:80-82` +Plan ref: the schema comment at `prisma/schema.prisma:204-209` says "explicitly opt in from Settings → Notifications". + +Neither this PR (nor anything in `git grep -n moodReminderEnabled src/app`) wires a Settings toggle for `moodReminderEnabled`. So the v0.5.4 server contract ships with a user-invisible flag — operators or iOS clients have to flip it manually, and the per-event `NotificationPreference` row also has to be created with `enabled = true` for the dispatcher to even visit any channel (because the per-event default is `false`). + +This means the feature is **dark-launched**, which may be intentional for the iOS coordination patch. But there is no operator-facing note, no admin route, and no API endpoint in this PR to set the flag. If iOS v0.5.4 is expected to drive the toggle, that contract is missing on the server. + +Recommended fix shape: either ship a `PATCH /api/users/me/notification-prefs` extension that accepts `moodReminderEnabled`, or document in the PR description that iOS is the only caller and link the corresponding iOS PR. At minimum, an integration test that hits the (planned) endpoint would make the contract explicit. + +--- + +## Medium + +### M-1. Cron `*/15 * * * *` interacts with `tz: "Europe/Berlin"` schedule — fine today, but the comment is misleading +File: `src/lib/jobs/reminder-worker.ts:181-188, 1692-1693` + +The comment says "every 15 minutes (same cadence as the medication-reminder loop)". The `boss.schedule(... { tz: "Europe/Berlin" })` is irrelevant for `*/15 * * * *` (which fires every 15 wall-clock minutes regardless of tz reference), but a future maintainer might assume the tz applies. The handler does NOT depend on the tz parameter — the per-user local-hour check inside `evaluateMoodReminderWindow` is the only timezone-aware piece. Add a comment clarifying that the `tz` on the schedule entry is inert for this cron. + +### M-2. `evaluateMoodReminderWindow` returns `localHour: -1` for opt-out users +File: `src/lib/jobs/mood-reminder.ts:80-82` + +Returning a sentinel `-1` for a "real" `localHour` is a smell — the field's type is `number` and any downstream consumer that does arithmetic (e.g. metric labelling) will silently produce garbage. Either change the return to a discriminated union (`{ fire: true; localDate: string; localHour: number } | { fire: false; reason: "opt_out" | "outside_window" }`) or make `localHour` nullable. + +### M-3. `localDate` formatting uses `padStart(4, "0")` on `year` but the YEAR is already 4-digit +File: `src/lib/jobs/mood-reminder.ts:85-87` + +Cosmetic. `parts.year` from `getLocalDateParts` is parsed from a 4-digit Intl format. `padStart(4, "0")` is dead defensive code that confused me on read — drop it or replace with `String(parts.year)`. + +### M-4. The handler scans every opted-in user on every tick, no hour-aware SQL prefilter +File: `src/lib/jobs/mood-reminder.ts:141-149` + +For N opted-in users this is N rows back from Postgres per tick, then a per-user CPU loop to do Intl formatting. Today N is small; once Marc onboards external users this becomes a hot loop running 96 times a day. A pre-filter — even a coarse one such as `WHERE timezone IN (zones where local-hour is 22 right now)` — would shrink the wire payload. + +Recommended fix shape: precompute the set of IANA zones whose current local hour equals `MOOD_REMINDER_LOCAL_HOUR` once per tick (≤ ~600 zones, easily cached), then `WHERE timezone = ANY ($1)`. Or push the whole decision down with a raw SQL aggregator á la the rollup readers. + +### M-5. Unit tests use a stubbed Prisma; no integration test asserts the unique constraint actually fires +File: `src/lib/jobs/__tests__/mood-reminder.test.ts:320-344` + +The "lost P2002 race" test fakes the error by string. There is no testcontainer test that proves the real `mood_reminder_dispatches_user_id_date_key` unique index rejects the second insert. The handler's idempotency claim is therefore unverified end-to-end. Add a Pg-backed integration test that inserts twice in parallel and asserts the second receives `P2002`. + +--- + +## Low + +### L-1. Migration is `additive + IF NOT EXISTS` but the `DO $$ ... EXCEPTION WHEN duplicate_object` block ignores other errors +File: `prisma/migrations/0069_v054_mood_reminder/migration.sql:32-39` + +The FK creation block catches *only* `duplicate_object`, which is correct. But the `IF NOT EXISTS` on the unique index + table + column makes the whole migration idempotent — except for the FK, which would still throw on any non-duplicate failure. That's the right behaviour, just worth a one-line comment that any error outside `duplicate_object` is intentional and should fail the migration loudly. + +### L-2. `MoodReminderDispatch` has no retention / cleanup job +File: `prisma/schema.prisma:1203-1211` + +One row per (user, day) — ~365 rows/user/year. There's no nightly cleanup analogous to `auditLogCleanup` or `idempotencyCleanup`. Not urgent (tiny rows, indexed by `userId`), but a 5-year-old row has zero operational value. Add to backlog: prune dispatches older than e.g. 90 days. + +### L-3. Timezone-change-mid-day edge case +File: `src/lib/jobs/mood-reminder.ts:83-92` + +If a user changes their `timezone` between 22:00-local-old and 22:00-local-new on the same calendar date, they could receive two pushes (one per zone) because `decision.localDate` differs across the two zones. Vanishingly rare; mention in the planning notes only if you hit it in field reports. + +--- + +## What was done well + +- Test coverage on the pure helpers (`evaluateMoodReminderWindow`, `buildMoodReminderPayload`) is exactly the right shape — pinned boundary cases (21:59 / 22:00 / 23:00), a cross-timezone case (NY DST), opt-out gating, and locale fallback all covered. +- The choice to put the dedup check **before** the mood-entry check is correctly justified by index cost in the comment at `mood-reminder.ts:168-172`. That kind of reasoning-in-the-code pays back when the next maintainer touches it. +- The APNs `category` / `mutableContent` plumbing is conservative — node-apn's `category` setter is poked via the type-cast escape hatch with a clear comment explaining why, and `mutableContent` ships now as a no-op so the iOS NSE work can land without a server roundtrip. Both are textbook forward-compat decisions. +- The `MOOD_REMINDER_QUEUE` is correctly added to `allQueues` (the W7c lesson) — this is the exact failure mode the W10 multi-agent QA caught in v1.4.37 (drain queue not registered), and the comment at `reminder-worker.ts:1624-1628` calls it out by name. +- New APNs tests for `MOOD_REMINDER` and `MEDICATION_REMINDER` category propagation pin the v0.5.4 iOS contract well (`apns.test.ts:425-467`). +- Migration is additive, defaults are conservative (`mood_reminder_enabled DEFAULT FALSE`, `MOOD_REMINDER` per-event default also `false`), so existing users are byte-identical to v1.4.38 behaviour. + +--- + +## Recommended sequencing + +1. **Fix C-1 before any production traffic hits the cron**. The fail-closed-then-silent behaviour is the only finding that affects user trust. +2. Fix H-1 before any non-EN/DE user toggles the opt-in. +3. Address H-2 + M-1 + M-4 together as a perf/observability follow-up; one PR, no behaviour change. +4. H-3 + L-1 can ride a hygiene release. +5. M-5 belongs in the next testcontainer pass. +6. L-2 + L-3 onto the v1.4.39 backlog. diff --git a/.planning/ios-coord/V054-QA-simplifier-findings.md b/.planning/ios-coord/V054-QA-simplifier-findings.md new file mode 100644 index 000000000..132c1e070 --- /dev/null +++ b/.planning/ios-coord/V054-QA-simplifier-findings.md @@ -0,0 +1,86 @@ +# V0.5.4 PR #190 — Simplifier QA Findings (READ-ONLY) + +Scope: commit `4049c6c7` + version bump `c4f2e0bc` on `main`. +Files in scope: `apns.ts` (+test), `notifications/types.ts`, `mood-reminder.ts` (+test), `reminder-worker.ts` (MOOD_REMINDER block only). + +No code modified. All findings below are candidates for a future simplification pass. + +--- + +## Safe simplifications (low risk, behaviour-preserving) + +### S1 — Drop redundant `moodReminderEnabled` round-trip in `runMoodReminderTick` +**File**: `src/lib/jobs/mood-reminder.ts:141-159` +The Prisma `findMany` already filters with `where: { moodReminderEnabled: true }`, then `select`s `moodReminderEnabled: true` only to pass it back into `evaluateMoodReminderWindow`, which short-circuits when it's `false` — an impossible branch at this call site. +Proposed shape: drop `moodReminderEnabled` from the `select`, and drop the `moodReminderEnabled` arg from the helper invocation. (The helper itself stays — `evaluateMoodReminderWindow` is also called directly by tests with `moodReminderEnabled: false` to assert the opt-out behaviour.) + +### S2 — `evaluateMoodReminderWindow` `localHour: -1` sentinel never observed +**File**: `src/lib/jobs/mood-reminder.ts:79-82` +The opted-out branch returns `localHour: -1` purely to satisfy the return shape. No production caller reads `localHour` (only three test asserts touch it, none from the opted-out branch). Could return `localHour: 0` or just drop `localHour` from that branch's payload — but `0` is the simplest in-place tweak. + +### S3 — `MoodReminderCandidate` interface is over-specified for its only use +**File**: `src/lib/jobs/mood-reminder.ts:44-49` +The interface is `Pick<>`-ed exactly once and never used as a whole-type. After applying S1, the `id` + `moodReminderEnabled` fields are dead — the helper only needs `timezone`. The interface can be deleted and the helper can take `{ timezone: string }` inline. The `MoodReminderSummary` interface is fine (it's the orchestrator's return type). + +### S4 — `if (!decision.fire || !decision.localDate)` second clause exists only for TS narrowing +**File**: `src/lib/jobs/mood-reminder.ts:161` +Logically `decision.localDate` is non-null iff `decision.fire === true`. The redundant second clause is load-bearing for TypeScript's flow analysis after the `continue`. **Leave as-is** — narrowing without it would force `!` on every later `decision.localDate` read. Listed for completeness. + +### S5 — `padStart` chain in `evaluateMoodReminderWindow` re-formats data Intl already 2-digited +**File**: `src/lib/jobs/mood-reminder.ts:85-87` +`getLocalDateParts` parses the values back to `Number` (`year/month/day`), so the helper has to re-pad. A `.toString().padStart(2, "0")` per field is fine, but the whole literal could collapse to a single template-literal helper or use `String(parts.month).padStart(...)`. Cosmetic only. + +### S6 — JSDoc on `getProvider` neighbour comment block can drop "Provider lifecycle" prose +**File**: `src/lib/notifications/senders/apns.ts:1-23` (file-header) +Out-of-scope per directive (pre-existing comment, not a new line in this PR). **Skip.** + +### S7 — `void jobs;` + try/catch + `recordError()` + rethrow is the established shape +**File**: `src/lib/jobs/reminder-worker.ts:632-660` (new `handleMoodReminderCheck`) +Matches every other handler in the file 1:1. **No simplification — keep.** Noted to confirm pattern consistency. + +--- + +## Worth-considering (judgement call, may regress clarity) + +### W1 — `resolveLocale` silently downgrades fr/es/it/pl to default +**File**: `src/lib/jobs/mood-reminder.ts:60-62` +`return locale === "en" || locale === "de" ? locale : defaultLocale;` +The PR added `moodReminders.dailyTitle/dailyBody` to all six locales (`fr.json:2991`, `es.json:2991`, `it.json:2991`, `pl.json:2991`), but this helper only ever resolves to `en` or `de`. fr/es/it/pl translations are dead at runtime. **Borderline behaviour change** — fixing it expands the supported-locale set. Probably the correct fix is `locales.includes(locale as Locale) ? locale as Locale : defaultLocale` to honour the JSON files that already exist. Surfaces because the v1.4.38 i18n wave (commits `d95820ab`/`9bd67564`/`7f490af7`) shipped those strings on purpose. **Flag for Marc — choose: (a) tighten resolver to accept all 6 locales, or (b) delete the unused 4-locale strings to remove the mismatch.** + +### W2 — Test setup duplication across 6 `runMoodReminderTick` cases +**File**: `src/lib/jobs/__tests__/mood-reminder.test.ts:186-381` +Every test rebuilds the same `FakePrismaState` shape with one `u-1` candidate, empty `moodEntries`/`dispatches`/`raceUserIds`, plus the `dispatch = vi.fn(async () => {})` line. A `beforeEach` returning `{ state, prisma, dispatch }` would cut ~12 lines × 6 tests = ~70 LOC. Trade-off: each test currently reads top-to-bottom without scrolling back to setup, which is the project's preferred style. **Marginal win — defer unless adding more cases.** + +### W3 — `MoodReminderCandidate` + `MoodReminderSummary` exports are internal-only +**File**: `src/lib/jobs/mood-reminder.ts:44, 51` +Both interfaces are `export`ed but consumed exclusively inside `mood-reminder.ts` (`MoodReminderSummary` for the orchestrator's return type, used by `reminder-worker.ts`'s `handleMoodReminderCheck`). After S3, `MoodReminderCandidate` is dead and can be deleted. `MoodReminderSummary` stays exported because `reminder-worker.ts` reads its fields (`summary.candidatesScanned` etc.) into the wide-event payload — but the file currently doesn't import the type. Re-checking: `handleMoodReminderCheck` infers the type from the function return, so the export is unused in practice. Could drop `export`, but the cost of leaving it is zero. + +### W4 — APNs `category` cast comment is 13 lines for a one-line cast +**File**: `src/lib/notifications/senders/apns.ts:251-266` +The cast `(note as unknown as { category: string }).category = ...` is annotated with a 13-line comment explaining why. Could compress to 2-3 lines ("node-apn 8.1 d.ts omits the public `category` setter; runtime writes through to `aps.category` unchanged"). The detail is valuable when a future engineer upgrades node-apn — keep or trim is a taste call. **Recommend keeping** given Marc's prose-over-brevity preference in the codebase. + +### W5 — `category` is set from `payload.eventType` in `sendViaApns`, then `threadId` from the same value +**File**: `src/lib/notifications/senders/apns.ts:398-404` +Three fields (`threadId`, `category`, `collapseId`) all receive `payload.eventType`. Could extract `const eventTypeId = payload.eventType;` to make the "same identifier doubles as three keys" pattern explicit. **Cosmetic.** + +### W6 — `[winH, winM] = schedule.windowStart.split(":").map(Number)` lacks NaN guard +**File**: `src/lib/jobs/reminder-worker.ts:584-587` +If `windowStart` is malformed, `scheduledAtIso` becomes `"Invalid Date".toISOString()` → throws. The validation happens upstream on schedule write so it's safe in practice, but a defensive `Number.isFinite(winH * winM)` would prevent a corrupt-row crash. **Out-of-scope for simplification — this is a hardening item, not a simplification.** + +--- + +## Out-of-scope (touched in PR but not the new code) + +- `handleWithingsSleepSync` signature was reflowed by Prettier (apns.ts diff `@@ -717`). Not a logic change. +- `handlePrDetection` payload destructure was reflowed by Prettier (`@@ -1248`). Not a logic change. +- `workerLog("error", "Failed to reconcile orphan ImportJob rows", err)` collapsed to one line (`@@ -1573`). Not a logic change. + +--- + +## Count summary + +- Safe simplifications: **3 actionable** (S1, S2, S3) + 1 cosmetic (S5) + 2 noted/skip (S4, S6, S7). +- Worth-considering: **5** (W1 = behaviour question, W2-W5 = taste, W6 = hardening not simplification). +- Out-of-scope formatting churn: 3 spots, no action. + +Total real candidates: **8** (3 safe + 5 worth-considering). diff --git a/.planning/ios-coord/V054-QA-specialist-findings.md b/.planning/ios-coord/V054-QA-specialist-findings.md new file mode 100644 index 000000000..50930ae58 --- /dev/null +++ b/.planning/ios-coord/V054-QA-specialist-findings.md @@ -0,0 +1,187 @@ +# V054 — QA specialist findings (APNs + Prisma migration) + +**Scope.** Squash-merge `4049c6c7` ("feat(notifications): APNs category + MOOD_REMINDER event for iOS v0.5.4 (#190)") plus the v1.4.38.1 chore bump `c4f2e0bc` on `main`. Files in scope: `prisma/migrations/0069_v054_mood_reminder/migration.sql`, `prisma/schema.prisma`, `src/lib/notifications/senders/apns.ts` + test, `src/lib/notifications/types.ts`, `src/lib/jobs/mood-reminder.ts` + test, `src/lib/jobs/reminder-worker.ts`, `messages/{de,en,es,fr,it,pl}.json` (moodReminders namespace only). + +Two specialist lenses applied: APNs payload schema (Apple 2024+) and Prisma migration idempotency / referential integrity. READ-ONLY review — no code modified, no migrations executed. + +--- + +## Severity counts + +| Severity | Count | +|----------|-------| +| Critical | 0 | +| High | 2 | +| Medium | 4 | +| Low | 3 | + +--- + +## Findings + +### H1 — `resolveLocale` drops es/fr/it/pl, defeats the just-shipped translations [High] + +**File.** `src/lib/jobs/mood-reminder.ts:60-62` + +```ts +function resolveLocale(locale: string | null | undefined): Locale { + return locale === "en" || locale === "de" ? locale : defaultLocale; +} +``` + +**Issue.** The HealthLog locale set declared in `src/lib/i18n/config.ts:1` is `["de", "en", "fr", "es", "it", "pl"]`. `getServerTranslator` correctly serves all six locales (it indexes into `allMessages[locale]` and falls back to English on missing keys). However `buildMoodReminderPayload` funnels the user's locale through `resolveLocale`, which whitelists only `en` and `de` and downgrades everything else to `defaultLocale` (= `en`). + +Net effect on iOS v0.5.4 ship: the Spanish / French / Italian / Polish `moodReminders.dailyTitle` + `dailyBody` strings introduced in the same PR (`messages/{es,fr,it,pl}.json:2991-2992`) are **unreachable** — every non-de/non-en user will receive the English copy regardless of `User.locale`. The translations are inert dead code today. + +The unit test `buildMoodReminderPayload — falls back to the app default for null / unknown locales` (mood-reminder.test.ts:171-177) passes for the wrong reason: it asserts that the `null` and `"zz-ZZ"` fallbacks land on the same string, which they do — but `"fr"`, `"es"`, `"it"`, `"pl"` also currently land on that same English fallback. + +**Fix shape.** Replace the explicit allow-list with a check against `locales` from `@/lib/i18n/config`, then cast the validated input to `Locale`: + +```ts +import { locales } from "@/lib/i18n/config"; +function resolveLocale(locale: string | null | undefined): Locale { + return (locales as readonly string[]).includes(locale ?? "") + ? (locale as Locale) + : defaultLocale; +} +``` + +Add three test cases asserting `fr` → `"Enregistrer votre humeur"`, `es` → `"Registrar el ánimo"`, `it` → `"Registra il tuo umore"`, `pl` → `"Zapisz nastrój"`. + +### H2 — French copy missing apostrophe: `aujourdhui` → `aujourd'hui` [High] + +**File.** `messages/fr.json:2993` + +```json +"dailyBody": "Comment vous sentez-vous aujourdhui ?" +``` + +**Issue.** Every other French string in the codebase writes `aujourd'hui` (curly apostrophe) or `aujourd'hui` (straight apostrophe) — see `fr.json:400, 529, 625, 979`. The new key is missing the apostrophe entirely, surfacing on iOS push notifications as the misspelled word. This is the user-visible mood reminder body — first impression for French users opting in. + +**Fix shape.** Change to `"Comment vous sentez-vous aujourd'hui ?"` (curly apostrophe, matching the existing convention in `fr.json:400, 529`). Once H1 is fixed and French users can actually receive the string, this becomes immediately visible. + +### M1 — node-apn d.ts cast pokes the `.category` setter directly; safer to write `aps.category` [Medium] + +**File.** `src/lib/notifications/senders/apns.ts:265` + +```ts +(note as unknown as { category: string }).category = input.payload.category; +``` + +**Issue.** The cast comment explains the d.ts gap accurately and node-apn 8.1's runtime does honour `.category` as a setter that writes through to `aps.category`. The fragility is the implicit dependency on an undocumented setter that the maintainer could rename in a minor release without breaking the d.ts contract (the contract doesn't mention it). A defensive belt-and-braces alternative is to write `note.rawPayload = { aps: { category: ... } }`, which is part of node-apn's documented API, or to fall back to a payload property assignment if the setter were ever removed. + +This is a Medium because the behaviour is correct today (an explicit test pins it — `apns.test.ts:425-447`) but the abstraction layer is unusual for this codebase and a node-apn upgrade is the breakage vector. + +**Fix shape.** Either (a) accept the runtime coupling and add an explicit comment-block changelog note pinning the supported node-apn version range, or (b) refactor to use node-apn's `note.aps.category = ...` access path, which is in the published `Aps` interface. Today's test would still pass. + +### M2 — Mood-reminder `data: null/undefined` ⇒ no custom-keys envelope [Medium] + +**File.** `src/lib/notifications/senders/apns.ts:275-277` + +```ts +if (input.payload.data) { + note.payload = { ...input.payload.data }; +} +``` + +**Issue.** The lower-level `sendApnsPush` only attaches the `data` envelope when `input.payload.data` is truthy. The dispatcher-level entry point `sendViaApns` (line 393-396) always sets `data: { eventType: ..., ...metadata }`, so the contract holds for the production path. But a future caller of `sendApnsPush` that omits `data` would ship a push with no client-side disambiguator at all — the iOS handler has no way to tell `MOOD_REMINDER` from `MEDICATION_REMINDER` from `SYSTEM_ALERT` once the user taps. The `category` field on `aps` is enough for the action-button row, but it does NOT surface in `userInfo` on the legacy `didReceive` delegate path before iOS 10. + +**Fix shape.** Always attach at least `{ eventType }` from the calling layer (or, alternatively, write `note.payload = { eventType, ...(data ?? {}) }` in the sender so the envelope is never empty). Add a sender-level test asserting `note.payload.eventType` is set even when the caller passes no `data`. + +### M3 — `scheduledAt` ISO string in med-reminder metadata bypasses scheduling source-of-truth [Medium] + +**File.** `src/lib/jobs/reminder-worker.ts:582-587` + +```ts +const [winH, winM] = schedule.windowStart.split(":").map(Number); +const scheduledAtIso = new Date( + todayStart.getTime() + winH * 3600000 + winM * 60000, +).toISOString(); +``` + +**Issue.** `todayStart` is a UTC Date returned from `getUserTodayBounds` (the user's local midnight expressed as UTC). Adding `winH * 3600000 + winM * 60000` to that timestamp is a straight UTC offset — correct ONLY when the user's local midnight + `winH` hours yields the same wall-clock as `winH` in local time, which is `false` across a DST transition day in any IANA tz that observes DST. + +Concrete example: user in `Europe/Berlin` on the autumn fall-back day (2026-10-25, clock goes back 1h at 03:00). `todayStart` resolves to 2026-10-24T22:00:00Z (00:00 local). For a `windowStart="22:00"` schedule, the code computes `2026-10-25T20:00:00Z` and stamps it as the schedule slot. The actual wall-clock 22:00 local that day is `2026-10-25T21:00:00Z` (one hour later in UTC because the day has 25 hours). The metadata sent to APNs is off by one hour exactly on transition days. + +This is a Medium because (a) the impact is one wrong second of `scheduledAt` per user per DST day per schedule, (b) the iOS client uses `scheduledAt` to anchor the "snooze 15 min" calculation — it would snooze relative to the wrong baseline on DST days, and (c) Marc's user base is concentrated in Europe/Berlin where this fires twice a year. + +**Fix shape.** Project `windowStart` into the user's timezone via the same `Intl.DateTimeFormat` machinery that powers `getLocalDateParts` — or, simpler, store `scheduledAt` as the local YYYY-MM-DDTHH:mm string + tz separately and let the iOS client convert. Either way, do not arithmetic-add hours to a UTC `todayStart` and call it a local-time slot. + +### M4 — `getServerTranslator` is synchronous but reads `allMessages` from disk on first call [Medium] + +**File.** `src/lib/jobs/mood-reminder.ts:104` (via `getServerTranslator`) + +**Issue.** Once per worker boot, the first `runMoodReminderTick` invocation triggers a synchronous read of every locale's messages bundle through `allMessages`. The worker has `localConcurrency=1` and this only happens once per process, so the runtime cost is irrelevant. The concern is initialisation order: if `runMoodReminderTick` ever runs on a code path where the locale messages aren't yet bundled into the runtime (e.g. a unit test that mocks `@/lib/i18n/shared-resolve` partially), the dispatcher silently returns the raw key like `"moodReminders.dailyTitle"` as the push body. + +The unit test `buildMoodReminderPayload — returns German strings for locale=de` (mood-reminder.test.ts:160-163) does NOT mock the i18n layer — it exercises the real translator. That guards the de/en path but not against future re-arrangements of how messages are bundled. + +This is a Medium because the production codepath works today; the brittleness is the dependency on global init. + +**Fix shape.** Add a defensive assertion in `buildMoodReminderPayload` that the resolved string is not equal to the input key, throwing if so. That converts a silent "user receives `moodReminders.dailyTitle` as their push" into a loud worker error visible in the wide-event log. + +### L1 — Idempotency-row created BEFORE dispatch ⇒ dispatcher failure leaves a poison-pill row [Low] + +**File.** `src/lib/jobs/mood-reminder.ts:198-216` + +**Issue.** The handler reserves the `MoodReminderDispatch` row first (lines 202-216), then calls `dispatchImpl` (lines 220-229). If the dispatcher throws — APNs network outage, web-push 500, Telegram quota — the ledger row is already persisted with `dispatched_at = now()`. The next 15-min tick checks the ledger, finds the row, and skips the user for the rest of the day. The user never receives the reminder despite the dispatcher having no successful delivery. + +The comment on line 197-201 acknowledges the race-vs-double-fire tradeoff but doesn't address the failure-mode tradeoff. The current ordering optimises for "never double-send"; the alternative would optimise for "always retry on transient failure". + +**Fix shape.** Either (a) accept the tradeoff explicitly and document that an APNs outage during the 22:00 window means the user gets no reminder that day — which may be the right call for a low-stakes nudge, or (b) move the ledger write to AFTER successful dispatch with a UNIQUE constraint catch on the second tick. Option (a) is probably right for v0.5.4. A test pinning the chosen contract would be valuable. + +### L2 — `scheduledAt` UTC string risks confusing the iOS handler in non-Berlin zones [Low] + +**File.** `src/lib/jobs/mood-reminder.ts:225-228` + +```ts +metadata: { + scheduledAt: now.toISOString(), + localDate: decision.localDate, +}, +``` + +**Issue.** `now.toISOString()` is the wall-clock UTC of the cron tick that fired, NOT the local 22:00 boundary. The iOS handler reading `scheduledAt` to derive "you should have logged your mood by 22:00 local" gets a UTC timestamp 0–14 minutes after 22:00 local. Not wrong, but ambiguous: the API field name reads as "when was this scheduled to fire" — answer is "22:00 local" — but the value is "22:07Z on a Friday in Berlin". + +This is a Low because the iOS contract for `scheduledAt` is not pinned anywhere in this PR (no iOS code in scope), and the dispatcher payload pattern matches the new med-reminder enrichment (M3). Worth aligning on a convention with the iOS team before v0.5.4 ships. + +**Fix shape.** Either rename to `dispatchedAt` (which is what the value is) or compute the actual 22:00 local boundary in UTC and ship that — same fix shape as M3, same convention. + +### L3 — Cron `*/15` may miss the 22:00 window for a user whose tz transitions during the hour [Low] + +**File.** `src/lib/jobs/reminder-worker.ts:183-194` + +**Issue.** A user on the `Asia/Kathmandu` (+05:45) timezone — or any other quarter/half-hour-offset tz — could in principle have a DST transition lining the cron's 15-minute UTC tick pattern up such that all four UTC ticks within the user's local 22:00 hour are on the "old" side of the transition and one falls on the "new" side that's no longer 22. Practically, no quarter-hour-offset tz observes DST today (Asia/Kathmandu, Asia/Yangon, Pacific/Chatham +12:45 don't), so this is theoretical. + +**Fix shape.** No action needed today. Add a comment near the cron declaration noting that the every-15-min cadence assumes IANA timezones with full-hour DST shifts, so the assumption is reviewable when a new tz is added to the supported list. + +--- + +## Prisma migration 0069 — verdict: GREEN + +The migration `prisma/migrations/0069_v054_mood_reminder/migration.sql:15-39` correctly implements the idempotency guards: + +- `ALTER TABLE "users" ADD COLUMN IF NOT EXISTS "mood_reminder_enabled" BOOLEAN NOT NULL DEFAULT FALSE` — the `IF NOT EXISTS` guard is supported on Postgres 9.6+ (HealthLog runs PG 16). Re-application is silent. Default `FALSE` matches the Prisma decl `Boolean @default(false)` in `schema.prisma:208` and the "default off" contract claimed in the commit message. +- `CREATE TABLE IF NOT EXISTS "mood_reminder_dispatches"` — also idempotent, ditto the `CREATE UNIQUE INDEX IF NOT EXISTS` + `CREATE INDEX IF NOT EXISTS` lines. +- The composite unique index `mood_reminder_dispatches_user_id_date_key` on `("user_id", "date")` matches the Prisma `@@unique([userId, date])` in `schema.prisma:1208` byte-for-byte (Prisma's auto-generated index name convention is `{table}_{col1}_{col2}_key`). The `userId_date` composite query at `mood-reminder.ts:174` will hit this index. +- The `DO $$ ... EXCEPTION WHEN duplicate_object THEN NULL END $$` block correctly handles re-application of the FK constraint, catching the `duplicate_object` SQLSTATE that PG raises when an FK with the same name already exists. +- `ON DELETE CASCADE` on `mood_reminder_dispatches.user_id` is the correct choice for privacy retention: when a user is deleted, their reminder ledger goes with them. The ledger contains no clinical data — only an opaque `id`, `user_id`, `date`, `dispatched_at` — so the cascade is also GDPR-clean. + +Re-applying the migration on a database where it already ran will succeed silently — verified by code inspection of all four `IF NOT EXISTS` / `EXCEPTION` guards. + +--- + +## APNs payload — verdict: GREEN with one nuance + +- `aps.category` is set via node-apn's `Notification.category` setter (`apns.ts:265`), which writes through to `aps.category` per the library's runtime contract. The value is a plain string matching exactly what iOS v0.5.3 registers (`MEDICATION_REMINDER`, `MOOD_REMINDER`). No typos. ✅ +- `aps.mutable-content` is set via `note.mutableContent = true` (`apns.ts:270`), which node-apn correctly serialises as the JSON integer `1`, not the boolean. node-apn's `Notification` setter is documented and respects Apple's strict-integer rule. ✅ +- Payload size: the largest enrichment added in this PR is `scheduledAt` (~26 chars ISO string) + `localDate` (~10 chars). The med-reminder branch adds the same `scheduledAt`. Total payload across all metadata keys is well under 1 KB — nowhere near the 4 KB / 5 KB APNs limits. ✅ +- Alert localisation: server-side string interpolation via `getServerTranslator`, no `loc-key` / `loc-args` template path. That's a deliberate choice — `loc-key` requires the iOS bundle to ship the matching `Localizable.strings`, and the server can change copy without an App Store release this way. Tradeoff is mentioned for completeness, not flagged. ✅ +- Device token handling: no token logging, no token leakage in the wide-event payload. `apns.ts:285-290` logs only `service`, `method`, `duration_ms`, `status`, `error`. The `addExternalCall` call never receives the token. ✅ + +--- + +## Test-coverage note + +The 13 unit tests in `mood-reminder.test.ts` pin window boundaries, opt-in gating, dedup, and the P2002 race correctly. The locale test at lines 159-177 is weak: it covers de + en + null/unknown but misses fr/es/it/pl. Adding four lines there would catch H1 immediately and is the smallest possible regression guard. + +The APNs test at `apns.test.ts:449-467` pins `MOOD_REMINDER` category forwarding correctly. Good coverage of the new event-type's APNs path. diff --git a/.planning/ios-coord/V054-QA-ux-findings.md b/.planning/ios-coord/V054-QA-ux-findings.md new file mode 100644 index 000000000..352c9965d --- /dev/null +++ b/.planning/ios-coord/V054-QA-ux-findings.md @@ -0,0 +1,222 @@ +# V054 PR #190 — UX Review of Notification Surfaces + +**Reviewer scope.** Squash-merge `4049c6c7` + version bump `c4f2e0bc` on `main`. +**Files in scope.** `messages/{de,en,es,fr,it,pl}.json`, +`src/lib/notifications/senders/apns.ts`, `src/lib/notifications/types.ts`. +**Audience.** Opted-in personal-health tracker user, iPhone lock-screen or +Notification Center, DE canonical / EN/ES/FR/IT/PL translated. + +--- + +## Severity counts + +| Severity | Count | +| --- | --- | +| Critical | **2** | +| High | **3** | +| Medium | **3** | +| Low | **2** | + +--- + +## Critical + +### C1 — French body has spelling bug `aujourdhui` (missing apostrophe) +**File.** `messages/fr.json:2993` +``` +"dailyBody": "Comment vous sentez-vous aujourdhui ?" +``` +Correct French is `aujourd'hui`. This is a hard typo that ships on the +lock-screen and reads aloud through VoiceOver. Native FR users will see +this as carelessness on a *health* product. The same string in DE/EN/ES/IT +is spelled correctly. Either the apostrophe was stripped because the +contributor avoided embedding `'` inside the JSON string, or it was simply +missed during the wave. + +**Fix.** `"Comment vous sentez-vous aujourd'hui ?"` (the curly apostrophe +`’` is also acceptable and is what Apple typography expects, but the +straight `'` is what the rest of the FR file uses — keep it consistent). + +### C2 — No Settings surface to toggle `mood_reminder_enabled` +**Files.** +- `src/components/settings/notifications-section.tsx` (the natural home) +- `src/app/settings/[section]/page.tsx` +- DB column `users.mood_reminder_enabled` exists, default `false` +- `NotificationsState` has a `moodLogGlobal` field but it is **unwired** + (`grep -n moodLog notifications-section.tsx` returns only the type + declaration on line 19; no `` / `` renders it, and the + field is *not* the new `moodReminderEnabled` flag — it's pre-existing + unrelated state). + +A user who installs iOS HealthLog v0.5.4 and grants notification permission +**cannot turn the daily 22:00 mood push on**, because the only way to flip +`User.moodReminderEnabled = true` today is via raw SQL or the Prisma +console. The feature ships as a dead opt-in. + +**Fix.** Add a single `` to `notifications-section.tsx` bound to +`/api/me` (or wherever profile prefs are PATCHed), with the +`notifications.moodReminder.toggleLabel` / +`notifications.moodReminder.toggleDescription` strings added to all six +locale files. Without this, ship the server foundation as "iOS-only" in +the release notes or pull the cron registration until a UI exists. + +--- + +## High + +### H1 — 22:00 fire-time ignores user sleep / quiet hours +**File.** `src/lib/jobs/mood-reminder.ts:37` +``` +export const MOOD_REMINDER_LOCAL_HOUR = 22; +``` +Hard-coded to 22:00 local. `grep -in "quiet\|sleep\|do.?not.?disturb" +src/lib/jobs/mood-reminder.ts` returns nothing. A user whose +`Settings → Sleep` schedule has them in bed at 22:00 (early sleepers, +shift workers, parents) gets buzzed during wind-down on a product that +otherwise tracks sleep as a first-class metric. iOS Focus modes will +suppress the banner, but the badge + lock-screen flash still happen, and +on Android (web push fallback) Focus modes don't apply. + +**Fix.** Either (a) make the hour user-configurable in the same Settings +row as C2, or (b) cross-reference the user's average bedtime from the +Sleep series and skip the push if `22:00 ≥ avg_bedtime - 30min`. (a) is +the minimum viable fix. + +### H2 — DE title `Stimmung erfassen` reads as a task instruction, not a nudge +**File.** `messages/de.json:2992` + +The DE title imitates the EN imperative `Log your mood`, but Marc's +existing DE notification copy (see `medicationReminders` block, +`messages/de.json:2976-2989`) uses *event-status* phrasing — `🟢 +Erinnerung: …`, `🟡 Bald fällig: …`. The new title breaks pattern and +sounds like a task on a TODO list rather than a soft check-in. The body +`Wie geht es dir heute?` already carries the conversational tone the +title is missing; the title should match the warmth. + +**Suggested DE.** `Kurzer Stimmungs-Check` or `Tagescheck Stimmung` — +mirrors the noun-led, status-flavoured style of the rest of the file. A +two-word title also keeps it well under the iOS 50-char truncation in any +locale. + +### H3 — `category = payload.eventType` is forwarded indiscriminately +**File.** `src/lib/notifications/senders/apns.ts:399` +```ts +category: payload.eventType, +``` +The comment claims "iOS ignores categories it doesn't know about, so adding +the key here is safe". That is true for *unknown* categories. But the iOS +app currently registers two known categories (`MEDICATION_REMINDER`, +`MOOD_REMINDER`); every *other* event type — `MEDICATION_MISSED`, +`WITHINGS_SYNC_FAILED`, `SYSTEM_ALERT`, `PERSONAL_RECORD` — now ships with +a category identifier that iOS will look up, find nothing for, and silently +drop. That's not a UX bug today, but the moment the iOS team registers a +3rd category they will inherit accidental wiring (e.g. registering +`SYSTEM_ALERT` with destructive actions automatically routes existing +server pushes through it). UX-wise: silent surface-coupling between +back-end event-types and iOS UI categories. + +**Fix.** Maintain an explicit allow-list (`KNOWN_IOS_CATEGORIES`) inside +the sender. Only forward `category` for event-types in the list. The cost +is one line; the safety is significant. + +--- + +## Medium + +### M1 — Inconsistent register across locales +| Locale | Form | Note | +| --- | --- | --- | +| DE | informal `du` (`Wie geht es dir heute?`) | matches AI Coach / app convention | +| EN | imperative + casual | OK | +| ES | informal `tú` (`¿Cómo te sientes hoy?`) | matches | +| FR | **formal `vous`** (`Comment vous sentez-vous`) | breaks pattern — rest of FR coach copy uses informal `tu` per recent v1.4.38 wave | +| IT | informal `tu` (`Come ti senti oggi?`) | matches | +| PL | informal `ty` (`Jak się dzisiaj czujesz?`) | matches | + +FR is the outlier. A health-tracker daily nudge is exactly the kind of +intimate, single-user surface where French apps drop `vous`. Re-check +against the rest of `messages/fr.json` and align. + +### M2 — Mood title `Registrar el ánimo` (ES) is jargon-y +**File.** `messages/es.json:2993` +`Registrar el ánimo` is grammatically fine but lands as form-filling +language ("register the mood"). More natural-feeling alternatives that +match the EN/IT softness: `Registra tu ánimo` or `¿Cómo te sientes hoy?` +(as title, dropping the body) or `Tu chequeo de ánimo`. Same observation +applies to PL `Zapisz nastrój` (literally "save mood"). + +### M3 — Title + body together are tautological +All six locales pair title="Log/Record your mood" + body="How are you +feeling today?". On the iOS lock-screen these stack vertically with the +body in lighter weight directly under the title, and the two-line read +becomes *"Log your mood — How are you feeling today?"* — the body answers +its own title. A better split: keep the question as the title (it's the +verb), use the body for the *why* (e.g. `Trag deine Stimmung ein, es +dauert nur 5 Sekunden.` / `Takes 5 seconds.`). Today the body is +load-bearing zero. + +--- + +## Low + +### L1 — `mutableContent = true` enabled before NSE exists +**File.** `src/lib/notifications/senders/apns.ts:407` +The comment is honest that no NSE ships today. UX impact: zero. But +shipping the flag now means any later NSE bug (image-download failure, +decryption stall) will block notification delivery for v0.5.4 users +*after* the iOS team adds an NSE in v0.5.5, with no server-side opt-out +path. Recommend a feature-flag gate so the flag can be turned off without +a backend redeploy. + +### L2 — No `threadId` thinking for MOOD_REMINDER +**File.** `src/lib/notifications/senders/apns.ts:395` +`threadId: payload.eventType` groups all mood reminders under one thread — +on iOS Notification Center this is correct (don't pile up 30 identical +"How are you feeling today?" rows). Confirm with iOS team that the same +identifier is treated as a thread for grouping; if not, today's behaviour +is fine but worth a one-line comment on the sender that the grouping is +intentional for MOOD_REMINDER specifically. + +--- + +## Per-locale roll-up + +| Locale | Title | Body | Verdict | +| --- | --- | --- | --- | +| **DE** | `Stimmung erfassen` | `Wie geht es dir heute?` | Body strong; title robotic (H2). | +| **EN** | `Log your mood` | `How are you feeling today?` | Adequate; tautological pair (M3). | +| **ES** | `Registrar el ánimo` | `¿Cómo te sientes hoy?` | Title jargon-y (M2); body strong. | +| **FR** | `Enregistrer votre humeur` | `Comment vous sentez-vous aujourdhui ?` | **Critical typo C1**, register break (M1). | +| **IT** | `Registra il tuo umore` | `Come ti senti oggi?` | Cleanest pair of the six. | +| **PL** | `Zapisz nastrój` | `Jak się dzisiaj czujesz?` | Title slightly imperative (M2); body strong. | + +--- + +## Settings-surface flag + +**MISSING** — the new `User.moodReminderEnabled` field has **no UI +control anywhere in the app**. PR #190 must not be considered +user-complete until either +1. a Switch lands in `src/components/settings/notifications-section.tsx` + bound to a PATCH endpoint, **or** +2. the cron registration in `reminder-worker.ts` is gated behind a + build-time `ENABLE_MOOD_REMINDER` flag and the release notes mark the + feature as "iOS-team-only preview". + +Without one of those, the merged code silently expands the schema and +worker plumbing but ships zero observable behaviour to end users. + +--- + +## Cross-reference: existing notification copy patterns + +`medicationReminders` (de.json:2976-2989) uses: +- title pattern: `🟢 Erinnerung: {medName}` — emoji + status noun + colon +- body pattern: `{medName} (…)\n{detail}` — bolded subject + meta + +`moodReminders` uses **no emoji**, **no status noun**, **no rich +formatting** — it stands out as a different surface. That's defensible +for APNs (rich HTML doesn't render on the lock-screen), but the emoji +absence is unexplained. A single matching emoji (🌤 / 💭 / 🫶) in the +title would unify the visual language at zero copy cost. Worth raising +with Marc as a design call, not a hard finding. diff --git a/.planning/ios-coord/V054-server-apns-mood-report.md b/.planning/ios-coord/V054-server-apns-mood-report.md new file mode 100644 index 000000000..fabf919f5 --- /dev/null +++ b/.planning/ios-coord/V054-server-apns-mood-report.md @@ -0,0 +1,126 @@ +# V0.5.4 iOS Coord — Server-side APNs + MOOD_REMINDER Patches + +**Branch:** `ios-coord/v054-apns-mood` +**PR:** https://github.com/MBombeck/HealthLog/pull/190 +**Date:** 2026-05-17 +**Status:** Open for operator review (do NOT merge to main without sign-off) + +## Mission + +Two server-side patches that unblock iOS HealthLog v0.5.4 push-notification +functionality: + +1. **SR-1** — APNs `aps.category = "MEDICATION_REMINDER"` so iOS renders the + three action-buttons wired in iOS v0.5.3 (Genommen / Snooze 15 min / + Übersprungen). +2. **SR-2** — `MOOD_REMINDER` event-type + daily 22:00-local-time cron + (opt-in, idempotent, locale-aware). + +## Patches landed + +### Commit 1 — `feat(notifications): set APNs category for med-reminders` + +- `src/lib/notifications/senders/apns.ts` + - `ApnsPayload` extended with optional `category` + `mutableContent` + fields. + - `sendApnsPush` writes the category through node-apn's setter + (cast wraps the d.ts gap in node-apn 8.1; runtime setter at + `apsProperties.js#166` lands the value at `aps.category`). + - `sendViaApns` auto-forwards `payload.eventType` as the category, + so every event-type the iOS app registers becomes actionable + without per-event-type plumbing. `mutableContent: true` is set + by default (NSE-ready). +- `src/lib/notifications/senders/__tests__/apns.test.ts` + - 3 new tests pinning `aps.category = MEDICATION_REMINDER` on the + dispatcher path, `aps.category = MOOD_REMINDER` for the new + event-type, and the explicit-override path on `sendApnsPush`. + +### Commit 2 — `feat(prisma): add moodReminderEnabled + MoodReminderDispatch ledger` + +- `prisma/schema.prisma` + - `users.mood_reminder_enabled BOOLEAN DEFAULT FALSE` (opt-in flag). + - New model `MoodReminderDispatch` with `@@unique([userId, date])` + — idempotency anchor for the daily cron. +- `prisma/migrations/0069_v054_mood_reminder/migration.sql` + - Additive migration with `IF NOT EXISTS` + `DO $$ EXCEPTION WHEN + duplicate_object` guards (idempotent on re-apply, matching the + pattern from `0061` / `0068`). + +### Commit 3 — `feat(notifications): add MOOD_REMINDER event type with daily 22:00 cron` + +- `src/lib/notifications/types.ts` + - `EVENT_TYPES` extended with `MOOD_REMINDER`. `EVENT_DEFAULT_ENABLED` + sets it to `false` as a defence-in-depth gate behind the per-user + `moodReminderEnabled` flag. +- `src/lib/jobs/mood-reminder.ts` (new module, 220 LOC) + - `evaluateMoodReminderWindow(user, now)` — pure predicate for the + 22:00 window across any IANA timezone (DST-safe via + `getLocalDateParts`). + - `buildMoodReminderPayload(locale)` — locale-aware title/body + (DE: "Stimmung erfassen" / "Wie geht es dir heute?"; + EN: "Log your mood" / "How are you feeling today?"). + - `runMoodReminderTick(prisma, now)` — orchestrator: pulls + opted-in users, applies window + already-logged + already- + dispatched filters, reserves the dedup row, dispatches. +- `src/lib/jobs/reminder-worker.ts` + - Registers the `mood-reminder-check` queue with cron `*/15 * * * *`, + `localConcurrency: 1`. Cadence matches the medication-reminder + cron so any user's 22:00 boundary is caught within 15 min of + wall-clock without one cron entry per IANA zone. + - Existing medication-reminder dispatch now enriches metadata with + a `scheduledAt` ISO 8601 string so the iOS snooze-15-min action + pins against the schedule slot, not wall-clock delivery time. +- `src/lib/jobs/__tests__/mood-reminder.test.ts` (new, 13 tests) + - Window-boundary: 21:59 / 22:00 / 22:59 / 23:00, plus a DST-bearing + `America/New_York` check. + - Opt-in gating, logged-today skip, ledger-already-exists skip, + P2002 lost-race handling, multi-user multi-locale fan-out. +- `messages/{de,en,es,fr,it,pl}.json` + - New `moodReminders.dailyTitle` + `moodReminders.dailyBody` keys + in all six locales. + +## Quality gates + +| Gate | Result | +|---|---| +| `pnpm typecheck` | clean | +| `pnpm lint` | clean | +| `pnpm test` | 4542 tests pass, 1 skipped (430 test files) | +| `pnpm openapi:check` | spec in sync | +| `pnpm format:check` | pre-existing warnings unchanged; my files prettier-formatted | + +## Migration + +`prisma/migrations/0069_v054_mood_reminder/migration.sql` ships with the +PR. Applied via `pnpm db:migrate:deploy` on staging — `IF NOT EXISTS` +guards make re-apply a no-op. + +## Blockers + +None. + +## Follow-ups for the iOS contributor + +- iOS app must register `UNNotificationCategory` identifiers + `MEDICATION_REMINDER` (Take / Snooze 15 min / Skip) and + `MOOD_REMINDER` (Log mood) at launch, otherwise iOS will render the + pushes as plain alerts (the category will be present on the payload + but iOS ignores unknown identifiers). +- iOS Settings → Notifications surface needs a toggle for + `User.moodReminderEnabled` (PATCH /api/me or similar — the + preferences route already exists at + `src/app/api/notifications/preferences/route.ts` but only covers + per-event-type matrix; this is a per-user flag, not a per-channel + preference. Surface TBD by operator). +- The `scheduledAt` metadata field is already present on every + outbound APNs payload for med-reminders — iOS just needs to read it + from `userInfo`. + +## Compatibility + +- APNs payload changes are strictly additive. Older iOS builds that + don't register the categories render plain alerts (iOS silently + ignores unknown categories). +- `MOOD_REMINDER` is double-gated (per-event default OFF + per-user + `moodReminderEnabled` defaults FALSE). Users who never opt in see no + behavioural change.