mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 04:32:42 +00:00
docs(planning): v1.4.40 backlog captures deferred QA findings
Six QA reviewers (senior / security / specialist / ux / product-lead / simplifier) ran on the v1.4.39 diff; their finding docs land here for traceability. The v1.4.40 backlog captures every Medium + Low the reconcile deliberately deferred — F-SEC-M-02 RLS (v1.5), F-M-01 long-window consumer (v1.5), F-M-02 unused tx parameter, F-M-03 partial index post-convergence, F-M-05 RollupBucketRow consolidation, F-M-06 per-agent worktree process note, UX-M1-3 compliance + mood polish, Specialist-M01 UNION arm cleanup, Simplifier dead-exports + duplicate tz helpers, plus low-severity observability polish. UX-M4 is marked obsolete (Specialist-H2 widen closed it).
This commit is contained in:
Marc-André Bombeck committed
1 parent
4e1f2d4694
commit
5329943e09
7 files changed
+1329
No files matched your search
@@ -0,0 +1,172 @@
|
||||
# v1.4.39 QA — Product-Lead Review
|
||||
|
||||
Reviewer: Product Lead (read-only, strategic).
|
||||
Inputs: 5 wave reports (W-MOOD, W-MED, W-SUM, W-WMY, W-SINCE),
|
||||
`.planning/round-v1438-perf-analysis.md`, `git log v1.4.38.8..develop`,
|
||||
`prisma/schema.prisma` diff, `.planning/round-v1439-backlog.md`.
|
||||
|
||||
## Strategic posture
|
||||
|
||||
v1.4.39 lands the v1.4.38 perf audit's §5 P2 + P3 + P4 + P6 in a single
|
||||
release — three new persistence tiers (`mood_entry_rollups`,
|
||||
`medication_compliance_rollups`, `measurement_rollups.sum_value`) plus
|
||||
the read-side helpers that finally consume the WEEK/MONTH/YEAR write
|
||||
amplification the audit called out as "dead weight." The five-wave
|
||||
decomposition tracks the audit cleanly: every wave anchors to a numbered
|
||||
P-item and every wave ships a writer + reader + boot-backfill + tests in
|
||||
the well-trodden pattern v1.4.35 / v1.4.38.5 established. This is the
|
||||
release where the rollup architecture stops being a measurement-only
|
||||
tier and becomes the canonical second layer for mood + medication +
|
||||
cumulative sums.
|
||||
|
||||
The release deliberately stays inside the read-path optimisation lane.
|
||||
No API contract changes, no UI changes, no iOS contract surface. The
|
||||
deferred items (W-SUM A2 swap, W-MED health-score-fast-path hookup,
|
||||
W-WMY slope90, W-WMY slim-slice all-time aggregate) are all internal
|
||||
helper swaps that wire newly-shipped tiers into not-yet-converted
|
||||
consumers — they sit in the v1.4.40 column and don't gate iOS work.
|
||||
Marc's strategic ask ("blazing-fast queries, never throw away raw data")
|
||||
is preserved on both axes: raw tables are untouched, every new tier is
|
||||
a derived second layer with a documented live fallback.
|
||||
|
||||
v1.5 readiness is **green from this release's perspective**. The
|
||||
release ships zero new iOS-facing contracts, the medication-compliance
|
||||
tier already uses a per-user-tz string `day` column (sidestepping the
|
||||
`isNearUtc` blocker that gates the measurement-rollup fast-paths), and
|
||||
W-WMY's `computeLongWindowSummary` is the helper a v1.5 "year in mood"
|
||||
or multi-year trend card can call without any further backend work.
|
||||
The cross-tz proper-fix + per-source rollup + slope90 hookup remain the
|
||||
v1.5 architectural backlog — unchanged from the v1.4.38 closure.
|
||||
|
||||
## Perf claim audit
|
||||
|
||||
| Endpoint | Claim | Evidence in report | Confidence |
|
||||
|---|---|---|---|
|
||||
| `/api/mood/analytics` cold | 12.7 s → ~200 ms | Audit §5 P2 estimate + writer + 21 unit/route tests + bounded 1 800-row read assertion | **Medium** — unit tests prove the read shape, not prod wall-clock. Verify in post-deploy `round-v1439-perf-verify.md`. |
|
||||
| `/api/medications/intake?scope=compliance` cold | 3.2 s → ~200 ms | Audit §5 P4 estimate + 16 unit + 2 route tests + bounded indexed read pattern | **Medium** — audit attributed the 3.2 s mostly to pool stall on cold, not row count. Rollup elides the stall but won't help if pool stall is the dominant residual. Verify in prod. |
|
||||
| `/api/dashboard/summary` cumulative tiles | sparkline now shows true SUM not MEAN | Asymmetric test (SUM=8120 ≠ MEAN×COUNT=8000) pins the column path is wired | **High** — semantic-correctness fix, not a wall-clock claim. |
|
||||
| `/api/measurements?groupBy=day` cumulative path | reads `sum_value` directly, no algebraic re-derivation | Test pin + legacy NULL fallback test | **High** — pure read-shape change, low-risk swap. |
|
||||
| `/api/analytics` live-fallback A2 loop | row bound 347 114 → ~5 000 | `since: liveSince` threaded into every `fetchMeasurementSeriesChunked` call + 3 route tests + integration-test reseed | **High** for row-count bound; **Medium** for the 20-40 s wall-clock estimate (was audit-derived, not measured). |
|
||||
| `/api/analytics` full-slice cold | not directly addressed in v1.4.39 | A2 cumulative-skip (P3 follow-on) deferred to v1.4.40 per W-SUM | **N/A** — the 74.6 s claim from the audit was the v1.4.38.8 fast-path-gate fix's domain; W-SINCE only defends the residual live-fallback path. |
|
||||
| `summaries-slice` long-window | new `computeLongWindowSummary` entry point ready for v1.5 trend card | 5 unit tests + auto-router + fall-through chain | **High** for helper correctness; **No consumer wired in v1.4.39** so zero user-visible win this release. |
|
||||
|
||||
The pattern is consistent across all five waves: rollup-tier
|
||||
correctness is proven by unit + route tests, but **wall-clock win on
|
||||
Marc's tenant is not proven until the post-deploy perf-verify**. This
|
||||
is the standard for the rollup-tier releases (v1.4.35, v1.4.38.5,
|
||||
v1.4.38.7) and the post-deploy verify has caught soft claims twice
|
||||
before (`round-v1436-perf-verify.md`, `round-v1437-perf-verify.md`).
|
||||
The release notes should phrase the wins as "expected" until verified.
|
||||
|
||||
## Release-readiness verdict
|
||||
|
||||
- **Ship as v1.4.39: YES.** All five waves landed, quality gates green
|
||||
per each report, tests delta +80 (4 551 → 4 631), no new API
|
||||
contract, no migration risk (all three migrations additive — new
|
||||
tables + nullable column with legacy-NULL fallback documented in
|
||||
every consumer). The only outstanding concern is the cross-agent
|
||||
commit-attribution drift (see Risk register) which is a process bug,
|
||||
not a release blocker.
|
||||
|
||||
- **Carry-overs to v1.4.40:**
|
||||
- W-SUM `/api/analytics` A2 cumulative-skip — single-route swap to
|
||||
`readCumulativeDaySumsBatch`; audit estimates 20-40 s saved on the
|
||||
full-slice cold path. Highest-impact carry-over.
|
||||
- W-MED `health-score-fast-path.ts:267-303` swap onto
|
||||
`readMedicationCompliance` — estimated 1-2 s on the analytics
|
||||
full-slice cold path.
|
||||
- W-WMY slope90 hookup in `health-score-fast-path.ts` — slope is
|
||||
non-linearly composable across DAY buckets; needs a slope
|
||||
re-derivation research step first.
|
||||
- W-WMY slim-slice all-time aggregate swap — currently reads
|
||||
`granularity = 'DAY'` for the all-time path; could move to YEAR
|
||||
once a sync watermark for WEEK/MONTH/YEAR lands (today they're
|
||||
async via pg-boss, so the swap would risk staleness).
|
||||
- W-MOOD WEEK/MONTH/YEAR reader consumer — tier ships writer-only;
|
||||
a Coach long-window prompt or "year in mood" tile is the v1.4.40
|
||||
consumer.
|
||||
- v1.4.38 backlog items (analytics envelope split, `ensureUserRollupsFresh`
|
||||
retry safety, RX-3, RX-7, RX-8 sparkline polish, QA-4 / QA-6
|
||||
medium items) — none gated on iOS work.
|
||||
|
||||
- **v1.5 readiness:** unblocked.
|
||||
- No new iOS contracts introduced (read-path optimisations only).
|
||||
- Medication-compliance tier uses per-user-tz `day` strings —
|
||||
`isNearUtc` non-Berlin-tenant gating doesn't apply to this tier,
|
||||
which is a small architectural step toward the v1.5 per-user-tz
|
||||
bucketing for the measurement tier (P7).
|
||||
- Cross-tz proper fix, per-source rollup (P5), and slope-window
|
||||
SQL-move (P8) remain the v1.5 architectural backlog — unchanged.
|
||||
- `computeLongWindowSummary` is ready for any v1.5 multi-year UI
|
||||
consumer to call without further backend work.
|
||||
|
||||
## Risk register
|
||||
|
||||
- **Wall-clock perf wins are audit estimates, not measurements** —
|
||||
mitigation: post-deploy `round-v1439-perf-verify.md` against Marc's
|
||||
tenant; release-notes phrase wins as "expected" until verified;
|
||||
rollback to v1.4.38.8 is clean (rollup reads have live fallback).
|
||||
|
||||
- **Cumulative `sum_value` legacy-NULL coverage on Marc's tenant** —
|
||||
the boot-backfill discovery UNION branch handles uncovered rows but
|
||||
every existing row has `sum_value = NULL` until the worker re-folds.
|
||||
Consumers (dashboard sparkline, measurements groupBy=day) carry a
|
||||
documented `mean × count` fallback so the chart never holes; risk is
|
||||
cosmetic delta on tenants where source-priority resolves to a single
|
||||
source (Marc's case via the iOS `dailyStatsExternalId` path → exact)
|
||||
but could over-count on multi-source days until per-source rollup
|
||||
(P5, v1.5) lands. Mitigation: documented in W-SUM report; verify in
|
||||
post-deploy.
|
||||
|
||||
- **Mood rollup UTC-anchored bucketing for non-Berlin tenants** —
|
||||
W-MOOD anchors on UTC midnight (mirroring the measurement-rollup
|
||||
convention). Same `isNearUtc` ±3 h guard applies. Marc's tenant is
|
||||
fine; a future non-near-UTC mood-logger would see day-key drift at
|
||||
the UTC boundary. Mitigation: documented in W-MOOD §"UTC vs
|
||||
TZ-anchored" + pinned in route parity test; v1.5 per-user-tz
|
||||
migration unblocks both tiers together.
|
||||
|
||||
- **Cross-agent commit-attribution drift (recurring process bug)** —
|
||||
W-WMY's `8763b3aa` absorbed W-SUM's test files; W-MOOD's
|
||||
`d15850d5` overwrote W-MED's reminder-worker.ts edits per the
|
||||
Marc-briefing pre-marathon note. This is the **third recurring
|
||||
occurrence** (also flagged in `project_v1437_final_web_release.md`).
|
||||
Test content is intact across both incidents (grep-verified per
|
||||
W-SUM and W-WMY reports), so no functional regression. **Mitigation
|
||||
for the next marathon: per-agent `git worktree` isolation** — this
|
||||
is now a hard requirement, not a nice-to-have. Recommend adding
|
||||
worktree-per-wave to the `release-marathon` skill's standard flow.
|
||||
|
||||
- **`ensureUserRollupsFresh` write storm on first post-deploy cold
|
||||
mounts** — three new tiers means three new boot-backfill discovery
|
||||
queries plus three new per-write hook fan-outs. Per the audit §4
|
||||
this is fire-and-forget so the request itself stays fast, but the
|
||||
worker queue depth will spike for the first hour post-deploy on
|
||||
Marc's tenant (347 k rows × 3 new tiers). Mitigation: existing
|
||||
pg-boss back-pressure handles this; monitor Coolify logs for
|
||||
`rollup-full-backfill` queue depth in the first hour.
|
||||
|
||||
- **Marc's strategic ask preservation — confirmed** — raw
|
||||
`measurements`, `mood_entries`, `medication_intake_events` tables
|
||||
are byte-unchanged in the schema diff. Every new tier is purely
|
||||
derived (`*_rollups` tables) and additive (one nullable column on
|
||||
`measurement_rollups`). Boot backfill is idempotent; rollup wipe on
|
||||
account-delete is in-transaction with the source-row wipe per
|
||||
W-MOOD §self-review item 4-5.
|
||||
|
||||
- **Semver call — minor bump (1.4.39) confirmed** — features added:
|
||||
3 new tables (`mood_entry_rollups`, `medication_compliance_rollups`,
|
||||
`measurement_rollups.sum_value` column), 8 new internal helpers
|
||||
(rollups + reader fns), 1 new analytics row-cap, ~80 new tests.
|
||||
Zero API contract breaks, zero new iOS surface, zero user-visible
|
||||
UI changes. Per Marc's conservative-semver doctrine
|
||||
(`feedback_semver_conservative.md`) this is squarely additive-only
|
||||
features inside an existing minor cycle → **v1.4.39 patch bump is
|
||||
the correct call** (v1.5 stays reserved for the iOS Swift sprint).
|
||||
|
||||
- **Release-marathon retrospective items** — beyond the worktree
|
||||
isolation fix above, no other process bugs surfaced in this
|
||||
marathon. Coolify auto-deploy fix from v1.4.38.4 is still in
|
||||
effect. The five-wave decomposition was clean — each wave was
|
||||
scoped to a single P-item and a small file set with documented
|
||||
"did NOT touch" sections to prevent collisions.
|
||||
@@ -0,0 +1,174 @@
|
||||
# v1.4.39 QA — Security Review
|
||||
|
||||
Scope: `git diff v1.4.38.8..develop` — migrations 0070/0071/0072, mood +
|
||||
medication-compliance rollup helpers, per-write hooks on every mood and
|
||||
medication-intake mutation site, `sum_value` column on the measurement
|
||||
rollup tier, three new pg-boss queues. No new HTTP routes were added.
|
||||
|
||||
## Findings
|
||||
|
||||
### Critical
|
||||
|
||||
_None._
|
||||
|
||||
### High
|
||||
|
||||
_None._
|
||||
|
||||
### Medium
|
||||
|
||||
- **F-SEC-M-01 — `enqueueBootTimeMedicationComplianceBackfill` runs a
|
||||
cluster-wide discovery query on every authenticated coverage-miss
|
||||
request, opening a soft DoS amplification surface.**
|
||||
Path: `src/app/api/medications/intake/route.ts:181` →
|
||||
`src/lib/medications/compliance-rollups.ts:490-551`.
|
||||
On every cache-miss + coverage-miss hit of
|
||||
`/api/medications/intake?scope=compliance`, the live-fallback fires a
|
||||
fire-and-forget `enqueueBootTimeMedicationComplianceBackfill()`. That
|
||||
helper does NOT scope to the caller's user — it issues a single
|
||||
`LEFT JOIN medication_intake_events × medication_compliance_rollups`
|
||||
scan across **every tenant in the database**. pg-boss `singletonKey`
|
||||
collapses the resulting enqueues, so worker load is bounded, but the
|
||||
discovery SQL itself runs unguarded on the request path. An
|
||||
authenticated user iterating coverage-miss requests across many
|
||||
sessions can drive that scan on each hit; on a multi-tenant cluster
|
||||
with millions of intake events the planner walks all of them per
|
||||
request. Containment: gate the enqueue behind an in-process throttle
|
||||
or only fire it when the **caller's** user is uncovered (cheap
|
||||
single-row probe before kicking the cluster-wide discovery). Mood-
|
||||
rollup's analog (`ensureUserMoodRollupsFresh`) already does this — it
|
||||
scopes to a single user.
|
||||
|
||||
- **F-SEC-M-02 — `mood_entry_rollups` and
|
||||
`medication_compliance_rollups` carry no per-user RLS or query
|
||||
enforcement; every read relies on application-layer `userId` filters.**
|
||||
Path: migrations 0070 / 0071, plus every `findMany / findFirst /
|
||||
upsert` in `src/lib/mood/rollups.ts` and
|
||||
`src/lib/medications/compliance-rollups.ts`.
|
||||
Inspected: 100 % of new Prisma reads include `where: { userId }` (or
|
||||
the composite key includes userId). Inspected: 100 % of new `upsert`
|
||||
/ `deleteMany` calls include `userId` in the where clause. No leak
|
||||
found. The pattern matches the existing `measurement_rollups` tier,
|
||||
so this is consistent — but it remains the only tenant-isolation
|
||||
layer for the two new tables. Suggest considering Postgres RLS as a
|
||||
defense-in-depth step before v1.5 multi-org work lands; not blocking
|
||||
for v1.4.39 since the contract matches the pre-existing tier.
|
||||
|
||||
### Low
|
||||
|
||||
- **F-SEC-L-01 — `recomputeMedicationComplianceForEvent` swallows
|
||||
errors silently in best-effort mode; one failure mode (the rollup
|
||||
row drifts permanently out of sync with the source intake table)
|
||||
has no observability past the `annotate({ medication_compliance_
|
||||
rollup_failed: true })` flag.**
|
||||
Path: `src/lib/medications/compliance-rollups.ts:277-289`.
|
||||
No PII is logged (only `error.message`, no medication name, no
|
||||
scheduledFor timestamp). The boot-time backfill plus the per-write
|
||||
hook's idempotency mean a single failed recompute self-corrects on
|
||||
the next mutation that touches the same `(user, medication, day)`
|
||||
tuple. Recommend: a follow-up metric / alert on the annotate flag
|
||||
so silent populator drift surfaces in ops dashboards.
|
||||
|
||||
- **F-SEC-L-02 — `recomputeUserMedicationCompliance` uses
|
||||
`prisma.$queryRaw` (tagged template) with `${safeTz}` interpolated
|
||||
into `to_char(... AT TIME ZONE ${safeTz}, 'YYYY-MM-DD')`.**
|
||||
Path: `src/lib/medications/compliance-rollups.ts:421-430`.
|
||||
Verified: tagged-template form parameterises `${safeTz}` as a bind
|
||||
variable (not string interpolation). `safeTz` is run through
|
||||
`isValidTimezone()` which whitelists against the IANA zone list, so
|
||||
even a "%' OR 1=1" attempt is rejected at the gate. Both belt-and-
|
||||
braces — note for the record but no vulnerability.
|
||||
|
||||
- **F-SEC-L-03 — `runMoodRollupAggregate` uses `$queryRawUnsafe` with
|
||||
an inlined `date_trunc('<unit>', ...)`.**
|
||||
Path: `src/lib/mood/rollups.ts:475-510`.
|
||||
`truncUnit` is sourced from the closed `DATE_TRUNC_UNIT` enum
|
||||
(DAY/WEEK/MONTH/YEAR → `day/week/month/year`) AND validated against
|
||||
a hard-coded whitelist (line 485) before splicing. `userId`, `from`,
|
||||
`to` are passed as `$1`/`$2`/`$3` parameters. No injection surface;
|
||||
mirrors the proven `measurement-rollups` pattern. Logged for the
|
||||
record only.
|
||||
|
||||
### Audit notes (no finding, recorded for completeness)
|
||||
|
||||
- **Migrations 0070 / 0071 / 0072 are additive-only.** No `ALTER` on
|
||||
existing tables beyond the additive `ADD COLUMN IF NOT EXISTS
|
||||
sum_value` on `measurement_rollups`. FKs on
|
||||
`mood_entry_rollups.user_id`, `medication_compliance_rollups.user_id`,
|
||||
`medication_compliance_rollups.medication_id` all carry
|
||||
`ON DELETE CASCADE` to the parent (correct — owner deletion sweeps
|
||||
the cache rows). No `SET NULL` paths that could orphan rows. The
|
||||
`IF NOT EXISTS` + `EXCEPTION WHEN duplicate_object` guards mirror
|
||||
the established 0067/0068/0069 pattern → replay-safe.
|
||||
|
||||
- **Worker queue idempotency:** `MOOD_ROLLUP_RECOMPUTE_QUEUE`,
|
||||
`MOOD_ROLLUP_FULL_BACKFILL_QUEUE`,
|
||||
`MEDICATION_COMPLIANCE_BACKFILL_QUEUE` all coalesce duplicate sends
|
||||
via per-user `singletonKey`. `retryLimit:3 / retryBackoff:true`
|
||||
prevent infinite-loop replay. A poisoned job throws once → pg-boss
|
||||
records the error and dead-letters after 3 retries. The discovery
|
||||
helpers are the only callers that enqueue these; one of them
|
||||
(`enqueueBootTimeMedicationComplianceBackfill`) is reachable from a
|
||||
request path → see F-SEC-M-01.
|
||||
|
||||
- **CSRF / idempotency-key reach intact:** `POST /api/medications/[id]/
|
||||
intake` (line 24-26) wraps `postIntake` in `withIdempotency`. The new
|
||||
`recomputeMedicationComplianceForEvent` hook (line 166-171) fires
|
||||
**after** the idempotency-key short-circuit (line 50-75) and after
|
||||
the `assertMedicationOwnership` privacy gate. The hook also fires
|
||||
after the `$transaction` create, so a duplicate request reuses the
|
||||
existing event without enqueuing a second rollup hit. Verified.
|
||||
|
||||
- **Cross-tenant leakage probes:** I searched every new `findFirst` /
|
||||
`findMany` / `upsert` / `deleteMany` / `$queryRaw` for missing
|
||||
`userId` clauses. All 23 new query sites scope to the authenticated
|
||||
user, either via explicit `where: { userId }`, via the composite
|
||||
primary key, or via a tagged-template `$queryRaw` parameter. No
|
||||
leak found.
|
||||
|
||||
- **Per-write hook PII surface:** The new `annotate` calls log only
|
||||
`mood_rollup_*_failed: true` / `medication_compliance_rollup_*_failed:
|
||||
true` boolean flags plus the JS `Error.message`. No mood text, no
|
||||
medication name, no `moodLoggedAt` / `scheduledFor` timestamp ever
|
||||
reaches the log payload from the v1.4.39-added sites. (Pre-existing
|
||||
backup-creation payload at `reminder-worker.ts:1517-1530` does log
|
||||
`medication.name` + `mood`, but that's untouched here and out of
|
||||
scope for this review.)
|
||||
|
||||
- **Telegram + moodlog + ingest auth paths:** The Telegram webhook
|
||||
hook fires only after `findTelegramUser` resolves a user with a
|
||||
valid `telegramBotToken`. The moodlog webhook hook fires after the
|
||||
X-Webhook-Secret timing-safe compare succeeds. The ingest hook
|
||||
fires after `hashToken` validates the bearer + the scope check
|
||||
passes. None of the new hooks bypass auth.
|
||||
|
||||
- **No new HTTP endpoints.** `git diff --diff-filter=A -- 'src/app/api/
|
||||
**/route.ts'` returned zero new route files. All route changes are
|
||||
modifications adding hooks to existing endpoints, every one of which
|
||||
is auth-gated by the pre-existing `requireAuth` / `withIdempotency`
|
||||
/ `apiHandler` chain.
|
||||
|
||||
## Brief-back (≤200 words)
|
||||
|
||||
No Critical or High findings. The v1.4.39 surface is tenant-scoped end
|
||||
to end: every new Prisma query carries an explicit `userId` clause,
|
||||
every new raw-SQL fragment is parameterised, the IANA-zone gate added
|
||||
in v1.4.38 protects the new `to_char(... AT TIME ZONE)` splice, and
|
||||
the three new migrations are additive with correct CASCADE FK
|
||||
behaviour. Per-write hooks fire AFTER idempotency / auth / privacy
|
||||
gates on every site I inspected (medications intake POST is the canary
|
||||
— hook lands at line 166, well after the auth + ownership + dedup
|
||||
short-circuits). Worker queues coalesce via `singletonKey` and retry
|
||||
with backoff.
|
||||
|
||||
One Medium worth fixing before ship: F-SEC-M-01 —
|
||||
`enqueueBootTimeMedicationComplianceBackfill` runs a cluster-wide
|
||||
discovery `LEFT JOIN` on every coverage-miss request. Mood's analog
|
||||
already scopes to a single user; the medication path should mirror
|
||||
that. Cheap one-line fix: probe the caller's `userId` first, only
|
||||
enqueue boot-backfill when they specifically lack coverage.
|
||||
|
||||
Two Lows are observability-only (silent rollup-drift annotate +
|
||||
follow-up RLS consideration). Three audit-notes captured for the
|
||||
record. Cleared to ship after F-SEC-M-01 is decided (mitigate or
|
||||
accept).
|
||||
@@ -0,0 +1,214 @@
|
||||
# v1.4.39 QA — Senior-Dev Review
|
||||
|
||||
Independent code-review pass on the full diff `v1.4.38.8..develop`
|
||||
(commits `0d33f91d` → `bbcb09e8`, 50 files, +6 404 / −26). Read-only.
|
||||
|
||||
Anchors:
|
||||
- Audit: `.planning/round-v1438-perf-analysis.md`
|
||||
- Wave reports: `phase-W-{MOOD,MED,SUM,WMY,SINCE}-v1439-report.md`
|
||||
|
||||
`pnpm typecheck` passes clean on HEAD.
|
||||
|
||||
## Severity bands
|
||||
|
||||
- **Critical** — release-blocking (data loss, crash, perf regression > 50 %).
|
||||
- **High** — must-fix before tag (parity break, edge case that wedges power users, hidden lint/type errors).
|
||||
- **Medium** — should-fix in v1.4.39 if cheap, otherwise v1.4.40 backlog.
|
||||
- **Low** — cosmetic, naming, dead code.
|
||||
|
||||
## Findings
|
||||
|
||||
### Critical
|
||||
|
||||
(none)
|
||||
|
||||
The four migrations are additive, write hooks fall back to a working
|
||||
legacy path on miss, and `typecheck` is clean. No release-blocking
|
||||
issue surfaced. Recommend ship after the High items below are
|
||||
triaged (the two parity / race concerns can be deferred with a
|
||||
documented v1.4.40 backlog entry).
|
||||
|
||||
### High
|
||||
|
||||
- **F-H-01** — `hasMedicationComplianceCoverage` returns `true` on
|
||||
partial coverage and serves zero-filled buckets for un-rolled
|
||||
days — `src/lib/medications/compliance-rollups.ts:373-389`.
|
||||
The probe finds ANY rollup row inside the trailing window and
|
||||
flips to the rollup path. If the boot backfill has minted only
|
||||
the first few days (long-running fold on a multi-medication
|
||||
legacy account) the probe returns `true` while days N..days-1
|
||||
are still zero-filled. The user sees a "compliance dropped to 0
|
||||
%" tile for the un-rolled days until the backfill finishes. The
|
||||
legacy `buildComplianceBuckets` would have returned the correct
|
||||
values because it reads `medication_intake_events` directly.
|
||||
**Fix**: tighten the probe to compare the count of rolled days
|
||||
vs. expected days, or persist a per-user `compliance_backfilled_at`
|
||||
watermark and gate the swap on it.
|
||||
|
||||
- **F-H-02** — Read-after-write race in mood + compliance DAY
|
||||
recompute — `src/lib/mood/rollups.ts:173-212`,
|
||||
`src/lib/medications/compliance-rollups.ts:177-252`. The DAY
|
||||
recompute issues `SELECT … aggregate` followed by an `UPSERT`
|
||||
without serialisation. Two concurrent writes for the same
|
||||
`(user, day)` interleave as: A SELECT → B SELECT → B UPSERT
|
||||
(correct N) → A UPSERT (stale N-1). The stale row stays until
|
||||
the next write or boot backfill. Low probability on mood
|
||||
(1/day human writes) but realistic on medication compliance
|
||||
where the reminder worker mints fresh `scheduledFor` rows while
|
||||
a user is acking via Telegram/web. The compliance rollup row
|
||||
serves the dashboard tile so the wrong "scheduled / taken"
|
||||
count is user-visible. **Fix**: wrap the recompute body in a
|
||||
`SELECT … FOR UPDATE` against a sentinel row, or convert the
|
||||
fold into a single SQL upsert that re-aggregates inside the
|
||||
upsert subquery (`INSERT INTO … SELECT … FROM … ON CONFLICT …`).
|
||||
Doable as v1.4.39 hotfix (~2 h) or deferred to v1.4.40 with a
|
||||
documented "best-effort tier" caveat.
|
||||
|
||||
- **F-H-03** — `enqueueMoodRollupRecompute` fans out 3 enqueues
|
||||
per mood write but the queue's `singletonKey` includes
|
||||
`from.toISOString()`, which differs per granularity bucket —
|
||||
fine. But the helper is `await`-fanned inside
|
||||
`recomputeMoodBucketsForEntry`, so a slow pg-boss send adds
|
||||
latency to the user's mood-write response.
|
||||
`src/lib/mood/rollups.ts:206-211`. The audit's `Promise.all`
|
||||
intent is preserved, but the enclosing `await` still serialises
|
||||
the route on pg-boss latency. The measurement-rollups path has
|
||||
the same pattern (already shipped). **Fix**: drop the outer
|
||||
`await` on the Promise.all and fire-and-forget the enqueue —
|
||||
the queue is a hint, not a write-path invariant, and the DAY
|
||||
pass that the read path actually depends on already committed
|
||||
synchronously above.
|
||||
|
||||
### Medium
|
||||
|
||||
- **F-M-01** — `computeLongWindowSummary` is exported but never
|
||||
consumed — `src/lib/analytics/summaries-slice.ts:615`. W-WMY
|
||||
shipped the helper + 5 tests for a v1.5 multi-year card that
|
||||
doesn't exist yet. Dead-but-tested code. Confirmed deferred in
|
||||
the W-WMY report; flag so the v1.5 plan keeps the consumer on
|
||||
the radar.
|
||||
|
||||
- **F-M-02** — `MoodEntryRollup` writer accepts a `tx` parameter
|
||||
(`src/lib/mood/rollups.ts:173-178`) but no call site passes one.
|
||||
Same for `MedicationComplianceRollup`. The atomic-with-parent
|
||||
story documented in the wave reports is therefore unrealised —
|
||||
every rollup write happens AFTER the parent commits. Acceptable
|
||||
for a cache tier, but the comments overstate the safety.
|
||||
**Fix**: drop the `tx` parameter or wire it through the
|
||||
`prisma.$transaction` blocks that exist (e.g. the Telegram skip
|
||||
webhook).
|
||||
|
||||
- **F-M-03** — `sum_value IS NULL` boot-backfill discovery branch
|
||||
has no supporting index — `src/lib/measurements/rollups.ts:762-780`
|
||||
+ `prisma/migrations/0072_v1439_rollup_sum_value/migration.sql`.
|
||||
The UNION branch `WHERE r2."granularity" = 'DAY' AND
|
||||
r2."sum_value" IS NULL` does a sequential scan of
|
||||
`measurement_rollups`. For Marc's single-tenant install it's
|
||||
cheap (~few thousand rows). For multi-tenant scaling it
|
||||
becomes a problem. **Fix**: add a partial index `CREATE INDEX
|
||||
CONCURRENTLY ON measurement_rollups(user_id) WHERE granularity
|
||||
= 'DAY' AND sum_value IS NULL` once the legacy NULL backfill
|
||||
finishes the first sweep (the index is only useful while there
|
||||
are still NULL rows).
|
||||
|
||||
- **F-M-04** — Integration test `analytics-bp-aggregate-paged.test.ts`
|
||||
is now wall-clock-anchored at `Date.now()` and seeds 6 000 rows
|
||||
at 15-min intervals (~62.5 days). 90-day cap leaves 27.5 days
|
||||
of headroom; once CI runs ≥ 28 days after the seed timestamp
|
||||
reaches the floor the test fails. **Fix**: re-anchor the seed
|
||||
to `nowMs - 30 * 86_400_000` so the youngest row is always 30 d
|
||||
old, keeping the test stable across time.
|
||||
|
||||
- **F-M-05** — `RollupBucketRow` shape divergence between
|
||||
`rollup-read.ts` (`DailyMeanRow`) and `rollup-read-wmy.ts`
|
||||
(`RollupBucketRow`). The latter carries `sd/slope/r2/sumValue`
|
||||
the former does not. Two coexisting bucket-row types invite
|
||||
drift. **Fix**: consolidate into one shape in
|
||||
`rollup-read.ts` and re-export from `rollup-read-wmy.ts`. Cheap
|
||||
in v1.4.40.
|
||||
|
||||
- **F-M-06** — Cross-agent commit attribution drift visibly hit
|
||||
this marathon (W-SUM's tests landed inside W-WMY's commit
|
||||
`8763b3aa`; see W-SUM report "Surprises"). No broken state, but
|
||||
it makes `git bisect` and `git log -p src/lib/measurements/`
|
||||
harder to follow. Same pattern as v1.4.37. **Fix**: per-agent
|
||||
`git worktree` in the next marathon (the marathon kickoff
|
||||
guidance already nods at this).
|
||||
|
||||
### Low
|
||||
|
||||
- **F-L-01** — `recomputeMedicationComplianceForDay` computes the
|
||||
day window twice via `start + 86_400_000` then via
|
||||
`startOfDayUtcInTz(nextDayKey)` and picks the larger — both
|
||||
helpers run unconditionally — `src/lib/medications/compliance-rollups.ts:186-199`.
|
||||
The simpler invariant is "always use `startOfDayUtcInTz` for
|
||||
both bounds". Cosmetic cleanup.
|
||||
|
||||
- **F-L-02** — `readMedicationCompliance` adds the special-case
|
||||
`if (!dedup.has(todayKey)) orderedKeys.push(todayKey)`
|
||||
(`src/lib/medications/compliance-rollups.ts:330-332`). The
|
||||
DST-edge-case it guards against (`days===1` on a DST boundary)
|
||||
is unreachable from production callers (route minimum is
|
||||
`days≥7`). Dead defensive code — fine to keep, worth a
|
||||
one-line comment that it's a belt-and-braces guard.
|
||||
|
||||
- **F-L-03** — Mood-rollup `persistMoodRollupRows` chunks at 500
|
||||
but issues a sequential `for…of` upsert loop inside each
|
||||
chunk — `src/lib/mood/rollups.ts:526-560`. The
|
||||
measurement-rollups equivalent uses `prisma.$transaction([…])`
|
||||
for parallel upsert. The mood DAY-hot-path writes 1 row so it
|
||||
doesn't matter, but a 5-year boot-fold pays N round-trips
|
||||
instead of N/500 transactions.
|
||||
|
||||
- **F-L-04** — `MOOD_ROLLUP_FULL_BACKFILL_QUEUE` discovery
|
||||
query (`src/lib/mood/rollups.ts:417-427`) doesn't restrict
|
||||
`granularity='DAY'` on the LEFT JOIN — same shape as the
|
||||
measurement-rollups query, which intentionally bridges all
|
||||
granularities. Inconsequential because the read path only
|
||||
consults DAY anyway. Comment clarification would help future
|
||||
reviewers.
|
||||
|
||||
- **F-L-05** — `mood/rollups.ts` typo: `MoodRollupRow.sd` is
|
||||
typed `number | null` but the SQL aggregator returns
|
||||
`STDDEV_POP` which is `0` (not `NaN`) for a single-row bucket.
|
||||
The downstream consumer never asserts SD on single-entry days
|
||||
so this is invisible. Worth a doc-comment.
|
||||
|
||||
- **F-L-06** — Marc-voice + no-PII check on the new diff: commit
|
||||
messages are English, lowercase scope, no Co-Authored-By, no
|
||||
emojis. Source comments mention "Marc" exactly once
|
||||
(`src/lib/measurements/rollup-read-cumulative.ts:28-30`) which
|
||||
is the in-code planning anchor, not user-facing copy. Compliant.
|
||||
|
||||
## Brief-back (≤200 words)
|
||||
|
||||
**Top 3 findings**:
|
||||
|
||||
1. **F-H-01 (High)** — `hasMedicationComplianceCoverage` can
|
||||
short-circuit to the rollup path while only some days are
|
||||
backfilled, exposing zero-filled tiles for the un-rolled days
|
||||
until the boot fold completes. Realistic on legacy accounts
|
||||
restoring v1.4.39 mid-deploy. Either tighten the probe or gate
|
||||
the swap on a per-user backfill watermark.
|
||||
|
||||
2. **F-H-02 (High)** — Read-aggregate-then-upsert race in the
|
||||
mood + compliance DAY recomputes. Concurrent writes for the
|
||||
same `(user, day)` can leave a stale row until the next write.
|
||||
Mood is human-paced so low risk; medication compliance under
|
||||
reminder-worker + Telegram contention is the realistic blast
|
||||
radius. Convert to a single SQL upsert or `FOR UPDATE` sentinel.
|
||||
|
||||
3. **F-H-03 (High)** — `recomputeMoodBucketsForEntry` awaits the
|
||||
WEEK/MONTH/YEAR pg-boss enqueue inside the write-path response
|
||||
cycle. Drop the outer `await` and fire-and-forget; the DAY
|
||||
pass the read path actually depends on already committed.
|
||||
|
||||
**Overall posture**: clean architecture, additive migrations,
|
||||
test coverage proportional to scope, typecheck green. Two
|
||||
real-world races + one perf-probe gap on partial backfill are
|
||||
the only correctness concerns. Cache-tier "best-effort" framing
|
||||
in the wave reports is defensible.
|
||||
|
||||
**Ship-readiness**: ship as v1.4.39.0 with F-H-01 + F-H-02 +
|
||||
F-H-03 documented in `.planning/round-v1439-backlog.md` for
|
||||
v1.4.39.1 hotfix or v1.4.40. None block the release.
|
||||
@@ -0,0 +1,32 @@
|
||||
# v1.4.39 QA — Simplification Findings
|
||||
|
||||
Reviewed diff: `git diff v1.4.38.8..develop`. Focus on dead helpers, near-duplicate code, and prunable fallbacks introduced by the W-MOOD / W-MED / W-SUM / W-WMY / W-SINCE waves.
|
||||
|
||||
No automated edits were applied — every candidate below is either consumed by tests as a contract anchor, kept around as an intentional v1.5 hook, or non-trivial enough to warrant Marc's sign-off before deletion.
|
||||
|
||||
## Suggested removals
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:54` — `isCumulativeType` — exported, tested, but no production caller; `/api/dashboard/summary/route.ts` reads `CUMULATIVE_HK_TYPES` directly and `/api/analytics/route.ts` uses `isCumulativeDaySumType` from `cumulative-day-sum.ts`. The shape was added "for future routes" by W-SUM but never wired — confidence: likely.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:83` — `readCumulativeDaySums` — exported and tested but no production caller. The cumulative tile flow inlines `sum_value` off `SparklineRow` instead — confidence: likely.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:121` — `readCumulativeDaySumsBatch` — exported and tested but no production caller. The W-SUM phase-report frames this as the future analytics-route batch reader; today `/api/analytics` still routes through `pickCumulativeDaySum` on live measurements — confidence: likely.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:164` — `resolveBucketSum` — exported and tested but no production caller; the inline `sum_value ?? mean * count` fallback in `dashboard/summary/route.ts` reimplements the same shape. Once a caller exists this should be the single source of truth, but right now it is dead — confidence: likely.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/summaries-slice.ts:615` — `computeLongWindowSummary` — only the file's own test suite imports it. The phase report flags it as the v1.5 multi-year-card hook; until that card lands the function ships dead in the production bundle — confidence: certain (no callers in `src/`).
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-wmy.ts:125,137,149` — `readWeekRollups` / `readMonthRollups` / `readYearRollups` — only the wmy reader's own tests import them; the production `readBestGranularityRollups` walker uses the internal `readGranularity`. Either delete the three thin wrappers and inline the call into the tests, or downgrade them to non-exported helpers — confidence: certain.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:453` — `ensureUserMedicationComplianceFresh` — the only caller is its own JSDoc reference. The coverage-probe → boot-backfill cascade fully owns the cold-mount path; this "kept for symmetry" wrapper is dead today — confidence: certain.
|
||||
|
||||
## Suggested dedups
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/mood/rollups.ts:571` — `startOfUtcDay` — byte-identical to `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:524`. Suggested action: lift to a shared `src/lib/time/start-of-utc-day.ts` (or re-export from `tz/format`) and have both rollups files import from it. The Postgres `date_trunc` parity comment stays meaningful in one place.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/mood/rollups.ts:583` — `bucketSpan` — structurally identical to `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:537` (only the variable name differs: `measuredAt` vs `moodLoggedAt`). Suggested action: extract a generic `bucketSpanForUtcInstant(instant: Date, granularity: RollupGranularity)` and share it. WEEK / MONTH / YEAR branches are line-by-line clones.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:73` — `wallClockInTz` — third copy of the same helper alongside `src/lib/medications/scheduling/cadence.ts:107` and `src/lib/tz/format.ts` (private). Suggested action: export the canonical one from `lib/tz/format.ts` and delete the two private copies. Marc-Voice rationale: "one tz helper, one bug fix radius."
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:96` — `tzOffsetMinutes` — third copy alongside `src/lib/tz/format.ts:225` and `src/lib/medications/scheduling/cadence.ts:149`; `src/lib/measurements/drain-per-sample-cumulative.ts:126` has a fourth variant (`tzOffsetMinutesAt`). Suggested action: keep the `tz/format.ts` implementation and have the other three import it.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:126` — `startOfDayUtcInTz` — bespoke two-pass convergence. `lib/tz/format.ts` already exports `userDayKey`; a `startOfUserDay(dayKey, tz)` companion would let this file shed its private DST helper.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-wmy.ts:204` — `aggregateWmyBuckets` — near-duplicate of `src/lib/measurements/rollup-read.ts:59` `aggregateBuckets`. The new version is a strict superset (adds `sum`). Suggested action: extend `aggregateBuckets` to carry `sum`/`sumValue` and delete `aggregateWmyBuckets`; or delete `aggregateBuckets` and have its single caller in `rollup-read.ts` consume the WMY one. Keeping two near-identical reducers invites a future drift bug.
|
||||
|
||||
## Suggested simplifications
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:177` — `recomputeMedicationComplianceForDay` re-derives `nextDayKey` via an IIFE that builds a probe-`Date`, calls `wallClockInTz`, and re-pads its components. Once `startOfDayUtcInTz` accepts a "next day" mode (or via a `+1 day` helper) this 8-line block collapses to one call. Behaviour-preserving.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:751` — boot-backfill discovery `$queryRaw` now `UNION`s the per-type-missing branch with the legacy-NULL `sum_value` branch. Once Marc's tenant has converged (per the W-SUM rollout plan), the legacy-NULL branch becomes a permanent no-op walk over the full rollup table. Suggested action: schedule the `OR r2."sum_value" IS NULL` branch for removal in v1.4.40 + add a TODO with the cut-off date — keeps the discovery query single-table after convergence.
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/analytics/route.ts:241` — `ANALYTICS_LIVE_WINDOW_DAYS = 90` and its 30-line provenance comment live inside `buildAnalyticsResponse`. Hoist to a module-level constant alongside the other thresholds so a future tuner does not have to scroll into the handler body. Behaviour-preserving.
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/summary/route.ts:441` — the cumulative-vs-spot `useSum` branch is a nested ternary `useSum ? (row.sum_value !== null ? Number(row.sum_value) : Number(row.mean) * Number(row.count)) : Number(row.mean)`. Per CLAUDE.md "no nested ternaries"; lift the inner branch into a named local: `const cumulativePoint = row.sum_value !== null ? Number(row.sum_value) : Number(row.mean) * Number(row.count); const point = useSum ? cumulativePoint : Number(row.mean);`. Or call the (currently dead) `resolveBucketSum` once it is wired.
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts` — once the four exports are either wired or removed, the file plus its 195-line test file collapses by ~360 lines. The W-SUM phase-report's "ready-for-v1.5" framing should be reflected by deletion-with-revival-PR rather than dead exports living in `develop`.
|
||||
|
||||
## Applied
|
||||
None — every candidate above either (a) intersects test contracts that should be migrated together, (b) sits behind a v1.5 wiring intent Marc signed off on in the phase reports, or (c) is a cross-file dedup whose target module needs design input. Listing as suggestions keeps Marc's voice in the eventual commit message.
|
||||
@@ -0,0 +1,310 @@
|
||||
# v1.4.39 — QA findings — Prisma + performance specialist
|
||||
|
||||
Read-only review of `git diff v1.4.38.8..develop` against the five wave
|
||||
reports (W-MOOD / W-MED / W-SUM / W-WMY / W-SINCE) and the v1.4.38 perf
|
||||
audit. Severity-band structure.
|
||||
|
||||
---
|
||||
|
||||
## Critical
|
||||
|
||||
None.
|
||||
|
||||
The three migrations (`0070_v1439_mood_rollups`,
|
||||
`0071_v1439_medication_compliance_rollups`,
|
||||
`0072_v1439_rollup_sum_value`) are correct and non-blocking under
|
||||
PostgreSQL ≥ 11:
|
||||
|
||||
- `0072` is `ADD COLUMN IF NOT EXISTS … DOUBLE PRECISION` with no
|
||||
`DEFAULT` and `NULL`-able. PG 11+ treats this as a catalog-only DDL
|
||||
(no table rewrite) — production rollout holds `ACCESS EXCLUSIVE` on
|
||||
`measurement_rollups` for milliseconds, writes resume immediately.
|
||||
- `0070` / `0071` are `CREATE TABLE IF NOT EXISTS` + indexes guarded by
|
||||
`DO $$ BEGIN … duplicate_object` for the FKs. Idempotent on replay.
|
||||
|
||||
Per-write hooks: I confirmed none of the touched routes
|
||||
(`mood-entries/*`, `medications/intake/*`, `medications/[id]/intake/*`,
|
||||
`mood-entries/bulk`, `medications/intake/bulk`, `ingest/medication`,
|
||||
`telegram/webhook`, `import`, `integrations/moodlog/webhook`,
|
||||
`settings/data`, `settings/moodlog`) wraps the entry create/update
|
||||
inside an interactive `prisma.$transaction(async tx => …)`. The
|
||||
admin/backups restore is the only interactive tx; its rollup
|
||||
`deleteMany` correctly uses `tx`, and the long-running
|
||||
`recomputeUserMoodRollups` / `recomputeUserMedicationCompliance` calls
|
||||
correctly run **outside** the tx (audit-doc'd, verified in
|
||||
`src/app/api/admin/backups/[id]/restore/route.ts:280-470`). The
|
||||
snapshot-isolation gotcha you flagged in #2 does not apply.
|
||||
|
||||
---
|
||||
|
||||
## High
|
||||
|
||||
### H1 — Mood rollup DST-boundary case is documented, NOT pinned in a test
|
||||
|
||||
W-MOOD report claims the UTC-bucket vs TZ-anchored `MoodEntry.date`
|
||||
divergence is "pinned in parity test which uses noon-anchored
|
||||
timestamps". The route parity test
|
||||
(`src/app/api/mood/analytics/__tests__/route.test.ts`) uses
|
||||
`moodLoggedAt: 2026-05-{08,09,10}T12:00:00.000Z` — i.e. noon UTC, far
|
||||
from any DST boundary. There is NO test that exercises a Berlin DST
|
||||
boundary case:
|
||||
|
||||
- 2025-03-30T01:30Z (Berlin: 03:30 CEST, after the spring-forward
|
||||
hour) → UTC bucket = 2025-03-30; user-tz bucket = 2025-03-30. OK.
|
||||
- 2025-10-26T01:30Z (Berlin: 02:30 CET, before fall-back hour) → UTC
|
||||
bucket = 2025-10-26; user-tz bucket = 2025-10-26. OK.
|
||||
- 2025-10-25T23:30Z (Berlin: 00:30 CET, AFTER fall-back) → UTC bucket =
|
||||
2025-10-25; user-tz bucket = 2025-10-26. **DIVERGES.**
|
||||
|
||||
For Berlin tenants the slip lands on entries logged 22:00-23:59 UTC,
|
||||
which is 23:00/00:00-00:59/01:59 local depending on DST. The W-MOOD
|
||||
report's noon-anchored test does not cover this. The legacy live path
|
||||
emitted `MoodEntry.date` (TZ-anchored); the rollup tier emits the
|
||||
UTC-midnight bucket. The route test will pass because all its
|
||||
fixtures live mid-day. A real user logging "right before bed" at 23:30
|
||||
local will see the rollup bucket attribute their entry to the wrong
|
||||
calendar day in the chart.
|
||||
|
||||
Severity: High because it's a silent data-mislabelling, not a 5xx.
|
||||
Recommend: add at least one route-parity test with
|
||||
`moodLoggedAt = "2025-10-25T23:30:00.000Z"` and a user `timezone:
|
||||
"Europe/Berlin"` that asserts the bucket lands on `2025-10-26` (or
|
||||
explicitly accepts the documented `2025-10-25` slip).
|
||||
|
||||
### H2 — Live-fallback `since` cap silently nulls `avg30LastYear` on cold mounts
|
||||
|
||||
`src/app/api/analytics/route.ts:240` introduces a 90-day floor on
|
||||
every per-type `fetchMeasurementSeriesChunked` call.
|
||||
`summarize().avg30LastYear` reads from days 365-395 ago, so it now
|
||||
returns `null` whenever the fast-path gate flips false. The W-SINCE
|
||||
report flags this trade-off but the consumer impact is real:
|
||||
|
||||
- `src/app/page.tsx:309` reads `summary.avg30LastYear` when
|
||||
`compareBaseline === "lastYear"` and falls back to `null` →
|
||||
`tileCompareDelta` returns `null` → the tile renders "no prior
|
||||
data".
|
||||
- `src/app/api/insights/generate/route.ts:149` and
|
||||
`src/components/insights/vo2-max-chart-row.tsx:70` do the same.
|
||||
|
||||
When the rollup fast-path covers the user, the rollup branch supplies
|
||||
the long-window numbers via the comprehensive aggregator + summaries
|
||||
slice (also 90-day capped, but `avg30LastYear` already arrives null
|
||||
there — confirmed by reading
|
||||
`src/lib/analytics/summaries-slice.ts:174,410,551`). So this is **not
|
||||
a regression for the rollup-covered common case**; the issue is the
|
||||
live-fallback path strictly worsens.
|
||||
|
||||
Severity: High because the fall-through path is the one users with
|
||||
brand-new measurement types or empty rollups land on, and that's
|
||||
exactly when the "vs last year" tile would be a useful baseline.
|
||||
|
||||
Recommend: either (a) document on the front-end that "vs last year"
|
||||
silently degrades during cold mounts, or (b) leave the cap at 395+30
|
||||
days so the year-ago window remains populated on the fall-through
|
||||
path. The audit's own quick-win bundle anchored on 90 days so option
|
||||
(a) is a fair release-time call, but it should be Marc-Voice noted.
|
||||
|
||||
### H3 — `fetchMeasurementSeriesChunked` `since` filter relies on the existing index
|
||||
|
||||
The new `measuredAt: { gte: liveSince }` filter (`route.ts:697`) plus
|
||||
the existing `orderBy: [{ measuredAt: "asc" }, { id: "asc" }]` is
|
||||
served by the existing `(user_id, type, measured_at)` index on
|
||||
`measurements`. I checked `prisma/schema.prisma:Measurement` — the
|
||||
schema declares the matching `@@index([userId, type, measuredAt])` (no
|
||||
diff in v1.4.39). Plan stays the same: indexed range scan, no seq
|
||||
scan. **No regression.**
|
||||
|
||||
The W-SUM `SUM(m."value")` addition to `runRollupAggregate`'s
|
||||
`$queryRawUnsafe` (rollups.ts:391, 424) is just one more
|
||||
expression in the existing `SELECT` list. The `GROUP BY m."type",
|
||||
date_trunc(...)` clause and the `WHERE user_id = $1 AND type IN (...)
|
||||
AND measured_at >= $2 AND measured_at < $3` predicate are unchanged.
|
||||
Same index path. **No regression.**
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### M1 — `enqueueBootTimeRollupBackfill` UNION branch causes a seq scan on rollup table
|
||||
|
||||
`src/lib/measurements/rollups.ts:762-779`: the second UNION arm
|
||||
|
||||
```sql
|
||||
SELECT DISTINCT r2."user_id" AS id
|
||||
FROM measurement_rollups r2
|
||||
WHERE r2."granularity" = 'DAY'
|
||||
AND r2."sum_value" IS NULL
|
||||
```
|
||||
|
||||
has no index on `sum_value` and no anchoring predicate. On every
|
||||
worker boot, this scans every `granularity = 'DAY'` row in
|
||||
`measurement_rollups` and filters by `sum_value IS NULL`. For Marc's
|
||||
tenant (~5-10k DAY rollup rows) this is millisecond-class. After the
|
||||
column converges across all users, the predicate matches zero rows
|
||||
but the seq scan still runs.
|
||||
|
||||
For v1.5 multi-tenant scale this should either gain a partial index
|
||||
(`CREATE INDEX … WHERE sum_value IS NULL`) or the UNION arm should be
|
||||
dropped once the discovery completes. The discovery is supposed to be
|
||||
idempotent and self-converging (the boot-backfill writer always
|
||||
populates `sum_value`), so a follow-up release can simply drop the arm.
|
||||
|
||||
Cosmetic note: the outer `SELECT DISTINCT id FROM (...) discovery`
|
||||
wrapping a `UNION` is redundant — `UNION` already de-dups. Use `UNION
|
||||
ALL` + outer `DISTINCT` for clarity, or just drop the outer
|
||||
`DISTINCT`. Not a bug.
|
||||
|
||||
### M2 — `medication_compliance_rollups.day` TEXT column lacks a tested DST-day case
|
||||
|
||||
`recomputeMedicationComplianceForDay`
|
||||
(`src/lib/medications/compliance-rollups.ts:177-252`) correctly
|
||||
handles DST 23/25-hour days via the `startOfDayUtcInTz(nextDayKey, …)`
|
||||
two-pass — the helper's `wallClockInTz` + `tzOffsetMinutes` honour
|
||||
`Intl.DateTimeFormat` DST. I verified `tests/integration` and
|
||||
`src/lib/medications/__tests__/compliance-rollups.test.ts` (382
|
||||
lines).
|
||||
|
||||
The unit test file exercises DST-adjacent days but the assertion only
|
||||
checks that the rollup row is written, not that the `[start, end)`
|
||||
window correctly absorbs the 25-hour day's extra hour. For a 03:00
|
||||
local intake event on the Berlin fall-back day, both `start +
|
||||
86_400_000` and the DST-safe `nextDayKey` boundary agree because
|
||||
03:00 is well after the 02:00→03:00 fall-back transition. A test that
|
||||
mints an event at 01:30 local on the fall-back day and asserts the
|
||||
event lands in the **expected** day's bucket would close this gap.
|
||||
|
||||
Severity: Medium — the writer code is correct (I traced
|
||||
`windowEnd = dstSafeEnd.getTime() > start.getTime() ? dstSafeEnd :
|
||||
end`), but the test does not pin the boundary, so a future refactor
|
||||
of the DST helpers could silently regress without the suite catching
|
||||
it.
|
||||
|
||||
### M3 — Compliance scheduler mint path fires the hook per `RED`-phase row, not batched
|
||||
|
||||
`src/lib/jobs/reminder-worker.ts:572-599`: for every medication whose
|
||||
`existingMissed === 0` on a RED-phase scheduled-window, the worker
|
||||
creates one `medicationIntakeEvent` row AND immediately calls
|
||||
`recomputeMedicationComplianceForEvent`. This is the audit's question
|
||||
#5 (`N+1 on scheduler mint?`).
|
||||
|
||||
Across **one user with N medication schedules** firing simultaneously
|
||||
the cost is N independent compliance recomputes (each is one bounded
|
||||
`findMany` plus one upsert). With ~5-10 medications per power user
|
||||
this is bounded and fine.
|
||||
|
||||
Across **all users** the worker iterates all `med` records — but each
|
||||
user's medications fire only on their own RED-phase clock. The fan-out
|
||||
is naturally per-tenant. **Not** an N+1 in the
|
||||
"single-batch-write-fires-N-hooks" sense. Acceptable.
|
||||
|
||||
For Marc's tenant explicitly: at most 4-5 hooks per evening when
|
||||
multiple medications hit their RED window inside the same worker
|
||||
tick. Negligible.
|
||||
|
||||
### M4 — `aggregateWmyBuckets` math is correct (validation)
|
||||
|
||||
I confirmed `src/lib/measurements/rollup-read-wmy.ts:204-240` computes
|
||||
`mean = sumWeighted / totalCount` where `sumWeighted = Σ(count * mean)`
|
||||
and `totalCount = Σ count`. This is the correct `Σ(count·mean) / Σ
|
||||
count`, NOT a naive `mean(mean)`. **Math is right.** The 18 unit
|
||||
pins in `rollup-read-wmy.test.ts` cover the weighted-mean composition
|
||||
(see `aggregateWmyBuckets > composes weighted mean correctly`).
|
||||
|
||||
### M5 — `since` cap in `/api/analytics` does not clip the v1.5 multi-year card
|
||||
|
||||
I grep'd every caller of `fetchMeasurementSeriesChunked` — only
|
||||
`src/app/api/analytics/route.ts:263` uses it. The new
|
||||
`computeLongWindowSummary`
|
||||
(`src/lib/analytics/summaries-slice.ts:615`) routes through
|
||||
`readBestGranularityRollups` (the new WMY reader), which has its own
|
||||
window-day inputs. **No collision.** Finding #7 in your prompt:
|
||||
clear.
|
||||
|
||||
### M6 — Reader fallback discriminator is correct for both new tiers
|
||||
|
||||
Mood rollup: route fallback uses `result.usedRollup`
|
||||
(`src/app/api/mood/analytics/route.ts`) and the route's
|
||||
`ensureUserMoodRollupsFresh` watermark check
|
||||
(`src/lib/mood/rollups.ts:334-374`) reads `newestRollup.computedAt`
|
||||
vs `Max(updatedAt, moodLoggedAt)`. **Robust** — not "0 rollup rows =
|
||||
no data" but "rollup tier may be stale and we know which side to
|
||||
trust".
|
||||
|
||||
Medication rollup: `hasMedicationComplianceCoverage`
|
||||
(`compliance-rollups.ts:373-389`) returns false when no rollup row
|
||||
exists in the trailing window. The route then falls back to
|
||||
`buildComplianceBuckets` (the legacy live aggregator) which is
|
||||
correct over an empty window. The boot backfill fires on the side.
|
||||
**Robust** for the "zero events" and "zero rollup but events exist"
|
||||
cases. The "rollup exists but is stale" case relies on the per-write
|
||||
hooks keeping it fresh — same posture as the measurement rollups.
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### L1 — Cosmetic SQL clean-up in the boot-backfill UNION
|
||||
|
||||
See M1 — `SELECT DISTINCT id FROM (... UNION ...)` wraps a `UNION`
|
||||
which already de-dups. Minor.
|
||||
|
||||
### L2 — `recomputeMoodBucketsForEntry` only flushes WEEK/MONTH/YEAR on enqueue
|
||||
|
||||
Same posture as `measurement-rollups`: the DAY pass is synchronous,
|
||||
the rest is fire-and-forget via pg-boss. Worth noting that the route
|
||||
test parity (which mocks pg-boss) does not assert the WEEK/MONTH/YEAR
|
||||
buckets get enqueued — if a future refactor drops the `Promise.all`
|
||||
fan-out, no test would catch it. Reachable via a `boss.send` spy
|
||||
assertion, not currently included.
|
||||
|
||||
### L3 — Compliance rollup `day` TEXT range scans for trailing window
|
||||
|
||||
`hasMedicationComplianceCoverage` and `readMedicationCompliance` use
|
||||
`day: { gte: oldestKey, lte: newestKey }` with `day` as TEXT. The
|
||||
primary key is `(user_id, medication_id, day)` and the descending
|
||||
index is on `(user_id, day DESC)`. The TEXT comparison is correct for
|
||||
ISO `YYYY-MM-DD` strings (lexicographic == chronological), so the
|
||||
range scan is index-served. **No regression.** Worth noting for
|
||||
future maintenance because TEXT day-keys are unusual in this codebase.
|
||||
|
||||
---
|
||||
|
||||
## Tests delta
|
||||
|
||||
W-MOOD: +21 unit/route tests. W-MED: +18. W-SUM: +8 across 3
|
||||
test files. W-WMY: +23. W-SINCE: +3. Net ~ +73 v1.4.38.8 → develop.
|
||||
|
||||
I sampled the writer tests for parity contracts and they correctly pin
|
||||
the new `sum_value` flow-through, the rollup partition deletion on
|
||||
last-row removal, and the weighted-mean composition.
|
||||
|
||||
---
|
||||
|
||||
## Brief-back (≤ 200 words)
|
||||
|
||||
**No critical issues.** Migrations are non-blocking (column add is
|
||||
catalog-only on PG 11+). Per-write hooks correctly call against the
|
||||
right Prisma client; no `tx` snapshot-isolation traps. `aggregateWmyBuckets`
|
||||
correctly weights mean by count. `since` cap does not clip the v1.5
|
||||
multi-year card (only the legacy analytics route's live-fallback).
|
||||
|
||||
**Two high findings worth Marc's call before tagging:**
|
||||
|
||||
1. **H1** — the mood rollup DST-boundary case is documented but not
|
||||
pinned in a test. The "noon-anchored" parity test misses the
|
||||
23:30-01:00 local-tz boundary that diverges UTC vs user-tz day-keys.
|
||||
Recommend adding one boundary-case route-parity test before
|
||||
release.
|
||||
|
||||
2. **H2** — the `/api/analytics` 90-day `since` cap silently nulls
|
||||
`summary.avg30LastYear` on the live-fallback path, breaking the
|
||||
dashboard / VO2 chart / insights generator's "vs last year"
|
||||
comparison tile when the fast-path gate flips false. The rollup
|
||||
common case is unaffected. Recommend either Marc-Voice user-facing
|
||||
note or widening the cap to ~425 days to preserve the year-ago
|
||||
window.
|
||||
|
||||
**One medium recommendation for v1.4.40:** drop the
|
||||
`measurement_rollups.sum_value IS NULL` UNION arm from the boot
|
||||
backfill once it has converged across all users (M1). Currently runs a
|
||||
seq scan on every worker boot.
|
||||
@@ -0,0 +1,229 @@
|
||||
# v1.4.39 — UX / Response-Shape Review (read-only)
|
||||
|
||||
Reviewer scope: byte-shape parity on the four read-path swaps + the
|
||||
new `computeLongWindowSummary` helper. Diff base
|
||||
`v1.4.38.8..develop`; agent reports
|
||||
`phase-W-MOOD-v1439-report.md`, `phase-W-MED-v1439-report.md`,
|
||||
`phase-W-SUM-v1439-report.md`, `phase-W-WMY-v1439-report.md` read.
|
||||
|
||||
The four target route handlers + test files inspected:
|
||||
- `src/app/api/mood/analytics/route.ts` (+ `__tests__/route.test.ts`)
|
||||
- `src/app/api/medications/intake/route.ts` (+ `__tests__/route.test.ts`)
|
||||
- `src/app/api/dashboard/summary/route.ts` (+ `__tests__/route.test.ts`)
|
||||
- `src/app/api/measurements/route.ts` (+ `__tests__/range-aggregation-route.test.ts`)
|
||||
|
||||
Helpers checked: `readMoodDayRollups`, `readMedicationCompliance`,
|
||||
`hasMedicationComplianceCoverage`, `readBestGranularityRollups`,
|
||||
`computeLongWindowSummary`, `resolveBucketSum`.
|
||||
|
||||
---
|
||||
|
||||
## SEV: HIGH
|
||||
|
||||
### H1 — Mood `summary.count / latest / min / max / mean / avg7 / avg30` semantics shift on multi-entry days (live → rollup divergence)
|
||||
|
||||
File: `src/app/api/mood/analytics/route.ts:97-101`
|
||||
|
||||
The legacy live path called `summarize()` over **per-entry**
|
||||
`DataPoint`s (`moodLoggedAt`, `score`). The new rollup fast-path
|
||||
calls `summarize()` over **per-day mean** `DataPoint`s
|
||||
(`bucketStart`, `r.mean`). For a user with two entries on the same
|
||||
day (score 7 + score 8), the legacy response emitted:
|
||||
|
||||
| field | legacy | rollup |
|
||||
|---------|-------------------|---------------------|
|
||||
| count | total entries | total days |
|
||||
| latest | last entry's score| last day's mean |
|
||||
| min/max | per-entry min/max | per-day-mean min/max|
|
||||
| mean | unweighted avg of all entries | unweighted avg of daily means (Simpson-shift on uneven day counts) |
|
||||
| slope* | OLS over per-entry x = elapsed ms | OLS over per-day x |
|
||||
|
||||
The route comment at lines 91-96 acknowledges this and asserts "power
|
||||
user's mood is typically 1/day" so the divergence is invisible. That
|
||||
assumption is reasonable for Marc's tenant but **is** a contract
|
||||
shift. Consumers on `summary.count` (`src/app/page.tsx:1195` —
|
||||
dashboard sort gate, harmless) and `summary.latest / avg7 / avg30 /
|
||||
slope30` (`src/app/page.tsx:878-883` — `TrendCard` tile) read these
|
||||
fields directly.
|
||||
|
||||
Existing parity test `emits byte-identical entries + summary shape
|
||||
between rollup tier and live fallback` only covers one-entry-per-day
|
||||
data so the divergence is uncovered. Add a multi-entry-per-day pin
|
||||
that either (a) accepts the new semantics with a documented contract
|
||||
or (b) pre-aggregates the live-fallback path through
|
||||
`aggregateDailyAverages` first so the rollup and live shapes match
|
||||
on multi-entry days too.
|
||||
|
||||
Recommendation: option (b) — make the live-fallback walk fold daily
|
||||
means before `summarize()` so the two branches stay byte-identical.
|
||||
Cheap, single map call, removes the silent-shift risk entirely.
|
||||
|
||||
---
|
||||
|
||||
## SEV: MEDIUM
|
||||
|
||||
### M1 — Compliance reader may emit `days + 1` buckets on DST fall-back edge
|
||||
|
||||
File: `src/lib/medications/compliance-rollups.ts:330-332`
|
||||
|
||||
```ts
|
||||
if (!dedup.has(todayKey)) {
|
||||
orderedKeys.push(todayKey);
|
||||
}
|
||||
```
|
||||
|
||||
Defensive "always include today" insert that runs **after** the
|
||||
`for (let i = days - 1; i >= 0; i--)` window builds `orderedKeys`.
|
||||
On a normal day the loop already includes `todayKey`, so the guard
|
||||
is a no-op. On a DST fall-back when the loop dedups two probes onto
|
||||
the same day-key, this branch pushes a fresh entry **outside the
|
||||
trailing window** if the dedup ever drops `todayKey` itself —
|
||||
returning `days + 1` buckets where the legacy route always returned
|
||||
exactly `days`.
|
||||
|
||||
Coverage gap: `compliance buckets for the last N days` parity test
|
||||
pins `body.data.length === 7` on a stable day, not on DST. Live
|
||||
fallback (`buildComplianceBuckets`) always emits exactly `days`. Add
|
||||
a DST-fall-back pin and either drop the defensive insert or cap the
|
||||
final array at `.slice(0, days)`.
|
||||
|
||||
### M2 — Mood `entries[].score` rounding can diverge by ≤0.005 on multi-entry days
|
||||
|
||||
File: `src/app/api/mood/analytics/route.ts:86`
|
||||
|
||||
Legacy: `Math.round((stats.sum / stats.count) * 100) / 100` — JS
|
||||
arithmetic over integer scores.
|
||||
Rollup: `Math.round(r.mean * 100) / 100` where `r.mean` is Postgres
|
||||
`AVG(score)::double precision` — IEEE-754 double through a
|
||||
different precision chain.
|
||||
|
||||
For mood scores (1-5 integers) the two paths produce identical
|
||||
two-decimal rounding in essentially every realistic case, but the
|
||||
intermediate float math is not provably identical. Worth a property
|
||||
test pinning a fixture where JS-AVG and PG-AVG could disagree at the
|
||||
fifth decimal (e.g. seven entries: 3, 3, 3, 4, 5, 5, 5 → 4.0
|
||||
vs 4.0). Currently uncovered; risk is cosmetic (Recharts axis tick
|
||||
shifts on the third decimal).
|
||||
|
||||
### M3 — `entries[].samples` semantics consistent BUT route-meta `entryCount` semantics shift
|
||||
|
||||
File: `src/app/api/mood/analytics/route.ts:102, 139`
|
||||
|
||||
Rollup fast-path: `entryCount = rollups.reduce((s, r) => s + r.count, 0)` — total raw entries across all days.
|
||||
Live fallback: `entryCount = moodEntries.length` — total raw entries.
|
||||
|
||||
These match. Good. **But** `entryCount` is only emitted into
|
||||
`annotate({ meta })` — not into `apiSuccess(...)` — so it never
|
||||
reaches the client. No client-facing shape impact; flagging for
|
||||
audit completeness only.
|
||||
|
||||
### M4 — `/api/analytics` live-fallback 90-day cap shifts `summary.avg30LastYear` to null for power users
|
||||
|
||||
File: `src/app/api/analytics/route.ts:240-269`
|
||||
|
||||
Out of the brief's strict 4-route scope but adjacent: commit
|
||||
`823føbc8 perf(analytics): cap live-fallback findMany at trailing
|
||||
90 days` caps every per-type chunked read at the trailing 90 days.
|
||||
Any user who lands on the live-fallback path (i.e. the rollup
|
||||
coverage probe missed) loses `summary.avg30LastYear` (window
|
||||
365-395 d ago) and `summary.avg30LastMonth` (30-60 d ago is
|
||||
preserved by the 90-day window).
|
||||
|
||||
The route comment at `route.ts:230-239` documents this as an
|
||||
intentional trade-off matching the slim slice's existing 90-day
|
||||
contract. **Shape parity is preserved (the field is still emitted,
|
||||
just `null`)** so the iOS / web consumer code handles it
|
||||
gracefully. Flagged because the marathon brief asked for
|
||||
"pagination / limit preservation" — there is no pre-existing limit
|
||||
on this path, but the new 90-day cap is a behavioural shift the
|
||||
dashboard tile's comparison-overlay will surface.
|
||||
|
||||
---
|
||||
|
||||
## SEV: LOW
|
||||
|
||||
### L1 — `computeLongWindowSummary` not yet exposed via any route (confirms brief)
|
||||
|
||||
File: `src/lib/analytics/summaries-slice.ts:615`
|
||||
|
||||
`computeLongWindowSummary` is exported but **not consumed from any
|
||||
route handler or server-component**. `grep -rn` on `src/app`
|
||||
confirms zero callers. Reserved for v1.5 multi-year trend card and
|
||||
Coach drawer per `phase-W-WMY-v1439-report.md`. No exposure risk.
|
||||
|
||||
### L2 — Locale-sensitive formatting absent on the swap surfaces
|
||||
|
||||
No new code in the four target routes does locale-aware date
|
||||
formatting. Mood rollup labels via `utcDayLabel(d)` which uses pure
|
||||
`getUTC*` calls (no `toLocaleDateString`). Compliance rollup keys
|
||||
use `userDayKey(date, tz)` from `tz/format` — same helper the legacy
|
||||
route used. No `de-DE` vs `en-US` bucket shift risk surfaced.
|
||||
|
||||
### L3 — Backend-only paths emit no user-facing strings
|
||||
|
||||
No `t(...)` / translator calls introduced into the four routes' new
|
||||
code paths. Coach evidence chips, i18n labels, all client-side. No
|
||||
i18n exposure regression.
|
||||
|
||||
### L4 — Sort-order parity holds across all swap surfaces
|
||||
|
||||
- `readMoodDayRollups`: `orderBy: { bucketStart: "asc" }` — matches
|
||||
legacy `aggregateDailyAverages` final `sort([a, b]) =>
|
||||
a.localeCompare(b)`.
|
||||
- `readMedicationCompliance`: builds keys oldest-first then sorts
|
||||
via `orderedKeys` array order. Legacy `buildComplianceBuckets`
|
||||
ended in `.sort((a, b) => a.date.localeCompare(b.date))`. Both
|
||||
produce ISO `YYYY-MM-DD` ascending. Match.
|
||||
- Dashboard `sparkBuckets`: `bucket_start ASC` in raw SQL.
|
||||
Unchanged.
|
||||
- Measurements `source=rollup` branch: `orderBy: { bucketStart:
|
||||
"asc" }`. Unchanged.
|
||||
|
||||
### L5 — Empty-state shape parity holds across all swap surfaces
|
||||
|
||||
- Mood: both branches return `{ entries: [], summary:
|
||||
summarize([]) }` where `summarize([])` is the canonical null
|
||||
envelope (`count: 0, latest: null, ...`). Pinned in test
|
||||
`returns an empty envelope for a brand-new user`.
|
||||
- Compliance: both branches produce zero-filled `days`-length
|
||||
array. Pinned for legacy path; new path falls through to it on
|
||||
coverage-miss.
|
||||
- Dashboard sparkline: `sparkOf(type)` returns `[]` on miss; tile
|
||||
consumer renders empty chart.
|
||||
- Measurements rollup: `rollupRows.length > 0` gate falls through
|
||||
to legacy `date_trunc` path on miss; shape preserved.
|
||||
|
||||
### L6 — Pagination / `limit` preservation
|
||||
|
||||
- Mood: legacy had no limit (unbounded); new path applies a 5-year
|
||||
`since` filter (~1 800 row ceiling). Documented in route comment.
|
||||
Strictly tighter, not a regression — but the legacy
|
||||
"I have 6-year-old mood entries" power user would lose the
|
||||
≥5-year-old tail. Realistically Marc started logging mood in
|
||||
2023 so this is theoretical.
|
||||
- Compliance: legacy passed `days` straight through; new path
|
||||
preserves `days` and clamps at 365 via Zod. Same surface.
|
||||
- Dashboard sparkline: `SPARK_DAYS = 7` unchanged.
|
||||
- Measurements: `take: cap = Math.min(limit, BUCKET_CAP.daily)`
|
||||
unchanged.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Sev | Count | Themes |
|
||||
|-----|-------|--------|
|
||||
| H | 1 | Mood summary shape shifts on multi-entry days (uncovered) |
|
||||
| M | 4 | Compliance DST edge, mood rounding, route-meta, analytics 90 d cap |
|
||||
| L | 6 | Helper not exposed yet, locale, i18n, sort, empty-state, limits |
|
||||
|
||||
The single H finding is **the** UX risk worth blocking on: power-user
|
||||
multi-entry-mood-day tiles will silently shift `latest / mean / avg7
|
||||
/ avg30 / slope30` when the rollup tier converges. The fix is
|
||||
mechanical (pre-aggregate the live-fallback path through
|
||||
`aggregateDailyAverages` before `summarize()`) and the parity test
|
||||
just needs one multi-entry fixture to lock it in.
|
||||
|
||||
All four routes preserve byte-shape for empty state, sort order,
|
||||
limits, locale-independence, and i18n absence. The new
|
||||
`computeLongWindowSummary` helper is correctly NOT exposed.
|
||||
@@ -0,0 +1,198 @@
|
||||
# v1.4.40 backlog
|
||||
|
||||
Seeded during the v1.4.39 QA reconcile. Items the six QA reviewers
|
||||
surfaced that the reconcile deliberately deferred — either too costly
|
||||
for the v1.4.39 release window, or naturally paired with a v1.5
|
||||
architectural step. Source-doc references in parentheses so the next
|
||||
session can re-read the original rationale.
|
||||
|
||||
---
|
||||
|
||||
## High — strategic follow-up
|
||||
|
||||
### F-SEC-M-02 — Postgres RLS for the two new rollup tables
|
||||
(`.planning/round-v1439-QA-security-findings.md` MEDIUM-F-SEC-M-02)
|
||||
- `prisma/migrations/0070_v1439_mood_rollups`,
|
||||
`prisma/migrations/0071_v1439_medication_compliance_rollups` —
|
||||
`mood_entry_rollups` and `medication_compliance_rollups` carry no
|
||||
per-user RLS or query enforcement; every read relies on
|
||||
application-layer `userId` filters.
|
||||
- Inspected: 100% of new Prisma reads include `where: { userId }` and
|
||||
100% of new upsert/deleteMany calls scope to userId. No leak found.
|
||||
- Background: this is consistent with the pre-existing
|
||||
`measurement_rollups` posture, but it remains the only
|
||||
tenant-isolation layer for the two new tables. Worth defense-in-depth
|
||||
RLS before v1.5 multi-org work lands.
|
||||
|
||||
### F-M-01 — `computeLongWindowSummary` consumer wiring (v1.5)
|
||||
(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-01)
|
||||
- `src/lib/analytics/summaries-slice.ts:615` — exported + tested but no
|
||||
production caller. W-WMY shipped the helper for a v1.5 multi-year
|
||||
trend card / Coach drawer that doesn't exist yet.
|
||||
- Action: wire into the v1.5 "year-in-mood" tile or Coach long-window
|
||||
prompt; until then this is dead-but-tested code in the production
|
||||
bundle.
|
||||
|
||||
---
|
||||
|
||||
## Medium — correctness + perf polish
|
||||
|
||||
### F-M-02 — Drop unused `tx` parameter from rollup writers
|
||||
(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-02)
|
||||
- `src/lib/mood/rollups.ts:173-178`,
|
||||
`src/lib/medications/compliance-rollups.ts:177-183` — both writers
|
||||
accept a `tx` parameter but no call site passes one. The
|
||||
atomic-with-parent story in the wave reports is therefore
|
||||
unrealised; every rollup write happens AFTER the parent commits.
|
||||
- Action: drop the `tx` parameter or wire it through the existing
|
||||
`prisma.$transaction` blocks (e.g. the Telegram skip webhook). The
|
||||
v1.4.39 atomic-upsert hotfix (QA F-H-02) replaced the JS aggregate
|
||||
with `$executeRaw`, so the cross-Prisma-client typing matters less
|
||||
now, but the parameter is still dead.
|
||||
|
||||
### F-M-03 — Add partial index `sum_value IS NULL` post-convergence
|
||||
(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-03,
|
||||
`.planning/round-v1439-QA-specialist-findings.md` MEDIUM-M1)
|
||||
- `src/lib/measurements/rollups.ts:762-780` — boot-backfill discovery
|
||||
UNION arm `WHERE r2."granularity" = 'DAY' AND r2."sum_value" IS NULL`
|
||||
does a seq scan over `measurement_rollups`. Negligible at single-
|
||||
tenant scale (~few thousand rows). For v1.5 multi-tenant scale this
|
||||
should either gain a partial index
|
||||
`CREATE INDEX CONCURRENTLY ON measurement_rollups(user_id)
|
||||
WHERE granularity = 'DAY' AND sum_value IS NULL` once the legacy
|
||||
NULL backfill finishes the first sweep, or the UNION arm should be
|
||||
dropped (see Specialist-M01 below).
|
||||
|
||||
### F-M-05 — Consolidate `RollupBucketRow` shape
|
||||
(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-05)
|
||||
- `src/lib/measurements/rollup-read.ts` exports `DailyMeanRow`,
|
||||
`src/lib/measurements/rollup-read-wmy.ts` exports `RollupBucketRow`.
|
||||
The latter is a strict superset (adds `sd/slope/r2/sumValue`). Two
|
||||
coexisting bucket-row types invite drift.
|
||||
- Action: consolidate into one shape in `rollup-read.ts` and re-export
|
||||
from `rollup-read-wmy.ts`. Cheap.
|
||||
|
||||
### F-M-06 — Per-agent worktree for next marathon (process note)
|
||||
(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-06,
|
||||
`.planning/round-v1439-QA-product-lead-findings.md` Risk register)
|
||||
- W-SUM's tests landed inside W-WMY's commit `8763b3aa`; W-MOOD's
|
||||
`d15850d5` overwrote W-MED's reminder-worker.ts edits. Third
|
||||
recurring occurrence of cross-agent commit-attribution drift.
|
||||
- Action: per-agent `git worktree` in the next marathon — add
|
||||
worktree-per-wave to the `release-marathon` skill's standard flow.
|
||||
|
||||
### UX-M1 — Compliance reader DST `days+1` slice
|
||||
(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M1)
|
||||
- `src/lib/medications/compliance-rollups.ts:330-332` — defensive
|
||||
"always include today" insert runs after the trailing-window loop.
|
||||
On a normal day it's a no-op; on a DST fall-back when the loop dedups
|
||||
two probes onto the same day-key, this branch may push a fresh entry
|
||||
outside the trailing window, returning `days + 1` buckets.
|
||||
- Action: add a DST-fall-back pin and either drop the defensive insert
|
||||
or cap the final array at `.slice(0, days)`.
|
||||
|
||||
### UX-M2 — Mood entry-score rounding pin
|
||||
(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M2)
|
||||
- `src/app/api/mood/analytics/route.ts:86` — rollup path uses
|
||||
`Math.round(r.mean * 100) / 100` over PG `AVG`; legacy live uses
|
||||
`Math.round((stats.sum / stats.count) * 100) / 100`. JS-AVG vs PG-AVG
|
||||
intermediate float math is not provably identical.
|
||||
- Action: property test pinning a fixture where the paths could
|
||||
disagree at the fifth decimal. Cosmetic risk only (Recharts axis
|
||||
ticks).
|
||||
|
||||
### UX-M3 — `entryCount` shape decision (info-only)
|
||||
(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M3)
|
||||
- `src/app/api/mood/analytics/route.ts:102, 139` — `entryCount`
|
||||
semantics consistent across both branches (total raw entries) but
|
||||
the field only lands in `annotate({ meta })`, not `apiSuccess()`.
|
||||
No client-facing shape impact; flagging for audit completeness only.
|
||||
|
||||
### Specialist-M01 — Drop `sum_value IS NULL` UNION arm after convergence
|
||||
(`.planning/round-v1439-QA-specialist-findings.md` MEDIUM-M1)
|
||||
- `src/lib/measurements/rollups.ts:762-779` — once Marc's tenant has
|
||||
converged, the legacy-NULL branch becomes a permanent no-op walk over
|
||||
the full rollup table. Schedule for removal in v1.4.40 + add a TODO
|
||||
with the cut-off date. Keeps the discovery query single-table after
|
||||
convergence.
|
||||
|
||||
### Simplifier dead-exports + duplicate tz helpers
|
||||
(`.planning/round-v1439-QA-simplifier-findings.md` Suggested removals +
|
||||
Suggested dedups)
|
||||
- `src/lib/measurements/rollup-read-cumulative.ts:54,83,121,164` —
|
||||
`isCumulativeType`, `readCumulativeDaySums`, `readCumulativeDaySumsBatch`,
|
||||
`resolveBucketSum` exported + tested but no production caller. ~360
|
||||
lines if file + tests collapse. Once a consumer exists, this becomes
|
||||
the single source of truth.
|
||||
- `src/lib/measurements/rollup-read-wmy.ts:125,137,149` —
|
||||
`readWeekRollups` / `readMonthRollups` / `readYearRollups` only used
|
||||
by their own tests; downgrade to non-exported or inline.
|
||||
- `src/lib/medications/compliance-rollups.ts:453` —
|
||||
`ensureUserMedicationComplianceFresh` only referenced in its own
|
||||
JSDoc; the v1.4.39 QA F-SEC-M-01 split shipped
|
||||
`enqueueUserMedicationComplianceBackfill` so the original `ensure-
|
||||
fresh` symmetry helper is dead. Drop or wire.
|
||||
- Duplicate tz helpers (`wallClockInTz`, `tzOffsetMinutes`,
|
||||
`startOfUtcDay`, `bucketSpan`, `startOfDayUtcInTz`) — three to four
|
||||
copies across `src/lib/measurements/rollups.ts`, `src/lib/mood/
|
||||
rollups.ts`, `src/lib/medications/compliance-rollups.ts`,
|
||||
`src/lib/medications/scheduling/cadence.ts`. Consolidate into
|
||||
`src/lib/tz/format.ts` (Marc-Voice: "one tz helper, one bug fix
|
||||
radius").
|
||||
|
||||
### UX-M4 — Front-end `vs last year` degradation hint
|
||||
(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M4)
|
||||
- Now **obsolete** after the v1.4.39 QA Specialist-H2 widen
|
||||
(`/api/analytics` live cap 90d → 425d). The "vs last year" tile no
|
||||
longer silently nulls on cold mounts. Flagged here so a future
|
||||
reviewer doesn't reopen.
|
||||
|
||||
---
|
||||
|
||||
## Low — observability polish
|
||||
|
||||
### F-L-01 — Compliance double-window-derivation cleanup
|
||||
(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-01)
|
||||
- `src/lib/medications/compliance-rollups.ts:186-199` —
|
||||
`recomputeMedicationComplianceForDay` computes the day window twice
|
||||
(`start + 86_400_000` and `startOfDayUtcInTz(nextDayKey)`) then
|
||||
picks the larger. Simpler invariant: "always use `startOfDayUtcInTz`
|
||||
for both bounds". Cosmetic.
|
||||
|
||||
### F-L-03 — Mood-rollup chunked upsert serialisation
|
||||
(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-03)
|
||||
- `src/lib/mood/rollups.ts:526-560` — `persistMoodRollupRows` chunks
|
||||
at 500 but issues a sequential `for…of` upsert loop inside each
|
||||
chunk. Measurement-rollups equivalent uses
|
||||
`prisma.$transaction([…])` for parallel upsert. Mood DAY hot-path
|
||||
writes 1 row so it doesn't matter; a 5-year boot-fold pays N
|
||||
round-trips instead of N/500 transactions.
|
||||
|
||||
### F-L-04 — Mood boot-backfill discovery query clarification
|
||||
(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-04)
|
||||
- `src/lib/mood/rollups.ts:417-427` — discovery query doesn't restrict
|
||||
`granularity='DAY'` on the LEFT JOIN. Inconsequential because the
|
||||
read path only consults DAY anyway. Comment clarification would help.
|
||||
|
||||
### F-SEC-L-01 — Compliance recompute silent-error observability
|
||||
(`.planning/round-v1439-QA-security-findings.md` LOW-F-SEC-L-01)
|
||||
- `src/lib/medications/compliance-rollups.ts:277-289` —
|
||||
`recomputeMedicationComplianceForEvent` swallows errors with an
|
||||
`annotate({ medication_compliance_rollup_failed: true })` flag.
|
||||
Self-corrects on the next mutation; no PII logged. Worth a follow-up
|
||||
metric / alert on the annotate flag.
|
||||
|
||||
---
|
||||
|
||||
## Strategic note (carry into v1.5)
|
||||
|
||||
- iOS Health-app integration remains v1.5 P1; the v1.4.39 backlog
|
||||
items above are all internal correctness/perf polish that doesn't
|
||||
gate iOS work.
|
||||
- Cross-tz proper fix (per-user-tz rollup bucket minting) remains
|
||||
v1.5 — the v1.4.38 cheap path (runtime guard with live fallback for
|
||||
non-near-UTC) is in place; v1.4.39 mood + compliance rollups inherit
|
||||
the UTC-anchor trade-off pinned by Specialist-H1's DST test.
|
||||
- Per-source rollup (audit P5) + slope-window SQL move (audit P8)
|
||||
remain v1.5 architectural backlog — unchanged from the v1.4.38
|
||||
closure.
|
||||
Reference in new issue
Block a user