mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-11 13:52:20 +00:00
docs(planning): v1.4.38 wave reports + QA findings + v1.4.39 backlog
Captures the marathon paper trail: - Wave reports (A cross-tz guard, C coach test invariants, F perf hotspots) — these landed in implementation but the report files were untracked at commit time. - QA findings (1 code-quality, 2 security, 3 UX/a11y, 4 architecture, 6 i18n parity) — six-axis review pass output, source-of-truth for the reconcile-apply decisions. - v1.4.39 backlog — items deferred during the reconcile (Coach client-side disabled copy, repo hygiene sweep, cumulative sparkline mean-vs-sum, gate walker file-ext closed set, sparkline live-fallback, BP/pulse term unification es/pl, plus observability polish from QA-1 Mediums).
This commit is contained in:
Marc-André Bombeck committed
1 parent
3fe09fe0f5
commit
3d21a22ee2
9 files changed
+1724
No files matched your search
@@ -0,0 +1,162 @@
|
||||
# Phase W-A — Cross-tz fast-path runtime guard
|
||||
|
||||
**Wave**: v1.4.38 W-A
|
||||
**Branch**: `develop`
|
||||
**Commits**:
|
||||
- `33dc00fe` — `feat(tz): isNearUtc helper for cross-tz fast-path guard`
|
||||
- `feae5b52` — `fix(correlations): fall back to live SQL when user is > 3h from UTC`
|
||||
- `c72b3ce8` — `fix(geo-backfill): drop batch cap from 5000 to 500 rows per pass`
|
||||
*(parallel wave W-B; absorbed my staged bp-in-target-fast-path.ts +
|
||||
analytics/route.ts edits before my own commit landed — net diff is
|
||||
still correct, just split across two commits)*
|
||||
- `9eda44fa` — `fix(bp-in-target): fall back to live SQL when user is > 3h from UTC`
|
||||
|
||||
## Scope landed
|
||||
|
||||
### Item 1 — `correlations-fast-path.ts` runtime guard
|
||||
- Added `isNearUtc(userTz, now)` gate to the
|
||||
`measurementsOnRollups` selector. When the user is more than ±3 h
|
||||
from UTC the helper now forces the live SQL path so the SYS /
|
||||
PULSE / WEIGHT per-day means are re-keyed via the same
|
||||
`userDayKey(measuredAt, userTz)` helper the mood / intake streams
|
||||
use, guaranteeing day-key parity.
|
||||
- Meta annotate gains `correlations.tz_guard:
|
||||
"near-utc" | "non-utc-live-fallback"`.
|
||||
|
||||
### Item 2 — `bp-in-target-fast-path.ts` runtime guard
|
||||
- New optional `userTz?: string` field on
|
||||
`computeBpInTargetFastPath`'s input. When omitted the guard
|
||||
defaults to `near-utc` (backwards-compat for any legacy caller —
|
||||
today only `src/app/api/analytics/route.ts`, which has been
|
||||
updated to thread `userTz` through).
|
||||
- Guard gates the rollup branch; threaded through to both
|
||||
`computeFromRollups` and `computeFromLive` so each annotate dict
|
||||
carries `analytics.bp_in_target.tz_guard`.
|
||||
|
||||
### Helper — `src/lib/tz/format.ts` + `src/lib/tz/resolver.ts`
|
||||
- New pure `isNearUtc(tz, now = new Date())` helper. Reuses the
|
||||
existing `tzOffsetMinutes` machinery; honours DST. Defaults to
|
||||
`true` for invalid zones (defensive — Berlin is near-UTC, so the
|
||||
rollup-path is the safer fallback for the canonical tenant).
|
||||
- Re-exported from `resolver.ts` for the server-side import path
|
||||
the analytics fast-paths already use.
|
||||
|
||||
## Tests delta
|
||||
|
||||
| File | Before | After | Notes |
|
||||
| --- | --- | --- | --- |
|
||||
| `src/lib/tz/__tests__/resolver.test.ts` | 28 cases | 35 cases | +7 cases pin Berlin / UTC / Moscow / Honolulu / Tokyo / Auckland branches plus the invalid-zone default and the `now`-omitted smoke path. |
|
||||
| `src/lib/analytics/__tests__/correlations-fast-path.test.ts` | 6 cases | 9 cases | +3 cases pin Berlin → rollup, Honolulu → live-fallback, Tokyo → live-fallback (with `tz_guard` annotate assertions on the first two). |
|
||||
| `src/lib/analytics/__tests__/bp-in-target-fast-path.test.ts` | 7 cases | 11 cases | +4 cases pin Berlin → rollup, Honolulu → live, Tokyo → live, and the legacy-compat "userTz omitted → near-UTC default" path. |
|
||||
| **Touched-file total** | 41 | 55 | +14 new assertions. |
|
||||
| Full suite | 4490 | 4504 | +14. |
|
||||
|
||||
Quality gates:
|
||||
- `pnpm typecheck`: clean (silent).
|
||||
- `pnpm lint`: clean for touched files; one pre-existing warning in
|
||||
`layout-coach-mount.tsx` unrelated to this wave.
|
||||
- `pnpm test --run src/lib/tz/__tests__/resolver.test.ts
|
||||
src/lib/analytics/__tests__/bp-in-target-fast-path.test.ts
|
||||
src/lib/analytics/__tests__/correlations-fast-path.test.ts`:
|
||||
3 files, 54 tests passed (touched files).
|
||||
- Full suite: 426 files, 4504 passed | 1 skipped (1 pre-existing
|
||||
skip unrelated to this wave).
|
||||
|
||||
## Self code-review (`superpowers:code-reviewer` substitute)
|
||||
|
||||
The `superpowers:code-reviewer` skill is not registered in this
|
||||
session's available-skills list (closest options:
|
||||
`code-review:code-review`, `review` — both PR-scoped, not diff-
|
||||
scoped). Performed a structured self-review against the same axes
|
||||
the dispatcher's skill would apply.
|
||||
|
||||
### Critical / High
|
||||
*None.* The guard is one-directional (rollup → live), the live
|
||||
fallback is the pre-existing v1.4.37 path with full coverage in
|
||||
`correlations.test.ts` + `tests/integration/bp-in-target.test.ts`,
|
||||
and the breadcrumb is purely additive on `meta`.
|
||||
|
||||
### Medium
|
||||
- **M1 — `userTz?` optionality asymmetry**.
|
||||
`bp-in-target-fast-path` accepts `userTz?: string` and defaults
|
||||
to `"near-utc"` on omission; `correlations-fast-path` keeps
|
||||
`userTz: string` required. Justified — bp-in-target has had a
|
||||
single caller for the entire v1.4.37 cycle so the "legacy compat
|
||||
default" is forward-looking insurance; correlations was added in
|
||||
v1.4.37 W2 with `userTz` already required and no caller has ever
|
||||
omitted it. The asymmetry is documented in both JSDoc blocks.
|
||||
*No action.*
|
||||
- **M2 — Positional-arg growth on the two private helpers**.
|
||||
`computeFromRollups` and `computeFromLive` now take 4 positional
|
||||
args (`userId, targets, now, tzGuard`). Both signatures remain
|
||||
reasonable for a private helper; the helpers are file-local so
|
||||
call sites are easy to grep. *No action — fold into an options
|
||||
bag only if a future arg lands.*
|
||||
|
||||
### Low
|
||||
- **L1 — `isNearUtc` boundary inclusivity**. `Math.abs(offset) <=
|
||||
180` includes the ±3 h boundary; Moscow (+3) and the Atlantic
|
||||
islands round to near-UTC. Test `"returns true for the ±3h
|
||||
boundary zones"` pins this as deliberate. The docstring says
|
||||
"within ±3 hours of UTC" — phrasing already covers the inclusive
|
||||
read. *No action.*
|
||||
- **L2 — Invalid-zone default returns true**. A junk tz string
|
||||
falls back to `DEFAULT_TIMEZONE` ("Europe/Berlin", near-UTC).
|
||||
Documented in the helper JSDoc; matches the broader resolver
|
||||
fallback semantics. *No action.*
|
||||
- **L3 — `tz_guard` annotate field lacks an OpenAPI surface**.
|
||||
The v1.4.38 backlog already tracks "CORRELATION_WINDOW_DAYS = 28
|
||||
not in OpenAPI" as a Low item; `tz_guard` should join that batch
|
||||
if Marc opts to surface the meta envelope in the spec. *Defer to
|
||||
the OpenAPI cleanup batch.*
|
||||
|
||||
## Brief-back (≤200 words)
|
||||
|
||||
**(a) Which file landed which guard.**
|
||||
- `src/lib/tz/format.ts` (+`resolver.ts` re-export) — new pure
|
||||
`isNearUtc(tz, now)` helper, ±3 h threshold, DST-aware via the
|
||||
existing `tzOffsetMinutes` machinery.
|
||||
- `src/lib/analytics/correlations-fast-path.ts` — gate folded
|
||||
into the `measurementsOnRollups` selector; meta annotate gains
|
||||
`correlations.tz_guard`.
|
||||
- `src/lib/analytics/bp-in-target-fast-path.ts` — new
|
||||
`userTz?: string` input field (defaults near-UTC for legacy
|
||||
compat); gate folded into the rollup-path conditional; both
|
||||
annotate sites carry `analytics.bp_in_target.tz_guard`.
|
||||
- `src/app/api/analytics/route.ts` — threads its existing
|
||||
`userTz` into `computeBpInTargetFastPath`.
|
||||
|
||||
**(b) Tests delta.** +7 resolver cases pinning Berlin / UTC /
|
||||
Moscow / Honolulu / Tokyo / Auckland / invalid-zone branches. +3
|
||||
correlations cases (Berlin rollup, Honolulu / Tokyo live-
|
||||
fallback). +4 bp-in-target cases (Berlin rollup, Honolulu / Tokyo
|
||||
live, legacy-compat omitted-userTz default). Full suite 4490 →
|
||||
4504, all green. Typecheck + lint clean for touched files.
|
||||
|
||||
**(c) Code-review finding worth surfacing.** None Critical/High.
|
||||
Two Medium items are intentional asymmetries already documented
|
||||
in JSDoc. The `superpowers:code-reviewer` skill itself is not
|
||||
registered in this session — the closest available skill is
|
||||
`code-review:code-review` which is PR-scoped; performed a
|
||||
structured self-review against the same axes instead.
|
||||
|
||||
## Constraint compliance
|
||||
|
||||
- File set: `src/lib/analytics/correlations-fast-path.ts`,
|
||||
`src/lib/analytics/bp-in-target-fast-path.ts`,
|
||||
`src/lib/tz/resolver.ts`, `src/lib/tz/format.ts` (the helper had
|
||||
to land in `format.ts` because `resolver.ts` only re-exports
|
||||
pure helpers — keeps the client/server split intact), the three
|
||||
matching `__tests__/` files, and `src/app/api/analytics/route.ts`
|
||||
(one-line `userTz` thread-through to consume the new field).
|
||||
Did not touch other fast-path files, the rollup populator, or
|
||||
any other analytics route surface.
|
||||
- Atomic Marc-Voice English commits; no `Co-Authored-By: Claude`,
|
||||
`--no-verify`, or `--no-gpg-sign`.
|
||||
- Other agents' commits interleaved on `develop` during the wave
|
||||
(W-B geo-backfill `c72b3ce8` between my staging and my commit
|
||||
of bp-in-target). Final tree state is correct; the interleave
|
||||
meant my bp-in-target source edits landed in `c72b3ce8` while
|
||||
the matching tests landed in `9eda44fa`. Net diff
|
||||
`dcd0b0a5..HEAD` against my file set is exactly the scope of
|
||||
this wave.
|
||||
@@ -0,0 +1,112 @@
|
||||
# Wave W-C — Coach cascade test invariants (v1.4.38)
|
||||
|
||||
## Scope
|
||||
|
||||
Three items from `.planning/round-v1438-backlog.md`:
|
||||
|
||||
- **H4** — fixture absorbs cross-cut gates OR grep-based discovery test
|
||||
(`src/lib/feature-flags/__tests__/coach-cascade.test.tsx`).
|
||||
- **M-5** — SSR proof spy/CSR render replacement
|
||||
(same file).
|
||||
- **M6** — Coach API route gate inventory
|
||||
(new `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts`).
|
||||
|
||||
## Outcome
|
||||
|
||||
### Item 1 (H4) and Item 2 (M-5)
|
||||
|
||||
Both items had **already landed in HEAD** by the time this wave woke up.
|
||||
Commit `c72b3ce8` (titled `fix(geo-backfill): drop batch cap from 5000 to 500
|
||||
rows per pass`) accidentally swept in 178 lines of additions to
|
||||
`src/lib/feature-flags/__tests__/coach-cascade.test.tsx` alongside the
|
||||
geo-backfill change. The diff matches what this wave produced
|
||||
independently — same `KNOWN_COACH_GATE_SITES` allowlist, same
|
||||
comment-aware `flags.coach` grep, same `featureFlagsSpy` + `vi.mock`
|
||||
seam, same `expect(featureFlagsSpy).toHaveBeenCalled()` assertion on the
|
||||
`proofWhenOn: ""` lazy-load surface. The wave's edits were therefore a
|
||||
no-op against the working tree.
|
||||
|
||||
Heads-up for `gsd:audit-milestone` at end of round: commit
|
||||
`c72b3ce8`'s subject line under-describes its real scope (test file
|
||||
got 178 LOC of additions that have nothing to do with geo-backfill).
|
||||
|
||||
### Item 3 (M6) — landed in two commits
|
||||
|
||||
- **`1eb00389`** — `test(coach): discovery test pins requireAssistantSurface on every coach insights route`
|
||||
- New `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts`
|
||||
walks every `route.ts` under `src/app/api/insights/`, requires each
|
||||
handler to either invoke `requireAssistantSurface("coach")` or
|
||||
appear on an explicit allowlist.
|
||||
- The walk uncovered **two orphan Coach routes** that lacked the
|
||||
operator kill-switch and were therefore reachable even when the
|
||||
operator turned Coach off:
|
||||
- `GET /api/insights/chat/[id]` and
|
||||
`DELETE /api/insights/chat/[id]`
|
||||
(`src/app/api/insights/chat/[id]/route.ts`) — read + delete
|
||||
Coach conversations (encrypted assistant prose).
|
||||
- `POST /api/insights/chat/messages/[id]/feedback`
|
||||
(`src/app/api/insights/chat/messages/[id]/feedback/route.ts`) —
|
||||
thumbs-up/-down on assistant messages.
|
||||
- Both routes now call `await requireAssistantSurface("coach")` right
|
||||
after auth; the existing `apiHandler` catch turns the
|
||||
`AssistantDisabledError` into the locked
|
||||
`{ data: null, error, meta: { errorCode: "assistant.disabled.coach" } }`
|
||||
403 envelope (per
|
||||
`.planning/RESPONSE-TO-IOS-TEAM-2026-05-16.md` §3 R5).
|
||||
- **`5ecc3152`** — `test(coach): make route-inventory gate check skip pure-comment lines`
|
||||
- Senior-dev self-review caught a false-positive in the inventory
|
||||
test: `text.includes("requireAssistantSurface(...)")` also matched
|
||||
a documentation comment that mentioned the gate, so a contributor
|
||||
who deleted the actual call but left the docstring would slip
|
||||
through. Refactored to a per-line check that ignores `//` and
|
||||
` *` JSDoc lines — same posture the coach-cascade discovery test
|
||||
already uses for `flags.coach`. Dropped a dead `repoRoot` variable
|
||||
from the stale-allowlist test along the way.
|
||||
- Sanity-test loop: temporarily replaced the actual gate call with a
|
||||
comment-only mention; both the H4 grep-test and the new M6 test
|
||||
fail as expected, then pass again when the call is restored.
|
||||
|
||||
## Self-review (`superpowers:code-reviewer` equivalent)
|
||||
|
||||
Findings applied as `5ecc3152`:
|
||||
|
||||
- **High** — pure-substring grep matched comments. Fixed by splitting
|
||||
the file into lines and rejecting any hit whose trimmed line starts
|
||||
with `//` or `*`.
|
||||
- **Low** — dead `repoRoot` variable in the stale-allowlist test.
|
||||
Removed (and dropped the meaningless `repoRoot.length > 0`
|
||||
silence-the-lint assertion).
|
||||
|
||||
## Quality gates (post-W-C state)
|
||||
|
||||
- `pnpm typecheck` — green.
|
||||
- `pnpm lint` — green.
|
||||
- `pnpm test --run src/app/api/insights/ src/lib/feature-flags/` —
|
||||
10 files, 67 tests pass (covers the new inventory test +
|
||||
pre-existing route + flag suites).
|
||||
- `pnpm test --run src/lib/feature-flags/__tests__/coach-cascade.test.tsx` —
|
||||
14 tests pass (covers fixture + H4 grep test + M-5 spy assertion).
|
||||
|
||||
## Brief-back inputs (≤200 words)
|
||||
|
||||
- **Total Coach gate sites discovered** — 7 web (`coach-launch-button`,
|
||||
`hero-strip`, `layout-coach-fab`, `layout-coach-mount`,
|
||||
`suggested-prompts`, `/targets/page`, `target-card`) + 5 Coach-gated
|
||||
routes (`chat`, `chat/[id]` GET, `chat/[id]` DELETE,
|
||||
`chat/messages/[id]/feedback` POST, `comprehensive`, `generate`). Web
|
||||
sites pin via `KNOWN_COACH_GATE_SITES`; routes pin via the inventory
|
||||
walk + `NON_COACH_GATED_ROUTES` / `NOT_COACH_OWNED_ROUTES`
|
||||
allowlists.
|
||||
- **Orphan gates found** — yes, two:
|
||||
`src/app/api/insights/chat/[id]/route.ts` (GET + DELETE) and
|
||||
`src/app/api/insights/chat/messages/[id]/feedback/route.ts` (POST).
|
||||
Both fixed in `1eb00389` by adding
|
||||
`await requireAssistantSurface("coach")` after `requireAuth()`.
|
||||
- **Invariants run green** — confirmed via the test gate runs above.
|
||||
|
||||
## Commits
|
||||
|
||||
- `1eb00389` — `test(coach): discovery test pins requireAssistantSurface on every coach insights route` (Item 3 + 2 missing-gate fixes)
|
||||
- `5ecc3152` — `test(coach): make route-inventory gate check skip pure-comment lines` (self-review)
|
||||
|
||||
Items 1 (H4) and 2 (M-5) were already in HEAD via `c72b3ce8`; no commits were authored by this wave for those items because the diffs were no-ops.
|
||||
@@ -0,0 +1,183 @@
|
||||
# v1.4.38 W-F — perf-hotspot fix-up
|
||||
|
||||
Two endpoints surfaced as 3-5 s cold during v1.4.37.x verification. Same
|
||||
shape as the v1.4.37.2 slim-summaries fix: in both cases the JS layer
|
||||
was throwing away most of the row volume it asked for. Applied the same
|
||||
SQL-aggregation playbook here, plus a 60 s LRU wrap on the iOS-only
|
||||
dashboard route and per-sub-query timing annotations so the next
|
||||
perf-verify can attribute regressions without re-instrumenting.
|
||||
|
||||
## Endpoint A — `GET /api/dashboard/summary`
|
||||
|
||||
### Before
|
||||
|
||||
Four sub-queries inside the `Promise.all`, plus a fifth conditional
|
||||
after it:
|
||||
|
||||
| # | Query | Row count on a power-user account |
|
||||
|---|-------|-----------------------------------|
|
||||
| A1 | `measurement.findMany({ measuredAt: { gte: -7d } })` | thousands (Apple Health step samples) |
|
||||
| A2 | `measurement.groupBy({ by: ["type"], _count, _max })` | ≤ N metric types (~10) |
|
||||
| A3 | `medicationIntakeEvent.findMany({ scheduledFor: today })` | ≤ daily schedule count |
|
||||
| A4 | `medicationIntakeEvent.findMany({ scheduledFor: { gte: -365d } })` | hundreds |
|
||||
| A5 (conditional) | `measurement.findMany({ measuredAt: { gte: -365d } })` | up to **tens of thousands** of raw rows |
|
||||
|
||||
Wall-clock: ~4.6 s cold mount per Marc's HAR capture. A1 and A5 were
|
||||
dominant on power-user accounts.
|
||||
|
||||
### After
|
||||
|
||||
Six bounded sub-queries; the conditional fifth is folded back into the
|
||||
Promise.all:
|
||||
|
||||
| # | Query | Bounded row count |
|
||||
|---|-------|-------------------|
|
||||
| B1 | `$queryRaw DISTINCT ON (type)` over 7d window | one row per metric type |
|
||||
| B2 | `$queryRaw` over `measurement_rollups` DAY buckets in 7d | ≤ `SPARK_DAYS × N` ≈ 70 |
|
||||
| B3 | `measurement.groupBy({ by: ["type"], _count, _max })` | unchanged |
|
||||
| B4 | `medicationIntakeEvent.findMany({ scheduledFor: today })` | unchanged |
|
||||
| B5 | `medicationIntakeEvent.findMany({ scheduledFor: { gte: -365d } })` | unchanged |
|
||||
| B6 | `$queryRaw SELECT DISTINCT to_char(measured_at AT TIME ZONE $tz, 'YYYY-MM-DD')` | ≤ 365 |
|
||||
|
||||
Then wrap the whole response in `caches.analytics` keyed
|
||||
`${userId}|dashboard-summary` with a 60 s TTL. The cache hit path skips
|
||||
the whole builder.
|
||||
|
||||
Behaviour notes:
|
||||
- `sparkline` is now the DAY-bucket mean per day instead of every raw
|
||||
reading. For BP/weight that's effectively the same shape because the
|
||||
user logs ≤1 per day. For ACTIVITY_STEPS / sleep / glucose the
|
||||
smoother per-day average is a better trend signal anyway.
|
||||
- `latestValue` semantics preserved — still the most recent reading in
|
||||
the trailing 7-day window. When the window is empty, `latestValue`
|
||||
stays null and `lastSeenAt` falls through to the all-time `_max`
|
||||
(unchanged behaviour).
|
||||
- The `AT TIME ZONE $tz` conversion is done server-side so the day-key
|
||||
set is byte-identical to what `userDayKey(measuredAt, userTz)` would
|
||||
produce in JS.
|
||||
|
||||
### Cache-invalidation hooks
|
||||
|
||||
Extended `invalidateUserMedications` to flush `caches.analytics` for
|
||||
the user prefix. Without this a medication intake event would not
|
||||
invalidate the dashboard summary (measurement / mood writes already
|
||||
flushed analytics via the existing helpers).
|
||||
|
||||
### Estimated wall-clock
|
||||
|
||||
| Path | Before | After |
|
||||
|---|---|---|
|
||||
| Cold mount, rollup-fresh | ~4.6 s | ~500 ms |
|
||||
| Warm cache hit (within 60 s TTL) | n/a | < 50 ms (Map lookup) |
|
||||
| Cold mount, rollup-empty (brand-new account) | ~4.6 s | ~600 ms (DAY-bucket query returns empty, sparkline degrades gracefully) |
|
||||
|
||||
### Per-sub-query timing labels
|
||||
|
||||
Emitted under `meta.dashboard.sub_<label>_ms`:
|
||||
- `dashboard.sub_latest7d_ms` — `DISTINCT ON` per type
|
||||
- `dashboard.sub_sparkline_ms` — DAY rollup buckets in 7d
|
||||
- `dashboard.sub_allTime_ms` — `groupBy` per type
|
||||
- `dashboard.sub_todaysIntakes_ms` — today's intake events
|
||||
- `dashboard.sub_streakIntakes_ms` — 365-day intake events
|
||||
- `dashboard.sub_streakDays_ms` — distinct activity day-keys
|
||||
|
||||
## Endpoint B — `GET /api/insights/comprehensive`
|
||||
|
||||
### Before
|
||||
|
||||
The route already used `buildComprehensiveAggregate` (the SQL-aggregate
|
||||
shape landed in v1.4.36). The remaining cost on the rollup-fresh happy
|
||||
path came from:
|
||||
|
||||
| # | Query | Notes |
|
||||
|---|---|---|
|
||||
| C1 | `$queryRaw` narrow aggregate | 90-day full scan twice (window_stats CTE + GROUP BY). Unavoidable: the windowed/regression columns don't compose linearly across DAY buckets. |
|
||||
| C2 | `$queryRaw DISTINCT ON (type)` latest per type | Cheap, indexed |
|
||||
| C3 | `measurementRollup.findMany` 90-day DAY buckets | ~720 rows for 8 types |
|
||||
| C4 | `measurement.findMany({ type: "BLOOD_PRESSURE_SYS" })` | ≤ 2k rows |
|
||||
| C5 | `measurement.findMany({ type: "BLOOD_PRESSURE_DIA" })` | ≤ 2k rows |
|
||||
| C6 | `$queryRaw SELECT MIN(measured_at)` | Cheap, indexed |
|
||||
|
||||
Wall-clock: ~3.4 s cold mount per Marc's HAR capture.
|
||||
|
||||
### After
|
||||
|
||||
| # | Query | Change |
|
||||
|---|---|---|
|
||||
| C1 | narrow aggregate | unchanged (already optimal for non-composable columns) |
|
||||
| C2 | latests | unchanged |
|
||||
| C3 | DAY buckets | unchanged |
|
||||
| **C4+C5 → C45** | bp raw rows | **merged** into one `findMany({ type: { in: [BP_SYS, BP_DIA] } })`; partition in JS by type. Saves one RTT per request. |
|
||||
| C6 | firstAt | unchanged |
|
||||
|
||||
Plus per-sub-query timing annotations on both the rollup-fresh and the
|
||||
cold-fallback paths.
|
||||
|
||||
### Estimated wall-clock
|
||||
|
||||
| Path | Before | After |
|
||||
|---|---|---|
|
||||
| Cold mount, rollup-fresh | ~3.4 s | ~3.1 s (one RTT shaved by the BP merge) |
|
||||
| Warm cache hit (already 60 s LRU at route level) | < 50 ms | < 50 ms |
|
||||
|
||||
### What I did NOT optimise
|
||||
|
||||
- **Narrow aggregate** (C1) — the windowed `avg7 / avg30 / avg30LastMonth`
|
||||
+ `slope7 / slope30 / slope90` + `anomalyCount` columns don't compose
|
||||
linearly across DAY buckets, so the 90-day scan stays canonical. The
|
||||
v1.4.35 design call stands.
|
||||
- **Cold-path heavy aggregate** — fallback path for brand-new accounts;
|
||||
one-shot cost that doesn't merit further work until the boot-time
|
||||
backfill misbehaves in prod.
|
||||
- **Mood / medications findMany** inside the route handler — bounded by
|
||||
user-recorded data shape (a few hundred rows per user); not the
|
||||
hotspot.
|
||||
|
||||
### Per-sub-query timing labels
|
||||
|
||||
Emitted under `meta.insights.sub_<label>_ms`:
|
||||
- `insights.sub_coverage_ms` — per-type rollup coverage probe
|
||||
- `insights.sub_narrow_ms` — windowed/regression aggregate (rollup-fresh path)
|
||||
- `insights.sub_heavy_ms` — full aggregate (cold-fallback path)
|
||||
- `insights.sub_latests_ms` — DISTINCT ON latest per type
|
||||
- `insights.sub_buckets_ms` — DAY rollup buckets read
|
||||
- `insights.sub_bpRaw_ms` — bp sys+dia consolidated read
|
||||
- `insights.sub_firstAt_ms` — earliest measurement timestamp
|
||||
|
||||
## File set
|
||||
|
||||
Commits:
|
||||
- `eba7d6aa perf(dashboard): rollup + DISTINCT ON read for the iOS summary route` (+228 / −63)
|
||||
- `0cc9f55a perf(insights): consolidated BP sys+dia read and per-sub-query timings` (+130 / −80)
|
||||
|
||||
Touched:
|
||||
- `src/app/api/dashboard/summary/route.ts` — rewrite
|
||||
- `src/app/api/dashboard/summary/__tests__/route.test.ts` — mock-shape update for the `$queryRaw` calls
|
||||
- `src/lib/cache/invalidate.ts` — analytics-cache flush added to `invalidateUserMedications`
|
||||
- `src/lib/insights/comprehensive-aggregator.ts` — bp-merge + per-sub-query timings
|
||||
- `src/lib/insights/__tests__/comprehensive-aggregator.test.ts` — fixture update for the merged BP findMany
|
||||
|
||||
LOC delta: +358 / −143.
|
||||
|
||||
## Quality gates
|
||||
|
||||
- `pnpm typecheck` clean
|
||||
- `pnpm lint` clean (3 pre-existing warnings unrelated to this wave)
|
||||
- `pnpm test --run` on touched files: 15 / 15 passing across the
|
||||
dashboard route, comprehensive aggregator, and comprehensive route
|
||||
shape tests
|
||||
- Broader smoke pass: `src/lib/insights` (162 tests) + `src/lib/cache`
|
||||
(22 tests) all green
|
||||
|
||||
## Recommendations for the next perf-verify
|
||||
|
||||
1. After deploy, capture a fresh HAR on Marc's account hitting both
|
||||
endpoints cold. Verify the new `meta.dashboard.sub_*_ms` /
|
||||
`meta.insights.sub_*_ms` annotations land in prod logs.
|
||||
2. Validate cache hit ratio for `analytics.dashboard-summary` — iOS
|
||||
polling at 30 s intervals should produce > 80 % hit rate.
|
||||
3. If `insights.sub_narrow_ms` still dominates at > 2 s, the next round
|
||||
should evaluate a per-type narrow aggregate sourced from rollup
|
||||
buckets for `avg7 / avg30 / avg30LastMonth` (composable when the
|
||||
bucket carries `count`-weighted sum) and only fall back to live SQL
|
||||
for slope/r2/sd.
|
||||
@@ -0,0 +1,221 @@
|
||||
# W-QA-1 v1.4.38 Code-Quality Review Findings
|
||||
|
||||
Reviewer: Senior Code Reviewer (W-QA-1 lane: code-quality only — security / a11y / arch / simplifier / i18n covered by parallel reviewers).
|
||||
Scope: `v1.4.37.2..HEAD` on `develop`, 43 commits, 52 files, +5916/-3731.
|
||||
Methodology: source-file diff read against PLAN wave summaries (W-A cross-tz guard, W-B robustness×14, W-C coach gates, W-D UX×17, W-E i18n, W-F perf).
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| Critical | 0 |
|
||||
| High | 2 |
|
||||
| Medium | 6 |
|
||||
| Low | 5 |
|
||||
|
||||
No tag-blocker. The W-C orphan-gate find (3 routes hardened mid-wave) is the most decision-relevant pre-tag observation: the marathon already closed the orphans and added the inventory test, so the surface is now self-policing. Two High items are correctness traps that surface only under uncommon load shapes (rejected-promise dedup race; cached envelope vs. enriched envelope contract drift). The remaining items are maintainability / observability polish.
|
||||
|
||||
Code-quality strengths that landed in this release:
|
||||
1. Every fast-path now annotates `tz_guard` + `path` so prod logs can attribute branch decisions.
|
||||
2. Drop-in `userTz?` parameter defaults keep legacy call sites compiling — additive change, no caller churn.
|
||||
3. Comprehensive aggregator's `timeSubquery` helper makes future perf-verify pivot-by-sub-query trivial.
|
||||
4. The W-C inventory test (`coach-route-gate-inventory.test.ts`) lifts the gate-presence contract from "ambient discipline" to "compile-time + test-time enforced".
|
||||
5. Read-time `daysAgo` derivation off the cached envelope (analytics route) is a clean separation of "what's stable enough to cache" vs "what must be wall-clock fresh".
|
||||
|
||||
---
|
||||
|
||||
## High
|
||||
|
||||
### H-1 — `ensureUserRollupsFresh` in-flight dedup races on rapid serial rejection
|
||||
|
||||
File: `src/lib/measurements/rollups.ts:608-633` (`ensureUserRollupsFresh` + `ensureUserRollupsFreshInFlight`)
|
||||
|
||||
Concern: the dedup map clears the slot in `finally`, which is correct for the happy path. But the wrapper assigns the in-flight promise into the map **before** the inner promise actually starts executing — there is a microtask gap where a same-tick second caller will pick up the stored promise correctly, but if the first caller catches the rejection and immediately re-issues (`await ensureUserRollupsFresh(uid).catch(retry)`), the `.finally` may not have fired yet inside the same await microtask boundary, and the retry will piggyback on the rejected promise instead of starting a fresh one.
|
||||
|
||||
Test coverage (`rollups.test.ts:350-367`) exercises serial calls across `await` boundaries which is the happy case; the racy case (same-microtask retry on rejection) is not pinned.
|
||||
|
||||
Recommended fix: clear the slot eagerly in the `.then` and `.catch` hooks instead of `.finally`, so even a synchronous-microtask retry sees a fresh slot:
|
||||
```ts
|
||||
const inflight = ensureUserRollupsFreshImpl(userId);
|
||||
inflight.then(
|
||||
() => ensureUserRollupsFreshInFlight.delete(userId),
|
||||
() => ensureUserRollupsFreshInFlight.delete(userId),
|
||||
);
|
||||
```
|
||||
Or document explicitly that callers must `await` and that rapid retry-on-rejection is not supported.
|
||||
|
||||
### H-2 — Analytics `enrichLastSeenDaysAgo` mutates type contract assumed by cache
|
||||
|
||||
File: `src/app/api/analytics/route.ts:86-152`
|
||||
|
||||
Concern: the cache key is `${user.id}|default` and stores `cachedBody` (`lastSeenByType: Record<string, { lastSeenAt }>`). The GET handler then runs `enrichLastSeenDaysAgo(cachedBody)` per request to attach `daysAgo`. The TypeScript shape returned to the client is `Record<string, { lastSeenAt; daysAgo }>` — but `buildAnalyticsResponse`'s own type (line 162+) builds `lastSeenByType: Record<string, { lastSeenAt: string } | null>`. There is no central type alias enforcing the cached-vs-enriched separation. If a future hand seeking to "fix the missing daysAgo field" reads the response shape and adds `daysAgo` to `buildAnalyticsResponse`'s output, it will land in the cache and re-stale across day boundaries — the exact bug the W-B fix was meant to close.
|
||||
|
||||
Recommended fix: extract a named interface pair `AnalyticsCachedBody` / `AnalyticsEnrichedBody` and pin the cached call site to the former, the API response to the latter. A failing test would assert `caches.analytics` only holds keys of shape `{ lastSeenAt }` (no `daysAgo`).
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### M-1 — `isNearUtc` defaults to "near-utc" on invalid tz silently
|
||||
|
||||
File: `src/lib/tz/format.ts:96-100`
|
||||
|
||||
Concern: `isNearUtc(tz, now)` falls back to `DEFAULT_TIMEZONE` (Berlin = near-UTC = `true`) when the supplied tz fails validation. Per the docstring this is the safer default for the canonical tenant — but for a future multi-tenant or a corrupted user row, "true" routes the user to the rollup path which may misbucket. The comment notes "the resolver layer is already defensive about junk values" but `isValidTimezone` doesn't log; a silent corruption upstream cannot be distinguished from a legitimate Berlin user.
|
||||
|
||||
Recommended fix: add a one-shot `console.warn` when the input fails validation (mirroring the `warnTrustViolationOnce` pattern in api-response.ts). Operator gains observability; semantics unchanged.
|
||||
|
||||
### M-2 — `bp-in-target-fast-path` cross-tz guard only flips per-fan-out, not per-window
|
||||
|
||||
File: `src/lib/analytics/bp-in-target-fast-path.ts:147-163`
|
||||
|
||||
Concern: when `userTz` is omitted (legacy callers), the helper unconditionally treats the user as near-UTC. The analytics route now passes `userTz` (line 399, 406), but any *other* caller (future iOS native API, scheduled snapshot job, doctor report aggregator) gets near-UTC silently. The `// when omitted... defaults to near-UTC for backwards-compat` comment is correct but the default is biased toward "approximate".
|
||||
|
||||
Recommended fix: either flip the default to "force live" on omitted `userTz` (safe-by-default; legacy correctness intact, just slower on uncovered call sites), or make `userTz` required and audit-fix the call sites in a follow-up.
|
||||
|
||||
### M-3 — `comprehensive-aggregator` `fetchBpRawRows` consolidated read changes ORDER BY guarantee
|
||||
|
||||
File: `src/lib/insights/comprehensive-aggregator.ts:301-327`
|
||||
|
||||
Concern: pre-v1.4.38 the sys / dia raw rows came from two separate `findMany`s each ordering `{ measuredAt: 'asc' }`. The W-F consolidation uses one `findMany` with `type: { in: [...] }` and partitions by `type` in JS. The partition loop preserves measurement order within each type because the SQL ORDER BY is global — but only because postgres yields rows in `(measuredAt asc, id asc)` order for the merged scan. If a future change adds a second ORDER BY column (e.g. ordering by `(type, measuredAt)` for index hint), the partition would emit unordered rows.
|
||||
|
||||
Recommended fix: change the orderBy to the explicit pair `[{ measuredAt: 'asc' }, { id: 'asc' }]` (stable tiebreaker — already the convention in `fetchSeriesChunked` at bp-in-target-fast-path.ts:435). Pin the partition order with a unit test.
|
||||
|
||||
### M-4 — Drain helper "per-user COMPLETE" log line uses snapshot delta — fragile
|
||||
|
||||
File: `src/lib/measurements/drain-per-sample-cumulative.ts:316-318, 434-442`
|
||||
|
||||
Concern: the per-user complete-log captures a "before" snapshot of three counters, then subtracts from the post-loop totals. If the per-user loop body ever throws (e.g. transaction failure on one of N users), the COMPLETE log line never fires for that user but earlier users' deltas are correct. Worse, if the loop body adds a parallel counter via `Promise.all` in a future refactor, the snapshot/delta pattern breaks silently. The cleaner shape is to scope counters per-user and sum into totals afterwards.
|
||||
|
||||
Recommended fix: introduce `const perUserSummary = { bucketsCollapsed: 0, ... }` inside the loop, accumulate into it directly, then `summary.totals.X += perUserSummary.X` at the end. Same numbers, no snapshot fragility, and the COMPLETE log line reads from a contained struct.
|
||||
|
||||
### M-5 — `bp-in-target-fast-path` priorYear `readSince` mutates input `now`
|
||||
|
||||
File: `src/lib/analytics/bp-in-target-fast-path.ts:189-197`
|
||||
|
||||
Concern: the `readSince` IIFE creates `d = new Date(now.getTime())` then calls `d.setUTCFullYear` / `d.setUTCDate` on it. Looks fine — `d` is a separate Date. But the in-place mutators are called on a fresh date built from `now`, which the rest of the helper continues to reference. If a future maintainer simplifies to `const d = now; d.setUTC...`, the caller's `now` argument mutates. Today's code is correct but the pattern invites the bug.
|
||||
|
||||
Recommended fix: prefer the immutable shape:
|
||||
```ts
|
||||
const readSince = new Date(
|
||||
Date.UTC(
|
||||
now.getUTCFullYear() - 1,
|
||||
now.getUTCMonth(),
|
||||
now.getUTCDate() - 31,
|
||||
now.getUTCHours(),
|
||||
now.getUTCMinutes(),
|
||||
now.getUTCSeconds(),
|
||||
),
|
||||
);
|
||||
```
|
||||
No setter calls, no mutation surface.
|
||||
|
||||
### M-6 — `coach-route-gate-inventory.test.ts` line-mode comment skipper is heuristic
|
||||
|
||||
File: `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts:79-87`
|
||||
|
||||
Concern: the comment skipper rejects lines starting with `//` or `*`. A multi-line block comment containing `requireAssistantSurface("coach")` on a line that does NOT start with `*` (e.g. a code-fence inside a `/* ... */` JSDoc) would falsely satisfy the gate-presence check, letting an orphan slip through. Equally, an actual call wrapped in inline conditional `/* prettier-ignore */ requireAssistantSurface("coach");` is rejected.
|
||||
|
||||
The exposure is small (no current Coach route ships either pattern) but the heuristic is brittle.
|
||||
|
||||
Recommended fix: lean on the TypeScript AST (`ts.createSourceFile` + walker for `CallExpression`) instead of string-matching lines. The Vitest harness already loads the TS compiler.
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### L-1 — `health-score-fast-path` `bpInTargetPctPriorWeek` `undefined` vs `null` semantics
|
||||
|
||||
File: `src/lib/analytics/health-score-fast-path.ts:140-143`
|
||||
|
||||
Concern: the helper distinguishes `undefined` (legacy caller — pin to current value) from `null` (caller knows there's no prior-week data — feed null through). The check `input.bpInTargetPctPriorWeek === undefined` is tight, but the surrounding type `bpInTargetPctPriorWeek?: number | null` allows both. A caller doing `{ ...x, bpInTargetPctPriorWeek: undefined }` after destructuring loses the legacy-compat path silently. Today this works correctly because the analytics route either passes a real number or `null`, never `undefined` explicitly.
|
||||
|
||||
Recommended fix: add a JSDoc explicit note on the field that `undefined` = legacy mode, `null` = no-prior-data; also add a unit test for both modes.
|
||||
|
||||
### L-2 — `geo-backfill` in-process singleton flag is module-scope but unprotected on hot-reload
|
||||
|
||||
File: `src/lib/jobs/reminder-worker.ts:1208-1238`
|
||||
|
||||
Concern: `let geoBackfillRunning = false;` is module-scope. In production this is per-process so the guard works. In dev with Next.js HMR or Vitest watch mode, a module reload between runs could reset the flag while a previous pass is still spinning. Test coverage doesn't pin this. The exposure is dev-only (production never HMRs) but the comment "fans the second invocation out as a no-op" reads as a stronger guarantee than the implementation actually offers.
|
||||
|
||||
Recommended fix: short note in the comment that the guard is per-Node-process and HMR resets it. No code change needed.
|
||||
|
||||
### M-1 fix-overlap: `correlations-fast-path` annotate emits `tz_guard` only on success path
|
||||
|
||||
File: `src/lib/analytics/correlations-fast-path.ts:311-326`
|
||||
|
||||
Concern: the annotate dict carrying `tz_guard` is built into the final `annotate({ meta: { correlations: { ... } } })` call. If an exception throws before line 311 (any of the Promise.all rejections, the rollup read), the annotate never fires, and ops logs cannot tell whether the failed request was a near-UTC or non-UTC user. The bp-in-target helper has the same shape (annotate at the END of each branch).
|
||||
|
||||
Recommended fix: emit a `tz_guard_attempted` annotate as the first line of the helper (right after computing `userNearUtc`), so even a partial run carries the guard decision into the wide-event pipeline.
|
||||
|
||||
### L-4 — `analytics/route.ts` line 173 fire-and-forget on `ensureUserRollupsFresh` swallows errors invisibly to route
|
||||
|
||||
File: `src/app/api/analytics/route.ts:173`
|
||||
|
||||
Concern: `void ensureUserRollupsFresh(user.id);` — the helper handles its own errors (annotate + console.error per the v1.4.36 H3 retrofit). But the route's annotate context is request-scoped; the populator's annotate fires in a detached microtask after the request has likely already resolved. The current `annotate` no-op fallback when no context is present is documented inside the helper, but the route-side observer reading wide events won't see the populator failure attributed to this request — it'll appear as an unattributed log line.
|
||||
|
||||
Recommended fix: change the `void` call to attach to the response Promise via `Promise.race([routeWork, populator.catch(noop)])` — keeps the fire-and-forget contract but the populator's annotate now fires within the route's logging context.
|
||||
|
||||
### L-5 — `localDayWindow` uses string-concat for next-day key derivation
|
||||
|
||||
File: `src/lib/measurements/drain-per-sample-cumulative.ts:205-218`
|
||||
|
||||
Concern: `nextUtcNoon.setUTCDate(nextUtcNoon.getUTCDate() + 1)` then `.toISOString().slice(0, 10)` — works because noon is mid-day so DST shifts don't cross the date boundary. The comment is explicit about why noon is the anchor. Brittle to "let's anchor on midnight" refactors.
|
||||
|
||||
Recommended fix: pull the next-day-key helper into a tiny named function `addOneDayUtc(dateKey: string): string` with a docstring on the noon-anchor invariant, so the calculation isn't inline.
|
||||
|
||||
---
|
||||
|
||||
## Cross-cutting Concerns
|
||||
|
||||
1. **Commit-attribution drift documented per wave** — the kickoff brief flags that bundled commits will collapse on squash-to-main. No code-quality issue; surfaced here so the merge author knows to expect noise in `--author` filtering of the develop log.
|
||||
|
||||
2. **Naming consistency: "fast-path" vs "fast-path"** — three fast-path helpers (`bp-in-target-fast-path`, `correlations-fast-path`, `health-score-fast-path`) all live under `src/lib/analytics/` but each has its own dispatch shape (some take `coverage?`, some don't; some take `userTz`, some don't; the prior-week BP pct is a one-off field on health-score only). A future refactor to a shared `FastPathContext` interface would tighten the surface; deferred to a v1.5 follow-up since the contract is correct today.
|
||||
|
||||
3. **Test mock convention drift** — `bp-in-target-fast-path.test.ts` uses inline `vi.mock` factories per dependency. `rollups.test.ts` uses the `vi.hoisted` pattern. Both work, but a single project-wide convention would help reviewers. Note for the test-style ADR backlog; not a release blocker.
|
||||
|
||||
4. **Annotate dict shape proliferation** — the W-F perf timings (`meta.dashboard.sub_*_ms`, `meta.insights.sub_*_ms`) are emitted alongside W-A's `tz_guard` field on `meta.analytics.bp_in_target` / `meta.correlations`. The wide-event consumer doesn't yet validate keyspaces. A meta-schema doc (or Zod schema) lives in `.planning/` is would prevent silent typos like `tz_gaurd` becoming a permanent prod field. Backlog candidate.
|
||||
|
||||
5. **Per-sub-query timing pattern is duplicated** — the `time(label, builder)` helper in `src/app/api/dashboard/summary/route.ts:288-293` and the `timeSubquery(timings, label, builder)` helper in `src/lib/insights/comprehensive-aggregator.ts:281-291` solve the same problem with slightly different signatures. Lift into `src/lib/logging/sub-query-timer.ts` so future routes pick up one canonical shape.
|
||||
|
||||
---
|
||||
|
||||
## Plan-Alignment Summary
|
||||
|
||||
| Wave | Plan deliverable | Implementation matches plan? | Notes |
|
||||
|------|------------------|------------------------------|-------|
|
||||
| W-A | Cross-tz fast-path guard | Yes | `isNearUtc` helper landed in `format.ts` (not `resolver.ts`) per the architecture comment; re-exported through `resolver.ts` for back-compat. Three fast-paths gated; analytics route threads userTz through both BP fan-outs. |
|
||||
| W-B | 14 robustness items | Yes — spot-checked 9 of 14 | drain const lift, drill-down 1000-cap validator, daysAgo derive-on-read, ensureUserRollupsFresh dedup, parallel WEEK/MONTH/YEAR enqueue, BP leap-year priorYear, bucketDayKey rename, IP `node:net.isIP`, dashboard medications staleTime — all match plan. |
|
||||
| W-C | Coach cascade test invariants + orphan-gate fix | Yes — and exceeded | Inventory test catches future regressions; 3 orphan gates added in same wave (chat GET/DELETE, message feedback POST). |
|
||||
| W-D | UX polish ~17 items | Not directly reviewed (UX/a11y lane) | Read the medication-intake quick-add for cache-sharing logic only. |
|
||||
| W-E | i18n 27% → 63% | Not directly reviewed (i18n lane) | |
|
||||
| W-F | dashboard/summary perf | Yes | DISTINCT ON + rollup buckets replace two unbounded findMany; per-sub-query timings; LRU wrap. |
|
||||
|
||||
No problematic plan deviations identified.
|
||||
|
||||
---
|
||||
|
||||
## Files referenced
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/tz/format.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/bp-in-target-fast-path.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/correlations-fast-path.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/health-score-fast-path.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/__tests__/bp-in-target-fast-path.test.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/api-response.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/__tests__/get-client-ip.test.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/__tests__/rollups.test.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/measurements/drain-per-sample-cumulative.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/jobs/geo-backfill.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/jobs/reminder-worker.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/insights/comprehensive-aggregator.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/validations/measurement.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/validations/__tests__/measurement.test.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/analytics/route.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/summary/route.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/insights/chat/[id]/route.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/insights/chat/messages/[id]/feedback/route.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts`
|
||||
- `/Users/marc/Projects/HealthLog/src/components/dashboard/medication-intake-quick-add.tsx`
|
||||
@@ -0,0 +1,109 @@
|
||||
# W-QA-2 — v1.4.38 Security Audit
|
||||
|
||||
**Scope**: `v1.4.37.2..HEAD` on `develop`
|
||||
**Mode**: READ-ONLY, security lane
|
||||
**Date**: 2026-05-17
|
||||
|
||||
## Severity counts
|
||||
|
||||
| Severity | Count |
|
||||
|---|---|
|
||||
| Critical | 0 |
|
||||
| High | 0 |
|
||||
| Medium | 1 |
|
||||
| Low | 2 |
|
||||
| Informational | 1 |
|
||||
| Confirmed-safe | 7 |
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### M-1 — `User.timezone` accepted unvalidated; new SQL aggregate fails-open to PG error
|
||||
**Where**: `src/lib/auth/profile-update.ts:14` (`timezone: z.string().min(1).max(64).optional()`) → reaches `src/app/api/dashboard/summary/route.ts:374` (`to_char(m."measured_at" AT TIME ZONE ${userTz}, 'YYYY-MM-DD')`) and the cross-tz fast-path callers via `resolveUserTimezone`.
|
||||
|
||||
**Finding**: The profile-update schema validates the timezone string only as a 1–64 char string. The `isValidTimezone()` helper exists in `src/lib/tz/format.ts:22` and is invoked at **read** time (`resolveUserTimezone`, `resolveServerDefaultTimezone`), but **not** at the write path. A user can PUT `{ timezone: "garbage" }` to `/api/auth/profile` or PATCH `/api/user/profile` and the column will be persisted.
|
||||
|
||||
**Blast radius (v1.4.38-new)**:
|
||||
1. `GET /api/dashboard/summary` runs `prisma.$queryRaw` with `AT TIME ZONE ${userTz}` where `${userTz}` is bound as a parameter (so no classic SQL-injection), but Postgres will raise `invalid_parameter_value` for an unknown IANA zone → 500 response. Per-user self-DoS only (read-path own-user-only).
|
||||
2. The cross-tz fast-paths (`bp-in-target`, `correlations`) and every other consumer of `userDayKey()` fall through to `DEFAULT_TIMEZONE = "Europe/Berlin"` so they are resilient. The W-A `isNearUtc(userTz, now)` guard accepts invalid TZ → returns `true` → would force the rollup path for a Berlin-equivalent user — defensible per the helper's docstring but inconsistent with the SQL aggregate's hard-fail.
|
||||
|
||||
**Authorization-scope finding for W-A fallback**: the live-SQL fallback in `bp-in-target-fast-path.ts:425`, `correlations-fast-path.ts:338`, and `health-score-fast-path.ts:202` consistently scopes every `findMany` on `{ userId }`. No cross-tenant leakage path. The rollup path also scopes on `userId` in `readRollupBuckets`. The tz_guard is purely a performance/correctness toggle; it does not affect authorization. CONFIRMED-SAFE on the scope-leak question.
|
||||
|
||||
**Recommendation**: Add `.refine(isValidTimezone, "Invalid IANA timezone")` to the schema in `src/lib/auth/profile-update.ts:14`. Pre-tag fix is one line; the failing read becomes a 422 on the write instead of a self-DoS on the read.
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### L-1 — `correlations-fast-path.ts` rollup path emits `degraded: false` unconditionally
|
||||
**Where**: `src/lib/analytics/correlations-fast-path.ts:319`
|
||||
|
||||
**Finding**: The `degraded` field is hard-coded `false` on both branches with a `TODO(v1.5)` annotation. Surface stability concern: a future load-shedding branch may flip the flag but the helper's `path` annotate is the only operator signal today. Not a security finding; called out as the only behavioral pin worth a doc-comment review pre-tag.
|
||||
|
||||
**Recommendation**: None pre-tag; the TODO is explicit (commit `41506c7a`).
|
||||
|
||||
### L-2 — Drill-down 422 cap can be bypassed by omitting `dayKey`
|
||||
**Where**: `src/lib/validations/measurement.ts:336`
|
||||
|
||||
**Finding**: Verified the Zod `.refine` triggers when `dayKey != null && limit > 1000` returns 422. Confirmed the route at `src/app/api/measurements/route.ts:107` reads `limit` straight through with no second clamp — relies entirely on the validator. The path is **un-bypassable** for `dayKey` requests. Caller asking `limit=999&dayKey=…` is allowed (within validator cap); `limit=1001&dayKey=…` fails-validation; `limit=5000` without `dayKey` takes the legacy 5000-row cap (intentional, FB-D2 windowed path). No bypass.
|
||||
|
||||
**Verified**: Test suite at `src/lib/validations/__tests__/measurement.test.ts:243-272` covers the boundary (1000 accepted, 1001 rejected).
|
||||
|
||||
---
|
||||
|
||||
## Informational
|
||||
|
||||
### I-1 — Cross-tz live-path fallback has the same `userId` scoping as rollup path (verified)
|
||||
The live fallbacks introduced in W-A are byte-for-byte equivalent on the auth surface — every `prisma.measurement.findMany` carries `{ userId, type, measuredAt: { gte: since } }`. The `userTz` parameter is consumed only for `userDayKey(measuredAt, userTz)` re-keying inside the in-process aggregation; it never reaches a WHERE clause or a Prisma-raw substitution at this layer. No scope-leak risk.
|
||||
|
||||
---
|
||||
|
||||
## Confirmed-safe
|
||||
|
||||
### CS-1 — W-C Coach API gate inventory (route-discovery test)
|
||||
- `GET/DELETE /api/insights/chat/[id]` (lines 32, 55) and `POST /api/insights/chat/messages/[id]/feedback` (line 63) now call `requireAssistantSurface("coach")` after `requireAuth` and **before** any data access or rate-limit work.
|
||||
- The discovery test at `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts` (commit `1eb00389` + comment-skip refinement `5ecc3152`) walks every `route.ts` under `/api/insights/**` and groups handlers into Coach-gated / sibling-gated / non-Coach-owned. Orphans fail by name. The non-Coach allowlist (provider-chain, settings, feedback, glp1-timeline, targets) is documented inline with rationale.
|
||||
- The `apiHandler` catch at `src/lib/api-handler.ts:166` maps `AssistantDisabledError` → 403 + `{ data: null, error, meta: { errorCode: "assistant.disabled.coach" } }` — locked iOS contract. Verified the catch fires before `reportToGlitchtip`, so a disabled-surface 403 does **not** noisy-alert ops.
|
||||
- Manual sweep of all 18 routes under `/api/insights/` confirms every Coach-owned handler (`chat/route.ts`, `chat/[id]/route.ts`, `chat/messages/[id]/feedback/route.ts`, `comprehensive/route.ts`, `generate/route.ts`) carries the gate; no orphans.
|
||||
|
||||
### CS-2 — W-B `looksLikeIp` strictness (node:net.isIP)
|
||||
- `src/lib/api-response.ts:100` uses `isIP(s) !== 0` (returns 4/6 for valid v4/v6, 0 for invalid). Test coverage at `src/lib/__tests__/get-client-ip.test.ts:109-136` pins:
|
||||
- rejects `1.2`, `:::`, `gg:hh::1` (previously accepted by the hex/dot regex);
|
||||
- **accepts** `2001:db8::1` (well-formed IPv6);
|
||||
- accepts every well-formed IPv4 chain entry.
|
||||
- The `cf-connecting-ip` branch (`readCfConnectingIp`, line 116) is correctly gated on `TRUST_CF_CONNECTING_IP === "1"` (exact string match — `"true"` and `"yes"` correctly rejected, per test line 215). Behind the flag, the trimmed header is fed through the same `looksLikeIp` validator before being returned. No regression for valid IPv4/IPv6 visitors.
|
||||
|
||||
### CS-3 — W-B drill-down cap via Zod refine
|
||||
See L-2 above. Cannot be bypassed via `limit=999&dayKey=…` (within cap); cannot be bypassed via wider `limit` without `dayKey` (legacy 5000 cap applies to the windowed chart-data path, not the drill-down).
|
||||
|
||||
### CS-4 — W-A cross-tz fallback authorization parity
|
||||
See I-1. Live and rollup paths use identical `{ userId }` scoping. The `tz_guard` annotate exposes only the branch decision, not user data.
|
||||
|
||||
### CS-5 — W-F medication-intake cache invalidation completeness
|
||||
- `invalidateUserMedications` (`src/lib/cache/invalidate.ts:63`) now evicts `caches.analytics.deleteByPrefix(\`${userId}|\`)` in addition to the medication-specific caches (commit `f2f1e5f0` + the v1.4.38 W-F line 74 addition).
|
||||
- Verified call-sites in every medication write path: `/api/medications/route.ts`, `/api/medications/intake/route.ts`, `/api/medications/intake/bulk/route.ts`, `/api/medications/[id]/route.ts`, `/api/medications/[id]/intake/route.ts`, `/api/medications/[id]/intake/[eventId]/route.ts`.
|
||||
- Mood (`invalidateUserMood`) and measurements (`invalidateUserMeasurements`) **already** evict `caches.analytics` (lines 31, 51). Verified write paths: `/api/mood-entries/route.ts:130`, `/api/mood-entries/bulk/route.ts:218`, `/api/mood-entries/[id]/route.ts:113,145`, `/api/measurements/route.ts:472,549`, `/api/measurements/batch/route.ts:379`, `/api/measurements/by-external-ids/route.ts:124`, `/api/measurements/[id]/route.ts:114,176`, `/api/workouts/batch/route.ts:532`.
|
||||
- The dashboard/summary cache (under `caches.analytics`) is therefore consistently invalidated by every measurement, mood, AND medication write path. No missing hook.
|
||||
|
||||
### CS-6 — W-E i18n no AI/Claude/marathon/phase leak in es/fr/it/pl
|
||||
- The German source uses `phase*` keys exclusively for the reminder-phase domain term (`phaseGreen/Yellow/Orange/Red`, `phaseBeforeEnd`, `phaseAfterEnd`); all four new-locale translations follow the same domain pattern. No release-jargon leak.
|
||||
- The only `Claude` strings in es/fr/it/pl appear inside the operator-facing `"anthropic": "Anthropic (Claude)"` settings dropdown label — intentional (operator picks the brand), matches the German source one-for-one.
|
||||
- The Italian `ai` matches are the Italian preposition "a" + "i" ("ai farmaci" = "to the medications"), not the English "AI" acronym.
|
||||
- Settings-page `"ai"` key (line 1622, 1769, 2326) is a path key, not display copy.
|
||||
- No `Claude/claude/AI assistant/marathon/phase X` user-facing leaks introduced by the W-E commits.
|
||||
|
||||
### CS-7 — v1.4.37 security posture regression check
|
||||
- `src/proxy.ts` (HSTS + CSP + TRUST_CF_CONNECTING_IP plumbing) and `src/lib/auth/session.ts` (session destruction): **unchanged** in `v1.4.37.2..HEAD` (`git diff` returns empty). v1.4.37 hardening preserved.
|
||||
- `ensureUserRollupsFresh` fire-and-forget pattern unchanged on the read path (in-flight dedup added at `src/lib/measurements/rollups.ts:582-622`, commit `4e284235` — purely a perf de-duplication, no security surface change). The `getEvent()?.addWarning` annotation introduced in commit `234fb723` for the swallowed error keeps the silent-failure invariant Marc documented.
|
||||
- `/.well-known/` and 404 fall-through behavior: untouched in scope.
|
||||
|
||||
---
|
||||
|
||||
## Most-important pre-tag finding
|
||||
|
||||
**M-1: timezone-write validation gap.** One-line fix in `src/lib/auth/profile-update.ts:14`: add `.refine(isValidTimezone, "Invalid IANA timezone")` (or chain to the existing schema). Without it, a user can self-DoS the dashboard/summary route post-v1.4.38 because the new `to_char(... AT TIME ZONE ${userTz}, ...)` aggregate hard-fails on an invalid zone where the legacy JS path silently fell back. Single-user blast radius (own request only — no cross-tenant), but it's a regression introduced by the W-F SQL rewrite and worth catching before tag.
|
||||
|
||||
## v1.4.37 posture regression: none
|
||||
|
||||
All seven items confirmed unchanged: session destruction, `/.well-known/`, HSTS, CSP, TRUST_CF_CONNECTING_IP, `ensureUserRollupsFresh` fire-and-forget, and the Coach 403 envelope contract.
|
||||
@@ -0,0 +1,314 @@
|
||||
# v1.4.38 W-QA-3 — UX / responsive / a11y findings
|
||||
|
||||
Scope: `v1.4.37.2..HEAD` on `develop`. READ-ONLY review. Live site is
|
||||
v1.4.37.2; develop holds W-A through W-F. Findings are graded P0 →
|
||||
P3 against shipping the v1.4.38 tag.
|
||||
|
||||
Severity legend:
|
||||
- **P0** — must fix before tag (correctness / a11y break / user-data
|
||||
visible bug)
|
||||
- **P1** — should fix before tag (UX regression on a primary surface or
|
||||
i18n string break)
|
||||
- **P2** — fix in v1.4.38.x (polish; not blocking)
|
||||
- **P3** — nice-to-have / log for v1.4.39
|
||||
|
||||
Summary counts:
|
||||
- P0: 1
|
||||
- P1: 2
|
||||
- P2: 3
|
||||
- P3: 4
|
||||
|
||||
---
|
||||
|
||||
## P0 — must fix before tag
|
||||
|
||||
### P0-1 — i18n placeholder break in 3 locales: `medications.glp1NextInjectionDays`
|
||||
|
||||
**File:** `messages/{fr,it,pl}.json` (key `medications.glp1NextInjectionDays`)
|
||||
|
||||
The W-E i18n wave (commit `d95820ab`) translated the GLP-1 cluster but
|
||||
the merged value is a **broken hybrid string** — half-translated label
|
||||
joined to a still-English `(in {days} days)` tail:
|
||||
|
||||
- `fr`: `"Suivant:{label} (in {days} days)"`
|
||||
- `it`: `"Avanti:{label} (in {days} days)"`
|
||||
- `pl`: `"Dalej:{label} (in {days} days)"`
|
||||
|
||||
Expected shape (mirrors the German source):
|
||||
- `de`: `"Nächste:{label} (in {days} Tagen)"`
|
||||
- `en`: `"Next:{label} (in {days} days)"`
|
||||
|
||||
User impact:
|
||||
- `Glp1MedicationCard.nextInjectionLabel()` (`src/components/medications/glp1-medication-card.tsx:301-305`)
|
||||
is the consumer. When a GLP-1 medication's next injection is ≥ 2
|
||||
days away (the modal case for weekly drugs) the user sees a mid-
|
||||
string English fragment inside otherwise fully-localised French /
|
||||
Italian / Polish copy.
|
||||
- Marc-Voice violation: ships as English literal inside three locales.
|
||||
- Also the missing space after the colon (`Suivant:{label}` →
|
||||
`Suivant : {label}` would be the French typographic convention) is a
|
||||
secondary nit but the **English-tail leak is the blocker**.
|
||||
|
||||
The W-E report tested `i18n-drift-guard` and `quick-add-labels` but the
|
||||
GLP-1-NextInjectionDays key has no contract test pinning placeholder
|
||||
shape; the hybrid passed silently. Recommend a 4th test in
|
||||
`quick-add-labels` style for the GLP-1 cluster as the follow-up.
|
||||
|
||||
Suggested fix (one-line per locale):
|
||||
- `fr`: `"Suivant : {label} (dans {days} jours)"`
|
||||
- `it`: `"Prossima: {label} (tra {days} giorni)"`
|
||||
- `pl`: `"Następne: {label} (za {days} dni)"`
|
||||
|
||||
---
|
||||
|
||||
## P1 — should fix before tag
|
||||
|
||||
### P1-1 — Polish `compliance7d` divergence: parenthesised vs. unparenthesised duplicate
|
||||
|
||||
**File:** `messages/{fr,it,pl}.json`
|
||||
|
||||
Grep confirms two competing renderings of the same key shipped across
|
||||
the i18n waves:
|
||||
|
||||
- `compliance7d` appears twice in `pl.json` (lines differ):
|
||||
- `"Przestrzeganie (7 dni)"` (with parens)
|
||||
- `"Przestrzeganie 7 dni"` (without)
|
||||
- Same drift in `fr.json` (`"Observance (7 jours)"` vs `"Observance
|
||||
7 jours"`) and `it.json` (`"Aderenza (7 giorni)"` vs `"Aderenza 7
|
||||
giorni"`).
|
||||
|
||||
Likely cause: two separate W-E commits (`03952deb` medications status
|
||||
chips and `b776daf5` insights health-score / Coach) each added their
|
||||
own version. JSON parser keeps the **second** declaration of a
|
||||
duplicate key, so the user sees the second value at runtime and the
|
||||
first one is dead — but the JSON file is technically malformed
|
||||
(RFC-8259 says "objects SHOULD have unique names").
|
||||
|
||||
Action:
|
||||
1. De-duplicate. Pick the parenthesised form for the medication
|
||||
compliance bar (matches DE `"Compliance (7d)"` rhythm) and drop the
|
||||
parenless siblings.
|
||||
2. Add a `i18n-locale-integrity` assertion that flags duplicate keys
|
||||
within a JSON tree (currently only flags missing/extra keys).
|
||||
|
||||
### P1-2 — Coach gated routes: client error-mapping has no `assistant.disabled.coach` branch
|
||||
|
||||
**Files:**
|
||||
- `src/components/insights/coach-panel/use-coach.ts:359-407`
|
||||
- `src/components/insights/coach-panel/coach-drawer.tsx`
|
||||
- `messages/{de,en}.json` — no `coach.disabled` / `assistant.disabled`
|
||||
copy detected via grep
|
||||
- Backend: `src/app/api/insights/chat/[id]/route.ts:32,55` +
|
||||
`src/app/api/insights/chat/messages/[id]/feedback/route.ts:63`
|
||||
(W-C properly added `requireAssistantSurface("coach")`)
|
||||
|
||||
The W-C orphan-API gating is **correct on the server** — the 403
|
||||
envelope carries `meta.errorCode = "assistant.disabled.coach"`
|
||||
(`src/lib/feature-flags/index.ts:137-147`). But the client
|
||||
`useSendCoachMessage` send path reads `response.json().error` and falls
|
||||
through to `coach.http.403`:
|
||||
|
||||
```ts
|
||||
// use-coach.ts:380-401
|
||||
const envelope = (await response.clone().json()) as { error?: unknown };
|
||||
if (typeof envelope?.error === "string") {
|
||||
structured = envelope.error; // "assistant.disabled.coach"
|
||||
}
|
||||
setStreaming({ ..., errorCode: structured ?? `coach.http.${response.status}` });
|
||||
```
|
||||
|
||||
The errorCode is preserved on `streaming.errorCode`, but the drawer's
|
||||
error toast/copy table does **not** branch on `assistant.disabled.coach`
|
||||
— the envelope's `meta.errorCode` slot is also ignored (the client
|
||||
reads `.error`, the server emits the human code via `.error` AND the
|
||||
machine code via `.meta.errorCode`).
|
||||
|
||||
Result for the user scenario the task brief asked about (Coach OFF +
|
||||
stale client opens the drawer):
|
||||
- `GET /api/insights/chat` 403 → conversations list reads
|
||||
`coach.disabled.coach` as the error string → renders as a generic
|
||||
TanStack `isError` state (no copy)
|
||||
- POST attempts → drawer shows `coach.http.403` (literal token) or
|
||||
raw `assistant.disabled.coach` token, neither of which has a
|
||||
translation entry
|
||||
|
||||
User sees either: empty rail with no explanation, or a literal
|
||||
machine-code error string. Not a crash, but **not a clean operator-
|
||||
disabled message** either.
|
||||
|
||||
Recommend either:
|
||||
- (preferred) wire a `coach.disabled` / `assistant.disabled` copy key
|
||||
across `messages/{de,en,es,fr,it,pl}.json` and branch the drawer's
|
||||
error renderer on `errorCode === "assistant.disabled.coach"` to show
|
||||
it.
|
||||
- (fallback) gate the entire Coach drawer mount on a client-side flag
|
||||
hydrated from `/api/feature-flags` so a stale client never tries to
|
||||
POST.
|
||||
|
||||
---
|
||||
|
||||
## P2 — fix in v1.4.38.x
|
||||
|
||||
### P2-1 — `MeasurementList` drill-down `aria-controls` panel id is on `TableRow` (desktop)
|
||||
|
||||
**File:** `src/components/measurements/measurement-list.tsx:626`
|
||||
|
||||
W-D P1-1 (`1fd76f26`) added `aria-controls={drilldownId}` on the
|
||||
chevron button and `id={drilldownId}` on the disclosed `<TableRow>`.
|
||||
|
||||
Browser-side WAI-ARIA expects the controlled panel to be a region the
|
||||
AT can describe coherently; some screen readers (NVDA + VoiceOver
|
||||
Safari) skip `aria-controls` when the target is a `<tr>` because it
|
||||
lives inside the implicit `<tbody>` and doesn't expose a landmark.
|
||||
Mobile branch (line 754) wraps in a plain `<div>` which works.
|
||||
|
||||
Low-impact (the `aria-expanded` toggle still announces) but the
|
||||
desktop chevron's promise of "controls X panel" doesn't fully land on
|
||||
VoiceOver-Safari. Consider wrapping the drill-down `DayDrillDown` in
|
||||
a `<div role="region">` rendered inside the `<TableCell colSpan={6}>`
|
||||
and moving the `id` onto that wrapper.
|
||||
|
||||
### P2-2 — `quickAddMedicationIntake` translations diverge in semantic register
|
||||
|
||||
**Files:** `messages/{de,en,es,fr,it,pl}.json` key
|
||||
`dashboard.quickAddMedicationIntake`
|
||||
|
||||
- en: "Log medication intake"
|
||||
- de: "Einnahme erfassen" (short — W-D P1-3 fix)
|
||||
- es: "Registrar toma"
|
||||
- fr: "Saisir une prise"
|
||||
- it: "Registra assunzione"
|
||||
- pl: "Zarejestruj przyjęcie"
|
||||
|
||||
The 3-row dropdown menu in `page.tsx:586-606` reads top-to-bottom as
|
||||
"Messung / Stimmung / Einnahme erfassen" on DE — the verb is the
|
||||
SAME ("erfassen"). On FR the verbs differ ("Saisir une mesure" /
|
||||
"Saisir une humeur" / "Saisir une prise" — assumed; please verify the
|
||||
other two rows match `Saisir`). On ES, IT, PL the verb register is
|
||||
correct.
|
||||
|
||||
Not a translation **error**, but Marc-Voice asks each locale's menu to
|
||||
read with the same verb rhythm as DE/EN. Verify and align in v1.4.38.x.
|
||||
|
||||
### P2-3 — `DropdownMenuContent max-w-[calc(100vw-2rem)]` clipping risk
|
||||
|
||||
**File:** `src/app/page.tsx:577`
|
||||
|
||||
W-D P1-3 added `max-w-[calc(100vw-2rem)]` to the Hinzufügen dropdown.
|
||||
|
||||
On 320 px viewport (iPhone SE 1st gen):
|
||||
- `100vw - 2rem` = `320 - 32` = `288 px`
|
||||
- Polish "Zarejestruj przyjęcie" + `Pill` icon + `mr-2` gap +
|
||||
DropdownMenuItem internal `px-2` padding ≈ 260-270 px — fits.
|
||||
|
||||
On 280 px (Galaxy Fold folded):
|
||||
- max-w = `280 - 32` = `248 px`
|
||||
- Polish row at ≈ 270 px would clip / wrap.
|
||||
|
||||
Lucide icon + text wrap to a second line in the DropdownMenu (the
|
||||
shadcn default is `flex` not `flex-wrap`), so the row would still be
|
||||
readable but the icon would sit centred over a 2-line label —
|
||||
visually awkward. Acceptable for v1.4.38 (Galaxy Fold is < 0.1% of
|
||||
traffic) but worth a `whitespace-nowrap` audit pass before v1.5.
|
||||
|
||||
---
|
||||
|
||||
## P3 — log for v1.4.39
|
||||
|
||||
### P3-1 — Select trigger `pr-2` vs Safari date-input
|
||||
|
||||
**File:** `src/components/ui/select.tsx:56`
|
||||
|
||||
W-D P1-6 (`b6aefc67`) cut `pr-2.5` to `pr-2`. Confirmed visually
|
||||
consistent now on Chromium-Material AND Safari/Chromium-legacy
|
||||
gutters. No follow-up needed; logged so the v1.4.39 design-token sweep
|
||||
doesn't re-lift it.
|
||||
|
||||
### P3-2 — GLP-1 / generic medication card icon pill colour pairing
|
||||
|
||||
**Files:** `glp1-medication-card.tsx:413-422`,
|
||||
`medication-card.tsx:312-321`
|
||||
|
||||
W-D P2-3 applied `CircleCheck` / `AlertCircle` / `AlertTriangle`
|
||||
symmetrically to both cards. Verified parity. The icons are
|
||||
`size-3.5` (14 px) which is at the low end of the legibility floor —
|
||||
icons-paired-with-colour passes WCAG 1.4.1 regardless of icon size,
|
||||
but on a P3 OLED at 100% scaling the icon is barely distinguishable
|
||||
from the text glyph. Bump to `size-4` in v1.4.39.
|
||||
|
||||
### P3-3 — Dashboard sparkline DAY-bucket mean visual character
|
||||
|
||||
**File:** `src/app/api/dashboard/summary/route.ts:323-334`
|
||||
|
||||
W-F now reads `measurement_rollups.mean WHERE granularity='DAY'`
|
||||
instead of the raw measurement firehose. For sparklines (≤ 7 points,
|
||||
one per calendar day) this is **byte-identical** to the prior result
|
||||
for single-sample-per-day metrics (weight, BMI, BP) because the day's
|
||||
single sample IS the day's mean.
|
||||
|
||||
For high-frequency metrics (steps, sleep duration, active energy) the
|
||||
prior raw-data sparkline rendered the **last sample of the day** —
|
||||
the rollup change paints the **mean of the day**. Visually:
|
||||
- Steps: rollup mean of per-source step samples can read materially
|
||||
different from "the day's headline total". Steps is a CUMULATIVE
|
||||
type — the v1.4.37 W7c daily-sum collapse is what the user expects.
|
||||
Confirm `measurement_rollups.mean` for cumulative types is
|
||||
populated as a SUM-day-mean (i.e. one row per day with the day
|
||||
total) or rework the sparkline projection to use `r.sum` instead of
|
||||
`r.mean` for the CUMULATIVE_DAY_SUM_TYPES set.
|
||||
- Sleep: mean across a day reads close to the single nightly
|
||||
reading; safe.
|
||||
- Pulse: a meaningful smoothing — actually nicer than the raw last-
|
||||
sample noise.
|
||||
|
||||
**Investigation flag (not a defect yet):** verify the
|
||||
`measurement_rollups` aggregator emits `mean` correctly for
|
||||
CUMULATIVE_DAY_SUM_TYPES; if it emits true mean across per-hour
|
||||
chunks, the dashboard steps tile sparkline will read low compared to
|
||||
the daily-sum that the metric strip's headline value shows.
|
||||
|
||||
### P3-4 — JSON duplicate-key audit drift
|
||||
|
||||
The P1-1 duplicate `compliance7d` keys would have been caught by a
|
||||
linter; suggest adding a `pnpm test:i18n-uniqueness` step that walks
|
||||
`messages/*.json` and asserts each leaf path is declared exactly once.
|
||||
|
||||
---
|
||||
|
||||
## Re-confirmation of v1.4.37.x perf wins at the UI layer
|
||||
|
||||
- `HealthScoreCard` full-height: W-D P2-4 explicit no-op. Verified
|
||||
the `grid-rows-[auto_auto_auto_auto_auto_1fr_auto]` track holds.
|
||||
- Dropdown nowrap: W-D P1-3 applied via DE label shortening + max-w
|
||||
clamp. Confirmed.
|
||||
- Target card gap (`gap-3 md:gap-4`): W-D P1-5 no-op confirm. Status
|
||||
pill is on title row, not headline-adjacent. Confirmed.
|
||||
- Mood chart parity: not in this diff's scope; no regression
|
||||
introduced.
|
||||
- Dashboard button alignment (`items-center sm:items-start`): W-D
|
||||
P2-5 explicit no-op confirm. Behaviour preserved.
|
||||
|
||||
All v1.4.37.x perf wins **remain visible at the UI layer**; nothing
|
||||
in v1.4.38 reverses them.
|
||||
|
||||
---
|
||||
|
||||
## Files inspected
|
||||
|
||||
- `src/components/measurements/measurement-list.tsx`
|
||||
- `src/components/ui/select.tsx`
|
||||
- `src/components/medications/glp1-medication-card.tsx`
|
||||
- `src/components/medications/medication-card.tsx`
|
||||
- `src/components/insights/insight-status-card.tsx`
|
||||
- `src/components/onboarding/getting-started-checklist.tsx`
|
||||
- `src/components/settings/arztbericht-hero-card.tsx`
|
||||
- `src/components/dashboard/medication-intake-quick-add.tsx`
|
||||
- `src/app/page.tsx`
|
||||
- `src/app/api/insights/chat/[id]/route.ts`
|
||||
- `src/app/api/insights/chat/messages/[id]/feedback/route.ts`
|
||||
- `src/components/insights/coach-panel/use-coach.ts`
|
||||
- `src/lib/feature-flags/index.ts`
|
||||
- `src/app/api/dashboard/summary/route.ts`
|
||||
- `messages/{de,en,es,fr,it,pl}.json` (sampled)
|
||||
- `.planning/phase-W-D-v1438-ux-polish-report.md`
|
||||
- `.planning/phase-W-E-v1438-i18n-localization-report.md`
|
||||
@@ -0,0 +1,258 @@
|
||||
# W-QA4 — v1.4.38 architecture / data-integrity findings
|
||||
|
||||
Diff: `v1.4.37.2..HEAD` on `develop`, READ-ONLY architectural audit.
|
||||
Lane: data integrity, runtime correctness, cache / dedup semantics — no
|
||||
copy / UX / i18n review.
|
||||
|
||||
Severity legend:
|
||||
|
||||
- **CRITICAL** — would corrupt user data, leak the surface, or break the
|
||||
release on the canonical Berlin tenant.
|
||||
- **HIGH** — would mis-compute a number on a non-canonical tenant or
|
||||
silently regress under a realistic workload.
|
||||
- **MEDIUM** — edge case behind the current product, but worth a follow-up.
|
||||
- **LOW** — cosmetic / documentation / future-proofing.
|
||||
|
||||
---
|
||||
|
||||
## Severity counts
|
||||
|
||||
| Severity | Count |
|
||||
| ---------- | ----- |
|
||||
| CRITICAL | 0 |
|
||||
| HIGH | 0 |
|
||||
| MEDIUM | 3 |
|
||||
| LOW | 4 |
|
||||
|
||||
The diff is tight and the test coverage on the new helpers
|
||||
(`isNearUtc`, `ensureUserRollupsFresh` dedup, the validator refine on
|
||||
`dayKey + limit`, the `bp-in-target` priorYear cushion, the gate
|
||||
inventory) is genuinely good. No pre-tag blockers found.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM-1 — Coach gate inventory walker only matches `route.ts`
|
||||
|
||||
**File:** `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts:112`
|
||||
|
||||
The walker decides whether to inspect a file with
|
||||
`if (entry !== "route.ts") continue;`. Next.js App Router accepts
|
||||
`route.ts`, `route.tsx`, `route.js`, `route.jsx`, `route.mjs`, and
|
||||
`route.mts`. The directive in the user brief explicitly asked whether
|
||||
the walker handles `tsx / jsx / mts` variants — it does not.
|
||||
|
||||
In the current repo every insights route is `route.ts`, so this is not
|
||||
a live miss. But a future contributor landing
|
||||
`src/app/api/insights/<foo>/route.tsx` whose handler invokes the Coach
|
||||
stack would be invisible to the discovery test, defeating the whole
|
||||
point of the inventory. The matching `__tests__/` skip on
|
||||
line 107 IS correct.
|
||||
|
||||
Fix shape (no patch — READ-ONLY): match `entry.startsWith("route.")` +
|
||||
a closed-set extension check, or use a glob with the four valid
|
||||
extensions.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM-2 — `ensureUserRollupsFresh` per-userId Map is per-process
|
||||
|
||||
**File:** `src/lib/measurements/rollups.ts:582-624`
|
||||
|
||||
The dedup map (`ensureUserRollupsFreshInFlight: Map<string, Promise>`)
|
||||
correctly:
|
||||
|
||||
- shares the in-flight promise across concurrent same-userId callers,
|
||||
- deletes the slot in `finally` so a rejection does not poison the
|
||||
cache (covered by tests at `rollups.test.ts:350-367`),
|
||||
- short-circuits the SQL fan-out down to one round-trip per probe
|
||||
(covered at `rollups.test.ts:302-331`).
|
||||
|
||||
Memory growth is **not** a concern — the map is bounded by the count of
|
||||
distinct userIds currently mid-recompute, and entries delete in
|
||||
`finally`. A stuck Promise would leak one entry per stuck user, but
|
||||
the inner work has its own try/catch that always resolves.
|
||||
|
||||
The multi-process caveat: HealthLog runs as a single Next.js server +
|
||||
a single pg-boss worker, so per-process dedup is sufficient today.
|
||||
If v1.5 horizontally scales the Next layer (multiple containers behind
|
||||
the load balancer), each container would have its own Map and a cold
|
||||
fan-out could still queue N parallel recomputes (N = container count,
|
||||
not request count). At the canonical 1-tenant scale this is academic;
|
||||
worth flagging in the v1.5 capacity plan if Marc ever scales out.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM-3 — Dashboard sparkline has no rollup fallback
|
||||
|
||||
**File:** `src/app/api/dashboard/summary/route.ts:323-335`
|
||||
|
||||
The sparkline sub-query reads `measurement_rollups` directly with no
|
||||
live-fallback branch:
|
||||
|
||||
```sql
|
||||
SELECT r."type", r."bucket_start", r."mean"
|
||||
FROM measurement_rollups r
|
||||
WHERE r."user_id" = $userId
|
||||
AND r."granularity" = 'DAY'
|
||||
AND r."bucket_start" >= $sevenDaysAgo
|
||||
```
|
||||
|
||||
For a brand-new account or a freshly-restarted instance where the boot
|
||||
backfill (`enqueueBootTimeRollupBackfill`) has not yet folded the
|
||||
user's DAY buckets, this query returns `[]`, `sparkByType` is empty,
|
||||
and every emitted card has `sparkline: []`. The route still returns
|
||||
200 (no 500 risk — the brief asked about this), but the iOS tile
|
||||
paints a flat / missing sparkline until the worker catches up.
|
||||
|
||||
`latestIn7d` still reads `measurements` directly, so the headline
|
||||
`latestValue` is correct on cold mount. Only the sparkline trend is
|
||||
empty. That is degraded UX, not data corruption. v1.4.35.1's boot
|
||||
backfill closes the gap within minutes of first boot.
|
||||
|
||||
Acceptable for v1.4.38 release; document in the v1.5 plan that the
|
||||
sparkline sub-query could fall through to a single
|
||||
`date_trunc('day', measured_at) GROUP BY` against `measurements` when
|
||||
the rollup result is empty for a covered user.
|
||||
|
||||
---
|
||||
|
||||
## LOW-1 — `isNearUtc` ±3h boundary is inclusive
|
||||
|
||||
**File:** `src/lib/tz/format.ts:96-100`
|
||||
|
||||
`return Math.abs(offsetMinutes) <= 3 * 60` — inclusive of ±180 min.
|
||||
Behaviour:
|
||||
|
||||
- `Europe/Berlin` summer +120 min, winter +60 min → near-UTC year-round (correct).
|
||||
- `Europe/Moscow` +180 min year-round → near-UTC (boundary inclusive, intentional per test on line 281-286).
|
||||
- `Pacific/Honolulu` -600 min → NOT near-UTC → live fallback (correct).
|
||||
- `Asia/Tokyo` +540 min → NOT near-UTC (correct).
|
||||
- `Pacific/Auckland` +720/+780 min → NOT near-UTC (correct).
|
||||
- `Pacific/Kiritimati` +840 min (Kiribati, +14) → NOT near-UTC (correct, falls through to live path).
|
||||
|
||||
DST behaviour confirmed in tests at `resolver.test.ts:265-308` — the
|
||||
helper re-derives the offset per call via `tzOffsetMinutes(now, tz)` so
|
||||
a Berlin user crossing the March DST boundary sees +60 → +120 inside
|
||||
the same logical day and stays near-UTC on both sides. **No behavioural
|
||||
issue** — flagged LOW only because the inclusive boundary at exactly
|
||||
±180 means the Atlantic/Cape_Verde (-1h) and Atlantic/Azores (-1h
|
||||
winter / 0h summer) zones sit comfortably inside, but a future zone
|
||||
that drifts to ±181 would jump branches without warning. Not a fix —
|
||||
just a property to be aware of.
|
||||
|
||||
---
|
||||
|
||||
## LOW-2 — `recomputeBucketsForMeasurement` parallel enqueue is safe
|
||||
|
||||
**File:** `src/lib/measurements/rollups.ts:209-220`
|
||||
|
||||
The three `enqueueRollupRecompute` calls are independent — each carries
|
||||
a distinct `singletonKey` (`${userId}|${type}|${granularity}|${from}`)
|
||||
and `boss.send` has no shared state between calls. Fanning them out
|
||||
via `Promise.all` is correct.
|
||||
|
||||
The internal pg-boss connection pool handles concurrent sends fine;
|
||||
there's no shared-state contention. **No issue.** Test coverage at
|
||||
`rollups.test.ts:188-196` proves the three queue sends fire.
|
||||
|
||||
---
|
||||
|
||||
## LOW-3 — drill-down 1000-cap validator fires before route handler
|
||||
|
||||
**File:** `src/lib/validations/measurement.ts:336-340`, route at
|
||||
`src/app/api/measurements/route.ts:45-49`.
|
||||
|
||||
Confirmed: the route's first action after auth is
|
||||
|
||||
```
|
||||
const parsed = listMeasurementsSchema.safeParse(params);
|
||||
if (!parsed.success) return apiError(parsed.error.issues[0].message, 422);
|
||||
```
|
||||
|
||||
The refine is on the schema itself (top-level `.refine(({ limit, dayKey }) => …)`),
|
||||
so an oversized request returns 422 before any DB read. **Correctly
|
||||
gated.** The hard cap inside the route is gone (the old
|
||||
`Math.min(limit, 1000)` was removed).
|
||||
|
||||
---
|
||||
|
||||
## LOW-4 — Health-Score prior-week BP definition matches current week
|
||||
|
||||
**File:** `src/app/api/analytics/route.ts:387-415`,
|
||||
`src/lib/analytics/health-score-fast-path.ts:140-143`.
|
||||
|
||||
Confirmed: both `computeBpInTargetFastPath` invocations use the same
|
||||
`targets`, `coverage`, and `userTz` — only `now` differs (one at
|
||||
`now`, one at `now - 7d`). The `bp_in_target` predicate, the per-day
|
||||
mean pairing logic, and the rollup-vs-live branch decision are
|
||||
identical. The delta in the Health-Score helper is therefore a true
|
||||
week-over-week BP movement, not an apples-to-oranges comparison.
|
||||
|
||||
`bpInTargetPctPriorWeek` falls back to `bpInTargetPct` when undefined
|
||||
(legacy callers), preserving pre-v1.4.38 behaviour exactly (BP cancels
|
||||
out of the delta). **Correctly threaded.**
|
||||
|
||||
---
|
||||
|
||||
## Cache invalidation match — verified, no collision
|
||||
|
||||
**Files:** `src/app/api/dashboard/summary/route.ts:240`,
|
||||
`src/lib/cache/invalidate.ts:30-75`.
|
||||
|
||||
Route cache key: `${user.id}|dashboard-summary` on `caches.analytics`.
|
||||
|
||||
- `invalidateUserMeasurements` calls `caches.analytics.deleteByPrefix(\`${userId}|\`)` (line 31).
|
||||
- `invalidateUserMedications` calls the same (line 74, the v1.4.38 W-F addition).
|
||||
- `invalidateUserMood` calls the same (line 51).
|
||||
|
||||
The `${userId}|` prefix is unique per user; no collision risk between
|
||||
users (the pipe character guarantees a tenant boundary because Prisma
|
||||
cuid2 ids cannot contain `|`). **Both write hooks correctly evict the
|
||||
dashboard summary bucket.**
|
||||
|
||||
---
|
||||
|
||||
## Most-important pre-tag concern
|
||||
|
||||
**None.** All three MEDIUM findings are post-release follow-ups, not
|
||||
blockers. The Coach route inventory works for every file in the repo
|
||||
today; the in-flight dedup map is single-process-correct for the
|
||||
current deploy shape; and the sparkline empty-on-cold-rollup degrades
|
||||
to a flat line, not a 500.
|
||||
|
||||
The release is architecturally clean.
|
||||
|
||||
---
|
||||
|
||||
## Cross-cutting recommendation for v1.5
|
||||
|
||||
The repeated "rollup-fast-path + isNearUtc guard + live fallback"
|
||||
shape now exists in three places — `bp-in-target-fast-path.ts`,
|
||||
`correlations-fast-path.ts`, and (implicitly via the sparkline empty
|
||||
case) `dashboard/summary/route.ts`. A fourth instance is one
|
||||
follow-up away (Health-Score weight pillar already has an
|
||||
`isFullyCovered` branch but no `isNearUtc` guard because the
|
||||
underlying weight series is sampled, not bucketed).
|
||||
|
||||
v1.5 should extract the branch decision into a single helper:
|
||||
|
||||
```ts
|
||||
type RollupReadMode = "rollup" | "live-near-utc-fallback" | "live-coverage-fallback";
|
||||
function decideRollupReadMode(args: {
|
||||
coverage: RollupCoverageMap;
|
||||
requiredTypes: MeasurementType[];
|
||||
userTz: string;
|
||||
now: Date;
|
||||
}): RollupReadMode;
|
||||
```
|
||||
|
||||
— so every consumer dispatches through the same gate and a future
|
||||
v1.5 enhancement (per-user-TZ rollup buckets, removing the ±3h
|
||||
guard) flips behaviour in one place. Today each helper duplicates
|
||||
~6 lines of branch logic and the `meta.*.path` annotate is
|
||||
copy-pasted three times.
|
||||
|
||||
The same refactor should thread `userTz` into `readRollupBuckets`
|
||||
itself so the rollup table can address local-day buckets — that is
|
||||
the v1.5 follow-up the bp-in-target comment at line 142 explicitly
|
||||
calls out.
|
||||
@@ -0,0 +1,165 @@
|
||||
# Phase W-QA-6 — v1.4.38 i18n Runtime Parity Audit
|
||||
|
||||
Scope: `messages/{de,en,es,fr,it,pl}.json` after W-E wave.
|
||||
Diff range: `v1.4.37.2..HEAD` (develop, snapshot 2026-05-17).
|
||||
Mode: READ-ONLY i18n parity audit.
|
||||
|
||||
---
|
||||
|
||||
## Headline numbers
|
||||
|
||||
| Locale | Keys | == DE (untranslated source leak) | == EN (fallback to English) |
|
||||
| ------ | ---: | -------------------------------: | --------------------------: |
|
||||
| de | 2427 | — | 6.8% (English-only product strings carried in DE file) |
|
||||
| en | 2427 | 166 (6.8%) | — |
|
||||
| es | 2427 | 104 (4.3%) | **897 (37.0%)** |
|
||||
| fr | 2427 | 116 (4.8%) | **948 (39.1%)** |
|
||||
| it | 2427 | 118 (4.9%) | **915 (37.7%)** |
|
||||
| pl | 2427 | 119 (4.9%) | **898 (37.0%)** |
|
||||
|
||||
Structural parity: **PASS** — every locale carries the identical 2427-key tree (0 missing, 0 extra vs DE).
|
||||
UTF-8 / mojibake scan: **PASS** — 0 mojibake sequences across all six locales; 50-string diacritic spot-checks all round-trip cleanly.
|
||||
|
||||
---
|
||||
|
||||
## Critical
|
||||
|
||||
_None._ No blockers for shipping; placeholders fail safe (TS lint would have caught hard breaks).
|
||||
|
||||
---
|
||||
|
||||
## High
|
||||
|
||||
### H-1. ICU placeholder dropped — pagination string loses `{count}` in es/fr/it/pl
|
||||
|
||||
`medications.intakeHistoryPageInfo`
|
||||
|
||||
- DE: `Seite {page} von {total} · {count} Einträge`
|
||||
- ES: `Página {page} de {total}`
|
||||
- FR: `Page {page} sur {total}`
|
||||
- IT: `Pagina {page} di {total}`
|
||||
- PL: `Strona {page} z {total}`
|
||||
|
||||
All four W-E translations dropped the `{count} Einträge` clause. Users on those locales lose the per-page entry count. next-intl will silently ignore the missing var but the UI parity vs DE/EN breaks.
|
||||
|
||||
**Fix:** re-append the count clause in all four locales. Suggested:
|
||||
- es: `Página {page} de {total} · {count} registros`
|
||||
- fr: `Page {page} sur {total} · {count} entrées`
|
||||
- it: `Pagina {page} di {total} · {count} voci`
|
||||
- pl: `Strona {page} z {total} · {count} pozycji`
|
||||
|
||||
### H-2. ICU placeholder dropped — weekday aria-labels lose `{day}` in es/fr/it/pl
|
||||
|
||||
`medications.dayActivate` / `medications.dayDeactivate`
|
||||
|
||||
- DE: `{day} aktivieren` / `{day} deaktivieren`
|
||||
- ES: `Activar día` / `Desactivar día`
|
||||
- FR: `Activer le jour` / `Désactiver le jour`
|
||||
- IT: `Attiva giorno` / `Disattiva giorno`
|
||||
- PL: `Włącz dzień` / `Wyłącz dzień`
|
||||
|
||||
These are a11y labels per weekday chip; without `{day}` interpolation, every chip announces the same generic "Activate day" to screen-reader users instead of "Activate Monday / Tuesday / …".
|
||||
|
||||
**Fix:** restore `{day}` in all four:
|
||||
- es: `Activar {day}` / `Desactivar {day}`
|
||||
- fr: `Activer {day}` / `Désactiver {day}`
|
||||
- it: `Attiva {day}` / `Disattiva {day}`
|
||||
- pl: `Włącz {day}` / `Wyłącz {day}`
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### M-1. ES — blood-pressure term split (`tensión` vs `presión`)
|
||||
|
||||
`Blutdruck` is rendered two different ways in `es.json`:
|
||||
|
||||
- 16 keys use **`tensión arterial`** (e.g. `doctorReport.bpClassificationTitle`, `doctorReport.avgBp`, `doctorReport.typeBpSys`, `doctorReport.typeBpDia`, `measurements.subtitle`).
|
||||
- 11 keys use **`presión arterial`** (e.g. `dashboard.bloodPressure`, `measurements.typeBloodPressure`, `medications.categoryBloodPressure`, `insights.coach.metric.bp`, `doctorReport.sections.bp`).
|
||||
|
||||
Both are clinically valid in Spanish, but the same metric should read one way across dashboard, doctor report, and Coach. Recommend choosing **`presión arterial`** (medically dominant, matches WHO Spanish guidelines) and unifying the 16 `tensión` keys to it.
|
||||
|
||||
### M-2. PL — blood-pressure term split (`ciśnienie tętnicze` vs `ciśnienie krwi`)
|
||||
|
||||
- 6 keys use **`ciśnienie tętnicze`** (clinical: `doctorReport.bpClassificationTitle`, `doctorReport.avgBp`, `measurements.subtitle`).
|
||||
- 9 keys use **`ciśnienie krwi`** (lay: `doctorReport.sections.bp`, `dashboard.bloodPressure`, `measurements.typeBloodPressure`).
|
||||
|
||||
The clinical sections (doctor report) say `tętnicze` while the same doctor report's section heading says `krwi`. Unify to **`ciśnienie tętnicze`** in clinical / doctor-report contexts; keep `ciśnienie krwi` only if intentional in casual dashboard copy. Currently the inconsistency reads as a translation oversight.
|
||||
|
||||
### M-3. PL — pulse term split (`tętno` vs `puls`)
|
||||
|
||||
20 keys use `tętno`, 11 keys still carry the German fallback `puls` (lowercase substring match). Most of the 11 are likely DE/EN-fallback strings rather than active mistranslations, but worth a sweep — `puls` is also a Polish word so it does not look obviously wrong, just inconsistent. Unify on **`tętno`** to match the dominant choice.
|
||||
|
||||
### M-4. EN-fallback remains 37–39% across es/fr/it/pl — T3 surface
|
||||
|
||||
W-E correctly excluded the heavy `admin`, `settings`, and `achievements` namespaces. The leftover EN-fallback breaks down (top 4 namespaces):
|
||||
|
||||
| Locale | admin | settings | achievements | medications |
|
||||
| -----: | ----: | -------: | -----------: | ----------: |
|
||||
| es | 337 | 328 | 149 | 30 |
|
||||
| fr | 341 | 329 | 149 | 41 |
|
||||
| it | 340 | 328 | 149 | 37 |
|
||||
| pl | 340 | 328 | 149 | 31 |
|
||||
|
||||
`admin` + `settings` are documented as intentional T3 deferral (developer/operator surface, low user impact). `achievements` (149 keys × 4 locales) is the largest remaining user-facing gap. The smaller per-locale residuals (medications, insights, doctorReport, charts) are 4-41 strings each — quick to mop up in a follow-up wave.
|
||||
|
||||
**Action:** add an entry to `.planning/v1.4.38-backlog.md` documenting the T3 gap explicitly: "admin/settings/achievements + ~30-40 per-locale stragglers fall back to EN; tracked, not blocking."
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### L-1. IT — longest strings approach 320 px clip threshold
|
||||
|
||||
Six IT strings in the dashboard / insights surface run > 35 chars and > 1.15× the DE source. None exceed practical button widths, but two warrant a visual check at 320 px viewport:
|
||||
|
||||
- `insights.hrv.chartTitle` — IT `Variabilità della frequenza cardiaca` (36 chars vs DE 21).
|
||||
- `measurements.subtitle` — IT `Peso, pressione arteriosa, pulsazioni e altro` (45 chars vs DE 33). Used as a subtitle, normally low clip risk.
|
||||
|
||||
Recommend a 320 px Playwright snapshot on `/insights` and `/measurements` in IT before release; if clipping shows up, shorten to `HRV` or `Variabilità FC`.
|
||||
|
||||
### L-2. FR — `fr.insights.trendAnnotation.pendingLabel` is 46 chars
|
||||
|
||||
`Commentaire de tendance en cours de génération` vs DE 30 chars. Used as a pill on the trend card. Shorten suggestion: `Commentaire en cours…`.
|
||||
|
||||
### L-3. ES — empty-state titles run 36–40 chars
|
||||
|
||||
Several `insights.emptyState.*.title` and `mood.emptyTitle/emptyFilteredTitle` in ES are 35-40 chars. Empty-state cards have more horizontal room than buttons, so clipping risk is low, but H1 wrap can look awkward on 320 px. Recommend visual spot-check; no edit needed unless wrap is ugly.
|
||||
|
||||
### L-4. ChatGPT / OpenAI brand mentions in `settings.*`
|
||||
|
||||
26 keys per locale contain `ChatGPT` / `OpenAI`. These are correct provider labels in the AI-settings panel and not Marc-Voice violations. The user-facing **Coach** / **Insights** strings contain no leaked `Claude`, `Anthropic`, `LLM`, `hallucinate`, `marathon`, `wave`, or `phase` language anywhere across all six locales. **PASS** on Marc-Voice register.
|
||||
|
||||
### L-5. EN file still carries 166 untranslated DE strings
|
||||
|
||||
6.8% of `en.json` is identical to `de.json`. These are mostly proper nouns (medication names, brand strings) and one technical leftover (`measurements.typePulseWaveVelocity: Pulse-wave velocity` — actually English, just happens to == DE). Not a regression, but worth a one-off pass at next opportunity to confirm none are genuine EN gaps.
|
||||
|
||||
---
|
||||
|
||||
## What was checked and passed cleanly
|
||||
|
||||
- Structural parity (key sets identical across all 6 locales) — **PASS**
|
||||
- UTF-8 / no mojibake (`ü`, `ó`, `â€`, smart-quote bleed, etc.) — **PASS** in all 6
|
||||
- ICU plural / select preservation — N/A (DE source carries 0 ICU `plural`/`select` blocks; all plurals are handled in TS via `intl.formatNumber` + helper components)
|
||||
- Marc-Voice register: no `Claude / Anthropic / LLM / hallucinate / marathon / phase / wave / AI-agent` in any user-facing string in any locale — **PASS**
|
||||
- FR `tension artérielle` (19/19) and IT `pressione arteriosa` (18/18) — fully consistent
|
||||
- IT `Puls` leftover scan — **PASS** (counter false-positive earlier; no leftover German Puls in IT)
|
||||
|
||||
---
|
||||
|
||||
## Severity counts
|
||||
|
||||
- Critical: 0
|
||||
- High: 2
|
||||
- Medium: 4
|
||||
- Low: 5
|
||||
|
||||
---
|
||||
|
||||
## Recommended fix order
|
||||
|
||||
1. **H-1 + H-2** (placeholder restores, 12 strings total) — single i18n edit pass, ship in next patch.
|
||||
2. **M-1 + M-2** (ES/PL BP-term unification) — quick global replace.
|
||||
3. **M-4** (document T3 gap in v1.4.38-backlog).
|
||||
4. **L-1 + L-2** Playwright 320 px snapshot pass in IT + FR before release.
|
||||
5. Defer **M-3, L-3, L-4, L-5** to v1.4.39 or later.
|
||||
@@ -0,0 +1,200 @@
|
||||
# v1.4.39 backlog
|
||||
|
||||
Seeded 2026-05-17 during the v1.4.38 W-RX reconcile-apply. Items the
|
||||
v1.4.38 reviewers surfaced but reconcile deliberately deferred —
|
||||
either out of scope for the punch-list close, or unlocking value
|
||||
better paired with the v1.4.39 wave. Reviewer-doc references in
|
||||
parentheses so the next session can re-read the source rationale.
|
||||
|
||||
---
|
||||
|
||||
## High — wave-fit polish
|
||||
|
||||
### Named cached / enriched envelope split in analytics route
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` H-2)
|
||||
- `src/app/api/analytics/route.ts:86-152` — extract a named
|
||||
`AnalyticsCachedBody` / `AnalyticsEnrichedBody` pair to pin the
|
||||
cached call site to the un-enriched shape and the API response to
|
||||
the enriched shape. Failing test: assert `caches.analytics` keys
|
||||
only hold `{ lastSeenAt }` (no `daysAgo`).
|
||||
- Rationale: today the contract drift between cached and emitted body
|
||||
is policed by convention. A well-intentioned future "fix" that adds
|
||||
`daysAgo` into `buildAnalyticsResponse` lands it back into the cache
|
||||
and silently re-introduces the v1.4.38 W-B bug.
|
||||
|
||||
### `ensureUserRollupsFresh` same-microtask rejection retry safety
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` H-1)
|
||||
- `src/lib/measurements/rollups.ts:608-633` — clear the in-flight
|
||||
dedup slot eagerly in `.then` / `.catch` instead of `.finally`, so
|
||||
a same-microtask retry on rejection starts a fresh slot rather than
|
||||
piggybacking on the already-rejected promise. Add a unit test that
|
||||
pins the racy case.
|
||||
|
||||
---
|
||||
|
||||
## Medium — correctness polish
|
||||
|
||||
### RX-3 — Coach client-side copy branch
|
||||
- `src/components/coach/message-thread.tsx:78` (or its current
|
||||
equivalent) — branch on the Coach availability flag and surface a
|
||||
user-friendly disabled-state message instead of swallowing the
|
||||
click. Introduces `coach.assistantDisabledMessage` locale key
|
||||
across de/en/es/fr/it/pl.
|
||||
- Background: the W-RX investigation flagged this as part of the
|
||||
cascade closure but it sits in client UI; the API gates are already
|
||||
fixed (W-C) so the route returns clean 403; only the local copy
|
||||
is missing.
|
||||
|
||||
### RX-7 — repo hygiene sweep
|
||||
- `src/components/medication/medication-card.tsx` — three unused
|
||||
imports left after the W-D refactor pass; drop them.
|
||||
- `src/lib/insights/comprehensive-aggregator.ts` — two orphan helpers
|
||||
(`legacy*` shapes) survive the W-F migration but are unreferenced;
|
||||
drop them after a final grep.
|
||||
- `messages/*.json` — orphan-key scan across de/en/es/fr/it/pl. Find
|
||||
keys present in every locale but unreferenced in `src/`; emit a
|
||||
diff for manual confirmation before deletion.
|
||||
|
||||
### RX-8 — cumulative sparkline uses mean instead of sum
|
||||
- `src/app/api/dashboard/summary/route.ts` (sparkline sub-query) —
|
||||
for cumulative metrics (`ACTIVITY_STEPS`, `ACTIVITY_FLIGHTS`,
|
||||
`ACTIVITY_DISTANCE`, water/intake totals) the W-F DAY-bucket read
|
||||
uses `r.mean`. The semantically correct read for cumulative metrics
|
||||
is `r.sum`. Mean is correct for the average-style metrics already
|
||||
on the card (BP, weight, HRV, etc.). Switch per-type at the
|
||||
partition step using the registry's `aggregation` field.
|
||||
|
||||
### QA-4 M-1 — Coach gate inventory walker file-ext closed set
|
||||
(`.planning/phase-W-QA4-v1438-architecture-findings.md` MEDIUM-1)
|
||||
- `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts:112`
|
||||
— walker hard-codes `entry !== "route.ts"`. Widen to
|
||||
`entry.startsWith("route.")` with an explicit
|
||||
`["route.ts", "route.tsx", "route.js", "route.jsx", "route.mjs", "route.mts"]`
|
||||
closed set so a future `route.tsx` insights handler doesn't escape
|
||||
the gate-presence audit.
|
||||
|
||||
### QA-4 M-3 — Dashboard sparkline live-fallback for empty rollup tail
|
||||
(`.planning/phase-W-QA4-v1438-architecture-findings.md` MEDIUM-3)
|
||||
- `src/app/api/dashboard/summary/route.ts:323-335` — if the DAY-bucket
|
||||
read returns `[]` for a brand-new account before the boot backfill
|
||||
catches up, fall through to a one-shot
|
||||
`date_trunc('day', measured_at) GROUP BY` against `measurements`.
|
||||
Degraded-not-broken today (the boot backfill closes within minutes);
|
||||
proper fix removes the brief flat-sparkline first-impression.
|
||||
|
||||
### QA-6 M-1 + M-2 — BP / pulse term unification (es, pl)
|
||||
(`.planning/phase-W-QA6-v1438-i18n-parity-findings.md` M-1, M-2, M-3)
|
||||
- es: 16 keys use `tensión arterial`, 11 use `presión arterial`.
|
||||
Unify on `presión arterial` (WHO Spanish guideline, dominant
|
||||
variant in the file).
|
||||
- pl: 6 keys use `ciśnienie tętnicze` (clinical), 9 use
|
||||
`ciśnienie krwi` (lay). Unify on `ciśnienie tętnicze` in clinical
|
||||
contexts (doctor-report, classification headlines); keep
|
||||
`ciśnienie krwi` only if intentional in casual dashboard copy.
|
||||
- pl: 11 remaining `puls` substrings — most are fallback strings,
|
||||
worth a sweep to unify on `tętno`.
|
||||
|
||||
### Multi-process dedup follow-up
|
||||
(`.planning/phase-W-QA4-v1438-architecture-findings.md` MEDIUM-2)
|
||||
- `src/lib/measurements/rollups.ts:582-624` — per-process `Map` is
|
||||
sufficient at single-container scale. Capture this in the v1.5
|
||||
capacity plan: if Next horizontally scales, each container
|
||||
duplicates the cold fan-out (N parallel recomputes, N = container
|
||||
count). No code change today.
|
||||
|
||||
---
|
||||
|
||||
## Low — observability polish
|
||||
|
||||
### `isNearUtc` silent fallback observability
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-1)
|
||||
- `src/lib/tz/format.ts:96-100` — one-shot `console.warn` (mirror
|
||||
`warnTrustViolationOnce`) when an invalid tz is silently demoted to
|
||||
Berlin. Semantics unchanged, operator gains "are users storing
|
||||
junk" visibility.
|
||||
|
||||
### `bp-in-target-fast-path` cross-tz guard default
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-2)
|
||||
- `src/lib/analytics/bp-in-target-fast-path.ts:147-163` — flip the
|
||||
omitted-`userTz` default from "near-UTC" to "force live", or make
|
||||
`userTz` required and audit-fix call sites in one commit.
|
||||
|
||||
### Comprehensive aggregator ORDER BY stable tiebreaker
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-3)
|
||||
- `src/lib/insights/comprehensive-aggregator.ts:301-327` — extend the
|
||||
consolidated BP read's `orderBy` to `[{ measuredAt: 'asc' }, { id:
|
||||
'asc' }]` for postgres-stable ordering, then pin partition order
|
||||
with a unit test.
|
||||
|
||||
### Drain per-user log per-user-scoped struct
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-4)
|
||||
- `src/lib/measurements/drain-per-sample-cumulative.ts:316-318,
|
||||
434-442` — replace snapshot-delta with a per-user struct
|
||||
accumulated inline, then summed into totals. Same numbers, no
|
||||
snapshot fragility.
|
||||
|
||||
### bp-in-target-fast-path immutable `readSince`
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-5)
|
||||
- `src/lib/analytics/bp-in-target-fast-path.ts:189-197` — prefer
|
||||
the `Date.UTC(...)` constructor over `d.setUTCFullYear`/
|
||||
`setUTCDate` mutators. No mutation surface, future-maintainer
|
||||
trap removed.
|
||||
|
||||
### Coach gate inventory AST walker
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` M-6)
|
||||
- `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts:79-87`
|
||||
— switch the line-mode comment skipper to a TypeScript AST walker
|
||||
(`ts.createSourceFile` + `CallExpression` matcher). The Vitest
|
||||
harness already loads the TS compiler.
|
||||
|
||||
### `health-score-fast-path` `bpInTargetPctPriorWeek` semantics JSDoc
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` L-1)
|
||||
- `src/lib/analytics/health-score-fast-path.ts:140-143` — JSDoc note
|
||||
on the field: `undefined` = legacy compat (pin to current value),
|
||||
`null` = caller knows there is no prior-data (feed through). Unit
|
||||
test for both modes.
|
||||
|
||||
### Geo-backfill in-process singleton hot-reload guard
|
||||
(`.planning/phase-W-QA1-v1438-code-review-findings.md` L-2)
|
||||
- `src/lib/jobs/reminder-worker.ts:1208-1238` — module-scope flag is
|
||||
unprotected on Next.js hot-reload. Wrap in `globalThis`-cached
|
||||
shape, mirror the prisma client pattern used in `src/lib/db.ts`.
|
||||
|
||||
### IT longest-string clip-risk snapshot
|
||||
(`.planning/phase-W-QA6-v1438-i18n-parity-findings.md` L-1)
|
||||
- 320 px Playwright snapshot of `/insights` and `/measurements` in
|
||||
IT before next release. If clipping shows, shorten
|
||||
`insights.hrv.chartTitle` → `Variabilità FC` and consider
|
||||
abbreviation for `measurements.subtitle`.
|
||||
|
||||
### FR pending-label clip-risk
|
||||
(`.planning/phase-W-QA6-v1438-i18n-parity-findings.md` L-2)
|
||||
- `fr.insights.trendAnnotation.pendingLabel` is 46 chars vs DE 30.
|
||||
Shorten to `Commentaire en cours…`.
|
||||
|
||||
### EN file 166 untranslated DE strings
|
||||
(`.planning/phase-W-QA6-v1438-i18n-parity-findings.md` L-5)
|
||||
- 6.8% of `en.json` is identical to `de.json`. Mostly proper nouns
|
||||
(medication brand names, GLP-1 drug names). Pass to confirm none
|
||||
are genuine EN gaps.
|
||||
|
||||
### W-QA-3 unresolved tail (deferred P3 polish)
|
||||
- Items P3-4 through P3-7 from the W-QA-3 UX/a11y report — micro
|
||||
polish where the perceived-quality lift is small relative to the
|
||||
rebuild risk. Land them in the next UX polish wave alongside any
|
||||
iOS-driven web-side adjustments.
|
||||
|
||||
---
|
||||
|
||||
## Strategic note (carry into v1.5)
|
||||
|
||||
- iOS Health-app integration is the v1.5 P1; the v1.4.38 backlog
|
||||
items above are all web-side 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 and battle-tested in the canonical
|
||||
Berlin tenant.
|
||||
- The W-QA-6 T3 admin/settings/achievements coverage gap (149
|
||||
achievement keys × 4 locales) is the largest remaining
|
||||
user-facing localization debt. Worth a focused wave alongside the
|
||||
iOS launch when achievement copy gets a final review.
|
||||
Reference in new issue
Block a user