diff --git a/.planning/round-v1443-QA-code-reviewer-findings.md b/.planning/round-v1443-QA-code-reviewer-findings.md new file mode 100644 index 000000000..28823a573 --- /dev/null +++ b/.planning/round-v1443-QA-code-reviewer-findings.md @@ -0,0 +1,401 @@ +# v1.4.43 QA-Code-Reviewer findings (W10-QA-CODE-REVIEWER) + +Scope: read-only cross-cutting review of `git diff 2c68a48d..develop` +covering 74 commits between v1.4.42 and the v1.4.43 release candidate. ++12 301 / -460 across 197 files. The individual wave closures were +already self-reviewed; this pass targets cross-wave drift, contract +preservation, migration safety, the two cherry-pick collision repair +commits, and Marc-voice/PII hygiene across commit subjects + bodies. + +Reviewer: W10-QA-CODE-REVIEWER (static review, no execution, no source +changes). + +## Verdict + +**APPROVE_WITH_FIXES** + +One Critical finding requires reconcile before tag — a third W4 i18n +collision was missed by the two existing repair commits (`32ca196e` +restored chart keys, `67434987` restored parked/resume keys, but the W4 +`relativeMinutesAgoOne` / `*Other` / `relativeHoursAgo*` / +`relativeDaysAgo*` plural keys are still absent from the messages +bundle after `2390438e i18n: tighten 404 + global-error copy across +all locales` reverted them). The consumer +(`src/lib/i18n/relative-time.ts`) reads the missing keys, the i18n +fallback at `src/lib/i18n/context.tsx:105-107` returns the key string +itself on miss, so the Insights hero strip, Daily Briefing, and +Coach history rail will paint literal strings like +`"insights.relativeMinutesAgoOne"` across all six locales. + +Everything else clears: migration is additive + idempotent, the W6 +Zod rollout preserves status codes and the `error` field, the +`checkAuthSurfaceRateLimit` wrapper composes correctly across five +auth surfaces, the W14 `parked` state machine has no obvious livelock, +and no `Co-Authored-By: Claude` / Marc-by-name lines appear in any +commit subject or body across the 74 commits. + +## Critical (must fix before tag) + +### C-1 — formatRelativeTime renders i18n keys as literal strings across Insights surfaces + +**Severity**: Critical — user-visible regression on three primary AI +features in six locales. + +**Files**: +- `src/lib/i18n/relative-time.ts:28-29, 37-38, 45-46` +- `messages/de.json` / `en.json` / `es.json` / `fr.json` / + `it.json` / `pl.json` — missing six keys each +- Consumers: `src/components/insights/hero-strip.tsx:135`, + `src/components/insights/daily-briefing.tsx:323`, + `src/components/insights/coach-panel/history-rail.tsx:146` + +**What happened**: +- `b8ca1c74 i18n(relative-time): branch on count === 1 to render + singular forms` correctly added `relativeMinutesAgoOne` / + `relativeMinutesAgoOther` / `relativeHoursAgoOne` / + `relativeHoursAgoOther` / `relativeDaysAgoOne` / + `relativeDaysAgoOther` keys across all six locale bundles AND + rewrote `formatRelativeTime` to branch on `count === 1` between the + two variants. +- `2390438e i18n: tighten 404 + global-error copy across all locales` + (landed on the same root branch) reverted the six locales back to a + pre-W4 baseline that only has the legacy singular `relativeMinutesAgo` + / `relativeHoursAgo` / `relativeDaysAgo` keys. +- The two earlier collision-repair commits (`32ca196e` for chart keys, + `67434987` for parked/resume keys) did NOT pick up the W4 plural + keys. + +**Evidence**: +```bash +$ grep -rn "relativeMinutesAgoOne\|relativeMinutesAgoOther" \ + /Users/marc/Projects/HealthLog/messages \ + /Users/marc/Projects/HealthLog/src --include="*.json" \ + --include="*.ts" --include="*.tsx" +src/lib/i18n/relative-time.ts:28: ? "insights.relativeMinutesAgoOne" +src/lib/i18n/relative-time.ts:29: : "insights.relativeMinutesAgoOther", +``` + +i18n fallback chain at `src/lib/i18n/context.tsx:95-117` confirms a +missing key returns the literal key string. So `formatRelativeTime` +output becomes: +- Hero strip's `heroGenerated` interpolation: + `"Generated insights.relativeMinutesAgoOther minutes ago"` +- Daily briefing's `time` slot: same shape +- Coach history rail row: literal `insights.relativeMinutesAgoOne` + +**Why missed**: +- `src/lib/i18n/relative-time.ts` has no companion test + (`src/lib/i18n/__tests__/relative-time*.test.ts` does not exist). + `format-date-or-relative.test.ts` exercises a SEPARATE helper + (`formatDateOrRelative` in `src/lib/format.ts`) that uses the + legacy singular keys. +- The two earlier collision-repair commits targeted only the keys the + failing tests pointed at; the W4 helper has no test so the regression + is invisible to CI. + +**Recommended fix** (a third collision-repair commit, same shape as +`32ca196e` / `67434987`): + +``` +i18n: restore W4 relative-time plural keys swept by 2390438e + +i18n: tighten 404 + global-error copy across all locales (2390438e) +reverted the W4 plural keys to a pre-fix baseline. formatRelativeTime +in src/lib/i18n/relative-time.ts reads relativeMinutesAgoOne / Other / +relativeHoursAgoOne / Other / relativeDaysAgoOne / Other but the bundle +only carries the legacy singular form, so the Insights hero strip, +Daily Briefing, and Coach history rail painted literal "insights.relative*" +keys across six locales. Restoring the six keys per locale with their +translations. + +Add a regression test src/lib/i18n/__tests__/relative-time.test.ts that +exercises every branch (just-now / count=1 / count=2 for each bucket) +against the actual bundle so a future tighten can't repeat the sweep. +``` + +Per-locale strings to restore (from `b8ca1c74`): +- en: `"{count} minute ago"` / `"{count} minutes ago"`, etc. +- de: `"vor {count} Minute"` / `"vor {count} Minuten"`, etc. +- es / fr / it / pl: see `git show b8ca1c74 -- messages/` + +## High (should fix before tag) + +### H-1 — package.json version still reads 1.4.42 at HEAD + +**Severity**: High — release-time invariant; not a defect in the diff, +but a checklist item the orchestrator/tagger must honour or every +post-deploy version reader will report stale. + +**Files**: +- `package.json:2` — `"version": "1.4.42"` +- `scripts/generate-sw-version.mjs:27-28` reads `pkg.version` and bakes + it into `public/sw-version.js` as `self.__APP_VERSION__`. +- `src/components/version-poller.tsx:38` reads + `process.env.NEXT_PUBLIC_APP_VERSION` (build-arg path). + +**What this means**: +- The Docker workflow (`docker-publish.yml:198-205`) now correctly + injects `NEXT_PUBLIC_APP_VERSION=${{ github.ref_name }}` so + `/api/version` and the version-poller's `SHELL_VERSION` will both + read `v1.4.43` at runtime independently of `package.json`. +- BUT `scripts/generate-sw-version.mjs` reads ONLY from `package.json`, + so the Service Worker's `CACHE_VERSION` will key as `v1.4.42` (the + current `package.json` value) instead of `v1.4.43`. +- Practical impact is contained: the v1.4.38.4 self-healing flow + (version-poller wipes ALL caches regardless of name) still fires + correctly because it compares `SHELL_VERSION` against + `/api/version`, both of which read the env var. The SW's own + `activate`-step eviction becomes a no-op (same cache key before + and after), but the destructive wipe still runs. + +**Recommended fix**: +Bump `package.json` to `1.4.43` as part of the release commit (the +established pattern — see `324d4bb4 chore(release): v1.4.44 — REG-11` +on main for reference). The release tagger should verify all three +of these match before pushing the tag: +1. `package.json` `"version"` field +2. The pushed git tag (`v1.4.43`) +3. The Docker build-arg (auto-derived from `github.ref_name`) + +Alternatively, update `scripts/generate-sw-version.mjs` to prefer +`process.env.NEXT_PUBLIC_APP_VERSION` over `pkg.version` (matches the +shape `/api/version` adopted in `c5d6029b`). + +## Medium + +### M-1 — Object.defineProperty discriminator on chart data is fragile + +**Severity**: Medium — works today, will silently break if the array is +ever spread / cloned / `.map`'d. + +**File**: `src/components/charts/health-chart.tsx:693-700, 706-710` + +**Context**: W2-CHART-GATE stashes `rawCount` on the returned data array +as a non-enumerable property: +```ts +Object.defineProperty(allData, "rawCount", { + value: rawMeasurementCount, + enumerable: false, + configurable: false, + writable: false, +}); +``` +Then reads it via `(data as ChartDataPoint[] & { rawCount?: number }) +.rawCount` at line 708. + +This works today because the consumer reads `rawCount` directly from +the React Query result without spreading. A future refactor that does +`const cloned = [...data]` or `data.map(...)` loses the discriminator +silently and the empty-state copy reverts to the legacy phrasing. + +**Recommended fix**: return `{ rows: ChartDataPoint[], rawCount: number }` +from the query and unpack both at the consumer. The current shape is +a tagged-array hack that hides the second return value behind a +nominally-typed property. + +### M-2 — recordWithingsSyncFailure pattern silently widens the error envelope + +**Severity**: Medium — code-quality / consistency. No runtime defect. + +**Files**: +- `src/lib/withings/sync.ts` — new `recordWithingsSyncFailure(userId, err)` + helper +- `src/lib/withings/sync-activity.ts:295-309` — calls it +- `src/lib/withings/sync-sleep.ts:240-244` — calls it + +**Observation**: the helper accepts `err: unknown` and uses an +`err instanceof Error ? err.message : String(err)` fallback inside. +`String(err)` on a non-Error object yields `"[object Object]"`, which +then lands in the encrypted `lastError` column AND the audit-row +`message` field. The pre-v1.4.43 paths did the same coercion, so this +is not a regression — but it now lives in a shared helper that's a +candidate for the v1.5 moodlog reuse, and a misuse there would +silently corrupt the audit ledger. + +**Recommended fix**: tighten the helper signature to +`err: Error | WithingsApiError | string` so the caller has to deal +with the type at the call site rather than the helper papering over +it. Alternatively, log a wide-event warning when `err` is neither an +Error nor a string so we can spot the misuse in production. + +### M-3 — Validation-failed audit rows use prisma.auditLog.create directly, bypassing the IP-geolocation backfill + +**Severity**: Medium — operator-artefact-quality only. + +**Files**: 41 routes from the W6 rollout. Sample: +- `src/app/api/measurements/route.ts` — both GET and POST paths +- `src/app/api/mood-entries/route.ts` +- `src/app/api/devices/route.ts:79-97` + +**Observation**: every `.validation-failed` audit row goes through +`prisma.auditLog.create(...).catch(() => {})` directly instead of the +`auditLog()` helper from `src/lib/auth/audit.ts`. The helper does an +extra MaxMind geo-resolution + carrier backfill on the IP that the +direct path skips. + +The result is that an operator filtering on `action LIKE +'%.validation-failed'` sees rows with `ipAddress = null` AND no +geo/carrier metadata, while a same-route business-logic audit row +through `auditLog()` carries the full geo enrichment. + +The fire-and-forget shape is intentional (don't slow the 422 reply on +audit-write), but the helper version is also non-blocking and +preserves the enrichment side-channel. + +**Recommended fix** (low effort, do as a follow-up if not in scope): +swap the direct create for `auditLog(...).catch(() => {})` in every +W6 route. Diff-stat scope: one import + one wrapper per route, no +behavioural change. + +### M-4 — backfillBuckets path in recordSyncFailure double-counts the legacy integer + +**Severity**: Medium — corner case affects exactly one transition per +user: the first failure after the v1.4.43 deploy on a user who already +had `consecutiveFailures > 0` from before. + +**File**: `src/lib/integrations/status.ts:193-200, 375-395` + +**Trace**: +- A user sitting at `consecutiveFailures: 4` and + `consecutiveFailuresByKind: null` (pre-migration row) hits a 5th + failure of kind `reauth_required`. +- `startingBuckets` resolves to `null` (the column is null). +- `existing` is set, so the helper goes through the `backfillBuckets` + branch (line 378): `buckets = backfillBuckets(4, 'reauth_required')` + → `{ transient: 0, reauth_required: 4, persistent: 0 }`. +- The increment branch at lines 384-395 SKIPS the increment for the + back-fill path because the comment says the legacy `+1` carries it. +- Then line 451 does `consecutiveFailures: { increment: 1 }` so the + legacy integer goes 4 → 5, while the bucket stays at 4. +- The next failure of the same kind increments the bucket to 5 (via + the post-migration branch) and the integer to 6. + +**Net effect**: the per-kind bucket undercounts the legacy integer by +exactly one for the first failure after the migration. The 24h park +decision keys off the bucket, but the alert ladder reads +`Math.max(consecutiveFailures, ...buckets)` (line 487-492) so the +ladder still pages at the right moment via the legacy integer. + +The park decision uses `persistentStreakBefore === 0` (line 405) to +decide whether to stamp `persistentFailureStartedAt`. For a row whose +back-fill seeds `persistent: 4`, `persistentStreakBefore = 4` so the +stamp is NOT set on this transition — meaning the 24h park can never +fire for this row until either (a) a success resets the buckets to +zero, or (b) the persistent bucket goes back to zero via some other +path that doesn't exist yet. + +In practice the moodlog + Withings reauth flow clears the bucket on +the next reconnect, so the corner case only matters for a row that +NEVER reconnects and stays in `reauth_required` indefinitely. That's +not a park-eligible scenario anyway. + +**Recommended fix** (cleanup, no behaviour change today): +when back-filling on a `persistent` kind, also set +`persistentFailureStartedAt = now` so the 24h park clock starts at +the back-fill moment instead of waiting for a downstream zero-reset. +Add a test case to `status.test.ts` that pins the back-fill + +persistent transition. + +## Low + +### L-1 — auditLog() helper is missing on the W14 resume route + +**Severity**: Low — audit row is written via `auditLog()` helper inside +`resumeIntegrationFromPark`, so the route handler itself is fine. No +gap; flagging for completeness only — was checking whether the +resume route uses the same direct-create pattern as W6 routes (it +doesn't, which is the correct choice for a stateful audit event). + +### L-2 — Hard-coded 24h park threshold without env override + +**File**: `src/lib/integrations/status.ts:125` — `const +PARK_PERSISTENT_FAILURE_AFTER_MS = 24 * 60 * 60 * 1000;` + +The 3-strike alert threshold reads from +`INTEGRATION_FAILURE_ALERT_THRESHOLD` env. The 24h park threshold does +not. An operator who wants to dial the park window for a specific +upstream outage has to redeploy. + +Defer to v1.4.44 backlog. + +### L-3 — phase-W*-v1443-report.md files mention "Claude" in their bodies + +The phase reports themselves include lines like "No `Co-Authored-By: +Claude`" as part of the Marc-voice / commit-discipline checklist. These +are INTERNAL planning artefacts under `.planning/`, not user-facing. +The directive bars Claude attribution from user-facing artefacts +(commits, CHANGELOG, GH releases, in-app copy, docs site, landing). + +`.planning/` files are operator-internal and the mentions there +explicitly document the absence of Claude attribution. No fix needed; +flagging only because the audit checklist asks us to scan for +"`Co-Authored-By: Claude`, no Marc by name, English only". + +Defer to v1.4.44 backlog as cleanup if desired (the planning docs +could be reworded to "no AI-assistant attribution" instead of "no +Claude"). + +## Strengths + +- **Migration discipline (W14)**: `0075_v1443_integration_park` is two + additive nullable columns with `IF NOT EXISTS` guards. Down + migration is documented in the SQL header. The application code + reads the column via a Zod-shape-guard + (`isFailureBucketObject` at status.ts:152-164) and back-fills nulls + on the next write, so an unmigrated row never crashes a reader. +- **W6 contract preservation**: `returnAllZodIssues` keeps the `error` + field intact and adds `details.issues` additively. Status codes + preserved (every migrated route stays on 422 except + `/api/devices` which the audit reports already documented). The + `sanitiseZodIssues` helper drops `issue.params` so user-typed + rejection values never round-trip — the H-1 PII directive is + respected at the envelope shape. +- **W13 M-4 trust-violation tighter bucket**: `checkAuthSurfaceRateLimit` + composes correctly across five surfaces (login, register, check-user, + passkey-login-options, passkey-login-verify, refresh). The + per-surface bucket key is preserved for the happy path + (`auth:login:{ip}`); the tighter bucket only kicks in when + `getClientIpOrTrustWarning` flags a chain mismatch. The wrapper's + test (`src/lib/__tests__/rate-limit-auth-surface.test.ts`, 213 + lines) covers both branches end-to-end. +- **W1 analytics fan-out cap**: the test + (`summaries-slice.wmy-cap.test.ts:48-96`) actually observes the + in-flight count via a controllable promise rather than just + asserting on the call count. This is the same shape the v1.4.40 + W-POOL test used and is the only way to catch a regression that + re-floods the pool on completion (not just on dispatch). +- **W14 state-machine docs**: the `IntegrationState` type union + comment at `status.ts:71-89` enumerates every state + transition + succinctly. The `formatAdminAlertPayload` security-invariant + comment at `status.ts:829-848` is a model of what an "this MUST + NOT change without re-doing the threat model" comment should look + like — explicit about which fields are upstream-influenced + which + Telegram parseMode is required. +- **Cherry-pick collision repair commits**: `32ca196e` and + `67434987` are correctly diagnosed, scoped, and named. Each commit + body explicitly calls out the collision pattern + the swept keys, + so a future grep for "swept" surfaces both. (The C-1 finding above + is that a THIRD collision was missed by the same pattern; the + pattern itself is sound.) +- **Marc voice + PII**: clean across all 74 commits — no + `Co-Authored-By: Claude`, no `Marc` by name, no first-person, all + English, all conventional-commit form. Spot-check confirms no + personal data in any commit body (no BD numbers, no measurement + counts, no specific user emails / IPs). +- **No accidental files**: only the audit findings + phase reports + landed under `.planning/`. The `.gitignore` extension for `*.har` + closes the v1.4.42 post-deploy debugging artefact loophole. No HAR + file in the tracked tree (`git ls-files | grep '\.har$'` returns + empty); the `.planning/v1442-postdeploy-new1.har` lives untracked + in the worktree which matches the rule. + +## Out of scope (flagged for v1.4.44 backlog if desired) + +- **L-1 → L-3**: defer per the spec. +- **M-3** (W6 audit-row helper inconsistency) is borderline — if the + release cycle is already tight, the direct-create pattern is + functionally correct, just less observable. Bundle the fix with + any future audit-row enrichment work. +- **M-1** (chart rawCount discriminator) is a maintainability + finding; works today, will break on a refactor years from now. + Move to backlog. diff --git a/.planning/round-v1443-QA-design-findings.md b/.planning/round-v1443-QA-design-findings.md new file mode 100644 index 000000000..9cb346100 --- /dev/null +++ b/.planning/round-v1443-QA-design-findings.md @@ -0,0 +1,192 @@ +# W10-QA-DESIGN findings — v1.4.43 (read-only) + +**Diff base:** `2c68a48d..develop` (74 commits, 197 files, +12 301 / −460 LOC) +**Reviewer:** W10-QA-DESIGN +**Scope:** design / UX / a11y of v1.4.43 UI surface (W5, W11, W12, W4, W2-CHART-GATE, W14 pill, W7) + +--- + +## Verdict + +**RED — one Critical i18n regression blocks ship.** + +A single self-inflicted regression on the `relativeMinutes/Hours/DaysAgoOne/Other` plural keys silently leaks raw i18n placeholder strings ("insights.relativeMinutesAgoOther") into three live Insights surfaces (Daily Briefing, Hero strip, Coach history rail) in all 6 locales. Discovered via cross-reference of the `b8ca1c74` (H6 plural-split) ↔ `2390438e` (404/global-error tighten) ↔ `32ca196e` (chart-keys restore) sequence — the second commit deleted the `*One/*Other` keys as collateral damage on top of the chart keys, and the third only restored chart keys. + +Everything else in the W5 / W11 / W12 / W4 / W2 / W14 scope landed in good shape. Empty-state coverage, motion-reduce sweep, skeleton heights, 44 px tap targets, and i18n parity on the *new* keys all check out. Fix the plural-keys regression and ship. + +--- + +## Critical (1) + +### C1 — `insights.relativeMinutes/Hours/DaysAgo{One,Other}` keys missing in all 6 locales → raw placeholder strings rendered live + +**Severity:** Critical — user-facing visible regression in 3 production surfaces. +**Surfaces affected:** Daily Briefing "Generated {time}", Hero Strip "Generated {time}", Coach History Rail relative-time labels. +**Locales affected:** de, en, es, fr, it, pl (all six). + +**Trace:** +- Commit `b8ca1c74` (W4 H6, "i18n(relative-time): branch on count === 1 to render singular forms") rewrote `src/lib/i18n/relative-time.ts:25-48` to call `t("insights.relativeMinutesAgoOne")` / `Other` etc., and added the matching keys across every locale. +- Commit `2390438e` ("i18n: tighten 404 + global-error copy across all locales") **deleted the new `*AgoOne/*AgoOther` key pairs** as well as several chart keys, replacing them with the legacy singular forms (`relativeMinutesAgo`). +- Commit `32ca196e` ("fix(i18n): restore chart keys W11 + W2 added across all locales") restored only the chart keys (`needMoreDistinctDaysTitle/Description`, `noDataInRangeTitle/Description`). The relative-time plural keys were missed. + +**Verification:** +``` +$ grep -c "relativeMinutesAgoOne\|relativeMinutesAgoOther" messages/*.json +de:0 en:0 es:0 fr:0 it:0 pl:0 +``` + +`src/lib/i18n/context.tsx:104-107` falls back to **the raw key string** when no locale (including English) carries the key: + +```ts +// Fallback to key itself +if (value === undefined) { + return key; +} +``` + +So every German user reading the Insights Hero strip currently sees `"insights.relativeMinutesAgoOther"` instead of `"vor 5 Minuten"` (and the same on five other locales). No tests catch this — no `formatRelativeTime` callers are exercised in the test suite. + +**Repro:** +1. Open `/insights` after a fresh Daily Briefing generation (or wait ~5 min after one). +2. Look at "Generated {time}" — should read "Generated 5 minutes ago" / "Generated 5 Minuten her" depending on locale. +3. Instead the literal `"insights.relativeMinutesAgoOther"` appears. + +**Fix sketch (read-only, do NOT apply):** Either +1. Re-add the 6 plural-form pairs across every locale (`relativeMinutesAgoOne/Other`, `relativeHoursAgoOne/Other`, `relativeDaysAgoOne/Other`), OR +2. Revert `relative-time.ts` to read the legacy singular keys (loses the H6 grammatical fix). + +The former is the right call — H6's "vor 1 Minuten" bug is a real grammar issue. + +**Recommended regression guard:** A new `__tests__/relative-time.test.ts` that asserts each of the 6 keys resolves in each of the 6 locales, plus an integration assertion that `formatRelativeTime(…)` never returns a string that starts with "insights." prefix. + +--- + +## High (1) + +### H1 — `IntegrationStatusPill` warning state added but never produced by the live status mapper + +**Severity:** High — feature shipped without end-to-end wiring (dead code paths). +**Surface:** Settings → Integrations → Withings card. + +`src/components/settings/integration-status-pill.tsx:121-128` accepts `state: "warning"` and the W4 phase report (commit `d1428650`) added the amber `warningServerError` translation in all 6 locales. But `pillStateFor()` in `src/components/settings/integrations-section.tsx:127-142` only emits `connected`, `error`, `parked`, `disconnected` — the underlying `IntegrationState` union never produces `warning`, so the chip is unreachable from the live card. + +The W4 phase report explicitly defers wiring (file-allow-list scope), so this is a known partial. Recommend either: +- Wire `failureKind === "persistent"` → `warning` in pillStateFor() (matches the W4-H3 acceptance criteria), OR +- Document the warning state as "iOS app reserve" so the test surface doesn't drift. + +Either way the state should not stay dead-code on the web shell. + +--- + +## Medium (4) + +### M1 — Switch tap target falls 1.6 px short of WCAG 2.5.5 in vertical axis + +**Severity:** Medium — within compliance grey-zone but spec-strict failure. + +`src/components/ui/switch.tsx:26` uses `data-[size=default]:h-[1.15rem]` = 18.4 px track + `before:inset-[-12px]` pseudo-element padding = 12 × 2 = 24 px → **42.4 px total vertical hit area**. WCAG 2.5.5 requires ≥ 44 × 44 px. + +Horizontal axis is fine (32 px track + 24 px = 56 px). Recommend bumping `before:inset-[-12px]` to `before:inset-y-[-13px] before:inset-x-[-12px]` (or just `-13px`) to clear the 44 px floor. + +The component comment explicitly cites WCAG 2.5.5 as the rationale, so this looks like an oversight, not a deliberate design choice. + +### M2 — `animate-pulse` ad-hoc skeletons missing `motion-reduce:animate-none` (10 sites in 3 files) + +**Severity:** Medium — motion-reduce sweep was scoped to `animate-spin` only (W11 sweep + regression test at `src/components/__tests__/motion-reduce-spin-coverage.test.ts`). The shared `` primitive (`src/components/ui/skeleton.tsx:24`) handles `motion-reduce`, but three files still hand-roll `animate-pulse` divs: + +- `src/components/insights/daily-briefing.tsx:230-233, 241` (5 sites) +- `src/components/insights/trend-annotation.tsx:100-102` (3 sites) +- `src/components/doctor-report/doctor-report-dialog.tsx:574-575` (2 sites) + +Recommend either: +- Extend the `motion-reduce-spin-coverage.test.ts` guard to cover `animate-pulse` as well, OR +- Migrate the inline skeletons to `` so they inherit `motion-reduce`. + +The motion-reduce coverage spec already passes for `animate-spin` (W11-L1) so the only gap is `animate-pulse`. + +### M3 — Coach `errorNetwork` copy split is good, but does not branch on the **client's** `navigator.onLine` first + +**Severity:** Medium — UX gap, not a bug. + +`message-thread.tsx:82-90` maps server error code `coach.network` to the `errorNetwork` copy. Good. But the client never pre-checks `navigator.onLine` before issuing the fetch, so a user who toggled airplane mode mid-form-fill sees: +- The new `` at the top (good — separate W12-M5 fix). +- The Coach Send button still active. +- A generic 0/network error after the request times out. + +Recommend disabling Send when `navigator.onLine === false` so the user gets the actionable copy instantly instead of after a request timeout. This is additive to W12-M6 and could ride into v1.4.44 backlog. + +### M4 — Doctor-report disabled-not-hidden: strike-through styling reads as "deleted", not "unavailable" + +**Severity:** Medium — visual semantic mismatch. + +`doctor-report-dialog.tsx:638-642` applies `line-through` to the label of unavailable section toggles. Strike-through carries a strong "removed / deleted" connotation; for "no data in this period" the convention is the opacity dim + caption line (which the dialog already paints below). Recommend dropping the strike-through and relying on `opacity-60 cursor-not-allowed` + the existing italic hint line. + +The hint line copy is good ("Keine Daten in diesem Zeitraum") and covers all 6 locales. + +--- + +## Low (5) + +### L1 — `comparison-baseline` pill DE copy "Dein Mittel" reads as personal-baseline noun, not toggle action verb + +**Severity:** Low — DE polish. + +`charts.personalBaseline` in `messages/de.json:858` reads "Dein Mittel" (noun phrase). On the comparison-baseline pill row in `chart-overlay-controls.tsx:251-260` the buttons are toggle actions; the noun form reads slightly odd next to the active label. Not a blocker, just a stylistic choice that may be worth tightening in a later QoL pass. + +### L2 — Sheet close-X mobile target uses `min-h-11 min-w-11 sm:min-h-9 sm:min-w-9`, fine — but the icon stays `size-4` + +**Severity:** Low — visual centering. + +`src/components/ui/sheet.tsx:88` and `src/components/ui/dialog.tsx:86` bump the tap target from 9 → 11 on mobile, leaving the 16 px X icon. The result is a 44 × 44 px target with a tiny dot in the middle — functionally correct, visually thin. Recommend `[&_svg:not([class*='size-'])]:size-4 sm:[&_svg:not([class*='size-'])]:size-4` stays but mobile gets `size-5`, OR keep as-is and rely on the larger tap zone being self-explanatory. + +This is the iOS / Apple HIG convention so not strictly wrong. Calling out for awareness only. + +### L3 — Chart-skeleton `loadingSlowHint` caption fires after 3 s — good copy, but no `aria-live` + +**Severity:** Low — a11y minor. + +`chart-skeleton.tsx:125-132` paints the caption inside the parent `role="status" aria-live="polite"` skeleton container. The transition from skeleton-without-hint to skeleton-with-hint should announce; with `aria-live="polite"` on the outer it likely will, but the caption itself is wrapped in `

` rather than an explicit live region. Recommend adding `aria-live="polite"` directly on the `

` or ensure the test covers the announcement. + +### L4 — `OfflineBanner` paints below `` even though comment says "first chrome line" + +**Severity:** Low — comment / behaviour mismatch. + +`auth-shell.tsx:164-169` puts `` immediately above `` (correct per code order). But the inline comment claims it sits "above the maintainership banner + top bar so the connection-status hint is always the first chrome line a user sees in the offline branch." Verified — the order is OfflineBanner → MaintainershipBanner → TopBar, so the comment is accurate. (This was a false alarm during my review — code matches the claim. Listing here for note-keeping only.) + +### L5 — `formatDateOrRelative` in `src/lib/format.ts:96-121` reads `relativeMinutesAgo` (legacy key — exists) while `formatRelativeTime` reads `*One/*Other` (missing keys — broken) + +**Severity:** Low — code-quality / consistency. + +Two helpers, two different key sets. `formatDateOrRelative` (`src/lib/format.ts`) reads the original `relativeMinutesAgo` keys (which still exist) and works correctly. `formatRelativeTime` (`src/lib/i18n/relative-time.ts`) reads the new `*One/*Other` plural-aware keys (which are missing — see Critical C1). + +Even after the C1 fix, having two helpers with two different key contracts is fragile. Recommend consolidating both call-sites onto one helper in a v1.4.44 simplification pass. + +--- + +## Strengths + +- **i18n parity on most new keys is rock-solid.** `noDataInRangeTitle/Description`, `needMoreDistinctDaysTitle/Description`, `loadingSlowHint`, `errorNetwork`, `offlineBanner.message`, `notFound.title/backToDashboard`, `unavailableHint`, `resumeCta/Success/Error`, `warningServerError/parkedReconnect` — all 6 locales present, tight Marc-voice copy across the board. +- **Empty-state coverage closes the silent-null gap.** `health-chart.tsx:1141-1334` swaps the legacy `return null` on empty `chartData` for an explicit `` with distinct copy from the < 3 raw and < 3 distinct days variants. The three-way gate is the right call — every "no data" path now paints something meaningful. +- **W2-CHART-GATE rawCount logic is correct.** The `rawMeasurementCount` accumulator + non-enumerable `rawCount` property on the returned array is a clean way to thread the metric through without disturbing downstream array consumers. Mood + medication mirror the pattern correctly. +- **Motion-reduce sweep + regression guard.** `motion-reduce-spin-coverage.test.ts` is the right kind of guard — textual, fast, catches drift. The 21-site sweep landed cleanly. (Gap: animate-pulse — see M2.) +- **Skeleton heights match loaded content.** Insights mother-page skeleton heights bumped from `h-48/h-32/h-64` → `h-[24rem]/h-[20rem]/h-[20rem]` with a pinning regression test (`page-skeletons.test.ts`). Scatter-correlation skeleton mirrors `aspect-square` / `sm:aspect-[3/2]` responsive contract. Recent-workouts-tile and DrugLevelChart now reserve `min-h-[10rem]` / `min-h-[240px]` respectively. +- **Reduced-motion scroll helper is the right shape.** `src/lib/motion.ts:scrollBehaviorForUser()` consolidates the four `scrollIntoView` / `scrollTo` smooth-behavior sites onto one helper that respects `prefers-reduced-motion`. Settings shell, admin shell, message thread, and the new W5-H5 sites all migrated; only WelcomeCarousel still does its own check (justified — separate context). +- **Danger-zone visual quieting (`AlertTriangle` icon dropped, neutral title colour, red CTA only).** GitHub-style. Account-delete CTA card mirrors the data-reset shaping so the two surfaces read as siblings on the same page. +- **Doctor-report disabled-not-hidden is the right architecture.** The submission payload still force-clears unavailable toggles (server never renders an empty section), so the user-facing change is purely visual — no behavioural regression risk. +- **OfflineBanner architecture is clean.** SSR-safe (initial state `true`, paints nothing during SSR), microtask-deferred initial read of `navigator.onLine`, proper add/removeEventListener cleanup, `role="status" aria-live="polite"`, and i18n-aware copy. +- **W14 parked-integration UX.** Distinct visual treatment (orange `dracula-orange/15`) from warning (yellow) and error (red destructive), proper aria, manual reconnect CTA with 44 px tap target on mobile (`min-h-[44px] sm:min-h-0`), success / error feedback as separate `role="status"` / `role="alert"` paragraphs. +- **404 page server-resolves locale.** No more English-only `not-found` for German users; the implementation uses the existing `resolveServerLocale()` + `getServerTranslator()` infrastructure correctly. global-error is bilingual lockup because it cannot reach the i18n provider — also the right call. +- **Apple Health / Withings visual coherence preserved.** The Apple Health pink badge accent on measurement source badges (`measurement-list.tsx:144-149`) survives unchanged, Withings parked banner uses dracula-orange (different tone from connected green / warning yellow), and the integration-pill remains the single source of truth for sync status (no v1.4.18-era redundancy resurfaced). + +--- + +## Recommendation + +**Fix C1 (relative-time plural keys), then ship.** H1 (warning state wiring) is a documentation-only fix or a 6-line `pillStateFor` extension — either is fine. M-tier items are not blockers; L-tier items are pure polish for v1.4.44. + +**Suggested follow-up backlog for v1.4.44:** +- C1 fix + regression test (5 min, 1 commit). +- H1 warning-state wiring or doc-only deferral (10 min). +- M1 Switch vertical inset (1 line). +- M2 animate-pulse motion-reduce sweep + extend the guard (15 min). +- L5 consolidate `formatDateOrRelative` + `formatRelativeTime` onto one helper. diff --git a/.planning/round-v1443-QA-product-lead-findings.md b/.planning/round-v1443-QA-product-lead-findings.md new file mode 100644 index 000000000..e2cf956cd --- /dev/null +++ b/.planning/round-v1443-QA-product-lead-findings.md @@ -0,0 +1,429 @@ +# v1.4.43 QA — Product Lead release-readiness review + +Scope: strategic + release-readiness review of the full diff +`2c68a48d..develop` (74 commits, +12,301 / -460 LOC across 197 files). +Read-only; no source modifications. All four audit reports and all +seven phase reports (W4 / W6 / W7 / W8 / W11 / W12 / W13 / W14) read +end-to-end before this verdict. + +## Verdict + +**SHIP** — v1.4.43 is ready for production at +`healthlog.bombeck.io` once the version bump + CHANGELOG entry + +operator notes land. No Critical issues. One High (H3 partial-wire +ship) is a quality concern, not a release blocker. All Discovery +Highs and Criticals from the four audits closed; almost every Medium +picked up; Lows correctly deferred to v1.4.44. + +## Critical / High count + +- **Critical**: 0 +- **High**: 1 (H3 partial wire — see below) +- **Medium**: 3 +- **Low**: 4 + +--- + +## High (must address before tag — but not blocker if Operator notes +the gap) + +### PL-H1 — `IntegrationStatusPill` `"warning"` state ships dark + +- **What**: W4 added a fourth pill state `"warning"` with locale copy + `settings.integrationPill.warningServerError` ("Verbunden, aber + Serverfehler" / "Connected, server error") to close audit-H3 + (v1.4.42 `persistent` failure-kind has no user surface). The pill + component renders the state correctly when called. But no + production code path passes `state="warning"` to the pill. +- **Why**: The wave's file allow-list excluded + `integrations-section.tsx` (the caller). W4's phase report + documents the gap explicitly and lists the three steps to wire it + (Prisma `lastFailureKind` column, `/api/integrations/status` + payload extension, `pillStateFor` branch). The follow-up wave W14 + did touch `integrations-section.tsx` but only to wire `"parked"`; + the `"warning"` wire-up was missed. +- **Result**: The audit-H3 contract is half-shipped. A Withings + persistent failure (rate-limit 601 / contract-mismatch 293/294) + still surfaces as the same red "Fehler — neu verbinden" pill that + every transient or reauth failure paints — the same UX confusion + the v1.4.42 classifier was meant to close. +- **Severity rationale**: High because audit-H3 was tagged + ship-before-tag and the audit's reasoning ("a user clicking the + reconnect link 10 times learns nothing") still holds. Not a + blocker because (a) the `"parked"` state in W14 partially + compensates (a >24h persistent streak escalates to the parked pill + with copy "Pausiert — manuell wieder verbinden", which IS + user-distinct), and (b) the under-24h gap is a missing improvement + rather than a regression — the v1.4.42 behaviour ships unchanged. +- **Recommended path**: + - Option A (defer cleanly): drop the `"warning"` pill state from + `integration-status-pill.tsx` and remove the four locale keys + (`warningServerError` × 6). The pill should not ship dark code. + Move the full wiring (Prisma + API + section + pill) to a + standalone v1.4.44 wave per the W4 phase-report blueprint. + - Option B (ship as-is + document): leave the dark code in place + and document under "Known follow-up" in the v1.4.43 operator + notes so v1.4.44 has the breadcrumb. Cost: one additional + component state to maintain. + - Recommend Option A — dark UI code violates the v1.4.40 + audit-G "ghost-purge" hygiene rule and the W-SIMPLIFIER pattern + that's been the consistent direction since v1.4.41. + +--- + +## Medium (recommended, not blockers) + +### PL-M1 — Version bump + CHANGELOG entry not yet landed + +- `package.json` still reads `"version": "1.4.42"`. +- `CHANGELOG.md` has no `[1.4.43]` entry. +- The handoff (`v1443-handoff.md` step 6 / 7) explicitly scopes both + for the fresh session. +- The CHANGELOG draft needs to cite the 4 audit reports + 7 phase + reports + REG-11 (the Marc-authored dashboard summary fix that + doesn't sit under a wave). Shape mirrors `[1.4.42]` (Added / + Changed / Fixed / Performance / Operator notes). + +### PL-M2 — Operator notes must clearly call out `prisma migrate deploy` + +- v1.4.43 ships migration `0075_v1443_integration_park` (additive, + idempotent — two `ADD COLUMN IF NOT EXISTS` statements). Coolify + auto-deploy DOES NOT run `prisma migrate deploy`; the operator + must run it manually. Recommended note: + > **Migration**: run `pnpm exec prisma migrate deploy` against + > production Postgres **before** the new container starts serving + > traffic. The two new columns are nullable + back-filled lazily + > by the status writer, so existing rows keep working through the + > migration boundary; rollback is `DROP COLUMN IF EXISTS` on both + > columns + `UPDATE integration_statuses SET state = + > 'error_transient' WHERE state = 'parked'`. + +### PL-M3 — Cross-agent commit-attribution drift not relevant this +release (positive observation) + +- Every commit on `develop` since `2c68a48d` is authored by + `Marc-André Bombeck` — `git log --format="%an" 2c68a48d..HEAD | + sort | uniq -c` returns `74 Marc-André Bombeck`. +- Per the `feedback_marathon_worktree_isolation.md` rule (4 + drift-marathons proved shared-tree is broken), the v1.4.43 + marathon dispatched 9 worktree-isolated agents (per the handoff) + and the cherry-pick chain landed clean. **No drift recurrence.** +- Action: cite this in the v1.4.43 retrospective. The worktree- + isolation rule is now proven across 3 consecutive marathons + (v1.4.41 / v1.4.42 / v1.4.43). + +--- + +## Low (defer to v1.4.44 / non-blocking) + +### PL-L1 — `.planning/` audit + phase reports contain "Marc" mentions + +- `round-v1443-AUDIT-mobile-ui-findings.md`, `…qol-findings.md`, + `…analytics-9s-findings.md`, `phase-W4-QoL-COPY-v1443-report.md`, + `phase-W8-OPS-v1443-report.md` all contain literal "Marc" + references. +- Per `feedback_no_pii_in_user_facing.md`, the rule scopes to + *user-facing* artifacts (CHANGELOG, GH releases, `docs/audit/v*`, + `docs/site`, landing). `.planning/` is operator-side work-product + and not published. The mentions are operationally-useful context + for the next marathon. +- **No action required** — confirming the rule doesn't trip here. +- The v1.4.43 CHANGELOG entry MUST stay PII-clean (no Marc-name, no + bombeck.io, no health figures, no BD-Zielbereich values). + +### PL-L2 — REG-11 (dashboard summary 7d window drop) lands without +a phase report + +- Commit `600aa369` is the dashboard `latestEver` fix that closed a + iOS-side BP/pulse tile blank-state bug (5 iOS-side attempts + failed because the bug was in server SQL). It rides v1.4.43 but + doesn't sit under any wave. Marc authored it directly between + W11 dispatch and the QA round. +- The commit body is in German (Marc-authored, not an agent — the + Marc-voice English rule applies to agent-authored commits, not + Marc's direct authorship). +- **Action**: cite REG-11 explicitly in the CHANGELOG `[1.4.43]` + "Fixed" section so the iOS team has the breadcrumb without + hunting commit logs. + +### PL-L3 — W7 deferral semantics drifted between handoff and W14 + +- The v1443-handoff (line 15) marks W7 as "B3 only — B4 + B7 + deferred to v1.4.44". W14 then landed B4 + B7 inside v1.4.43. +- Net effect: v1.4.43 ships more than the handoff scope claimed. + Audit closure is BETTER than promised, but the v1.4.44 backlog + documented in the handoff (line 53) now contains items that + already shipped. +- **Action**: when v1.4.44 backlog rolls forward, drop W7-B4 + W7-B7 + from the "deferred" list. Reflect the actual close in the + retrospective memo. + +### PL-L4 — Test-count delta not yet aggregated + +- Each phase report cites a local test-count delta against their + worktree baseline (W11: 4815→4861, W12: 4815→4877, W13: 4815→4839, + W14: 4815→4842, W6: 4929 total). With overlapping baselines the + net total against `develop` HEAD is unclear. The phase reports + add up to ~+300 unit tests across W4 + W6 + W11 + W12 + W13 + W14 + ignoring overlaps; the full `pnpm test --run` against HEAD will + produce the authoritative number for the CHANGELOG. +- **Action**: run `pnpm test --run` once at HEAD before the version + bump; cite the resulting test count in the CHANGELOG. + +--- + +## Discovery → close coverage matrix + +### Audit: analytics-9s + +| Item | Severity | Status | +|---|---|---| +| W1 — `computeAvg30LastYearMap` `p-limit(4)` cap | URGENT | **Closed** via `c9a54154` + test pin `3218cb50` | + +Verdict: complete. The audit's "recommended fix" body landed +exactly as prescribed. Optional follow-ups (granularity router pin, +24h cache) correctly deferred. + +### Audit: mobile-ui + +| Severity | Closed | Open / Deferred | +|---|---|---| +| Critical (1) | C1 (raw-count gate + `noDataInRange` split) | — | +| High (5) | H1 Switch 44 px, H2 comparison-baseline 44 px, H3 mood-kebab, H4 sheet close-X, H5 reduced-motion smooth-scroll | — | +| Medium (6) | M1–M6 all closed via W11 | — | +| Low (6) | L1, L2, L4 (folded), L6 | L3 (Textarea primitive — refactor scope), L5 (injection-site SVG — non-trivial refactor) | + +Verdict: **all Highs + Criticals closed**. Two Lows correctly +deferred with audit-report-honest documentation in W11 phase report +(L5 specifically rebutting the audit's misread of the element type +— well-handled, Marc-voice). + +### Audit: QoL + +| Severity | Closed | Open / Deferred | +|---|---|---| +| High (6) | H1 loadError key, H2 Anbieter rename, H3 persistent (partial — see PL-H1), H4 not-found i18n, H5 global-error bilingual, H6 plural forms | — | +| Medium (8) | M3 account-delete, M4 doctor-report disabled rows, M5 offline-banner, M6 errorNetwork, M7 daysAgo "d", M8 full Locale union | M1 (drag-drop reorder — a11y scope), M2 (Coach disable — feature scope) | +| Low (8) | L1, L2, L3, L4, L5, L7, L8 | L6 (onboarding gate) | + +Verdict: H3 partial (PL-H1 above) is the single open thread. +Everything else closed cleanly. + +### Audit: security + +| Severity | Closed | Open / Deferred | +|---|---|---| +| High (2) | H-1 audit-row identifier-hash, H-2 WithingsApiError 1024 slice | — | +| Medium (4) | M-1 check-user audit, M-2 plain-text invariant docblock, M-3 replay-injection scan, M-4 trust-violation bucket | — | +| Low (4) | L-2 snapshot scan guard, L-3 passkey Zod boundary | L-1 (withings_state nonce table — OAuth refactor scope), L-4 (legacy_form_total — wait for v1.4.27 cut) | + +Verdict: **all Highs + every Medium + 2/4 Lows closed**. M-3 was +listed as "defer" in the audit but W13 closed it inside the +release. Defence-in-depth surface significantly strengthened. + +--- + +## Release-theme coherence + +v1.4.43 has a clear three-strand narrative: + +1. **9 s perf regression closure** (W1 + measurable in HAR — the + audit's URGENT call) — single largest user-visible win. +2. **Audit-close marathon** — every Critical + every High across + four audits closed, almost every Medium picked up. +3. **iOS-handoff hardening** — Zod multi-issue rollout (41 routes), + Withings parking + per-kind counters, docker version-cache fix + (B11) closes the v1.4.42 recurring paper-cut. + +A clean release-note title: **"v1.4.43 — Analytics 9 s closure, +audit-marathon close (4 reports / 9 waves), Withings parking, iOS +contract Zod rollout."** + +This is a *focused polishing release*, not a grab-bag. Compare with +the v1.4.36 / v1.4.37 perf marathons or v1.4.40 architecture +closure — same shape, same coherence. + +--- + +## iOS contract preservation + +- **W6 Zod rollout (41 routes)**: envelope strictly additive. Every + caller reading `body.error` still gets a human-readable "Validation + failed" string. New callers branch on `body.details.issues`. iOS + v0.5.4 receives the same shape — no break. +- **W14 `/api/integrations/withings/resume`**: net-new POST endpoint + with no payload. Additive. iOS v0.5.4 doesn't call it; future iOS + release MAY add it. +- **W14 `IntegrationStatus.state = "parked"`**: new enum value in a + string field. Per the 0029 migration comment ("adding new + sentinels in v1.5 doesn't require a migration") this is the + documented evolution pattern. iOS v0.5.4 will fall through to a + generic state branch; verified the new `consecutiveFailuresByKind` + + `persistentFailureStartedAt` columns are NOT in the API + view-model (`/api/integrations/status` `IntegrationViewModel` + unchanged). +- **W12 `DELETE /api/settings/account`**: pre-existing endpoint; the + v1.4.43 work is UI-only. +- **REG-11 `/api/dashboard/summary`**: shape unchanged; behaviour + fixed — now serves BP/pulse rows older than 7 d. Pure server-side + bug fix, iOS surface improves with no client changes. + +**No iOS contract drift.** v1.4.43 is iOS v0.5.4-compatible. + +--- + +## Migration readiness + +- Migration `0075_v1443_integration_park` is additive + idempotent + + reversible: + ```sql + ALTER TABLE "integration_statuses" + ADD COLUMN IF NOT EXISTS "consecutive_failures_by_kind" JSONB; + ALTER TABLE "integration_statuses" + ADD COLUMN IF NOT EXISTS "persistent_failure_started_at" TIMESTAMP(3); + ``` +- Both columns nullable — pre-migration rows keep working. The + status writer back-fills `consecutiveFailuresByKind` lazily on the + next failure write. +- Operator notes draft: see PL-M2 above. The migration is the only + release-blocking ops action. + +--- + +## v1.4.44 backlog seed (recommended) + +Carry forward to the next marathon brief: + +### High (must do early) + +- **WTH-WIRE-WARNING** — finalise the audit-H3 wiring: Prisma + `lastFailureKind` column on `IntegrationStatus`, plumb through + `/api/integrations/status` payload, branch `pillStateFor` to + return `"warning"` when `failureKind === "persistent"` (or drop + the dark `"warning"` pill state per PL-H1 Option A). +- **WTH-DROP-LEGACY-COUNTER** — drop `IntegrationStatus.consecutiveFailures` + per the W14 phase report ("v1.4.44 will drop it once every reader + has migrated"). Migration `0076_v1444_drop_legacy_counter`. + +### Medium + +- **TEXTAREA-PRIMITIVE** (audit mobile-ui L3) — introduce + `