From 0f4431cbf1670962dbb638c62294256dfaa7054f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 14 May 2026 22:02:38 +0200 Subject: [PATCH] docs(planning): W21 senior-dev findings --- .planning/research/w21-senior-dev-findings.md | 344 ++++++++++++++++++ 1 file changed, 344 insertions(+) create mode 100644 .planning/research/w21-senior-dev-findings.md diff --git a/.planning/research/w21-senior-dev-findings.md b/.planning/research/w21-senior-dev-findings.md new file mode 100644 index 000000000..cb91116da --- /dev/null +++ b/.planning/research/w21-senior-dev-findings.md @@ -0,0 +1,344 @@ +# W21 senior-dev findings — v1.4.25 + +Reviewer pass over HealthLog `develop` at the v1.4.25 release-candidate +snapshot (commit near `51f23ef3`). Scope: architectural correctness of +the six new Prisma migrations (0051..0059), purity claims on the new +helper modules, edge cases on the multi-day flows (onboarding wizard, +Withings sleep / activity sync, PR detection, Coach prompt versioning, +two-axis source-priority), performance posture on the new list / +section endpoints, and error-handling shape consistency. + +I reviewed read-only. No code changed. + +## Summary + +The schema additions are solid. Every new table carries the right FK +`ON DELETE` posture for the delete-user cleanup story (CASCADE on the +user / medication axis, `SET NULL` on `PersonalRecord.sourceMeasurementId` +where the historical fact has to survive the underlying measurement +deletion). Every new index pairs with a real read path. The Migration +0055 `NULLS NOT DISTINCT` switch on the measurement composite is the +single most important detail in the bundle and it is implemented +correctly. + +The five pure-module claims (`glp1-pk.ts`, +`research-mode-staleness.ts`, `titration/ladder.ts`, +`scheduling/cadence.ts`, `scheduling/compliance.ts`, +`side-effects/taxonomy.ts`) hold up — none of them import the Prisma +client, none of them call `fetch`, none of them read `process.env`, +none of them call `Date.now()` or `new Date()` inside their public +surface. `taxonomy.ts` does import from `@/generated/prisma/client` but +only for the two enum *type* re-exports; no runtime client touch. + +One real concern, two medium-grade observations, a handful of minor +notes. No critical findings — the bundle is safe to deploy. + +## Critical + +None. + +## High + +None. + +## Medium + +**M1 — Migration 0057 `onboarding_step` ships nullable with an +unenforced default on existing rows.** `migration.sql` adds the column +as `INTEGER DEFAULT 0` without `NOT NULL`, so every pre-W14b user row +keeps `NULL`. The Prisma schema also models the field as `Int? +@default(0)`. The wizard page at `src/app/onboarding/[step]/page.tsx` +treats `NULL` as `0` via `clampCurrentStep`, and `POST /api/onboarding/step` +treats `fresh.onboardingStep ?? 0` the same way. The behaviour is +correct, but the schema invariant ("every user has an onboarding step +between 0 and 4") is enforced only in application code. A future-self +joining a new query path that reads `User.onboardingStep` directly will +hit a tri-state (`null` / `0..3` / `4`) where they expect a bi-state +(`0..3` / `4`). Two acceptable resolutions: (a) backfill `onboarding_step = 0` +for `onboarding_completed_at IS NULL` rows and `onboarding_step = 4` for +`onboarding_completed_at IS NOT NULL` rows in a follow-up data +migration, then flip the column to `NOT NULL DEFAULT 0`; (b) keep the +nullable column and add a Prisma-validator decoder helper that +collapses the tri-state at the boundary. Either is fine. Today's risk is +non-zero only because the wizard URL handlers fan out to step pages +each of which has to remember the null-equals-zero convention. + +**M2 — Withings activity sync anchors per-day rows at 23:59:59 UTC, +which mis-buckets for users east of UTC+0.** `src/lib/withings/sync-activity.ts` +parses `YYYY-MM-DD` from the Withings response as +`new Date(\`${yyyymmdd}T23:59:59.000Z\`)`. For a Berlin-resident user +(UTC+1 / UTC+2 across DST), 2026-06-01T23:59:59Z reads as +2026-06-02T01:59:59 local, which the `dayKey()` helper buckets into the +*wrong* local day. The file comment claims "Berlin-timezone day +analytics consume the row through the standard `dayKey()` helper, which +buckets it into the right local day even across DST shifts" — that's +true for negative or zero offsets, not for positive offsets. The W17b/c +sleep sync side does not have this issue because Withings ships unix +seconds for sleep segments. Recommended fix: anchor at noon-UTC of the +reported `YYYY-MM-DD` (resilient to ±12h offsets) or convert through the +user's stored timezone (`User.timezone` exists per the v1.4.15 +per-user-tz migration). Same-machine analytics in dev (Marc, Berlin) +will not surface this, but a v1.5 international user lands a one-day +shift on every activity row. + +**M3 — `pickCanonicalSourceRows` cross-source workout dedup is +deliberately deferred to v1.5 but the same shape applies to +`PersonalRecord` rows seeded from workouts.** Migration 0054's unique +key is `(userId, metricType, metricSlot, achievedAt)` — `source` is not +in the dedup key. The workout-PR loop in +`src/lib/personal-records/pr-detection-worker.ts` writes the row tagged +with the source of the *best* workout it found per slot, and relies on +the unique key to dedup. That's safe for workout-derived PRs in +v1.4.25 (workout cross-source dedup is a known-deferred problem and the +detector reads the canonical workout row per slot). But the +measurement-driven branch with `metricSlot = null` has the NULLS-DISTINCT +problem the comment at line 245 of `pr-detection-worker.ts` already +calls out and the application-level pre-flight already handles. Just +flagging that the worker correctness depends on that pre-flight — the +test in `pr-detection-worker.test.ts` should pin the contract by +exercising "same userId / metricType / null slot / same achievedAt, two +back-to-back invocations" and asserting one row written. Spot-check +recommended. + +## Low + +**L1 — Migration 0058 `research_mode_enabled` ships `NOT NULL DEFAULT +false` while the two companion columns ship NULL.** That's the right +shape (the timestamp + version are recorded only after a positive +acknowledgment) but the `ADD COLUMN IF NOT EXISTS` posture means a +dev-DB that had this column from a prior hand-edit could now have +`NULL` rows for `research_mode_enabled` if the prior shape was +nullable. Production is fine — this migration has never landed before +0059. Worth a `WHERE research_mode_enabled IS NULL` sanity probe on +each staging deploy. + +**L2 — Migration 0054 `personal_records_dedup_key` uses +NULLS-DISTINCT for `metric_slot`.** This is documented and the +detection worker compensates with an application-level pre-flight +`findFirst`. The W16c phase report flagged it. The worker handles it. +The risk on this specific axis is concurrent worker invocations — two +batch ingests landing within the same millisecond, both calling +`enqueuePrDetection`, both pre-flighting before either writes. Today's +PR detection queue has `retryLimit: 3` and `retryBackoff: true` but no +single-flight gate per user. The chance of two workers landing the +exact same `(userId, metricType, null, achievedAt)` is low (you'd need +two backfills running in true parallel) — but if it ever happens, two +rows get written. Acceptable today; consider a `boss.singletonKey` +keyed on `userId` for the queue when v1.5 adds Apple Health backfill +streams. + +**L3 — Migration 0055 silently depends on the legacy index name being +`measurements_user_id_type_measured_at_source_key`.** The file comment +notes this is the Prisma `0001_init` default and was verified via +grep. `DROP INDEX IF EXISTS` makes the migration idempotent even if +that name happens to be wrong, but a wrong name plus `IF NOT EXISTS` +on the new index would leave both the old composite *and* the new +composite in place, with the old NULLS-DISTINCT semantics still +breaking sleep dedup. Recommend a follow-up data check: after each +prod deploy, query +`SELECT indexname FROM pg_indexes WHERE tablename = 'measurements'` +and assert exactly one composite index covering +`(user_id, type, measured_at, source, *)`. + +**L4 — `pg-boss` `pr-detection` queue has no dead-letter handler.** +`retryLimit: 3` + `retryBackoff: true` means a permanently-failing job +exhausts retries and dies silently in the pg-boss archive table. The +30-minute fallback cron re-creates it next sweep. Today's worker only +fails on transient DB errors so this is fine; if v1.5 adds external +API calls to the detection path (e.g. Apple Health workout fetch), a +DLQ + alert becomes necessary. + +**L5 — `OnboardingStepPage` step gating is server-rendered but the +task brief mentioned a "step 4.5 GLP-1 branch" that does not exist in +the codebase.** I searched onboarding for any `4.5`, `glp1Branch`, +`GLP-1` references and found none — the wizard is purely 0..4. Either +the brief is stale or step 4.5 is a future v1.4.26 item. Either way no +bug; just noting the gap so the W21 reviewers don't write tests for a +non-feature. + +## Migrations audited (one paragraph per migration) + +**0051 (`measurement_device_type`).** Single `ALTER TABLE` adding a +nullable `device_type TEXT`. Idempotency comes free with no `IF NOT +EXISTS` because the column is brand-new and the migration runs once. +Default is implicitly NULL. The source-priority analytics layer treats +NULL as "unknown" and the picker falls through to keep every row when +no device-type signal is present (verified in +`src/lib/analytics/source-priority.ts:181-193`). Free-text rather than +enum was the right call; coordinating an enum bump every time a new +device class arrives would have been a sustained tax. No index on the +column — that's correct because the column is a tie-breaker filter, not +a query predicate; the existing +`measurements_user_id_type_measured_at_idx` covers the hot read path. +Safe. + +**0052 (`apple_health_enum_extensions`).** Three `ALTER TYPE ... ADD +VALUE IF NOT EXISTS` statements appending `AUDIO_EXPOSURE_ENV`, +`AUDIO_EXPOSURE_HEADPHONE`, `TIME_IN_DAYLIGHT` to the `measurement_type` +enum. Forward-additive, idempotent, no row backfill needed (no existing +row carries any of the new values, so the type extension is pure +metadata). The W8d phase report's decision to drop `WORKOUT_ROUTE` from +the enum (workouts are first-class on the `Workout` table from +Migration 0053) is the right architectural call — a sentinel enum +value with no corresponding row is dead weight in every downstream +consumer (chart registry, doctor-PDF allow-list, analytics summariser). +Safe. + +**0053 (`workout_and_route`).** Two new tables. `workouts` has the +expected FK posture (`user_id` CASCADE), composite unique on +`(user_id, source, external_id)` with NULL-distinct semantics keeping +manual entries non-colliding, two read-path indexes +(`workouts_user_started_idx`, `workouts_user_sport_started_idx`). The +known-deferred cross-source dedup is correctly flagged in the schema +TODO. `workout_routes` has 1:1 enforcement via `workout_id UNIQUE`, +CASCADE on workout delete, GeoJSON as JSONB to avoid the PostGIS +extension dependency. Safe. + +**0054 (`personal_record`).** `personal_records` table with NEW +`personal_record_direction` enum (MAX | MIN). FK posture: user +CASCADE, `source_measurement_id SET NULL` so the historical fact +survives measurement deletion. Composite unique is +`(user_id, metric_type, metric_slot, achieved_at)` with PG's default +NULLS-DISTINCT semantics; the application-level pre-flight in +`pr-detection-worker.ts` compensates for the `metric_slot IS NULL` +case. Idempotency contract is explicit and the worker uses +`createMany({ skipDuplicates: true })` on top of it. Safe with the +caveat documented in L2. + +**0055 (`measurement_sleepstage_composite`).** Drops the legacy 4-axis +composite, recreates a 5-axis composite with `NULLS NOT DISTINCT` so +non-sleep rows (sleep_stage IS NULL) continue to dedup on the first +four columns. Pin-point-correct fix for the Withings Sleep v2 sync +that writes one row per stage. The PG 15+ syntax is available on the +deploy target (Postgres 16-alpine). The hand-rolled migration is +intentional — Prisma 7 regenerates the index without `NULLS NOT +DISTINCT` and the schema comment correctly warns future-self to +preserve the clause on any `prisma migrate dev --create-only` that +touches this index. Safe. The single soft spot is the silent +dependency on the legacy index name being `..._source_key`; see L3. + +**0056 (`medication_inventory_item`).** New `medication_inventory_state` +enum + `medication_inventory_items` table. FK posture: user + medication +both CASCADE. Two indexes: composite on +`(user_id, medication_id, state)` for the "active inventory for this +medication" UI tile, and `(user_id, expires_at)` for the daily +expire-stale background job. State machine is documented inline in +the migration (ACTIVE → IN_USE → EXPIRED | USED_UP). Persisted +`expires_at` rather than derived is the right trade-off for the cron +index pattern. Safe. + +**0057 (`user_onboarding_step`).** Single `ADD COLUMN IF NOT EXISTS +"onboarding_step" INTEGER DEFAULT 0` on the `users` table. Existing +rows keep NULL (PG behaviour: DEFAULT applies to new inserts only). +The application code (`OnboardingStepPage.clampCurrentStep` and +`/api/onboarding/step` `onboardingStep ?? 0`) treats NULL as 0 +consistently. See M1 for the schema-invariant note. Safe. + +**0058 (`user_research_mode`).** Three columns on `users`: +`research_mode_enabled BOOLEAN NOT NULL DEFAULT false`, +`research_mode_acknowledged_at TIMESTAMP(3)` nullable, +`research_mode_acknowledged_version TEXT` nullable. Idempotent via +`ADD COLUMN IF NOT EXISTS`. The version + enabled pairing is the +right shape — version drift forces re-acknowledgment without losing +the historical acknowledgment timestamp. Safe. + +**0059 (`medication_side_effect`).** Two new enums +(`medication_side_effect_category` with 5 values, +`medication_side_effect_entry` with 21 values), one new table. +FK posture: user + medication both CASCADE. Severity bounded with +`CHECK (severity >= 1 AND severity <= 5)` — DB-side defence in depth +against client-side Zod drift, exactly right. Two indexes: +`(user_id, medication_id, occurred_at)` for the drug-detail timeline, +`(user_id, occurred_at)` for the Coach snapshot aggregation. Both are +real read patterns. The DB stores `category` denormalised even though +the entry → category mapping is fixed in the pure +`taxonomy.ts` — the schema comment correctly notes the API write path +*derives* category from entry rather than trusting the client, which +guards against an inconsistent write slipping through. Safe. + +## Pure-module verification + +All six claimed-pure modules check out: + +- `src/lib/medications/glp1-pk.ts` — imports only from + `@/lib/medications/glp1-knowledge` (the static GLP-1 knowledge + table). No Prisma, no fetch, no `Date.now()`. Every function accepts + `asOf: Date` per the W19c-Backend claim. Pure. +- `src/lib/medications/research-mode-staleness.ts` — no imports at all + beyond the type signatures. Functions take `asOf` explicitly. Pure. +- `src/lib/medications/titration/ladder.ts` — imports from + `@/lib/medications/glp1-knowledge`. Same posture as `glp1-pk.ts`. + Pure. +- `src/lib/medications/scheduling/cadence.ts` — imports + `parseScheduleRecurrence` from `@/lib/medication-schedule`, which is + itself pure (no Prisma, no fetch, no environment reads). Functions + take `asOf` and `anchor` explicitly. Pure. +- `src/lib/medications/scheduling/compliance.ts` — depends on + `cadence.ts`. Pure. +- `src/lib/medications/side-effects/taxonomy.ts` — imports only the + enum *types* from `@/generated/prisma/client`. No runtime Prisma + surface. Pure. + +## Coach prompt versioning + +`PROMPT_VERSION` is a single exported constant at +`src/lib/ai/prompts/insight-generator.ts:34` = `"4.25.0"`. Every other +locale-specific prompt path (`buildNativeCoachPrompt`, the EN body, the +DE body, the system-prompt assembler) imports that constant. No +locale carries a hand-coded version string. Mismatched-version +detection is centralised through the feedback-attribution layer +(`src/lib/ai/feedback-attribution.ts`) which falls back to the current +`PROMPT_VERSION` when the message's stored version is missing — so a +legacy cached snapshot still attributes feedback against the live +version rather than crashing on a null. Sound. + +## Source-priority two-axis (W8c) + +`src/lib/analytics/source-priority.ts` implements the source-axis +ladder + device-type-axis ladder with all the right fallbacks: every +metric × deviceType combination falls through to "keep every row of +this type" when the user's ladder doesn't enumerate any present +deviceType for that metricType (lines 239-244), and the single-row +fast path is taken first (line 176). The default ladder lives at +`src/lib/validations/source-priority.ts` and the `sourcePriorityJson` +column is read on the User row for per-user overrides. Two-axis +implementation correctness checks out. + +## Performance / N+1 + +`/api/medications/[id]/cadence/route.ts` reads schedules + intake +events with a single bounded query each (windowDays clamped to 180); +no per-row fetches. `/api/medications/[id]/side-effects/route.ts` +ships a single `findMany` with the `(user, medication, occurred_at)` +index covering the query path. `/api/personal-records/route.ts` ships +a single `findMany` with the `(user, metric_type, value)` index. No +N+1 in the new endpoints I inspected. + +## Error-handling + +The `safeRequestProp` narrow-catch (introduced W15, narrowed-then-widened +through the FixG/FixH hotfix loop) is correctly scoped to the +NextRequest probe path only. Spot-checking the new W19d/e/f routes: +each uses `apiHandler` (which runs the global narrow-catch under it), +each uses `safeJson` for body parsing (which returns a `{ error }` +shape rather than throwing), each returns `apiError(message, status)` +on the validation-fail branch. Consistent. + +## API contract consistency + +Every new route I read returns `apiSuccess(payload)` which the +`api-response` helper wraps as `{ data: payload }`, matching the +Marc-memory convention `(await res.json()).data`. Error responses go +through `apiError(message, status, code?)` which produces +`{ error: string, code?: string }`. Consistent. + +## Closing + +The bundle is in good shape. M1 (onboarding_step nullable invariant) +is a hygiene item that compounds if left untouched — recommend folding +it into the v1.4.26 schema-tidy phase. M2 (Withings activity sync +positive-offset TZ mis-bucket) is invisible at Marc's home zone but +trips the first international user; worth fixing before the v1.5 iOS +release that opens the door to Apple Health users worldwide. M3 is a +test-coverage tightening rather than a code change. Nothing here +blocks the v1.4.25 deploy.