docs(planning): v1.4.43 audit findings + HAR gitignore

Four audit reports from the v1.4.43 discovery round: analytics 9s root cause, mobile-UI WCAG sweep, QoL copy gaps, security PII + log-growth.

Also adds *.har to gitignore so production HAR captures (which carry auth headers and cookies) do not land in the repo by accident.
This commit is contained in:
Marc-André Bombeck committed 2026-05-21 22:05:05 +02:00
1 parent 600aa3697c
commit 7e0f4afeeb
5 files changed
+927

No files matched your search

+3
View File
@@ -11,6 +11,9 @@
# testing
/coverage
# HAR captures from production debugging (may contain auth headers/cookies)
*.har
# playwright e2e
/test-results
/playwright-report
@@ -0,0 +1,265 @@
# v1.4.43 /api/analytics 9s perf investigation
## Verdict
**URGENT** — both `/api/analytics` (default + `?slice=summaries`) wait
~9 030 ms server-side on every dashboard mount of the live v1.4.42
tree. The regression has been latent since v1.4.40 W-WMY-WIRE and was
masked through v1.4.41/42 because no other endpoint moved. It is the
single largest user-visible regression on the v1.4.42 deploy and must
land in v1.4.43.
## Root cause
`src/lib/analytics/summaries-slice.ts:731-750`
(`computeAvg30LastYearMap`) is an **unbounded `Promise.all` fan-out**
over the per-type list of measurement types with data. It was
introduced in v1.4.40 W-WMY-WIRE (commit `24568c80`,
`perf(summaries-slice): consume monthly rollup buckets for long-window
slice`). Both `computeFromRollups`
(`src/lib/analytics/summaries-slice.ts:421`) and
`computeFromLiveAggregate`
(`src/lib/analytics/summaries-slice.ts:584`) call it unconditionally
on every slim-slice request.
On Marc's tenant `typesWithData.length` is ~15 (every measurement type
the dashboard tile-strip surfaces). For each type the helper invokes
`readBestGranularityRollups(userId, type, 395)`
(`src/lib/rollups/measurement-read-wmy.ts:133-155`), which **sequentially**
walks `GRANULARITY_FLOORS` from coarsest to finest. With `windowDays=395`
the loop tries MONTH (floor 181) first, then WEEK (91), then DAY (0)
— up to **three sequential Prisma round-trips per type** when MONTH /
WEEK coverage is partial. The only coverage the boot-time inline
refresh (`ensureUserRollupsFresh`,
`src/lib/rollups/measurement-rollups.ts:632-642`) guarantees is DAY
(trailing 90 d); WEEK/MONTH/YEAR depend on the v1.4.35.1 background
`rollup-full-backfill` queue and per-write hooks having converged.
So on a hot dashboard mount with two concurrent `/api/analytics`
requests:
- **Slim** (`computeFromRollups`) holds 3 base slots (narrows /
latests / dayBuckets `$queryRaw`) and then **bursts 15 simultaneous
`findMany`s** against `measurement_rollups`. Each can serialise into
2-3 round-trips on coverage miss.
- **Thick** (`buildAnalyticsResponse`) probes coverage, fans out 15-way
per-type live walks behind `p-limit(4)` (4 slots), runs two parallel
`computeBpInTargetFastPath` calls (4 slots for the rollup pair-read),
`correlations-fast-path` (3 slots), `health-score-fast-path` (4
parallel slots), plus stand-alone `sleepStages` + `glucoseByContext`
findManys (2 slots).
- **Background**: each route fires `void
ensureUserRollupsFresh(userId)` which itself holds 2 slots for the
watermark probe and, when stale, a `recomputeUserRollups` of the
DAY window.
Peak concurrency easily exceeds the `pg.Pool` max of 20 (see
`src/lib/db.ts:35`). The W-POOL `p-limit(4)` cap on the per-type
live walk inside the thick route (`route.ts:55`,
`ANALYTICS_TYPE_FETCH_CONCURRENCY`) was added in v1.4.40 explicitly to
prevent this — but the same release added the W-WMY-WIRE fan-out in
the slim slice WITHOUT any equivalent cap. The two concurrent routes
now drown the pool that v1.4.40 raised from 10→20 specifically to
absorb this kind of burst.
Secondary contributors (audited but NOT the dominant cost):
- `computeFromRollups`'s three lead `$queryRaw`s are correctly
parallelised in one `Promise.all`. They are not the bottleneck.
- The thick route's `summarize()` per-type fan-out is correctly capped
at 4. Its 425-day cap (`route.ts:264`) is in place.
- `probeRollupCoverage` (`src/lib/rollups/measurement-coverage.ts:45`)
is one indexed query and lands sub-50 ms in practice.
- `glucoseByContext` and `sleepStages` are both bounded to 30 days and
narrow projections — fine.
- The cache (`caches.analytics`, 60s TTL) is keyed
`{userId}|summaries` vs `{userId}|default`. Both miss simultaneously
on cold mount; both are single-flighted **per key** but NOT across
keys, so the two slices fire concurrently. Correct behaviour, but
exposes the pool to the combined fan-out below.
## Recommended fix
Two-line minimum-delta change — cap the WMY fan-out with the same
`p-limit` discipline v1.4.40 W-POOL applied to the thick route.
### `src/lib/analytics/summaries-slice.ts`
Add `import pLimit from "p-limit";` at the top of the file (alongside
the existing imports — already a project dependency, used in
`src/app/api/analytics/route.ts:1`).
Replace `computeAvg30LastYearMap`
(`src/lib/analytics/summaries-slice.ts:731-750`) with:
```ts
/**
* v1.4.43 — cap concurrent per-type WMY reads at 4.
*
* Pre-fix this helper ran an unbounded Promise.all over every type
* the user has data for. On a 15-type tenant the burst held 15+
* Prisma slots simultaneously, and with the slim and thick analytics
* slices firing in parallel on dashboard mount the combined fan-out
* drowned the pg.Pool max=20 even after the v1.4.40 W-POOL raise.
*
* Capping at 4 keeps the same total work but holds at most 4 slots
* for the WMY branch of the slim slice at any moment, leaving
* headroom for the concurrent thick slice's own fan-outs.
*/
const WMY_FANOUT_CONCURRENCY = 4;
async function computeAvg30LastYearMap(
userId: string,
types: ReadonlyArray<string>,
): Promise<Map<string, number | null>> {
const out = new Map<string, number | null>();
if (types.length === 0) return out;
const limit = pLimit(WMY_FANOUT_CONCURRENCY);
const results = await Promise.all(
types.map((type) =>
limit(async () => {
const value = await computeAvg30LastYearForType(
userId,
type as MeasurementType,
);
return [type, value] as const;
}),
),
);
for (const [type, value] of results) {
out.set(type, value);
}
return out;
}
```
That alone collapses the slim-slice burst back to 4-at-a-time, which
matches the thick slice's existing W-POOL cap.
### Optional follow-up (defer to v1.4.44 if v1.4.43 needs minimum-delta)
If perf-verify confirms the cap fix is insufficient on its own, two
incremental escalations are available:
1. **Pin the WMY router to the granularity the boot backfill
actually mints.** Today `readBestGranularityRollups(395)` falls
through MONTH → WEEK → DAY on coverage miss. If the tenant has
converged DAY but not MONTH, the helper pays the cost of two
miss probes per type before landing on DAY. The cheap fix is to
issue a single MONTH-or-DAY read using the per-type coverage map
the route already probed (`route.ts:209` /
`summaries-slice.ts:221`) instead of a sequential floor walk.
See `.planning/round-v1438-perf-analysis.md` §2 for the original
"WMY tier sits as write amplification" framing.
2. **Hoist `computeAvg30LastYearMap` behind a per-(userId,date) cache.**
The 395-day window is anchored on `Date.now()` so the result is
stable across an entire calendar day. A 24h TTL in
`caches.analytics` keyed on a `${userId}|avg30-last-year-${ymd}`
shape would collapse the burst to zero on warm days. The current
`caches.analytics` envelope already carries the full slice; the
per-call sub-helper just doesn't see the cache because it lives
inside `computeFromRollups`.
Neither follow-up is required to close the 9 s regression; the
`p-limit(4)` cap is the single minimum-delta land.
## Expected delta
Empirical reference points from the v1.4.39 cold-mount trace
(`.planning/round-v1439-empirical-trace.md` § B1-B3) and the v1.4.40
W-POOL closure show:
- W-POOL `p-limit(4)` on the thick route's 15-way per-type walk took
the pool-wait from 6.5 s → ~1.5 s under a similar burst.
- The slim slice's pre-W-WMY-WIRE timing (v1.4.37) was 1.5-3 s cold,
<100 ms warm. Adding the 15-way WMY fan-out in v1.4.40 widened the
cold-mount window proportionally.
Projecting the same `p-limit(4)` shape onto the slim slice:
| state | before | after (est) |
|---|---|---|
| dashboard mount, both slices cold | 9.0 s + 9.0 s in parallel | ~2.0-3.0 s + ~2.0-3.0 s in parallel |
| dashboard mount, slim warm + thick cold | ~9.0 s thick | ~2.0-3.0 s thick |
| dashboard mount, both warm (60s TTL hit) | ~50 ms | ~50 ms |
| WMY tier converged + cap applied | n/a | <500 ms cold (best case) |
The cap restores the v1.4.37 target band (1.5-3 s) under combined
slim+thick concurrent load. Warm-cache behaviour is unchanged.
## Test plan
1. **Unit** — extend
`src/lib/analytics/__tests__/summaries-slice.test.ts` with a test
that mocks `computeAvg30LastYearForType` and asserts the helper
never holds more than 4 in-flight calls simultaneously. Pattern
mirrors the v1.4.40 `route.test.ts` test pinning
`ANALYTICS_TYPE_FETCH_CONCURRENCY=4`
(`src/app/api/analytics/__tests__/route.test.ts` —
`pin concurrency cap and pool ceiling`, commit `433874e3`).
2. **Integration** — add a smoke test that issues a concurrent
`?slice=summaries` + default request against a seeded
15-type tenant and asserts both resolve under 5 s. Place
alongside the existing `route.test.ts` integration suite.
3. **Wide-event probe** — in production, after deploy, confirm both
routes emit `meta.analytics.slim_summaries.path:"rollup"` AND a new
`wmy_fanout.concurrency: 4` annotate (add to the helper's annotate
call alongside `slim_summaries` block so ops can prove the cap
fired). Compare wall-clock to the HAR baseline.
4. **HAR re-capture** — Marc to re-record the live tree post-deploy
and replace `.planning/v1442-postdeploy-new1.har` with the
post-fix capture. Both `/api/analytics*` rows should land in the
2-3 s band.
## Related findings
- **WMY rollup tier dead-write-amplification risk**: the v1.4.38 perf
audit (`round-v1438-perf-analysis.md` § 2, P6) flagged that
WEEK/MONTH/YEAR rollups are written on every measurement but never
read. v1.4.39 W-WMY readers + v1.4.40 W-WMY-WIRE wiring closed
that — but the wiring landed without a concurrency cap, which is
exactly the regression this report documents. The pattern
recurrence (Read-swap should replace, not parallel-run — see the
memo `feedback_read_swap_replace_not_parallel.md`) is worth
surfacing for the v1.4.43 marathon retrospective.
- **Boot backfill MONTH/WEEK coverage**: `ensureUserRollupsFresh`
(`src/lib/rollups/measurement-rollups.ts:632`) inlines only the
DAY refresh. If the per-write hooks or boot backfill have not
drained the MONTH partition for a user, every WMY call pays a
miss-probe penalty before landing on DAY. The v1.4.35.1
`rollup-full-backfill` queue (`enqueueBootTimeRollupBackfill`,
`measurement-rollups.ts:688`) should be doing this on each worker
boot — operator verification via `pg-boss` queue depth on
`ROLLUP_FULL_BACKFILL_QUEUE` would confirm. Not on the v1.4.43
critical path; deferrable to a v1.4.44 ops doc.
- **Cache key splitting**: the analytics cache uses
`${userId}|summaries` and `${userId}|default` as separate keys
(`route.ts:96` and `route.ts:115`), so the two concurrent dashboard
fetches do NOT share a single-flight. Each must compute
independently. Acceptable today (different response shapes), but
if v1.5 unifies the analytics envelope a single-flight collapse
would halve the cold-mount cost outright.
- **`mood/analytics` at 200 ms (HAR row 3)** is healthy — it does
not share the WMY fan-out and serves directly from
`mood_entry_rollups` (`src/lib/rollups/mood-rollups.ts`). No
action.
- **`ensureUserRollupsFresh` void-fire**: the v1.4.37.1 hotfix
decoupling was correct and remains. The in-flight dedup map
(`measurement-rollups.ts:582-588`) handles the concurrent slim+thick
case correctly — both join the same promise. No action.
- **Charts intermittent empty-state ("Erfasse mindestens 3
Einträge…")**: traceable to the slim slice's 9 s tail letting the
chart's `count >= 3` predicate evaluate against an undefined
summary before the response lands. Capping the fan-out closes this
symptom as a side effect — the chart will see the populated
`summary` within the 2-3 s window TanStack Query's `useQuery`
loading skeleton already accommodates. No separate fix needed.
@@ -0,0 +1,148 @@
# v1.4.43 Mobile UI / responsive audit findings
## Verdict
APPROVE_WITH_FIXES
Mobile-UI hygiene is in good shape overall — v1.4.27/33/34 mobile passes have closed the major touch-target / iOS-zoom / safe-area gaps. The user-reported "Erfasse mindestens 3 Einträge" finding is a real semantic bug (not a slow-data artifact), and there are a few residual sub-44 px tap targets and motion-handling inconsistencies worth addressing before the tag. None block the release on their own.
---
## Critical (must fix before tag)
### C1 — Chart empty-state fires on the 7-day tab even when the user has lots of data
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/health-chart.tsx:1251` (gate) + `:672-770` (chartData derivation) + `:675` (slice)
- **What's wrong**: The empty state `t("charts.emptyStateTitle")` = "Erfasse mehr Messungen, um Trends zu sehen" + `t("charts.emptyStateDescription")` = "Erfasse mindestens 3 Einträge, um die Trendlinie freizuschalten." paints when `chartData.length >= 1 && chartData.length < 3`. `chartData` is the *daily-aggregated* slice over the active range tab. A user with 50 BP readings in the last 30 days but only 2 distinct days inside the 7-day tab still sees this message even though raw count is ≥ 50. The message is factually wrong in that case (they have many entries; what they don't have is points across distinct days). The same condition holds in `mood-chart.tsx:679` and `medication-compliance-chart.tsx:391`.
- **Recommended fix**: Gate the message on raw measurement count (or `summary.count`), not aggregated `chartData.length`. Two options:
1. When `chartData.length < 3 && rawCount >= 3`, render a different copy: "Mehr Messtage erforderlich" / "Erfasse an mehreren Tagen für eine Trendlinie" (the underlying constraint is *day diversity*, not entry count).
2. Render the chart line anyway with the 1-2 points and skip only the regression overlay — the empty-state was added in v1.4.16 B1a for genuinely brand-new accounts; an established user just doesn't need it.
- **Severity rationale**: Marc explicitly flagged this as confusing under "even when data exists". It misleads experienced users into thinking the app forgot their data.
---
## High (should fix before tag)
### H1 — Switch tap area is 18×32 px, well below WCAG 2.5.5
- **File**: `/Users/marc/Projects/HealthLog/src/components/ui/switch.tsx:20`
- **What's wrong**: `data-[size=default]:h-[1.15rem] data-[size=default]:w-8` = 18.4×32 px. 24 instances across the app (settings sections, sources-section, dashboard-layout-section, coach-settings-sheet, notification toggles). On a touch device a 18 px tall target is one finger-pad's height — the Radix-Switch root *is* the hit area; the label next to it is not part of the tappable region. v1.4.34 IW-G floored Inputs at 44 px but Switches were missed.
- **Recommended fix**: Wrap the Switch root in a 44×44 padded `<label>` or extend the Switch component itself with `before:absolute before:inset-[-12px] before:content-['']` to enlarge the hit zone without changing the visual. Alternatively expose a `tapSize="lg"` variant that pads via `relative` + `::after` pseudo-element.
- **Severity rationale**: WCAG 2.5.5 violation across the most-used setting toggles. Discovered now while everyone walks Settings on phones.
### H2 — Comparison-baseline buttons in chart overlay popover are 36 px
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/chart-overlay-controls.tsx:251`
- **What's wrong**: `className="h-9 px-1 text-[11px]"` = 36×~36 px buttons inside the per-chart cog popover (3-column grid: `none` / `lastMonth` / `lastYear`). On a mobile chart the popover already lives inside a flyout the user tapped a 44 px cog to open, so undersized targets compound a precision problem.
- **Recommended fix**: Bump to `min-h-11 sm:min-h-9 px-2 text-[11px]` so mobile gets 44 px and desktop keeps the dense 36 px row.
- **Severity rationale**: Affects every dashboard chart's comparison-toggle, which Marc surfaced for v1.4.21 + v1.4.22 specifically as a recurring mobile irritation.
### H3 — Mood-form "more options" dropdown trigger is 36 px on mobile
- **File**: `/Users/marc/Projects/HealthLog/src/components/mood/mood-form.tsx:143`
- **What's wrong**: `<Button size="icon" className="h-9 w-9">` = 36×36 px. Lives inside the bottom-sheet footer where the user has to thumb between Cancel / Save / kebab — same mobile sheet shipped to every quick-add. The medication-form analogue at `medication-form.tsx:781,994` uses `h-11 w-11` correctly; the mood-form lagged behind, and the same legacy size sits on `mood-list.tsx:616` (the *edit*-mode footer mounted on mobile).
- **Recommended fix**: Swap both to `className="h-11 w-11"` to match the medication-form pattern.
- **Severity rationale**: Bottom-sheet finger ergonomics — exactly the surface mobile users hit most frequently.
### H4 — Bottom-sheet close-X is 36 px (intentional but inconsistent with other tap-target hardening)
- **File**: `/Users/marc/Projects/HealthLog/src/components/ui/sheet.tsx:88` + `dialog.tsx:86`
- **What's wrong**: `min-h-9 min-w-9` = 36×36 px. The inline comment notes "intentional exception per dialog primitive's rationale: 44 px would crowd the sheet header out of proportion". Acceptable in isolation, but Marc reported this collision with the settings cog in v1.4.20 post-deploy ("settings cog vs Sheet close-X collision"), and the surrounding chrome (form submit / cancel buttons) is already 44+ px. The argument that 44 px would crowd the header doesn't hold when the header is `p-4 pr-12` — there's room.
- **Recommended fix**: Bump to `min-h-11 min-w-11`. The icon stays `size-4`; only the hit zone grows. If the visual crowding really is a problem on a 320 px viewport, gate on `min-h-9 sm:min-h-11`-style breakpoint so phones get the safer target.
- **Severity rationale**: 36 px is below WCAG 2.5.5; comment acknowledges the tension but lands on the wrong side for mobile users.
### H5 — Smooth-scroll calls ignore prefers-reduced-motion (3 sites)
- **Files**:
- `/Users/marc/Projects/HealthLog/src/components/settings/settings-shell.tsx:172`
- `/Users/marc/Projects/HealthLog/src/components/admin/admin-shell.tsx:186`
- `/Users/marc/Projects/HealthLog/src/components/insights/coach-panel/message-thread.tsx:197,216`
- **What's wrong**: `el.scrollIntoView({ behavior: "smooth" })` / `el.scrollTo({ behavior: "smooth" })` fires unconditionally. `WelcomeCarousel.tsx:126` already shows the right pattern: `behavior: reduceMotion ? "auto" : "smooth"` driven by a media-query hook.
- **Recommended fix**: Add a shared helper (`scrollBehaviorForUser()` in `src/lib/motion.ts` or similar) that reads `window.matchMedia('(prefers-reduced-motion: reduce)').matches` and returns `"auto"` when true. Wire all four sites to it.
- **Severity rationale**: Motion sensitivity is an accessibility class HealthLog already supports elsewhere (CSS keyframes are gated; Lottie/Framer is `motion-reduce`-aware) — JS-driven scrolling is the lone gap.
---
## Medium (recommended for tag)
### M1 — `Loader2 animate-spin` missing `motion-reduce:animate-none` (21 sites)
- **Files**: `health-chart.tsx:1249` (chart loading spinner), `settings/account-section.tsx:362,620,711`, `settings/about-section.tsx:214`, `settings/sources-section.tsx:595`, `settings/ai-section.tsx:642,685,706`, `medications/DrugLevelChart.tsx:235`, `admin/backups-section.tsx:365,421,432,462,521`, `admin/api-token-overview-section.tsx:169`, `admin/system-status-section.tsx:231,240`, `doctor-report/doctor-report-dialog.tsx:486,575`.
- **What's wrong**: ~16 % of `animate-spin` call sites lack the motion-reduce modifier (107 honor it, 21 don't). The chart's own loading spinner inside the dashboard's tile-strip area is the most visible offender (`health-chart.tsx:1249`).
- **Recommended fix**: Sweep all `animate-spin` occurrences to append `motion-reduce:animate-none`. A `<Spinner />` primitive that bakes the class in (similar to ChartSkeleton) would close the gap permanently.
- **Severity rationale**: Continuous rotation is the canonical "trigger" animation for motion-sensitive users; missing the modifier on a chart loading-state defeats the rest of the motion-reduce defence.
### M2 — `phase-config-dialog.tsx` Input + Button are 32 px tall with `text-sm`
- **File**: `/Users/marc/Projects/HealthLog/src/components/medications/phase-config-dialog.tsx:237,243`
- **What's wrong**: `<Input className="h-8 w-20 text-sm" />` and `<Button className="h-8 w-12 px-0 text-xs" />`. The Input's `text-sm` override defeats the default `text-base md:text-sm` iOS-zoom defence — 14 px input will zoom the viewport on iOS Safari focus. The 32 px height for both is well below 44 px.
- **Recommended fix**: Drop the `text-sm` from the Input (let the default win on mobile) and bump heights: `h-11 sm:h-9`.
- **Severity rationale**: Phase-config-dialog is medication-tracker P0 surface; iOS zoom-on-focus + tiny buttons compound.
### M3 — Insights dynamic skeletons don't match loaded content height exactly
- **File**: `/Users/marc/Projects/HealthLog/src/app/insights/page.tsx:43,55,67`
- **What's wrong**:
- `DailyBriefing` skeleton `h-48` (192 px) vs loaded content varies (~220-360 px depending on briefing content).
- `CorrelationRow` skeleton `h-32` (128 px); loaded grid is 1-col (mobile) or 2-col (md+) of cards each ~250-300 px including disclaimer.
- `TrendsRow` skeleton `h-64` (256 px); loaded row is a 3-up grid each ~300+ px with annotation.
- **Recommended fix**: Either reserve the larger height (`h-[24rem]` for DailyBriefing, `h-[20rem]` for the others) or use a content-mirror approach like `<ChartSkeleton>` does. The shift is small (skeleton is short, paint is taller — content pushes down, not up) but it does CLS the page on slow networks.
- **Severity rationale**: User-facing only on cold mounts with slow networks; ranks as Medium because the v1.4.42 W5 placeholder polish already nailed the chart-tile case.
### M4 — `correlation-card.tsx` scatter skeleton mismatches loaded aspect ratio on `sm+`
- **File**: `/Users/marc/Projects/HealthLog/src/components/insights/correlation-card.tsx:34` + `scatter-correlation-chart.tsx:100`
- **What's wrong**: Skeleton is fixed `h-[180px]`; the loaded chart is `aspect-square min-h-[180px] sm:aspect-[3/2] sm:h-auto`. On `sm+` the loaded chart becomes a 3:2 aspect rectangle — typically 240+ px tall on a 360 px-wide card — so the skeleton is ~60 px shorter than the painted chart. CLS on insights cold mount.
- **Recommended fix**: Mirror the chart's class on the skeleton: `aspect-square sm:aspect-[3/2] min-h-[180px] sm:h-auto`.
- **Severity rationale**: Single below-the-fold spec; corrects a known CLS hot spot.
### M5 — Dashboard "blocked-then-burst" tile-strip when slim-analytics also lags
- **File**: `/Users/marc/Projects/HealthLog/src/app/page.tsx:1360-1424` (tile-strip render gate)
- **What's wrong**: The grid wrapper only renders when `trendCards.length > 0`. Every tile is gated on `has*` flags read from `data?.summaries?.*?.count`. If both `slim` and `thick` `/api/analytics` are slow (the HAR shows 9 s; slim is normally <1 s but during cache eviction it can be slower too), the user sees the page header + 0 tiles + then 9 s later 7 tiles appear at once. The per-tile `<Suspense>` placeholder at `:1412` only paints if the tile body itself suspends (which the synchronous tile bodies never do today). v1.4.39.2 split the analytics query specifically to fix this; the fix works only when slim is fast.
- **Recommended fix**: Two options:
1. Render a "tile-strip skeleton" (N greyed-out cards keyed off the layout's visible-tile count) when `trendCards.length === 0 && analyticsSlimQuery.isLoading && layout.widgets.some(w => w.tileVisible)`. The user sees the strip silhouette during the slow window.
2. Treat the 9 s thick fetch as a perf bug to chase separately (v1.4.43 audit-data scope) and rely on slim being fast.
Either way: when only thick is slow, partial-paint the strip from slim and avoid waiting on thick.
- **Severity rationale**: Marc reported the "etwas nervig" feel in v1.4.39 already; the slim/thick split was supposed to close it, but a thick-slow scenario still re-creates the impression on cold mounts.
### M6 — Chart's `aria-busy` reuses `isLoading` flag but the empty-state branch silently swallows announcements
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/health-chart.tsx:1095,1247`
- **What's wrong**: `if (!isLoading && !data?.length) return null;` (line 1095) returns NO DOM when data is empty after load. The chart's containing widget shows nothing at all — no empty card, no skeleton, no message. Combined with the empty-state-on-1-2-points branch at line 1251, the chart's user-visible states are: skeleton (loading), 1-2 points → "Erfasse 3 Einträge", 3+ points → chart line, 0 points → nothing-at-all. The 0-points branch is unexpected ergonomically (other Recharts wrappers paint a "Keine Daten" message).
- **Recommended fix**: Replace `return null` with a "no data in this window" state that mirrors `<ChartEmptyState>` styling. Different copy than the < 3-points branch to avoid the C1 confusion.
- **Severity rationale**: User reads "the dashboard is broken" instead of "this range has no data for me".
---
## Low (defer to v1.4.44)
### L1 — Insights tab-strip group-popover items at 40 px instead of 44 px
- **File**: `/Users/marc/Projects/HealthLog/src/components/insights/insights-tab-strip.tsx:380`
- **What's wrong**: `className="flex min-h-10 items-center …"` = 40 px sub-page links inside the group popover. Outer tab pills meet the 44 px floor; inner popover items are 4 px short.
- **Fix**: Bump to `min-h-11`.
### L2 — `chart-overlay-controls.tsx` dropdown trigger uses `size="sm"` + `min-h-11 min-w-11 px-0` mix
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/chart-overlay-controls.tsx:101-102`
- **What's wrong**: `size="sm"` (32 px default) stretched to 44 px via `min-h-11 min-w-11 px-0`. Same v1.4.27 fix pattern Marc later called klobig in dashboard-layout-section. Visually correct but the height stretch is the wrong fix shape now; v1.4.33 maintainer-item-7 settled on `size="default" min-h-11 sm:min-h-9`.
- **Fix**: Adopt the responsive `min-h-11 sm:min-h-9` pattern consistently.
### L3 — `bugreport/page.tsx` and `medication-form` JSON import textarea inline-styled
- **Files**: `/Users/marc/Projects/HealthLog/src/app/bugreport/page.tsx:212`, `medications/page.tsx:475`, etc.
- **What's wrong**: Each textarea has its own copy-pasted className with the `text-base sm:text-sm` iOS-zoom defence. Maintained six places (audit grep shows three more inside form components). Drift risk.
- **Fix**: Introduce a `<Textarea>` primitive in `src/components/ui/textarea.tsx` (parallel to `<Input>`) that bakes in the defence + autocomplete defaults.
### L4 — `next/dynamic` loading skeletons miss `motion-reduce:animate-none` on insights page
- **File**: `/Users/marc/Projects/HealthLog/src/app/insights/page.tsx:43,55,67`
- **What's wrong**: `animate-pulse` without `motion-reduce:animate-none`. The chart skeleton primitives have it; the insights mother-page skeletons skipped it.
- **Fix**: Append `motion-reduce:animate-none` to all three.
### L5 — `tabIndex={0}` on `injection-site-picker.tsx:178`
- **File**: `/Users/marc/Projects/HealthLog/src/components/medications/injection-site-picker.tsx:178`
- **What's wrong**: Explicit `tabIndex={0}` on a `<div role="button">` is fine, but the element should be a `<button>` to inherit native semantics — easier than maintaining the keyboard handler manually.
- **Fix**: Use `<button type="button">` instead of `<div tabIndex={0} role="button">`.
### L6 — Recent-workouts tile placeholder + drug-level loading don't pre-reserve height
- **Files**: `RecentWorkoutsTile` (no reserved height during initial fetch), `medications/DrugLevelChart.tsx:235`
- **What's wrong**: Both render `<Loader2 spinner />` centered inside a `Card` without a reserved minimum height — content pops into place once data lands.
- **Fix**: Add `min-h-[X]` to the loading branch matching the loaded card's typical height.
---
## Strengths
- **Touch-target hygiene mostly green** — `Input.tsx` floors at 44 px on mobile (`h-11 sm:h-10`); `Button` icon variants documented and most call sites override correctly (settings drag-handles `size-11 sm:size-9`, medication-card `min-h-11 min-w-11`, coach-drawer header buttons `size-11`); bottom-nav primary + overflow pills correctly at 44 px each with safe-area-inset awareness.
- **iOS zoom defence** — `Input.tsx` defaults to `text-base md:text-sm`; coach-input textarea + bugreport textarea + side-effects textarea + admin-feedback textarea all explicitly pin `text-base` on mobile.
- **Safe area + viewport-fit** — `layout.tsx` sets `viewportFit: "cover"`; bottom-nav uses `pb-[env(safe-area-inset-bottom)]`; FAB sits at `bottom-[calc(env(safe-area-inset-bottom,0px)+5rem)]`.
- **prefers-reduced-motion** — globals.css guards `.animate-insight-in` + `.pulse-dot`; ChartSkeleton notes the `motion-reduce` modifier; the `WelcomeCarousel` pattern is the gold standard for JS-driven scroll.
- **Per-tile Suspense fallbacks** — v1.4.41 W-FRONTEND-FACTORY layout-stable `min-h-[6rem]` placeholders are correct; ChartSkeleton mirrors loaded `<HealthChart>` height to within a pixel (`h-[var(--chart-height,…)]`).
- **Responsive grid track** — dashboard tile strip's `repeat(auto-fit, minmax(min(100%,11rem),1fr))` scales cleanly from 280 px (Galaxy Fold, 1 col) up to 1920 px (8 cols) without breakpoints.
- **Bottom-nav 5+More pattern** — well-thought-out compromise (4 core + Insights pill + overflow sheet); WCAG 2.5.5 floor honoured per-pill.
- **iOS-specific footgun coverage** — `inputMode` derivation in `Input.tsx`, `enterKeyHint` on coach-input, `autoCapitalize="sentences"` on free-text textareas, status-bar styling for PWA standalone.
+152
View File
@@ -0,0 +1,152 @@
# v1.4.43 Quality-of-life audit findings
## Verdict
APPROVE_WITH_FIXES
Net assessment: the product has a strong empty-state + onboarding spine, layout-stable Suspense boundaries, and consistent shadcn primitives. The paper cuts that compound — Provider-jargon leaks into German UI, ungrammatical singular/plural strings, an unstyled English-only 404, the new v1.4.42 `persistent` integration state has no user surface, and a global-error chunk-load shows a sub-card error rather than the user-friendly "we'll reload" copy — are 1–4-line edits each. No regressions block v1.4.43, but six items would significantly cheapen the product's "feels rough" tax.
## Critical (must fix before tag)
None. No findings rise to release-blocker severity.
## High (should fix before tag)
### H1 — DayDrillDown error renders `measurements.saveError`, copy mismatch
- `src/components/measurements/measurement-list.tsx:1056-1060` — the per-day drill-down query's error branch surfaces `t("measurements.saveError")` ("Fehler beim Speichern" / "Save error") but the failing operation is a `GET`, not a save. Users see "Save error" when their step-history drill-down fails.
- Recommended: introduce `measurements.loadError` ("Fehler beim Laden" / "Could not load measurements") and route the catch-block through it. Affects EN + DE.
- Severity rationale: visible on every Apple-Health-heavy account that touches the cumulative-type chevron when the network falters. Cheap fix; copy bug, not architecture.
### H2 — "Provider" + "rate-limited" leak through German UI
- `messages/de.json:1136-1137`: `providerRateLimitTitle: "Provider rate-limited"`, `providerRateLimitBody: "Provider ist temporär überlastet; Reset in ~5 min."`. Both terms are English jargon in a German string.
- `messages/de.json:1062`: `errorProvider: "Der Coach konnte gerade keinen Auswertungs-Anbieter erreichen…"` — already migrated to "Anbieter"; the v1.4.25 rate-limit copy missed the same migration.
- `messages/de.json:1619`: settings AI section description `"Provider, Modell, API-Key."` — same.
- `messages/de.json:1757-1759`: `rawDataOnDescription`/`rawDataOffDescription`/`rawDataWarning` reference "Provider".
- Recommended: rename "Provider" → "Anbieter", "rate-limited" → "gedrosselt" or "temporär überlastet", "Reset" → "wieder verfügbar". Per the [Marc voice memo](feedback_marc_voice_english.md), no English jargon in German UI.
- Severity rationale: violates the Umlaute-required / German-UI-purity rule already pinned in user memory. Six-line copy edit.
### H3 — v1.4.42 `persistent` state has no user surface
- `src/lib/withings/response-classifier.ts:59` introduces a fourth `FailureKind`: `persistent` (for Withings rate-limit `601` / contract-mismatch `293/294`).
- `src/lib/integrations/status.ts:218-222` maps it to `state="error_transient"` in the DB — the user-facing column.
- `src/components/settings/integration-status-pill.tsx:117-136` collapses both `error_transient` and `error_reauth` to the same `"error"` pill label "Fehler — neu verbinden". A `persistent` failure (operator misconfig, contract mismatch) reads identically to a transient one.
- Recommended: either (a) plumb a third pill state `"warning"` for `persistent` so the user can see "Verbunden, aber Server liefert ungültige Daten" without telling them to reconnect — that's misleading because the access token still works, or (b) extend `IntegrationErrorMessage` to display `viewModel.failureKind` so a user can read "Withings antwortet mit Fehlercode 293 — bitte den Hosting-Operator kontaktieren". (a) is the v1.4.42-promised behaviour; the wave landed the classifier without the surface.
- Severity rationale: the whole point of the v1.4.42 classifier change was operator visibility; right now the operator gets the audit-log trail but the user still sees "reconnect" copy that won't fix the problem. A user clicking the reconnect link 10 times learns nothing.
### H4 — `not-found.tsx` is English-only
- `src/app/not-found.tsx:32-44`: hard-coded English strings ("Page not found", "The page you were looking for doesn't exist…", "Back to dashboard"). No `useTranslations()` call.
- Recommended: lift the four strings into `messages/{de,en,…}.json` under `errors.notFound.{title,body,backToDashboard}` and import `useTranslations` in the page. Page is now a client component anyway? — it's currently a server component, so the translation has to come via `getServerTranslations()` (the helper exists for the doctor-report PDF path; reuse it).
- Severity rationale: a logged-in German user landing on a typo'd URL sees only English. Self-evidently inconsistent.
### H5 — `global-error.tsx` is English-only
- `src/app/global-error.tsx:36-117`: by contract this fires when even the root layout fails (no i18n provider yet). The English-only design is intentional, but the copy "Something went wrong" / "A critical error occurred" reads cold to a German user already in a bad state.
- Recommended: bilingual lockup ("Etwas ist schiefgegangen / Something went wrong", "Ein kritischer Fehler ist aufgetreten / A critical error occurred"). 6 lines of static text, no runtime cost.
- Severity rationale: matches the same fix landed for the legacy 404 in v1.4.27 MB6. The boundary itself can't read context; the static fallback should be bilingual.
### H6 — `relativeMinutesAgo` / `relativeHoursAgo` / `relativeDaysAgo` have no singular form
- `messages/de.json:1140-1142` + `messages/en.json:1140-1142`: each "ago" key carries only the plural form. `formatRelativeTime` (`src/lib/i18n/relative-time.ts:20-24`) passes `count: 1` and the user reads "vor 1 Minuten" / "1 minutes ago" / "1 days ago".
- The same project did get this right for the stale-hint where the code threads `count === 1 ? "...One" : "...Other"` (`dashboard.staleHintWeeksOne` vs `staleHintWeeksOther`). The pattern needs to extend to the three remaining keys.
- The integration-status pill (`integrationPill.{minutes,hours,days}Ago`) lives separately and uses abbreviated units ("vor 1 min" / "vor 1 Std.") — abbreviated forms read OK at count=1, so the pill itself is fine.
- Recommended: split each into `…One` / `…Other`, update `formatRelativeTime` to branch on `count === 1`. Mirror across all six locales.
- Severity rationale: visible everywhere — hero strip "last update", daily briefing time-since-load, conversation history rail timestamps. Tiny grammar paper-cut, but pervasive.
## Medium (recommended for tag)
### M1 — Dashboard widget reorder via arrow buttons, not drag-and-drop
- `src/components/settings/dashboard-layout-section.tsx:320-343`: each widget row exposes ↑ / ↓ buttons to reorder. Modern UX expectation (Apple Health, Withings web) is drag-to-reorder.
- The current implementation is keyboard-accessible — which drag-and-drop libraries famously aren't without extra work — so the arrow-button approach has a legitimate a11y story.
- Recommended (defer-worthy but worth noting): if drag-to-reorder lands later, keep the arrow buttons as the accessible fallback. For v1.4.43 the only edit is a tooltip on each button explaining "Verschieben Sie diese Kachel nach oben/unten" instead of just the aria-label. The current aria-label "Nach oben" is short to the point of cryptic for first-time users.
- Severity rationale: not broken, just non-modern. Defer to v1.4.44 with intent.
### M2 — No user-facing AI Coach disable toggle
- `src/lib/feature-flags/index.ts:25,115`: `flags.coach` is server/admin-only via `getAssistantFlags()`. A user cannot disable the Coach for themselves; if the operator has enabled it globally, the FAB and drawer are everywhere.
- `messages/de.json:1539`: settings copy mentions a research-mode disable but no AI-coach disable.
- Recommended: add a per-user `disableCoach` toggle to `Settings → AI` with copy "Coach ausblenden". Defaults to off. The CoachFab + drawer mount guards on `flags.coach && !user.disableCoach`.
- Severity rationale: privacy-leaning users want this; it's also the answer to "I never use the Coach, why is it taking space on every screen?" The audit-scope brief explicitly listed this as a Settings-findability item.
### M3 — Account-section danger zone has no separate "Delete account" CTA
- `src/components/settings/advanced-section.tsx:240-321`: the danger zone "DELETE" button deletes all data (`/api/settings/data` DELETE) but leaves the account row alive (passkeys, profile, audit log). A user reading "Gefahrenzone" / "Danger Zone" expects "delete my account entirely" — what the button does is half that.
- Recommended: split into two destructive actions — "Alle Gesundheitsdaten löschen" (current behaviour) and "Konto vollständig löschen" (cascade-delete user). The copy currently says "deine Daten" without disambiguating; users have asked for the full-account-delete via the bug-report channel.
- Severity rationale: ambiguous destructive UX is a known compliance flag (GDPR Article 17 right to erasure). The current button is GDPR-compliant only if you read the copy carefully.
### M4 — Doctor-report unavailable sections vanish rather than show "no data"
- `src/components/doctor-report/doctor-report-dialog.tsx:305-307,549-563`: when a section has no data in the selected range, the toggle row is filtered out entirely. The user has no signal that "Compliance" was an option but is empty for this date range.
- Recommended: render the missing sections as a disabled toggle with strike-through copy + tooltip "Keine Daten in diesem Zeitraum". The user immediately understands why their report has 5 sections instead of 7 and what range to pick to get the 7th.
- Severity rationale: the doctor-report PDF is the flagship export. Silent omission is worse than explicit absence.
### M5 — No offline / network-error banner
- The PWA service worker (`public/sw.js:1-50`) caches the shell but there's no `navigator.onLine`-based "you're offline" banner. A user toggling airplane mode mid-form-fill sees blank tiles and no explanation.
- Recommended: a slim `<OfflineBanner>` mounted in `auth-shell.tsx` that listens for `online`/`offline` window events and renders "Keine Verbindung — Änderungen werden gespeichert, sobald du wieder online bist". The service worker can already serve the read paths; the banner closes the explanation gap.
- Severity rationale: progressive enhancement deferred; the PWA promises offline-friendly but the UX doesn't acknowledge offline. Low-cost to ship, high-comfort signal.
### M6 — Coach error copy "Provider rate-limited" not user-friendly
- `errorCodeToI18nKey` in `src/components/insights/coach-panel/message-thread.tsx:78-95` maps multiple distinct provider failures (`coach.provider.unavailable`, `coach.network`, `coach.stream`, `coach.provider.empty`) to one bucket (`insights.coach.errorProvider`). A user sees "Der Coach konnte gerade keinen Auswertungs-Anbieter erreichen" whether the network is dead, the provider is down, or their own connection is flaky.
- Recommended: split into `coach.network` → "Keine Internetverbindung — versuche es erneut, sobald du online bist" vs the others. Network errors are the user's problem and they need a different next action.
- Severity rationale: paper cut; a sophisticated user already infers from context, but a new user sees the same copy in three different failure modes.
### M7 — German integration pill uses "T." for "Tage" — confusing
- `messages/de.json:1896`: `daysAgo: "vor {count} T."`. The "T." abbreviation is non-standard and reads as a typo. Compare with the existing "Std." (Stunden) which is at least recognised shorthand.
- Recommended: either spell out "Tage" / "Tag" (with the singular fix from H6) or use the more conventional `d` (matches the rest of the app's `staleHint` `vor Xd` pattern).
- Severity rationale: minor copy inconsistency; nobody is confused for long but it reads cheap.
### M8 — Sub-locale users (fr/es/it/pl) get English number formats
- `src/lib/format.ts:21-32`: `activeLocale()` returns `'de' | 'en'` only. A French user reading the dashboard sees French strings but `formatDateTime` produces "12/24/2025, 2:30 PM" via the `en` fallback.
- The codebase has a parallel `useFormatters()` hook in `src/lib/i18n/context` that handles all locales correctly — `src/lib/format.ts` is the legacy path used by ~25 call sites (server-rendered SSR strings, audit-log helpers).
- Recommended: migrate `format.ts`'s `activeLocale` to read the full `Locale` union, with `Intl.DateTimeFormat` doing the heavy lifting. Fallback to `en` only when the cookie is unrecognised.
- Severity rationale: known fallback per the [Marathon docs](v1.4.38 multi-locale ship); v1.4.43 should close this since the non-maintained locales are growing.
## Low (defer to v1.4.44)
### L1 — `not-found.tsx` and `global-error.tsx` use static raw text instead of i18n keys
- Already covered as H4/H5 but parallel issue: even when localised, the messages are wordy ("The page you were looking for doesn't exist or has been moved.") Marc's voice memo says copy should be tight + professional; the current shape is consumer-app-marketing. Recommended: "Diese Seite existiert nicht." / "This page doesn't exist." period.
### L2 — Getting-started checklist measurement threshold = 1
- `src/lib/onboarding/checklist.ts` (referenced from `getting-started-checklist.tsx`) defines the measurement-completion threshold as `count >= 1`. The trend tile needs ≥5 measurements to draw, so a user clears the checklist's "first measurement" before they get visual reward on the dashboard. The "Eine Messung reicht, um deine erste Trend-Kachel zu zeichnen" copy in `messages/de.json:1510` is technically false — a single point can't draw a trend slope.
- Recommended: raise the bar to 3 or rewrite the description to "Ein Messpunkt reicht, um die Kachel zu aktivieren — der Trend zeichnet sich ab dem 5. Eintrag." Pick truth.
### L3 — `CACHE_VERSION = "v1.4.38.4"` literal stale in `public/sw.js:16`
- The literal hasn't bumped for the v1.4.39 → v1.4.42 chain. The `<VersionPoller>` self-heal still fires correctly because it reads `NEXT_PUBLIC_APP_VERSION` from the build env (line 38), but the SW cache key is now 4 releases stale, meaning the activate-step's old-cache eviction has been a no-op for four deploys.
- Recommended: replace with `const CACHE_VERSION = (self as any).__APP_VERSION__ || "v1.4.42";` and inject `__APP_VERSION__` via the SW build step. Documented self-heal pattern from v1.4.38.4 implies this was the intent.
### L4 — `/api/analytics` 9-second perf — user-visible UX impact
- Per the W-AUDIT-analytics-9s sibling finding: the thick `/api/analytics` query blocks the dashboard for ~9 s when the rollup-coverage probe misses. The dashboard does paint slim-slice tiles first (good!) but the BD-Zielbereich tile, glucose-by-context tile, and the correlations panel under `/insights` sit on a `ChartSkeleton` for 9 s with no progress signal.
- Recommended (UX-only, defer perf to W-AUDIT-analytics-9s): drop a "Auswertungen werden berechnet — das kann einen Moment dauern" caption inside the `ChartSkeleton` after 3 s. Today the skeleton just shimmers indefinitely. Sets user expectation.
### L5 — "Gefahrenzone" / "Danger Zone" branding is louder than the action warrants
- Current copy: red `AlertTriangle` + red title + red button. Twice as scary as the actual destructive action (which is already double-gated by a confirmation dialog). Compare with GitHub's danger-zone shaping, which uses neutral icon + red CTA only.
- Recommended: drop the AlertTriangle icon, render the title in neutral grey, keep the button red. Same protective gate, less visual hostility.
### L6 — Onboarding wizard "Welcome" carousel and tour both auto-launch
- `src/components/onboarding/WelcomeCarousel.tsx` + `src/components/onboarding/tour-launcher.tsx` both gate on different fields (`onboardingCompletedAt` for the carousel, `onboardingTourCompleted` for the tour). A user can finish the carousel and immediately get the tour. Both are valuable, but the chained flow takes ~90 seconds before the user gets to actually use the app.
- Recommended: gate the tour on `onboardingCompletedAt + 24h` so a new user gets the dashboard between the two onboarding flows. The tour becomes "the second-visit thing."
### L7 — `cancel`/`save` button order in `responsive-sheet` footer is not platform-conventional
- `src/components/measurements/measurement-list.tsx:940-965`: in the edit-measurement sheet, the visual order is `[…] Cancel Save`. On iOS that's correct (Save is the rightmost confirmation); on Android the convention is the inverse. Since iOS is the primary mobile target, the current ordering is correct. Worth pinning in a comment so a future refactor doesn't "fix" it.
### L8 — `formatDateTime` everywhere; `formatRelativeTime` only inside Insights/Coach
- The measurement list (line 572, 717) renders absolute "21.05.2026, 14:32" timestamps; the daily-briefing renders "vor 12 min". Two formats for "when". The list-view absolute style is appropriate for accurate referencing, but the heading "Erfasst am 21.05.2026, 14:32" sits awkwardly next to "vor 3 min" on the same screen.
- Recommended: introduce a `formatDateOrRelative(iso)` helper — within 24h → relative, older → absolute. Defer to v1.4.44; not broken, just inconsistent.
## Strengths
S1. **Empty-state primitive (`empty-state.tsx`) is excellent.** Variants (`card` / `plain`), sizes (`default` / `compact`), CTA-floor enforcement at 44 px on mobile, `aria-live="polite"`, dashed-border affordance. The pattern is used consistently across 25+ call sites.
S2. **Empty-state copy is filter-aware.** `measurement-list.tsx:447-480` distinguishes "no measurements yet" from "no measurements match this filter" with a separate CTA to clear the filter. Few apps do this; HealthLog does.
S3. **Doctor-report dialog availability probe.** `doctor-report-dialog.tsx:165-250` re-probes `/api/doctor-report/availability` on every range change, paints a quiet skeleton-row loading state first, then either the toggle list or a "no data in range" empty-state. Genuinely well-designed.
S4. **Chart empty-state matches chart card height** (`chart-empty-state.tsx:46-49`) so the dashboard layout doesn't reflow when a chart enters/leaves the empty branch. This is the layout-stability paper-cut other apps trip over.
S5. **TrendCard's headline-arrow + 7-day-delta + comparison-caption all share one `getTrendSentiment` helper** (`trend-card.tsx:67-78`). Three signals route through one source of truth — no risk of green-arrow-next-to-orange-value desyncs.
S6. **Getting-started checklist is opt-out, collapsed by default, progress-meter-visible even when collapsed** (`getting-started-checklist.tsx:114-128, 380-408`). Marc's brief in v1.4.15 phase-A3 fix #3 explicitly noted the previous always-expanded layout as the dominant complaint; the current shape addresses it.
S7. **Integration status pill collapses meaningfully** — one chip with state-conditional colour + relative timestamp, mobile-safe with `whitespace-nowrap` and abbreviated time units. Replaces the v1.4.18 four-fold redundant status surface cleanly.
S8. **`<ResponsiveSheet>` primitive** (`responsive-sheet.tsx:64-90`) flips between bottom-Sheet on mobile and centred Dialog on desktop via the existing `useIsMobile()` hook. Footer sticky-pinning solves the "Save button hidden under iOS keyboard" paper-cut directly.
S9. **Stale-hint bucketing** (`trend-card.tsx:338-365`) — days under a week stay silent, 8–30 → "vor Xd", 31–60 → "vor X Wochen", >60 → "vor X Monaten", with proper one/other plural pairs. The pattern this audit wishes `relativeMinutesAgo` would adopt.
S10. **Coach `errorCodeToI18nKey`** centralises the server-emitted error-code → user-copy mapping in one helper (`message-thread.tsx:78-95`), exported for unit-test pinning. Distinct keys for daily-budget vs provider-rate-limit so users know whether resetting is on their side or the provider's.
S11. **AppError chunk-load auto-reload** (`src/app/error.tsx:35-52`) — sessionStorage-gated so a recoverable stale-shell error self-heals with one hidden reload instead of showing the user a scary error page. Marc's v1.4.38.3 paper-cut closed cleanly.
S12. **Skip-to-main-content link + ARIA-live error regions in forms** (e.g. `measurement-list.tsx:899-908`). The form's error banner is bound to the inputs via `aria-describedby` so screen readers announce the validation failure paired with the field. Most apps half-implement this; HealthLog gets it right.
@@ -0,0 +1,359 @@
# v1.4.43 Security audit findings
Scope: sweep of every security surface for the v1.4.43 release. `develop`
is at `7daff01a` (post v1.4.42 merge). Reviewer: W-AUDIT-SECURITY
(read-only, static review).
## Verdict
APPROVE_WITH_FIXES
The crypto, OAuth, encryption-at-rest, webhook-auth, rate-limit and
admin-gating surfaces are solid. No Critical / data-leak / auth-bypass
issues. Two High findings (PII in `auth.login.failed` audit row, audit
ledger writes carrying un-capped strings) should land before tag — they
are small, isolated fixes that close PII / log-growth gaps the
v1.4.20 retroactive directive explicitly forbids. Several Mediums are
defence-in-depth strengtheners; the Lows are deferable to v1.4.44.
## Critical (must fix before tag — vuln / data leak / auth bypass)
None.
## High (should fix before tag — defence-in-depth gap, log leak, error swallow)
### H-1 `auth.login.failed` persists the raw typed identifier in `AuditLog.details`
- **File**: `src/app/api/auth/login/route.ts:60-65`.
- **Attack vector**: When the user-lookup branch of `/api/auth/login`
fails (no matching user OR no `passwordHash`), the handler writes
`auditLog("auth.login.failed", { ipAddress: ip, details: { identifier, reason: "user_not_found_or_no_password" } })`.
`identifier` is `parsed.data.email.trim()` — i.e. exactly what the
caller typed. This persists the raw email or username string into
`AuditLog.details` JSON forever (audit rows never expire by design).
Three concerns:
1. The v1.4.20 retroactive-cleanup directive forbids ALL user-facing
PII in any operator-readable artefact, and `/api/admin/audit-log`
surfaces this row to the admin. An admin scrolling the audit log
sees every wrong-email a user typo'd at the login screen.
2. A failed brute-force probe of a guessed email lands in the audit
ledger with the guessed string, so an attacker who later compromises
the audit table walks away with a list of valid + invalid email
candidates each user-not-found row was probed against.
3. The successful-login + invalid-password branches (lines 70-75) do
NOT carry the identifier — they hash to a `userId` instead. The
user-not-found branch is the only one that leaks because no
`userId` exists at that point. Replace with a one-way hash of the
identifier (the same `hashToken(identifier)` shape v1.4.41 L-2
proposed for `check-user`) so the audit row still says "this IP
tried email-X N times" without persisting the literal string.
- **Severity rationale**: H rather than Critical — the data is operator-
read-only, not internet-reachable, and the brute-force surface is
already throttled by `auth:login:<ip>` at `5/15min`. But the v1.4.20
directive is a hard rule, the fix is one line, and shipping a release
that breaks the rule retroactively requires another scrub pass.
- **Recommended fix**: drop the raw `identifier` from the details and
replace with `identifier_hash: hashToken(identifier)` (or simply
drop the field entirely — `reason: "user_not_found_or_no_password"`
is the operationally interesting bit, the literal string is not).
### H-2 v1.4.42 L-1 still open — `WithingsApiError.message` lands un-capped in `AuditLog.details`
- **File**: `src/lib/integrations/status.ts:250-260` (audit-log call site),
consumed by every `WithingsApiError` thrown from `src/lib/withings/client.ts:127-135, 170-178, 290-298, 374-382`.
- **Attack vector**: The classifier shipped in v1.4.42 builds
`WithingsApiError.message` as `` `Withings <verb> error: <status> - <json.error>` `` where `json.error` is whatever Withings put in the
response body. That message flows verbatim into the
`auditLog("integrations.sync.failed", { details: { …, message, … } })`
call. The encrypted `IntegrationStatus.lastError` column is sliced to
1024 chars via `safeEncryptError`, but the audit-row JSON is NOT —
Postgres TEXT can hold arbitrary length. A compromised or buggy
Withings response could pump a large `error` string into every
connected user's audit ledger.
- **Severity rationale**: H, not Critical — Withings is the trusted
upstream and the failure path runs at most 3 times per user before
the 3-strike alert ladder fires. But the directive from v1.4.42 was
explicit (L-1) that audit-row payload sizes must match the encrypted-
error column behaviour and the gap is still open in v1.4.43. Closing
it costs one `.slice(0, 1024)` in either the constructor or the call
site.
- **Recommended fix**: apply `slice(0, 1024)` to `message` inside the
`WithingsApiError` constructor (single owner) so every audit /
notification path inherits the cap.
## Medium (recommended for tag)
### M-1 `auth.check-user` enumeration probe still un-audited
- **File**: `src/app/api/auth/check-user/route.ts:95-113`.
- **Attack vector**: v1.4.41 L-2 noted that `/api/auth/check-user`
emits `annotate()` for the wide-event log but never calls
`auditLog(...)`. The v1.4.41 H-1 fix landed the per-IP rate-limit
but did NOT land the audit row. The route now correctly throttles a
burst to 30/15min, but a throttled burst leaves no `auditLog`
evidence; wide-events are sampled and retained shorter than audit
rows. An admin investigating "did anyone enumerate accounts last
week" has no row to grep.
- **Severity rationale**: M — the throttle closes the operationally
important attack surface; the absent audit row is defence-in-depth.
Adding it is one line; matches the depth pattern of every other
/api/auth/* route.
- **Recommended fix**: emit
`auditLog("auth.check-user", { ipAddress: ip, details: { branch, identifier_hash: hashToken(identifier) } })`
on every branch. Hash the identifier rather than storing it raw —
same reasoning as H-1.
### M-2 `dispatchNotification` formatting may carry an arbitrary upstream
string to Telegram (the slice trims to 280, but the field is still
read from user-influenced data)
- **File**: `src/lib/integrations/status.ts:505-547` (the
`formatAdminAlertPayload` path is the unchanged Telegram alert body).
- **Attack vector**: Same pipeline as H-2. The `message` argument
reaches `formatAdminAlertPayload`, which trims to 280 chars before
pushing to Telegram. Bounded, so not a payload-size DoS. The concern
is that an HTML / Markdown injection in `json.error` would land in
the Telegram body unescaped — the existing alert body uses no
parse_mode (verified by reading `sendTelegramMessage` callers from
this file), so Telegram renders plain text and the injection is
inert. Note for the next reader: if anyone flips the alert body to
`parseMode: "HTML"` (which the medication-reminder paths use) the
upstream string would suddenly become an HTML injection vector.
- **Severity rationale**: M — preventive, not exploitable today.
- **Recommended fix**: keep alerts on plain text (no `parseMode`).
Document the constraint with a comment on `formatAdminAlertPayload`
so a future contributor doesn't silently flip it.
### M-3 Coach SSE transcript replays prior assistant + user messages
verbatim from DB without re-running `detectRefusal`
- **File**: `src/app/api/insights/chat/route.ts:193-244`.
- **Attack vector**: `detectRefusal` runs only on the inbound `message`
per turn (line 164-177). Prior turns (`priorTurns`) are loaded
straight from `coach_messages.encryptedContent` and concatenated
into the `userPrompt` transcript at line 235-244. If an injection
pattern slipped past the refusal once (false-negative on the regex
bank), it is replayed verbatim on every subsequent turn of the same
conversation. Today the same regex bank gates every new turn so the
user cannot send a follow-up that adds new payload; but the
ALREADY-PERSISTED payload keeps re-entering the prompt every turn,
so the model sees it on every reply.
- **Severity rationale**: M — the refusal pass is the existing defence
layer and the regex bank is conservative. The risk is "false-negative
amplification" rather than a fresh bypass. The fix is to re-run
`detectRefusal` against every turn re-loaded from DB and short-
circuit the conversation when an injection re-surfaces (or to clamp
per-turn content to a length that the provider's system-prompt
precedence overrides).
- **Recommended fix**: defer — pin the pattern in v1.4.44 with a
dedicated `replay_injection` audit annotation so the failure case is
observable before adding the kill-switch.
### M-4 `getClientIp` falls back to a literal `"unknown"` string in
every rate-limit caller — a misconfigured trust chain collapses
every anonymous request into one shared bucket
- **File**: every `checkRateLimit(\`...:\${ip ?? "unknown"}\`, ...)` call
site (e.g. `src/app/api/auth/login/route.ts:25-26`,
`src/app/api/auth/register/route.ts:24-25`).
- **Attack vector**: The v1.4.37 trust-violation warning fires once
per process and degrades the rate-limit precision silently
afterwards (`api-response.ts:217-237` documents this exact concern).
Anonymous traffic shares the `"unknown"` bucket → one attacker can
exhaust the bucket for every legitimate caller. The
`getClientIpOrTrustWarning` helper already returns a `trustViolation`
flag (line 254-300) but no call site routes the request to a tighter
global bucket when the flag is set; every caller still falls back to
`?? "unknown"`.
- **Severity rationale**: M — requires misconfigured `TRUST_PROXY_HOPS`
to trigger, but is exploitable in that misconfigured-but-still-running
state. The warning fires once but the precision stays degraded for
the rest of the process lifetime.
- **Recommended fix**: extend `checkRateLimit` (or wrap every auth-
surface call site) so that `trustViolation === true` routes the
request to a single global bucket with a much tighter limit (e.g.
100/15min across the whole "unknown" pool). Defer to v1.4.44 if the
v1.4.43 release window is tight.
## Low (defer to v1.4.44)
### L-1 `withings_state` cookie carries `userId:nonce` plaintext
- **File**: `src/app/api/withings/connect/route.ts:25-37`.
- **Attack vector**: The Withings OAuth `state` parameter is built as
`${user.id}:${stateNonce}` and persisted both as the cookie value AND
the URL state. The cookie is `httpOnly`, `Secure` (prod) and
`sameSite: "lax"` so the XSS exfiltration surface is closed. But the
user id is recoverable from a request log entry (where the cookie is
not redacted) and from a network-traffic capture of the redirect.
Cosmetic — knowing your own user id leaks nothing — but a future
refactor that switches the cookie to a non-httpOnly variant would
silently expose it. The state is also compared via constant-time
string-equal (line 31), good.
- **Recommended fix** (defer): switch the state to a fully-random
16-byte nonce and persist `(nonce → userId)` in a short-lived (10
min) row; CSRF check then becomes "this nonce is in the table" rather
than "this nonce includes the right userId".
### L-2 The Coach `userPrompt` is built via raw string concatenation
- **File**: `src/app/api/insights/chat/route.ts:235-244`.
- **Attack vector**: None today — the snapshot is server-built JSON
(already sanitised at the source via `sanitizeForPrompt` for every
free-text field — verified across `glp1-snapshot.ts:318-332`,
`blood-pressure-status.ts:323-324`, `medication-compliance-status.ts:234-235`,
`glp1-plateau.ts:128-129`). User messages are the only free-text;
`detectRefusal` is the gate. Worth a note for the next reader so
the boundary stays explicit: any new free-text field added to the
snapshot MUST run through `sanitizeForPrompt` before landing in
`snapshot.snapshotJson`.
- **Recommended fix** (defer): add a lint-style test in
`src/lib/ai/coach/__tests__/snapshot.test.ts` that fails when a
free-text field is added to the snapshot blob without the
`sanitizeForPrompt` wrap. Cheap regression guard.
### L-3 `passkey/login-verify` uses raw `as AuthenticationResponseJSON` cast
- **File**: `src/lib/auth/passkey.ts:189`.
- **Attack vector**: The SimpleWebAuthn verifier owns shape validation
immediately downstream (line 202-213) so an arbitrary-shaped body
fails fast. But the cast bypasses TS's type-narrowing — a future
refactor that reads `typedResponse.id` or another field BEFORE
calling the verifier would crash on `undefined.id`. Note from
v1.4.41 L-1's twin direction: explicit Zod parsing in front of the
verifier closes this comprehensively.
- **Recommended fix** (defer): add a thin Zod schema upstream of
`verifyAuthentication` so the cast becomes an explicit narrowing
rather than an implicit one.
### L-4 `legacy_form_total` counter is in-memory only — useless in a
multi-container deploy
- **File**: `src/app/api/withings/webhook/route.ts:45-64`.
- **Attack vector**: None — the counter is observability, not a
security control. Note for the v1.4.27 removal cut: if the gate is
"legacy form usage trending toward zero", a single-container reading
of the counter undersells the actual traffic. Two containers each
serving 30 % of legacy traffic each report ~30 % of total volume.
- **Recommended fix** (defer): move to Postgres if the v1.4.27 cut
matters for the gate; otherwise drop the counter when the legacy
form is removed.
## Strengths
1. **Encryption-at-rest is comprehensive + versioned.**
- Withings access + refresh tokens (`WithingsConnection.{accessToken, refreshToken}`),
Codex access + refresh tokens (`User.codex{Access,Refresh}TokenEncrypted`),
Telegram bot token (`User.telegramBotToken`), Withings client secret
(`User.withingsClientSecretEncrypted`), Anthropic API key
(`User.aiAnthropicKeyEncrypted`), and Coach messages
(`CoachMessage.encryptedContent`) all flow through `src/lib/crypto.ts`'s
versioned AES-256-GCM envelope with a documented rotation CLI.
- The legacy bare-base64 format refuses to decrypt under the active
key without an explicit `v1` entry, preventing silent corruption
during rotation.
- APNs key, password hashes (argon2id), and API token hashes
(HMAC-SHA256) cover the remaining sensitive columns.
2. **Webhook authentication is uniform + timing-safe.**
- Withings webhook (path-segment + legacy query): `timingSafeStringEqual`
wrapper around `node:crypto.timingSafeEqual`, length-pre-check
guards leak-via-length-mismatch (`src/lib/withings/webhook-handler.ts:50-59`).
- Telegram webhook: same shape with the `x-telegram-bot-api-secret-token`
header (`src/app/api/telegram/webhook/route.ts:36-48`).
- Withings OAuth callback `state` cookie: `timingSafeEqual` on
length-matched buffers (`src/app/api/withings/callback/route.ts:27-31`).
- Per-IP rate-limit applied BEFORE the secret check on every webhook
so a brute-force probe of the secret is bounded.
3. **Per-IP rate-limit coverage on every public auth surface.**
- `/api/auth/login` 5/15min, `/api/auth/register` 5/15min,
`/api/auth/passkey/login-options` 10/15min,
`/api/auth/passkey/login-verify` 10/15min,
`/api/auth/refresh` 60/15min,
`/api/auth/check-user` 30/15min (v1.4.41 H-1 close),
`/api/auth/password` 5/15min (per-user, not per-IP),
Withings webhook 30/min, Telegram webhook 120/min.
- Routes that lack `checkRateLimit` were swept: the omissions are
authenticated routes where the session cookie / Bearer token IS
the rate gate (revoke + audit on every miss is sufficient — the
attacker would have to compromise a valid token first).
4. **Encryption-key versioning + rotation refuses to fail silently.**
- `decrypt()` on a legacy-format ciphertext without a `v1` key
entry throws an explicit "restore the original ENCRYPTION_KEY or
add a 'v1' entry" error (`src/lib/crypto.ts:230-241`) instead of
trying the active key (which would either fail with an opaque GCM
tag error or succeed silently with junk on a key collision).
- The `envSignature()` cache invalidator (line 40-46) ensures a
mid-process key reload reflects in the next decrypt; the test
helper `_resetCryptoCacheForTests` is the only opening.
- Production rejects short-padded dev keys at startup
(`decodeKey:62-67`).
5. **Cookie security posture is correct.**
- `healthlog_session`: `httpOnly`, `secure` (prod), `sameSite: "lax"`,
`path: "/"`, 30-day max-age with 1-day sliding refresh
(`src/lib/auth/session.ts:71-79, 130-141`).
- `hl_onboarding`: `httpOnly: false` (UX hint), `secure` (prod),
`sameSite: "strict"` (no cross-site flow depends on it), explicit
documentation that the real gate is server-side.
- `withings_state`: `httpOnly`, `secure` (prod), `sameSite: "lax"`,
`maxAge: 600` (10 min — matches the OAuth state lifetime).
- `codex_device`: `httpOnly`, `secure` (prod), `sameSite: "lax"`,
`maxAge: 900` (15 min — matches Hydra's device-code lifetime).
Payload is `encrypt(JSON.stringify({deviceAuthId, userCode}))` — even
a stolen cookie cannot be decrypted without the active key.
6. **CSP + HSTS + frame-ancestors are baked into `src/proxy.ts`.**
- The Next 16 file-convention rename `middleware.ts` → `proxy.ts`
is correctly applied. The `Content-Security-Policy`,
`Strict-Transport-Security`, `X-Frame-Options: DENY`,
`X-Content-Type-Options: nosniff`, `Referrer-Policy`,
`Permissions-Policy` headers all ship from `src/proxy.ts:226-275`.
- CSP `connect-src` is route-gated: `https://api.openai.com` and
`https://chatgpt.com` are only added on `/settings/ai*`;
`https://wbsapi.withings.net` only on `/settings/integrations/withings*`
and `/api/withings/*`. A DOM-XSS on an unrelated page cannot
exfiltrate to AI or Withings origins.
- HSTS in prod carries `preload` so the first-visit MITM window on
hostile networks is closed.
- Nonce-based `script-src` with per-request 128-bit `crypto.getRandomValues`
nonce.
7. **CORS is correctly absent.**
- No `Access-Control-Allow-Origin: *` anywhere in `src/` (grep
clean). iOS native uses the same-origin URL `https://healthlog.bombeck.io`
so no preflight is required. A future cross-origin call would
have to opt in explicitly.
8. **`check-env` CLI is operator-only + secret-clean.**
- v1.4.42 W6 verdict still holds: `renderResults` (`scripts/check-env.ts:188-220`)
emits only `[OK]` / `[MISSING-REQUIRED]` / `[missing-optional]`
plus the variable NAME — no value substitution. `parseEnvFile`
never echoes the value back to stdout.
- `MANIFEST_PATH = resolve(__dirname, "env-manifest.json")` is
module-frozen; `--file` controls only the env-source.
- Run as `pnpm check-env` — operator-only, no network-attacker
surface.
9. **Admin gating is uniform — every `/api/admin/**/route.ts` calls
`requireAdmin()`.** Grep `find … -L "requireAdmin"` returned empty
for the admin tree. Bearer-token authentication never elevates to
admin (`src/lib/api-handler.ts:412-431` — `requireAdmin` is
session-cookie-only by design).
10. **User-scope filtering is consistent on every owned-resource read.**
Every route under `/api/measurements`, `/api/mood-entries`,
`/api/medications`, `/api/workouts`, `/api/insights`, `/api/withings`,
`/api/dashboard` filters by `userId: user.id` from the auth context
in the WHERE clause. Forged IDs in JSON bodies are server-overridden
(`src/app/api/workouts/batch/route.ts:228`). Soft-delete (`deletedAt: null`)
is applied to every read tier consistently after the v1.4.40 W-DELETED
sweep.
11. **Dashboard-widgets 422 audit-ledger writes are user-scoped + best-effort.**
`prisma.auditLog.create({ data: { userId: user.id, action, details: JSON.stringify({ issues }) } })`
uses the sanitised projection from `sanitiseZodIssues`, never raw
`issue.params`. `.catch(() => {})` ensures a DB miss never 5xx's
the 422 contract. Verified in v1.4.42 W2 review.
12. **`dedupeWorkoutBatch` is user-scoped + tamper-proof.** The
group key includes `userId` so cross-user collapse is impossible;
the batch route hard-overrides `userId: user.id` from the auth
context (line 228) so a forged JSON `userId` is silently dropped.
The duplicate envelope to iOS carries only `index` and `status` —
no PII echo.
---
(56 words follow for the stdout summary)