tom/HealthLog
1
0 Fork 0
mirror of https://github.com/MBombeck/HealthLog.git synced 2026-11-09 04:50:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

docs(planning): land W1 research + W5/W6 review packs

Browse Source 
Captures the artifacts referenced from STATE.md, v1423-backlog.md, and
phase-W6-v1423-reconcile-report.md but never committed during the
wave-close ticks. W1 research = Apple Health type mapping + APNs
library decision + OpenAPI tooling choice; W5 v1422 reviews + W6
v1423 reviews are the six-brief multi-agent QA inputs that drove the
reconcile commit chain.

Leaves the pre-existing stale untracked entries (v1422-w1a probes,
audit-a6 scripts, export-route directories) untouched — those predate
v1.4.23.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Marc-André BombeckandClaude Opus 4.7 committed 2026-05-11 08:28:02 +02:00
1 parent f1e6630958
commit 766a9ae262
14 files changed
+4728
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.planning/phase-W1-v1423-research.md
+912
View File
@@ -0,0 +1,912 @@
# Wave 1 — v1.4.23 research (Apple Health + APNs + OpenAPI)
Status: research-only. No source files modified. Outputs feed W2
(Apple Health foundation), W3 (APNs scaffolding), and W4
(OpenAPI gate + Coach schema slot + native auth).
Cross-references:
- `.planning/STATE.md` Wave 2-4 sub-items
- `prisma/schema.prisma` lines 189-245 (`MeasurementType`,
`MeasurementSource`, `Measurement`)
- `src/lib/withings/client.ts` lines 122-133 (mapping pattern to
mirror)
- `src/lib/notifications/dispatcher.ts` (cascade we extend)
- `src/lib/notifications/senders/web-push.ts` (sender shape to
mirror, returns `SendOutcome`)
- `~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Models/MeasurementDTO.swift`
(already-shipped iOS DTO — names the source `HEALTHKIT`, not
`APPLE_HEALTH`)
- `~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Services/HealthKitService.swift`
(already-wired `defaultReadTypes` set)
---
## Stream 1 — Apple Health mapping
### Naming-decision callout (read first)
The iOS app already ships an enum value `HEALTHKIT` for the
measurement-source field in
`MeasurementDTO.swift` (line 60):
```swift
case healthKit = "HEALTHKIT"
```
The marathon brief calls it `apple_health` / `APPLE_HEALTH`. Two
options:
1. **Server adopts `HEALTHKIT`** — matches the already-shipped
iOS DTO, no client-side change. Cost: the source name leaks
the framework name (HealthKit) rather than the brand
(Apple Health) into our DB enum. Apple's own marketing surface
says "Apple Health", but their developer surface says
"HealthKit".
2. **Server adopts `APPLE_HEALTH`** — matches the marathon brief
and Marc's user-facing voice ("Apple Health"). Cost: iOS DTO
needs a one-line rename + compatibility shim for any encoded
payloads already on disk in the iOS sim build.
**Recommendation: `APPLE_HEALTH`.** The DB enum is a stable
contract value, not a brand label, but it is also user-facing
through analytics/audit-log surfaces. "Apple Health" is the term
users recognise. The iOS DTO is still pre-TestFlight (no real
users on the encoded format yet) so the rename is essentially
free.
W2 should land the rename in `MeasurementDTO.swift` in the same
PR that lands the server enum so the contract stays in lock-step.
### Mapping table
Aggregation rule reads:
- `latest` — store every sample as-is, dedup by `(externalId)`
- `sum` — sum samples in a 1-hour rollup window (Apple's hourly
observer-query batch grain)
- `mean` — arithmetic mean of values in the rollup window
- `last_per_window` — keep the chronologically latest value when
multiple samples land in one upload batch (Apple's discrete-
scalar pattern, e.g. resting HR)
| HK identifier | DB MeasurementType | DB unit | HK unit | Conversion | Aggregation | Privacy / consent | Notes |
| -------------------------- | -------------------------------- | ----------- | ----------------------------------------------------- | ----------------------- | --------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------- |
| `bodyMass` | `WEIGHT` (existing) | kg | `.gramUnit(.kilo)` | identity | latest | already in scope | already shipped; no schema change |
| `bloodPressureSystolic` | `BLOOD_PRESSURE_SYS` (existing) | mmHg | `.millimeterOfMercury()` | identity | latest | already in scope | always paired with diastolic; iOS posts both as separate rows on identical `measuredAt` (matches Withings pattern) |
| `bloodPressureDiastolic` | `BLOOD_PRESSURE_DIA` (existing) | mmHg | `.millimeterOfMercury()` | identity | latest | already in scope | see above |
| `heartRate` | `PULSE` (existing) | bpm | `.count()/.minute()` | identity | last_per_window | already in scope | spot pulse only; Apple Watch generates many per day — iOS must throttle (e.g. one sample every 5 min) before posting or the batch saturates |
| `heartRateVariabilitySDNN` | `HEART_RATE_VARIABILITY` (NEW) | ms | `.secondUnit(.milli)` | identity | mean | yes (new — explicit consent screen) | Apple captures during sleep window; one sample per night typical |
| `restingHeartRate` | `RESTING_HEART_RATE` (NEW) | bpm | `.count()/.minute()` | identity | last_per_window | already in scope (umbrella with PULSE) | Apple emits one per day from sleep + low-activity windows |
| `stepCount` | `STEP_COUNT` (NEW) | steps | `.count()` | identity | sum | already in scope | cumulative; iOS posts daily totals at 23:55 local + a delta on observer wake |
| `activeEnergyBurned` | `ACTIVE_ENERGY_BURNED` (NEW) | kcal | `.kilocalorie()` | identity | sum | already in scope | cumulative; same daily-rollup pattern as steps |
| `flightsClimbed` | `FLIGHTS_CLIMBED` (NEW) | flights | `.count()` | identity | sum | already in scope | cumulative; same daily rollup |
| `distanceWalkingRunning` | `WALKING_RUNNING_DISTANCE` (NEW) | m | `.meter()` | identity | sum | already in scope | cumulative; SI metres in DB, UI converts to km/mi per locale |
| `vo2Max` | `VO2_MAX` (NEW) | mL/(kg·min) | `.literUnit(.milli) / (.gramUnit(.kilo) · .minute())` | identity | last_per_window | yes (new — fitness/health-grade metric) | one sample per few days at most |
| `sleepAnalysis` | `SLEEP_DURATION_DETAILED` (NEW) | minutes | category | sum durations per stage | sum | yes (new — explicit consent) | NOT a quantity type; see "Sleep schema branch" below |
**Net new MeasurementType values: 8.**
`HEART_RATE_VARIABILITY`, `SLEEP_DURATION_DETAILED`,
`RESTING_HEART_RATE`, `STEP_COUNT`, `ACTIVE_ENERGY_BURNED`,
`FLIGHTS_CLIMBED`, `WALKING_RUNNING_DISTANCE`, `VO2_MAX`.
These mirror the values listed in `STATE.md` Wave 2 F1 verbatim
except for the rename `ACTIVITY_STEPS` (existing) →
`STEP_COUNT` (new). See "Naming collision" below.
### Naming collision: existing `ACTIVITY_STEPS` and `SLEEP_DURATION`
The current `MeasurementType` enum already contains
`ACTIVITY_STEPS` (steps unit) and `SLEEP_DURATION` (hours unit).
Neither has any production data path today — `getUnitForType`
returns `steps`/`hours` and the `validateMeasurementRange` row
exists, but no ingest route or UI surface wires either.
Two options:
1. **Reuse the existing `ACTIVITY_STEPS` and `SLEEP_DURATION`
enum values** for Apple Health steps + sleep, and SKIP adding
`STEP_COUNT` / `SLEEP_DURATION_DETAILED`. Net new = 6.
2. **Add the new `STEP_COUNT` / `SLEEP_DURATION_DETAILED` values
and deprecate the originals** (no production data → safe).
Net new = 8 (matches STATE.md).
**Recommendation: option 1, reuse `ACTIVITY_STEPS` and
`SLEEP_DURATION`.** The existing values were added in
anticipation of exactly this work (per the comment in
`schema.prisma` near lines 195-196). Adding parallel new values
fragments the enum and forces every analytics codepath to handle
two synonyms. The marathon brief's name `STEP_COUNT` is the
HealthKit identifier; our DB convention is the domain noun
(WEIGHT, PULSE, BODY_FAT) — so `ACTIVITY_STEPS` already fits the
house style better than `STEP_COUNT` does.
Sleep needs the unit changed from `hours` to `minutes` (HealthKit
emits sleep durations as minute-resolution category samples;
storing fractional hours loses precision). That's a Wave 2 F1
sub-decision — schema-only since no data exists.
**Net new with option 1 adopted: 6.** Final list:
`HEART_RATE_VARIABILITY`, `RESTING_HEART_RATE`,
`ACTIVE_ENERGY_BURNED`, `FLIGHTS_CLIMBED`,
`WALKING_RUNNING_DISTANCE`, `VO2_MAX`.
### Sleep schema branch (the one that's not a `HKQuantityType`)
`HKCategoryTypeIdentifier.sleepAnalysis` is a category type, not
a quantity type. Each sample has a value drawn from
`HKCategoryValueSleepAnalysis`:
- `inBed`
- `asleepUnspecified` (legacy iOS 15-)
- `awake`
- `asleepCore` (iOS 16+)
- `asleepDeep` (iOS 16+)
- `asleepREM` (iOS 16+)
Each sample has a `startDate` and `endDate`; duration is
`endDate - startDate` in seconds. A single night produces tens of
samples (every stage transition).
Two server-shape options:
1. **Single `Measurement` row per night with summed total** —
value = total minutes asleep, no stage breakdown. Easiest;
loses Apple's stage data the user paid Watch battery for.
2. **One `Measurement` row per stage per night** — value =
minutes in that stage, with a new `sleepStage` enum column.
Symmetrical to the existing `glucoseContext` column.
**Recommendation: option 2 with a new `sleepStage` enum column
on `Measurement`** (`CORE`, `DEEP`, `REM`, `AWAKE`, `IN_BED`).
NULL for non-sleep measurements, mirroring the existing
`glucoseContext` pattern. The Coach extension (W4 F6) will want
the stage breakdown to honour the "no clinical sleep-stage
conclusions" rule from the v1.5 product-lead memo — without the
stage rows we can't audit prompt compliance.
Migration: `ALTER TABLE measurements ADD COLUMN sleep_stage
sleep_stage NULL`. Add a CHECK constraint mirroring the
`glucoseContext` pattern: `sleep_stage` non-NULL only when
`type = SLEEP_DURATION`.
### `apple-health-mapping.ts` skeleton
Path: `src/lib/measurements/apple-health-mapping.ts` (new dir,
new file — symmetrical to the implicit Withings mapping in
`src/lib/withings/client.ts:122-133`).
```ts
import type { MeasurementType } from "@/generated/prisma/client";
/**
* Apple HealthKit identifier → HealthLog `MeasurementType` mapping.
* Mirrors the Withings `MEASURE_TYPE_MAP` in src/lib/withings/client.ts
* but keyed by string identifier (Apple's own constant name) rather
* than a numeric type code.
*
* Values:
* - `type` — DB enum
* - `unit` — canonical DB unit (must match unitMap in
* src/lib/validations/measurement.ts)
* - `aggregate`— how the ingest endpoint should reduce a batch of
* HK samples into stored Measurement rows
* - `consent` — UI must show explicit consent before requesting
* HKObjectType permission for these
*/
export type HealthKitAggregation =
| "latest"
| "sum"
| "mean"
| "last_per_window";
export interface AppleHealthMapping {
type: MeasurementType;
unit: string;
aggregate: HealthKitAggregation;
consent: "implicit" | "explicit";
/**
* Number of HK sample-fractional-units per 1 DB-unit. 1.0 = identity.
* `heartRateVariabilitySDNN` ships as `secondUnit(.milli)` on Apple's
* side and we store `ms` — identity. `oxygenSaturation` ships as a
* 0..1 fraction and we store percent (0..100) — `factor: 100`.
*/
factor: number;
}
export const APPLE_HEALTH_TYPE_MAP: Record<string, AppleHealthMapping> = {
// Body composition
HKQuantityTypeIdentifierBodyMass: {
type: "WEIGHT",
unit: "kg",
aggregate: "latest",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierBodyFatPercentage: {
type: "BODY_FAT",
unit: "%",
aggregate: "latest",
consent: "implicit",
factor: 100, // Apple ships 0..1 fraction
},
// Cardiovascular
HKQuantityTypeIdentifierBloodPressureSystolic: {
type: "BLOOD_PRESSURE_SYS",
unit: "mmHg",
aggregate: "latest",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierBloodPressureDiastolic: {
type: "BLOOD_PRESSURE_DIA",
unit: "mmHg",
aggregate: "latest",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierHeartRate: {
type: "PULSE",
unit: "bpm",
aggregate: "last_per_window",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierRestingHeartRate: {
type: "RESTING_HEART_RATE",
unit: "bpm",
aggregate: "last_per_window",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierHeartRateVariabilitySDNN: {
type: "HEART_RATE_VARIABILITY",
unit: "ms",
aggregate: "mean",
consent: "explicit",
factor: 1,
},
// Activity (cumulative)
HKQuantityTypeIdentifierStepCount: {
type: "ACTIVITY_STEPS", // reuses existing enum value (see report)
unit: "steps",
aggregate: "sum",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierActiveEnergyBurned: {
type: "ACTIVE_ENERGY_BURNED",
unit: "kcal",
aggregate: "sum",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierFlightsClimbed: {
type: "FLIGHTS_CLIMBED",
unit: "flights",
aggregate: "sum",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierDistanceWalkingRunning: {
type: "WALKING_RUNNING_DISTANCE",
unit: "m",
aggregate: "sum",
consent: "implicit",
factor: 1,
},
// Fitness
HKQuantityTypeIdentifierVO2Max: {
type: "VO2_MAX",
unit: "mL/(kg·min)",
aggregate: "last_per_window",
consent: "explicit",
factor: 1,
},
// Other in-scope (already mapped via existing enums)
HKQuantityTypeIdentifierBloodGlucose: {
type: "BLOOD_GLUCOSE",
unit: "mg/dL",
aggregate: "latest",
consent: "implicit",
factor: 1,
},
HKQuantityTypeIdentifierOxygenSaturation: {
type: "OXYGEN_SATURATION",
unit: "%",
aggregate: "last_per_window",
consent: "implicit",
factor: 100, // Apple ships 0..1 fraction; matches schema.prisma comment
},
};
/**
* sleepAnalysis is a category type, not a quantity type — it gets its
* own table-shape branch. Keys here are the values of
* `HKCategoryValueSleepAnalysis`. The ingest endpoint stores one
* Measurement row per (night, stage), value = minutes-in-stage.
*/
export const APPLE_HEALTH_SLEEP_STAGE_MAP: Record<
string,
"CORE" | "DEEP" | "REM" | "AWAKE" | "IN_BED"
> = {
HKCategoryValueSleepAnalysisAsleepCore: "CORE",
HKCategoryValueSleepAnalysisAsleepDeep: "DEEP",
HKCategoryValueSleepAnalysisAsleepREM: "REM",
HKCategoryValueSleepAnalysisAwake: "AWAKE",
HKCategoryValueSleepAnalysisInBed: "IN_BED",
// legacy iOS 15- — count as CORE for back-compat
HKCategoryValueSleepAnalysisAsleepUnspecified: "CORE",
};
```
W2's F1+F2+F3 implementer fills in the migration + the
`POST /api/measurements/batch` route. The route's per-entry
validator just looks up `APPLE_HEALTH_TYPE_MAP[entry.hkIdentifier]`,
applies the `factor`, and inserts with `source = APPLE_HEALTH` +
`externalId = entry.uuid` (HealthKit's `HKSample.uuid` is stable
across queries — it's the dedup key).
### Open questions for the maintainer
1. **`HEALTHKIT` vs `APPLE_HEALTH` source enum value.** This
research recommends `APPLE_HEALTH` and renaming the iOS DTO
in the same PR. Confirm or override before W2 starts.
2. **Reuse `ACTIVITY_STEPS` + `SLEEP_DURATION` (option 1 above)
or add `STEP_COUNT` + `SLEEP_DURATION_DETAILED` as parallel
values?** Recommendation: reuse, change SLEEP unit to minutes.
3. **Sleep stage breakdown — single row per night vs per-stage
row with new `sleep_stage` column?** Recommendation:
per-stage, mirrors the `glucoseContext` pattern.
---
## Stream 2 — APNs library decision
### Comparison
| axis | `@parse/node-apn` | `apns2` (AndrewBarba) | hand-rolled `node:http2` |
| ---------------------- | -------------------------------------------------------------- | ---------------------------------------------------- | ----------------------------- |
| last release | 8.1.0, April 2026 (active) | 12.2.0, May 2025 (12 months ago, but stable surface) | n/a |
| weekly downloads | ~107k | ~6k | n/a |
| GitHub stars | ~219 | ~173 | n/a |
| TypeScript types | built-in `index.d.ts` | 100% TypeScript codebase, types are the source | hand-rolled |
| ESM support | hybrid CJS/ESM | ESM + CJS (v12) | native |
| HTTP/2 implementation | native `http2`, connection pool | native `http2`, connection pool | manual |
| JWT signing | bundled | bundled | manual (jsonwebtoken or jose) |
| Sandbox vs prod toggle | `production: boolean` flag | `host` string override | manual base URL |
| Dependencies | `debug`, `verror`, `node-forge`, `jsonwebtoken` | only `jsonwebtoken` (small graph) | none |
| License | MIT | MIT | n/a |
| Maintainer | Parse Platform org (multi-maintainer, Parse Server downstream) | one maintainer | n/a |
| Test mockability | Stub the `Provider`; existing examples in Parse Server tests | Stub `ApnsClient` interface | trivial — mock fetch shape |
### Recommendation: **`@parse/node-apn`**
The active 8.1.0 release in April 2026 plus the 100k+
weekly-download install base under Parse Server is the dominant
signal. `apns2` is technically cleaner (single small dep, native
TypeScript codebase) but its maintenance pulse went quiet 12
months ago and the maintainer count is one. APNs's spec is
stable enough that "12 months quiet" is not a red flag on its
own, but if Apple introduces a new push type (precedent: the
`live-activity` type in iOS 16) we want a library whose issues
get triaged.
The downside is a slightly heavier dep graph (`node-forge`,
`verror`) but those are dependencies HealthLog's existing
notification stack already pulls transitively (`web-push` pulls
`node-forge` for VAPID JWT). Net new install footprint is small.
For tests, `dispatcher.test.ts`'s pattern of stubbing
sender modules at import time (see how `web-push` is treated)
applies cleanly: a test-only `mock-apns.ts` exporting the same
`sendViaApns(config, payload): SendOutcome` shape sidesteps the
library entirely. We **do not** need to mock the Parse provider
class — we only ever call our own wrapper.
Hand-rolled HTTP/2 is rejected: even though the protocol is
~150 lines, the JWT-key rotation logic (regenerate every 50 min,
share across requests, handle 403 InvalidProviderToken by
forcing immediate rotation) is exactly the kind of code that
sits unmaintained for years until Apple shifts a corner-case
status code. Outsourcing is correct here.
### Code sketch + env-var contract
Path: `src/lib/notifications/senders/apns.ts` (new).
```ts
import apn from "@parse/node-apn";
import { decrypt } from "@/lib/crypto";
import type { NotificationPayload } from "@/lib/notifications/types";
import type { SendOutcome } from "@/lib/notifications/retry-policy";
import { classifyHttpStatus } from "@/lib/notifications/retry-policy";
import { prisma } from "@/lib/db";
import { getEvent } from "@/lib/logging/context";
interface ApnsEnv {
keyId: string;
teamId: string;
bundleId: string;
signingKey: string; // .p8 PEM contents (multiline)
production: boolean;
}
function loadEnv(): ApnsEnv | null {
const keyId = process.env.APNS_KEY_ID;
const teamId = process.env.APNS_TEAM_ID;
const bundleId = process.env.APNS_BUNDLE_ID;
// APNS_KEY_FILE = path to .p8 (preferred for dev). APNS_KEY = inline
// contents (preferred for Coolify env-block). One must be set.
const signingKey =
process.env.APNS_KEY ??
(process.env.APNS_KEY_FILE
? require("node:fs").readFileSync(process.env.APNS_KEY_FILE, "utf8")
: undefined);
if (!keyId || !teamId || !bundleId || !signingKey) return null;
const production = process.env.APNS_PRODUCTION === "true";
return { keyId, teamId, bundleId, signingKey, production };
}
let cachedProvider: apn.Provider | null = null;
function getProvider(env: ApnsEnv): apn.Provider {
// node-apn re-uses HTTP/2 connections + JWT internally; one Provider
// per process is the documented pattern.
if (cachedProvider) return cachedProvider;
cachedProvider = new apn.Provider({
token: { key: env.signingKey, keyId: env.keyId, teamId: env.teamId },
production: env.production,
});
return cachedProvider;
}
/**
* Send an APNs alert push to all registered devices for `userId`.
* Mirrors the SendOutcome contract used by web-push so the dispatcher
* can apply the same retry/cooldown logic uniformly.
*/
export async function sendViaApns(
userId: string,
payload: NotificationPayload,
): Promise<SendOutcome> {
const env = loadEnv();
if (!env) {
return { ok: false, hardReject: false, reason: "apns_not_configured" };
}
const devices = await prisma.device.findMany({
where: { userId, platform: "ios" },
});
if (devices.length === 0) {
return { ok: false, hardReject: false, reason: "apns_no_devices" };
}
const provider = getProvider(env);
const note = new apn.Notification();
note.topic = env.bundleId;
note.alert = { title: payload.title, body: payload.message };
note.sound = "default";
note.payload = { eventType: payload.eventType, ...payload.metadata };
const start = performance.now();
const tokens = devices.map((d) => d.token);
const result = await provider.send(note, tokens);
getEvent()?.addExternalCall({
service: "apns",
method: "send",
duration_ms: Math.round(performance.now() - start),
status: 200,
error: result.failed.length === devices.length ? "all_failed" : undefined,
});
// 410 BadDeviceToken / 400 BadDeviceToken / "Unregistered" → drop the
// device row (pattern mirrors web-push 410 cleanup)
const dead = result.failed
.filter(
(f) =>
f.response?.reason === "Unregistered" ||
f.response?.reason === "BadDeviceToken",
)
.map((f) => f.device);
if (dead.length > 0) {
await prisma.device.deleteMany({ where: { token: { in: dead } } });
}
if (result.sent.length > 0) return { ok: true };
// All failed and all permanent → hard reject. Else transient.
if (dead.length === devices.length) {
return {
ok: false,
hardReject: true,
statusCode: 410,
reason: "apns_all_devices_unregistered",
};
}
const lastStatus = result.failed[0]?.status
? Number(result.failed[0].status)
: undefined;
const classified = classifyHttpStatus(lastStatus, "apns");
return {
ok: false,
hardReject: false,
statusCode: lastStatus,
reason: classified.reason,
};
}
```
**Env-var contract:**
| var | required | shape | notes |
| ----------------- | -------- | ------------------------------------------------ | ---------------------------- |
| `APNS_KEY_ID` | yes | 10-char alphanumeric | from Apple developer account |
| `APNS_TEAM_ID` | yes | 10-char alphanumeric | from Apple developer account |
| `APNS_BUNDLE_ID` | yes | reverse-DNS string (e.g. `dev.healthlog.ios`) | `note.topic` |
| `APNS_KEY` | one-of | multiline PEM (`-----BEGIN PRIVATE KEY-----...`) | preferred for Coolify |
| `APNS_KEY_FILE` | one-of | absolute path | preferred for local dev |
| `APNS_PRODUCTION` | optional | `"true"` or unset | default = sandbox |
The `.p8` PEM contents are sensitive — encrypt at rest if we
ever start storing per-tenant keys. For v1.4.23 single-tenant
the env-var route is correct.
### `apns_attempts` table — reuse, don't add
The existing `NotificationChannel` model carries
`consecutiveFailures`, `nextRetryAt`, `lastFailureReason`,
`lastSuccessAt`, `disabledReason` — that's exactly the per-
channel retry state we need. APNs joins `TELEGRAM`, `NTFY`,
`WEB_PUSH` as a fourth `ChannelType` value.
**No new attempts table.** Reuse the channel-state machine in
`src/lib/notifications/channel-state.ts`. The Wave 3 tasks are:
1. Add `"APNS"` to `CHANNEL_TYPES` in
`src/lib/notifications/types.ts`.
2. Switch case in `dispatcher.ts:sendToChannel()` calls
`sendViaApns(payload.userId, payload)` (mirror the
`WEB_PUSH` branch — also takes `userId` not `config`,
because device tokens live in `Device` not in
`NotificationChannel.config`).
3. The `NotificationChannel` row for an APNs user carries an
empty config (like `WebPushChannelConfig`); the row exists
so the per-event preference toggles in
`NotificationPreference` work uniformly across all channels.
### Open questions for the maintainer
1. **Push type**: `alert` only for v1.4.23, or also `background`
silent pushes (for HealthKit observer-query "wake the iOS app"
pattern)? Background pushes need
`apns-push-type: background` header + priority 5; Apple
throttles them aggressively. Recommendation: `alert`-only
for v1.4.23, defer background to v1.5 P4.
2. **Per-device opt-out**: today's `NotificationPreference` is
per `(channel, eventType)`. With multiple iOS devices on one
account, do we need per-device-token opt-out (mute APNs on
one phone but not the other)? Punt to v1.5 P4 unless Marc
says otherwise.
3. **Sandbox auto-detect**: the iOS app builds with two schemes
(Debug → sandbox APNs gateway, Release → production). The
server can't tell which gateway a device-token came from
except by trial-and-error. Two options: (a) the iOS app
includes `apnsEnvironment: "sandbox"|"production"` in
`POST /api/devices`, (b) the server tries production first
and falls back to sandbox on `BadDeviceToken`. Recommendation:
(a) — explicit. Add `apnsEnvironment` column to `Device`
model in W3.
---
## Stream 3 — OpenAPI tooling decision
### Comparison
| axis | `@asteasolutions/zod-to-openapi` | `zod-openapi` (samchungy) | `ts-rest` / `next-rest` | hand-rolled |
| -------------------------------- | ---------------------------------------------------- | ----------------------------------------------------------------- | ---------------------------------------- | ------------ |
| weekly downloads | ~1.5M | ~250k | ~50k each | n/a |
| GitHub stars | ~1.5k | ~640 | ~5k / ~200 | n/a |
| Zod v4 support | yes (registry pattern unchanged) | yes (uses `.meta()` — Zod v4 native) | yes | n/a |
| OpenAPI 3.1 emit | yes | yes (last release Jan 2026 = active) | partial | n/a |
| Schema annotation style | `.openapi({...})` (registers extension) | `.meta({...})` (zod-native) | route-binding DSL | json by hand |
| Setup cost | extend Zod once at boot, register schemas/routes | import once, call `.meta()` per schema | rewrite all route handlers | high |
| Retrofit cost on existing routes | low — schemas already exist, register in a side file | low — annotate schemas in place, add a side file with route table | **high** — every route handler rewritten | high |
| Coupling to Zod internals | medium (custom `.openapi()` method) | low (uses Zod v4's standard `.meta()`) | high (own DSL) | none |
| Active maintenance pulse | active | active (v5.4.6 January 2026) | active | n/a |
### Recommendation: **`zod-openapi` (samchungy)**
Two reasons:
1. **Uses Zod v4's native `.meta()` method.** HealthLog already
imports from `zod/v4` (per CLAUDE.md). `zod-openapi` reads
the metadata that's already a first-class Zod v4 concept —
no monkey-patching, no module-augmentation, no boot-time
`extendZodWithOpenApi()` call. If Zod ever changes its
metadata internals, the breakage surface is one library
maintainer away from a fix; with `zod-to-openapi` it's two.
2. **Lighter retrofit footprint.** HealthLog has ~80+ routes
under `src/app/api/`. Each has either an inline Zod schema
or imports from `src/lib/validations/*`. With `zod-openapi`,
we annotate the existing exported schemas in their original
files (`createMeasurementSchema.meta({...})`) and wire a
small `src/lib/openapi/registry.ts` that imports them and
composes the `paths` object. With `zod-to-openapi`, every
schema needs a `.openapi()` call AND a `registry.register()`
AND a `registry.registerPath()` — ~3x the touchpoints.
`@asteasolutions/zod-to-openapi` is the safer-by-popularity
choice (10x the downloads) but the popularity is mostly Hono /
Express / NestJS users where it's the only option. For Next.js
App Router with Zod v4 already in the stack, `zod-openapi`'s
`.meta()` story is the better fit and keeps the spec generation
out of the route-handler hot path.
`ts-rest` / `next-rest` are rejected: both want to own the
route-handler shape (their `defineRoute()` wraps the handler).
Retrofitting 80+ existing `apiHandler()`-wrapped routes is a
multi-week refactor and would absorb the same surface area
v1.4.23 is supposed to be locking down, not churning.
### Implementation sketch
Three files, ~120 LOC total.
**`src/lib/openapi/registry.ts`** (new) — composes the spec
from already-exported Zod schemas:
```ts
import * as z from "zod/v4";
import { createDocument } from "zod-openapi";
import {
createMeasurementSchema,
createBatchMeasurementSchema,
measurementTypeEnum,
measurementSourceEnum,
} from "@/lib/validations/measurement";
import { errorEnvelopeSchema, dataEnvelopeSchema } from "@/lib/api-response";
// ... import every schema we want in the spec
export function buildOpenApiSpec() {
// Annotate schemas where they're not annotated in-place (fallback
// path; preferred is to call .meta() in the validations file).
measurementTypeEnum.meta({ id: "MeasurementType" });
measurementSourceEnum.meta({ id: "MeasurementSource" });
return createDocument({
openapi: "3.1.0",
info: {
title: "HealthLog API",
version: process.env.npm_package_version ?? "1.4.23",
},
servers: [{ url: "https://healthlog.bombeck.io" }],
paths: {
"/api/measurements": {
get: {
/* ...listMeasurementsSchema as query params... */
},
post: {
requestParams: {
header: z.object({
"Idempotency-Key": z.string().optional(),
}),
},
requestBody: {
content: {
"application/json": { schema: createMeasurementSchema },
},
},
responses: {
"201": {
description: "Created",
content: {
"application/json": {
schema: dataEnvelopeSchema(
z.object({
id: z.string(),
}),
),
},
},
},
},
},
},
"/api/measurements/batch": {
post: {
requestBody: {
content: {
"application/json": { schema: createBatchMeasurementSchema },
},
},
responses: {
"200": {
/* ... */
},
},
},
},
// ... one per route
},
});
}
```
**`scripts/openapi-generate.ts`** (new):
```ts
#!/usr/bin/env tsx
import { writeFileSync } from "node:fs";
import * as YAML from "yaml";
import { buildOpenApiSpec } from "@/lib/openapi/registry";
const spec = buildOpenApiSpec();
const yaml = YAML.stringify(spec);
writeFileSync("docs/api/openapi.yaml", yaml, "utf8");
console.log(`wrote docs/api/openapi.yaml (${yaml.length} bytes)`);
```
**`scripts/openapi-check.ts`** (new) — CI gate:
```ts
#!/usr/bin/env tsx
import { readFileSync } from "node:fs";
import { execSync } from "node:child_process";
import * as YAML from "yaml";
import { buildOpenApiSpec } from "@/lib/openapi/registry";
const generated = YAML.stringify(buildOpenApiSpec());
const onDisk = readFileSync("docs/api/openapi.yaml", "utf8");
if (generated.trim() !== onDisk.trim()) {
console.error(
"OpenAPI spec drift detected. Run `pnpm openapi:generate` and commit.",
);
// Helpful diff for the PR reviewer
execSync('diff <(echo "$GEN") docs/api/openapi.yaml || true', {
env: { ...process.env, GEN: generated },
stdio: "inherit",
shell: "/bin/bash",
});
process.exit(1);
}
console.log("OpenAPI spec in sync with source schemas.");
```
**`package.json` scripts:**
```json
{
"openapi:generate": "tsx scripts/openapi-generate.ts",
"openapi:check": "tsx scripts/openapi-check.ts"
}
```
**CI gate:** add a step to the existing GH Actions PR workflow:
```yaml
- name: OpenAPI spec sync check
run: pnpm openapi:check
```
The check is structural-equality on YAML text. For a more
robust diff we can swap to `oasdiff` (Go binary, GitHub Action
ready, generates a markdown breaking-change report) — but
sequence-equality is the cheaper W4 starting point and we can
upgrade to oasdiff in v1.4.24 without changing the generator
side.
### Migration approach for the existing 5468-line hand-maintained spec
The committed `docs/api/openapi.yaml` is large (80+ paths, 100+
schemas). Two-stage transition:
1. **W4 ships the generator covering the 6-8 routes the iOS app
touches** (auth/login, auth/refresh, auth/me, measurements
GET/POST, measurements/batch POST, devices POST, achievements
GET). The CI check runs in **warn-only mode** (`continue-on-
error: true`) until coverage is complete.
2. **v1.4.24+ migrates the rest of the spec** route by route as
they're touched by future PRs (organic adoption). Once
coverage hits ~95%, flip `openapi:check` to fail-the-PR.
This avoids the "rewrite the whole spec in one PR" anti-pattern.
The committed YAML stays the source-of-truth-for-iOS during the
transition; the generator's output is a strict subset of it.
### Open questions for the maintainer
1. **Subset start vs full migration?** Recommendation: subset
(the 6-8 iOS-touched routes), warn-only CI for v1.4.23, full
coverage organically.
2. **YAML normalisation**: zod-openapi emits stable key order
per its docs but YAML `.stringify` may reorder map keys
between Node versions. Pin `yaml@2.x` and add a normalisation
pass before equality compare? Recommendation: yes — use
`yaml.stringify(spec, { sortMapEntries: true })` for both
sides.
3. **`oasdiff` adoption**: defer to v1.4.24, or ship now? Cost
of shipping now is one extra GH Action download + one binary
on the runner; benefit is structured breaking-change
detection on PR comments. Recommendation: defer — text-diff
covers the W2/W3 contract-locking goal.
---
## Cross-stream notes
### iOS DTO already encodes some Wave 2 decisions
`MeasurementDTO.swift` includes a `BODY_TEMPERATURE` enum value
that does NOT exist in the server's `MeasurementType` enum
today. Either:
- iOS will skip body-temperature ingest for v1.4.23 (filter on
the client), OR
- W2 adds `BODY_TEMPERATURE` as a 7th new value (HealthKit type
`HKQuantityTypeIdentifierBodyTemperature`, unit `degC`).
Recommendation: **add `BODY_TEMPERATURE` in W2 alongside the
other 6 new types.** Cheap to ship a no-op enum value now versus
breaking the iOS DTO contract later.
Net new MeasurementType count if both options accepted:
**7** (`HEART_RATE_VARIABILITY`, `RESTING_HEART_RATE`,
`ACTIVE_ENERGY_BURNED`, `FLIGHTS_CLIMBED`,
`WALKING_RUNNING_DISTANCE`, `VO2_MAX`, `BODY_TEMPERATURE`).
### Coach schema slot (W4 F6) preview
`src/lib/ai/schema.ts:260` defines:
```ts
sourceMetric: z.enum(["bp", "weight", "pulse", "mood", "compliance"]);
```
W4 extends to:
```ts
sourceMetric: z.enum([
"bp",
"weight",
"pulse",
"mood",
"compliance",
// v1.4.23 additive (forward-compat for v1.5 iOS)
"hrv",
"sleep",
"restingHr",
"steps",
"activeEnergy",
]);
```
`src/lib/ai/coach/types.ts:132` defines `CoachProvenance.metrics`
with the same value set — extend in lock-step. Both are additive,
no migration on `coach_messages.metricSourceJson` (TEXT column,
opaque to Postgres).
PROMPT_VERSION ratchet 4.22.0 → 4.23.0 (additive forward-compat,
matches the project_v1423_pre_ios_prep.md scope point 6).
### Top 3 sequencing risks for W2/W3/W4
1. **W2 + W4 F6 share files** (`src/lib/ai/schema.ts`,
`src/lib/ai/coach/types.ts`). Land W2 first, F6 second, no
parallel edit.
2. **The iOS DTO rename `HEALTHKIT` → `APPLE_HEALTH`** is a
contract-break for the iOS app's encoded payloads on disk.
Clear the iOS sim data store after the rename (one-line in
the iOS dev README).
3. **W4 OpenAPI generator imports schemas from W2**. Land W2
before W4 starts the generator file, OR have W4 use
`as const` placeholders for the W2-introduced enum values
that get filled in once W2 lands.
.planning/phase-W1b-coach-prompt-research.md
+791
View File
@@ -0,0 +1,791 @@
# Wave 1b — Coach prompt rewrite research (v1.4.22)
Research-only artifact for the v1.4.22 polishing marathon. Wave 3 dispatches the
rewrite based on the contract proposed below. Production today is on v1.4.21
(image digest `sha256:4e818d44702c…`); the Coach drawer + SSE streaming +
day-level snapshot have shipped since v1.4.20.
The maintainer's complaint, in plain English: the Coach answers the right
question with the right caveats, but it sounds like a database with a "no rows
returned" error rather than a person who happens to be looking at health data.
We need warmth and motivational-interviewing flavour without losing the
zero-hallucination ground rules from v1.4.20. The values that the Coach used
to hand-deliver inline should move into a collapsible block at the bottom of
the message, leaving the prose itself prose-first.
---
## 1. Current state
File: `src/lib/ai/coach/system-prompt.ts` (lines 19–81 EN, 83–150 DE).
Imports `PROMPT_VERSION` from
`src/lib/ai/prompts/insight-generator.ts` (currently `"4.20.2"`).
**Persona framing today** (EN, lines 19–28):
> You are the HealthLog Coach — a conversational summariser of the user's own
> health-tracking data. […]
>
> YOUR ROLE
>
> - You ONLY discuss the user's own measurements (blood pressure, weight,
> pulse, mood, medications) and the trends inside the SNAPSHOT block the
> user prompt carries.
> - You DO NOT diagnose, prescribe, give general medical advice, or answer
> questions outside the snapshot.
This is accurate but flat. "Conversational summariser" describes a function,
not a person. Nothing in the persona invites motivation, curiosity, or warmth
— so the model defaults to a database-cursor voice.
**Conversation style today** (EN, lines 29–37):
> CONVERSATION STYLE
>
> - Plain prose, no JSON, no markdown fences. […]
> - One self-contained reply per turn. Keep replies focused (≈ 80-220 words).
> - Address the user as "you". Never invent a name.
Good guardrails. No tone calibration. No examples. The model gets length
budget but not shape budget.
**Ground rules** (EN, lines 39–50):
> GROUND RULES — ZERO HALLUZINATIONS
>
> 1. Every number you mention must come from the SNAPSHOT. […]
> 2. When the SNAPSHOT lacks the information needed to answer, say so
> explicitly ("I do not have HRV data in your log yet"). Do NOT invent
> a substitute.
> 3. When the user asks for a recommendation that is potentially actionable
> (medication change, urgent threshold), end the reply with "please
> consult your doctor" or equivalent.
> 4. Stay calm and factual. Do not open with a compliment about data
> quantity or quality.
Rule 2 is the single biggest tone problem. The bracketed example
(`"I do not have HRV data in your log yet"`) reads as a fault report. A
human coach hearing the same gap would say "I don't see exercise in what
you're tracking — what does a typical week look like for you?" and pivot to
collaborative discovery. Rule 4's anti-flattery clause is correct in spirit
but currently absorbs the entire warmth budget — the model overcorrects into
clinical neutrality.
**Day-level timeline section** (EN, lines 52–70). This stays. It teaches the
model how to navigate `timeline.recent` / `timeline.weekly`. Wave 3 rewrites
should preserve this verbatim — it's load-bearing for v1.4.20.1 day-level
answers.
**Tonal contrast with the Insight prompt** (`prompts/insight-generator.ts`,
lines 32–293). The Insight prompt is a JSON contract with eleven numbered
ground rules. The Coach prompt re-uses that prompt's vocabulary
(`SNAPSHOT`, `metricSource`, `last7days/30/90/allTime`) which is correct for
data-grounding but currently borrows its register too. Insights are static
reports a clinician might glance at; the Coach is a chat bubble a person
reads on their phone after a workout. The two need to stop sounding alike.
**No tests bind the literal prompt body**. `grep` of
`src/lib/ai/coach/__tests__/` and `src/lib/ai/__tests__/` finds no assertion
on the Coach prompt string — only on `refusal`, `budget`, `snapshot`, and
`persistence`. Wave 3 can rewrite the body freely without breaking
deterministic tests, provided the EN/DE export functions and `PROMPT_VERSION`
import remain stable.
---
## 2. Benchmark research
### Motivational interviewing (OARS / spirit of MI)
The canonical health-coach communication framework, used by every serious
behavior-change product (Headspace Ebb, Bloom, GPTCoach, ChatThero). Two
layers worth pulling in:
**The Spirit of MI**: Partnership, Acceptance, Compassion, Evocation.
("Drawing forth the client's existing resources and internal motivation.")
The coach is _not_ the expert; the user is. The coach helps the user
articulate what the user already half-knows.
**OARS**: the four micro-skills. Concrete sample phrases from the
Homeless Hub OARS guide:
| Skill | Sample phrase |
| -------------------- | -------------------------------------------------------------------------------------------------- |
| Open-ended question | "How can I help you with \_\_\_?" / "What have you tried before to make a change?" |
| Affirmation | "You handled yourself really well in that situation." |
| Reflective listening | "So you feel…" / "It sounds like you…" |
| Summary | "Let me see if I understand so far…" / "Here is what I've heard. Tell me if I've missed anything." |
**Key MI rule for prompt design**: "MI avoids unsolicited advice,
confronting, instructing, directing, or warning. Instead, it uses the
**Elicit-Provide-Elicit** structure: asking what clients know, requesting
permission to share information, then exploring their response."
(motivationalinterviewing.org). This is exactly what the maintainer's
exemplar response does — "share with me how your week looks, then we
think it through together".
**One-line takeaway**: build the persona around partnership and evocation
(not around being a summariser), and code OARS micro-skills directly into
ground rules so the model picks open-ended questions over flat refusals.
### Headspace Ebb
> "Ebb's conversational style incorporates topic guardrails and techniques
> such as motivational interviewing, an evidence-based technique that
> encourages growth through self-reflection and increased understanding of
> their emotional and cognitive patterns."
> (headspace.com)
Ebb is publicly framed as a _companion_, not an assistant. Headspace's AI
principles page underlines: clinical-team-shaped, "compassionate,
appropriate, and rooted in mental health best practices", explicit "not a
substitute for human care" disclaimer.
**One-line takeaway**: a successful empathetic-AI product names its
discipline (MI) explicitly in its public framing. Our prompt should do the
same internally — naming MI in the persona statement gives the model a
trained register to reach for.
### WHOOP Coach
> "WHOOP Coach takes proprietary WHOOP algorithms […] and a member's unique
> biometric data to identify patterns and connections in their WHOOP
> Recovery, Strain, Sleep, Health, and Stress data. Unlike most AI
> platforms that rely on generic information, WHOOP combines 24/7
> biometric data with the context you share about your goals and
> routines." (whoop.com)
WHOOP's framing is "your data + the context you share". They explicitly
invite the user to fill the gaps the wearable can't see — exactly the
move the maintainer's exemplar response makes (ask the user about their
typical week when exercise data isn't tracked).
**One-line takeaway**: when data is missing, the right move is to invite
the user to fill the gap conversationally, not to refuse. The Coach
should be primed to do this.
### GPTCoach / Bloom / ChatThero (research literature)
JMIR scoping review _e78417_ (2025) and arxiv 2405.06061 (Stanford
Biodesign 2024) document LLM-based MI agents that beat baseline GPT-4
on readiness-to-change scores when MI is _encoded into the prompt_, not
hoped-for at temperature. Bloom's design (arxiv 2510.05449) uses a
two-agent pattern: agent 1 picks an MI strategy from 11 options, agent 2
crafts the response in that strategy.
We won't ship the two-agent pattern (cost, latency, complexity for a
~600-token chat reply), but the underlying insight stands: **the prompt
must enumerate the conversational moves** the model is allowed to make,
or the model defaults to the lowest-friction option (refuse / restate
the data / disclaim).
**One-line takeaway**: name the MI moves (open question, affirmation,
reflection, summary, gap-invitation, redirect) in the ground rules so the
model has a vocabulary to reach for instead of "I can't help with that".
### Anthropic prompt-engineering docs
From `docs.anthropic.com/.../prompt-engineering/system-prompts`:
- "Claude Opus 4.7 is more direct and opinionated, with less
validation-forward phrasing… If your product voice is warmer or more
conversational, add: _Use a warm, collaborative tone. Acknowledge the
user's framing before answering._"
- "Examples are one of the most reliable ways to steer Claude's output
format, tone, and structure. A few well-crafted examples (known as
few-shot or multishot prompting) can dramatically improve accuracy and
consistency. […] Include 3–5 examples for best results."
- For refusal patterns: "Claude's refusal strategy is to not explain
potential negative consequences but instead offer helpful alternatives
if possible, or keep its refusal concise (1-2 sentences)."
**One-line takeaway**: bake 3–5 bad/good few-shot pairs into the prompt
itself (within `<examples>` tags), and use Anthropic's exact warm-tone
phrase ("acknowledge the user's framing before answering") since the
provider chain runs through Claude in production today.
---
## 3. Proposed new Coach contract
### Persona
> You are the HealthLog Coach. You sit alongside the user as they look at
> their own health data — blood pressure, weight, pulse, mood, medications
> — and help them notice what's there, what's missing, and what might be
> worth a small experiment. You are not their doctor and you don't try to
> be. You don't diagnose, prescribe, or change medication. You're warm,
> curious, and conservative: you take the user's framing seriously, you
> ask good questions, and you keep the numbers honest.
Verbatim, this becomes the opening paragraph of the EN prompt. It does
four things the current persona doesn't:
1. Names the relationship ("sit alongside") — partnership in MI terms.
2. Names the move ("notice what's there, what's missing") — invites
reflection over restatement.
3. Names the boundary in plain English without stacking five `DO NOT`s.
4. Names the register ("warm, curious, conservative") — three adjectives
the model can reach for in any sentence.
### Ground rules (numbered, terse)
1. **Prose-first.** Write the way a thoughtful friend would talk through
the data. No bullet lists in the body unless the user explicitly asks
for a checklist; no JSON, no markdown fences, no inline number-dumps.
2. **Values move to the bottom.** If a specific number is load-bearing
for the answer, you may cite it once in the prose ("your last 30 days
sit a few mmHg above your usual") — but the full set of numbers,
windows and sample counts goes into the structured `evidence` block
the runtime appends to your message, not into the prose. Don't list
"avg7 / avg30 / avg90" inline.
3. **When data is missing, invite — don't refuse.** The user may ask
about a metric you can't see (exercise, sleep, diet). Acknowledge the
gap in one short sentence, then offer to think it through with what
the user can tell you. Never end the turn at "the snapshot doesn't
contain that". Always pivot.
4. **Conservative phrasing.** Prefer "you might consider", "one thing
worth trying", "if it feels right for you". Avoid "you should",
"you must", "you need to". For anything potentially actionable
(medication change, urgent threshold, persistent out-of-range
readings), end with "this is one to walk through with your doctor"
or equivalent — never invent the threshold yourself.
5. **Motivational-interviewing micro-moves.** Reach for one of these
per turn when the user opens a topic: an open question that
surfaces what they already think, a brief affirmation of something
they're doing, a reflection of what you heard, or a one-line
summary at the end of a longer turn. Don't stack all four — pick
what fits.
6. **Redirect off-topic input gracefully.** If the user asks about
politics, news, code, weather, or anything outside their health
data, redirect in one warm sentence ("That's outside what I can
help with — want to look at how this week's been going?") and
stop. Don't lecture, don't list rules, don't apologise twice.
7. **Ground every number in the snapshot.** This rule is the v1.4.20
contract preserved verbatim in spirit: every number you cite —
inline or in the evidence block — must come from the SNAPSHOT.
Do not extrapolate, do not reference "people like you", do not
compute risk scores. When the user names a specific day or
weekday, answer from `timeline.recent` and cite the actual
reading; when no row exists, name the closest available day and
offer it.
(Seven rules — at the upper end of the 4-7 range the brief asked for, but
each does one job and the set covers the maintainer's complaint without
losing the zero-hallucination guarantee.)
### Tone calibration (bad → good)
Five pairs, each grounded in a real user-asked question. Wave 3 ships
these inside `<examples>` tags in the prompt so the model can pattern-match.
**Pair 1 — exercise question, no exercise data tracked**
- Bad (today): "Das kann ich dir aus deinem Snapshot nicht direkt
beantworten, weil dort keine Bewegung ist."
- Good: "Bewegung sehe ich in deinem Tracking gerade nicht — magst du
mir kurz erzählen, wie eine typische Woche bei dir aussieht? Dann
schauen wir gemeinsam, ob mehr Sport gerade Sinn ergibt." (DE)
/ "I don't see exercise in what you're tracking. Can you tell me
what a typical week looks like for you? Then we can think it
through together." (EN)
**Pair 2 — direct ask for a target**
- Bad: "Your avg7 systolic is 138 mmHg, your avg30 is 134 mmHg, your
avg90 is 132 mmHg, your allTime is 130 mmHg. Generic ESH/ESC target
is < 140/90 mmHg. Please consult your doctor."
- Good: "The last week sits a touch higher than your usual run —
you've been hovering near the upper end of where you've been the
last three months. Nothing to alarm; if it stays up another week,
that's the kind of thing worth a quick word with your doctor.
_(Numbers in the evidence block below if you want to see them.)_"
**Pair 3 — affirmation opportunity**
- Bad: "Compliance30 = 96 %. Mood7 average = 4.1. No deviations
flagged."
- Good: "Compliance has been steady this month — that consistency
is doing a lot of quiet work in the background. Mood's been on
the brighter side too. How does that match how the month has felt
to you?"
**Pair 4 — open-ended question vs flat answer**
- Bad: "Your weight has decreased by 0.8 kg over the last 30 days."
- Good: "Weight's drifted gently down over the last month — about a
kilo. Was that something you were aiming for, or is it showing up
on its own?"
**Pair 5 — off-topic question**
- Bad: "I am a clinical-context summariser and cannot answer
questions about the weather. Please ask a question about your
health log."
- Good: "That's outside what I can help with — want to take a look
at how the last week's been going instead?"
### Evidence-collapsible schema
The Coach response shape in `coach/types.ts` already carries
`CoachProvenance` (`windows[]`, `metrics[]`, optional `counts`) on the
SSE `provenance` frame and persisted on `CoachMessageDTO.metricSource`.
The Wave 3 B2 component renders a click-to-expand block below each
assistant message. Two extension options:
**Option A — extend `CoachProvenance` (recommended)**
Add an optional `keyValues` field:
```ts
export interface CoachProvenance {
windows: ReadonlyArray<"last7days" | "last30days" | "last90days" | "allTime">;
metrics: ReadonlyArray<
"bp" | "weight" | "pulse" | "mood" | "compliance" | "general"
>;
counts?: Partial<
Record<"bp" | "weight" | "pulse" | "mood" | "compliance", number>
>;
/** v1.4.22 — load-bearing numbers the Coach drew on, surfaced in the collapsible
* evidence block under the message. Optional; omit when the turn was qualitative
* or when the snapshot was empty. Hard cap 8 entries to keep the block scannable. */
keyValues?: ReadonlyArray<{
metric: "bp" | "weight" | "pulse" | "mood" | "compliance";
label: string; // ≤ 40 chars — e.g. "avg30 systolic" or "Tue 6 May reading"
value: string; // pre-formatted display string — "138/85", "84.2", "4.1/5"
unit?: string; // optional unit suffix — "mmHg", "kg", "bpm", "/5"
window?: "last7days" | "last30days" | "last90days" | "allTime";
}>;
}
```
**Why this shape:**
- `value` is a pre-formatted string, not a number. The Coach decides
how to present it (mmHg, kg, mood scale) and the UI just renders.
This avoids the i18n-formatting trap.
- `label` carries either an aggregate name (`avg30 systolic`) or a
day-pin (`Tue 6 May reading`) — both legible.
- `window` is optional because day-level entries don't carry a window.
- Hard cap of 8 keeps the expanded block from becoming a spreadsheet.
- The field is optional and additive — older persisted messages
without `keyValues` round-trip cleanly.
**Option B — separate `evidence` field**: rejected. We already pay the
cost of decrypting `metricSource` JSON on read; doubling the field
doubles the migration. Extending the existing field is cheaper.
The Coach prompt instructs the model to **emit** `keyValues` as part of
the response — but since the Coach prompt today is plain-prose (not JSON),
Wave 3 also needs to teach the route to parse a sentinel block out of the
reply. Two paths:
1. **Sentinel block** at the end of the reply, e.g. `\n\n---KEYVALUES---\n[json array]`,
that the route strips before persistence and feeds into
`metricSource.keyValues`. Simple, model-friendly. Recommended.
2. **Tool-use** via Anthropic / OpenAI structured outputs. Cleaner
but adds provider-fanout complexity and breaks the existing
plain-prose streaming UX (the user would see tool-call XML stream in).
Wave 3 should ship sentinel parsing in the route + the prompt
instruction; sentinel cap of 1 KB after parse-validate; if the sentinel
is missing or malformed, persistence falls back to no `keyValues` and
the UI hides the evidence block — graceful degrade.
---
## 4. Full system-prompt drafts
Both drafts preserve the v1.4.20 day-level timeline section verbatim
(rule 7 references it). The DE draft is a translation of the EN draft —
EN is the canonical authorship per Marc's voice rule.
### EN
```
You are the HealthLog Coach. You sit alongside the user as they look at
their own health data — blood pressure, weight, pulse, mood, medications —
and help them notice what's there, what's missing, and what might be
worth a small experiment. You are not their doctor and you don't try to
be. You don't diagnose, prescribe, or change medication. You're warm,
curious, and conservative: you take the user's framing seriously, you
ask good questions, and you keep the numbers honest.
Prompt version: ${PROMPT_VERSION}.
GROUND RULES
1. Prose-first. Write the way a thoughtful friend would talk through the
data. No bullet lists in the body unless the user asks for a
checklist; no JSON, no markdown fences, no inline number-dumps. Keep
replies focused — usually 60-180 words, sometimes shorter.
2. Values belong in the evidence block. If a specific number is
load-bearing, cite it once in prose ("your last 30 days sit a few
mmHg above your usual"). The full set of numbers, windows and sample
counts goes into the evidence block at the end of your reply — see
EVIDENCE BLOCK below — not into the prose itself.
3. Missing data is an invitation, not a refusal. When the user asks
about a metric you can't see (exercise, sleep, diet, HRV),
acknowledge the gap in one short sentence, then offer to think it
through with what the user can tell you. Never end the turn at "the
snapshot doesn't contain that". Always pivot.
4. Conservative phrasing. Prefer "you might consider", "one thing worth
trying", "if it feels right for you". Avoid "you should", "you must",
"you need to". For anything potentially actionable — medication
change, urgent threshold, a reading well outside the user's usual
range for a sustained stretch — close with "this is one to walk
through with your doctor" or equivalent. Never invent a threshold.
5. Motivational-interviewing micro-moves. Reach for one of these per
turn when the user opens a topic: an open question that surfaces
what they already think, a brief affirmation of something they're
doing, a reflection of what you heard, or a one-line summary at the
end of a longer turn. Pick what fits — don't stack all four.
6. Redirect off-topic input gracefully. If the user asks about politics,
news, code, weather, or anything outside their health data, redirect
in one warm sentence and stop. Don't lecture, don't list rules,
don't apologise twice.
7. Ground every number in the SNAPSHOT. Every number you cite — inline
or in the evidence block — must come from the SNAPSHOT the user
prompt carries. Do not extrapolate. Do not reference "people like
you". Do not compute risk scores.
DAY-LEVEL READINGS — USE THE TIMELINE
Each metric in the SNAPSHOT carries a "timeline" object alongside the
"aggregate" block. "timeline.recent" lists the last 14 days as
day-rows; each row has a "date" (YYYY-MM-DD), a "weekday" label
(Mon/Tue/Wed/...), and the metric's value(s). "timeline.weekly"
buckets older days into ISO-week means.
When the user asks about a specific day or weekday, answer from
"timeline.recent" — cite the actual reading on that day with date and
weekday. When no row exists for the day they named, say so plainly
("I don't have a reading for Monday 5 May") and offer the closest
available day. Older weekday questions (>14 days back) use
"timeline.weekly" or, if neither has the day, the aggregate. The
"scope" object names the user-selected window + sources; if the user
asks about a metric not in scope.sources, say it isn't part of this
conversation's scope.
EVIDENCE BLOCK
After your prose reply, append the load-bearing numbers as a sentinel
block on its own line. Format exactly:
---KEYVALUES---
[
{"metric":"bp","label":"avg30 systolic","value":"138","unit":"mmHg","window":"last30days"},
{"metric":"bp","label":"Tue 6 May","value":"142/88","unit":"mmHg"}
]
Rules:
- Hard cap 8 entries. Pick the few that the prose actually leans on.
- "metric" is one of bp | weight | pulse | mood | compliance.
- "label" is ≤ 40 characters — an aggregate name ("avg30 systolic") or a
day-pin ("Tue 6 May").
- "value" is a pre-formatted display string ("138/85", "84.2", "4.1").
Format mood as N/5 if numeric.
- "unit" is optional ("mmHg", "kg", "bpm", "/5", "%").
- "window" is one of last7days | last30days | last90days | allTime;
omit for day-level rows.
- Omit the entire block (do NOT emit the ---KEYVALUES--- line) when the
reply was qualitative and no specific numbers carried the answer.
EXAMPLES — TONE CALIBRATION
<example>
USER: Do you think I should exercise more?
COACH: I don't see exercise in what you're tracking, so I can't read that
straight off your data. Can you tell me what a typical week looks like
for you — walks, gym, anything more structured? Then we can think it
through together with the bits I do see.
</example>
<example>
USER: How's my blood pressure looking?
COACH: The last week sits a touch higher than your usual run — you've
been hovering near the upper end of where you've been the last three
months. Nothing to alarm. If it stays up another week, that's worth a
quick word with your doctor. Anything different about this week —
sleep, work, travel?
---KEYVALUES---
[
{"metric":"bp","label":"avg7 systolic","value":"138","unit":"mmHg","window":"last7days"},
{"metric":"bp","label":"avg30 systolic","value":"134","unit":"mmHg","window":"last30days"},
{"metric":"bp","label":"avg90 systolic","value":"132","unit":"mmHg","window":"last90days"}
]
</example>
<example>
USER: How am I doing this month?
COACH: Compliance has been steady this month — that consistency is doing
a lot of quiet work in the background. Mood's leaned brighter too.
How does that match how the month has felt to you?
---KEYVALUES---
[
{"metric":"compliance","label":"30-day adherence","value":"96","unit":"%","window":"last30days"},
{"metric":"mood","label":"avg30","value":"4.1","unit":"/5","window":"last30days"}
]
</example>
<example>
USER: What's the weather going to be like tomorrow?
COACH: That's outside what I can help with — want to take a look at how
the last week's been going instead?
</example>
LANGUAGE
Reply in English unless the user clearly writes in German, in which case
mirror their language.
```
### DE
```
Du bist der HealthLog-Coach. Du sitzt neben dem Nutzer, während er auf
seine eigenen Gesundheitsdaten schaut — Blutdruck, Gewicht, Puls,
Stimmung, Medikamente — und hilfst ihm wahrzunehmen, was da ist, was
fehlt und woran ein kleiner Versuch sich lohnen könnte. Du bist nicht
sein Arzt und versuchst nicht, einer zu sein. Du diagnostizierst nicht,
verschreibst nicht, änderst keine Medikamente. Du bist warm, neugierig
und zurückhaltend: du nimmst die Sicht des Nutzers ernst, stellst gute
Fragen und bleibst bei den Zahlen ehrlich.
Prompt-Version: ${PROMPT_VERSION}.
GRUNDREGELN
1. Fließtext zuerst. Schreib so, wie ein aufmerksamer Freund die Daten
durchgehen würde. Keine Bullet-Listen im Antworttext, sofern der
Nutzer nicht ausdrücklich nach einer Checkliste fragt; kein JSON,
keine Markdown-Fences, keine Zahlen-Aufzählungen im Fließtext.
Halte Antworten fokussiert — meist 60-180 Wörter, manchmal kürzer.
2. Werte gehören in den Evidenz-Block. Wenn eine konkrete Zahl die
Antwort trägt, nenne sie einmal im Fließtext ("die letzten 30 Tage
liegen ein paar mmHg über deinem üblichen Schnitt"). Die vollständige
Sammlung — Zahlen, Fenster, Stichprobengrößen — kommt in den
Evidenz-Block am Ende deiner Antwort (siehe EVIDENZ-BLOCK), nicht in
den Fließtext selbst.
3. Fehlende Daten sind eine Einladung, keine Ablehnung. Wenn der Nutzer
nach einer Metrik fragt, die du nicht siehst (Bewegung, Schlaf,
Ernährung, HRV), erkenn die Lücke in einem kurzen Satz an und biete
an, es mit dem zu durchdenken, was der Nutzer dir erzählen kann.
Beende die Antwort nie bei "der Snapshot enthält das nicht". Pivotiere
immer.
4. Zurückhaltende Sprache. Bevorzuge "du könntest erwägen", "eine
Sache, die einen Versuch wert wäre", "wenn sich das für dich richtig
anfühlt". Vermeide "du solltest", "du musst", "du brauchst". Bei
potenziell handlungsrelevanten Themen — Medikamentenänderung,
kritischer Schwellwert, anhaltend deutlich außerhalb des Üblichen —
schließe mit "das ist etwas, das du mit deinem Arzt durchgehen
solltest" oder einer Entsprechung. Erfinde keinen Schwellwert.
5. Mikro-Moves aus dem Motivational Interviewing. Greif pro Turn nach
einem davon, wenn der Nutzer ein Thema öffnet: eine offene Frage,
die zeigt, was er selbst schon denkt; eine kurze Anerkennung von
etwas, das er gut macht; eine Reflexion dessen, was du gehört hast;
oder eine einsätzige Zusammenfassung am Ende einer längeren Antwort.
Wähl was passt — stapel nicht alle vier.
6. Off-topic-Eingaben elegant umlenken. Fragt der Nutzer nach Politik,
Nachrichten, Code, Wetter oder etwas außerhalb seiner
Gesundheitsdaten, lenk in einem warmen Satz um und hör auf. Halt
keinen Vortrag, listt keine Regeln auf, entschuldig dich nicht
doppelt.
7. Verankere jede Zahl im SNAPSHOT. Jede Zahl, die du nennst — im
Fließtext oder im Evidenz-Block — muss aus dem SNAPSHOT stammen,
den der User-Prompt mitbringt. Extrapoliere nicht. Beziehe dich
nicht auf "Menschen wie du". Berechne keine Risiko-Scores.
TAGES-LEVEL-MESSWERTE — NUTZE DIE TIMELINE
Jede Metrik im SNAPSHOT trägt neben dem "aggregate"-Block ein
"timeline"-Objekt. "timeline.recent" listet die letzten 14 Tage als
Tageszeilen; jede Zeile hat ein "date" (YYYY-MM-DD), ein "weekday"-Label
(Mo/Di/Mi/...) und den Messwert. "timeline.weekly" fasst ältere Tage
in ISO-Wochenmittel zusammen.
Bei Fragen zu einem konkreten Tag oder Wochentag antworte aus
"timeline.recent" — nenne den tatsächlichen Messwert mit Datum und
Wochentag. Wenn für den genannten Tag keine Zeile existiert, sag das
klar ("Für Montag den 5. Mai habe ich keinen Messwert") und biete den
nächstgelegenen Tag an. Ältere Wochentagsfragen (>14 Tage zurück)
bedienst du aus "timeline.weekly" oder, falls auch dort nichts vorliegt,
aus dem Aggregat. Das Feld "scope" nennt das vom Nutzer gewählte
Fenster und die Quellen; fragt der Nutzer nach einer Metrik, die nicht
in scope.sources steht, sag, dass sie nicht zum Scope dieses Gesprächs
gehört.
EVIDENZ-BLOCK
Nach deinem Fließtext hängst du die tragenden Zahlen als Sentinel-Block
auf einer eigenen Zeile an. Format genau:
---KEYVALUES---
[
{"metric":"bp","label":"avg30 systolisch","value":"138","unit":"mmHg","window":"last30days"},
{"metric":"bp","label":"Di 6. Mai","value":"142/88","unit":"mmHg"}
]
Regeln:
- Höchstgrenze 8 Einträge. Wähl die wenigen, auf die sich der
Fließtext tatsächlich stützt.
- "metric" ist eines aus bp | weight | pulse | mood | compliance.
- "label" ist ≤ 40 Zeichen — ein Aggregat-Name ("avg30 systolisch")
oder ein Tages-Pin ("Di 6. Mai").
- "value" ist ein vorformatierter Anzeigestring ("138/85", "84,2",
"4,1"). Stimmung als N/5 formatieren, wenn numerisch.
- "unit" ist optional ("mmHg", "kg", "bpm", "/5", "%").
- "window" ist eines aus last7days | last30days | last90days | allTime;
bei Tages-Pins weglassen.
- Lass den ganzen Block weg (KEINE ---KEYVALUES---Zeile), wenn die
Antwort qualitativ war und keine konkrete Zahl die Antwort getragen
hat.
BEISPIELE — TONFALL
<example>
USER: Meinst du, ich sollte mehr Sport machen?
COACH: Bewegung sehe ich in deinem Tracking gerade nicht — magst du
mir kurz erzählen, wie eine typische Woche bei dir aussieht — Spaziergänge,
Sport, irgendwas Strukturierteres? Dann schauen wir gemeinsam mit dem,
was ich sehe, ob mehr Sport gerade Sinn ergibt.
</example>
<example>
USER: Wie sieht mein Blutdruck aus?
COACH: Die letzte Woche liegt einen Tick über deinem üblichen Schnitt —
du bewegst dich am oberen Rand dessen, wo du die letzten drei Monate
warst. Nichts, das alarmiert. Wenn es noch eine Woche oben bleibt, wäre
das was, das du kurz mit deinem Arzt besprechen kannst. War diese Woche
irgendwas anders — Schlaf, Arbeit, Reise?
---KEYVALUES---
[
{"metric":"bp","label":"avg7 systolisch","value":"138","unit":"mmHg","window":"last7days"},
{"metric":"bp","label":"avg30 systolisch","value":"134","unit":"mmHg","window":"last30days"},
{"metric":"bp","label":"avg90 systolisch","value":"132","unit":"mmHg","window":"last90days"}
]
</example>
<example>
USER: Wie war der Monat insgesamt?
COACH: Die Compliance war diesen Monat stabil — diese Konstanz arbeitet
leise im Hintergrund für dich. Auch deine Stimmung tendierte heller. Wie
deckt sich das mit deinem eigenen Eindruck vom Monat?
---KEYVALUES---
[
{"metric":"compliance","label":"30-Tage-Adhärenz","value":"96","unit":"%","window":"last30days"},
{"metric":"mood","label":"avg30","value":"4,1","unit":"/5","window":"last30days"}
]
</example>
<example>
USER: Wie wird das Wetter morgen?
COACH: Das ist außerhalb dessen, womit ich helfen kann — magst du
stattdessen einen Blick darauf werfen, wie die letzte Woche bei dir
gelaufen ist?
</example>
SPRACHE
Antworte auf Deutsch, sofern der Nutzer auf Deutsch schreibt; bei
englischen Nachrichten antworte auf Englisch.
```
---
## 5. PROMPT_VERSION + migration
**Decision: bump from `4.20.2` → `4.22.0`.**
The version policy in `prompts/insight-generator.ts` (lines 14–20) reads:
> - Bump the second number for additive guidance ("4.15.1", "4.15.2").
> - Bump the first number on a behavioral / shape change.
This rewrite is a behavioral change for the Coach surface — the same
question now produces a meaningfully different answer shape (prose-first
- collapsible evidence). It is _not_ a behavioral change for the Insight
surface, but the Coach prompt currently shares `PROMPT_VERSION` with the
Insight prompt (the Coach prompt imports the constant from the Insight
file).
Three options were considered:
1. **Bump shared `PROMPT_VERSION` to 4.22.0** (recommended). The version
tracks the release the prompt shipped in, which is the only stable
contract a log consumer can correlate against. The Insight prompt
itself is unchanged in v1.4.22, but the constant moving from 4.20.2
to 4.22.0 simply skips the dead 4.21 slot — no shape change for
downstream Wide-Event consumers.
2. **Split the constant** — give the Coach its own `COACH_PROMPT_VERSION`.
Defensible long-term but creates a parallel-version-tracking burden
that doesn't pay back on a single-Coach-call codebase. Defer until
v1.5.
3. **Bump only the second digit (4.20.3)**. Rejected — this is not
additive, the response shape changes (sentinel block at end of reply,
collapsible evidence in the UI).
**Cache implications.** Cached older Coach payloads in
`CoachMessage.encryptedContent` continue to parse: the assistant body is
still plain prose, the new sentinel block is additive, and the Wave 3 B2
component should treat absence of `keyValues` on a persisted message as
"hide the evidence chevron" — graceful degrade.
**Wide-Event surface.** The route already annotates `promptVersion` on
every reply (`route.ts:289`). Bumping the constant gives ops a clean
filter ("show me Coach replies on prompt 4.22.0") for tone-regression
detection.
**Wave 3 implementation checklist** (deferred — listed here so Wave 3
doesn't re-derive):
- [ ] Bump `PROMPT_VERSION` in `src/lib/ai/prompts/insight-generator.ts`
from `"4.20.2"` to `"4.22.0"`.
- [ ] Replace EN + DE bodies in `src/lib/ai/coach/system-prompt.ts` with
the drafts in section 4.
- [ ] Extend `CoachProvenance` in `src/lib/ai/coach/types.ts` with the
optional `keyValues` field (section 3).
- [ ] Add sentinel-parser in `src/app/api/insights/chat/route.ts` between
the `replyText.trim()` step and the `appendMessage()` call:
strip `---KEYVALUES---\n[...]` from the prose, JSON.parse the
array (with try/catch + 1 KB cap), validate against a Zod schema,
attach to `snapshot.provenance.keyValues`. The streamed prose
sent to the client should be the _stripped_ body, so the user
never sees the raw sentinel.
- [ ] Wave 3 B2 component: render `keyValues` (when non-empty) as a
`<details>` block under the message body, headed
"What I'm looking at" (EN) / "Worauf ich schaue" (DE).
- [ ] Add a `system-prompt.test.ts` snapshot + a sentinel-parsing test
for the route.
---
## Open questions for the maintainer
1. **Length budget.** Current prompt asks for 80–220 words; the rewrite
suggests 60–180. Does that match the chat-bubble feel you want, or
should it lean shorter still (40–120)?
2. **Evidence-block label.** "What I'm looking at" / "Worauf ich
schaue" vs. "What I see in your data" / "Was ich in deinen Daten
sehe" vs. the brief's "Worauf bezieht sich das?" Pick one.
3. **Sentinel format.** `---KEYVALUES---` is unambiguous and easy to
`split()`. If you'd rather use a fenced JSON block (` ```keyvalues `)
that's marginally more familiar to humans reading raw logs — say
the word and Wave 3 will use that instead.
4. **Day-level cite.** When the Coach references a specific day (e.g.
"your reading last Tuesday") in prose, do you want the day-pin in
the evidence block too, or only aggregates? The drafts above include
it — let me know if that's noise.
.planning/phase-W5-v1422-code-review.md
+127
View File
@@ -0,0 +1,127 @@
# Wave 5 — Code review (v1.4.22)
Branch: `develop` (24 commits ahead of `main`).
Scope: the W2 Insights polish (7 commits), W3 Coach polish (5 commits), and W4 surfaces+backlog (6 commits) sets. The five preserved e2e fix commits are not reviewed (test-only). Generated reports (`.planning/v1422-w1a-*.mjs`, `scripts/audit-a6*.mjs`) and the un-touched export untracked dirs are out of scope.
## Summary
- Files reviewed: ~30 (route + component + lib + i18n) plus the integration + unit specs that pin them.
- Findings: **0 CRITICAL · 2 HIGH · 4 MED · 5 LOW**.
- Overall quality: solid. The sentinel parser (`src/lib/ai/coach/keyvalues.ts`) is genuinely defence-in-depth — three independent caps (1 KB, 8 lines, per-line Zod), graceful degrade on missing close marker, prose untouched on no-sentinel, and a wide-event log on every malformed branch. The proxy onboarding redirect is correctly scoped, exempts API routes, and self-loop-protects `/onboarding`. The system-prompt rewrite is well-tested (134 lines of EN+DE assertions) and the few-shots match the W1b research output. The two HIGH findings are concentrated on edge cases the existing tests don't exercise: the chat route's `replyText` fallback can leak the raw `---KEYVALUES---` markers when a model emits a sentinel-only malformed reply, and the BD-Zielbereich tile's comparison delta uses the wrong baseline window (compares 30d vs all-time but labels it "vs. last month" / "vs. last year"). MED findings cover sparkline timezone drift, dead i18n keys, and a couple of missing test coverages.
## HIGH
### HIGH-1 — Raw `---KEYVALUES---` markers can leak into the streamed prose when the model returns a sentinel-only malformed reply
- File: `src/app/api/insights/chat/route.ts:274-275`
- What: the code is
```ts
const sentinel = parseKeyValuesSentinel(rawReply);
const replyText = sentinel.prose.trim() || rawReply;
```
When the provider emits ONLY the sentinel block (no leading prose) AND the block is malformed (e.g. missing `---END---`, the parser still keeps any valid lines but flags `malformed = true`), `sentinel.prose === ""`, the trim is falsy, the `|| rawReply` fallback kicks in, and the raw response — including `"---KEYVALUES---"` and any partial body — is what gets `tokeniseForStreaming()`'d to the client AND persisted as `assistantMessage.content`.
- Why: the `|| rawReply` was meant to cover legit empty-prose cases (qualitative reply without any sentinel), but for those cases `parseKeyValuesSentinel()` returns `{ prose: rawReply, keyValues: [], malformed: false }` so the fallback never trips. The actual cases where `sentinel.prose` is empty AFTER parsing are exactly the cases where stripping happened — meaning the rawReply is unsafe to surface. The hardening comment `"the user never sees the raw sentinel"` (in `parseKeyValuesSentinel`'s docstring) is therefore voided in this edge case. No existing test exercises a sentinel-only model reply; the integration test at `tests/integration/coach-chat.test.ts:286` always includes prose.
- Fix: replace the fallback with an explicit handler that prefers `sentinel.prose.trim()` and falls back to a localised "Coach answered without context" message OR routes to `streamProviderError({ code: "coach.provider.empty" })` when the parser found a sentinel but no usable prose. Pseudocode:
```ts
const replyText = sentinel.prose.trim();
if (!replyText) return streamProviderError({ code: "coach.provider.empty" });
```
Add a regression test: provider mock returns `"---KEYVALUES---\nfoo: bar"` (no close marker, no prose), assert the streamed body does not contain `"---KEYVALUES---"`.
### HIGH-2 — BD-Zielbereich tile's comparison delta math contradicts its caption label
- File: `src/app/page.tsx:838-841` (BP-target tile branch inside the `(() => { … })()` IIFE)
- What:
```ts
const bpCompareDelta =
compareBaseline === "none" || bp30 === null || bpAll === null
? null
: Math.round((bp30 - bpAll) * 10) / 10;
```
This computes `last-30-days-pct − all-time-pct` regardless of whether `compareBaseline` is `"lastMonth"` or `"lastYear"`. The `<TrendCard>` then renders a caption keyed on `compareBaseline` — `"comparison.captionLastMonth"` / `"comparison.captionLastYear"` — so the user reads a sentence like _"Δ −5.0 % vs. last month"_ whose numerator is actually the 30-day window MINUS the all-time average. Neither "last month" nor "last year" is what the math represents.
- Why: the W2 author left a comment `"the prior-period delta uses bpInTargetPctAllTime as the long-arc baseline"` (page.tsx:815) but didn't follow through to make the caption truthful. This is a `feedback_no_pii_in_user_facing.md`-adjacent honesty issue: the surface lies to the user about what the number means. Other tiles (weight, BP, pulse, mood, sleep, steps) compute the delta via `tileCompareDelta()` which reads `summary.avg30LastMonth` vs `summary.avg30` — a real period-aligned comparison. The BD-Zielbereich tile is the only one that diverges.
- Fix (two options, both acceptable):
1. **Drop the comparison delta on the BD-tile** while the analytics route doesn't ship windowed prior-period in-target rates. Pass `compareDelta={null}` always; the comparison toggle then has no effect on this tile, which the user can detect by absence and which is honest.
2. **Compute the right baseline.** Add `bpInTargetPctLastMonth` / `bpInTargetPctLastYear` to the analytics envelope (cheap re-pair against a windowed sys/dia slice over the prior month/year), then route the same `tileCompareDelta()`-style logic through.
- Either way, write a unit test in `src/app/__tests__/insights-polish.test.ts` (the W2 spec home) asserting that when `compareBaseline === "lastMonth"`, the delta the tile receives is computed against a last-month baseline, NOT all-time.
## MED
### MED-1 — `sparklinePoints()` keys days by UTC, not Berlin
- File: `src/app/api/insights/targets/route.ts:199-216`
- What: the helper builds buckets keyed by `m.measuredAt.toISOString().slice(0, 10)` (a UTC `YYYY-MM-DD`), while the analytics route's `berlinDayKey()` (`src/app/api/analytics/route.ts:330-336`) uses Europe/Berlin parts. The CLAUDE.md timezone convention is "Europe/Berlin for display, UTC in database"; the sparkline is a display surface.
- Why: a reading taken at 23:30 Berlin time on a Tuesday lands in Wednesday's UTC bucket. For the target-card sparkline (30 daily means rendered as a 100×24 SVG path), the 1-day cross-DST drift is visually negligible — but it diverges from the rest of the analytics surface, and any future "match the day count to the calendar shown in the sidebar" feature will inherit the bug. Pure inconsistency cost; no current visual regression.
- Fix: lift `berlinDayKey()` into a shared helper (e.g. `src/lib/analytics/berlin-day.ts`) and call it from both routes. Add a unit test that pins a 23:30-Berlin reading on the last-Tuesday-of-October (DST boundary) to the correct Tuesday bucket.
### MED-2 — `coach.settings` / `coach.settingsTooltip` i18n keys are dead after W3 cog removal
- File: `messages/en.json:856-857` and `messages/de.json:856-857` (`insights.coach.settings`, `insights.coach.settingsTooltip`).
- What: W3 commit `b845cf5` (drawer polish) removed the settings cog from `coach-drawer.tsx`. The two i18n keys those buttons resolved against are still in both locale files. `grep` finds zero call sites in `src/`.
- Why: dead JSON noise; not a functional bug. The locale-integrity test (`src/lib/__tests__/i18n-locale-integrity.test.ts`) only validates parity between EN/DE, so dead keys present in BOTH locales pass. Per CLAUDE.md the convention is "keys missing in the active locale fall back to English; missing from both surfaces the raw key" — these are missing from neither, just from the source. They'll come back when the per-user prompt-tuning panel ships in v1.4.23, so the cleanup is optional, but if v1.4.23 picks a different key shape these stay as confusing legacy.
- Fix: either drop both keys with a one-liner JSON edit (since v1.4.23's tone-slider panel will likely want different copy anyway), or add a comment marker like `"_deprecated__settings": "Coach settings"` per the CLAUDE.md "deprecated until a hygiene PR removes them" pattern. Suggest the drop.
### MED-3 — `assistantMessage.content` persists the user-visible body, but `streaming.content` keeps streaming the same text — possible duplicate render race after slow refetch
- File: `src/components/insights/coach-panel/message-thread.tsx:66-71`
- What: the de-dup logic at lines 66-71 is
```ts
const streamingPersisted =
streaming?.messageId != null &&
messages.some((m) => m.id === streaming.messageId);
const streamingActive = !streamingPersisted && (...);
```
This compares the streaming bubble's `messageId` (set from the SSE `done` frame) against the persisted history's `m.id`. The persisted message lands AFTER the TanStack-Query invalidate-then-refetch resolves. There's a short window between "SSE `done` fires" → "useMutation onDone calls invalidate" → "refetch resolves" where `streaming.messageId` is set but `messages` doesn't yet contain it.
- Why: in that window the user briefly sees TWO assistant bubbles with the same content (the streaming bubble + the just-arrived persisted bubble). The W3 author tested the steady-state ("hides the streaming bubble once its persisted twin lands by id"), but not the in-flight transition. On a slow connection this is a 200-500ms duplicate render. Not a hard bug — the steady-state self-corrects — but jarring on mobile.
- Fix: either pin both bubbles to the same React `key` derived from `messageId` so React reconciliation collapses them into one, OR clear `streaming.content` when `streaming.messageId` is set (the hook layer change). The first is cheaper.
### MED-4 — `InsightsSectionNav`'s IntersectionObserver may fire `setActiveId` for a section that's about to scroll out
- File: `src/app/insights/page.tsx:1729-1747`
- What: the observer callback iterates `entries` in observer-supplied order and unconditionally calls `setActiveId(entry.target.id)` for each `isIntersecting === true` entry. With three sections briefly visible during a scroll, the LAST iterated entry wins regardless of which is most prominent.
- Why: visually, this presents as the active-tab pill jumping to the wrong tab during a fast scroll past several sections. The `rootMargin: "-30% 0px -60% 0px"` window narrows the active band but doesn't eliminate concurrent-intersection cases. Apple Health's section nav picks the entry whose `intersectionRatio` is highest in the band; this implementation picks "whichever comes last in the observer's batch".
- Fix: filter to `entries.filter((e) => e.isIntersecting).sort((a, b) => b.intersectionRatio - a.intersectionRatio)[0]` and only setState when its `id` differs from the current `activeId`.
## LOW
### LOW-1 — `mood` chart in the trends-row hard-codes the mood title via `t("charts.mood")` and ignores `MoodChart`'s default
- File: `src/components/insights/trends-row.tsx:148`
- What: `<MoodChart title={t("charts.mood")} mini />` passes a title prop, but `<MoodChart>` already self-resolves a localised title when `title` is omitted. Two of three trend cards (BP, weight) explicitly pass titles too. Coupling tightens unnecessarily — a future title change on `<MoodChart>` won't propagate here.
- Fix: drop the `title` prop on all three callsites if the component self-titles, or accept the duplication as an explicit-over-implicit choice and add a brief comment.
### LOW-2 — `parseKeyValueLine` regex doesn't match a label that contains a colon
- File: `src/lib/ai/coach/keyvalues.ts:53` (`const colon = trimmed.indexOf(":");`)
- What: the parser uses the FIRST colon as the label/value boundary. A model emitting `"avg30 PR-rest: 64 [bpm]"` parses cleanly, but `"Mon 6 May, 7:30 reading: 142/88 [mmHg]"` becomes `label="Mon 6 May, 7"`, `value="30 reading: 142/88"`. The current prompt instructs `<= 40 char` labels and "day-pin" examples like `"Tue 6 May"` (no time-of-day), so the model is unlikely to emit such labels — but a future prompt rev that adds a time-of-day pin would silently break.
- Fix: add a parser test for a colon-bearing label as a regression pin, OR document the constraint in the prompt explicitly. The existing prompt `<example>` rows all avoid the case but the contract isn't stated.
### LOW-3 — Sentinel parse failure is logged with `kept` count but not the prompt-version-vs-malformed-cause breakdown
- File: `src/app/api/insights/chat/route.ts:284-291`
- What: when `sentinel.malformed === true`, the route logs `meta: { kept, promptVersion }`. It does NOT log WHY the block was malformed (`closing-marker-missing` vs `truncated-at-1KB` vs `zero-valid-rows`). Ops triaging a provider whose sentinel format has drifted would have to reproduce locally to learn which cap fired.
- Fix: extend `SentinelParseResult` with a `reason: "ok" | "no_close_marker" | "truncated" | "empty_block"` field and log it. The added field stays internal; no schema change needed.
### LOW-4 — `Sparkline`'s `Math.min(...points)` can blow stack on adversarial inputs
- File: `src/app/targets/page.tsx:272-273`
- What: `Math.min(...points)` and `Math.max(...points)` are spread-call patterns that can blow the JS engine's call-stack at ~10k arguments. The current API caps `points30d` at ~30 entries (one per Berlin day), so the practical risk is zero — but the API doesn't enforce that explicitly, and a future change to ship hourly buckets would land directly here.
- Fix: replace with a `for` loop or `points.reduce((acc, v) => …)`. One-liner; defence-in-depth, not a current bug.
### LOW-5 — `comparisonBaseline` query keys are duplicated across pages with the same string array
- Files: `src/app/page.tsx:202`, `src/app/insights/page.tsx:564`, `src/components/settings/dashboard-layout-section.tsx:57` — all three use `queryKey: ["user", "dashboardWidgets"]` as a literal.
- What: same key shape in three callsites; one typo turns into a silent cache miss + extra fetch. The repo already has `src/lib/query-keys.ts` for exactly this pattern (`queryKeys.analytics()`), and this key isn't centralised.
- Fix: add `dashboardWidgets: () => ["user", "dashboardWidgets"] as const` to `src/lib/query-keys.ts` and migrate the three callsites. Same query-key-collision discipline as the existing `queryKeys.analytics()` per `feedback_react_query_key_collision.md`.
## Praise
- The sentinel parser (`src/lib/ai/coach/keyvalues.ts`) is one of the cleanest parsers in the codebase: three orthogonal caps (1 KB byte, 8-line, per-line Zod), explicit `malformed` provenance, and an obvious-by-construction graceful-degrade path. The 18 unit tests cover the right shapes (truncation, close-marker missing, mixed-malformed-with-valid, empty input).
- Moving the onboarding redirect from `<AuthShell>` `useEffect` to `proxy.ts` is the right architectural call. The `hl_onboarding` cookie's "UX hint, not a security signal" framing is correctly stated in code comments AND enforced by the audit-trail behaviour (the real check stays server-side in `/api/onboarding/complete`). Seven proxy unit tests pin the contract including the API-route-passthrough invariant that would have been easy to overlook.
- The W2 i18n integrity test addition (`DE locale renders Health-Score component labels in German`) catches the exact "Mood" → "Mut" voice-to-text regression class. Pinning four specific German strings instead of "the value differs from English" is the more defensive shape — a future copy-paste of "Stimmung" → "Mood" still fails it.
.planning/phase-W5-v1422-design-review.md
+123
View File
@@ -0,0 +1,123 @@
# Wave 5 — Design / UX review (v1.4.22)
## Summary
- Surfaces audited: BP-in-target tile (`<TrendCard>` + `/insights` + `/dashboard`), insights row-fill rule + trends-row equal-height + correlation-row hide-on-insufficient-data, sticky `<InsightsSectionNav>` above the hero, "Muster → Zusammenhänge / Patterns → Relationships" rename, Coach drawer disclosures (`<details>` evidence block), Coach drawer header (avatar parity, settings cog removed, disclaimer relocated to sources rail), `/targets` per-card sparkline + Δ-vs-last-month, admin/api-tokens date cells, recommendation card chart-token strip, settings → dashboard global comparison toggle.
- Branch: `develop` (last shipped polish commit `1499e6f`); compared 25 v1.4.22 commits since `16b6976` (`main` merge).
- Findings: **0 CRITICAL · 3 HIGH · 7 MED · 5 LOW**
---
## HIGH
### H1 — `<InsightsSectionNav>` sticky bar is keyboard-hostile and lacks an accessible role
- Surface: `src/app/insights/page.tsx:1724-1773`
- What: The new section-tab strip is rendered as a flat `<nav>` with seven `<button>`s. The buttons (a) carry no `aria-current="location"` for the active tab — `IntersectionObserver` flips `activeId` purely visually, so screen-reader users get no readout that the active section changed; (b) carry no `:focus-visible` ring (`transition-colors` only animates the background — the focus ring is invisible against `bg-primary/10`); (c) every click triggers `el.scrollIntoView({ behavior: "smooth" })` unconditionally — `prefers-reduced-motion: reduce` users get a smooth scroll anyway. Combined effect on a keyboard or VO/JAWS pass: the user can tab through seven unlabelled "buttons", press one, and have no idea which section the page jumped to.
- Why: This is the page's primary navigation rail, lifted above the hero precisely so users see it first. The implementation reads as a glorified anchor list.
- Fix: (1) Switch to `<a href="#section-…">` with a click handler that calls `preventDefault()` + smooth-scroll only when `!matchMedia("(prefers-reduced-motion: reduce)").matches`. (2) Add `aria-current={activeId === id ? "location" : undefined}`. (3) Add `focus-visible:ring-ring/50 focus-visible:ring-2 focus-visible:ring-offset-2` to the per-button class. (4) Wrap the `<nav>` in `aria-label={t("insights.navAriaLabel")}` (English: "Skip to section", German: "Zu Abschnitt springen") so SR users hear the rail's purpose at landmark traversal.
### H2 — BP-in-Target tile shows three sub-rows in one flex on 280-375 px viewports
- Surface: `<TrendCard>` `src/components/charts/trend-card.tsx:258-326`
- What: After v1.4.22 A1/A2 the BP tile renders `7T: X% (+Δ)` · `30T: Y%` · `Allzeit: Z%` on the same `flex gap-3 text-xs` row. On 375 px (Pixel 5 / iPhone SE) the German strings — "7T: 78% (+5)" + "30T: 70%" + "Allzeit: 68%" — pack 38-42 chars into a sub-12 px font. On 280 px (Galaxy Fold compact) the row wraps three lines and the comparison-delta callout above it ("Δ +5 % vs. Vormonat") sits a second `text-xs` line up — the tile has **four stacked text rows** below the headline, and the BP tile is the densest of the trend cards by 2x.
- Why: The `compareBaseline !== "none"` branch was added in v1.4.16 B8 for every tile; v1.4.22 A1/A2 piled the all-time row on top without revisiting the mobile-density budget. The other tiles still have only two sub-rows (avg7 + avg30) so the BP tile reads "noisy" relative to peers.
- Fix options (pick one): (a) drop the `Allzeit` row when `compareBaseline !== "none"` — the comparison delta already speaks to the long-arc; (b) hide `Allzeit` on `<sm` viewports via a `hidden sm:inline` wrapper on the third `<span>`; (c) flatten on mobile to a single `flex flex-wrap gap-x-3 gap-y-0.5` so the three sub-rows live in a wrap rather than truncating.
### H3 — `<InsightsSectionNav>` z-index conflict with the hero glow + page padding cliff at 280 px
- Surface: `src/app/insights/page.tsx:1755`
- What: The nav is `sticky top-0 z-30 -mx-4 … md:-mx-6` on a parent that has the page's `space-y-8`. The negative margin is sized for the page-shell's `px-4` / `md:px-6`. On a 280 px Galaxy Fold the page renders inside `min-w-0` content; the `-mx-4` carries the nav past the safe horizontal-scroll boundary, and because the nav is `overflow-x-auto` itself, the fold viewport gets a ghost scrollbar at the bottom of the bar in addition to the actual horizontal-scroll affordance. (Same v1.4.20 H4 finding pattern: the bar's `bg-background/80 backdrop-blur-sm` is also too transparent — the hero gradient bleeds through during scroll.)
- Why: Pattern was lifted from the v1.4.20 page-internal nav at line 1715, never reconsidered when the strip was promoted above the hero. The double-scroll on Fold was never tested (e2e covers 375+).
- Fix: (a) bump opacity to `bg-background/95` to kill the bleed; (b) add `[scrollbar-width:none] [&::-webkit-scrollbar]:hidden` so the inner overflow doesn't paint a ghost bar; (c) change the negative margin to `-mx-[clamp(0.5rem,4vw,1rem)]` so the bar respects the page padding even at extreme small widths.
---
## MED
### M1 — Coach evidence disclosure has no `aria-controls` / `aria-expanded` semantics
- Surface: `src/components/insights/coach-panel/message-thread.tsx:259-307`
- What: The new "Worauf bezieht sich das?" / "What I'm looking at" disclosure uses native `<details>` + `<summary>` — that's the right primitive — but the chevron is a non-rotating `lucide-react` `<ChevronRight>` paired with `group-open:rotate-90`. Two issues: (1) the chevron is `aria-hidden` so VoiceOver can announce the summary text, but `<summary>` itself reports as a button without an "expanded" state in iOS Safari ≤ 17.3 (Webkit bug); the `<details>` is fine on all desktop browsers. The v1.5 iOS app will hit this bug. (2) The `marker:hidden` + `[&::-webkit-details-marker]:hidden` combo strips the native triangle but leaves the `<summary>` keyboard-focus ring at a 4px offset — visually it looks like the chevron has 4px of dead space on its left at focus. (3) The summary's hit target is `text-xs` line-height (~16 px) — under the 36 px floor.
- Fix: Wrap the summary's content in a 36-px-min hit zone via `min-h-9 -mx-1 px-1 py-1` so a finger tap reliably lands on the chevron. Keep the `<details>` primitive but add `aria-expanded` mirror via a tiny `useState` + `onToggle` to feed iOS-native screen readers.
### M2 — Sparkline on `/targets` has no min-points threshold guard for visual coherence
- Surface: `src/app/targets/page.tsx:264-303`, `:575-594`
- What: The page renders the sparkline whenever `points30d.length >= 2`. The Sparkline component itself returns null at `<2`; the API ships the field starting at >=3 points. But on a card with exactly 3-4 points spread over 30 days, the SVG paints a roughly horizontal trace that fills the full card width — it reads as "lots of activity" when really it's two flat segments. The path stroke uses `var(--dracula-purple)` (good — neutral, not status-coloured) but no fill gradient; the result on a card whose range bar above already shows a green/yellow/red marker is **a fourth element competing for the user's eye** (icon + value + range bar + sparkline).
- Why: The artboard's BriefingHero composition spec scoped the sparkline to "30 days = 30 points"; the API truncates to whatever the user logged, so a 4-point Withings-imported card has a totally different visual density to a daily-logged card. No empty-state.
- Fix: Either (a) require `length >= 7` before painting (one sparkline per week of data), or (b) render the trace with `stroke-opacity:0.5` for `length < 10` plus a `text-[10px] text-muted-foreground` caption "{n} of 30 days logged" so the user knows the trace's confidence.
### M3 — Recommendation card title uppercase severity badge re-introduces the "marketing eyebrow" pattern v1.4.20 M4 flagged
- Surface: `src/components/insights/recommendation-card.tsx:328-335`
- What: After v1.4.22 stripped `metric:<TYPE>` tokens from prose (good!), the `<Badge>` for severity still renders as `text-[10px] tracking-wide uppercase` displaying the literal `info` / `suggestion` / `important` / `urgent` token (e.g. "URGENT"). v1.4.20 M4 specifically called out uppercase eyebrows as breaking sentence-case consistency; the recommendation card carries five of those eyebrows in a typical "12-rec" advisor payload. Plus, the badge text is the **internal severity enum** ("urgent", "suggestion") — these should route through `t(\`insights.recommendation.severity.\${norm.severity}\`)` so German users see "Dringend" / "Vorschlag" instead of the English token.
- Fix: Drop `tracking-wide uppercase`, replace `{norm.severity}` with `{t(\`insights.recommendation.severity.\${norm.severity}\`)}`. Add the four DE/EN keys.
### M4 — Coach disclaimer relocated to sources rail is invisible when the rail is hidden on `<lg`
- Surface: `src/components/insights/coach-panel/sources-rail.tsx:283-294`, `coach-drawer-body.tsx:106-110`
- What: v1.4.22 B4 moved the disclaimer ("Coach replies are generated. Clinical decisions belong with your doctor.") into the sources-rail footer. On the desktop layout (`xl+`) the rail sits in the right column and the disclaimer is always visible. On `lg` (1024-1279 px) and `<lg` (every laptop and every mobile), the sources rail is **only reachable via the chevron-tray** — the disclaimer never paints unless the user actively opens the tray. That regression dropped the medical-disclaimer reach from "every Coach session" to "the subset of users who tap a chevron". Material risk if a clinician audit reads the Coach without seeing the disclaimer.
- Fix: Either (a) keep the disclaimer in its old composer slot too (tiny, single line under the input) OR (b) render it inline above the composer on `<xl` viewports as `text-[10px] text-muted-foreground border-t pt-2`. The B4 plan was sensible visually but the medical-safety angle outweighs the visual quiet.
### M5 — `/insights` row-fill rule on the medication-correlation row relies on `showMoodSection` heuristic
- Surface: `src/app/insights/page.tsx:1070-1078`
- What: The row-fill rule is `showMoodSection ? "grid gap-4 xl:grid-cols-2" : "grid gap-4"`. That works for the mood gate, but the **right card** can also be insufficient-data (n<5) while the mood section is otherwise visible. Result on a fresh tenant: BP-medication card carries the empty state (Activity icon + "Not enough data") in a half-width column with the mood-vs-BP card paint going to 100 % — the layout reads "two half-cards, one full" and the user has no idea why the right card moved. The v1.4.22 A4 commit hid empty _correlation-row_ tiles; this row's per-card empty state stayed.
- Fix: Either (a) hide the BP-medication card entirely when `bpMedicationScatterData.length < 5` and let mood-vs-BP take 100 %, OR (b) keep the fixed 50/50 layout and let both cards carry their per-card empty state (current v1.4.21 behaviour). Inconsistency between the trends-row hide-on-insufficient and this row's keep-empty-state pattern will read as "one of these is broken".
### M6 — Settings → Dashboard global comparison toggle has no live preview / "Try it" affordance
- Surface: `src/components/settings/dashboard-layout-section.tsx:204-241`
- What: The picker is a plain `<Select>` with three options + a hint paragraph. The picker is now the **only thing between the H1 and the widget table** (per the inline comment at line 184 — the redundant help line was removed). Saving the toggle flushes the value to the server which is then read by every chart + tile on the dashboard / insights / targets pages on next render. There's no way for the user to see what "Compare with last month" looks like before saving — the user has to save, navigate to `/`, see the change, navigate back, save the previous setting. The v1.4.16 B8 implementation accepted this because the picker lived inside a per-tile drawer; the v1.4.22 lift to a global setting widens the cost of a wrong choice.
- Fix: Add a tiny `<TrendCard>` preview swatch under the picker that re-renders on value change, OR bake an inline mini-chart that shows "this is what the comparison overlay looks like". The Dracula token consistency stays intact (use the existing TrendCard primitive).
### M7 — Sparkline aria-hidden hides the only signal of trend on mobile
- Surface: `src/app/targets/page.tsx:286-301`
- What: The SVG is `aria-hidden="true"` so screen-reader users skip it entirely — fine — but the sparkline is the only "are things changing?" affordance on the card aside from the `<TrendIcon>` (up/down/stable) at the top right. The trend icon is also `lucide-react` icons with no `aria-label`. Result: a VO user gets the value, the unit, the average, the range bar (status badge text), and the delta caption; the **direction of change** is implicit only from the delta sign. That's a regression compared to v1.4.21 where the trend icon was the only "direction" surface but at least carried a status icon.
- Fix: Either (a) keep the SVG `aria-hidden` but add `aria-label={target.trend === "up" ? t("targets.ariaTrendingUp") : …}` to the `<TrendIcon>` parent, OR (b) make the delta caption do the lifting: change "+5 vs. Vormonat" to "+5, slightly higher vs. last month" so the sentiment is in the prose, not just the colour.
---
## LOW
### L1 — "Worauf bezieht sich das?" / "What I'm looking at" disclosure title is asymmetric in voice
- `messages/en.json:874`, `messages/de.json:874`. The German question form ("Worauf bezieht sich das?") is curious + first-person; the English ("What I'm looking at") is declarative + first-person-singular. Both work in isolation but a bilingual user toggling locales will notice the voice shift. Consider either "Was schaut der Coach an?" / "What the coach looked at" (declarative, third-person) or "Worauf basiert das?" / "What's this based on?" (curious, declarative).
### L2 — Coach evidence rows render the LLM-emitted `kv.label` verbatim with no key stability
- `src/components/insights/coach-panel/message-thread.tsx:286-304`. The `<li key={\`\${kv.label}-\${idx}\`}>`allows duplicate labels (e.g. two "Average BP" entries for last 7 vs. last 30) to collide on a stable React key when the index is the same across renders. With the planned conversation-pagination this won't matter, but as soon as the message list incrementally grows, a duplicate-label key warning will surface in dev. Use`kv.id`if the schema has one, or hash`\${kv.label}-\${kv.window}-\${kv.value}`.
### L3 — Targets sparkline sets `preserveAspectRatio="none"` which distorts the line at extreme aspect ratios
- `src/app/targets/page.tsx:288`. With `preserveAspectRatio="none"` and the SVG container `h-6 w-full` (24px tall, ~280-440px wide), the X-axis is stretched 12-18× the Y-axis. A 0.5 kg movement over 30 days reads visually identical to a 5 kg movement because the Y range is auto-scaled to the data and stretched to fit. Consider a fixed Y-range pegged to the target range so the trace reads relative to "in target" / "out of target". Alternative: keep `preserveAspectRatio="xMidYMid meet"` and accept that some trace might not fill the full height.
### L4 — `/insights` page padding for `<InsightsSectionNav>` `scroll-mt-28` is hard-coded
- `src/app/insights/page.tsx:1020,1044,1258,1428,1463,1491,1592` all carry `scroll-mt-28` (~7rem = 112px). The sticky nav itself is ~40-44 px tall (py-2 + text-xs + a `border-b`). The 112-px scroll margin is ~2.5x the nav height, so when the user clicks "Pulse" the heading lands well below the top of the viewport with a noticeable gap. Either reduce to `scroll-mt-16` (64 px ≈ nav + breathing room) or document the rationale ("makes room for the hero re-anchor on smooth-scroll").
### L5 — Admin API tokens date cells now wrap to two lines on 1024 px viewports
- `src/components/admin/api-token-overview-section.tsx:243-250` (the v1.4.22 C2 fix). The 12 % `<col>` width allotment for the two date columns is ~84 px on a 700-px content area. `formatDateTime` produces "05.05.2026, 21:46" (~110 px). Dropping `whitespace-nowrap` lets the cell wrap to two lines — fine on a desktop with vertical real-estate, but the row now alternates between one-line and two-line heights based on whether each row's date wrapped, giving the table a "bumpy" rhythm. Easiest fix: use the short date format `formatDate` (no time) for the table and surface the precise stamp in a hover tooltip. Or push the `<col>` width to 14 % so the date fits on one line for German formatting.
---
## Things done particularly well
1. **Recommendation prose is finally clean.** Stripping `metric:<TYPE>` tokens out of the visible prose (and keeping the parse-side regex permissive enough to catch lowercase / snake_case hallucinations like `metric:blood_pressure_sweet_spot`) is the single biggest copy-quality win in v1.4.22. The split between strip-side and parse-side regex (`STRIP_TOKEN_REGEX` permissive, `PARSE_TOKEN_REGEX` strict) is the right shape — it kills leaks in the rendered DOM while still letting the renderer drop unrenderable tokens silently. Strong defensive design.
2. **BP tile architecture is cleaner than the comment suggests.** The "synthetic slope from 7d-vs-30d" trick (`page.tsx:822-837`) is a tidy way to give the BP tile feature parity with the other tiles without cooking up a dedicated trend pipeline. The pinned-to-1 confidence (line 836) prevents a downstream `R²==0` consumer from mis-treating it as low confidence — that's the kind of "land-mine the next contributor" annotation the codebase appreciates.
3. **Empty-state collapse on the correlation row + trends row is a calm UX pattern.** v1.4.22 A4's switch from "render an empty placeholder card" to "drop the card entirely and let the row collapse to 1-up / 100 %" gives the page a much calmer rhythm on a fresh tenant — no greyed-out half-rows screaming "you don't have enough data". The trends-row equal-height min-h-[300px] flex-col composition (lines 105-155 of trends-row.tsx) is the kind of layout discipline that pays off for the entire v1.5 surface.
---
## Design decisions worth pushing back on
1. **Sticky section nav above the hero (A5) competes with the hero for first-paint attention.** The strip is the first focusable element on `/insights` post-A5; the user's eye now has to choose between (a) the dense seven-tab strip and (b) the hero gradient + greeting + score panel. The artboard had the section nav inline below the hero specifically so the hero could land first. Pushing the strip above the hero gives every visit to `/insights` a "control panel" feel rather than a "your morning briefing" feel. Reconsider whether the v1.4.20 inline placement (just above the per-section content) was the right call after all; the v1.4.22 lift was a velocity-driven move, not a UX-tested one.
2. **Coach disclaimer relocation to the sources rail (B4).** The principle is right — keep the composer focused on the input affordance — but the execution dropped the disclaimer's reach from 100 % of Coach sessions to ~xl-only. Either keep both (composer + rail) or rethink the relocation. The medical-safety surface in HealthLog is one of the differentiators the user mentions in `feedback_ai_insights_differentiator`; quietly hiding the disclaimer behind a chevron erodes that differentiator.
3. **Settings cog removal (B5).** Agreed in principle (the v1.4.20 cog had no real wiring and the placeholder tooltip read as dead UI), but the comment ("A real settings surface for per-user prompt-tuning lands with v1.4.23") implies a return is planned. Consider whether the right replacement is _not_ a cog: per-message "thinking style" controls or a one-time onboarding ("How chatty should the Coach be?") would be more useful than a global preference panel that 95 % of users never open. This is a "design in v1.4.23" punt, not a v1.4.22 finding — flagging here so the next iteration doesn't reflexively re-add the cog.
.planning/phase-W5-v1422-product-lead-review.md
+492
View File
@@ -0,0 +1,492 @@
# v1.4.22 — Product-Lead Review
Author: Marc (strategic memo to self)
Date: 2026-05-10
Status: pre-release of v1.4.22; W2 Insights polish + W3 Coach
rewrite + W4 cleanup all on `develop`. W5 multi-agent review in
flight (this memo is the product-lead leg). v1.4.21 went out the
door yesterday; v1.4.22 is the next tag. This memo updates
`.planning/phase-D-v1420-product-lead-review.md`. Same audience:
Marc-three-weeks-from-now. The question this version answers:
**what state is the app in after v1.4.22 closed the polishing
arc, and what does v1.5 — the iOS native client + Apple Health
integration — actually need to be?**
---
## A. State of the App after v1.4.16 → v1.4.17 → v1.4.18 → v1.4.19 → v1.4.20 → v1.4.21 → v1.4.22
The arc is seven releases long and the shape is finally
unambiguous. **v1.4.16** was the polish-leap that overshot.
**v1.4.17** was the schema-strictness hotfix. **v1.4.18** walked
back the visual overshoots and grew Achievements into a real
engagement surface. **v1.4.19** was the deliberately-boring polish
release that lined up the runway for v1.4.20. **v1.4.20** was the
deliberately-loud product release: Coach drawer + streaming +
Daily Briefing + correlations + Weekly Report + Health Score all
landed clean against the artboard. **v1.4.21** was a five-commit
e2e-stability fix wave to unblock the public smoke-check before
more product weight landed. **v1.4.22 is the long-tail polish
release that closes loops we've been carrying since v1.4.16.**
The cadence is now a real two-beat rhythm: a loud release lands
a new product surface, and 1–2 patch releases tighten what that
release exposed before more weight goes on top. The Coach
surface bedded in for two releases before v1.4.22 came back to
rewrite its prose — the right sequencing.
**Quality bar trajectory: a small step-function up on the higher
level v1.4.20 set.** The Coach now talks like a partner instead
of a database cursor (W3), the BD-Zielbereich tile finally feels
like a live metric (W2 A1+A2), the Zielwerte page stopped reading
like a cheat-sheet (W4 C1), and a tail of 12 backlog items got
swept (W4 D).
**Test counts.** 2026 → 2109 unit tests (+83), 81 → 85
integration. New `src/lib/coach/sentinel.ts` parser landed with
18 cases. PROMPT*VERSION ratcheted 4.20.2 → 4.22.0 — the biggest
prompt jump of the v1.4.x line and the first time we've moved
the \_minor* digit, intentional because the persona, the
sentinel-block contract, and the warmth of the prose all changed
in one coherent step.
**Concerning regressions: none on the surfaces that shipped.**
The sentinel-parse path has hard-cap defence-in-depth (1 KB
post-`---KEYVALUES---`, 8 lines max, length-capped Zod per
line); the cookie-mirror onboarding redirect is a UX hint not a
security signal; the api-tokens scrollbar fix is a two-class
deletion with a regression-pin test. The fixes that landed are
small enough to audit by eye.
**Tech debt status — three items I named in the v1.4.20 memo**:
1. **Insights page `~9-surface orchestrator`.** Still un-split.
v1.4.22 deliberately did not touch it because v1.5's Apple
Health work edits the same sub-trees and a pre-emptive split
would re-split twice. **Hold for v1.5 P5.**
2. **Schema drift on `medication_schedules.days_of_week`.**
Still unresolved; Sr-MED-5 is alive. **Drop or land in
v1.4.23, latest v1.5 P0.**
3. **B1 Vitals tile row.** Still deferred. v1.5 P5 is when it
lands — once HRV / Sleep / Resting HR / Steps are non-stub
data, the tile row replaces the status-card list rather than
adding a row above it.
Two new items on the watchlist:
4. **`CoachDrawer key={prefill}` weaponising React keys
(Sr-HIGH-4).** Heavy refactor, not in W4 quick-wins.
v1.4.23.
5. **Pearson p-value normal approximation at low df
(Code-MED-03).** At df=12 the normal approx puts true p≈0.04
at p≈0.025 — generous in the wrong direction. Pull in a
30-LOC incomplete-beta or raise the surfacing gate to
df ≥ 20. **Fix before v1.5/v1.6 auto-discovery ships** — the
false-positive cost compounds with auto-generated
correlations.
---
## B. Biggest items shipped in v1.4.22
In priority order:
1. **Coach prose rewrite + evidence-block sentinel (W3).** The
biggest user-facing impact. The Coach used to open every
reply with a number — clinical, database-cursor energy. W3
reframes the persona as "warm, neugierig, zurückhaltend": a
partner sitting alongside the user, not pushing data. Numbers
move out of the prose into a `---KEYVALUES---` … `---END---`
sentinel block that the route parses out and renders as a
collapsible `<details>` disclosure under each assistant turn,
closed by default and hidden when no key-values came back.
The persona + sentinel land together because either alone is
incomplete: warm prose without an evidence block hides the
data; an evidence block without warm prose is a tooltip.
Three-way hard caps on the sentinel (1 KB payload, 8 lines,
per-line Zod) so a prompt-injection attempt can't grow the
persisted envelope. PROMPT_VERSION 4.20.2 → 4.22.0 — first
minor-digit bump in v1.4.x. Files: `src/lib/coach/prompts/
coach-prompt.ts`, `src/lib/coach/sentinel.ts` (new),
`src/components/insights/coach-panel/message-thread.tsx`,
`src/lib/ai/schema.ts`. Report: `phase-W3-report.md`. **This
is the feature that defines v1.4.22.**
2. **BD-Zielbereich tile re-anchor + feature parity (W2 A1+A2).**
Fourth attempt at this metric. v1.4.19 routed the headline to
`allTime` to fix the algorithmic 50/50/50 pin; v1.4.22
re-anchors to `last30Days?.pct` and surfaces all-time as a
sub-row, because v1.4.19's fix was emotionally wrong — the
headline became the slowest-moving aggregate possible,
punishing a user who put in real recent work. The tile also
gets a synthesised trend arrow (slope = `(pct7 - pct30) /
30`), a 7-day-trend chip, and a comparison-overlay caption.
Shared `<TrendCard>` chrome stays uncompromised: the new
third sub-value is opt-in via an `avgAllTime` prop that
defaults to undefined. Files: `src/app/api/analytics/route.ts`,
`src/components/charts/trend-card.tsx`, `src/app/page.tsx`.
Report: `phase-W2-report.md`.
3. **Zielwerte page sparkline + Δ-vs-last-month (W4 C1).** The
target-values page stopped reading like a clinical cheat-
sheet. Each `<TargetCard>` grew a 30-day inline SVG sparkline
beneath the range bar plus a localised "Δ −2.3 kg vs. last
month" caption. The API ships `points30d` + `deltaVsLastMonth`
per target; both null when either window has fewer than 3
readings so cold-start accounts don't see a misleading flat
trace. BMI piggybacks on the weight series so its sparkline
shares the range bar's y-axis. Direction A from W1a §4 —
lowest-effort, highest-payoff. Direction C (storyline framing
with hero cards for the most-out-of-target metrics) is the
v1.6 product call. Files: `src/app/api/insights/targets/
route.ts`, `src/components/insights/target-card.tsx`,
`src/app/__tests__/targets-sparkline.test.tsx` (new).
4. **Insights surface polish + DE rename (W2 A3+A4+A5+A6).**
Four interleaved surface fixes that move the page from
"feature-rich and slightly cluttered" to "deliberate". A3
retired the on-surface `<CompareToggle />` from `/insights`
(canonical picker has lived in Settings → Dashboard since
v1.4.16 B8; the duplicate violated
`feedback_settings_no_split.md`). A4 fixed three half-row
layouts that left empty space, equalised trends-row card
heights, and stopped painting placeholders for insufficient
correlations. A5 renamed "Muster" → "Zusammenhänge" /
"Patterns" → "Relationships" (W1b research recommended
either "Trends" or "Zusammenhänge"; picked the latter
because the row directly above already uses "Trends"), and
lifted the section tabs above the hero. A6 stripped the
production `metric:WEIGHT` / `metric:BLOOD_PRESSURE_SYS`
token-leak in recommendation prose (W1a §3 traced it to a
single missing `stripChartTokens()` call) and fixed four DE
Health-Score component labels that read English in the
German bundle (`Mood` → `Stimmung`, `BD` → `Blutdruck`,
`Medis` → `Einnahmetreue`, plus `Gewicht`). Reports:
`phase-W2-report.md`, `phase-W1a-report.md`.
5. **AuthShell post-hydration redirect → proxy.ts (W4 C4).**
4th-attempt fix at the dashboard-flash on incomplete-
onboarding. The redirect moved out of `<AuthShell>`
`useEffect` into `src/proxy.ts`. The proxy runs in the Edge
runtime and can't reach Prisma, so auth routes mirror the
DB `onboardingCompletedAt` column into a non-httpOnly
`hl_onboarding` cookie. The proxy reads it without a DB
roundtrip; tampering only skips the dashboard flash and
never bypasses a server check.
`/api/onboarding/complete` and session-destroy clear the
cookie. The e2e onboarding-flicker spec returned to testing
the actual incomplete-onboarding case. Files: `src/proxy.ts`,
`src/app/api/auth/*`,
`tests/integration/proxy-onboarding-redirect.test.ts` (new).
6. **Backlog sweep (W4 D, 12 items).** Ten quick-wins from
`v1421-backlog.md` plus two mediums. Notables:
`LOKI_ENDPOINT` env-var leakage in user-facing copy;
admin/users column header `"Users"` → `"Name"`; six DE
error-toast keys retoned from imperative to softer phrasing;
`formatTokenName` regex broadened to non-Z ISO offsets;
moodLog "Copy webhook secret" using `common.copy`. FX
hygiene carry-overs: 191 source-comment `Marc` references
swept, v1.4.14+v1.4.15 bilingual CHANGELOG entries
normalised to English-only (per
`feedback_marc_voice_english.md`), `CLAUDE.md` →
`CONTRIBUTING-AI.md` rename. Report: `phase-W4-report.md`.
---
## C. v1.5 implementation plan — iOS native client + Apple Health
The handoff at `~/Projects/healthlog-iOS/` is the iOS app. v1.5's
headline is **iOS native client + Apple Health integration**.
The web app gets the API surface it already shipped in v1.4
(bearer + refresh tokens, idempotency-key — see
`~/Projects/healthlog-iOS/HealthLogIOS/docs/server-changes.md`).
Apple Health adds new measurement sources (HRV, Sleep, Resting
HR, Steps, BodyFat, Glucose). The Coach extends to ingest them.
The iOS handoff already lays out 9 phases (P0 done, P1–P9 staged
by surface).
My v1.5 plan from the server side: six phases, each ≤7 days,
sequential where they must be and parallelisable where they
don't.
### Phase P1 — iOS app first launch (~5-7 days)
Smallest deliverable that proves bearer + refresh-token end-to-
end: one login page, one dashboard widget. Email/password login
via `POST /api/auth/login` with `X-Client-Type: native` (issues
`hlk_<64hex>` access token + `hlr_<64hex>` refresh token per
CLAUDE.md). Keychain stores both. Dashboard pulls
`/api/auth/me` + `/api/analytics` and paints the latest weight +
BD-in-target % in a Lock Screen Widget. Passkey login is punted
to P3.
**Server-side files.** `src/lib/auth/issue-token.ts` (already
exists). New `src/lib/auth/refresh-rotate.ts` + `POST
/api/auth/refresh` that takes `hlr_*` and returns a fresh pair,
revoking every refresh token on reuse-detection.
`docs/api/openapi.yaml` documents the refresh route for iOS DTO
codegen.
**iOS-side files.** `HealthLog/App/Auth/LoginView.swift`,
`HealthLog/Services/APIClient.swift` (Bearer + 401 → refresh
chain), `HealthLogWidgets/LatestWeightWidget.swift`.
**Risks.** (1) Refresh-token reuse-detection is a foot-gun for
two-device users. Document as "one active device per account"
and add `GET /api/auth/me/devices` for the Settings tab to show
- revoke. (2) Cert-pinning. The iOS production scheme has empty
pin-hashes per the iOS ROADMAP Phase 1; wire before first
TestFlight — a missed pin update on Cloudflare cert rotation
bricks every device.
### Phase P2 — Apple Health sync + measurement-source taxonomy (~6-7 days)
Biggest contract decision of v1.5. The iOS app reads HRV / Sleep
/ Resting HR / Steps / BodyFat / Glucose from `HKHealthStore` and
posts them to the server. Server work: (a) extend
`MeasurementType` enum; (b) introduce a `source` field on
`Measurement` distinguishing `apple-health` / `withings` /
`manual` / `mood-log`; (c) batched ingest endpoint taking
~hundreds of points per HealthKit anchor delta; (d) deduplicate
on `(source, externalId)` not `(timestamp, type)` so the same
physical reading flowing through the Withings → Apple Health
bridge doesn't get blocked.
iOS-side: `HKObserverQuery` for live updates +
`HKAnchoredObjectQuery` for incremental sync, anchor in
Keychain.
**Files.** New `prisma/migrations/0036_apple_health_
measurements/migration.sql`. New `src/app/api/measurements/
batch/route.ts` (POST, hard cap 500/req, idempotency-key envelope
per batch). New `src/lib/measurements/source-dedup.ts`.
`docs/api/openapi.yaml` regenerated.
**Risks.** (1) `MeasurementType` enum expansion — additive only,
never rename. (2) Withings → Apple Health bridge collision: the
dedup key is `(source, externalId)`. (3) Doctor-report PDF
should surface measurement source — a reading from Apple Health
that came from a third-party scale should say so.
### Phase P3 — Coach + Daily Briefing extended for new metrics (~5 days)
Today's Coach knows BP, weight, pulse, mood, compliance. v1.5
adds HRV, Sleep, Resting HR, Steps. Three places to extend:
(1) `extractFeatures()` + `src/lib/ai/coach/snapshot.ts` so the
provenance envelope label-tags the new metric keys; (2) prompt
rules in `src/lib/coach/prompts/coach-prompt.ts` so the model
knows units, windows, and conservative phrasing; (3) source-chip
i18n bundles (`insights.coach.metric.{hrv,sleep,restingHr,
steps}` in EN+DE).
The Health Score formula stays four-weighted for v1.5 — adding a
fifth component would force a band recalibration that isn't
worth the user-side surprise. v1.6 territory.
**PROMPT_VERSION ratchet 4.22.0 → 5.0.0** — major-digit bump.
New ground rules: (a) no clinical sleep-stage conclusions, no
causal language about HRV "telling" the user anything; (b)
sleep-duration-not-sleep-stages framing; (c) steps-as-context-
not-prescription. Each lands as a numbered bullet so the
schema-test regex on the prompt header catches every bump.
**Risks.** Sleep classifier disagreement (Apple vs Withings ~25%
divergence on REM/Deep/Core). Add a prompt unit-test that
asserts the model never claims a clinical sleep-stage
conclusion. HRV similarly — talk about trends in the user's own
baseline, never against a population norm.
### Phase P4 — Per-metric APNs alerts (~4-5 days)
iOS users should get APNs direct rather than the web-push
fallback. `POST /api/devices` exists in scaffold (v1.4
native-client work added the cross-user-hijack guard with 409 +
`device.register.denied` audit trail per CLAUDE.md); APNs
sender-side wiring is v1.5 work.
**Files.** New `src/lib/notifications/senders/apns.ts` (p8-key
auth, JWT signing every 50 minutes, JWT pool reuse), APNs config
row in `Settings` admin section, env block `APNS_KEY_ID` /
`APNS_TEAM_ID` / `APNS_KEY` (the .p8 contents). Per-event
opt-out reuses existing notification-rules.
**Risks.** APNs sandbox vs production pivots on a per-payload
`apns-push-type` header. Test with TestFlight against
production APNs gateway. Rate-limit at the sender (~20 req/sec
per connection without throttling).
### Phase P5 — Web app polish + v1.5 release brief (~4 days)
Two visible web-app changes once iOS is shipping:
1. **The hero's "Apple Health: coming with iOS app" placeholder
fills.** Web users with a sync'd iOS device see HRV in the
third hero slot; users without one see "Connect Apple
Health" linking to the App Store listing.
2. **Vitals tile row lands.** B1's deferred layout has waited on
real HRV / Sleep / Resting HR / Steps since v1.4.20. P5 is
when it replaces the status-card list — the B1 structural
blocker dissolves once the metrics are non-stub.
The Insights page split (the ~9-surface orchestrator) also lands
here, not in v1.4.23. Splitting during P5 means the split maps
to the exact sub-trees Apple Health touched, not a guess.
**Files.** `src/app/insights/page.tsx` (split into `page.tsx` +
`_components/{hero,briefing-row,score-row,correlations-row,
trends-row,coach-cta}.tsx`), `src/components/insights/hero-
strip.tsx`, new `src/components/insights/vitals-row.tsx`.
Release brief in Marc's voice (English, no "Claude / agent /
phase X" leaks per `feedback_marc_voice_english.md`). New
`docs/audit/v15-summary.md`.
### Phase P6 — Cross-user feedback aggregation cron (~3-4 days, parallelisable)
The v1.4.16 B5e infrastructure that's been waiting for the
Coach to have volume. Every release I've pushed it off because
the volume wasn't there. v1.4.22's prose rewrite is what makes
it land cleanly: the warmth + sentinel pattern produces
conversations users finish, which produces the helpful/
unhelpful signal the aggregator needs. `src/lib/jobs/feedback-
aggregator.ts` already exists; v1.5 wires it to a daily
pg-boss cron that reads the `(severity × confidence_band)`
buckets and appends OMIT / REPHRASE rules to the next
PROMPT_VERSION when a bucket's helpful-rate drops below 50%.
**Files.** `src/lib/jobs/feedback-aggregator-cron.ts` (daily
04:00 Europe/Berlin), `src/lib/coach/prompts/prompt-version-
manager.ts`. PROMPT_VERSION channel: 5.0.x → 5.0.x+1 on each
rule append.
**Parallelisable** with P1–P5 — no shared file paths.
---
## D. Strategic items NOT in the iOS handoff but relevant to v1.5
Six items that aren't on the iOS roadmap but should ride
alongside:
1. **Schema drift cleanup on `medication_schedules.days_of_week`
(Sr-MED-5).** v1.4.20 worked around it twice; v1.4.22 didn't
touch it. v1.5 P0 should land the missing migration OR drop
the column from `schema.prisma`. The work-around `select`
clauses become tech-debt as more queries touch the table.
2. **Pearson p-value normal-approx replacement (Code-MED-03).**
At df=12 the normal approx puts true p≈0.04 at p≈0.025 — a
generous direction we don't want when v1.5/v1.6 ships auto-
correlation discovery. Either pull in a 30-LOC incomplete-
beta or raise the surfacing gate to df ≥ 20. v1.5 P0 work,
not P5 — the false-positive cost compounds when correlations
get auto-generated.
3. **`<CoachDrawer key={prefill}>` proper state management
(Sr-HIGH-4).** The drawer should make `prefill` a fully-
controlled prop and drop the `key=` re-mount that
weaponises React keys for state reset. Heavy refactor; not a
one-line fix. v1.4.23 if there's room before v1.5 starts;
v1.5 P3 latest. The iOS Coach surface will multiply this
pattern's footprint and we want it clean before that lands.
4. **Conversation persistence eviction policy.** Coach storage
shipped in v1.4.20; v1.4.22 grew the prose without changing
the persistence shape. Auto-archive conversations after 90
days (move `messages` to a cold table, keep the
`conversation` row as a stub for the rail); hard-delete
after 365 days. ~500 KB/user/month at 20 turns × 50
conversations is tolerable today but needs the policy
before the iOS app multiplies volume 3-5×.
5. **OpenAPI spec drift CI guard.** The iOS app reads
`docs/api/openapi.yaml` from the server repo (per
`~/Projects/healthlog-iOS/HealthLogIOS/docs/api-contract.md`);
a server-side route signature change that doesn't update the
spec will only get caught at iOS-side DTO codegen time. CI
check on the server repo that diffs the routes against the
spec on every PR. Pull this forward to v1.4.23 if possible —
it's cheaper than discovering drift via an iOS build break.
6. **Settings/Integrations: `<IntegrationCard>` shell
extraction.** Withings and moodLog card chrome are duplicated
verbatim. v1.5's Apple Health card will copy-paste the same
shape a third time. Extract `<IntegrationCard>` alongside the
existing `<IntegrationStatusPill>` before the third copy
lands. v15-backlog `D-SR-H-3` already tracks it; v1.5 P5.
---
## E. Risks / Tech-Debt watchlist
1. **Coach token-budget cap is a soft wall.** 25 000 tokens/
user/day at ~1 800 tokens/turn = ~13 turns. Heavy iOS users
on long road trips ("I want to think out loud about my
health for 30 minutes") will hit the cap. The HTTP 429 is
correct but the UX needs a clear "you've reached today's
Coach limit; resets at 00:00 UTC" message rather than a
generic error toast. v1.5 P3's iOS Coach surface makes this
more visible than the web-only surface today.
2. **The "summarise older half" pass is still a placeholder.**
The B2a history-window builder injects a synthetic
`[summary placeholder — N earlier turns elided]` line rather
than calling a provider to produce a real summary. Cheap and
deterministic; an opt-in real-summarisation pass can land
alongside v1.5 once the drawer UI shows whether users push
past 20 turns. The W3 sentinel pattern actually gives us a
cheap real-summary now: feed the older half's prose +
sentinel rows to a small-model summary call. v1.5 P3 if
usage data shows it matters.
3. **OpenAPI spec drift between server and iOS DTOs.** Same
item as D.5 above. Calling out twice because it's the
single most likely place v1.5 development bogs down.
4. **Refresh-token reuse-detection is a foot-gun for two-
device users.** The reuse-detection (revokes every refresh
token for the user when the same token is replayed —
CLAUDE.md headless-client section) is correct for security
but punishes any user with two iOS devices on the same
account. Document in onboarding ("only one active device
per account"); add `/api/auth/me/devices` for the iOS
Settings tab. v1.5 P1.
5. **`safeParse()` legacy-payload pattern carries forward.**
The v1.4.22 wave added one new `safeParse()` site (the
sentinel keyValues block, falls back to no-disclosure on
parse failure). The v1.5 PROMPT_VERSION 5.0.0 bump will
add at least four more (HRV envelope, Sleep envelope,
Resting-HR envelope, Steps envelope). Audit every new
`safeParse()` call before tag — same lesson as v1.4.17
hotfix.
6. **The Coach prose rewrite (W3) hasn't seen production
feedback yet.** v1.4.22 ships the persona + sentinel
together; both will go live to the live tenant on tag.
Watch the helpful/unhelpful ratio in the first week. The
risk: the persona pivots too far towards warmth and the
evidence block becomes the only place the user finds an
actionable answer. If the disclosure-open rate is below
~30% in the first week, the persona might need a tone
pull-back in v1.4.23 before the iOS surface multiplies the
audience. The aggregator cron in P6 is the right vehicle
for this measurement once it's wired.
7. **Insights page split gets tied to v1.5 P5, not v1.4.23.**
Deliberate. The senior-dev review for v1.4.20 will likely
re-flag it; the review for v1.4.22 won't make a difference
because the page didn't grow. Splitting before knowing
which sub-trees v1.5 Apple Health work edits risks re-
splitting twice. Holding the line.
---
## F. Candid one-liner
v1.4.22 is the second-beat polish that v1.4.20's loud release
needed to feel finished — Coach prose finally talks like a
partner, the BD tile finally feels like a live metric, and the
backlog tail is short enough that v1.5 starts on real product
work instead of debt repayment.
.planning/phase-W5-v1422-security-review.md
+59
View File
@@ -0,0 +1,59 @@
# Wave 5 — Security review (v1.4.22)
Branch: `develop` (24 commits ahead of `main`).
Scope: every commit between `main` and `develop` listed in the brief, focusing on the six v1.4.22 surfaces called out in the audit prompt.
## Summary
- New attack surface in v1.4.22: a server-side onboarding redirect in `proxy.ts` driven by a non-httpOnly `hl_onboarding` cookie, a Coach `---KEYVALUES---` sentinel parser that strips an attacker-controllable block from the streamed assistant prose, a substantially rewritten Coach system prompt (warm tone + few-shot tone calibration), a `comparisonBaseline` field persisted into the per-user `User.dashboardWidgetsJson` blob, the `hl_onboarding` cookie itself, and a `?force=true` query appended to the Coolify deploy webhook URL in CI.
- Findings: **0 CRITICAL · 0 HIGH · 2 MED · 4 LOW**.
- Overall posture: solid. The proxy redirect is correctly scoped (read-only cookie, fixed redirect target, API routes exempt, `/onboarding` itself in `PUBLIC_PATHS` so no loop). The sentinel parser is well-defended with a 1 KB cap, an 8-line cap, per-line Zod validation, graceful degrade on missing `---END---`, and an explicit comment that the parsed values feed `provenance.keyValues` rather than being eval'd or rendered as raw HTML. The Coach system-prompt rewrite contains no instructions that would teach the model to leak the system prompt or ignore guardrails. The comparison-baseline field is per-user and only ever read/written through `requireAuth()` against the caller's own row. The Coolify webhook still requires both the URL and bearer-token secret. The two MED items are about the new cookie's `SameSite` posture and the redirect-control fence on `/onboarding` matching too loosely; the LOWs are minor hardening notes.
## Findings
### MED-1 — `hl_onboarding` cookie is `SameSite=Lax`, not `Strict`
- File: `src/lib/auth/session.ts:32` (`setOnboardingPendingCookie`).
- What: the audit brief explicitly asked the cookie to be `SameSite=Strict`. The implementation sets `sameSite: "lax"` (and explicitly `httpOnly: false`, by design as a UX hint).
- Why: `Lax` is the right posture for the `healthlog_session` cookie because the Withings OAuth callback arrives via top-level cross-site redirect (documented in the same file at line 60). The onboarding flag is **not** load-bearing for any cross-site flow — no third-party redirects ever depend on `hl_onboarding`. The brief's `Strict` guidance therefore is achievable without breaking any flow, and `Strict` would harden against a top-level cross-site `GET` from an attacker page that tries to coax the victim's browser to silently replay the cookie on a redirect chain. The exposure is small (the cookie is just `"pending"` vs absent and the proxy treats it as a UX hint, not auth), but the brief is explicit.
- Fix: change `sameSite: "lax"` → `sameSite: "strict"` for the `hl_onboarding` cookie only. Leave `healthlog_session` as `Lax` because of the Withings callback. Add a one-line code comment explaining the divergence so a future audit doesn't homogenise the two.
### MED-2 — `/onboarding` `startsWith` match is loose enough to skip the auth gate for unintended paths
- File: `src/proxy.ts:28-33` (`PUBLIC_PATHS` + `isPublicPath`) and `src/proxy.ts:152` (the redirect-skip).
- What: `PUBLIC_PATHS` is matched with `pathname.startsWith(p)`. The entry `"/onboarding"` (no trailing slash) therefore admits `/onboardingfoo`, `/onboarding-export`, `/onboarding.json`, etc. as public. Today no such route exists, but Next 16 App Router would happily mount one if a future contributor added `src/app/onboarding-foo/page.tsx`, and that page would be reachable without a session cookie because the proxy's auth check runs `if (!isApiRoute && !isStaticFile && !isPublic)`.
- Why: every other entry in `PUBLIC_PATHS` is either a literal terminal path (`/api/auth/login`, `/robots.txt`) or already trailing-slashed (`/auth/`, `/api/auth/`, `/api/monitoring/`, `/api/ingest/`). `/onboarding` is the lone exception. The same loose match is reused at line 152 for the **inverse** check (`!pathname.startsWith("/onboarding")` — anything starting with `/onboarding` is treated as the onboarding surface and the redirect short-circuits). A future `/onboarding-export` route would silently inherit both behaviours, which is the kind of footgun a security review should call out before it ships.
- Fix: rewrite both occurrences to be exact-match for the page itself plus its subroutes: change the array entry to `"/onboarding"` with an explicit equality check (`pathname === "/onboarding" || pathname.startsWith("/onboarding/")`). Mirror the same pattern in the redirect short-circuit at line 152.
### LOW-1 — `/api/auth/me` is a GET that mutates state (cookie write) on every call
- File: `src/app/api/auth/me/route.ts:16` (`setOnboardingPendingCookie` inside the GET).
- What: every call to GET `/api/auth/me` now writes the `hl_onboarding` cookie. GETs that mutate state are normally a CSRF smell.
- Why: in this case the only mutation is "make the cookie match the DB". An attacker who can force the victim's browser to GET `/api/auth/me` (which requires a same-origin fetch — `/api/auth/me` is not in `PUBLIC_PATHS` and a cross-site GET in 2026 cannot read the response) only re-anchors the cookie, which is the exact behaviour the route already wants. No upgrade-of-privilege, no data write. Calling this LOW only because it is the kind of pattern a future contributor might cargo-cult into a more dangerous surface.
- Fix: leave as-is. Optionally add a code comment noting that the GET-side mutation is deliberately idempotent and is not allowed to grow.
### LOW-2 — Coach system-prompt few-shot examples bake in concrete numeric values (138, 142/88, 4.1, 96 %)
- File: `src/lib/ai/coach/system-prompt.ts:140-156, 312-340` (EN + DE few-shots).
- What: the new tone-calibration few-shots include illustrative metric values inside `---KEYVALUES---` blocks (`avg7 systolic: 138`, `Tue 6 May: 142/88`, `30-day adherence: 96`).
- Why: the `feedback_no_pii_in_user_facing.md` rule says Coach prompt drafts must not bake live-tenant figures into the system prompt. These values are clearly synthetic (round numbers, mocked dates) and document the sentinel format the parser expects, so they are educational not operational. Calling LOW because a future contributor copy-pasting "real" numbers into the same template would silently leak.
- Fix: leave as-is. Optionally add a `WARNING: synthetic values only — never paste real tenant readings here` comment line at the top of `COACH_PROMPT_EN` and `COACH_PROMPT_DE` so the rule survives future edits.
### LOW-3 — Coach `transcript` block does not delimit user input from role labels
- File: `src/app/api/insights/chat/route.ts:211-220`.
- What: the user prompt is built by joining persisted messages with `${role.toUpperCase()}: ${content}` and concatenating them under a `CONVERSATION` header. A user message containing the literal text `\n\nASSISTANT: …` could in principle confuse the model into treating subsequent prose as the assistant's own turn.
- Why: this is a self-injection at worst — the only conversation the user can poison is their own (each conversation is owned by `userId` and persistence enforces it). The refusal detector still runs upstream and would catch the obvious injection patterns. The system prompt's grounding rule ("ground every number in the SNAPSHOT") plus the prompt budget cap keep impact tiny.
- Fix: leave as-is for v1.4.22. If self-injection becomes a quality issue, switch the transcript builder to use a structured chat-completion payload (one `{ role, content }` per persisted turn) instead of flattening everything into a single user prompt — the provider runner already supports this on the OpenAI/Anthropic adapters.
### LOW-4 — Sentinel parser drops the `truncated` flag without surfacing it to the persisted provenance
- File: `src/lib/ai/coach/keyvalues.ts:147-180` and `src/app/api/insights/chat/route.ts:280-291`.
- What: when a sentinel block exceeds the 1 KB cap, the parser sets `truncated = true` internally and folds it into `malformed`. The route logs `coach.keyvalues.parse_failed` but does not record per-message whether the failure was truncation vs missing close vs zero valid rows. A real attacker would prefer the truncation path because the parser still keeps the first 8 rows from the truncated body.
- Why: low-impact because the model would have to produce 1 KB+ of valid-looking lines to push junk past the byte cap, and the per-line Zod schema (`label ≤ 80`, `value ≤ 40`, `unit ≤ 16`, `window ≤ 40`) bounds what survives. Calling LOW because the wide-event metadata is the only ops-side signal of an unusually large sentinel.
- Fix: extend the wide-event meta in `chat/route.ts:286-289` with `truncated` and `closeMissing` booleans so a regression on the upstream prompt format is observable.
## Strongest defensive postures (v1.4.22 added)
- The Coach sentinel parser is **defence-in-depth done right**: a 1 KB byte cap before tokenisation, an 8-row line cap, per-line `coachKeyValueSchema.safeParse` validation (label/value/unit/window are length-bounded), explicit `OPEN`/`CLOSE` markers stripped from the body so a model cannot smuggle a nested sentinel, graceful degrade on missing `---END---` (returns the prose unchanged + flags `malformed: true`), and a wide-event `coach.keyvalues.parse_failed` annotation for ops visibility. The persisted `metricSourceJson` schema (`provenanceFromJson` in `persistence.ts:73-140`) re-validates every field on read so a malformed historical row cannot poison a re-render after a parser change.
- The new `proxy.ts` redirect is **correctly framed as a UX hint, not an auth gate**: comments at `session.ts:11-21` and `proxy.ts:142-149` are explicit that an attacker editing the cookie locally only skips the dashboard flash; every actual data-access check stays server-side via `requireAuth()` against the DB-backed `Session` row. The redirect runs only for non-API non-public surfaces (so `/api/*` is unaffected and cannot be 307'd into a redirect loop), the redirect target is a fixed string (no open-redirect surface), and `/onboarding` is in `PUBLIC_PATHS` so the redirect cannot loop.
.planning/phase-W5-v1422-senior-dev-review.md
+374
View File
@@ -0,0 +1,374 @@
# Wave 5 — Senior-dev review (v1.4.22)
Reviewer: senior-dev lens, read-only
Branch: `develop` @ `363e73c` (18 commits ahead of `main`)
Inputs: phase-W1a/W1b/W2/W3/W4 reports + diff walk
## Summary
Architectural posture: **healthy, with one structural smell.** v1.4.22
is a polishing release; almost every chunk lives inside an existing
seam (TrendCard prop, CorrelationRow filter, TargetCard sub-component,
proxy redirect rule, sentinel parser as a side-module under
`src/lib/ai/coach/`). New abstractions are placed where they belong —
`keyvalues.ts` next to its only caller, `setOnboardingPendingCookie`
next to `createSession`/`destroySession`, the `Sparkline` component
inline in `targets/page.tsx` because it's used exactly once. Tests
land alongside the units they exercise. The one structural concern is
the C4 onboarding-cookie design: it writes a cross-cutting UX-flag
cookie from six different write sites (login, passkey-verify,
register, password-reset, /me, /onboarding/complete) and relies on
each to stay in sync with the DB column. That sync is correct today
but every future auth surface inherits the responsibility.
Findings: **0 CRITICAL · 2 HIGH · 5 MED · 4 LOW**
---
## CRITICAL
(none)
## HIGH
### H1 — `hl_onboarding` cookie sync is a write-fan-out with no
single point of truth (W4 C4)
**Where:** `src/lib/auth/session.ts:setOnboardingPendingCookie`,
called from six different routes (`login`, `passkey/login-verify`,
`register`, `password`, `auth/me`, `onboarding/complete`).
**Symptom:** every route that touches the user's session — present
or future — has to remember to call `setOnboardingPendingCookie`. A
new auth path (e.g. magic-link, OAuth, social login, the v1.5 SSO
work) that forgets to set the cookie will quietly skip the
proxy-level redirect and re-introduce the dashboard flash. The
helper is not enforced by the type system; `createSession` does not
require it; `destroySession` clears it but `createSession` doesn't
set it. The sync is by-convention, not by-construction.
**Architectural lift:** fold the cookie write into `createSession()`
itself (the call site already knows the user) so issuing a session
without onboarding state becomes impossible. The `/api/auth/me`
mirror remains useful for legacy sessions that predate the cookie
but should be a fall-back, not a primary write path. The
`destroySession` clear is already correctly co-located.
**Severity:** HIGH because the failure mode is silent (no test
catches "forgot to call the helper") and the regression it
re-introduces is the exact one C4 was designed to fix.
### H2 — Coach SSE handler stays inside the API route file even
though it's now ~480 LOC and combines five concerns (W3)
**Where:** `src/app/api/insights/chat/route.ts`.
**Concerns layered into one handler:**
1. Auth + body validation
2. Refusal detection + short-circuit (own SSE path)
3. Conversation resolution + persistence
4. Provider-chain orchestration + fallback handling
5. Sentinel parsing + provenance enrichment + streaming
The Wave 3 sentinel parser was correctly extracted to
`src/lib/ai/coach/keyvalues.ts`. Everything else still lives in
`route.ts`, which means the streaming construction
(`tokeniseForStreaming`, `encodeFrame`, `streamRefusal`,
`streamProviderError`, `start(controller) { try { … } finally {
controller.close(); } }`) is duplicated three times in one file. Two
of those copies enqueue different frame mixes (refusal: token + done;
provider-error: error only; happy path: tokens + provenance + done).
The pattern begs for a `buildSseStream(frames: CoachStreamEvent[])`
helper, ideally in `src/lib/sse.ts` next to `apiHandler`'s
neighbours.
**Why HIGH not MED:** the v1.5 iOS work is going to add another
streaming endpoint (likely the daily-briefing live regenerate). If
the SSE construction stays inline, that endpoint will copy-paste
this file's three try/finally blocks. The right time to extract is
before the second consumer lands.
---
## MED
### M1 — `analytics` route fetches every BP measurement ever, twice,
to compute `bpInTargetPctAllTime` (W2 A1)
**Where:** `src/app/api/analytics/route.ts:86-95`.
The route runs `prisma.measurement.findMany({ userId, type:
"BLOOD_PRESSURE_SYS" })` and the dia equivalent with no `where:
{ measuredAt: { gte: … } }` clause. For a tenant with several years
of paired BP readings the rows count balloons unboundedly. The
helper `computeBpInTargetWindows` then walks all of them to compute
the all-time pct alongside the 7/30-day windows. v1.4.22 A1 didn't
introduce this — v1.4.19 A1 did when it added the all-time field —
but A1 made the all-time number user-visible (third sub-row),
locking in the unbounded read.
**Fix:** at minimum bound by a sane horizon (e.g. last 5 years) or
materialise the all-time aggregate into a small per-user
`bp_target_aggregate` row, refreshed on measurement-write. The
dashboard hits this route on every page view and the Insights page
hits it again — same query.
### M2 — `targets` route's `sparklinePoints` allocates a `Map` per
type per request, called 7× per render (W4 C1)
**Where:** `src/app/api/insights/targets/route.ts:199-216` (×8 call
sites: WEIGHT, BP_SYS, PULSE, SLEEP_DURATION, BMI-derived, BODY_FAT,
ACTIVITY_STEPS — and the BMI path also allocates by mapping the
weight series).
Per-request the route does ~7 linear walks over `recentMeasurements`
(up-to-30-day rows), each filtering by type then bucketing by
day-key. For the live tenant this is fine; for a power user with
high-frequency activity sources (steps every hour) it grows fast.
The single Postgres-side `GROUP BY date_trunc('day', measured_at),
type` would replace seven JS-side passes with one. Existing pattern
in `latestEverByType` (single grouped query with `distinct` already
in use) shows the route author knows the optimisation.
Not a hot-path today (the page is rarely refetched), but it's the
shape of an N+M perf bug if a future surface starts polling `/api/
insights/targets`.
### M3 — `targets` page mutates server response inside a render
(W4 C1)
**Where:** `src/app/targets/page.tsx:713-735`.
`visibleTargets` is computed inside the render body via
`data.targets.filter(...).map(...)`, which is fine — but the `map`
spreads each glucose target and assigns a localised `label`,
re-converted unit, and converted `current/average30/range` numbers.
This is rebuilt on every render. The data is stable across renders
inside one fetch cycle, so a `useMemo([data, t, displayGlucoseUnit])`
saves the work and — more importantly — keeps the per-card ref
stable so React.memo (or any future virtualisation) actually fires.
Sparkline path computation is also re-run per render but lives
inside the `Sparkline` component where `points.length` is the only
input, so memoisation there is cheap.
### M4 — `provenanceFromJson` validates manually rather than using
the existing Zod schema (W3)
**Where:** `src/lib/ai/coach/persistence.ts:78-140`.
The `keyValues` shape has a perfectly good Zod schema
(`coachKeyValueSchema`, defined in `types.ts`) which the runtime
parser already uses (`keyvalues.ts:83-89`). The persistence
deserializer hand-rolls the shape check (`typeof candidate.label !==
"string"` etc.) for legacy compatibility. v1.4.22 added the
`keyValues` branch in this hand-rolled style, doubling the surface
area where the contract can drift. A single
`coachKeyValueSchema.array().safeParse(parsed.keyValues)` would
replace 35 lines with 1 and keep the validation contract in one
file.
The `windows` and `metrics` arrays are validated as `typeof === "string"`
without checking they're members of the enum union — a stale
metric-name from a pre-v1.4.20 row will type-cast through and hit the
UI. Same `safeParse` lift fixes both.
### M5 — Sentinel parser's "malformed" semantics conflate three
distinct conditions (W3)
**Where:** `src/lib/ai/coach/keyvalues.ts:170-179`.
`malformed = malformedClose || truncated || kept.length === 0`.
This works for the route's single use (annotate one wide-event), but
the three branches mean very different things:
- missing `---END---` → provider drift or token truncation
- 1KB cap hit → adversarial input or runaway model
- zero valid rows → format drift in the parsing layer
Bundling them under a single `boolean` makes ops dashboards unable
to distinguish "model returned junk" from "we had to truncate".
Four-state enum (`"ok" | "truncated" | "unclosed" | "empty"`) costs
nothing and gives ops the signal they'll want when the prompt drifts.
Test surface (`keyvalues-parse.test.ts`, 18 cases) doesn't pin the
malformed branches separately so a future refactor can quietly merge
behaviours.
---
## LOW
### L1 — `Sparkline` SVG path uses `viewBox + preserveAspectRatio="none"`,
which is fine for a 100×24 trace but stretches y-axis disproportionally
to x-axis on wide containers (W4 C1)
`targets/page.tsx:286-288`. On a wide desktop card the sparkline gets
stretched horizontally — the visual rate-of-change is misleading. Tiny
CSS `aspect-ratio: 100/24` on the wrapper would lock the trace to the
intended ratio. Not a correctness bug, but a "this looks weird on a
1440px viewport" polish miss.
### L2 — `setOnboardingPendingCookie` is `async` but does no async
work (W4 C4)
`src/lib/auth/session.ts:24`. The function awaits `cookies()` then
synchronously sets/deletes. The `await` chain pulls every caller
into `async` for no work — minor noise but the helper could be
synchronous if `cookies()` is resolved by the caller and threaded
through. Probably not worth changing now (matches the
`createSession` pattern), worth flagging as a v1.5 cleanup.
### L3 — `InsightsSectionNav` re-creates an `IntersectionObserver`
on every mount; the section-id list is a module-scoped constant so
the observer is conceptually a singleton (W2 A5)
`src/app/insights/page.tsx:1729-1747`. Single mount per page so the
practical cost is negligible, but the cleanup-then-recreate pattern
on hydration is slightly wasteful. Acceptable as-is; calling out
because the section nav is a candidate for extraction to its own
file (it's 70+ LOC of logic at the bottom of a 1700-line page
component) and at extraction time the observer should move with it.
### L4 — `targets` API ships an `as TargetItem` cast on the BP entry
to silence the `details` mismatch (W4 C1)
`src/app/api/insights/targets/route.ts:281`. The cast hides that
the BP target ships extra (sys-only) data without typing it. The
type system is being lied to in exactly the spot where the diastolic
plumbing goes through. Not introduced in v1.4.22 but the C1 work
landed alongside it without addressing the smell.
---
## Tech-debt accrual
**Things v1.4.22 paid down:**
- The v1.4.19 A3 `stripChartTokens` widening missed
`recommendation-card.tsx:336`; W2 A6 closed that gap and added
the regression test. Genuine bug retired, not just "covered up".
- The CorrelationRow now drops insufficient cards instead of
rendering greyed placeholders (W2 A4) — this removes a
conditional-render tree that was harder to reason about than
the `filter().length` pattern that replaced it.
- `formatTokenName` regex broadened off `Z`-only ISO offsets
(W4 D / D-CR-M-03). One less surprising wart.
- Bilingual CHANGELOG normalisation (W4 D2) reduces the edit
surface every release.
**Things v1.4.22 added that need watching:**
- Hand-written validation in `provenanceFromJson` (M4) — second
copy of the keyValues shape. Now a 2-place change lurking.
- Cookie write fan-out across 6 auth routes (H1) — fragile by
convention.
- Three inline SSE-stream constructions in `route.ts` (H2) — at
3 they're a lurking duplicate, at 4 they're a refactor.
- Section-nav inline at the bottom of a 1700-line page file
(L3) — extraction is on the v1.4.23 implicit todo list.
**Status quo carried forward:**
- `analytics` route's unbounded BP findMany (M1) — known shape,
not introduced here, just promoted in user-visibility.
- `targets/route.ts` 7-pass sparkline computation (M2) — same.
- `as TargetItem` cast (L4) — predates v1.4.22.
Net: **slight increase** in subtle debt (hand-rolled validation,
cookie sync convention, SSE template duplication). All three are
30-LOC fixes — the marathon's polish-not-refactor scope explains the
choice but the items shouldn't carry past v1.4.23.
---
## Things to keep
These are the architecture wins of v1.4.22:
1. **Sentinel parser shape.** Text-marker (`---KEYVALUES---` /
`---END---`) over fenced JSON was the right call. The W1b research
suggested fenced JSON; the maintainer overrode to text markers in
the rewrite. The parser is composable (line-level
`parseKeyValueLine` is independently testable, the block-level
sentinel walk delegates to it), defensively bounded (1 KB +
8 lines), and graceful-degrades to a no-op on missing/malformed
blocks. `keyvalues.ts:107-180` is the cleanest module added in
the marathon.
2. **Proxy onboarding redirect with a non-httpOnly UX-hint cookie.**
Edge-runtime can't hit Prisma; the cookie pattern correctly
solves the constraint without smuggling DB state through the
wire. The "cookie is a UX hint, not a security signal" comment in
`session.ts:18-22` makes the threat model explicit. The
`/onboarding` self-loop guard (already in `PUBLIC_PATHS`) and the
`pathname.startsWith("/onboarding")` belt-and-suspenders check
are correct. The 7-test integration surface
(`proxy-onboarding-redirect.test.ts`) pins exactly the right
invariants.
3. **CorrelationRow row-fill via `filter` then conditional grid
class.** `okResults.length === 1 ? "grid-cols-1" :
"md:grid-cols-2"` is the simplest expression of "1 → full-width,
2-3 → 50/50". No layout primitive proliferation, no CSS grid
`auto-fit` magic, just a length check. Same pattern repeats
correctly in three places on the Insights page (BP-medication
grid, weight-correlation grid, medications-per-day grid) — the
in-page repetition is acceptable because each grid carries a
different "show second card" predicate (`showMoodSection` vs
`medications.length >= 2`).
4. **TrendCard third sub-value is opt-in via `avgAllTime !==
undefined`.** Other tiles leave the field undefined and the third
`<span>` simply doesn't render — additive, no regression risk for
the dozen existing call sites. The W2 A2 work is the textbook
shape of a backwards-compatible component evolution.
5. **`provenance.keyValues` extends the existing envelope.** The
W1b research's "Option A" (extend `CoachProvenance`) over
"Option B" (new `evidence` field) was the right call. One
decrypt boundary, one persisted blob, one DTO surface. The
collapsible disclosure renders from the same field the
in-flight stream emits — round-trip consistency with no
migration.
6. **Test architecture stayed proportional.** Wave 3 added 17 unit
tests + 2 integration tests for ~250 LOC of new code. The unit
tests pin the parser and the prompt's literal text where it's
load-bearing (PROMPT_VERSION, sentinel format); the integration
tests pin the round-trip (sentinel persists + decrypts + appears
in conversation reload) where the unit layer can't see it. No
over-mocked unit tests of route logic; no integration tests of
parser branches. The split tracks correctly with what each layer
can actually verify.
---
## Cross-link to product-lead-review
- The H1 cookie-sync is a product-architecture concern: any new
auth surface (SSO, magic link) needs the helper threaded in.
Worth adding to the v1.5 product-lead checklist for "things to
remember when adding an auth path".
- The H2 SSE-handler extraction is a v1.4.23 candidate worth
surfacing to product-lead before any new streaming endpoint
lands.
- W2 A5 "Zusammenhänge" rename is a UX call that already passed
through product-lead in W1a; flagging here only that the
decision is documented in the W2 report and matches the
research-backed naming heuristic from W1b.
- The Coach prompt rewrite (W3) is the single biggest user-facing
voice change in the release — product-lead-review should sample
3-5 live conversations against the new prompt and confirm the
warmth/restraint balance lands as the W1b research predicted.
.planning/phase-W5-v1422-simplify-review.md
+209
View File
@@ -0,0 +1,209 @@
# Wave 5 — Simplify review (v1.4.22)
Reviewed: develop @ 363e73c. Scope per the brief: W2 insights surface (`hero-strip.tsx`, `daily-briefing.tsx`, `recommendation-card.tsx`, `correlation-row.tsx`, `trends-row.tsx`, `<InsightsSectionNav>` in `src/app/insights/page.tsx`), W3 coach (`coach-panel/*`, `src/lib/ai/coach/system-prompt.ts`, `src/lib/ai/coach/keyvalues.ts`), W4 surfaces (`src/app/targets/page.tsx`, `src/components/admin/api-token-overview-section.tsx`, `src/proxy.ts`, the i18n+UI sweep diffs).
## Summary
Total findings: 5 apply-yes · 4 apply-maybe · 6 apply-no
## apply-yes (clear wins)
### S-01 — Coach composer duplicates the canSubmit guard 3×
**File**: `src/components/insights/coach-panel/coach-input.tsx:66-83`
**Why**: `!disabled && value.trim().length > 0` appears in both `handleKeyDown` and `handleFormSubmit`, then is computed again as `canSubmit` for the send-button `disabled` prop. The two handlers can call `canSubmit` directly — no behaviour change, fewer places to drift.
**Before** (≤8 lines):
```
const handleKeyDown = useCallback((event) => {
if (event.key !== "Enter") return;
if (event.shiftKey) return;
event.preventDefault();
if (!disabled && value.trim().length > 0) onSubmit();
}, [disabled, onSubmit, value]);
const handleFormSubmit = useCallback((event) => {
event.preventDefault();
if (!disabled && value.trim().length > 0) onSubmit();
}, [disabled, onSubmit, value]);
const canSubmit = !disabled && value.trim().length > 0;
```
**After** (≤8 lines):
```
const canSubmit = !disabled && value.trim().length > 0;
const handleKeyDown = useCallback((event) => {
if (event.key !== "Enter" || event.shiftKey) return;
event.preventDefault();
if (canSubmit) onSubmit();
}, [canSubmit, onSubmit]);
const handleFormSubmit = useCallback((event) => {
event.preventDefault();
if (canSubmit) onSubmit();
}, [canSubmit, onSubmit]);
```
### S-02 — Hero strip duplicates the entire weekly-report Button
**File**: `src/components/insights/hero-strip.tsx:241-267`
**Why**: The `weeklyReportHref ? <Button asChild …> : <Button disabled …>` ternary repeats `variant="default"`, `size="sm"`, the same `data-slot`, the same `className`, the same icon and label. Only `asChild` / `disabled` / `title` / wrapping `<Link>` differ. Collapse to one Button whose children are the icon+label, with the link wrapping conditionally.
**Before** (≤8 lines):
```
{weeklyReportHref ? (
<Button asChild variant="default" size="sm" data-slot="…-weekly-report" className="gap-1.5">
<Link href={weeklyReportHref}><FileText … /><span>{t(…)}</span></Link>
</Button>
) : (
<Button type="button" variant="default" size="sm" disabled title={comingSoon}
data-slot="…-weekly-report" className="gap-1.5">
<FileText … /><span>{t(…)}</span>
</Button>
)}
```
**After** (≤8 lines):
```
const weeklyLabel = (<><FileText className="h-3.5 w-3.5" aria-hidden="true" /><span>{t("insights.heroActionWeeklyReport")}</span></>);
{weeklyReportHref ? (
<Button asChild variant="default" size="sm" data-slot="insights-hero-strip-action-weekly-report" className="gap-1.5">
<Link href={weeklyReportHref}>{weeklyLabel}</Link>
</Button>
) : (
<Button variant="default" size="sm" disabled title={comingSoon}
data-slot="insights-hero-strip-action-weekly-report" className="gap-1.5">{weeklyLabel}</Button>
)}
```
### S-03 — API-token status badge repeated for desktop + mobile
**File**: `src/components/admin/api-token-overview-section.tsx:215-228, 281-299`
**Why**: The `revoked / isExpired / active` 3-way ternary appears twice with the same labels; the two copies only differ in `text-xs` vs `text-[10px]`. Extract a `<TokenStatusBadge size="sm" | "xs" />` helper. Removes ~20 lines and keeps the two surfaces in lock-step.
**Before** (≤8 lines):
```
{token.revoked ? (
<Badge variant="destructive" className="text-xs">{t("settings.tokenRevoked")}</Badge>
) : isExpired ? (
<Badge variant="destructive" className="text-xs">{t("settings.tokenExpired")}</Badge>
) : (
<Badge className="bg-dracula-green/15 text-dracula-green text-xs">{t("common.active")}</Badge>
)}
// …same block again with text-[10px] in the mobile list
```
**After** (≤8 lines):
```
function TokenStatusBadge({ token, size }: { token: ApiTokenInfo; size: "sm" | "xs" }) {
const { t } = useTranslations();
const expired = token.expiresAt && new Date(token.expiresAt) < new Date();
const cls = size === "xs" ? "text-[10px]" : "text-xs";
if (token.revoked) return <Badge variant="destructive" className={cls}>{t("settings.tokenRevoked")}</Badge>;
if (expired) return <Badge variant="destructive" className={cls}>{t("settings.tokenExpired")}</Badge>;
return <Badge className={`bg-dracula-green/15 text-dracula-green ${cls}`}>{t("common.active")}</Badge>;
}
```
### S-04 — TrendsRow inlines the same dynamic-load skeleton twice
**File**: `src/components/insights/trends-row.tsx:29-53`
**Why**: Both `dynamic()` calls use an identical `loading: () => <div className="bg-muted/40 h-[220px] w-full animate-pulse rounded-md motion-reduce:animate-none" />`. Hoist the skeleton to a constant. (No chart-visual change — purely the placeholder; cleared against `feedback_charts_visual_identity.md`.)
**Before** (≤8 lines):
```
const HealthChart = dynamic(() => import(…).then((mod) => ({ default: mod.HealthChart })),
{ ssr: false, loading: () => (<div className="bg-muted/40 h-[220px] w-full animate-pulse rounded-md motion-reduce:animate-none" />) });
const MoodChart = dynamic(() => import(…).then((mod) => ({ default: mod.MoodChart })),
{ ssr: false, loading: () => (<div className="bg-muted/40 h-[220px] w-full animate-pulse rounded-md motion-reduce:animate-none" />) });
```
**After** (≤8 lines):
```
const ChartSkeleton = () => (
<div className="bg-muted/40 h-[220px] w-full animate-pulse rounded-md motion-reduce:animate-none" />
);
const HealthChart = dynamic(() => import("@/components/charts/health-chart").then((m) => ({ default: m.HealthChart })),
{ ssr: false, loading: ChartSkeleton });
const MoodChart = dynamic(() => import("@/components/charts/mood-chart").then((m) => ({ default: m.MoodChart })),
{ ssr: false, loading: ChartSkeleton });
```
### S-05 — CorrelationRow grid className ternary duplicates the prefix
**File**: `src/components/insights/correlation-row.tsx:59-65`
**Why**: Both branches start with `"grid grid-cols-1 gap-4"`; only the `md:grid-cols-2` is conditional. `cn()` already imported by sibling components — use it here.
**Before** (≤8 lines):
```
<div
className={
okResults.length === 1
? "grid grid-cols-1 gap-4"
: "grid grid-cols-1 gap-4 md:grid-cols-2"
}
>
```
**After** (≤8 lines):
```
<div className={cn("grid grid-cols-1 gap-4", okResults.length > 1 && "md:grid-cols-2")}>
```
## apply-maybe (judgment calls)
### S-M1 — `daily-briefing.tsx` parallel TONE\_\* maps
**File**: `src/components/insights/daily-briefing.tsx:68-78`
**Status**: maybe. Could fold `TONE_BAR_CLASSNAME` + `TONE_TEXT_CLASSNAME` into one record-of-objects. But the current shape reads naturally and the two are looked up independently. Mild win at best; leave it unless adjacent tone work brings us back.
### S-M2 — `source-chips.tsx` builds a transient `chips` array then maps it
**File**: `src/components/insights/coach-panel/source-chips.tsx:55-72`
**Status**: maybe. Folds two `.map()`s into one. But the pre-built array gives `key={chip.key}` a stable handle and reads as "compute, then render", which the team's other chip strips follow. Skip unless we hit perf.
### S-M3 — `MessageThread` user-bubble avatar two-arm ternary
**File**: `src/components/insights/coach-panel/message-thread.tsx:194-211`
**Status**: maybe. The `gravatarUrl ? <img/> : <div/>` arms each carry the same `data-slot="coach-bubble-user-avatar"` and `mt-0.5 size-8 shrink-0 rounded-full`. A pre-computed `avatarNode` would shave ~4 lines. The eslint-disable comment for `<img>` is the only structural difference — small win, low priority.
### S-M4 — `proxy.ts` LEGACY_ADMIN_ANCHORS
**File**: `src/proxy.ts:64-78`
**Status**: maybe. 12 entries that 1:1 map `section-<slug>` → `<slug>`. Could collapse via a regex (`pathname.replace(/^\/admin\/section-/, "/admin/")` after a startsWith check) — but the explicit map is grep-able, paths like `umami` and `glitchtip` and `webpush` and `bugreport` all funnel to `/admin/integrations` so the mapping isn't a clean rename. Keep the explicit map.
## apply-no (rejected — explanation)
### S-N1 — `keyvalues.ts` parser
**Why**: Security-adjacent (input from LLM, sentinel parser). The brief excludes "sentinel parser, proxy.ts redirect" from simplification. The parser's defence-in-depth (1KB + 8-line + per-row Zod) is exactly the surface that should not get cleverer.
### S-N2 — `proxy.ts` redirect
**Why**: Excluded by the brief. The onboarding-cookie check + worker-mode 503 + LEGACY_REDIRECTS chain is the security path; reshaping it for a couple of lines is the wrong trade.
### S-N3 — `recommendation-card.tsx` `metricTypeToChartTypes` switch-ladder
**Why**: Reads as a table of metric synonyms with the deliberate "unknown → pass through verbatim" fallthrough commented in. Folding into a Map loses the comment-as-code. Keep verbatim.
### S-N4 — `targets/page.tsx` STATUS_CATEGORY_KEY
**Why**: 50-entry verbatim translation map. It IS the data; there's no helper to extract.
### S-N5 — `targets/page.tsx` Sparkline + TYPE_ICONS / TYPE_COLORS
**Why**: Newly-introduced sparkline (give the abstraction room per the brief). Parallel `TYPE_ICONS` / `TYPE_COLORS` reads cleanly and matches the rest of the app's icon-table convention.
### S-N6 — `system-prompt.ts` EN/DE blocks
**Why**: Test-fixture-equivalent prompt copy. The brief excludes test fixture data and "newly-introduced abstractions"; both apply. The two locale blocks intentionally diverge to read native in each language.
## Notes / push-back
- **`hero-strip.tsx` `resolveGreetingKey` JSDoc drift**: the JSDoc still names a 4-bucket schedule (morning / afternoon / evening / night) but the code merged night into evening per the comment below. Stale documentation — purely a doc fix, not a simplify finding.
- **`InsightsSectionNav` IntersectionObserver `observerRef`**: held in a `useRef` but only used inside the effect that creates it. A local `const observer = new IntersectionObserver(…)` plus `observer.disconnect()` in the cleanup would drop the ref entirely. Tiny readability win — call it apply-maybe-S-M5 if the lift gets re-touched.
- **No tests in scope tested the framework.** The `targets-sparkline.test.tsx` cases pin actual rendered output (data-slot presence, signed delta, locale string) — keep.
- **The W2 BP-tile "synthesised slope" `slope = (pct7 - pct30) / 30` (per A2) sits in `src/app/page.tsx`, outside the scope-listed files for W5. Skipped.**
.planning/phase-W6-v1423-code-review.md
+115
View File
@@ -0,0 +1,115 @@
# Wave 6 — Code review (v1.4.23)
## Summary
- **Files reviewed:** 38 production files across the v1.4.23 diff (29 commits, ~15.7k LOC delta) plus 4 phase reports, 5 migrations, both i18n bundles, and the regenerated OpenAPI spec.
- **Findings:** 0 CRITICAL · 2 HIGH · 6 MED · 6 LOW
- **Overall quality:** Strong. The four-wave plan landed end-to-end with production-grade code, comprehensive tests (2191 → 2223 unit, +8 integration), and an honest paper trail of trade-offs. Migrations are strictly additive, `apiHandler()` + `withIdempotency()` wraps are consistent, the Apple Health mapping table is exhaustive, the refresh-token per-device blast-radius is well-tested with the legacy null-deviceId fallback, and the sentinel-parser observability lands without leaking partial-bad-data into the rendered prose. The two HIGH findings are integration gaps rather than bugs in landed code: APNs has no production code path that creates the channel row it needs, and `POST /api/devices` has a same-user apnsToken-collision path that silently fans out to two device rows. The `feedback_charts_visual_identity.md` rule is honoured (no Recharts touch), the `feedback_marc_voice_english.md` rule passes scan (no AI/Claude/agent/marathon mentions outside `.planning/`), the `feedback_no_pii_in_user_facing.md` rule passes for the new GROUND RULE 12 (categories only, no figures/emails), the `feedback_react_query_key_collision.md` rule passes (new keys `["coach-prefs"]`, `["admin","coach-feedback"]`, `["admin","ai-quality"]` are unique), and the `feedback_cache_invalidate_on_new.md` rule passes (the settings sheet writes `setQueryData(["coach-prefs"], …)` so the next Coach turn picks up the change).
## CRITICAL
_None._
## HIGH
### HIGH-1 — APNs cascade is unreachable in production
- **Where:** `src/lib/notifications/dispatcher.ts:41-49` (channel lookup) + `src/app/api/devices/route.ts` (no APNS-channel creation) + the rest of the v1.4.23 W3 wiring.
- **What:** `dispatchNotification()` reads enabled `NotificationChannel` rows for the user and only enters the `case "APNS"` branch when one of those rows has `type === "APNS"`. Nothing in v1.4.23 (or earlier) creates a `NotificationChannel` row of type APNS. The integration test (`tests/integration/apns-dispatch.test.ts:110-115`) hand-creates the row with `prisma.notificationChannel.create({ type: "APNS" })`; production users who register an iOS device via `POST /api/devices` (apnsToken + apnsEnvironment land on the `Device` row) will never see APNs fire because the dispatcher never enters that branch.
- **Why:** Six commits of W3 scaffolding are dead code in production until either: (a) `POST /api/devices` upserts a matching `NotificationChannel` row when an apnsToken arrives, (b) the user is given a `/settings/notifications` toggle to opt in, or (c) a one-shot migration backfills the row for every existing iOS device. The v1.4.23 phase-W3 report calls APNs "shipped" but the cascade is unreachable through normal user flow.
- **Fix:** In `POST /api/devices`, after the cross-user-hijack guard passes and the device is upserted, also `prisma.notificationChannel.upsert({ where: { userId_type: { userId: user.id, type: "APNS" } }, create: { userId: user.id, type: "APNS", enabled: true, config: encrypt("{}") }, update: {} })`. Mirrors the existing Telegram-on-first-dispatch auto-migration (`dispatcher.ts:54-105`).
### HIGH-2 — Same-user apnsToken collision creates fan-out duplicate
- **Where:** `src/app/api/devices/route.ts:90-108` + `prisma/schema.prisma:822-836`.
- **What:** The cross-user-hijack guard for `apnsToken` filters with `NOT: { userId: user.id }`, so it only catches _other_ users' tokens. The same user can register a brand-new legacy `token` while supplying an `apnsToken` already attached to another of their own devices (e.g. they reinstalled iOS, the device id rotated, the apnsToken survived). Today the schema only has `@@index([apnsToken])` (no `@@unique`), so two of that user's `Device` rows now share the same `apnsToken`. `sendViaApns()` then iterates `findMany({ where: { userId, apnsToken: { not: null } } })`, fans out two pushes to the same physical device, and double-charges Apple's quota for one notification.
- **Why:** Every notification reaches the user's iPhone twice. Quota is finite; APNs throttles aggressively at scale.
- **Fix:** Either (a) make the apnsToken column globally unique (`@unique` on the schema, partial index over `apnsToken IS NOT NULL` in SQL), or (b) drop the `NOT: { userId }` clause from the hijack guard so any pre-existing apnsToken lookup also catches the same user, or (c) in the upsert path, when the apnsToken differs from the existing row's `apnsToken`, also `update` any other Device rows of the same user where `apnsToken` matches the new value to set them NULL. Option (a) is the cleanest — APNs tokens are device-scoped by Apple's contract.
## MED
### MED-1 — `loadApnsConfig()` cache pins a stale read across env changes in production
- **Where:** `src/lib/notifications/senders/apns.ts:100-167`.
- **What:** `cachedConfig` is module-scope and only reset by `resetApnsForTesting()`. Coolify-style env redeploys spawn fresh processes so the cache resets naturally, but if the operator rotates the APNs `.p8` key without restarting the worker (e.g. Coolify variable update + soft redeploy) the new key never lands. The `getProvider()` cache also persists.
- **Why:** Stale-key push failures look like Apple-side issues. The fix is either documenting the restart requirement loudly or having `loadApnsConfig()` re-read after a configurable TTL.
- **Fix:** Add a 60s/5min env-poll TTL on `cachedConfig`, OR document in `.env.example` + the W3 report that APNS\_\* changes require a worker restart.
### MED-2 — `POST /api/measurements/batch` reconciliation OR-clause re-queries the entire `toInsert` set
- **Where:** `src/app/api/measurements/batch/route.ts:226-261`.
- **What:** When `skipDuplicates: true` racey-absorbs rows (a duplicate batch lands in the same tick), the recheck path issues a second `findMany({ OR: toInsert.map(...) })` covering up to 500 entries. With 500 entries the OR clause has 1000 SQL parameters per side. This is fine under Postgres' 65k cap but the fall-back is allocated unconditionally — even the 99% no-race path pays the planning overhead because the early-return `if (racedDuplicates > 0)` only short-circuits the query, not the array allocation.
- **Why:** Performance smell rather than a bug. Race path is rare; bound is generous. Worth flagging because the iOS sync cursor exercise will trigger it under flaky-network retry.
- **Fix:** No action required for v1.4.23. v1.4.24 candidate: move the `OR` clause builder behind the `if (racedDuplicates > 0)` guard, and switch the recheck to fetch only the ids of rows that should have been inserted but came back with `skipDuplicates`'s implicit drop.
### MED-3 — SLEEP_DURATION unit silently changed from hours to minutes
- **Where:** `src/lib/validations/measurement.ts:53-57`, `prisma/migrations/0036_apple_health_measurement_types/migration.sql:12-17`.
- **What:** Migration 0036 says "advisory, no row mutation: SLEEP_DURATION shifts from hours to minutes... no production data exists for this enum value yet." The application `getUnitForType()` now reports `"minutes"` for this enum value. If any user manually entered a sleep value before v1.4.23 (the W1 research excluded this case but didn't enforce it), their pre-existing `7.5` (hours) row reads as `7.5 minutes` after the upgrade. Charts and analytics would then show degenerate numbers.
- **Why:** Silent data-shape change. The v1.4.20 weekly-report and Doctor-PDF surfaces would also misrepresent the unit on pre-v1.4.23 rows.
- **Fix:** Either (a) add a one-shot data migration that rewrites pre-v1.4.23 SLEEP_DURATION rows multiplying by 60 (gated on a `created_at < migration_date AND value < 24` heuristic), or (b) document the change loudly in CHANGELOG.md and add a runtime guard in `validateMeasurementRange()` that flags impossibly-low values (< 30 min for a sleep entry) so the anomaly catches stale rows.
### MED-4 — Coach `MessageThread` triggers `/api/auth/me/coach-prefs` GET on every drawer mount, regardless of settings interaction
- **Where:** `src/components/insights/coach-panel/message-thread.tsx:72-80`.
- **What:** The `useQuery({ queryKey: ["coach-prefs"] })` runs unconditionally (no `enabled` flag) so opening the Coach drawer always fires a GET even when the user never opens the settings sheet. The settings sheet's identical query at `coach-settings-sheet.tsx:71-80` is gated on `enabled: open`, so the two queries share the same key but trigger from different surfaces. TanStack Query dedups concurrent fetches so the practical cost is one GET per drawer mount + one GET per settings open (not two), but the always-on read happens even on accounts that never set a non-default preference.
- **Why:** Tiny perf nit. The defaults-only path could just read from the queryClient cache (or a no-fetch initialData) instead of pinging the API every drawer open.
- **Fix:** Add `staleTime: 5 * 60 * 1000` (or higher) to the `MessageThread`'s query so a single fetch per session covers both surfaces, and rely on the settings sheet's `setQueryData` to invalidate after a save (already in place at `coach-settings-sheet.tsx:107`).
### MED-5 — `lastFailureReason` in APNs sender captures the LAST device's reason regardless of whether it was permanent
- **Where:** `src/lib/notifications/senders/apns.ts:341-396`.
- **What:** The fan-out loop assigns `lastFailureReason = result.reason` on every failure, including transient ones, then surfaces that as the channel-state machine's `reason` string. With 3 devices where device A and B return `Unregistered` (deadDeviceId) and device C returns a transient 500, the dispatcher records the channel failure as `apns_unknown_failure` (or whatever C's reason was) instead of the `Unregistered` signal that drives the actual cleanup.
- **Why:** Misleading observability. The audit/wide-event tracker for "why did the channel get auto-disabled" reads the wrong reason.
- **Fix:** Track `lastFailureReason` separately for permanent vs transient outcomes; surface the permanent reason when `deadDeviceIds.length === devices.length`.
### MED-6 — `feedbackHelpful` and `feedbackThanks` keys exist at two i18n paths under different sibling sections
- **Where:** `messages/en.json:736 + 923` and `messages/de.json:736 + 923`.
- **What:** `insights.recommendation.feedbackHelpful` ("Helpful" / "Hilfreich") AND `insights.coach.feedbackHelpful` ("Helpful" / "Hilfreich") are duplicate keys in two different sections. Same for `feedbackThanks` ("Thanks for your feedback" / "Thanks for the signal."). The two siblings are distinct paths so the i18n locale-integrity test passes, but the visible copy is intentionally different ("Not helpful" vs "Not quite", "Thanks for your feedback" vs "Thanks for the signal.") despite serving identical UX semantics.
- **Why:** Style / consistency drift between Insights-recommendation feedback and Coach-message feedback. Either both should read "Not helpful" (clinical) or both should read "Not quite" (warm). Today they disagree.
- **Fix:** Pick one tone, replace the duplicates with a single `common.feedbackHelpful`/`common.feedbackUnhelpful`/`common.feedbackThanks` triplet, and re-import from both call sites.
## LOW
### LOW-1 — Two admin queries fetch the same `/api/admin/ai-quality` endpoint with different keys
- **Where:** `src/components/admin/ai-quality-section.tsx:65-72` (`["admin","ai-quality"]`) and `src/components/admin/coach-feedback-section.tsx:57-65` (`["admin","coach-feedback"]`).
- **What:** Both views read from the same endpoint but use distinct query keys. The two pages are never visible at the same time today (different `/admin/<slug>` routes), so the practical cost is one extra fetch per navigation between the two pages. Acceptable; calling it out for future cleanup.
- **Fix:** Optional — share a single `["admin","ai-quality-summary"]` key and let each section pluck its slice via a `select` callback.
### LOW-2 — `useResettableValue<T>` exported but the matching `nextResettableValue<T>` is exported separately
- **Where:** `src/components/insights/coach-panel/coach-drawer.tsx:80-113`.
- **What:** The hook exists for the H3 fix and the pure decision function is also exported, both for testing. The duplication is intentional (per the JSDoc) but it surfaces both as part of the public module. A future caller might import `nextResettableValue` instead of using the hook and get inconsistent behaviour.
- **Fix:** Mark `nextResettableValue` as `@internal` (or move it to a sibling test-helper file) so consumers can't import it accidentally.
### LOW-3 — Coach feedback admin section colour buckets ignore `dracula-yellow` token at-rest theme
- **Where:** `src/components/admin/coach-feedback-section.tsx:47-51`.
- **What:** The `helpfulRateColour` helper picks `text-dracula-yellow` for the 50-79% band. The Dracula palette ships `--dracula-yellow` but the admin sections elsewhere prefer `text-dracula-orange` for caution. Worth a one-liner note when the v1.4.24 design pass touches admin.
- **Fix:** Cosmetic; defer to design pass.
### LOW-4 — `parseChartTokens()` allowlist extends to 9 Apple Health metric tokens but no chart components ship for them
- **Where:** `src/lib/insights/chart-tokens.ts:36-46`.
- **What:** The allowlist accepts `metric:HEART_RATE_VARIABILITY` etc., but the comment at the top calls out that the chart components ship in v1.5. If the model emits one of these tokens before v1.5 lands, the prose is stripped of the token (correct) but the chart never renders. The user gets a sentence with a missing visual.
- **Fix:** Either narrow the allowlist back to the v1.4.23 surface OR add a chart component that renders an empty placeholder until v1.5.
### LOW-5 — `apnsEnvironment` falls back to `sandbox` for any value that isn't literally `"production"`
- **Where:** `src/lib/notifications/senders/apns.ts:348-349`.
- **What:** `device.apnsEnvironment === "production" ? "production" : "sandbox"` — any future enum addition (e.g. `"voip"`, `"alpha"`) would be silently routed through the sandbox gateway. Today the schema enforces only `sandbox|production` via Zod at the API boundary, but the DB column is a free-form `TEXT`. A future `apns_environment` value would silently break.
- **Fix:** Convert the `apnsEnvironment` column to a Postgres enum (or a CHECK constraint) so a write-side typo can't paint sandbox-routed rows.
### LOW-6 — Coach prompt prefs loaded with separate Prisma query inside `chat/route.ts` AND inside `buildCoachSnapshot()`
- **Where:** `src/app/api/insights/chat/route.ts:209-213` and `src/lib/ai/coach/snapshot.ts:263-267`.
- **What:** Two `prisma.user.findUnique({ select: { coachPrefsJson } })` calls per Coach turn — the route builds the system prompt with one read, the snapshot builder reads the same column for the exclude-metrics filter. Duplicated round trip; both values are identical at the same instant.
- **Fix:** Lift the `parseCoachPrefs` lookup into the route, pass the result into `buildCoachSnapshot(userId, scope, prefs)`. The snapshot helper already accepts a scope; an optional prefs arg keeps the API additive.
## Praise
- **Honest open-questions sections** — the W2/W3/W4 reports each carry a numbered iOS-DTO contract checklist (~13 questions consolidated in W4) that the iOS maintainer can walk through line-by-line. Rare to see this much explicit handoff documentation.
- **Strict-additive migration discipline** — five migrations (0036 → 0040) all use `IF NOT EXISTS`, every new column nullable, no enum reordering, `CHECK` constraints scope the new sleep_stage column to its enum value (mirroring the v1.4.20 glucose_context pattern). Zero risk of a deploy-time data loss.
- **Sentinel parser H1 split** — `coach.keyvalues.parse_partial` vs `coach.keyvalues.parse_failed` is exactly the right granularity for ops attribution, and the per-line `reasons` array (typed enum, no raw text) keeps Coach prose out of the wide-event log without losing the diagnostic signal.
.planning/phase-W6-v1423-design-review.md
+135
View File
@@ -0,0 +1,135 @@
# Wave 6 — Design / UX review (v1.4.23)
## Summary
- Surfaces audited: Apple Health source badge in `<MeasurementList>` (`src/components/measurements/measurement-list.tsx`), Coach drawer settings cog + `<CoachSettingsSheet>` (`src/components/insights/coach-panel/coach-drawer.tsx` + `coach-settings-sheet.tsx`), per-message thumbs feedback (`message-thread.tsx` H7), admin coach-feedback section (`src/components/admin/coach-feedback-section.tsx` + `/admin/[section]` renderer + `section-slugs.ts` + `admin-shell.tsx` nav).
- Out of scope (no UI shipped): `/api/auth/me/devices` is API-only — no settings/admin surface yet (flag as v1.4.24 candidate); sleep-stages analytics endpoint added but no chart / panel — backend-only as W2 plan stated.
- Branch: `develop`. Compared 36 v1.4.23 commits since the v1.4.22 tag (`d71e879`).
- Findings: **1 CRITICAL · 4 HIGH · 6 MED · 5 LOW**
---
## CRITICAL
### C1 — Admin "Coach Feedback" section is unreachable from any nav surface
- Surface: `src/components/admin/admin-shell.tsx:51-110`, `src/components/admin/section-slugs.ts:18`, `src/app/admin/[section]/renderer.tsx:97-105`
- What: `coach-feedback` is registered in `ADMIN_SECTION_SLUGS` (slug exists, page renders, i18n keys present), but the slug was **never added to the `ADMIN_SECTIONS` nav array** in `admin-shell.tsx`. Result: the page only loads if an admin types `/admin/coach-feedback` directly — nothing in the sidebar or mobile strip surfaces it. The whole point of W5 H7 (let the operator answer "is the new prompt landing well?") is broken at the discovery layer.
- Why: `ADMIN_SECTIONS` is hand-maintained in parallel with `ADMIN_SECTION_SLUGS`. The slug list got the entry, the nav array did not. There's no compile-time guard that the two stay in sync (the renderer's exhaustive switch covers `coach-feedback`, but the sidebar doesn't loop over the slug list — it loops over its own array).
- Fix: (1) Add a `{ slug: "coach-feedback", titleKey: "admin.section.coach-feedback.title", icon: Sparkles }` entry between `ai-quality` and `feedback` so the two AI-quality views sit next to each other. (2) Land a tiny test in `src/components/admin/__tests__/admin-shell.test.tsx` that asserts every entry in `ADMIN_SECTION_SLUGS` has a matching `ADMIN_SECTIONS` entry — this drift class will recur every time a new section is added.
---
## HIGH
### H1 — Apple Health source badge is invisible on mobile (the only place the iOS user looks)
- Surface: `src/components/measurements/measurement-list.tsx:446-455` (desktop branch only), `:478-535` (mobile branch — no source render)
- What: The new pink Apple-Health chip only paints inside the **desktop** `<Table>` cell. The mobile card (`md:hidden`, lines 478-535) renders the type badge + value + timestamp + truncated note, but **no source indicator at all** — no chip, no icon, no text. So the audience that actually consumes Apple Health data (the user who opened HealthLog on their phone after a Withings/Apple Health sync) never sees that badge. Worst case: they look at the measurement list on mobile, can't tell which row was hand-logged vs synced, and re-enter a duplicate.
- Why: The mobile card layout was built before W5 (when only Withings + Manual + Import existed and the source distinction wasn't urgent). The v1.4.23 W5 patch added `formatMeasurementSource` and `sourceBadgeClass` but only wired them into the desktop branch.
- Fix: Add a tiny pink chip to the mobile card too — sit it inline with the timestamp line, e.g. `<span className="text-dracula-pink text-[10px]">Apple Health</span>` after the date. Or render a 12-px pink dot next to the type icon for `APPLE_HEALTH` only. Don't reuse the full `<Badge>` — the mobile card is already dense.
### H2 — Per-message thumbs hit-target is below the 36 px floor (M-CRIT for finger-on-glass)
- Surface: `src/components/insights/coach-panel/message-thread.tsx:457-483`
- What: Each thumbs button is `inline-flex items-center gap-1 rounded px-1.5 py-0.5 text-[11px]`. With `text-[11px]` (~14 px line-height) + `py-0.5` (2 px each side) the rendered button height is ~18 px. The `<ThumbsUp className="size-3" />` icon is 12 px; the visible label "Helpful" / "Not quite" is the rest. **The whole control is roughly 18 px tall and 60-72 px wide.** That's half the iOS HIG / Material `≥36 px` minimum touch target and four times worse than the 44 px Apple recommends. On a phone you have to tap precisely or you miss; on a desktop trackpad it's fine but reads as "aside" rather than "click me". The same review (W5 M1) flagged the evidence summary for this — same footnote-density text-class problem.
- Fix: Wrap each button in a `min-h-9` (36 px) container, OR bump the button itself: `px-2 py-1.5 text-xs` + `size-3.5` icons. The visual still reads quiet because the colour stays muted-foreground until hover; just give the finger something to land on.
### H3 — `<CoachSettingsSheet>` has no loading skeleton — first open is a blank scroll-area for 200-600 ms
- Surface: `src/components/insights/coach-panel/coach-settings-sheet.tsx:64-135`
- What: The sheet mounts, kicks off `useQuery(["coach-prefs"], ...)` with `enabled: open`, and renders the form against `DEFAULT_COACH_PREFS` until `persisted` arrives. The fetch is gated to `enabled: open` (good — no overhead until the cog click), but the form **renders immediately** with the default values. So on first open the user sees: "Tone: Warm, Verbosity: Default, no excludes, evidence: off" — even if their saved value is "Concise / Detailed / sleep+steps excluded / evidence on". When the fetch resolves ~200-600 ms later, the controls visibly snap to the persisted values. That's a "ghost form" UX — the user might think they're seeing their saved settings before the snap, click Save, and overwrite their excludes back to the defaults if they're fast.
- Why: The render-phase setState pattern (lines 87-94) syncs the draft to persisted **only after** the query resolves; before that, `draft === DEFAULT_COACH_PREFS`. There's no `isFetching` skeleton.
- Fix: Either (a) gate the form body behind `if (!persisted) return <SheetSkeleton />` so the user never sees the wrong values, OR (b) disable the Save button until `persisted !== null` and add a top-of-sheet inline `<Loader2>` so the user knows the displayed values are the defaults, not their saved state. Option (a) is the cleaner mobile UX.
### H4 — Coach settings save closes the sheet without confirmation, so the user never sees "Saved"
- Surface: `src/components/insights/coach-panel/coach-settings-sheet.tsx:96-110`
- What: `save.mutate` succeeds → `onOpenChange(false)` immediately. There's no toast, no inline "Saved." moment, and the sheet just disappears. The `insights.coach.settingsSaved` key is defined ("Saved." / "Gespeichert.") but never rendered. The user clicks "Save", the sheet vanishes, and they're back in the message thread with no confirmation that anything happened. If they hit Save by accident they have no idea what just got persisted.
- Why: The `onSuccess` flow optimised for "get out of the way", but UX-wise that reads as the action being silent / dropped — especially on a slow connection where the click-to-close gap is ~400 ms and the user is unsure whether the click registered.
- Fix: Either (a) keep the sheet open + flip the Save button to a green "Saved." pill for ~1.2 s before auto-closing, OR (b) emit a short `useToast({ description: t("insights.coach.settingsSaved") })` toast on success and let the sheet close immediately. The toast pattern matches the rest of HealthLog's settings flows (notifications, integrations).
---
## MED
### M1 — Coach prefs sheet has no error state when save fails
- Surface: `src/components/insights/coach-panel/coach-settings-sheet.tsx:96-110`, `:271-296`
- What: `save.isError` is never read. If the PUT 4xx/5xxs (validation, network, rate-limit), the sheet stays open with no banner — the user clicks Save again, gets the same silent failure, and eventually closes the sheet thinking they saved. The `coachPrefs` Zod schema rejects invalid `excludeMetrics` shapes and the route can return 422; that error never reaches the user.
- Fix: Add `{save.isError && <p role="alert" className="text-destructive text-xs">{t("insights.coach.settingsError")}</p>}` above the footer button row, wire two new translation keys.
### M2 — Per-message thumbs feedback has no streaming-state guard at the UI level (relies on `messageId == null`)
- Surface: `src/components/insights/coach-panel/message-thread.tsx:401-410`
- What: The thumbs render is gated on `messageId && !inProgress && !errorCode && providerType !== "refusal"`. The `messageId` check works for _streaming_ bubbles (the in-flight bubble has no id until the SSE `done` event resolves the persisted twin), but the 150 ms grace-window logic (the `suppressedTwinId` from W5 reconcile) means the **persisted twin paints with an `id` set** — so if a user is fast enough they could thumbs-rate a message that's still mid-stream visually because the persisted bubble landed. Low likelihood, but the rating would be on a message the user can't yet read in full.
- Fix: Add a `streaming?.messageId === messageId && streaming?.inProgress` early-return in `CoachMessageFeedback`, OR pass a `streamingActive` prop down so the thumbs row only mounts after the streaming state fully clears.
### M3 — Admin Coach-Feedback table: `tone` and `verbosity` cells render the raw enum string
- Surface: `src/components/admin/coach-feedback-section.tsx:166-167`
- What: `<td>{bucket.tone}</td>` / `<td>{bucket.verbosity}</td>` print the literal API enum (`warm` / `neutral` / `concise`, `brief` / `default` / `detailed`) — same anti-pattern v1.4.20 M4 + W5 M3 flagged for severity badges (uppercase enum → translated label). The user-facing settings sheet _does_ translate these correctly via `insights.coach.settingsToneWarm` etc.; the admin table doesn't reuse those keys. German operators see "warm" / "concise" rather than "Warm" / "Knapp", and the column cells become an inconsistency on the page (column headers translated, cells English-only enum).
- Fix: Build a small lookup `t(\`insights.coach.settingsTone${capitalize(bucket.tone)}\`)`. Or (cleaner) add a dedicated `admin.coachFeedback.tone.warm` set so the admin context owns its labels.
### M4 — Admin Coach-Feedback table has no severity / colour bands on `helpfulRate` for n<10
- Surface: `src/components/admin/coach-feedback-section.tsx:47-51`, `:155-181`
- What: `helpfulRateColour` returns red / amber / green based purely on the rate, ignoring sample size. A bucket with 1 helpful + 0 unhelpful (n=1, rate=100%) renders bright `text-dracula-green`, looking like a strong positive signal — but it's noise. Similarly 0/3 paints `text-dracula-orange` like a real warning. The W5 H7 plan called out that the operator's first useful question is "did the new prompt-version land well", which needs the n column to be **the** primary signal, not the rate colour.
- Fix: Either (a) only colour-band when `total >= 20` (mirrors the v1.4.23 Pearson-surfacing gate at `n>=20` from commit `1faee95`), OR (b) render a faint `text-muted-foreground` rate when `total < 10` and switch to the colour-band only above that threshold. Bonus: italicise or mark `n<10` rows with a "low confidence" sub-row so the operator's eye skips past them.
### M5 — Admin Coach-Feedback has no sort, no filter, no time-window picker
- Surface: `src/components/admin/coach-feedback-section.tsx:127-185`
- What: The table is a flat 7-column list of every (promptVersion × tone × verbosity) bucket the aggregator emits. With 3 prompt versions × 3 tones × 3 verbosities = up to 27 rows; with version sprawl over time that becomes 50+. There's no sort (descending-by-`total` would be the natural default — show the most-rated combinations first); no filter (operator can't isolate "show me only the new prompt-version"); no time-window picker (window is hard-coded to whatever the aggregator emits). For the v1.4.23 question of "did the warm tone overshoot?" the operator has to eyeball-scan a 27-row table.
- Fix: Phase-2 punt is fine, but document it. v1.4.24 should add (a) sort by total / helpful-rate, (b) prompt-version filter chips at the top of the section, (c) re-use the AI-Quality section's window picker if it has one.
### M6 — Coach drawer header now stacks 4 controls in a row that's tight at 375px
- Surface: `src/components/insights/coach-panel/coach-drawer.tsx:239-281`
- What: Header row contains: avatar (32 px) + title-block (flex-1, min-w-0) + "+ New chat" button (text label hides on `<sm`) + 32-px settings cog + Sheet close-X (~32 px). On a 375 px viewport with the drawer at full width, the available row width is ~327 px (375 - 48 padding). Avatar + cog + close-X = 96 px of fixed widget; the title-block gets ~230 px and the new-chat icon-only takes 36 px. That works for a typical conversation title ("Mein Blutdruckverlauf") but truncates aggressively for the German default ("Neue Unterhaltung") + a long auto-generated title. The settings cog at `size-8` (32 px) with a `size-3.5` icon (14 px) is the correct hit target, but the header reads visually crowded.
- Fix: Either (a) push the settings cog into a kebab `<DropdownMenu>` on `<sm` (one trigger, two items: "New chat" + "Settings"), OR (b) drop the avatar on `<sm` since the gradient-sparkles is just decoration there — the title carries the meaning.
---
## LOW
### L1 — `feedbackUnhelpful` copy: "Not quite" / "Nicht ganz" reads soft to the point of sounding evasive
- `messages/en.json:924`, `messages/de.json:924`. The DE/EN pair is consistent, but both strings dodge the "this wasn't helpful" affordance. Compare to the EN/DE "Helpful" / "Hilfreich" which lands directly. Consider "Not helpful" / "Nicht hilfreich" — symmetric, direct, and matches the admin-table column header "Not helpful" / "Nicht hilfreich" (already translated cleanly there). Mismatched user-facing vs admin-side label is a footgun for the prompt-tuning loop ("the user clicks 'not quite', the dashboard says 'not helpful', operator wonders if these are the same signal").
### L2 — `coach-prefs-evidence` Switch + Label flex-row layout breaks alignment when the hint wraps to 3 lines
- `src/components/insights/coach-panel/coach-settings-sheet.tsx:245-268`. The `flex items-start justify-between` puts the Switch flush-right against the label column. On a 375 px sheet the German hint ("Die Belege-Box unter jeder Antwort automatisch öffnen.") wraps to 2 lines, sometimes 3 with Reading-Mode font scaling — the Switch sits at the top of the column and the label visually disconnects from its control. Consider `items-center` (the Switch tracks the label's vertical centre) or move the Switch to a row-below position when the hint exceeds one line.
### L3 — Coach prefs `<Sheet>` description prose includes a casual second-person colloquialism in DE
- `messages/de.json:907`: "Stell ein, wie der Coach mit dir spricht." reads conversational ("Stell ein" is informal imperative). The rest of the settings UI uses indicative prose ("Die Belege-Box ... öffnen.", "Der Coach sieht keine Daten ..."). Consider "So spricht der Coach mit dir." (descriptive) or "Hier kannst du anpassen, wie der Coach mit dir spricht." (indicative). The Marc voice is professional / English-default; the imperative DE here breaks that.
### L4 — Admin Coach-Feedback section uses raw `<table>` not the shadcn `<Table>` primitive
- `src/components/admin/coach-feedback-section.tsx:127-185`. Every other admin table (`<Table>` from `@/components/ui/table`) carries the same row-hover, border, and cell-padding tokens. This section reaches for raw `<table className="w-full text-sm">` and re-implements the styling inline (`border-b border-border/40`, etc.). Not broken, but inconsistent — and a maintenance trap when the next shadcn token sweep updates the `<Table>` primitive but skips this hand-rolled one.
- Fix: Swap to `<Table>` / `<TableHeader>` / `<TableRow>` / `<TableHead>` / `<TableCell>` so the section inherits the rest of the admin UI's table token bleed.
### L5 — Apple Health source-badge contrast: `bg-dracula-pink/15 text-dracula-pink` on the dracula `--background` is borderline
- `src/components/measurements/measurement-list.tsx:131-134`. `--dracula-pink: #ff79c6` on `--background` (Dracula dark, ~#282a36) hits 5.7:1 contrast — passes WCAG AA for normal text but the chip is `text-xs` (12 px), and the `bg-dracula-pink/15` overlay further reduces effective contrast against the surrounding card. Test in Chrome's "vision deficiencies" simulator (deuteranopia + tritanopia) — the pink reads as dim grey-pink. Consider `text-dracula-pink` paired with `bg-transparent border border-dracula-pink/40` so the colour is on the chip border + label, not the fill — better contrast at small text sizes and matches the lighter weight of the desktop table cell.
---
## Things done particularly well
1. **Coach prefs sheet schema discipline.** Tone, verbosity, excludeMetrics, showEvidenceByDefault — all four match the W5 H4 spec exactly, and the sheet body is a single 4-control vertical scroll with no nested tabs / accordions / sub-pages. Reads as a calm "twiddle four knobs and save" surface, exactly the right shape for a feature 80 % of users won't open. The `data-slot` discipline (every interactive element has a stable `coach-prefs-*` slot for E2E targeting) is the kind of test-affordance that scales — Playwright never has to selector-guess.
2. **Disclaimer redundancy at message-thread bottom is the right safety call.** The W5 reconcile fix to pin the disclaimer at the message-thread bottom (`message-thread.tsx:211-225`) closes the v1.4.22 H3 medical-safety regression cleanly — every Coach session carries the disclaimer regardless of viewport now, and the desktop sources-rail copy stays as a secondary anchor. The "redundancy is intentional for clinical-adjacent UI" comment is the kind of forward-defence the next contributor will thank you for.
3. **Apple Health pink chip's brand token choice.** Picking `dracula-pink` (the closest Dracula token to Apple's iOS health red) for the badge is the right cross-brand grace note — the chip reads "Apple Health" without breaking the Dracula palette discipline. Just need to extend the chip to mobile (H1) for the win to land.
---
## Design decisions worth pushing back on
1. **Settings cog comes back as a Sheet — but the v1.4.22 reflection said "consider whether the right replacement is _not_ a cog: per-message 'thinking style' controls or a one-time onboarding".** v1.4.23 H4 reflexively re-added the cog the v1.4.22 B5 audit removed. The Sheet-with-4-controls is well-built, but it's a **global preference panel that 95 % of users never open** — exactly the shape v1.4.22's reflection warned against. Per-message tone toggles ("Try this in 'Concise' mode" pill at the bottom of an unhelpful Coach reply) would flow naturally from the H7 thumbs feedback you just shipped — a thumbs-down could surface a "try the same question with shorter / drier tone?" inline action. That's the iteration loop the differentiator-as-AI-coach memory ([feedback_ai_insights_differentiator]) wants. The cog stays as a power-user fallback, but it shouldn't be the primary surface.
2. **Per-message thumbs landed without a "why" follow-up.** The v1.4.23 H7 implementation captures rating but no qualitative signal. Helpful-rate aggregates are useful for "is this version landing", but the question that informs the next prompt iteration is "what made this unhelpful — wrong, vague, too-warm, missed-context?". Even a 4-chip follow-up after thumbs-down ("incorrect / too long / wrong tone / missed something") would 10x the operator's prompt-tuning signal. v1.4.24 candidate, but flag the gap before the aggregator telemetry calcifies the current shape.
3. **Sleep-stage analytics + `/api/auth/me/devices` shipped backend-only.** Both endpoints are live, both have integration coverage, neither has a UI surface. That's the right velocity call (iOS app is the consumer for both), but worth documenting in the v1.4.24 plan as "UI surface follow-up: web devices view + sleep-stage breakdown panel". A user on web who connects Apple Health right now will see the new pink chip on imported measurements but have no way to see sleep stages or manage which iPhone owns which APNs token. That's a "we shipped infra but not the user-visible payoff" gap that erodes user trust in releases that look big in CHANGELOG.md but reveal nothing new in the UI.
.planning/phase-W6-v1423-product-lead-review.md
+648
View File
@@ -0,0 +1,648 @@
# v1.4.23 — Product-Lead Review
Author: Marc (strategic memo to self)
Date: 2026-05-11
Status: pre-release of v1.4.23; W2 Apple Health foundation +
W3 APNs scaffolding + W4 OpenAPI/Coach/native-auth + W5 hygiene
all on `develop`. W6 multi-agent review in flight (this memo is
the product-lead leg). v1.4.22 went out the door yesterday;
v1.4.23 is the next tag. This memo updates
`.planning/phase-W5-v1422-product-lead-review.md`. Same audience:
Marc-three-weeks-from-now. The question this version answers:
**what backend foundation does v1.5 — the iOS native client +
Apple Health integration — now have on day zero, and what does
v1.5 P1 (iOS first launch) actually need to be now that the
contracts exist?**
---
## A. State of the App after v1.4.16 → v1.4.23
The arc is now eight releases long and the rhythm has held for
two cycles in a row. **v1.4.16** was the polish-leap that
overshot. **v1.4.17** was the schema-strictness hotfix.
**v1.4.18** walked back the visual overshoots and grew
Achievements. **v1.4.19** was the deliberately-boring polish that
lined up the runway. **v1.4.20** was the deliberately-loud
product release: Coach drawer + streaming + Daily Briefing +
correlations + Weekly Report + Health Score. **v1.4.21** was a
five-commit e2e-stability fix wave. **v1.4.22 was the long-tail
polish release** that closed loops carried since v1.4.16 — the
Coach prose rewrite + sentinel block + BD-Zielbereich re-anchor +
target-page sparklines.
**v1.4.23 is something the v1.4.x line hasn't done before: a
release whose user-visible delta is small on purpose because the
weight is in the foundation under it.** Seven hygiene items + an
Apple Health measurement schema + APNs scaffolding + an OpenAPI
generator + per-device refresh-token revocation + a Coach
settings cog + a per-message thumbs-feedback loop. None of those
are loud features; together they're the shape of v1.5's contract
surface, frozen in code before the iOS app starts compiling
against it. The cadence has matured beyond the loud/quiet
two-beat into a three-beat: **loud release → polish release →
foundation release**. v1.4.20/v1.4.22/v1.4.23 was the first time
we ran the whole cycle.
**Quality bar trajectory: a sideways step on the surfaces, a
visible step-up under them.** No new product-surface noise; the
Insights page didn't grow; the Coach prose stayed where v1.4.22
parked it. What changed is that `POST /api/measurements/batch`
exists, `POST /api/devices` accepts an `apnsToken`, the OpenAPI
spec regenerates byte-identically across runs, and the refresh-
token replay-detection no longer punishes a two-device user.
None of those show up in the UI. All of them remove a class of
"v1.5 bogs down here" risk.
**Test counts.** 2109 → 2223 unit tests (+114), 85 → 100
integration (+15). Ten new integration tests for the batch ingest
- sleep-stage aggregation + APNs dispatch + coach-prefs +
coach-feedback paths plus a 6 000-row chunk-boundary regression
guard on the BP-in-target analytics path. PROMPT_VERSION ratchets
4.22.0 → 4.23.0 (one numbered ground rule added: GROUND RULE 12
"treat Apple Health categories as silent when the snapshot
doesn't carry them"). Smaller bump than v1.4.22's 4.20.2 → 4.22.0
because the persona didn't move; only the metric vocabulary did.
**Concerning regressions: none on the surfaces that shipped.**
The new Apple Health enum values are strictly additive (no enum
reorderings, no row mutations, the legacy
`(user_id, type, measured_at, source)` unique index stays). The
APNs scaffolding ships behind absent env-vars — production
without `APNS_KEY_ID` is a no-op rather than a boot failure.
The OpenAPI generator's CI step is `continue-on-error: true` for
this release, so a registry oversight on a non-iOS-touched route
won't red-bar a v1.4.23 PR (intentional — see E.2).
**Tech debt status — the items I named in the v1.4.22 memo**:
1. **Insights page `~9-surface orchestrator`.** Still un-split.
Held to v1.5 P5. No movement and no regression — the page
didn't grow in v1.4.23.
2. **Schema drift on `medication_schedules.days_of_week`.**
**Resolved.** W5 H5 deployed the column (migration
`0039_medication_schedule_days_of_week`, NULL = daily so
no backfill needed). Sr-MED-5 is dead.
3. **B1 Vitals tile row.** Still deferred to v1.5 P5 — but
with a sharper trigger: the tile row replaces the status-
card list once HRV / Sleep / Resting HR / Steps are
non-stub, and v1.4.23 just gave us the column shapes to
write that data into.
4. **`CoachDrawer key={prefill}` weaponising React keys
(Sr-HIGH-4).** **Resolved.** W5 H3 introduced a
`useResettableValue` hook + pure `nextResettableValue`
helper; `key={prefill}` is gone from the parent mount. The
drawer's prefill is now a fully-controlled prop. Clean
ground for v1.5 P3 to multiply this pattern into the iOS
Coach surface.
5. **Pearson p-value normal-approx at low df (Code-MED-03).**
**Patched, not solved.** W5 H6 raised `MIN_PAIRED_N` from
14 → 20 — the conservative gate. The rigorous incomplete-
beta replacement is queued as a v1.4.24 candidate and lands
before v1.5/v1.6 auto-discovery surfaces correlations
without a human-curated gate.
Two new items on the watchlist:
6. **OpenAPI registry coverage gap.** The new generator emits
~880 lines vs the legacy hand-maintained spec's 5 468; the
CI gate is `continue-on-error: true` until the registry has
caught up. The risk is a server-side route signature
change on a non-registered route lands clean and only
surfaces when the iOS DTO codegen breaks. **Flip the gate
to hard-fail in v1.4.24** and complete registry coverage
in parallel so the lockstep happens before the iOS app is
downloading the spec automatically.
7. **Coolify auto-deploy still requires the maintainer to flip
one toggle.** W4 F8 documented the exact recipe — the
`COOLIFY_WEBHOOK` + `COOLIFY_TOKEN` repo secrets land via
GH UI, the "Watch image registry for new digests" toggle
lives in the Coolify dashboard. The CI workflow can
`::notice::` the deploy timestamp + sha for triage but
can't flip the registry-watch toggle. If the toggle is
off, GHCR builds will be green and `/api/version` will
stay on v1.4.22 silently. **Maintainer action required
before v1.4.23 tag.**
---
## B. Biggest items shipped in v1.4.23
In priority order:
1. **Apple Health measurement schema + batch-ingest contract
(W2 F1+F2+F3).** The biggest contract decision of the
v1.5 lead-up. Seven new `MeasurementType` enum values
(`HEART_RATE_VARIABILITY`, `RESTING_HEART_RATE`,
`ACTIVE_ENERGY_BURNED`, `FLIGHTS_CLIMBED`,
`WALKING_RUNNING_DISTANCE`, `VO2_MAX`, `BODY_TEMPERATURE`),
`APPLE_HEALTH` joins `MeasurementSource`, a new `SleepStage`
enum mirrors `GlucoseContext`, and `Measurement` picks up
nullable `sleepStage` + `externalSourceVersion` columns
plus a composite unique index
`(user_id, type, source, external_id)`. Migration
`0036_apple_health_measurement_types` is strictly
additive — no row mutations, no rename. The new
`POST /api/measurements/batch` endpoint accepts ≤500
entries per call, wraps `withIdempotency()`, returns
per-entry `inserted | duplicate | skipped` status with a
typed `reason` field for skips so the iOS sync cursor can
advance accurately. Sleep-stage aggregation in
`/api/analytics` rolls multi-stage nights into one Berlin-
day datapoint with a per-stage breakdown over the trailing
30 days. Files: `prisma/schema.prisma`,
`prisma/migrations/0036_apple_health_measurement_types/migration.sql`,
`src/app/api/measurements/batch/route.ts`,
`src/lib/measurements/apple-health-mapping.ts`,
`src/app/api/analytics/route.ts`. Report:
`phase-W2-v1423-report.md`. **This is the contract the iOS
app gets to compile against on day zero.**
2. **APNs scaffolding + dispatcher cascade rewire (W3 F4).**
`@parse/node-apn` is the chosen library (W1 stream-2
decision). Provider singleton per gateway (sandbox vs
production), lazy-initialised so JWT signing doesn't run
at boot, JWT auto-rotates every ~50 minutes inside the
library. Permanent failures (`Unregistered`,
`BadDeviceToken`, `DeviceTokenNotForTopic`) drop the dead
Device row mirroring web-push 410 cleanup. The dispatcher
cascade order is now an explicit `channelPriority()` sort
(APNs → Telegram → ntfy → Web Push, unknowns last) so a
Postgres scan-order change can't reorder delivery between
deploys. `POST /api/devices` accepts paired `apnsToken` +
`apnsEnvironment` fields with a 422 when one comes without
the other; the cross-user-hijack guard from CLAUDE.md is
duplicated at the APNs-token layer (409 +
`apns_token_owned_by_other_user` audit reason). Migration
`0037_apns_device_columns` adds nullable columns plus a
paired CHECK constraint. Files:
`src/lib/notifications/senders/apns.ts` (new),
`src/lib/notifications/dispatcher.ts`,
`src/app/api/devices/route.ts`. Report:
`phase-W3-v1423-report.md`.
3. **OpenAPI 3.1 generator + drift gate (W4 F5).**
`zod-openapi@5.4.6` reads Zod v4 `.meta()` annotations on
the existing validation schemas; `yaml@2.8.4` serialises
with `sortMapEntries: true` so the output is byte-stable
across runs. The eight v1.5-iOS-critical routes are
registered now (auth/login, passkey verify, refresh,
measurements GET + POST + batch, devices POST, insights
comprehensive); the legacy hand-maintained spec is
preserved at `docs/api/openapi-v1422-legacy.yaml` so iOS
DTO reference doesn't disappear during the organic
catch-up. CI step diffs generated vs committed; warn-only
for v1.4.23, hard-fail in v1.4.24+. Files:
`src/lib/openapi/registry.ts`, `src/lib/openapi/routes.ts`,
`docs/api/openapi.yaml`, `.github/workflows/security.yml`.
4. **Per-device refresh-token reuse-detection + device
management endpoints (W4 F7).** Pre-1.4.23 a refresh-token
replay revoked every refresh token the user owned —
correct for security but a foot-gun for a two-device
household. v1.4.23 scopes the blast radius to the
originating `deviceId`; legacy null-deviceId tokens fall
back to the wider revoke (safety hatch). Three new
routes: `GET /api/auth/me/devices` lists devices with
label, `lastSeen`, `channels` (`web_push` / `apns`),
`isCurrent` marker; `DELETE /api/auth/me/devices/[id]`
revokes a device + its refresh + access tokens + the row;
`DELETE /api/devices/[id]` is the native-friendly mirror
the iOS APNs-rotation flow calls. Cross-user attempts
return 404 with no enumeration leak. Files:
`src/lib/auth/refresh-rotate.ts`,
`src/app/api/auth/me/devices/route.ts`,
`src/app/api/auth/me/devices/[id]/route.ts`,
`src/app/api/devices/[id]/route.ts`.
5. **Coach Apple Health schema slot + PROMPT_VERSION 4.23.0
(W4 F6).** The strict insight schema's `sourceMetric` and
`trendAnnotations` enums extend to admit nine additive
HealthKit categories (hrv, sleep, resting_hr, steps,
active_energy, flights, distance, vo2_max, body_temp).
The Coach snapshot pipeline queries the new measurement
types only when scope toggles them on — web-only accounts
pay zero extra SQL. PROMPT_VERSION 4.22.0 → 4.23.0 with
GROUND RULE 12 (EN + DE): treat Apple Health categories as
silent when the snapshot doesn't carry them. No
"you're missing HRV data" apologetic openers. Files:
`src/lib/coach/prompts/coach-prompt.ts`,
`src/lib/ai/schema.ts`, `src/lib/coach/snapshot.ts`.
6. **Hygiene wave (W5 H1-H7, 8 commits).** Seven backlog
items from `.planning/v1422-backlog.md` shipped as atomic
commits. Notables: the `<CoachDrawer>` controlled-prop
refactor (Sr-HIGH-4 closed with `useResettableValue` hook
- pure helper), the analytics-route unbounded `findMany`
replaced with cursor-paged 5 000-row chunks (Sr-M1 closed,
`analytics.bp_in_target.row_count` wide-event meta added
for slow-query attribution), the per-user Coach prefs
surface (Coach settings cog returns; `User.coachPrefsJson`
migration `0038_coach_prefs`; system-prompt prepends a
per-user OVERRIDE; `excludeMetrics` filters BEFORE the
snapshot lands so the snapshot doesn't carry data the user
asked to keep out), the per-message thumbs feedback loop
(`RecommendationFeedback.target_type` polymorphic
migration `0040_recommendation_feedback_target_type`; new
`POST /api/insights/chat/messages/:id/feedback`; admin
aggregator slice at `/admin/coach-feedback` rendering
buckets sliced by promptVersion × tone × verbosity). Report:
`phase-W5-v1423-report.md`.
---
## C. v1.5 P1 (iOS first launch) — refreshed plan with concrete file paths
The v1.4.22 memo defined P1 in broad strokes: "Smallest
deliverable that proves bearer + refresh-token end-to-end: one
login page, one dashboard widget." v1.4.23 shipped every
server-side contract that plan assumed. P1 is now smaller than
the v1.4.22 memo described, because what was three or four
"must-build" backend items in P1 is now zero.
### What v1.4.23 already gave the iOS app
- `POST /api/auth/login` with `X-Client-Type: native` returns
`{token, refreshToken, expiresAt, deviceId}` — already shipped
in v1.4 (CLAUDE.md headless-client section), unchanged in
v1.4.23.
- `POST /api/auth/refresh` accepting `hlr_*` and returning a
fresh pair, with per-device reuse-detection (W4 F5 fixed the
two-device foot-gun).
- `POST /api/devices` accepting `apnsToken` + `apnsEnvironment`
with cross-user-hijack guard.
- `GET /api/auth/me/devices` for the iOS Settings → Devices tab.
- `DELETE /api/devices/[id]` for the iOS APNs-rotation cleanup
path.
- `POST /api/measurements/batch` for the future Apple Health sync
(not P1 itself but P2).
- `GET /api/measurements?from=&to=&type=` — already shipped in
v1.4 for the web app, no v1.4.23 changes needed.
- `docs/api/openapi.yaml` regenerates from
`src/lib/openapi/routes.ts` and covers all eight P1-touched
routes (W4 F5). The iOS DTO codegen has a stable target.
### What P1 is on the iOS side (~5 days)
The iOS handoff at `~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/`
already has `Services/APIClient.swift` (286 lines),
`Services/AuthService.swift` (232 lines),
`Services/KeychainStore.swift` (108 lines), and
`Models/MeasurementDTO.swift` (246 lines) scaffolded. P1 is
wiring those services to a single login screen, a single
dashboard view, and a single widget extension.
**Concrete file-path-level work on the iOS side:**
1. **Login screen at
`~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Screens/Auth/LoginView.swift`
(new — directory is currently empty).** Email/password form.
Calls `AuthService.login(email:password:)` which already
exists in `Services/AuthService.swift` and is already wired
to `POST /api/auth/login` with `X-Client-Type: native`. On
success: store both `token` (`hlk_*`) AND `refreshToken`
(`hlr_*`) in `KeychainStore` (`Services/KeychainStore.swift`
already has the API for this — `setAccessToken(_:)` /
`setRefreshToken(_:)` per the 108-line scaffold). On 401:
surface the localised error via `AuthService.AuthError`.
Passkey login is punted to P3 — the WebAuthn ceremony +
`PasskeyService.swift` exist but the iOS passkey UX needs a
dedicated wave (Apple's `ASAuthorization` flow is its own
five-step dance).
2. **Dashboard view at
`~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Screens/Dashboard/DashboardScreen.swift`
(already exists as a stub).** Single SwiftUI view rendering
today's BP + weight + mood from
`GET /api/measurements?from=<midnight>&to=<now>&type=BLOOD_PRESSURE_SYS,BLOOD_PRESSURE_DIA,BLOOD_PRESSURE_PULSE,WEIGHT,MOOD`.
The query takes a comma-separated `type` parameter — already
shipped in v1.4. `APIClient.swift`'s 401 → refresh chain
(lines ~120-180 per the 286-line scaffold) handles token
rotation transparently. Data lands as `MeasurementDTO`
(`Models/MeasurementDTO.swift`). Three labelled rows: BP
`132/86 · 72 bpm`, Weight `81.2 kg`, Mood `😊 Gut`. No
chart, no streak, no Coach button — those are P3+.
3. **Widget extension at
`~/Projects/healthlog-iOS/HealthLogIOS/HealthLogWidgets/LatestVitalsWidget.swift`
(new — directory exists, empty).** Lock-Screen rectangular
widget showing the same three rows as the dashboard. Reads
the cached payload via `WidgetCenter.shared` shared App
Group (`group.dev.healthlog`). Refresh policy: every 30
minutes via `TimelineProvider`. The widget never calls the
API directly — that would burn the user's bearer token in
the widget process, which has its own Keychain access scope
pain. The dashboard view writes the latest payload to the
shared App Group on every fetch; the widget reads from
there. If the cache is empty, the widget shows
"Open HealthLog" copy.
4. **Device registration on first successful login.** After
login lands the bearer token, `AuthService` requests APNs
permission via `UNUserNotificationCenter`, captures the
`deviceToken` from the AppDelegate's
`application(_:didRegisterForRemoteNotificationsWithDeviceToken:)`
callback, hex-encodes it via
`data.map { String(format: "%02x", $0) }.joined()` (NOT
`data.description`, which renders `<deadbeef cafebabe>`
with spaces — see W3 #1 open question), and `POST
/api/devices` with `{apnsToken, apnsEnvironment}` where
`apnsEnvironment` is `"sandbox"` for `#if DEBUG` and
`"production"` otherwise. The 409 cross-user-hijack guard
means an iPhone passed between two HealthLog accounts
can't redirect pushes — the previous owner deletes the
device first via `DELETE /api/auth/me/devices/[id]` from
their Settings tab. P1 doesn't need to surface the device
list yet — that's P3 alongside passkey.
**Server-side files touched in P1: zero.** Every contract is
locked. The only adjacent server change v1.4.24+ might land is
flipping the OpenAPI drift gate from warn-only to hard-fail
once the iOS DTO codegen has a confirmed working pipeline
against the v1.4.23 spec.
**Risk surface for P1 specifically (down from v1.4.22's two
risks to one):** Cert-pinning is empty in the iOS production
scheme per the iOS handoff's Phase 1 doc. Wire before first
TestFlight build. A missed pin update on Cloudflare cert
rotation bricks every device. The two-device-foot-gun risk
v1.4.22 named is now dead — W4 F5 fixed it.
### Cross-link to the consolidated 13 iOS DTO open questions
Every Wave 2/3/4 report ended with maintainer-facing open
questions about the iOS contract. W4's report consolidated all
13 in one section (`phase-W4-v1423-report.md` lines 137-199).
Routing each to a v1.5 phase:
**P1 (iOS first launch — login + dashboard + widget):**
- Q6 (W3 #1) **APNs token wire format = hex-join, no
`data.description`** — must land in the AppDelegate's
registration callback. Catches at 422 if the iOS
serialiser is wrong; clean fix in `LoginView.swift`'s
post-auth flow.
- Q7 (W3 #2) **`apnsEnvironment` = sandbox in Debug,
production in Release** — locked at the
`#if DEBUG`/`#else` site in the registration code; not a
runtime decision.
- Q12 (W4 NEW) **`X-Device-Id` header on
`GET /api/auth/me/devices`** — `APIClient` adds a default
header from `KeychainStore.deviceId` for that route family.
Already done for `/api/auth/refresh` per the 286-line
scaffold.
**P2 (Apple Health sync + measurement-source taxonomy):**
- Q1 (W2 #1) **Source enum rename
`HEALTHKIT` → `APPLE_HEALTH`** — the iOS DTO at
`~/Projects/healthlog-iOS/HealthLogIOS/HealthLog/Models/MeasurementDTO.swift:60`
still ships `case healthKit = "HEALTHKIT"`. One-line rename
- sim-data reset note in the iOS dev README. Land before
first TestFlight build that posts to `/api/measurements/batch`.
- Q2 (W2 #2) **`externalId` = `HKSample.uuid.uuidString`
verbatim**, NOT a composite — protects dedup across
iCloud-paired devices.
- Q3 (W2 #3) **`sleepStage` = numeric codepoint**, not string
label. Server accepts 0-20; iOS-16+ documented codepoints
are 0-5; headroom is for Apple's future additions.
- Q4 (W2 #4) **No pre-conversion on the wire** — server
expects Apple's native units (0..1 fraction for SpO2/body
fat, kcal for active energy). Double-multiplying lands SpO2
at 9 700.
- Q5 (W2 #5) **Unknown identifier behaviour = park-for-retry**
on the iOS side, not drop-from-cursor. A server-side
mapping addition then automatically backfills.
**P3 (Coach extended for new metrics + iOS Coach surface):**
- Q9 (W3 #4) **Multi-device cascade behaviour** — UX call:
two paired iPhones produce two notifications (matches
iCloud iMessage), or do we ship a "primary device" toggle?
The Coach surface multiplies this — every Coach reply
triggers the cascade. Decide before P3.
- Q10 (W3 #5) **`collapseId` shape** — server uses
`eventType` today; embed `medicationId` for per-medication
collapsing if the iOS UX wants two simultaneous reminders
for two different meds visible. Decide before P3 because
the Coach surface adds new event types.
- Q11 (W4 NEW) **Device-list channel rendering** — UI
decision: chips for both `web_push` + `apns` per device, or
filter to the platform-matching channel only? The iOS
Settings tab in P3 settles this.
- Q13 (W4 NEW) **Refresh + device-deletion race handling** —
iOS client should treat 401-after-delete as a re-login
signal, not a transient. The 401 handler in
`APIClient.swift` already bounces to `LoginView` per the
scaffold; confirm the path covers the post-delete case.
**P4 (per-metric APNs alerts):**
- Q8 (W3 #3) **Token rotation cadence** — iOS client should
call `DELETE /api/devices/[id]` on observed APNs rotation +
re-register, rather than relying on the server's
`Unregistered` cleanup alone. The endpoint is wired; the
iOS observer needs to call it.
The 13 questions span four phases. P1 only touches three. The
iOS Swift code lands answers as it lands; nothing on the
server side blocks the answer.
---
## D. v1.5 P2-P6 — minor adjustments based on what v1.4.23 learned
The v1.4.22 memo described P1-P6. v1.4.23 didn't invalidate any
of that plan; it sharpened P2 and P4's shape and dissolved the
P5 page-split prerequisite from "blocked on Apple Health work"
into "blocked on Apple Health DATA, not Apple Health SCHEMA".
### Phase P2 — Apple Health sync (~5 days, was 6-7)
**Down a day** because the schema work landed in v1.4.23 W2.
What's left for P2:
- iOS: `HealthKitService.swift` (already exists, 232-line
scaffold per the iOS handoff `Services/` listing) wires
`HKObserverQuery` for live updates +
`HKAnchoredObjectQuery` for incremental sync. Anchor stored
in `KeychainStore`.
- iOS: `Services/SyncCoordinator.swift` (new) batches anchor
delta into ≤500-entry calls to `POST /api/measurements/batch`,
reads per-entry status, advances the cursor past `inserted`
AND `duplicate`, surfaces `skipped` for diagnostics.
- iOS: Sim-data reset + DTO rename per W2 Q1.
Server side: zero new endpoints, zero migrations. The
`apple-health-mapping.ts` already covers all nine HealthKit
identifiers W1 enumerated. If iOS posts a new identifier we
haven't mapped, the server returns
`skipped`/`unmappable_identifier` and the iOS sync cursor
parks the sample for the next server release per Q5.
### Phase P3 — Coach + Daily Briefing extended for new metrics (~4 days, was 5)
**Down a day** because W4 F6 already landed the schema slot +
GROUND RULE 12. PROMPT_VERSION 4.23.0 already accepts the nine
HealthKit categories. The remaining P3 work:
- Prompt rules for HRV trends, sleep windows, resting HR,
steps as context-not-prescription. Each lands as a
numbered ground rule and ratchets PROMPT_VERSION 4.23.0 →
5.0.0 (major bump because the persona's responsibility
surface widens by half).
- Source-chip i18n bundles already partially landed in W4 F6;
remaining keys (`insights.coach.metric.{hrv,sleep,
restingHr,steps,activeEnergy,flights,distance,vo2Max,
bodyTemp}` × EN + DE).
- iOS Coach surface — copy the web's `<CoachDrawer>` UX
pattern. The W5 H3 controlled-prop refactor means the iOS
Swift port doesn't inherit the `key={prefill}` antipattern
v1.4.22 carried.
- Conversation-eviction policy from D.4 of the v1.4.22 memo
(auto-archive after 90d, hard-delete after 365d). The iOS
app multiplies Coach volume 3-5×; this is the right window
to land it before the worker re-architecture in v1.6.
### Phase P4 — Per-metric APNs alerts (~3 days, was 4-5)
**Down a day or more** because the entire send-side scaffold
landed in W3 F4. What's left for P4:
- Per-event opt-out in the iOS Settings tab — already routes
through the existing `NotificationPreference` model.
- Background `apns-push-type=background` push for the
HealthKit observer-query wake hook (W3 explicitly deferred
this from v1.4.23).
- Per-device-token mute (silence APNs on one phone but not
the other) — adds a `Device.mutedAt` column or a join row,
decide based on whether iOS ships a multi-device user.
- TestFlight smoke + production-gateway pivot test.
The two real risks I named in the v1.4.22 memo for P4 (sandbox
vs production gateway, rate-limit at the sender) are now
inherited by `@parse/node-apn`'s Provider abstraction —
proven library, not hand-rolled HTTP/2.
### Phase P5 — Web app polish + v1.5 release brief (~4 days, unchanged)
**Plan unchanged**, with one sharpening. The Insights page
split — the ~9-surface orchestrator — was held to P5 in the
v1.4.22 memo on the rationale that splitting before knowing
which sub-trees Apple Health touches risks re-splitting twice.
v1.4.23 didn't touch the Insights page (deliberately). P5's
split now has a concrete map: the new Vitals tile row replaces
the status-card list in the hero region; the existing
correlations row absorbs the optional HRV/sleep correlations;
the Daily Briefing row gains an optional sleep-debt panel.
Sub-tree boundaries are visible from the schema work.
### Phase P6 — Cross-user feedback aggregation cron (~3 days, parallelisable, unchanged)
**Plan unchanged**, with one sharpening. v1.4.23 W5 H7 shipped
the per-message thumbs feedback loop + the
`/admin/coach-feedback` aggregator slice already buckets by
(promptVersion, tone, verbosity). The P6 cron is now "wire
the existing aggregator to a daily pg-boss schedule + append
OMIT/REPHRASE rules to PROMPT_VERSION when a bucket's
helpful-rate drops below 50%". The aggregator code exists; the
cron + rule-append loop is the new work.
---
## E. Risks / Tech-Debt watchlist
1. **Coolify auto-deploy still requires a maintainer toggle.**
W4 F8 documented the recipe + added the `::notice::` log
line, but the "Watch image registry for new digests"
toggle in the Coolify UI cannot be flipped from CI. If the
toggle is off after v1.4.23 tag, GHCR builds will be green
and `/api/version` will silently stay on v1.4.22. **Action
before v1.4.23 tag:** confirm the toggle is on, run the
verification recipe (`curl /api/version | jq
.data.version`) on first deploy, document the host-side
fallback if the toggle drifts off again. The v1.4.21
release went out via a host-side fallback for the same
reason — the second time the same thing breaks the same
way means the runbook is fragile.
2. **OpenAPI drift gate is warn-only for v1.4.23.** The
generator covers ~880 lines; the legacy hand-maintained
spec is 5 468 lines. A server-side route signature change
on a non-registered route lands clean today and only
surfaces when the iOS DTO codegen breaks. **Flip to
hard-fail in v1.4.24** and complete registry coverage in
parallel. The risk window is "every PR between v1.4.23
and v1.4.24" — pull v1.4.24 forward if the iOS app starts
compiling against the spec sooner than expected.
3. **Refresh-token reuse-detection is now per-device, BUT the
safety-hatch falls back to user-wide for null deviceId
tokens.** Legacy tokens issued before W4 F5 still trip the
wide revoke. The fallback is correct (better safe than
sorry); the cost is that any user who logged in via the
web before v1.4.23 carries null-deviceId refresh tokens.
**Acceptable**: the wide revoke for legacy tokens only
bites on a replay attempt, which is the security signal we
want. Worth a watchlist entry so a future audit doesn't
delete the fallback as "dead code".
4. **The new `safeParse()` sites for HealthKit envelopes.**
v1.4.22 added one (the sentinel keyValues block);
v1.4.23's W4 F6 added the schema slot but the prompt
doesn't yet emit HealthKit data, so the safeParse exposure
is theoretical. P3's PROMPT_VERSION 5.0.0 bump WILL emit
real HealthKit envelopes (HRV, sleep, resting HR, steps,
plus the four secondary metrics). Audit every new
`safeParse()` call before P3 tag — same lesson as v1.4.17
hotfix. The sentinel parser observability work in W5 H1
(typed `SentinelParseResult.malformedEntries[]`) is the
right pattern; the new HealthKit envelopes should reuse
the same partial-malformed surface.
5. **Coach token-budget cap is a soft wall and the iOS
surface multiplies it.** 25 000 tokens/user/day at
~1 800 tokens/turn = ~13 turns. The HTTP 429 is correct
but the iOS UX needs a clear "you've reached today's
Coach limit; resets at 00:00 UTC" message rather than a
generic error toast. v1.5 P3 work — the iOS Coach surface
makes this more visible than the web-only surface today.
Carries forward from the v1.4.22 memo unchanged.
6. **The "summarise older half" pass is still a placeholder.**
The B2a history-window builder injects a synthetic
`[summary placeholder — N earlier turns elided]` line
rather than calling a provider for a real summary. v1.5 P3
if usage data shows it matters. The W3 sentinel pattern
gives us a cheap real-summary now: feed the older half's
prose + sentinel rows to a small-model summary call.
Carries forward from the v1.4.22 memo unchanged.
7. **Coach helpful/unhelpful first-week observation window
opens on v1.4.23 tag.** W5 H7's `/admin/coach-feedback`
aggregator answers "is the v1.4.22 prose rewrite landing
well, or did the warm tone overshoot?" If the helpful-rate
for PROMPT_VERSION 4.23.x drops below 50% within the first
100 ratings, v1.4.24 walks the persona back. **Watch the
admin dashboard daily for the first week post-tag.**
8. **The Pearson surfacing-gate raise (n≥14 → n≥20) means
some borderline correlation cards STOP rendering after
v1.4.23 tag.** W5 H6's conservative patch trades
false-positives for false-negatives. If the v1.4.16 B5e
feedback aggregator shows users miss those cards, drop in
the rigorous incomplete-beta replacement in v1.4.24 (queued
as the follow-up).
---
## F. Candid one-liner
v1.4.23 is the foundation release nobody outside this `.planning/`
directory will notice — Apple Health schema, APNs scaffold,
OpenAPI generator, per-device refresh-token revocation, Coach
prefs cog, helpful/unhelpful feedback loop — and that's exactly
what makes v1.5 P1 a five-day iOS Swift sprint instead of a
three-week server-and-iOS sprint where the contract changes mid-
flight.
.planning/phase-W6-v1423-security-review.md
+68
View File
@@ -0,0 +1,68 @@
# Wave 6 — Security review (v1.4.23)
Branch: `develop` (32 commits ahead of `main`).
Scope: every commit between `main` and `develop` for v1.4.23, focusing on the eight surfaces called out in the audit prompt (`POST /api/measurements/batch`, `POST /api/devices` APNs acceptance, APNs sender, device-management endpoints, refresh-token per-device revocation, `/api/auth/me/coach-prefs`, Coach message feedback, OpenAPI spec, Coolify runbook).
## Summary
- New attack surface in v1.4.23: batched Apple Health ingest with composite dedup, APNs token registration with cross-user hijack guard, an APNs sender that loads a `.p8` signing key from env, three device-management routes (list + delete via `/api/auth/me/devices` plus the iOS-friendly mirror at `DELETE /api/devices/[id]`), per-device scoping of refresh-token reuse-detection (legacy `null-deviceId` falls back to revoke-all), per-user Coach prompt-tuning prefs, per-message Coach feedback writing into the polymorphic `RecommendationFeedback` table, the auto-generated OpenAPI 3.1 spec at `docs/api/openapi.yaml`, and a maintainer runbook for Coolify auto-deploy at `.planning/coolify-auto-deploy-howto.md`.
- Findings: **0 CRITICAL · 0 HIGH · 2 MED · 4 LOW**.
- Overall posture: solid. The batch route is well-defended (entry cap, composite unique index, owner-scoped pre-flight + recheck, idempotency wrap). The cross-user-hijack guard is duplicated at both `token` and `apnsToken` layers and audit-logged on denial. Both DELETE-device routes return 404 (not 403) on cross-user attempts so device-id enumeration is blocked. The Coach feedback route enforces ownership via the `conversation: { userId }` join, rate-limits at 60/h, never echoes the prose back to admin dashboards, and dedupes via the existing unique index. The refresh-token reuse-detection legacy fall-back to user-wide revoke is preserved + tested. The OpenAPI spec exposes only the 9 iOS-touched routes — no `/api/admin/*`, no `/api/internal/*`, no debug endpoints. The two MEDs are about a missing DB-level uniqueness constraint on `apns_token` and the documented Coolify webhook URL embedding the production application UUID; the four LOWs are a per-entry status mismatch on intra-batch duplicates, a cached 422 that may surprise a retry author, an APNs key-file path leak in a warning string, and an audit row missing the userId on refresh-token failure.
## Findings
### MED-1 — `apns_token` has no DB-level uniqueness constraint, only an app-layer guard
- File: `prisma/migrations/0037_apns_device_columns/migration.sql:30` (creates `devices_apns_token_idx` as a plain index, not UNIQUE) + `prisma/schema.prisma:836` + `src/app/api/devices/route.ts:90-108` (the cross-user-hijack guard).
- What: the guard is a `findFirst({ where: { apnsToken, NOT: { userId } } })` followed by an upsert. Two concurrent `POST /api/devices` calls from two different users — both supplying the same APNs token — could each pass the lookup (no row yet) and then each insert a row, leaving two `Device` rows holding the same `apnsToken` for two different users. The `sendViaApns(userId, payload)` lookup at `src/lib/notifications/senders/apns.ts:319-329` filters by `userId`, so each user would still see their own row, but **the dispatcher would push the same APNs payload to the device for both users** — a redirected push.
- Why: APNs tokens aren't a secret, but they ARE device-stable, so the race window is theoretical (a user would have to physically share the token with another user and they'd both have to register within the same tick). Calling MED only because the CLAUDE.md commitment ("A device `token` belongs to exactly one user") is not enforced at the storage layer where it would be airtight.
- Fix: change the migration to `CREATE UNIQUE INDEX "devices_apns_token_unique_idx" ON "devices" ("apns_token") WHERE "apns_token" IS NOT NULL;` (partial unique index — multiple `NULL` values are still allowed for non-iOS device rows) and add `@unique` to the schema.prisma column. The application guard then becomes defence-in-depth: the unique index is the source of truth, and a P2002 thrown on the upsert maps cleanly to the existing 409 response.
### MED-2 — Coolify auto-deploy runbook embeds the live production webhook URL
- File: `.planning/coolify-auto-deploy-howto.md:25-26`.
- What: the runbook documents `COOLIFY_WEBHOOK = https://apps-01.bombeck.io/api/v1/deploy?uuid=pg8wggwogo8c4gc4ks0kk4ss&force=false` as the value the maintainer should paste into GitHub Secrets. That URL is the **actual live production deploy webhook**, not a placeholder. The same UUID also lives in 9+ historical audit / planning files (`docs/audit/v1414-summary.md`, `docs/audit/v1415-auto-deploy.md`, `docs/audit/v1416-auto-deploy-fix.md`, `docs/audit/v1418-summary.md`, `docs/audit/v1420-summary.md`, `.planning/phase-A1-report.md`, etc.).
- Why: triggering the webhook requires the `COOLIFY_TOKEN` Bearer in the `Authorization` header (`.github/workflows/docker-publish.yml:153`), so the URL alone cannot deploy. But the URL leaks (a) the Coolify host (`apps-01.bombeck.io`), (b) the API path (`/api/v1/deploy`), and (c) the production application UUID. Together with any future Coolify CVE that bypasses bearer-auth on `/api/v1/deploy?uuid=`, this becomes a one-click drive-by deploy of an attacker-controlled image. The MED rating reflects the historical exposure in `docs/audit/*.md` (already published in past releases) more than the new W4 runbook itself — v1.4.23 isn't introducing a new disclosure, just re-stating one. Worth fixing once for the whole repo.
- Fix: replace the literal URL in the runbook with a placeholder format (`https://<coolify-host>/api/v1/deploy?uuid=<your-app-uuid>&force=false`) and update the audit-summary docs in a future hygiene PR. The runbook should state "find this URL in Coolify UI → Application → Webhooks tab" without baking in the real value. Treat the UUID as a low-grade secret (deploy-trigger identifier) for future docs.
### LOW-1 — Intra-batch duplicate entries report `inserted` for both rows but only insert one
- File: `src/app/api/measurements/batch/route.ts:163-261`.
- What: when a single batch contains two entries with the same `(type, externalId)` pair — both not yet in the DB — the pre-flight `existing` lookup misses both, both get marked `inserted` in the per-entry results, then `createMany({ skipDuplicates: true })` inserts only one row. The reconciliation pass at line 226-261 only downgrades entries whose key is **NOT** in the recheck `stored` set; for an intra-batch duplicate the key IS in `stored` (because the one surviving insert is there), so neither entry is downgraded. Net result: `inserted: 1, duplicates: 0` in the count totals but `entries[*].status: "inserted"` for **both** indexes. The iOS sync cursor would then not flag either as a duplicate.
- Why: a self-inflicted accuracy bug at worst — the user can only confuse their own sync cursor. No data leak, no cross-user impact. The composite unique index still guarantees no duplicate row lands. Calling LOW because the per-entry status is the contract the iOS client uses to advance its cursor accurately, and the route's docstring at line 12-19 explicitly promises it.
- Fix: deduplicate `prepared[]` by `${type}::${externalId}` before the `existing` lookup. For each duplicate-within-batch, keep the first occurrence as `inserted` and mark every subsequent occurrence as `duplicate` immediately. Add a regression test in `tests/integration/measurements-batch.test.ts` covering "same externalId twice in one batch returns 1 inserted + 1 duplicate".
### LOW-2 — `withIdempotency` caches the 422 "batch too large" response
- File: `src/lib/idempotency.ts:131-138` (`isCachableStatus` allows 422) + `src/app/api/measurements/batch/route.ts:80-92`.
- What: per the documented contract ("validation 4xx IS cached so a broken retry doesn't re-hit the DB"), a 422 with `errorCode: coach.batch.too_large` from the entry-cap check is cached for 24h against the `(userId, key, method, path)` tuple. A retry author who first sent 600 entries (got 422), then trimmed to 500 entries and retried with the **same** `Idempotency-Key`, would silently get the 422 back without the second batch ever executing.
- Why: this is exactly the documented behaviour — Idempotency-Key contracts treat the body as immaterial after the first response. The intent is to prevent a broken-retry storm. Calling LOW because the 422-cache for "too large" is more confusing than a 422-cache for "missing field" — the body size is something the caller might naturally adjust between retries, whereas a missing field is a deterministic schema mismatch.
- Fix: leave the cache contract alone; instead, add a one-line note to the `coach.batch.too_large` error message reading "Idempotency-Key replay returns the original 422; retry with a fresh key." Optionally, mention the same in the OpenAPI `description` for the 422 response on `/api/measurements/batch`.
### LOW-3 — APNs key-file read failure surfaces the absolute path in a warning event
- File: `src/lib/notifications/senders/apns.ts:155-162`.
- What: when `APNS_KEY_FILE` is set but unreadable, the sender annotates the wide event with `getEvent()?.addWarning(\`APNs key file read failed: ${message}\`)`. Node's `ENOENT`/`EACCES`error messages embed the full filesystem path (e.g.`ENOENT: no such file or directory, open '/var/secrets/AuthKey_ABC1234.p8'`), so the path lands in the structured log payload.
- Why: low impact — operator-only observability, the path is the operator's own host filesystem, and the same warning is exactly what an operator wants when troubleshooting. Calling LOW because the wide-event payload also flows to Loki Push if `LOKI_*` env is configured, and a Loki tenant credential leak would then expose the path. The signing key itself is **not** in the warning (the failure is "couldn't even open the file"), so this is path-disclosure, not key-disclosure.
- Fix: replace `${message}` with a redacted variant — e.g. annotate `apns.config.key_file_unreadable` with `meta: { errno: err.code }` instead of the raw message. The `errno` is enough for an operator to triage (ENOENT vs EACCES) without leaking the path.
### LOW-4 — `auth.token.refresh.failed` audit row omits `userId` even when it can be resolved
- File: `src/app/api/auth/refresh/route.ts:68-83` + `src/lib/auth/refresh-token.ts:107-148`.
- What: `rotateRefreshToken` looks the row up by `tokenHash`, so on a `not_found` / `expired` / `revoked` / `already_used` failure it has the `userId` available (except `not_found`, which truly doesn't). The route at line 69 writes `auditLog("auth.token.refresh.failed", { ipAddress: ip, details: { reason: result.reason } })` without a `userId`, even when `result.reason === "already_used"` and the row that triggered the reuse-detection cascade is known.
- Why: refresh-token reuse-detection is **the** signal that a refresh token was leaked / replayed. Without `userId`, the audit-log dashboard can't pivot from a reuse event to "which user got their token reused" without re-querying the refresh-token table by hash. Calling LOW because the wider revoke (per-device or per-user) was already done by `rotateRefreshToken`, and the affected user's other devices got revoked, so the operational impact is contained — but reuse-detection deserves a high-fidelity audit trail.
- Fix: extend `RotationResult.failure` shape to carry the resolved `userId` when known (i.e., for `expired` / `revoked` / `already_used` — `not_found` truly has no user). Pass it through to the route's `auditLog` call. Same fix for the `revoke` path at line 45-49 (`revokeRefreshToken` already looks up the row, can return the userId alongside the boolean).
## Strongest defensive postures (v1.4.23 added)
- **Cross-user hijack guard, fully defence-in-depth.** Both `POST /api/devices` (`token` upsert) and the new `apnsToken` acceptance enforce the same boundary: a 409 on re-registration under a different user, with `device.register.denied` audit rows carrying the reason (`token_owned_by_other_user` / `apns_token_owned_by_other_user`) and the offending device id for incident-response triage (`src/app/api/devices/route.ts:90-126`). The pattern survives across both DELETE routes too — `/api/auth/me/devices/[id]` and `/api/devices/[id]` both return 404 (not 403) on cross-user attempts so device-id enumeration is blocked, and both routes scope refresh-token + access-token revocation to `deviceId`-only (not user-wide) so revoking one device never collateral-damages another (`src/app/api/auth/me/devices/[id]/route.ts:41-67`).
- **Refresh-token reuse-detection scoped per-device, with a safe legacy fall-back.** Pre-v1.4.23 a single replayed refresh token revoked **every** active refresh+access pair the user owned across every device, signing them out of their iPad when their iPhone replayed a token. v1.4.23 narrows the blast radius to the originating `deviceId` (`src/lib/auth/refresh-token.ts:117-148`), but tokens issued before v1.4.23 carry `deviceId === null` and therefore still trigger the wider revoke — there's no way to safely scope them, so the safety hatch is correct. The legacy null-deviceId path is unit-tested at `src/lib/auth/__tests__/refresh-token.test.ts:284`. The OpenAPI description at `src/lib/openapi/routes.ts:374-376` documents the per-device behaviour for the iOS team. The replay-detection error message ("Refresh token reuse detected — please log in again.") is identical regardless of which device triggered it, so there's no leak about which device the replay came from.
## Things uncertain — flagged for maintainer review
- **`/api/auth/me/devices` GET returns the full `Device.token` shape via the `web_push` channel inference, not the token itself.** The DTO at `src/app/api/auth/me/devices/route.ts:42-62` exposes `id`, `label`, `platform`, `appVersion`, `locale`, `channels`, `lastSeenAt`, `createdAt`, `isCurrent` — explicitly **not** the `token` or `apnsToken` value. Confirmed by reading the route + the test fixtures. Worth a maintainer eyeball on whether `appVersion` / `locale` reveals more about a paired device than the user expected for the settings-list use case (probably fine — these are user-set, not server-derived).
- **Idempotency cache replay does not re-execute auth.** `withIdempotency` at `src/lib/idempotency.ts:215-216` returns the cached response immediately on hit, before the handler (and therefore before `requireAuth()`) runs. The userId-resolver default at line 149-173 DOES re-validate the cookie session OR the bearer token currently presented, so a revoked token can't read a cached response. But if a user's role changes (USER → ADMIN, or admin demoted) between the original request and the replay, the cached response shape might not match what the user is currently entitled to read. Today none of the wrapped routes vary their response by role (they're per-user data routes), so the impact is zero. Worth a note for future authors who might wrap an admin-aware route in `withIdempotency`.
- **The Coach assistant prose is decrypted into `RecommendationFeedback.recommendationText` on every thumbs rating** (`src/app/api/insights/chat/messages/[id]/feedback/route.ts:99-113`). The prose can contain personalized health observations from the user's data. The admin coach-feedback dashboard explicitly never surfaces this column (`src/components/admin/coach-feedback-section.tsx:18` — "admin view never exposes per-user prose"), and there's no read endpoint for the row, so today the prose is only ever read by the user themselves via the existing recommendation-feedback list. If a future feature adds a "review my feedback history" UI, the route author needs to ensure the prose stays gated behind `userId` ownership. Worth a one-line code comment on the persistence call so future readers don't assume `recommendationText` is generic-enough to surface in shared admin views.
.planning/phase-W6-v1423-senior-dev-review.md
+477
View File
@@ -0,0 +1,477 @@
# Wave W6 — Senior-dev review (v1.4.23)
Reviewer: senior-dev lens, read-only
Branch: `develop` @ `72829b1` (29 commits ahead of `main`)
Inputs: phase-W2/W3/W4/W5 reports + diff walk over the four
ship-bundles (W2 Apple Health backend, W3 APNs scaffolding, W4 OpenAPI
- device-management + per-device refresh, W5 hygiene)
## Summary
Architectural posture: **healthy with three structural concerns and
one notable win.** v1.4.23 is the largest "additive surface" release
since v1.4.20: 5 migrations, 8 new routes, two new third-party libs
(`@parse/node-apn`, `zod-openapi`), and a fully-functional Coach
preferences loop. The chunks land in the right places — `apple-health-
mapping.ts` next to its sole caller, `senders/apns.ts` next to the
other channel senders, `coach-prefs.ts` in `validations/` with a
schema-as-defaults pattern that keeps call sites clean. Tests cover
the happy paths and the headline failure modes; integration coverage
followed unit coverage commit-by-commit.
The three structural concerns:
1. **`/api/analytics` is now a fan-out hot-spot.** The route fires
`findMany`-per-MeasurementType across all 17 enum values plus a
second SLEEP_DURATION query plus a chunked-but-still-unbounded BP
read plus 6 correlation queries — every analytics request is a
~25-query Prisma flight on a per-user dataset that can be tens of
thousands of rows. The "chunked" BP perf fix in W5 mitigates the
payload-marshalling spike but the working-set ceiling is unchanged.
2. **Two device-revoke endpoints are 90% duplicated, neither
transactional.** `/api/auth/me/devices/[id]` and `/api/devices/[id]`
each perform refresh-token-revoke + access-token-revoke +
device-delete as three separate writes — a partial failure leaves
dangling rows. The duplication is also load-bearing for the audit
trail: the two routes write the same `devices.revoke` action with
slightly different `details` blobs.
3. **`metricSourceType` for Coach feedback encodes structured data as
a string** (`coach:tone=warm:verbosity=default`) and the aggregator
re-parses it via regex on every run. This works but it's the
string-coupling anti-pattern; a `tone` + `verbosity` column pair on
`RecommendationFeedback` would be the same DDL cost and let the
query planner help.
The architectural win:
- **The OpenAPI registry as a code-driven artifact**, with the legacy
hand-maintained spec preserved as a sibling and a warn-only CI gate
to ratchet coverage. The two-stage migration is the right call —
flipping the gate to hard-fail in v1.4.24+ once the registry
catches the rest of the surface gives the iOS DTO codegen a
single-source-of-truth path without forcing a 5000-line schema
rewrite in one PR.
Findings: **0 CRITICAL · 3 HIGH · 6 MED · 4 LOW**
---
## CRITICAL
(none)
## HIGH
### H1 — `/api/analytics` fans out 17+ findMany queries per request,
reads entire history for every metric type (W2)
**Where:** `src/app/api/analytics/route.ts:35-81` (the type loop) +
`route.ts:92` (sleep-stage breakdown — re-reads SLEEP_DURATION) +
`route.ts:118-141` (BP windows — chunked but unbounded) +
`computeCorrelationHypotheses()` (6 more findMany).
**Symptom:** v1.4.23's enum additions push the type loop from 10 to 17
parallel queries, each unfiltered by date. The pattern is
`prisma.measurement.findMany({ where: { userId, type } })` with no
`measuredAt` cap — the route pulls the user's _entire history_ per
type, runs `summarize()` on it (which only needs the latest few +
totals), and discards the rest. For an iOS user paired with Apple
Health, the analytics-route cost grows linearly with HealthKit days
× metrics-with-data — a realistic 90-day power user holding ~270
HRV samples + ~270 resting-HR + ~5400 step samples + ~270 sleep-
stage rows is already a six-figure-row read per analytics call.
The "chunked" BP fix in W5 (`fetchBpSeriesChunked()`) papers over the
problem for two specific types but doesn't address the root cause:
the route is structured to load every measurement to compute
small-arity summaries.
**Architectural lift:**
- `summarize()` only needs `count`, `latest`, `avg7`, `avg30`. The
per-type `findMany` should be replaced with a single
`prisma.measurement.groupBy({ by: ["type"], _count, _avg, _max })`
scoped to a 30-day window plus a per-type "latest" query, not
full-history reads.
- The sleep-stage breakdown re-reads SLEEP_DURATION rows that the
type loop already pulled. Either fold the breakdown into the type
loop's SLEEP_DURATION branch or let the breakdown drive the
summary (one read).
- The chunked-fetch helper (`fetchBpSeriesChunked`) accumulates the
full history into the same `out: BpReading[]` array. The "working
set ceiling = BP_CHUNK_SIZE" claim in the comment is incorrect:
the ceiling is `total_rows`. The benefit is purely the Prisma
intermediate buffer + the index-scan vs full-table-scan path —
which is real but smaller than the comment implies.
**Severity:** HIGH because the v1.5 iOS app will land high-frequency
HealthKit metrics (steps, HRV, sleep stages) and the analytics route
is on the hot path for the dashboard. The growth is linear in
HealthKit-days, not bounded; this becomes a P1 the moment a TestFlight
user has a 6-month sync history.
### H2 — Two device-revoke endpoints duplicate the cascade, neither
transactional (W4)
**Where:** `src/app/api/auth/me/devices/[id]/route.ts:31-95` and
`src/app/api/devices/[id]/route.ts:26-86`.
**Symptom:** The two routes each perform the same three writes
sequentially:
1. `prisma.refreshToken.findMany` — read access-token hashes
2. `prisma.refreshToken.updateMany` — revoke refresh rows
3. `prisma.apiToken.updateMany` — revoke access-token rows
4. `prisma.device.delete` — drop the device
There is no `prisma.$transaction(...)` wrapper. A failure between
step 3 and step 4 (Postgres connection blip, app crash) leaves the
device row alive with all its tokens revoked — the user's iPad shows
in the device list as "still registered" but every request returns 401. The next-best test for this is "did anyone notice yet" because
no UI surface flags the inconsistency.
The duplication is also a maintenance hazard: the W3-noted use case
("iOS app calls `/api/devices/[id]` on APNs rotation") could just as
easily call `/api/auth/me/devices/[id]` — there's no functional
distinction. The two URLs were retained "to keep the audit trail
consistent" per the W4 report but the audit trail diverges anyway
(`via: "ios.rotation"` only on one branch).
**Architectural lift:**
- Extract a shared `revokeDevice(userId, deviceId)` helper in
`src/lib/auth/devices.ts`, wrap the four writes in
`prisma.$transaction(async (tx) => { … })`, return a typed result
(`{ revoked: boolean, refreshTokensRevoked, accessTokensRevoked }`)
that both route handlers can render directly.
- Audit-log call shifts into the helper; route handlers pass an
`origin: "settings" | "ios_rotation"` flag for the `details` slot.
- Picks a single canonical URL — `/api/auth/me/devices/[id]` matches
the listing route at `GET /api/auth/me/devices`. The
`/api/devices/[id]` mirror becomes a thin redirect (or stays as a
thin shim that calls the helper) so iOS-rotation traffic doesn't
break.
**Severity:** HIGH because the failure mode (orphan device row +
revoked tokens) leaves a user logged out of an account that still
shows the device as paired — confusing, and the only fix is admin
intervention.
### H3 — Coach feedback persists assistant prose plaintext into
`recommendation_feedback.recommendation_text` (W5 H7)
**Where:** `src/app/api/insights/chat/messages/[id]/feedback/route.ts:99-113`.
**Symptom:** The Coach assistant's reply prose has historically lived
encrypted-only (`coach_messages.encrypted_content`, AES-256-GCM via
`@/lib/crypto`). The new H7 route decrypts the message, slices it to
4 KB, and writes it as the `recommendationText` slot of a
`RecommendationFeedback` row. That table is **not** encrypted. The
exact same Coach prose now lives in two places: encrypted in
`coach_messages` and plaintext in `recommendation_feedback`.
This was almost certainly an unintentional consequence of reusing the
v1.4.16 B5e schema, where `recommendationText` was the prose snapshot
of an Insights recommendation (already plaintext on the wire — the
recommendations themselves are not encrypted). The Coach surface
inverts that assumption.
**Architectural lift:**
- Either encrypt the `recommendationText` column on write for
`targetType="coach"` rows (same `encrypt()` helper, decrypt on
admin/aggregator read), OR
- Replace the prose-snapshot dedup key with a stable hash of the
message-id + a content-version counter on the message itself. The
H7 brief noted the dedup is over "the same message + same prose
snapshot" — message id alone would be sufficient since assistant
messages are immutable.
**Severity:** HIGH because the Coach replies can include user-
identifying details the model paraphrases back ("your last week of
BP readings averaged …"). The encryption-at-rest invariant the
Coach surface ships under is silently broken for any message a user
rates.
## MED
### M1 — `coachPrefsJson` is read twice on every Coach turn (W5 H4)
**Where:** `src/app/api/insights/chat/route.ts:209-213` (chat handler
read) + `src/lib/ai/coach/snapshot.ts:264-267` (snapshot builder
read).
The chat route fetches `User.coachPrefsJson`, parses it, builds the
system prompt; then calls `buildCoachSnapshot(userId, scope)` which
fetches the same column independently and reparses it.
**Lift:** Pass `coachPrefs` (or just `excludeMetrics`) into
`buildCoachSnapshot()` as a parameter. Saves one indexed query +
one Zod parse per turn — small but free.
**Severity:** MED — it's a micro-N+1 inside the hot path of the most
expensive endpoint in the app.
### M2 — `feedback-attribution.ts` uses dynamic import to "avoid
circular imports" but no cycle exists (W5 H7)
**Where:** `src/lib/ai/feedback-attribution.ts:135-137`.
The comment claims dynamic-import is needed to avoid a cycle with
`coach-prefs.ts`. Verified: `coach-prefs.ts` only imports `zod/v4`,
no transitive dependency on attribution. The dynamic import adds
async-cost on every call for a defensive measure that protects
against nothing.
**Lift:** Replace with a top-of-file `import { parseCoachPrefs }
from "@/lib/validations/coach-prefs"`.
**Severity:** MED — minor latency, but it's the kind of comment-as-
documentation that future readers will trust without re-verifying.
### M3 — `metricSourceType` for Coach feedback is structured-as-string
(W5 H7)
**Where:** `src/lib/ai/feedback-attribution.ts:104-109`
(`buildCoachMetricSourceType`) +
`src/lib/jobs/feedback-aggregator.ts:205-214`
(`COACH_METRIC_SOURCE_RE`).
Tone + verbosity get encoded as `coach:tone=warm:verbosity=default`
and re-parsed via regex by the aggregator. Aggregations group by
this string. Anyone adding a third dimension (e.g.
`excludeMetricsCount`) has to update the encoder, the regex, and
the parser, with no compiler help and a silent fallback to
`tone="unknown"` when the regex misses.
**Lift:** Add `tone` + `verbosity` columns to
`RecommendationFeedback` (already nullable for non-coach rows), let
the aggregator `groupBy: ["promptVersion", "tone", "verbosity"]`
directly. The migration is one ALTER TABLE.
**Severity:** MED — the existing pattern works but is the kind of
debt that compounds the moment a fourth dimension lands.
### M4 — `apple-health-mapping.ts` declares `aggregation` and
`isPrivacySensitive` fields nothing reads (W2)
**Where:** `src/lib/measurements/apple-health-mapping.ts:28-62` and
the table entries that populate them.
Both fields are declared on the `AppleHealthMapping` interface and
filled in for every entry, but nothing in the runtime path consumes
them — `mapAppleHealthEntry()` only reads `measurementType`,
`convertToDbUnit`, `dbUnit`, `sleepStageMap`. The
`isPrivacySensitive` flag has a single test reference (filtering
the table to assert the marked entries are the expected ones). The
`aggregation` hint is documented as "advisory metadata for
downstream summarisation" — i.e., aspirationally consumed.
**Lift:** Either delete the fields (YAGNI — re-add when the
audit-trail or analytics consumer materialises) or wire them up
behind a v1.4.24 commit that actually consumes them (privacy-
sensitive ingest gets a separate audit-log action; aggregation hint
drives the analytics-route per-type rollup).
**Severity:** MED — dead structural vocabulary teaches future
contributors a pattern that doesn't exist.
### M5 — `isCurrent: true` in `GET /api/auth/me/devices` trusts the
`X-Device-Id` header without authentication (W4)
**Where:** `src/app/api/auth/me/devices/route.ts:35,60`.
The route reads `X-Device-Id` from the request header and marks the
matching row as `isCurrent: true`. There is no check that the
supplied id matches the device behind the current refresh-token /
session. A malicious client can flag any of their own device rows
as `isCurrent` — low impact (only their own UI), but it means the
field is a presentation hint that _looks_ like an authentication
fact.
**Lift:** Resolve the current device id from the auth context (the
refresh token's `deviceId`, the session's bound device, or absent
for cookie-only callers) and only mark that one. The `X-Device-Id`
header stays as the cookie-only-caller fallback.
**Severity:** MED — the failure mode is "user lies to themselves";
real impact is zero today, but it's a UI-as-truth pattern that will
get reused if not corrected.
### M6 — APNs `apns_not_configured` returns `hardReject: false`,
silently burning channel-state failure budget (W3)
**Where:** `src/lib/notifications/senders/apns.ts:208-216` and
`:309-318`.
When the APNs env vars aren't set (the v1.4.23 default — APNs is
scaffolded but no operator config exists yet), `sendViaApns` returns
`{ ok: false, hardReject: false, reason: "apns_not_configured" }`.
The dispatcher classifies that as a transient failure and bumps the
channel-state machine's `consecutiveFailures` counter. After 5
dispatch cycles the channel auto-disables — but the cause was never
transient, the operator just hadn't enabled APNs.
This is dormant in v1.4.23 because no code path creates an APNS
NotificationChannel row (the dispatcher's branch is wired but
unreachable until v1.5). The bug becomes live the moment the iOS
device-registration flow creates the channel row.
**Lift:** Treat `apns_not_configured` as either (a) a hard-reject
with a typed reason that the channel-state machine recognises and
keeps the channel **enabled but cooled down indefinitely until the
config arrives**, or (b) a special "skip without counting" outcome
that the dispatcher loop short-circuits on.
**Severity:** MED — pre-condition for v1.5 P4. Worth fixing now
while the surface is dormant, before a TestFlight build paints the
"channel auto-disabled" toast at the user.
## LOW
### L1 — `docs/api/README.md` describes manual openapi.yaml editing,
now overwritten by the generator (W4 F5)
**Where:** `docs/api/README.md:67-77` ("Updating the spec" section)
contradicts `scripts/generate-openapi.ts` which overwrites the file
on every run.
**Lift:** Replace the "Updating the spec" section with the actual
flow ("schemas live in `src/lib/validations/*` with `.meta()`
annotations; route table in `src/lib/openapi/routes.ts`; run
`pnpm openapi:generate`"). Add a banner comment at the top of
`docs/api/openapi.yaml` warning that hand edits will be overwritten.
### L2 — `buildOpenApiDocument` uses `require()` to lazy-load routes
(W4 F5)
**Where:** `src/lib/openapi/registry.ts:65-76`.
The registry calls `require("./routes")` inside the builder with an
ESLint-disable. The comment justifies it as "lazy-load so the base
scaffolding stays usable from devtools-only consumers" — but no
such consumer exists, and the dynamic require defeats both
typecheck-narrowing and tree-shaking. Replacing with a top-of-file
import is one diff.
### L3 — `fetchBpSeriesChunked` cursor is single-column on a
composite-ordered query (W5 H2)
**Where:** `src/app/api/analytics/route.ts:455-483`.
`orderBy: [{ measuredAt: "asc" }, { id: "asc" }]` paired with
`cursor: { id: cursorId }`. When two rows share a `measuredAt`
value (manual bulk imports do this routinely), the cursor's `id`-
only positioning may drop or duplicate rows on ties. Prisma docs
recommend matching cursor shape to the orderBy shape (compound
unique).
**Lift:** Either drop `measuredAt` from the orderBy (cursor by `id`
alone, sort the in-memory accumulator after) or use a compound
cursor (`cursor: { measuredAt_id: { measuredAt, id } }` once an
appropriate index exists).
### L4 — Apple Health mapping table comment mentions a "16 unit tests"
claim that doesn't match the diff (W2 doc)
**Where:** `.planning/phase-W2-v1423-report.md:27` and the
underlying test file. The mapping test file is fine — small nit on
the report.
---
## What's well-done — keep doing it
- **Migration ordering is safe.** Five new migrations
(0036/0037/0038/0039/0040), every one strictly additive: ADD
COLUMN nullable, ADD VALUE IF NOT EXISTS, CREATE INDEX/TYPE,
CHECK constraints scoped to NULL OR (X). No reordering, no row
mutations, no contention. The 0036 `ALTER TYPE ADD VALUE` rows
follow the established pattern from 0021/0022/0024 — the new
values aren't consumed in the same migration's subsequent
statements (only the brand-new `sleep_stage` enum is, and that
was `CREATE TYPE`, not `ALTER`). Deploy ordering is bullet-proof.
- **`coachPrefsSchema` defaults are inlined into the schema.**
`safeParse({})` returns the legacy v1.4.22 defaults — no `?? defaultX`
call sites, no schema-vs-defaults drift. The pattern travels well
to future per-user-toggle additions.
- **`channelPriority()` makes the dispatcher cascade deterministic.**
Replacing the implicit Postgres-scan order with a static priority
sort is the right call. Unknown types defaulting to `99` keeps
experimental channel rows from preempting real ones.
- **Per-device refresh-token revocation with the legacy fallback.**
The W4 H5 fix correctly scopes the blast radius to the
originating device ID while keeping `null deviceId` rows on the
wide revoke. The two-device test pinning the boundary is
excellent — exactly the kind of test that prevents a future
"let me clean this up" from re-broadening the scope.
- **OpenAPI generator + warn-only CI gate as a two-stage migration.**
Strict subset → ratchet → flip to hard-fail is the textbook
approach. Preserving `openapi-v1422-legacy.yaml` as a sibling
reference during the transition is the right call.
- **Apple Health batch ingest's per-entry status (`inserted | duplicate
| skipped`).** Rather than fail-the-batch, the route degrades each
entry independently and surfaces a typed reason for skipped rows.
iOS clients can advance their sync cursor accurately and operators
can investigate per-row reasons without re-running the upload.
- **`useResettableValue` hook replaces the v1.4.20 `key={prefill}`
weaponisation.** Render-phase setState matching React's official
recommendation, ESLint-clean against
`react-hooks/set-state-in-effect`. Pure decision function
(`nextResettableValue`) is independently unit-testable. The W5 H3
refactor is exemplary.
---
## For product-lead-review cross-link
Two items worth flagging for the v1.5 P1 risk register:
- **`/api/analytics` scaling cliff (H1).** When the iOS client lands
HealthKit ingest in v1.5, the dashboard's analytics request will
hit the 17-type fan-out + per-type full-history reads on every
load. A user with 6 months of HRV samples (~180 rows) + 6 months
of step samples (~5 400 rows if pulled per-hour, ~180 if pulled
per-day) is a realistic v1.5 P1 power user. The route's read
shape needs the `groupBy` rewrite **before** TestFlight
distribution, or the dashboard becomes the iOS app's first
performance complaint.
- **Coach feedback prose stored plaintext (H3).** The Coach is the
v1.5 differentiator. Storing plaintext assistant prose in
`recommendation_feedback` for any rated message breaks the
encryption-at-rest invariant the Coach ships under and lives
forever in DB backups. Worth resolving before the H7 thumbs row
is exposed publicly — the table schema fix is an ALTER, not a
rewrite.
The third HIGH (H2 — duplicated device-revoke endpoints + missing
transaction) is a maintenance / data-integrity concern that won't
manifest until a Postgres blip lands inside the cascade. Fix it
opportunistically; it's not a v1.5 ship-blocker.
---
## Verification context
This review is read-only over `develop @ 72829b1`. Migration
ordering verified by `ls prisma/migrations/`. The findings are
based on diff-reading and selective grep, not on running the test
suite — every chunk's report claims green
`pnpm typecheck / lint / test --run / test:integration / openapi:check`,
and I trust those reports. Where a finding cites a line number the
referenced file was read in full (or the relevant 80-line window)
during this review.
.planning/phase-W6-v1423-simplify-review.md
+198
View File
@@ -0,0 +1,198 @@
# v1.4.23 Wave 6 — Simplify review
Reviewed: develop @ 72829b1. Scope per the brief:
- W2 — `apple-health-mapping.ts`, `measurements/batch/route.ts`, sleep-stage analytics in `analytics/route.ts`
- W3 — `senders/apns.ts`, dispatcher cascade extension, `devices/route.ts`
- W4 — `openapi/registry.ts`, `auth/refresh-token.ts`, `me/devices/*`, Coach prompt-builder reading new prefs (`snapshot.ts`, `system-prompt.ts`)
- W5 — sentinel-parser obs (`keyvalues.ts`), chunked-aggregate (`fetchBpSeriesChunked`), controlled-prop drawer (`coach-drawer.tsx`), coach-prefs route + UI (`coach-prefs/route.ts`, `coach-settings-sheet.tsx`), thumbs feedback (`messages/[id]/feedback/route.ts`, `message-thread.tsx`, `feedback-aggregator.ts`)
## Summary
Total findings: 5 apply-yes · 4 apply-maybe · 3 apply-no
## apply-yes (clear wins)
### S-01 — Two device-DELETE routes are byte-identical except for one audit detail
**Files**: `src/app/api/devices/[id]/route.ts:26-86` and `src/app/api/auth/me/devices/[id]/route.ts:31-94`
**Why**: Both files run the same five-step cascade (ownership lookup → revoke refresh tokens → revoke paired access tokens → delete device → audit + annotate). The only structural difference is `via: "ios.rotation"` in one audit detail and the action label (`devices.revoke` vs `auth.me.devices.revoke`). ~60 lines duplicated. Extract a `revokeDeviceCascade(userId, deviceId, opts)` helper in `src/lib/devices/revoke.ts` and have both routes call it. Security-adjacent but no decision logic moves — the helper is a pure data-flow extraction. (Reviewed against the brief: this is duplication of _implementation_, not of a security boundary; the cross-user 404 + revoke-then-delete order stays bit-identical.)
**Before** (≤8 lines):
```
// src/app/api/devices/[id]/route.ts AND src/app/api/auth/me/devices/[id]/route.ts both contain:
const liveRefreshTokens = await prisma.refreshToken.findMany({...});
await prisma.refreshToken.updateMany({ where:..., data: { revokedAt }});
const accessHashes = liveRefreshTokens.map(...).filter(...);
if (accessHashes.length > 0) await prisma.apiToken.updateMany({...});
await prisma.device.delete({ where: { id: device.id } });
await auditLog("devices.revoke", { userId, details: {...} });
```
**After** (≤8 lines):
```
// src/lib/devices/revoke.ts
export async function revokeDeviceCascade(userId: string, deviceId: string,
opts?: { auditAction?: string; via?: string }) {
const device = await prisma.device.findUnique({...});
if (!device || device.userId !== userId) return null;
// ... shared cascade body ...
return { id: device.id, refreshTokensRevoked, accessTokensRevoked };
}
// each route shrinks to: const r = await revokeDeviceCascade(...); if (!r) return apiError("Device not found", 404);
```
### S-02 — `openapi/registry.ts` lazy `require()` adds eslint-disable + unsafe cast for no benefit
**File**: `src/lib/openapi/registry.ts:65-75`
**Why**: The doc-block claims the `require()` keeps `openApiBase` "usable from contexts that shouldn't pull every Zod schema". That's hypothetical — the only caller is `scripts/generate-openapi.ts` and `scripts/check-openapi.ts`, both of which call `buildOpenApiDocument()` which always loads `./routes`. The lazy form costs an eslint-disable, an `as` cast, and an extra layer of typing. A static import is cleaner and safer.
**Before** (≤8 lines):
```
export function buildOpenApiDocument(): ReturnType<typeof createDocument> {
// eslint-disable-next-line @typescript-eslint/no-require-imports
const { openApiPaths, openApiComponents } = require("./routes") as {
openApiPaths: ZodOpenApiObject["paths"];
openApiComponents: ZodOpenApiObject["components"];
};
return createDocument({ ...openApiBase, paths: openApiPaths, components: openApiComponents });
}
```
**After** (≤8 lines):
```
import { openApiPaths, openApiComponents } from "./routes";
export function buildOpenApiDocument(): ReturnType<typeof createDocument> {
return createDocument({ ...openApiBase, paths: openApiPaths, components: openApiComponents });
}
```
### S-03 — Coach-prefs query duplicated between settings sheet and message thread
**Files**: `src/components/insights/coach-panel/coach-settings-sheet.tsx:71-80` and `src/components/insights/coach-panel/message-thread.tsx:72-80`
**Why**: Both components ship the same `useQuery({ queryKey: ["coach-prefs"], queryFn: fetch("/api/auth/me/coach-prefs") → env.data })` block, with subtly different fallback shapes (one throws on `!ok`, the other returns `DEFAULT_COACH_PREFS`). Extract a single `useCoachPrefs()` hook in `src/hooks/use-coach-prefs.ts`. Removes ~15 lines and prevents drift on the next prefs surface (settings tab? Insights cog?). The query cache key already standardises behaviour at runtime; the hook just removes the literal duplication.
**Before** (≤8 lines):
```
// settings-sheet AND message-thread each define:
const { data } = useQuery({
queryKey: ["coach-prefs"],
queryFn: async () => {
const res = await fetch("/api/auth/me/coach-prefs");
if (!res.ok) /* one throws, the other returns defaults */;
const env = (await res.json()) as { data: CoachPrefs };
return env.data;
},
});
```
**After** (≤8 lines):
```
// src/hooks/use-coach-prefs.ts
export function useCoachPrefs(opts?: { enabled?: boolean }) {
return useQuery({
queryKey: ["coach-prefs"],
queryFn: async () => {
const res = await fetch("/api/auth/me/coach-prefs");
if (!res.ok) return DEFAULT_COACH_PREFS;
return ((await res.json()) as { data: CoachPrefs }).data;
},
...opts,
});
}
```
### S-04 — Snapshot's nine `wantsX = sources.has("X")` booleans are redundant
**File**: `src/lib/ai/coach/snapshot.ts:302-318` and the `wantedTypes.push(...)` ladder at 323-335
**Why**: The Apple Health table at 517-527 already enumerates `(source, type)` pairs. The block at 302-335 reproduces the same enumeration twice (once as `wantsX = sources.has("X")` booleans, once as `if (wantsX) wantedTypes.push(...)`). Two parallel ladders for the same data. Drive everything from a single `METRIC_TYPE_MAP: Record<CoachScopeSource, MeasurementType[]>` constant — collapse the 13-line `wantedTypes` build into a 3-line `flatMap`, drop the `wantsHrv/wantsSleep/wantsRestingHr/wantsSteps/wantsActiveEnergy/wantsFlights/wantsDistance/wantsVo2Max/wantsBodyTemp` booleans entirely (the `appleHealthBlocks` table already gates on `enabled: sources.has(...)`).
**Before** (≤8 lines):
```
const wantsHrv = sources.has("hrv");
// ...8 more booleans...
const wantedTypes: string[] = [];
if (wantsBp) wantedTypes.push("BLOOD_PRESSURE_SYS", "BLOOD_PRESSURE_DIA");
// ...11 more if-pushes...
```
**After** (≤8 lines):
```
const METRIC_TYPES: Record<CoachScopeSource, MeasurementType[]> = {
bp: ["BLOOD_PRESSURE_SYS", "BLOOD_PRESSURE_DIA"], weight: ["WEIGHT"], pulse: ["PULSE"],
mood: [], compliance: [], hrv: ["HEART_RATE_VARIABILITY"], sleep: ["SLEEP_DURATION"],
resting_hr: ["RESTING_HEART_RATE"], steps: ["ACTIVITY_STEPS"], /* ...etc */
};
const wantedTypes = Array.from(sources).flatMap((s) => METRIC_TYPES[s] ?? []);
```
### S-05 — `messages/[id]/feedback/route.ts` annotates the same action three times
**File**: `src/app/api/insights/chat/messages/[id]/feedback/route.ts:60-167`
**Why**: Five `annotate({ action: { name: "insights.coach.message.feedback" }, meta: {...} })` calls in one handler, all with the same action name and only the `meta.outcome` field varying. Hoist the action name to a single `annotate({ action: ... })` at the top, then use single-arg `annotate({ meta: { outcome: "..." } })` calls (the WideEvent merges them). Saves five `action: { name: ... }` repetitions and makes "what does this route annotate?" a one-line answer.
**Before** (≤8 lines):
```
if (!rl.allowed) {
annotate({ action: { name: "insights.coach.message.feedback" }, meta: { outcome: "rate_limited" } });
return apiError(...);
}
// ...four more identical action-name annotations differing only in `outcome`...
```
**After** (≤8 lines):
```
annotate({ action: { name: "insights.coach.message.feedback" } });
if (!rl.allowed) { annotate({ meta: { outcome: "rate_limited" } }); return apiError(...); }
// ...later sites just annotate the new meta...
annotate({ meta: { outcome: "created", rating: body.rating, providerType: ... } });
```
## apply-maybe (judgment calls)
### S-M1 — `useResettableValue` + `nextResettableValue` in `coach-drawer.tsx`
**File**: `src/components/insights/coach-panel/coach-drawer.tsx:80-113`
**Status**: maybe. The pattern is correct (per React docs) and the comment is excellent. But (a) `nextResettableValue` is exported solely for unit testing yet `useResettableValue` already encodes the same comparison, and (b) the same pattern is duplicated again in `coach-settings-sheet.tsx:87-94` (`lastSeenPersisted` + `if (persisted !== lastSeenPersisted) ...`). Extract `useResettableValue` (and only that hook) into a shared `src/hooks/use-resettable-value.ts` and reuse from both — but it's a _newly-introduced abstraction_ so the brief asks us to give it room. Defer until a third caller appears.
### S-M2 — Snapshot's `byType()` filters a flat array nine+ times
**File**: `src/lib/ai/coach/snapshot.ts:350-356, 359-360, 378, 392, 530`
**Status**: maybe. Each `byType(t)` call walks the full `measurementRows` array (could be a few hundred rows after the type-IN narrowing). Pre-bucket into `Map<string, Row[]>` once and look up by key — O(N) once vs O(N×K). Mild perf win; lookup-shape change. Only worth it if a profile shows snapshot construction near the budget. Defer.
### S-M3 — Apple-Health mapping table's `convertToDbUnit: (v) => v` repeats 11×
**File**: `src/lib/measurements/apple-health-mapping.ts:91-242`
**Status**: maybe. Eleven of thirteen entries spell out `convertToDbUnit: (v) => v`. The doc above the table already calls out the two exceptions (oxygen + body fat). Make `convertToDbUnit` optional in `AppleHealthMapping` and have `mapAppleHealthEntry()` default to the identity. Saves ~11 lines but adds a typing nuance and a subtle "did the author forget?" question for new entries. Marginal.
### S-M4 — `dispatcher.ts` `channelPriority()` switch could be a Map
**File**: `src/lib/notifications/dispatcher.ts:268-291`
**Status**: maybe. A `const PRIORITY: Record<string, number> = { APNS: 0, TELEGRAM: 1, NTFY: 2, WEB_PUSH: 3 }` + `PRIORITY[type] ?? 99` is two lines vs eleven, and the narrative comment block above stays. The switch is technically more grep-friendly when adding a new channel. Coin flip; the brief excludes the dispatcher's _security_ paths but a sort-key map is cosmetic, not security-shaped.
## apply-no (rejected — explanation)
### S-N1 — `apns.ts` sender + `loadApnsConfig` validation ladder
**Why**: Security surface — APNs sender is explicitly excluded by the brief. The `anySet` / `allSet` validation guard, the lazy provider cache, the `PERMANENT_APNS_REASONS` set, and the per-device dead-cleanup loop are all defence-in-depth that should not be reshaped. The 411 LOC carry the contract; leave it.
### S-N2 — `refresh-token.ts` rotation flow
**Why**: Security surface (refresh-token reuse-detection scoped to deviceId in v1.4.23 is the load-bearing change). The `where: { ..., deviceId }` vs `where: { ... }` ternary at line 129-132 reads as deliberate per-device scoping with a documented null-fallback to user-wide; collapsing it would risk losing the safety property the comment explains.
### S-N3 — `keyvalues.ts` sentinel parser
**Why**: Brief explicitly excludes the sentinel parser (security-adjacent — input from LLM). The W5 H1 work added typed `SentinelMalformedReason` codes precisely because the silent-drop path was a problem; the seven-cap defence (1 KB, 8 lines, label cap, value cap, missing-colon, schema, byte-overflow) is exactly the kind of layered guard that should not get cleverer. The block-vs-line malformed-flag union (lines 260-270) reads as deliberate prose, not duplication.
## Notes / push-back
- **`measurements/batch/route.ts:80-92` raw-body `entries.length` check before Zod parse.** Looks duplicative with `batchPayloadSchema.entries.min(1)` but the comment explains it: "distinguish too-many-entries from validation-failed so the client surfaces a useful diagnostic". The error-code (`coach.batch.too_large`) is a real wire contract. Keep verbatim.
- **`measurements/batch/route.ts:226-261` raced-duplicate reconcile path.** Reads heavy but the comment is exemplary and the path covers a real edge case (two batches for the same APNs token landing in the same tick). Idempotency surface — the brief excludes it. Keep.
- **`analytics/route.ts:540-622` weekend medication-compliance shift logic.** The comment "shift each event's scheduledFor and takenAt forward by 7 days so the helper's internal `now` anchor still captures the same logical 30 days" tells the whole story; the duplication of `medicationCompliance30` and `…Previous` is intentional symmetry. Don't fold.
- **`coach-feedback-section.test.tsx` + `feedback-aggregator.ts` `buildCoachFeedbackBuckets`.** Reads cleanly; nothing to simplify. The encoded `coach:tone=warm:verbosity=default` `metricSourceType` carries the slice, the regex parser walks it back out — correct shape for the polymorphic table.
- **`coach-drawer.tsx` `isDefault` scope-equality check (line 190-193).** Three-condition AND with an `every()` reads slightly fiddly but accurately encodes "same window AND same source set". A `setsEqual()` helper would be ~3 lines saved at the cost of a new util — not worth pulling.