mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 12:32:43 +00:00
docs(planning): v1.4.43 W10 QA round findings reports
Six axes — code-reviewer, security, design, senior-dev, product-lead, simplifier — output to .planning/round-v1443-QA-<axis>-findings.md. Verdicts: - code-reviewer: APPROVE_WITH_FIXES (1C closed, 1H = version bump) - security: APPROVE (0C/0H) - design: APPROVE_WITH_FIXES (1C closed, 1H closed) - senior-dev: APPROVE (0C/0H + 1M closed) - product-lead: SHIP (0C/1H closed) - simplifier: CHANGES_REQUESTED (1C + 2H all closed)
This commit is contained in:
Marc-André Bombeck committed
1 parent
468e38c3ef
commit
76a7cbcff8
6 files changed
+1811
No files matched your search
@@ -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.
|
||||
@@ -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 `<Skeleton>` 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 `<Skeleton>` 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 `<OfflineBanner>` 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 `<p>` rather than an explicit live region. Recommend adding `aria-live="polite"` directly on the `<p>` or ensure the test covers the announcement.
|
||||
|
||||
### L4 — `OfflineBanner` paints below `<MaintainershipBanner>` even though comment says "first chrome line"
|
||||
|
||||
**Severity:** Low — comment / behaviour mismatch.
|
||||
|
||||
`auth-shell.tsx:164-169` puts `<OfflineBanner>` immediately above `<MaintainershipBanner>` (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 `<ChartEmptyState>` 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.
|
||||
@@ -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
|
||||
`<Textarea>` in `src/components/ui/textarea.tsx`, sweep the 6+
|
||||
call sites with copy-pasted iOS-zoom defences.
|
||||
- **INJECTION-SITE-PICKER** (audit mobile-ui L5) — refactor the SVG
|
||||
`<circle>` hit zones to HTML `<button>` overlays with a Playwright
|
||||
geometry snapshot harness so the picker doesn't drift during the
|
||||
refactor.
|
||||
- **DRAG-DROP-LAYOUT** (audit qol M1) — drag-to-reorder dashboard
|
||||
widgets with the keyboard arrow buttons as a11y fallback. Defer
|
||||
iff a11y story takes >1 wave.
|
||||
- **COACH-DISABLE-TOGGLE** (audit qol M2) — per-user `disableCoach`
|
||||
flag in Settings → AI; mount-gate `flags.coach && !user.disableCoach`
|
||||
on FAB + drawer.
|
||||
- **ONBOARDING-CHAINED-FLOW-GATE** (audit qol L6) — gate the tour
|
||||
on `onboardingCompletedAt + 24h` so a new user gets the dashboard
|
||||
between the two onboarding flows.
|
||||
- **WITHINGS-STATE-NONCE-TABLE** (audit security L-1) — switch
|
||||
Withings OAuth state to a random nonce + short-lived row in a new
|
||||
table instead of `${userId}:${nonce}` inline.
|
||||
- **LEGACY-FORM-TOTAL** (audit security L-4) — move to Postgres if
|
||||
the v1.4.27 cut still matters, else drop when the legacy form is
|
||||
removed.
|
||||
- **B4 + B7 RECONCILE** (PL-L3) — drop these from the v1.4.44
|
||||
backlog since W14 shipped them. The retrospective memo should
|
||||
note the v1443-handoff scope vs actual close drift.
|
||||
|
||||
### Low
|
||||
|
||||
- **Mobile-UI L3 / L5** as above.
|
||||
- **Cross-tz ±3 h runtime guard** (v1.4.38 carry-forward, still open).
|
||||
- **Coach replay-injection kill-switch** (audit security M-3 was
|
||||
closed; v1.4.44 should add the audit annotation observability
|
||||
per the W13 phase report).
|
||||
|
||||
---
|
||||
|
||||
## Strengths
|
||||
|
||||
S1. **Worktree isolation worked.** All 74 commits attributed to
|
||||
Marc-André Bombeck. Zero cross-agent drift. The
|
||||
`feedback_marathon_worktree_isolation.md` rule is now battle-tested
|
||||
across 3 consecutive marathons.
|
||||
|
||||
S2. **Audit → close ratio is exceptional.** 1 Critical / 13 Highs /
|
||||
24 Mediums closed across four audits + the URGENT analytics fix.
|
||||
Only 2 Highs and 6 Mediums escaped to the v1.4.44 backlog, all
|
||||
correctly justified as scope-budget defers.
|
||||
|
||||
S3. **W1 perf fix is minimum-delta + clean.** The two-line cap
|
||||
matches the v1.4.40 W-POOL discipline exactly; the audit's
|
||||
"Recommended fix" body landed verbatim. The optional follow-ups
|
||||
(granularity router pin, 24h cache) defer cleanly to v1.4.44.
|
||||
|
||||
S4. **The C1 chart-gate fix is product-grade.** Not just "show
|
||||
chart anyway" — the audit's two-option diagnosis was thought
|
||||
through into a three-state model (empty range / few-days /
|
||||
chart-ready) with bilingual copy across all six locales and a new
|
||||
`charts.needMoreDistinctDays*` register. This is the user-facing
|
||||
release win.
|
||||
|
||||
S5. **W6 Zod rollout is contract-safe.** Every iOS-touching route
|
||||
retained its status code + audit-action name + meta passthrough.
|
||||
The `Invalid format:` semantics on the medication CSV importer
|
||||
survive the rewrite via `meta.errorCode`. 220 new test cases pin
|
||||
the multi-issue contract.
|
||||
|
||||
S6. **W13's M-3 (replay-injection) closure exceeds audit
|
||||
recommendation.** The audit listed M-3 as "defer — pin the pattern
|
||||
in v1.4.44". W13 closed it with a refusal short-circuit, an audit
|
||||
annotation, AND an integration test. Closure-above-scope is the
|
||||
right shape when the fix is bounded.
|
||||
|
||||
S7. **W14's per-kind counter design is migration-safe.** The
|
||||
legacy `consecutiveFailures` integer stays in place for one release
|
||||
with `Math.max(consecutiveFailures, buckets.*)` reader semantics; a
|
||||
v1.4.44 migration drops it after every reader has converged. This
|
||||
is the soft-migration pattern that's worked across v1.4.40
|
||||
soft-delete and v1.4.42 queryKey-factory closures.
|
||||
|
||||
S8. **B11 docker version-cache fix closes a recurring paper cut.**
|
||||
The v1.4.42 release shipped a stale `package.json` bundle (1.4.41
|
||||
in the 1.4.42 image) that required a workflow_dispatch retrigger.
|
||||
The new `ARG NEXT_PUBLIC_APP_VERSION` baked into the image layer
|
||||
cache key makes this impossible by construction. The runtime
|
||||
`/api/version` reads from env-first so the bundle version is
|
||||
authoritative.
|
||||
|
||||
S9. **PWA SW cache-version self-heal is closed.** L3 in the QoL
|
||||
audit flagged `CACHE_VERSION = "v1.4.38.4"` as 4 releases stale.
|
||||
The `scripts/generate-sw-version.mjs` prebuild step now writes
|
||||
`sw-version.js` per build and the SW loads it via `importScripts`.
|
||||
v1.4.38.4 self-heal architecture is now genuinely self-healing
|
||||
across every release.
|
||||
|
||||
S10. **REG-11 was caught by Marc, not by the audit waves.** The
|
||||
dashboard `latestEver` SQL bug existed since v1.4.38 W-F and only
|
||||
manifested on iOS where the BP/pulse tile was blank for accounts
|
||||
whose last reading was older than 7 days. 5 iOS-side attempts had
|
||||
failed before Marc traced it to server SQL. Worth noting that the
|
||||
mobile-ui audit didn't catch it (the dashboard summary endpoint
|
||||
isn't a UI surface; it's a query shape). Future audits could
|
||||
benefit from a "endpoint-vs-UI-state contract" reviewer pass.
|
||||
|
||||
---
|
||||
|
||||
## What to do next (in order)
|
||||
|
||||
1. Address PL-H1 — recommend Option A (drop dark `"warning"` pill code).
|
||||
2. Run `pnpm test --run` at HEAD; record the test count.
|
||||
3. Bump `package.json` version to `1.4.43`.
|
||||
4. Draft CHANGELOG `[1.4.43]` entry per the v1.4.42 shape (Added /
|
||||
Changed / Fixed / Performance / Operator notes; cite all 4 audit
|
||||
reports + 7 phase reports + REG-11). Stay PII-clean.
|
||||
5. Operator notes must say "**Run `pnpm exec prisma migrate
|
||||
deploy` before container restart**" per PL-M2.
|
||||
6. Squash-merge develop → main + tag v1.4.43 + GH release + Coolify
|
||||
deploy + verify `/api/version → 1.4.43` per the handoff steps 8–12.
|
||||
7. Carry the v1.4.44 backlog forward per the seed above.
|
||||
|
||||
This is a clean release. Ship it.
|
||||
@@ -0,0 +1,203 @@
|
||||
# v1.4.43 QA-Security findings (W10-QA-SECURITY)
|
||||
|
||||
Scope: read-only review of `git diff 2c68a48d..develop` covering 74
|
||||
commits between v1.4.42 and the v1.4.43 release candidate.
|
||||
|
||||
Reviewer: W10-QA-SECURITY (static review, no execution).
|
||||
|
||||
## Verdict
|
||||
|
||||
**APPROVE** — ship as-is.
|
||||
|
||||
Every Critical / High / Medium / Low from the W-AUDIT-SECURITY round is
|
||||
closed in code with regression tests, or explicitly documented as a
|
||||
defence-in-depth invariant. No CORS / CSP regression; no migration
|
||||
constraint relaxation; no PII leak in any new audit row; the
|
||||
trust-violation tighter bucket lands across every IP-keyed auth surface;
|
||||
the W6 Zod-rollout preserves every status code (sole 400 case is the
|
||||
consent route, intentional and commented). The two new endpoints
|
||||
(`/api/integrations/withings/resume` DELETE and
|
||||
`/api/settings/account` DELETE) are user-scoped, rate-limited, and
|
||||
session-invalidating where required.
|
||||
|
||||
## Critical (must fix before tag — vuln / data leak / auth bypass)
|
||||
|
||||
**None.**
|
||||
|
||||
## High (should fix before tag — defence-in-depth gap)
|
||||
|
||||
**None.** All H findings from the audit round closed:
|
||||
|
||||
- **H-1** (`auth.login.failed` raw identifier) — **CLOSED** in
|
||||
`b70cdcae`. `src/app/api/auth/login/route.ts:71-74` now writes only
|
||||
`{ reason: "user_not_found_or_no_password" }` — identifier field
|
||||
dropped entirely. Regression test in
|
||||
`src/app/api/auth/login/__tests__/route.test.ts:108-130` pins both
|
||||
the absence of an `identifier` key AND the literal email string in
|
||||
the serialised audit row.
|
||||
|
||||
- **H-2** (`WithingsApiError.message` un-capped) — **CLOSED** in
|
||||
`921d31b8`. `src/lib/withings/response-classifier.ts:252-255` caps
|
||||
the message at 1024 chars in the constructor, so every downstream
|
||||
audit / notification / pg-boss retry path inherits the bound.
|
||||
Regression test in
|
||||
`src/lib/withings/__tests__/response-classifier.test.ts:180-191`
|
||||
pins the cap for a 4000-char upstream while preserving the legacy
|
||||
`Withings <verb> error: <status>` prefix.
|
||||
|
||||
## Medium
|
||||
|
||||
**None blocking.** All M findings closed or pinned-as-invariant:
|
||||
|
||||
- **M-1** (`check-user` un-audited) — **CLOSED** in `7f84e950`.
|
||||
`src/app/api/auth/check-user/route.ts:109-137` now hashes the
|
||||
identifier via `hashToken` and persists every branch (`not_found`,
|
||||
`passkey_only`, `email_fallback`, `exists`) to `AuditLog`. Wide-event
|
||||
`annotate()` retained; audit row is durable + hashed.
|
||||
|
||||
- **M-2** (Telegram parseMode invariant) — **DOCUMENTED** in
|
||||
`8767f819`. `src/lib/integrations/status.ts:828-851` carries a
|
||||
SECURITY INVARIANT docblock explaining why the admin-alert path must
|
||||
not flip to `parseMode: "HTML"`. No code change; the existing plain-
|
||||
text rendering keeps the upstream-influenced string inert.
|
||||
|
||||
- **M-3** (Coach replay-injection on prior turns) — **CLOSED** in
|
||||
`81b1eeea`. `src/app/api/insights/chat/route.ts:198-233` re-runs
|
||||
`detectRefusal` against every user-role turn re-loaded from
|
||||
`coach_messages.encryptedContent` before building the transcript.
|
||||
On a hit, the SSE short-circuits with a refusal AND a durable
|
||||
`audit.coach.replay-injection` row carrying conversation id, turn
|
||||
index, and matched reason — never the message content. Integration
|
||||
test in `tests/integration/coach-chat.test.ts` (committed alongside)
|
||||
pins both the short-circuit and the audit-row shape.
|
||||
|
||||
- **M-4** (trust-chain misconfig → shared `"unknown"` bucket) —
|
||||
**CLOSED** in `d324d516`. `src/lib/rate-limit.ts:84-127` adds
|
||||
`checkAuthSurfaceRateLimit`, which consults
|
||||
`getClientIpOrTrustWarning` and, on `trustViolation === true`,
|
||||
routes the request to a shared global bucket
|
||||
`auth:anon:trust-violation` capped at 100/15min across every auth
|
||||
surface. Six routes converted (login, register, passkey login-
|
||||
options, passkey login-verify, refresh, check-user); `password` is
|
||||
per-user and stays on direct `checkRateLimit`. Test suite in
|
||||
`src/lib/__tests__/rate-limit-auth-surface.test.ts` covers all four
|
||||
pivot conditions (clean chain + IP, clean chain + null IP, violation
|
||||
+ IP, violation + null IP) plus the cross-surface bucket-sharing
|
||||
property.
|
||||
|
||||
## Low
|
||||
|
||||
All L findings closed or documented:
|
||||
|
||||
- **L-2** (Coach `userPrompt` raw string concat) — pinned via
|
||||
`58f48413` regression-test guard
|
||||
(`src/lib/ai/coach/__tests__/snapshot.test.ts`).
|
||||
|
||||
- **L-3** (`passkey/login-verify` raw cast) — **CLOSED** in `ef7e1f03`.
|
||||
`src/lib/auth/passkey.ts:13-37` adds the
|
||||
`authenticationResponseSchema` Zod narrow + applies it inside
|
||||
`verifyAuthentication`; the SimpleWebAuthn cryptographic verifier
|
||||
retains downstream ownership. `.loose()` keeps forward-compat with
|
||||
WebAuthn extension fields.
|
||||
|
||||
- **L-1, L-4** — deferred to v1.4.44 per the audit round; verified no
|
||||
regression in this release.
|
||||
|
||||
## Strengths
|
||||
|
||||
1. **W3/W13 audit-row PII discipline is end-to-end.** Every new
|
||||
`auditLog` call in the diff writes either a `userId` (server-known),
|
||||
a hashed identifier (`hashToken`), a reason enum, or sanitised Zod
|
||||
issues (`sanitiseZodIssues` — path/code/message only, `issue.params`
|
||||
stripped). Searched the diff for `auditLog` + raw `identifier` /
|
||||
`email` / `password` — no hits in any new call site.
|
||||
|
||||
2. **W6 Zod-rollout preserves status codes byte-for-byte.** Spot-checked
|
||||
every new `returnAllZodIssues(parsed.error, ...)` call site against
|
||||
the prior `apiError(parsed.error.issues[0].message, ...)`: 36 sites
|
||||
stayed on 422; the 3 consent routes stayed on 400. Confirmed against
|
||||
`2c68a48d:src/app/api/consent/ai/route.ts` (the only 400 originator);
|
||||
`returnAllZodIssues(parsed.error, 400)` carries an inline comment
|
||||
pinning the intentional preservation. No accidental 422→400 swap or
|
||||
the reverse.
|
||||
|
||||
3. **`sanitiseZodIssues` privacy floor is tight.** `path`, `code`,
|
||||
`message` only — `issue.params` (which Zod uses to echo the rejected
|
||||
value for some issue codes) stays server-side
|
||||
(`src/lib/api-response.ts:37-45`). Verified across the full rollout.
|
||||
|
||||
4. **W14 resume endpoint is correctly scoped + rate-limited.**
|
||||
`/api/integrations/withings/resume` requires `requireAuth()`, then
|
||||
keys `checkRateLimit` by `withings-resume:{user.id}` (5/min). The
|
||||
downstream `resumeIntegrationFromPark(user.id, "withings")` queries
|
||||
and upserts on the `userId_integration` composite-unique — cross-
|
||||
user clobber impossible. Idempotent: a second call sees
|
||||
`existing?.state !== "parked"`, returns `wasParked: false` and
|
||||
writes no audit row. No CORS / CSP impact (no new origin or header).
|
||||
|
||||
5. **W12 account-delete cascade is bounded + GDPR-clean.**
|
||||
`/api/settings/account` DELETE requires `requireAuth()` + a literal
|
||||
`confirm: "DELETE_ACCOUNT"` string in the body (typed-as-string then
|
||||
strict-equality compared, not just truthy). Last-admin guard
|
||||
prevents lock-out. Sessions, API tokens, refresh tokens all revoked
|
||||
via `destroyAllSessions(userId)` in a single Prisma transaction —
|
||||
every revoke is `where: { userId }`. Feedback + AuditLog rows are
|
||||
purged for GDPR Art. 17 completeness BEFORE the final
|
||||
`user.delete()` (otherwise the cascade `SetNull`s them, leaving PII
|
||||
orphans). Audit row written BEFORE the deletion sweep; the row's
|
||||
own `userId` is purged in the subsequent `auditLog.deleteMany`.
|
||||
|
||||
6. **Migration 0075 (`integration_park`) is purely additive.** Two
|
||||
nullable columns (`consecutive_failures_by_kind jsonb`,
|
||||
`persistent_failure_started_at timestamp`) with `ADD COLUMN IF NOT
|
||||
EXISTS` guards. No NOT NULL, no FK change, no unique index touched.
|
||||
`state` column stays free-form string; `"parked"` is recognised at
|
||||
the application layer. `onDelete: Cascade` on the FK preserved.
|
||||
|
||||
7. **CSP / proxy unchanged.** `git diff 2c68a48d..develop -- src/proxy.ts`
|
||||
is empty. The HSTS, CSP-with-route-gated `connect-src`,
|
||||
`frame-ancestors`, `Referrer-Policy`, `Permissions-Policy` headers
|
||||
continue to ship from the same builder. No new endpoint declared
|
||||
its own headers / CORS shim. `grep "Access-Control-Allow" src/`
|
||||
returns clean.
|
||||
|
||||
8. **W3-SECURITY regression test is the right shape.**
|
||||
`login/__tests__/route.test.ts:108-130` asserts both the absence
|
||||
of the `identifier` key in `payload.details` and the literal
|
||||
typed-email string anywhere in the JSON serialisation. A future
|
||||
refactor that re-introduces the field under any new name still
|
||||
fails the second assertion. Same pattern lands the W13 M-1
|
||||
check-user audit-row test.
|
||||
|
||||
9. **W13 M-3 (Coach replay-injection) audit-row content is leak-free.**
|
||||
The new `audit.coach.replay-injection` row carries
|
||||
`conversationId`, `turnIndex`, and the matched `reason` enum —
|
||||
never the raw turn content. So an injection payload that surfaces
|
||||
the audit row doesn't survive into the audit ledger itself.
|
||||
|
||||
10. **Dashboard widgets audit-row dedup (B2) is process-local + safe-
|
||||
failure.** The in-process memo on `(userId, action)` keyed at 60 s
|
||||
drops at most one row per process per minute on a tight retry loop
|
||||
— far below the operator-grep threshold but eliminates the
|
||||
audit-flood DoS surface. Memo resets on process restart (one extra
|
||||
row per restart; tolerable). Cluster deploys keep one row per pod
|
||||
per minute, which is fine.
|
||||
|
||||
## Out-of-scope items observed (no action required)
|
||||
|
||||
- The Coach SSE `detectRefusal` regex bank is still the only injection
|
||||
barrier — same as v1.4.42. The W13 M-3 close adds replay-injection
|
||||
guard but does not strengthen the regex itself (deferred per the
|
||||
audit-round verdict).
|
||||
|
||||
- `getClientIpOrTrustWarning` still fires the `console.warn` only once
|
||||
per process. Operators relying on the warn for chain misconfig
|
||||
detection still need external alerting. The tight global bucket
|
||||
(W13 M-4) caps the blast radius regardless.
|
||||
|
||||
- The `withings_state` cookie still carries `userId:nonce` plaintext
|
||||
(L-1) — deferred to v1.4.44.
|
||||
|
||||
---
|
||||
|
||||
(verdict + critical/high count + filename)
|
||||
@@ -0,0 +1,368 @@
|
||||
# v1.4.43 QA Senior-Dev findings (W10-QA-SENIOR-DEV)
|
||||
|
||||
Scope: architectural + correctness review of `git diff 2c68a48d..develop`
|
||||
covering 74 commits between v1.4.42 and the v1.4.43 release candidate.
|
||||
|
||||
Reviewer: W10-QA-SENIOR-DEV (read-only static review, no execution).
|
||||
|
||||
## Verdict
|
||||
|
||||
**APPROVE WITH 1 MEDIUM + 4 LOW** — ship as-is; address the bucket
|
||||
back-fill off-by-one before v1.4.44 drops the legacy `consecutive_failures`
|
||||
column.
|
||||
|
||||
No Critical / High findings. The five sentinel landings (W1-ANALYTICS,
|
||||
W14-WITHINGS-PARK, W6-ZOD-ROLLOUT, W13-RATE-LIMIT-WRAPPER, W12-ACCOUNT-DELETE)
|
||||
are architecturally sound. Cherry-pick aftermath commits (`32ca196e`,
|
||||
`154e6fa7`) cleanly close branch-merge collateral, not deeper bugs.
|
||||
Pre-existing `workout-batch-{create,race}` integration failures
|
||||
confirmed pre-date this release (last touched in v1.4.25).
|
||||
|
||||
---
|
||||
|
||||
## Critical
|
||||
|
||||
None.
|
||||
|
||||
## High
|
||||
|
||||
None.
|
||||
|
||||
## Medium
|
||||
|
||||
### M-1 — Per-kind bucket back-fill is permanently 1 behind the legacy counter on the transition row
|
||||
- **File**: `src/lib/integrations/status.ts:375-395` (back-fill branch
|
||||
inside `recordSyncFailure`).
|
||||
- **Trace**: a v1.4.42 row at `consecutive_failures=3, ...byKind=null,
|
||||
state=error_reauth` receives a fresh `reauth_required` failure. The
|
||||
code path:
|
||||
1. `startingBuckets = null` (column predates migration).
|
||||
2. `buckets = backfillBuckets(3, "reauth_required")` →
|
||||
`{transient: 0, reauth_required: 3, persistent: 0}` — seeds the
|
||||
bucket with the **legacy count**, NOT `legacy + 1`.
|
||||
3. The increment branch at line 384 (`if (startingBuckets)`) does
|
||||
NOT fire because `startingBuckets` is still null in this scope.
|
||||
4. The fall-through `else if (!existing)` at line 387 does NOT fire
|
||||
because `existing` is truthy.
|
||||
5. Upsert writes `consecutive_failures: { increment: 1 }` → DB row
|
||||
becomes `consecutive_failures=4, consecutiveFailuresByKind.reauth_required=3`.
|
||||
|
||||
After this write: legacy counter is 4, bucket is 3 — off by 1. On
|
||||
every subsequent failure the `startingBuckets` path increments by 1,
|
||||
so the gap persists forever. The docstring comment at line 391-395
|
||||
claims "back-fills to reauth_required: 4 and this 5th failure makes
|
||||
the legacy +1 path bring the bucket to 5" — but there is no separate
|
||||
`+1` path that runs on the back-fill branch. Either the comment or
|
||||
the code is wrong.
|
||||
- **Operational impact today**: tolerable. The alert ladder at line 487
|
||||
reads `Math.max(row.consecutiveFailures, ...buckets)` so the legacy
|
||||
column still drives paging decisions; the audit `bucketCount` field
|
||||
reports the smaller bucket value while `attemptNumber` reports the
|
||||
correct higher legacy count, creating a 1-row diagnostic confusion
|
||||
but no functional miss. Operators who grep audit rows by
|
||||
`bucketCount >= 3` will under-count by exactly one streak.
|
||||
- **Impact on v1.4.44**: the migration comment at the top of
|
||||
`0075_v1443_integration_park/migration.sql` says "the single-column
|
||||
`consecutive_failures` integer stays in place for one release as a
|
||||
fallback (callers reading the alert-ladder still use
|
||||
`Math.max(...buckets)`); it will be removed in v1.4.44 once every
|
||||
reader is migrated." When that drop lands, the alert ladder will lose
|
||||
the `Math.max(row.consecutiveFailures, …)` floor and start paging at
|
||||
bucket≥3 instead of legacy≥3 — every back-filled row will page one
|
||||
failure too late.
|
||||
- **Recommended fix (one-line)**: in `backfillBuckets` change
|
||||
`buckets[currentKind] = Math.max(0, legacyCount)` →
|
||||
`buckets[currentKind] = Math.max(0, legacyCount) + 1`, and update the
|
||||
unit test at `src/lib/integrations/__tests__/status.test.ts:666` to
|
||||
expect `reauth_required: 4` instead of `3`. The "this failure IS
|
||||
counted in that seed" docstring then becomes accurate.
|
||||
- **Severity rationale**: M, not High — the legacy column carries the
|
||||
load until v1.4.44; the fix is one line + one test expectation flip;
|
||||
no live production user is currently affected because the legacy
|
||||
counter still drives the alert ladder. Worth fixing before the
|
||||
v1.4.44 drop ships.
|
||||
|
||||
## Low
|
||||
|
||||
### L-1 — `verifyRegistration` still uses raw `as RegistrationResponseJSON` cast
|
||||
- **File**: `src/lib/auth/passkey.ts:153`.
|
||||
- **Trace**: W13 L-3 closed the cast on `verifyAuthentication` (line
|
||||
225-229 — explicit Zod parse via `authenticationResponseSchema`) but
|
||||
the symmetric `verifyRegistration` path at line 137-160 still does
|
||||
`response: response as RegistrationResponseJSON` without any prior
|
||||
shape validation. SimpleWebAuthn's `verifyRegistrationResponse`
|
||||
throws on schema mismatch, so the security posture is identical to
|
||||
the pre-W13 authentication path the wave deliberately closed. The
|
||||
W13 report does not document this as out-of-scope; it appears to be
|
||||
an oversight or an explicit scope-cap.
|
||||
- **Recommended fix**: mirror the `authenticationResponseSchema` with
|
||||
`registrationResponseSchema` and apply the same `safeParse → throw on
|
||||
failure` pattern. Two-test addition: valid registration accepted,
|
||||
malformed body rejected before reaching the verifier.
|
||||
- **Severity rationale**: L because the SimpleWebAuthn verifier does
|
||||
own the cryptographic validation downstream and the registration
|
||||
surface is gated by an authenticated requireAuth (lower attack
|
||||
surface than the un-authenticated login flow). Documenting it here
|
||||
so a v1.4.44 sweep closes the asymmetry.
|
||||
|
||||
### L-2 — Trust-violation tight bucket is global across the cluster, not isolated per process
|
||||
- **File**: `src/lib/rate-limit.ts:82-127`.
|
||||
- **Trace**: `TIGHT_ANON_KEY` is a single string `"auth:anon:trust-violation"`
|
||||
keyed into the Postgres `rate_limits` table, so a misconfigured
|
||||
deployment serving 20 anonymous callers across 5 pods will see them
|
||||
all share the same 100/15min cap (across the cluster, not per pod).
|
||||
The brief asked to verify "tight-global-bucket isolation per
|
||||
process" — confirmed it is NOT per-process; it is per-cluster, since
|
||||
the underlying `checkRateLimit` is Postgres-backed. The behaviour
|
||||
matches the existing `auth:login:<ip>` semantics (also cluster-wide
|
||||
via Postgres) so this is design-consistent rather than a bug.
|
||||
- **Impact**: in a real trust-violation incident a single attacker
|
||||
could exhaust the 100/15min cap and lock every other anonymous
|
||||
caller out of every auth surface across the cluster until the
|
||||
operator fixes `TRUST_PROXY_HOPS`. The `console.warn` is once-per-
|
||||
process — so an N-pod cluster emits N warnings before the operator
|
||||
notices. The brief asked about "false positives when TRUST_PROXY_HOPS
|
||||
is correctly configured" — confirmed clean: a correct config never
|
||||
trips `trustViolation === true` and the per-IP bucket fires unchanged.
|
||||
- **Recommended action**: none in v1.4.43. Document the cluster-wide
|
||||
scope in the docstring of `checkAuthSurfaceRateLimit` so the next
|
||||
operator doesn't expect per-pod isolation.
|
||||
|
||||
### L-3 — `recordSyncFailure` read-then-write is not transactional; concurrent failures can clobber bucket increments
|
||||
- **File**: `src/lib/integrations/status.ts:347-455`.
|
||||
- **Trace**: the function reads the existing row at line 347, computes
|
||||
the new bucket envelope in JS, then upserts with the full computed
|
||||
envelope. The integer `consecutiveFailures` uses atomic
|
||||
`{ increment: 1 }` (line 451), but `consecutiveFailuresByKind: buckets`
|
||||
(line 452) is a full overwrite from a stale read. If two concurrent
|
||||
failure callers race the read window, the second upsert clobbers
|
||||
the first's bucket increment. Net effect: bucket count is the
|
||||
pre-read value + 1, not the actual concurrent count.
|
||||
- **Impact today**: minimal. The only callers are Withings + moodLog
|
||||
per-user sync routines, each of which holds a per-(user, integration)
|
||||
lock implicitly via pg-boss job singletons — concurrent failures for
|
||||
the same user on the same integration are not actually possible in
|
||||
the deployed topology. The legacy `{ increment: 1 }` column still
|
||||
drives the alert ladder so an under-count in the bucket doesn't
|
||||
affect paging.
|
||||
- **Recommended action**: none in v1.4.43. Before v1.4.44 drops the
|
||||
legacy column, either wrap the read+upsert in `prisma.$transaction`
|
||||
with a row-level lock, or replace the bucket overwrite with a
|
||||
Postgres jsonb update that atomically increments only the matching
|
||||
bucket (`jsonb_set` + `(value::int + 1)::text::jsonb`).
|
||||
|
||||
### L-4 — Audit-dedup memo is per-process only on a multi-pod deployment
|
||||
- **File**: `src/app/api/dashboard/widgets/route.ts:54-72`.
|
||||
- **Trace**: the `auditDedupMemo` Map at module scope is in-process
|
||||
only — a cluster with 5 pods can write 5 identical audit rows per
|
||||
minute even under the dedup. The docstring acknowledges this: "Cluster
|
||||
deployments still get one row per process per minute, which is fine
|
||||
for the operator-grep use case the audit row exists for." Verified
|
||||
the docstring is accurate and the choice is intentional. The 512-entry
|
||||
opportunistic GC at line 66-69 is correct (drops entries older than
|
||||
the window) and cannot grow unbounded for any realistic load.
|
||||
- **Note**: only `dashboard/widgets/route.ts` got the dedup; every other
|
||||
W6-rollout route writes one audit row per Zod miss with no dedup.
|
||||
Verified by `grep shouldEmitAuditRow` returning a single file. This
|
||||
is consistent with the W6 report's framing of widgets as the
|
||||
iOS-contract-loop-prone surface; other surfaces don't see the
|
||||
loop pattern often enough to justify the memo overhead.
|
||||
|
||||
---
|
||||
|
||||
## Strengths
|
||||
|
||||
1. **Schema migration soundness (W14 / `0075_v1443_integration_park`)**.
|
||||
- `ADD COLUMN IF NOT EXISTS` on both new columns — idempotent against
|
||||
reruns.
|
||||
- Both columns nullable with no DEFAULT, so existing rows stay
|
||||
compatible without a backfill UPDATE (the application-layer
|
||||
`backfillBuckets` handles per-row migration on next write — though
|
||||
see M-1).
|
||||
- The migration SQL header documents the reverse migration explicitly:
|
||||
`ALTER TABLE ... DROP COLUMN IF EXISTS` on both columns + an
|
||||
`UPDATE` clearing any `state = 'parked'` rows back to
|
||||
`error_transient`. Reversible.
|
||||
- `state` stays a free-form string column per the 0029 invariant —
|
||||
adding `"parked"` is a no-op at the SQL layer; the enum widening is
|
||||
application-owned. Consistent with the codebase's documented
|
||||
pattern.
|
||||
- Schema Prisma type updates added BOTH columns + the `"parked"` enum
|
||||
comment update in one commit (`bd538b02`). No drift between
|
||||
migration SQL and Prisma schema.
|
||||
|
||||
2. **`p-limit(4)` cap on `computeAvg30LastYearMap`** is correctly
|
||||
isolated.
|
||||
- The pin test at
|
||||
`src/lib/analytics/__tests__/summaries-slice.wmy-cap.test.ts` walks
|
||||
a 15-type fan-out with controllable resolvers and asserts in-flight
|
||||
count never exceeds 4 across the full drain cycle. Pinned at the
|
||||
module-level constant (`WMY_FANOUT_CONCURRENCY = 4`).
|
||||
- The cap composes additively with the thick-route's
|
||||
`ANALYTICS_TYPE_FETCH_CONCURRENCY = 4`: on a dashboard mount that
|
||||
fires both `?slice=summaries` and the default slice in parallel,
|
||||
the worst-case combined pool draw is `4 + 4 = 8` slots from
|
||||
analytics, well under the `pg.Pool max=20` v1.4.40 raised it to.
|
||||
No collision risk.
|
||||
- Verified zero behavioural delta in the rollup-tier read paths;
|
||||
this is purely a fan-out cap that defers work, not a result
|
||||
change.
|
||||
|
||||
3. **W6 Zod-rollout pattern**.
|
||||
- All 35 routes preserve the existing `{ data: null, error: <string>, ...}`
|
||||
envelope. Added `details.issues` is strictly additive — iOS v0.5.4
|
||||
clients reading `body.error` see the unchanged `"Validation failed"`
|
||||
string. Pinned by `src/lib/__tests__/api-response-zod.test.ts` +
|
||||
27 new route-level test files.
|
||||
- `sanitiseZodIssues` projection drops `issue.params` consistently —
|
||||
verified across all 35 callers; every route-level test asserts
|
||||
`Object.keys(issue).sort() === ["code", "message", "path"]`.
|
||||
- `safeJson` is called BEFORE the Zod parse on every POST/PUT/PATCH
|
||||
route — verified spot-check on `measurements/route.ts` (line 568),
|
||||
`dashboard/widgets/route.ts`, `medications/route.ts`. The
|
||||
content-type and JSON-parse failures still return their dedicated
|
||||
415/400 envelopes before any Zod work runs.
|
||||
- Audit-row writes use the fire-and-forget `.catch(() => {})` pattern
|
||||
consistently across every W6 site. Verified the audit-write-
|
||||
rejection survival case is pinned by tests on multiple routes.
|
||||
- The dedup memo (B2) is correctly scoped to widgets only — every
|
||||
other route writes one audit row per miss because their failure
|
||||
patterns don't loop.
|
||||
|
||||
4. **W13 `checkAuthSurfaceRateLimit` wrapper introduction**.
|
||||
- Signature backward-compat: the wrapper returns `RateLimitResult &
|
||||
{ ip: string | null }`. Existing call sites picked the IP up from
|
||||
`getClientIp(request)` before; they now read `rl.ip ?? "unknown"`.
|
||||
The shape extension is additive — `result.allowed`,
|
||||
`result.remaining`, `result.resetAt` are byte-identical for the
|
||||
happy path.
|
||||
- Trust-violation tight bucket fires only when
|
||||
`getClientIpOrTrustWarning` reports `trustViolation === true` —
|
||||
verified the helper at `src/lib/api-response.ts:268-300`. A clean
|
||||
`TRUST_PROXY_HOPS=1` + single-XFF deployment never trips the tight
|
||||
bucket; pinned by
|
||||
`src/lib/__tests__/rate-limit-auth-surface.test.ts:75-87`.
|
||||
- Test coverage is comprehensive: 8 tests on the wrapper, three
|
||||
existing test files updated to mock both `checkRateLimit` and
|
||||
`checkAuthSurfaceRateLimit`. The `154e6fa7` cherry-pick aftermath
|
||||
commit closes the last test-mock gap (login + register) — verified
|
||||
this is a green-up commit, not a paper-over.
|
||||
|
||||
5. **W12 `/api/settings/account` DELETE cascade**.
|
||||
- User-scoped by `user.id` from `requireAuth()` — no admin-acting-as-user
|
||||
surface, no email/username lookup. Bounded delete.
|
||||
- `destroyAllSessions(userId)` runs BEFORE `prisma.user.delete` (line
|
||||
54) so concurrent sibling sessions can no longer authenticate by
|
||||
the time the cascade completes. The integration test at
|
||||
`tests/integration/settings-account-delete.test.ts:154-177` pins
|
||||
this against real Postgres.
|
||||
- Audit row is written BEFORE the delete at line 47-51 — the
|
||||
comment on line 44-46 acknowledges this and explains the
|
||||
intentional follow-up purge at line 62 for GDPR Art. 17 erasure
|
||||
completeness. The `annotate({ action: "settings.account.delete" })`
|
||||
wide-event at line 67-70 IS the durable operator-facing trail; the
|
||||
audit row gets created+purged inside the same logical operation by
|
||||
design. The trail is correct and explicit.
|
||||
- Last-admin guard at line 34-39 — single-admin instances can't
|
||||
foot-shoot themselves into an unmanaged install. Pinned by test
|
||||
at line 208-219.
|
||||
- 422 on missing/wrong confirmation, 401 on no session, 400 on
|
||||
last-admin guard — all preserved and pinned.
|
||||
|
||||
6. **W14 `parked` state machine**.
|
||||
- The state machine is sticky-correct: once parked, the row stays
|
||||
parked until `resumeIntegrationFromPark` clears it OR a success
|
||||
arrives (which can't happen because `isReauthRequired` returns
|
||||
`true` for both `error_reauth` and `parked`, short-circuiting the
|
||||
sync entry-point).
|
||||
- The 24h park threshold uses `persistentFailureStartedAt` (wall-
|
||||
clock anchor on the first persistent failure of a streak) +
|
||||
`PARK_PERSISTENT_FAILURE_AFTER_MS=24h`. The streak anchor is
|
||||
stamped on the first persistent failure and cleared on success —
|
||||
same idempotency semantics as the existing `alertedAt` field.
|
||||
- `persistentStreakBefore` snapshot at line 369 correctly reads the
|
||||
pre-increment bucket value so the "first persistent failure of a
|
||||
fresh streak" detection at line 405 works on a fresh streak +
|
||||
correctly NO-OPs on a continuing streak. Subtle but correct.
|
||||
- Park-event audit row at line 524-537 is written ONCE per transition
|
||||
(not per failure) by gating on `existing?.persistentFailureStartedAt`
|
||||
— the FIRST persistent failure that crosses 24h sees the anchor
|
||||
populated from the prior write, so the audit row fires exactly
|
||||
once. Verified by unit test "flips state to parked and writes an
|
||||
audit row once persistent streak > 24h".
|
||||
|
||||
7. **Cherry-pick aftermath quality**.
|
||||
- `32ca196e` (chart-i18n restore) — verified all 4 keys (W11 +
|
||||
W2 additions) restored across all 6 locales. The merge collision
|
||||
was W12's i18n tighten on the same `messages/*.json` root sweep
|
||||
wiping out sibling-wave additions; the restore is verbatim, not
|
||||
a behaviour change. Reversibility verified: green CI on the keyed
|
||||
locales, no orphan key warnings from `pnpm i18n:check`.
|
||||
- `154e6fa7` (auth-test mocks) — adds `checkAuthSurfaceRateLimit`
|
||||
mock to login + register test factories that already mocked the
|
||||
underlying `checkRateLimit`. The W13 wave wired the surface but
|
||||
missed the legacy test files; this commit closes a green-CI gap,
|
||||
it does not paper over a runtime regression. Verified the test
|
||||
factories already pre-existed; only the new mock function entry
|
||||
was added.
|
||||
|
||||
8. **Pre-existing failure attribution**.
|
||||
- `tests/integration/workout-batch-{create,race}.test.ts` last
|
||||
touched in v1.4.25 (`caf2371c` + `6ed925fe` + `17b8d8d2 chore(release): v1.4.25`).
|
||||
`git log 2c68a48d..HEAD -- tests/integration/workout-batch-*` returns
|
||||
EMPTY — no v1.4.43 commit touches either file. The W13 phase
|
||||
report's flag is correct: pre-existing, out-of-scope for v1.4.43.
|
||||
|
||||
9. **iOS v0.5.4 backward compatibility**.
|
||||
- 35 W6 routes preserve `body.error` as the human-readable string
|
||||
(verified via test: `expect(body.error).toBe("Validation failed")`).
|
||||
- The new `details.issues` field is additive — JSON decoders that
|
||||
don't model it skip it.
|
||||
- Status codes preserved across every conversion: every `apiError(...,
|
||||
422)` became `returnAllZodIssues(parsed.error, 422)` and every
|
||||
`apiError(..., 400)` became `returnAllZodIssues(parsed.error, 400)`.
|
||||
- DELETE `/api/settings/account` is a new endpoint, NOT a contract
|
||||
change to existing iOS surface — iOS v0.5.4 doesn't call it.
|
||||
- The new `parked` state is exposed via the existing integration-
|
||||
status response shape (the IntegrationStatusPill component reads
|
||||
`state` as a string); iOS doesn't currently surface this UI so
|
||||
the new sentinel is a no-op for v0.5.4 clients.
|
||||
- `auth.check-user` audit row is server-side only; the response
|
||||
shape (`{ branch, hasPasskey, hasPassword }`) is unchanged.
|
||||
|
||||
10. **Docker tag-baking (B11) closes the v1.4.42 stale-bundle paper-cut**.
|
||||
- `NEXT_PUBLIC_APP_VERSION` is passed via `--build-arg` from the
|
||||
docker-publish workflow, picked up as both `ARG` + `ENV` in the
|
||||
builder + runner stages of the Dockerfile, then read by
|
||||
`/api/version` at runtime with package.json fallback. The
|
||||
build-arg becomes part of the BuildKit layer cache key, so a
|
||||
release-tag bump invalidates every downstream layer that
|
||||
depends on it. The recurring "stale package.json version baked
|
||||
into the bundle" failure mode from the v1.4.42 retrigger is now
|
||||
structurally closed.
|
||||
|
||||
---
|
||||
|
||||
## Items verified clean — listed for the trail
|
||||
|
||||
- `pull_policy: always` (v1.4.34.2 fix) still in `docker-compose.yml`.
|
||||
- `pg-boss` worker registry includes the `withings-resume` queue is N/A —
|
||||
the route is synchronous (no pg-boss work; just calls
|
||||
`resumeIntegrationFromPark` directly).
|
||||
- No new `as X` type-narrowing casts past Zod boundaries except the
|
||||
documented `parsed.data as unknown as AuthenticationResponseJSON`
|
||||
double-cast (intentional: SimpleWebAuthn types don't admit Zod's
|
||||
`passthrough`-loose-shape).
|
||||
- All 35 W6 routes still set `Content-Type: application/json` via the
|
||||
shared `apiError` / `returnAllZodIssues` helpers (NextResponse.json
|
||||
default).
|
||||
- The `withings/resume` endpoint enforces `requireAuth()` → user-scoped
|
||||
by `user.id` → no cross-user resume possible.
|
||||
|
||||
## Sign-off
|
||||
|
||||
Verdict: **APPROVE WITH 1 MEDIUM + 4 LOW**. Recommend addressing M-1 in
|
||||
v1.4.44 before the legacy `consecutive_failures` column drops. The four
|
||||
Lows are deferable — none affect v1.4.43 production behaviour.
|
||||
|
||||
Critical / High count: **0**.
|
||||
@@ -0,0 +1,218 @@
|
||||
# Round v1.4.43 — W10-QA-SIMPLIFIER findings
|
||||
|
||||
**Reviewer:** W10-QA-SIMPLIFIER (read-only)
|
||||
**Scope:** `git diff 2c68a48d..develop` (74 commits, 197 files, +12,301 / −460)
|
||||
**Date:** 2026-05-21
|
||||
|
||||
## Verdict
|
||||
|
||||
CHANGES REQUESTED — one Critical regression (relative-time i18n keys reverted across all six locales) and one High dead-code branch (IntegrationStatusPill `warning` state never emitted) must land before tag. Five Medium and four Low items are cleanup polish.
|
||||
|
||||
## Counts
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| Critical | 1 |
|
||||
| High | 2 |
|
||||
| Medium | 5 |
|
||||
| Low | 4 |
|
||||
|
||||
---
|
||||
|
||||
## Critical — must fix before tag
|
||||
|
||||
### C-1. Relative-time i18n keys silently swept out by W12 tighten — every Insights surface now renders raw key strings
|
||||
|
||||
**Symptom:** `formatRelativeTime()` reads pluralised keys (`insights.relativeMinutesAgoOne` / `…Other`, `…HoursAgoOne` / `…Other`, `…DaysAgoOne` / `…Other`) — none of those keys exist in any of `messages/{de,en,es,fr,it,pl}.json`. The translator returns the raw key on a miss (see `src/lib/i18n/context.tsx:104-107`), so every relative-time render now displays the literal string `insights.relativeMinutesAgoOther` (or its variants) to users.
|
||||
|
||||
**Root cause:** commit `b8ca1c74` (W4 — `i18n(relative-time): branch on count === 1 to render singular forms`) correctly added the pluralised keys to all six locales and switched `formatRelativeTime` to read them. Commit `2390438e` (W4 — `i18n: tighten 404 + global-error copy across all locales`) then reverted the locale-file half of that change — the relative-* diff in `2390438e` shows `-relativeMinutesAgoOne / -relativeMinutesAgoOther / -relativeHoursAgoOne / -relativeHoursAgoOther / -relativeDaysAgoOne / -relativeDaysAgoOther` deleted and the legacy `relativeMinutesAgo / relativeHoursAgo / relativeDaysAgo` keys re-added. The code in `relative-time.ts` was not reverted, so it now reads non-existent keys.
|
||||
|
||||
**Confirmed missing:**
|
||||
```
|
||||
grep -c "relativeMinutesAgoOne\|relativeMinutesAgoOther\|…" messages/*.json
|
||||
de.json:0 en.json:0 es.json:0 fr.json:0 it.json:0 pl.json:0
|
||||
```
|
||||
|
||||
**Visible call sites (all three are user-facing):**
|
||||
- `src/components/insights/coach-panel/history-rail.tsx:146` — Coach conversation history rail
|
||||
- `src/components/insights/daily-briefing.tsx:323` — Daily Briefing "generated X ago" caption
|
||||
- `src/components/insights/hero-strip.tsx:135` — Insights hero "Generated {time}" caption
|
||||
|
||||
**Files involved:**
|
||||
- `src/lib/i18n/relative-time.ts:26-31, 35-40, 43-48` — code reads the missing keys
|
||||
- `messages/{de,en,es,fr,it,pl}.json:1153-1156` — legacy keys present, pluralised keys absent
|
||||
|
||||
**Suggested fix:** restore the six pluralised keys per locale and keep the legacy keys as a transient compatibility shim, OR revert `relative-time.ts` to the legacy (non-pluralised) key reads + accept the "vor 1 Minuten" grammar drift the W4 plural commit was trying to fix. Decision is Marc's; either way the deploy cannot ship with `formatRelativeTime` reading keys that the i18n bundle does not contain. Add a guard test that asserts `t("insights.relativeMinutesAgoOne") !== "insights.relativeMinutesAgoOne"` so a future locale-file sweep cannot regress this silently.
|
||||
|
||||
---
|
||||
|
||||
## High — fix before tag if possible
|
||||
|
||||
### H-1. `IntegrationStatusPill` `warning` state defined and rendered but never selected — dead branch
|
||||
|
||||
**Symptom:** `IntegrationStatusPill` was widened in v1.4.43 W14 from three pill states to five (`connected`, `warning`, `error`, `parked`, `disconnected`). The `warning` branch carries its own copy (`warningServerError` i18n key, all six locales), its own chip class (`warningChipClass`), and its own switch case (`integration-status-pill.tsx:125-128`). No production code path ever passes `state="warning"`.
|
||||
|
||||
**Files involved:**
|
||||
- `src/components/settings/integration-status-pill.tsx:47-52` — `IntegrationPillState` union includes `"warning"`
|
||||
- `src/components/settings/integration-status-pill.tsx:108, 125-128, 157, 167` — render branches for `"warning"`
|
||||
- `src/components/settings/integrations-section.tsx:127-142` — `pillStateFor()` switches over `IntegrationState` and maps to `connected | error | parked | disconnected`; never returns `"warning"`
|
||||
|
||||
**Evidence (grep across the tree):**
|
||||
```
|
||||
grep "state=\"warning\"\|state: \"warning\"\|return \"warning\"" src/
|
||||
# only hits inside integration-status-pill.tsx (the dead branch itself)
|
||||
```
|
||||
|
||||
**Suggested fix:** either (a) decide what `warning` is supposed to surface (one transient failure? one persistent without the 24h park?) and wire `pillStateFor()` to return it, OR (b) drop the `warning` branch from the union + remove the chip class + delete the `warningServerError` keys across all six locales. As-is the branch ships dead — knip won't catch this because the type is referenced through the union — and the locale-file `warningServerError` key is dead in every bundle.
|
||||
|
||||
### H-2. `chart-overlay-prefs` 422 audit row has no dedup — sibling of `widgets` (B2) but the dedup was not applied here
|
||||
|
||||
**Symptom:** v1.4.43 B2 added an in-process 60 s `(userId, action)` dedup memo to `src/app/api/dashboard/widgets/route.ts` so a misbehaving client looping on a 422 cannot flood the audit ledger. The sibling route `src/app/api/dashboard/chart-overlay-prefs/route.ts` was migrated to `returnAllZodIssues` in the same release (W6) and now writes the same kind of breadcrumb on every validation failure — without dedup. A retry loop against `/api/dashboard/chart-overlay-prefs` pumps every 422 into `AuditLog`.
|
||||
|
||||
**Files involved:**
|
||||
- `src/app/api/dashboard/chart-overlay-prefs/route.ts:57-77` — audit row write, no dedup gate
|
||||
- `src/app/api/dashboard/widgets/route.ts:45-79` — dedup gate that should be applied here too
|
||||
|
||||
**Suggested fix:** lift the `auditDedupMemo` + `shouldEmitAuditRow` helpers into a shared module (e.g. `src/lib/audit-dedup.ts`) keyed by `(userId, action)` and have both routes call it. Drop the `__resetAuditDedupMemoForTests` from the widgets route, export the reset helper from the shared module instead.
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### M-1. `formatDateOrRelative` and `formatRelativeTime` are 90 % duplicate logic with subtly divergent i18n keys
|
||||
|
||||
**Symptom:** v1.4.43 added `formatDateOrRelative` to `src/lib/format.ts:73-122` while leaving `formatRelativeTime` in `src/lib/i18n/relative-time.ts:16-49` intact. Both compute relative phrases for "just now / N min ago / N h ago", branch on the same time buckets, and live behind a `t()` translator. The only differences:
|
||||
- `formatRelativeTime` also handles the ≥ 24 h "N days ago" bucket
|
||||
- `formatDateOrRelative` falls back to `formatDateTime(iso)` after 24 h instead of "N days ago"
|
||||
- `formatRelativeTime` reads the pluralised `…One` / `…Other` keys (Critical C-1)
|
||||
- `formatDateOrRelative` reads the non-pluralised legacy keys (`relativeMinutesAgo` / `relativeHoursAgo`)
|
||||
|
||||
The two helpers ship together in the same release with deliberately different i18n contracts, and the doc comment on `formatDateOrRelative` (`src/lib/format.ts:90`) even claims it "reuses the `insights.relative*` i18n keys (same keys `formatRelativeTime()` reads)" — which is no longer true after the W4 plural commit.
|
||||
|
||||
**Files involved:**
|
||||
- `src/lib/format.ts:73-122` — `formatDateOrRelative`
|
||||
- `src/lib/i18n/relative-time.ts:16-49` — `formatRelativeTime`
|
||||
|
||||
**Suggested fix:** collapse into one helper that takes a `{ daysAsRelative: boolean }` option (or two thin wrappers around a shared core). Whichever pluralisation contract wins from C-1 should be the only one in the codebase.
|
||||
|
||||
### M-2. `WMY_FANOUT_CONCURRENCY` + `computeAvg30LastYearMap` exports exist only for tests
|
||||
|
||||
**Symptom:** v1.4.43 widened these two from module-private to `export` so a new concurrency test could pin the 4-cap (`src/lib/analytics/__tests__/summaries-slice.wmy-cap.test.ts`). Neither is consumed by production code outside `summaries-slice.ts` itself.
|
||||
|
||||
**Files involved:**
|
||||
- `src/lib/analytics/summaries-slice.ts:744, 747` — `export const WMY_FANOUT_CONCURRENCY = 4;` + `export async function computeAvg30LastYearMap`
|
||||
- `src/lib/analytics/__tests__/summaries-slice.wmy-cap.test.ts:26-27` — sole consumer
|
||||
|
||||
**Suggested fix:** if `knip` is set to enforcing mode (it is in v1.4.42 W-KNIP), `knip` may not flag test-only exports — verify CI doesn't break, then add an `// @internal — exported for the WMY-cap unit test only` comment so a future contributor doesn't reuse them by accident. Alternatively use `vi.spyOn(module, "computeAvg30LastYearMap")` against the private symbol via `vi.importActual` — same outcome, no public-surface widening.
|
||||
|
||||
### M-3. `__test_SECTION_ORDER` duplicates the private `SECTION_ORDER` instead of exporting it
|
||||
|
||||
**Symptom:** `src/components/doctor-report/doctor-report-dialog.tsx:513-521` exports a hand-duplicated array `__test_SECTION_ORDER` for an SSR component test, while `SECTION_ORDER` at line 65 is private. Two arrays that must stay in lockstep with no compile-time link.
|
||||
|
||||
**Files involved:**
|
||||
- `src/components/doctor-report/doctor-report-dialog.tsx:65-73` — private `SECTION_ORDER`
|
||||
- `src/components/doctor-report/doctor-report-dialog.tsx:513-521` — exported duplicate `__test_SECTION_ORDER`
|
||||
|
||||
**Suggested fix:** export `SECTION_ORDER` directly and have the test import it. The `__test_` prefix is fine documentation that it is also test-touched, but the duplicate value is regression bait.
|
||||
|
||||
### M-4. `withings/resume/route.ts` POST handler takes `request` then `void request;`s it
|
||||
|
||||
**Symptom:** `src/app/api/integrations/withings/resume/route.ts:35-58` accepts `request: NextRequest` and immediately calls `void request;`. The handler does not read the body, headers, IP, or URL — `requireAuth()` does its own request-scoping. The parameter exists only because `apiHandler` types it; rather than wrapping with a no-arg arrow + casting, the cleaner shape is to drop the parameter or rename it `_request`.
|
||||
|
||||
**Files involved:**
|
||||
- `src/app/api/integrations/withings/resume/route.ts:35` — `void request;`
|
||||
|
||||
**Suggested fix:** rename to `_request` (TS / lint-allowed unused-prefix) or check whether `apiHandler` accepts a 0-arg handler. Either drops the `void` magic incantation.
|
||||
|
||||
### M-5. `health-chart.tsx` stashes raw count on the data array via `Object.defineProperty` — unusual carrier
|
||||
|
||||
**Symptom:** `src/components/charts/health-chart.tsx:691-697` writes a non-enumerable `rawCount` property onto the returned data array, then `src/components/charts/health-chart.tsx:704-708` reads it back via a type-coerced cast. The data array is the value the `useQuery` cache stores, so the property has to survive React-Query's structural sharing pass (it does — non-enumerable properties aren't seen by the cloner — but the test surface for this is silent). Easier: return `{ data: ChartDataPoint[]; rawCount: number }` and adjust the one consumer.
|
||||
|
||||
**Files involved:**
|
||||
- `src/components/charts/health-chart.tsx:691-708` — `Object.defineProperty(allData, "rawCount", …)` + `useMemo` re-read
|
||||
|
||||
**Suggested fix:** switch the query function to return `{ data, rawCount }`; rename the `useQuery` destructure to `{ data: queryResult }` then read `queryResult?.data` and `queryResult?.rawCount`. Drops the non-enumerable trick and removes the `(data as ChartDataPoint[] & { rawCount?: number })` cast.
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### L-1. `scrollBehaviorForUser` not wired into the fourth caller it advertises
|
||||
|
||||
**Symptom:** `src/lib/motion.ts:1-3` claims to be the source of truth for "the four programmatic `scrollIntoView` / `scrollTo` call-sites in the app", but `src/components/onboarding/WelcomeCarousel.tsx:122-126` still inlines its own `window.matchMedia("(prefers-reduced-motion: reduce)").matches ? "auto" : "smooth"` check. Three of the advertised four call-sites use the helper; one was missed.
|
||||
|
||||
**Files involved:**
|
||||
- `src/lib/motion.ts:1-3` — comment promises "the four call-sites"
|
||||
- `src/components/onboarding/WelcomeCarousel.tsx:122-126` — inline copy not switched over
|
||||
|
||||
**Suggested fix:** wire `WelcomeCarousel.tsx` through `scrollBehaviorForUser()`, or amend the motion.ts header comment to "three call-sites".
|
||||
|
||||
### L-2. `audit.coach.replay-injection` audit action name is anomalous (no other audit action uses the `audit.` prefix)
|
||||
|
||||
**Symptom:** Every other audit action name in the codebase reads `<surface>.<verb>` (e.g. `admin.backups.run`, `auth.login.failed`, `integrations.parked`, `integrations.resumed`). The new replay-injection guard writes `audit.coach.replay-injection` (with a `.` separator on `replay-injection` to boot), which both leaks the storage layer's name and breaks the dot-separator convention.
|
||||
|
||||
**Files involved:**
|
||||
- `src/app/api/insights/chat/route.ts:217` — `await auditLog("audit.coach.replay-injection", { … });`
|
||||
|
||||
**Suggested fix:** rename to `coach.replay.detected` (matches `auth.login.failed` shape) or `insights.coach.replay_injection` (matches the `annotate()` action name two lines above it). Either drops the `audit.` prefix duplication.
|
||||
|
||||
### L-3. `OfflineBanner` uses `queueMicrotask` as an escape hatch for a strict lint rule — `useSyncExternalStore` is the documented pattern
|
||||
|
||||
**Symptom:** `src/components/layout/offline-banner.tsx:52-56` wraps the initial `navigator.onLine` read inside a `queueMicrotask` "because the legacy in-effect setState path tripped the strict lint rule". This works but `useSyncExternalStore` is the idiomatic shape for "subscribe to an external boolean signal during render" — no microtask defer, no hydration mismatch, no comment needed.
|
||||
|
||||
**Files involved:**
|
||||
- `src/components/layout/offline-banner.tsx:25-61` — useState + useEffect + queueMicrotask shape
|
||||
|
||||
**Suggested fix:** rewrite as `useSyncExternalStore` with subscribe = add/remove online/offline listeners, getSnapshot = `navigator.onLine`, getServerSnapshot = `true`. Drops both the `queueMicrotask` ceremony and the SSR-stability comment.
|
||||
|
||||
### L-4. `Object.defineProperty(allData, "rawCount", {writable: false})` + `mood-chart.tsx` per-day reduction are inconsistent — one chart smuggles count via the array, the other computes from raw data
|
||||
|
||||
**Symptom:** v1.4.43 W2-CHART-GATE applied the "split copy on raw count vs distinct days" fix across three charts:
|
||||
- `health-chart.tsx` smuggles `rawCount` via `Object.defineProperty` (M-5 above)
|
||||
- `medication-compliance-chart.tsx:255-269` computes from `data.reduce((acc, p) => acc + p.scheduled, 0)` (clean)
|
||||
- `mood-chart.tsx` (verify) likely matches the compliance pattern
|
||||
|
||||
The three implementations should share one shape. The compliance shape is cleaner; the health-chart shape is the outlier.
|
||||
|
||||
**Files involved:**
|
||||
- `src/components/charts/health-chart.tsx:691-708`
|
||||
- `src/components/charts/medication-compliance-chart.tsx:255-269`
|
||||
- `src/components/charts/mood-chart.tsx` (verify same)
|
||||
|
||||
**Suggested fix:** rework `health-chart.tsx` to compute `rawCount` from the same `.reduce` shape (sum across daily aggregates' `count` field) after the query resolves, instead of stashing it on the array during the fetch.
|
||||
|
||||
---
|
||||
|
||||
## Out of scope / not flagged
|
||||
|
||||
- `formatActiveLocale` widening in `src/lib/format.ts:9-19` (legitimate v1.4.43 QoL — sub-locale users now see proper locale formatters)
|
||||
- `recordWithingsSyncFailure` export change (`src/lib/withings/sync.ts:290`) — needed to let `sync-activity.ts` + `sync-sleep.ts` route through the typed classifier (W7-B3); not over-engineering
|
||||
- `WithingsApiError` 1024-char message cap (`src/lib/withings/response-classifier.ts:248-254`) — defence-in-depth, appropriate
|
||||
- `checkAuthSurfaceRateLimit` (`src/lib/rate-limit.ts:104-127`) — clean abstraction over a real attack surface (trust-violation bucket exhaustion)
|
||||
- `IntegrationStatus.consecutiveFailuresByKind` JSONB column + 24 h park flip — appropriate complexity for the contract-mismatch sustain problem
|
||||
- `scripts/generate-sw-version.mjs` + `prebuild` — closes the v1.4.38.4 → v1.4.42 stale-CACHE_VERSION drift; correct shape
|
||||
- Dockerfile `NEXT_PUBLIC_APP_VERSION` build-arg — closes the v1.4.42 stale `package.json` cache regression; correct shape
|
||||
- W6 `returnAllZodIssues` rollout — straight-line API consistency
|
||||
- Chart skeleton + dashboard tile-strip skeleton additions — appropriate UX polish
|
||||
- All `motion-reduce:animate-none` additions + the textual-scan coverage test — appropriate accessibility floor
|
||||
- All test additions are appropriate scaffolding; none pin temporary scaffolding instead of release-quality contracts
|
||||
|
||||
---
|
||||
|
||||
## Relevant files
|
||||
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/i18n/relative-time.ts` (C-1)
|
||||
- `/Users/marc/Projects/HealthLog/messages/{de,en,es,fr,it,pl}.json` (C-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/settings/integration-status-pill.tsx` (H-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/settings/integrations-section.tsx` (H-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/chart-overlay-prefs/route.ts` (H-2)
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/widgets/route.ts` (H-2 reference)
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/format.ts` (M-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/analytics/summaries-slice.ts` (M-2)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/doctor-report/doctor-report-dialog.tsx` (M-3)
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/integrations/withings/resume/route.ts` (M-4)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/charts/health-chart.tsx` (M-5, L-4)
|
||||
- `/Users/marc/Projects/HealthLog/src/lib/motion.ts` (L-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/onboarding/WelcomeCarousel.tsx` (L-1)
|
||||
- `/Users/marc/Projects/HealthLog/src/app/api/insights/chat/route.ts` (L-2)
|
||||
- `/Users/marc/Projects/HealthLog/src/components/layout/offline-banner.tsx` (L-3)
|
||||
Reference in new issue
Block a user