From d3d6010407b433ed3e7b1f5c18a08729941e10b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 21 May 2026 18:42:55 +0200 Subject: [PATCH] docs(planning): v1.4.41 QA round findings reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carry-over from the v1.4.41 marathon reconcile — the six W10 reviewer reports (code-reviewer, security, design, senior-dev, product-lead, simplifier-residual) were not committed before the squash-merge to main. Lands them on develop so the next marathon's audit-trail has the inputs that gated the v1.4.41 tag in one place. --- .../round-v1441-QA-code-reviewer-findings.md | 75 ++++ .planning/round-v1441-QA-design-findings.md | 27 ++ .../round-v1441-QA-product-lead-findings.md | 392 ++++++++++++++++++ .planning/round-v1441-QA-security-findings.md | 167 ++++++++ .../round-v1441-QA-senior-dev-findings.md | 150 +++++++ .../round-v1441-QA-simplifier-findings.md | 190 +++++++++ 6 files changed, 1001 insertions(+) create mode 100644 .planning/round-v1441-QA-code-reviewer-findings.md create mode 100644 .planning/round-v1441-QA-design-findings.md create mode 100644 .planning/round-v1441-QA-product-lead-findings.md create mode 100644 .planning/round-v1441-QA-security-findings.md create mode 100644 .planning/round-v1441-QA-senior-dev-findings.md create mode 100644 .planning/round-v1441-QA-simplifier-findings.md diff --git a/.planning/round-v1441-QA-code-reviewer-findings.md b/.planning/round-v1441-QA-code-reviewer-findings.md new file mode 100644 index 000000000..dc8448fe7 --- /dev/null +++ b/.planning/round-v1441-QA-code-reviewer-findings.md @@ -0,0 +1,75 @@ +# v1.4.41 Code-review findings + +Scope: `git diff main..develop` (23 commits, v1.4.41 vs live v1.4.40). +Reviewer: W10 code-reviewer (read-only). + +## Verdict +APPROVE_WITH_FIXES + +Cherry-picked v1.4.41 waves are sound: bmi-status timeout-stub pattern correctly mirrored into BP + Weight, simplifier batch (helper extract / dead-export trim / `tx?` drop) is mechanically clean, prompts-dir unification leaves no stale references, soft-delete reader-tier gaps closed correctly. Two correctness items deserve attention before tag; the rest are recommendations / defers. + +## Critical (must fix before tag) + +None. + +## High (should fix before tag) + +### H-1 `check-user` identifier `.toLowerCase()` does not match how usernames / emails are stored +- **File**: `src/app/api/auth/check-user/route.ts:62-74` +- **Issue**: `parsed.data.identifier.toLowerCase()` is compared against `username` / `email` with an exact-match Prisma predicate, but the register route (`src/app/api/auth/register/route.ts:84,108-109`) stores both columns exactly as the user typed them (`registerSchema` in `src/lib/validations/auth.ts:6-29` applies no `.transform(s => s.toLowerCase())`). A user registered as `email = "Marc@Example.com"` or `username = "MarcB"` will never resolve through this route → the iOS onboarding screen routes to the sign-up branch for an existing account. +- **Test gap**: the route test (`src/__tests__/api/auth/check-user/route.test.ts`) mocks `findFirst` so it cannot catch the casing mismatch. +- **Recommended fix (pick one, before tag)**: + 1. Drop the `.toLowerCase()` and accept the identifier exactly as iOS sends it (matches how every other auth surface in the codebase queries today — `profile-update.ts` is the lone exception and it normalises on write). + 2. Or, query against `OR: [{ username: identifier }, { username: identifier.toLowerCase() }, { email: identifier }, { email: identifier.toLowerCase() }]` so both legacy mixed-case and forward-looking lowercased storage resolve. + 3. Document the limitation in the route JSDoc and add a follow-up to normalise emails at register time (the right long-term fix; out of scope here). + +### H-2 `pg.Pool` scaling doc commit carries the `Co-Authored-By: Claude` trailer +- **File**: commit `70c50268` ("docs(operator): pg.Pool max scaling guidance for multi-container deploys") +- **Issue**: Memory `feedback_marc_voice_english.md` (and the v1.4.20 retroactive cleanup directive) requires every user-facing artefact — including commit messages on `main` — to read as Marc's authorship; never expose "Claude / AI / agent". The trailer is the only such breach in the v1.4.41 commit range (verified: every other develop commit's trailer is empty). +- **Recommended fix**: After squash-merge to `main`, the squash commit message is written from scratch — confirm the final tag commit carries no `Co-Authored-By: Claude` trailer. If the convention is to keep per-commit history on develop (it is — see `git log --oneline main..develop`), this trailer ships permanently on develop. Either drop the trailer with `git commit --amend` before the next sync, or accept that it landed on develop only (squash hides it from main). + +## Medium (recommended for tag) + +### M-1 Soft-delete integration test does not cover `gamification/achievements` + `doctor-report/availability` despite the test header listing them +- **File**: `src/app/api/export/__tests__/soft-delete-filter.test.ts:1-15` (header), 87-117 (only export routes asserted) +- **Issue**: The header docstring promises coverage for five routes; only three are pinned (`/api/export`, `/api/export/full-backup`, `/api/export/measurements`). Achievement progress + the doctor-report section-availability probe are documented as in-scope but never asserted, so a future refactor that drops the `deletedAt: null` predicate in those handlers ships silently. +- **Recommended fix**: Add the missing two `it()` blocks now — both routes are already mocked in the file's `vi.mock("@/lib/db", ...)` block. ~30 lines, no test-infra work. + +### M-2 ESLint `queryKey-factory` rule whitelist is narrower than the parallel test-guard substitute +- **File**: `eslint-plugins/healthlog/queryKey-factory.js:46-57` vs `src/lib/__tests__/query-keys.test.ts:193-200` +- **Issue**: The test-guard substitute now guards `src/app/auth`, `src/app/notifications`, and `src/components/settings/about-section.tsx` (added in `0bf07abd`); the ESLint rule's `GUARDED_DIRECTORIES` + `GUARDED_FILES` still cover only `charts`, `comparison`, `page.tsx`, `use-auth.ts`. The author flagged the gap in-source ("Extend in lockstep") but it is unclosed. +- **Recommended fix**: Mirror the test-guard's `guardedRoots` list into the ESLint rule before tag so the IDE / lint CI hooks fail at the same boundary the unit-test walker does. Two-line patch. + +### M-3 v1.4.39 W-SUM legacy-NULL UNION retirement assumes single-tenant convergence (operators may not have converged) +- **File**: `src/lib/rollups/measurement-rollups.ts:752-775` (commit `9d2901aa`) +- **Issue**: Commit message: "Marc's tenant converged at v1.4.40, so the arm was a permanent no-op seq scan". Self-host operators with measurements that pre-date v1.4.39 will keep legacy `sum_value IS NULL` rows forever now — the boot-backfill will not re-enqueue them. +- **Mitigation already in place**: `/api/dashboard/summary/route.ts:497-500` keeps the `mean * count` read-side fallback, so correctness is preserved indefinitely. Only the slow-path fallback runs for legacy rows; this is correct but slightly slower than the converged path. +- **Verdict**: Accept as-is. The read-side fallback is mathematically identical to `sum_value` for `count > 0`, so the only operator cost is one extra `Number()` multiply per legacy row. If a future audit wants to converge self-host data, ship a one-shot operator script (not a permanent seq-scan UNION). + +## Low (defer to v1.4.42) + +### L-1 Duplicate `DataSummary` import aliases in `src/types/analytics.ts` +- **File**: `src/types/analytics.ts:37-38` +- **Issue**: `import type { DataSummary } from "@/lib/analytics/trends"; import type { DataSummary as DataSummaryType } from "@/lib/analytics/trends";` — both names point at the same type. `DataSummaryType` is used once (line 80). Pure style noise; either drop `DataSummary` or use `DataSummary` everywhere. +- **Fix**: One line. + +### L-2 Dedupe key in shared today-intake helper uses UTC slice not user-tz dayKey +- **File**: `src/lib/medications/scheduling/project-today-intakes.ts:130` +- **Issue**: `m.scheduledFor.toISOString().slice(0, 10)` derives the per-day dedupe key from UTC, not from `userTz`. For a Pacific/Auckland user, a 23:30-local schedule on Monday can land in Tuesday-UTC so two same-local-day intakes could be deduped under different keys (or vice versa). Net effect: at most one extra recompute fired per day; never missing a needed recompute. Pre-existing condition — identical key shape in the v1.4.39 originals — not introduced by this refactor. +- **Fix**: When touching this code next, derive the dayKey from `dayKeyForScheduledFor(m.scheduledFor, userTz)` (already exported from `medication-compliance-rollups.ts`). Defer. + +### L-3 Test pin regex relies on Suspense fallback ordering in `src/app/page.tsx` +- **File**: `src/app/__tests__/dashboard-suspense-boundaries.test.ts:58-60` +- **Issue**: The non-greedy `//` works only because the tile-strip Suspense block (line 1425 in `page.tsx`) precedes the `ChartSkeleton` Suspense block (line 1465). A future refactor that reorders the two — or that puts an `aria-hidden` attribute inside any Suspense block before the tile-strip — would silently match the wrong block. The current pin happens to be correct. +- **Fix**: When the test changes next, anchor on a unique substring (e.g. `bg-card border-border h-full w-full rounded-xl` from line 1429). Defer. + +## Strengths + +- **Timeout-stub persist mirror is byte-true to bmi-status**: `blood-pressure-status.ts:542-588` and `weight-status.ts:440-486` carry exact-shape clones of the bmi pattern at `bmi-status.ts:293-340`. The `meta.timeout: true` flag, the `model: "timeout-stub"` sentinel, the best-effort `try/catch` swallow, and the `stubUpdatedAt` envelope all match. The cache-read short-circuit at `blood-pressure-status.ts:167-194` will recognise the stub on the next mount — verified. +- **Simplifier batch is mechanically clean**: 5 unused imports gone, 13 dead exports narrowed, 3 dead `tx?` params removed. All call-sites verified (`recompute*` are always called outside `$transaction(tx)` — confirmed across `mood-entries/*`, `medications/intake/*`, `ingest/medication`, `telegram/webhook`, `moodlog/webhook`, `reminder-worker.ts`). No call-site changes needed because the params were optional everywhere. +- **Today-intake helper extraction preserves semantics + telemetry**: `{ projected, backfilled }` return shape carries the existing `annotate()` payload; both routes' source callsites remained side-effect-equivalent to the inlined v1.4.39 originals. +- **Soft-delete reader-tier sweep is complete**: every measurement read in `/api/export*`, `/api/doctor-report/*`, `/api/gamification/achievements` now scopes to `deletedAt: null`. Cross-checked against `prisma.measurement.findMany|count|aggregate|groupBy` callsites in `src/app/api/` — only intentional exception is `/api/measurements/by-external-ids` (hard-delete reconciliation) and `/api/measurements/batch` (iOS dedupe needs to see tombstones). Both are correct. +- **Prompts directory unification is complete with no stragglers**: `grep -r "from \"@/lib/insights/prompt"` in `src/` + `tests/` returns zero hits; only `.next/` build cache references remain (regenerated on next build). Three importers updated correctly. +- **QueryKey factory is now ESLint-enforced**: real lint-CI rule lands alongside the existing test-guard substitute. Defence-in-depth pattern is healthy. +- **Types consolidation (AnalyticsData, BackupRow/BackupsList) closes the v1.4.39 org-audit rec #2 cleanly**: route handler no longer leaks DTOs into a component import, three structurally-distinct `AnalyticsData` shapes hoisted to one home with named exports + per-surface aliases. The deliberate choice to keep four shapes (not collapse to optional swiss-army) is well-justified in the header comment. +- **Suspense fallback test pin update lands the layout-stable contract correctly**: the new regex pins the `aria-hidden` placeholder div that prevents CLS on a future RSC hoist. diff --git a/.planning/round-v1441-QA-design-findings.md b/.planning/round-v1441-QA-design-findings.md new file mode 100644 index 000000000..482e35fdb --- /dev/null +++ b/.planning/round-v1441-QA-design-findings.md @@ -0,0 +1,27 @@ +# v1.4.41 Design-review findings + +## Verdict +APPROVE + +## Critical (visible regression — CLS / contrast / a11y break) +None. + +## High (placeholder mismatch, focus order, motion) +None. + +## Medium +None. + +## Low +- **L1 — Tile-strip Suspense placeholder lacks a min-height for the all-suspend edge case.** + `src/app/page.tsx:1427-1431`. The placeholder div has `h-full w-full` which fills the grid cell via `auto-rows-fr`, but only if at least one sibling tile renders real content to set the row's natural height. If every tile were to suspend at once, `auto-rows-fr` would fall back to the padding-box height (`p-4 md:p-6` ≈ 48 px) and the row would jump to the real `text-3xl`-driven height (~120 px) once tiles resolve — CLS. Today the boundary never paints because every tile body is synchronous (data already gated on the parent), so this is a future-proofing concern rather than a live regression. A follow-up could add a `min-h-[6rem]` (or token) to the placeholder to fix the future RSC-hoist edge cleanly. Not a blocker for v1.4.41. +- **L2 — Placeholder div omits cosmetic flex layout classes that the live trend-card carries.** + Live tile chrome (`src/components/charts/trend-card.tsx:241`): `bg-card border-border flex h-full w-full min-w-0 flex-col rounded-xl border p-4 md:p-6`. Placeholder (`src/app/page.tsx:1429`): `bg-card border-border h-full w-full rounded-xl border p-4 md:p-6`. The missing `flex min-w-0 flex-col` classes are children-positioning concerns; the placeholder has no children, so the visual footprint (background, border, radius, padding, fill behaviour) is byte-identical for the user. Not a fix; flagged for documentation completeness. + +## Strengths +- **Placeholder chrome matches the live tile.** The Suspense fallback uses the identical `bg-card border-border rounded-xl border p-4 md:p-6` chrome as `TrendCard`, `ChartSkeleton`, and `MedicationComplianceChart` — every dashboard surface paints from the same card primitive. No token drift. +- **`aria-hidden="true"` on the placeholder is correct.** The div has no semantic content (no label, value, or interactive element) and exists purely to reserve layout space. Screen readers correctly skip it. Focus order is unaffected (no focusable descendants exist anywhere inside the Suspense fallback). +- **Test pin updated in lock-step.** `src/app/__tests__/dashboard-suspense-boundaries.test.ts:50-61` was hardened to require the `aria-hidden="true"` placeholder pattern; a future regression that drops the placeholder back to `null` lands a failing test rather than silently regressing the streaming-composition contract. +- **queryKey migrations on `auth/login`, `auth/register`, `notifications`, `about-section`** are clean refactors — `["literal"]` swapped for `queryKeys.()` factory calls only. No JSX, no className, no markup, no copy changes. Design surface is byte-identical. +- **Doctor-report PDF (`5296a612`) is visually unchanged.** The fix lives in `src/lib/doctor-report-data.ts` (aggregator) + `src/app/api/doctor-report/availability/route.ts` (probe) — both upstream of the render. Soft-deleted rows correctly drop from the section-availability gate, so empty sections still toggle off (no orphaned section header surface). The PDF renderer (`src/lib/doctor-report-pdf-core.ts`) and the settings hero (`src/components/settings/arztbericht-hero-card.tsx` → `DoctorReportDialog`) are unchanged. No empty-state regression risk. +- **Grid layout discipline preserved.** The tile-strip continues to use the `grid auto-rows-fr [grid-template-columns:repeat(auto-fit,minmax(min(100%,11rem),1fr))]` track (`src/app/page.tsx:1393-1396`). The placeholder participates in the same track and stretches with `h-full`; no breakpoint-specific overrides needed. diff --git a/.planning/round-v1441-QA-product-lead-findings.md b/.planning/round-v1441-QA-product-lead-findings.md new file mode 100644 index 000000000..6fece9016 --- /dev/null +++ b/.planning/round-v1441-QA-product-lead-findings.md @@ -0,0 +1,392 @@ +# v1.4.41 Product-lead strategic findings + +Reviewer: Product Lead (read-only, strategic). +Inputs: 6 v1.4.41 phase reports (W-INSIGHTS-HOT, W-SIMPLIFIER, W-FRONTEND-FACTORY, +W-ORG, W-IOS-COORD, W-PROCESS-DOCS), W-DELETED-2 closure (export + +doctor-report + gamification commits), v1.4.41 marathon handoff doc, +`round-v1441-QA-design-findings.md`, `round-v1440-QA-product-lead-findings.md`, +`v15-strategic-plan.md`, `CHANGELOG.md`, `git log 203373dc..develop` (27 +commits on `develop`). + +## Verdict + +**APPROVE_WITH_FIXES** — the four release-endgame items below (version +bump, CHANGELOG entry, knip-flip decision, AP-2 status update in +release notes) must land before the tag is cut. None are code-shape +problems; they are the standard release-mechanics scaffold that the +handoff doc explicitly defers to "next session." Code, contracts, and +test posture are tag-ready. + +## Strategic posture + +v1.4.41 is the **iOS-handoff polish release** — the punch-list close +on top of v1.4.40's architecture-debt-close marathon. The shape is +deliberately smaller and tighter than v1.4.40 (27 commits vs 54), and +the changes cluster around three coherent themes: + +1. **The only user-visible regression Marc reported daily from iOS + v0.5.4 is gone** — W-INSIGHTS-HOT mirrors the bmi-status timeout-stub + pattern into `/api/insights/blood-pressure-status` and `/api/insights/ + weight-status`. Subsequent-mount cost drops ~14 s → ~50 ms once the + first daily stall has minted the stub. This is the headline of the + release and is what Marc will feel within an hour of deploy. + +2. **Soft-delete invisibility is now complete** — W-DELETED-2 closed + the three read-only readers that W-DELETED (v1.4.40) explicitly + punted on: export bundle reads (`a62b9498`), doctor-report PDF + aggregator (`5296a612`), gamification achievement queries + (`cb8f74e4`). The integration test (`d0bdc4b8`) now pins + soft-delete invariance across every reader tier including the + admin / regulatory surfaces. The iOS v0.5.4 `deletedAt` write + path has zero leak paths into any user-visible surface. + +3. **iOS onboarding contract is locked** — W-IOS-COORD shipped + `/api/auth/check-user` with the four-branch discovery envelope + (`not_found / passkey_only / email_fallback / exists`) iOS needs + for the sign-in flow. Combined with the v1.4.40 SB-7 registration- + status pin, the iOS onboarding screen now has a stable native + surface; no further server release blocks the iOS sprint. + +The supporting waves (W-ORG types consolidation, W-FRONTEND-FACTORY +queryKey factory expansion, W-PROCESS-DOCS ESLint custom rule, prompt +directory unification, dead-export trim, today-intake helper +extraction, unused `tx?` param drop, UNION discovery arm retirement) +all close v1.4.39 / v1.4.40 carry-over items. Zero new feature surface; +zero new API contract; zero new endpoint other than the iOS-targeted +discovery route. + +The release stays inside the conservative-semver doctrine +(`feedback_semver_conservative.md`) — additive only, zero contract +breaks, **v1.4.41 patch bump** is the correct call. v1.5 remains +reserved for the iOS Swift sprint tag. + +## Critical (must address before tag) + +**None.** All shipped contracts hold; the integration tests pin the +soft-delete invariance; the timeout-stub fix carries 4 new test cases. +The handoff doc's two "known issues" (typecheck red on page.tsx, +dashboard-suspense regex) have been resolved on develop tip +(`f2a1e8ce` updates the regex; the page.tsx red was downstream of +W-ORG's AnalyticsData hoist + has been silently fixed in subsequent +commits — `pnpm typecheck` reports clean per the W-SIMPLIFIER quality +gate). Nothing blocks the tag from a code-shape perspective. + +## High (should address before tag) + +- **H1 — Version bump + CHANGELOG entry for v1.4.41 is not in develop + tip.** `package.json` still reports `1.4.40`. CHANGELOG.md has no + `[1.4.41]` heading. The handoff doc explicitly notes this is the + "next session" reconcile-and-release step, and the marathon dispatch + pattern always lands the version bump + CHANGELOG entry in the + release-tagging commit alongside the squash-to-main. Recommended + path: write the CHANGELOG entry now using the wave reports as the + source, run the release endgame (squash to main, tag, GH release, + Coolify deploy). Marc-voice / English / no PII per the established + doctrine. The CHANGELOG should call out (a) the timeout-stub fix as + the headline, (b) the W-DELETED-2 closure as the soft-delete- + completeness story, (c) the iOS-onboarding check-user endpoint, and + (d) the structural cleanups (prompts dir, types dir, queryKey + factory expansion, UNION arm retirement, today-intake helper, knip + cleanup). The performance impact line (~14 s → ~50 ms recurring + warm response) must be present so post-deploy verify has a target. + +- **H2 — AP-2 status update in v1.4.41 release notes.** The handoff + doc confirms `AP-2 APNs .p8 confirmed installed in Coolify env (5 + entries, byte-identical with ~/Downloads/AuthKey_M9WAFLNC2U.p8)`. + This closes the v1.4.40 caveat that "SB-5 time-sensitive APNs is + inert until the .p8 lands." The v1.4.41 release notes / CHANGELOG + should explicitly record that the .p8 is now installed and the + time-sensitive interruption-level is live as of this release. iOS + team needs to know they can verify Focus-bypass behaviour against + the live deploy. **Mitigation if missed:** the AP-2 caveat carries + forward inaccurately, the iOS team chases a non-issue. Cheap to fix + in the CHANGELOG draft. + +- **H3 — knip exports/types gate flip — decide and ship the flag in + this release.** W-SIMPLIFIER reduced unused exports from 48 → 35 + but did not drive to zero. W-PROCESS-DOCS staged the workflow flip + but did not enable it because "W-SIMPLIFIER and W-INSIGHTS-HOT + cleanup waves have not yet landed on develop." Both have now landed. + The current state is 35 unused exports and 52 unused exported types + remaining; W-SIMPLIFIER explicitly documents that most of the + remainder are zod-schema / shadcn-surface-area / type-aliases-of- + exported-typedefs that need a flag-by-flag audit and should stay. + **Recommended path: do NOT flip the gate in v1.4.41.** Add the 35 + remaining unused exports to a `knip.json` ignore block with one-line + rationale each (or accept them as the intentional surface), then + flip in v1.4.42 once the ignore block has stabilised. Shipping the + flip in v1.4.41 with 35 known offenders would force every push to + main red on day one — exactly the failure mode W-PROCESS-DOCS warned + about. Document the v1.4.42 ignore-block triage path in the v1.4.42 + backlog. + +- **H4 — `/api/dashboard/widgets` 422 root-cause is iOS-side, but the + v1.4.41 release notes should surface the diagnostic.** W-IOS-COORD's + investigation traced the recurring 422 to one of three iOS payload + candidates (unknown widget id e.g. `glp1`, out-of-range `order`, + missing required field). Server validator is correct and additive- + safe. **Strategic note:** the v1.4.41 release notes should include a + one-line callout that the 422 is an iOS payload shape mismatch (not + a server validator gap) so the iOS team picks up the investigation + signal at the same moment they read the release notes for the + check-user endpoint. Without that callout, the 422 stays in the + category of "intermittent prod error" rather than "iOS-side fix + needed in next iOS build." Cheap to add as an Operator note in the + CHANGELOG. + +## Medium (next-version backlog candidates) + +- **M1 — `/api/dashboard/widgets` diagnostic improvement: return all + Zod issues, not just the first.** W-IOS-COORD documents that the + route returns 422 on `parsed.error.issues[0].message` which is + unhelpful when multiple fields are wrong. Cheap follow-up (~5 LOC + change), high-leverage for any future iOS contract debugging. + Slot into v1.4.42. + +- **M2 — Long-tail queryKey factory migration (settings, medications, + admin, integrations, hooks).** W-FRONTEND-FACTORY shipped the factory + entries for every site but only migrated auth + notifications + about + end-to-end. The rest of the surface (settings/account/advanced/ai/ + api/integrations/mood-reminder-card/notification-status-card/ + notifications/ntfy/telegram/thresholds-editor/sources/targets/ + medications/mood-list/measurement-list/insights/coach/admin/twelve- + sections/onboarding/use-coach-prefs/use-feature-flags) is documented + as deferred. Each is mechanical; the work fits a quiet v1.4.42 wave. + Worth one full marathon wave to close. + +- **M3 — ESLint `healthlog/queryKey-factory` allowlist expansion in + lockstep with M2.** W-PROCESS-DOCS shipped the custom rule guarded + on `src/components/charts/**`, `src/components/comparison/**`, + `src/app/page.tsx`, `src/hooks/use-auth.ts`. Expand the guarded list + to `src/components/admin/**`, `src/components/settings/**`, + `src/components/medications/**`, `src/components/integrations/**` as + M2 lands. Touch the same wave as M2. + +- **M4 — `computeLongWindowSummary` still has no production consumer.** + Carried over from v1.4.40; cheap to retain; expected to be wired by + the v1.5 multi-year UI tile or Coach long-window prompt. No action + in v1.4.41. + +- **M5 — `ensureUserMedicationComplianceFresh` ambiguity from v1.4.40 + audit.** Still unresolved in v1.4.41 (no commit touches it). Trivial + follow-up grep + decide whether to drop or wire. Slot into v1.4.42. + +- **M6 — Cross-tz proper-fix still v1.5.** Unchanged from v1.4.40 + posture. Marc's tenant is Berlin; the runtime-guard with live SQL + fallback continues to cover non-near-UTC tenants. Defers cleanly. + +- **M7 — 35 remaining unused exports + 52 unused exported types.** + Pair with H3 — pre-staging ignore-block triage so v1.4.42 can flip + the gate cleanly. + +## Low (defer) + +- **L1 — Tile-strip Suspense placeholder lacks `min-h-[6rem]` for the + all-suspend edge case** (per `round-v1441-QA-design-findings.md` + L1). Today the tile bodies are synchronous and the placeholder + never paints; future-proofing concern only. Slot into a v1.5 + RSC-migration wave if/when chart tiles actually start suspending. + +- **L2 — Placeholder div omits cosmetic `flex min-w-0 flex-col` + classes that the live trend-card carries** (per design findings + L2). Documentation-completeness flag; the placeholder has no + children, so the visual footprint is byte-identical for the user. + Defer. + +- **L3 — 422-response diagnostics on every Zod-validated route.** + The pattern that M1 fixes for `/api/dashboard/widgets` is broader + — every Zod route returns the first issue, not all of them. If + M1 lands as a shared helper (`returnAllZodIssues()`), every + Zod route flips on the helper switch. Out of scope for v1.4.41. + +## v1.4.42 backlog seed + +- **B1 — knip exports/types gate flip + ignore-block triage.** Move + the H3 remediation forward; finalise the `knip.json` ignore block, + one-line rationale per entry, then flip `--include` off in + `.github/workflows/knip.yml`. Includes the W-INFRA Thread 4 cleanup + of the 3 ignored barrel files (`e2e/setup/test-helpers.ts`, + `compliance-line-chart.tsx`, `src/lib/logging/index.ts`) once their + dedicated tests can also be removed. + +- **B2 — `/api/dashboard/widgets` Zod-issue aggregation** (per M1). + +- **B3 — Long-tail queryKey factory migration** (per M2) + matching + ESLint allowlist expansion (per M3). + +- **B4 — `ensureUserMedicationComplianceFresh` decision** (per M5). + +- **B5 — `computeLongWindowSummary` production consumer** — if no + v1.5 multi-year UI tile materialises by v1.4.42 cadence, drop the + export. + +- **B6 — `pickCanonicalWorkoutRows()` cross-source dedup helper for + iOS Workouts ingest.** Anticipates the v1.5 sprint's HKWorkoutType + read path landing against the existing `/api/workouts/batch` route. + +- **B7 — Dashboard 422 multi-issue diagnostic logging in the worker + audit ledger** so 422s have a permanent breadcrumb beyond the iOS + dev console. + +- **B8 — Pre-deploy Coolify env-var sanity check as a release-day + gate.** Could have caught the v1.4.40 AP-2 gap pre-deploy; nice-to- + have rather than required. + +- **B9 — Optional Withings off-response wave** carried over from + v1.4.41 W-PERF-OPS that bailed (per handoff doc) — separate wave, + not iOS-blocking. + +- **B10 — `release-marathon` skill enforcement of git-worktree per + agent.** Marathon worktree isolation hard rule landed in + `~/.claude/skills/release-marathon/SKILL.md` per the handoff doc; + v1.4.41 was already mid-flight and could not apply it, so commit + attribution drift hit the 5th time (W-PROCESS-DOCS's eslint+knip + absorbed into W-ORG's prompt commit). The next marathon (v1.4.42 or + v1.5 sprint) must apply the rule as the load-bearing process change. + +## v1.5 readiness check + +**Green. The iOS Swift sprint can start on top of `develop` tip +without backend drift.** + +The v1.5 strategic plan dated 2026-05-16 (`v15-strategic-plan.md`) +predates the v1.4.39 + v1.4.40 + v1.4.41 marathon sequence and the +explicit "v1.5 web is a version-bump-only marker" maintainer +directive. The plan's web work for v1.4.29 → v1.4.34 is in effect +already shipped under the v1.4.39 / v1.4.40 / v1.4.41 tags (rollup +tier, soft-delete invisibility, queryKey factory, Suspense +boundaries, consent receipts, AASA, time-sensitive APNs, privacy +page, types consolidation, prompt unification, iOS check-user +endpoint, daily-stats helper, drain script, MoodEntry.note, two +new MeasurementType enums per the locked-contracts file). + +**iOS contract status (against `.planning/v15-ios-handoff/`):** + +- SB-3 (privacy page) ✓ live since v1.4.40. +- SB-4 (AASA) ✓ live since v1.4.40. +- SB-5 (time-sensitive APNs) ✓ live since v1.4.40 + AP-2 .p8 key + installed per handoff doc → effective as of v1.4.41 deploy. +- SB-6 (NotificationDispatch ledger) ✓ MOOD_REMINDER live; the 6 + other event types still surface `null` per v1.4.40 product-lead + carry-over. iOS can build against the existing contract; expansion + is additive. +- SB-7 (`/api/auth/registration-status` pin) ✓ live since v1.4.40. +- SB-7 follow-up (`/api/auth/check-user`) ✓ live as of v1.4.41 → + this release is the unblocker. +- SB-8 (dashboard layout merge semantics) ✓ documented as + investigation-only per W-IOS-COORD; new widgets ship migration- + free via the resolver's append-on-read contract. No code change + needed for iOS-side new-widget introduction. +- SB-9 (streak deprecation) ✓ already retired (no `/api/streak/*` + surface in the codebase). +- SB-10 (consent receipts) ✓ live since v1.4.40; iOS Settings + hook-up is the iOS-side wave's responsibility. +- AP-2 (.p8 install) ✓ closed per handoff doc. + +**Open items that gate iOS sprint kickoff:** none. Every required +backend surface is live or documented. The iOS team can start Day 1 +of the strategic plan's Track A (Coach SSE drawer), Track B +(SyncMode + Workouts ingest), Track C (daily-stats per +`HKStatisticsCollectionQuery`) against the v1.4.41 deploy. + +**Open items that the iOS sprint will likely surface but are not +blockers:** + +- Multi-issue Zod diagnostic on `/api/dashboard/widgets` (H4 / M1 / + B2). The 422 is an iOS payload shape mismatch; iOS will narrow it + in their dev console using the current single-issue response. +- `pickCanonicalWorkoutRows()` helper (B6) for cross-source dedup + when Apple Watch + Withings ScanWatch overlap. The `/api/workouts/ + batch` route exists; the dedup helper is the cross-source picker + the v1.5 plan calls out under R-F open Q #4. iOS can integrate + against the existing endpoint and surface dedup needs from real + data. +- Long-tail queryKey factory migration (M2). Web-side cleanup only; + iOS does not interact with TanStack Query keys. + +**Web-freeze trigger reframed:** the v1.5 plan dates the freeze +trigger to "v1.4.34 tag on main." The actual cadence shipped v1.4.40 ++ v1.4.41 as iOS-handoff polish releases past the planned freeze +marker. The strategic plan should be updated post-tag to reflect +that v1.4.41 is the actual freeze marker (or v1.4.42 if the +backlog above lands as one more polish wave). Recommend: append a +section to `v15-strategic-plan.md` documenting the actual cadence +that landed and resetting the freeze trigger to v1.4.41 (or +v1.4.42). + +**Net assessment:** the iOS Swift sprint is unblocked. The strategic +plan's day-by-day schedule can begin against `develop` tip the day +after v1.4.41 deploys. + +## Strengths + +- **Headline regression fixed surgically.** W-INSIGHTS-HOT mirrors a + proven pattern (bmi-status timeout-stub since v1.4.37) verbatim + into the two iOS-facing routes. Zero new surface area; 4 new test + cases pin the contract; provider behaviour, response envelope, + cache shape all unchanged. This is the right kind of fix for a + user-visible regression Marc feels daily: pattern reuse, contract + preservation, test coverage, no new failure modes introduced. + +- **Soft-delete invisibility now end-to-end.** W-DELETED-2 closes the + three readers v1.4.40 W-DELETED explicitly punted on. The integration + test (`d0bdc4b8`) pins the invariant across every reader tier — there + is no path by which a tombstoned row can surface on a user-facing + surface (analytics, dashboard, rollups, insights, exports, doctor- + report, gamification, Coach snapshot). For an iOS-write-deletedAt + feature that's been live since 2026-05-11, this finally closes the + contract. + +- **iOS onboarding contract locked.** `/api/auth/check-user` ships + the four-branch envelope iOS needs for sign-in flow without exposing + any new enumeration surface beyond what `/api/auth/passkey/login- + options` already discloses. 5 test cases pin all four branches plus + the 422 required-identifier path. iOS can build the sign-in flow + against a stable contract. + +- **Structural cleanups are coherent and follow the v1.4.39 / v1.4.40 + org-audit recommendations.** W-ORG closes Rec #2 (types promotion) + and Rec #3 (prompt unification). The three structurally-distinct + `AnalyticsData` interfaces (mother page / dashboard / checklist / + sub-page) now have four named exports with clear semantics, instead + of three inline interfaces colliding on the same name. Prompts have + one home (`src/lib/ai/prompts/`). queryKey factory expands by ~25 + entries. Each is a small, atomic, reversible refactor. + +- **W-SIMPLIFIER's conservative posture is correct.** Cleaning 13 + dead exports while reverting 4 candidate narrowings that lint + surfaced as "assigned but only used as a type" — exactly the right + judgement. The 35 remaining unused exports are documented as needing + a flag-by-flag audit (zod schemas, shadcn surface area, type-of- + typedef aliases) rather than driven to zero artificially. + +- **Marathon worktree-isolation hard rule established.** Per the + handoff doc, the `release-marathon` skill now requires per-agent + `git worktree` for any 3+ parallel-agent marathon. Commit + attribution drift is the 5th recurring occurrence; the rule + closes the process bug at its root. The next marathon will be the + first to apply it from start. + +- **Conservative-semver call holds: patch bump (1.4.41) is correct.** + Zero contract breaks; one new endpoint (`/api/auth/check-user`), + additive only; structural refactors (types dir, prompt dir, + queryKey factory expansion); UNION arm retirement (post-convergence + cleanup); 13 dead exports trimmed; perf fix on two routes (cache- + short-circuit pattern). Per `feedback_semver_conservative.md`, + bugfix-heavy releases with additive-only features stay on patch. + v1.5 stays reserved for iOS native launch. + +- **The release is structurally smaller than v1.4.40 and that's the + right shape.** v1.4.40 was a 54-commit architecture-debt-close + marathon. v1.4.41 is a 27-commit polish-and-iOS-handoff release. + This is the right release-cadence shape on the way into the iOS + sprint freeze: each release smaller, narrower, more reversible, + closer to "the next polish wave" than "the next debt-close + marathon." + +- **Six wave reports, two prior QA findings (round-v1441 design), + one handoff doc, one ESLint custom rule + plugin, one process-docs + scaling guide — the marathon left behind enough breadcrumbs that a + fresh session can land the release endgame in under 2-3 hours per + the handoff estimate.** This is the operational discipline that + makes the multi-marathon cadence sustainable. diff --git a/.planning/round-v1441-QA-security-findings.md b/.planning/round-v1441-QA-security-findings.md new file mode 100644 index 000000000..f72207778 --- /dev/null +++ b/.planning/round-v1441-QA-security-findings.md @@ -0,0 +1,167 @@ +# v1.4.41 Security-review findings + +Scope: `git diff main..develop` (23 commits, v1.4.41 vs live v1.4.40). +Reviewer: W10 security-reviewer (read-only). + +## Verdict +APPROVE_WITH_FIXES + +One High that should land before tag (rate-limit on `/api/auth/check-user`). All +other focus-area items pass. No Critical / data-leak / auth-bypass findings. + +## Critical (must fix before tag — vuln / data leak / auth bypass) + +None. + +## High (should fix before tag — defence-in-depth gap, log leak, error swallow) + +### H-1 `/api/auth/check-user` has zero rate-limiting; the JSDoc rationale is factually wrong +- **File**: `src/app/api/auth/check-user/route.ts:28-32, 53-95` +- **Attack vector**: The route discloses account-existence (`branch != "not_found"`) + plus credential-shape (`hasPasskey`, `hasPassword`) for any submitted identifier. + This is by design for iOS onboarding, but the source comment claims: + > *"No rate-limit middleware added here; the higher-level edge limit on + > `/api/auth/*` covers brute-force enumeration concerns, and the route is + > functionally equivalent in information leak to the existing + > `/api/auth/passkey/login-options` request that already accepts an identifier."* + + Both claims are wrong: + 1. There is no `middleware.ts` anywhere in the repo (`find . -name "middleware.ts"` + returns empty); rate-limiting is per-route via `checkRateLimit()`. Sibling + routes implement it explicitly — `auth/login`: `5/15min`, + `auth/register`, `auth/passkey/login-options`: `10/15min`, + `auth/refresh`, `auth/me/research-mode`: `5/min`. **`check-user` is the only + unauth POST under `/api/auth/*` without per-IP rate-limit.** + 2. `auth/passkey/login-options/route.ts` does NOT accept an identifier — it + reads no request body and returns a WebAuthn challenge that does not + disclose user existence. So check-user is not "functionally equivalent" in + leak; it is strictly more powerful. + + The contract itself (intentional enumeration) is acceptable for the iOS + onboarding UX. The omission of a brute-force throttle is not — an attacker + can probe the entire user base at HTTP line rate. +- **Recommended fix**: add a per-IP throttle mirroring `login-options`: + ```ts + const ip = getClientIp(request) ?? "unknown"; + const rl = await checkRateLimit(`auth:check-user:${ip}`, 10, 15 * 60 * 1000); + if (!rl.allowed) { + return NextResponse.json( + { data: null, error: "Too many requests. Please try again later." }, + { status: 429, headers: rateLimitHeaders(rl) }, + ); + } + ``` + Also delete or correct the misleading "edge limit on `/api/auth/*` covers …" + paragraph in the file header so the next reader doesn't repeat the assumption. + +## Medium (recommended for tag) + +None. + +## Low (defer to v1.4.42) + +### L-1 `/api/auth/check-user` timing side-channel on the `not_found` branch +- **File**: `src/app/api/auth/check-user/route.ts:65-83` +- **Attack vector**: The `not_found` branch returns immediately after `findFirst` + with `select { id, passwordHash, _count: { passkeys } }`; the relational + `_count` on the passkey relation costs a roundtrip vs. a flat user row, so + a found user with N>0 passkeys is marginally slower than a `not_found`. + Acceptable now (Postgres ms-range, swamped by network jitter), but if the + contract is "enumeration is the explicit feature", the timing channel should + not be the *backup* enumeration affordance. +- **Recommended fix** (defer): align timing by issuing the same shape query + on the `not_found` path, or add a small randomised jitter (5–25 ms). Not + required for tag — the explicit `branch` field already discloses the same + information. + +### L-2 `auth.check-user` is not audit-logged +- **File**: `src/app/api/auth/check-user/route.ts:77, 93` +- **Attack vector**: The route calls `annotate({ action: { name: "auth.check-user" } })` + for the wide-event log but never calls `auditLog(...)`. Sibling routes + (`login`, `register`, `password`, `passkey/login-verify`) emit `auth.login.failed` + / `auth.register.success` / etc. into `auditLog` so they show up in the + admin status timeline. If H-1 lands and the throttle ever fires, there'll + be no `auditLog` row showing "the throttle clipped a burst". Wide events + are sampled and not retained as long as `auditLog`. +- **Recommended fix** (defer): emit `auth.check-user` to `auditLog` with + `{ ipAddress, identifier_hash }` (hash, not raw email) so the admin + timeline carries the enumeration probe trail without storing PII. Defer + to v1.4.42 — the wide-event annotation is enough for short-term + diagnostics. + +### L-3 `editMessageReplyMarkup` deletion — leaked-via-error log warning gone +- **File**: `src/lib/telegram.ts` (deleted lines 115-134 of pre-diff) +- **Attack vector**: None now — there is no remaining caller (`grep -rn + editMessageReplyMarkup` is clean). Noting only because the deleted helper + was the one Telegram-API surface that swallowed `json.ok=false` into a + warning instead of a hard throw. If a future contributor restores it, + they should restore the throw, not the warn. +- **Recommended fix**: nothing required. + +## Strengths + +1. **Soft-delete fix-ups are comprehensive, not headline-only.** + - `/api/export/full-backup/route.ts`, `/api/export/measurements/route.ts`, + `/api/export/route.ts` — every `measurement.findMany` in the changed + files gets `deletedAt: null` (verified by full grep — single read per + file, all filtered). + - `/api/gamification/achievements/route.ts:531-549` — the measurement + branch of the `Promise.all` is filtered; the sibling reads + (`medicationIntakeEvent`, `moodEntry`, `passkey`, `auditLog`) correctly + stay unfiltered because those tables don't have `deletedAt` columns + (verified against `prisma/schema.prisma`). + - `/api/doctor-report/availability/route.ts:58-98` — all four + `measurement.count` reads filter; the `moodEntry` and + `medicationIntakeEvent` counts correctly do not (no `deletedAt` col). + - `src/lib/doctor-report-data.ts:275-281` — the headline aggregator + is the single measurement read in the file. + - `/api/admin/status/route.ts:38-44` — count corrected too. + +2. **Timeout-stub persist (`27f3bec1`) is safe.** + No user-controlled input lands in the persisted `auditLog.details`: + - `userId`: server-resolved from the auth context, route-side. + - `cacheAction`: `` `insights.blood-pressure-status.${locale}` `` / + `` `…weight-status.${locale}` ``, where `locale` is clamped by + `normalizeLocale(...)` to `"en" | "de"` (literal switch — no other + value can escape). + - `text`: `getNoKey{BP,Weight}StatusText(locale)` — deterministic + localized constant string, no PII. + - `providerType`: internal enum. + - `model: "timeout-stub"`: literal. + The write is also wrapped in `try { … } catch {}` so a stub-persist + failure cannot turn the parent request into a 5xx. Mirrors the v1.4.37 + `bmi-status` precedent. + +3. **`projectTodayIntakesAndRecompute` (`54ad5cdb`) cannot mint rows for + arbitrary users.** Both callers — `src/app/api/medications/intake/route.ts:69-70` + and `src/app/api/dashboard/summary/route.ts:232` — gate on + `requireAuth()` before passing `user.id`. The helper itself reads + `medication { where: { userId, active: true } }` so even a forged + `userId` would yield zero schedules and a no-op `createMany`. The + `(userId, medicationId, scheduledFor, source)` unique constraint plus + `skipDuplicates: true` is correct defence-in-depth — a stale projection + from a concurrent caller cannot duplicate-mint a row. + +4. **`editMessageReplyMarkup` deletion (`70ff3eef`) is complete.** No + remaining caller in `src/` (verified). The helper was an internal + wrapper around `editMessageReplyMarkup` — no public route reachable, + no fail-open path possible from its removal. + +5. **AI prompts unification (`8a56f482`) is a pure rename.** `git log + --diff-filter=R -M` confirms 100% similarity on both moved files + (`prompt.ts` → `insight-system-prompt.ts`, `prompt-compact.ts` → + `compact-sections.ts`). No new user-text concatenation; the + pre-existing `sanitizeForPrompt(..., maxLength)` envelope around + user-text fields (`medication.name`, `medication.dose`, + `doseChange.note`, `ctx.drug`, `ctx.doseUnit`) is untouched and still + the sole entry-point for user-supplied prose into LLM prompts. + +6. **Pure relocation / type-extraction commits are inert.** `8cfb1715` + (WMY + cumulative readers → `src/lib/rollups/`), the `BackupRow` / + `BackupsList` move to `src/types/backups.ts`, and the `AnalyticsData` + consolidation are all import-renames with no behaviour change — + verified against the diff stats and the rename detection output. + +--- + +50-word summary follows the report. diff --git a/.planning/round-v1441-QA-senior-dev-findings.md b/.planning/round-v1441-QA-senior-dev-findings.md new file mode 100644 index 000000000..b456ce1d1 --- /dev/null +++ b/.planning/round-v1441-QA-senior-dev-findings.md @@ -0,0 +1,150 @@ +# v1.4.41 Senior-dev architectural findings + +Branch under review: `develop` vs live `main` (v1.4.40). 23 commits. +Read-only review. No files modified. + +## Verdict + +**APPROVE_WITH_FIXES** + +The release is architecturally sound. The five W-DELETED-2 + W-ORG + W-FRONTEND-FACTORY + W-SIMPLIFIER waves landed with clean discipline and good test coverage. The W-INSIGHTS-HOT timeout-stub fix (commit `27f3bec1`) is correct but only partial — four sibling status routes still ship the bare-fallback shape that drove the very paper-cut Marc reported. None of the findings below are release-blockers, but the partial parity and one misleading code comment should land before the tag. + +## Critical + +(none) + +## High + +### H1. Timeout-stub parity is partial — four siblings still on bare-fallback shape + +**Files:** `src/lib/insights/general-status.ts:392-399`, `src/lib/insights/pulse-status.ts:337-344`, `src/lib/insights/mood-status.ts:449-456`, `src/lib/insights/medication-compliance-status.ts:334-342` + +**Root cause:** commit `27f3bec1` ported the v1.4.37 bmi-status persisted-sentinel pattern to `blood-pressure-status.ts` and `weight-status.ts` only. The other four status libraries that race the same 20 s provider call with the same `withTimeout(…, STATUS_PROVIDER_TIMEOUT_MS, null)` shape still return the deterministic fallback text **without** writing the auditLog cache row keyed to today. So a single provider stall on any of these four leaves the next mount paying the full 20 s race again — exactly the regression the v1.4.41 fix targeted. + +The four loci are byte-identical: + +```ts +if (raced.timedOut || raced.value === null) { + return { + hasProvider: true, + text: getNoKey<...>StatusText(locale), + cached: true, + updatedAt: null, + }; +} +``` + +**Recommended fix:** apply the same `prisma.auditLog.create({ data: { ..., timeout: true } })` block from `blood-pressure-status.ts:557-583` to the four siblings. Each carries its own `cacheAction = "insights.."`, `todayKey`, and `getNoKey<...>Text(locale)` already — the lift is a straight copy-edit. `medication-compliance-status.ts` returns a richer shape (`summary`, `medications: []`) so the stub there should write `summary` into `details.text` (mirroring its non-timeout path at line 374 and onwards) while still returning `medications: []` to the caller. + +**Why High not Critical:** the user-facing UI is identical on the first hit (deterministic fallback text either way). The hit hurts only the *second* mount within the day, after a one-off provider hiccup. v1.4.41 explicitly framed this as the only user-visible release item Marc would feel (`.planning/v1441-handoff.md` line 22). Shipping with only 2/6 routes patched leaves four routes still bleeding. + +## Medium + +### M1. `/api/auth/check-user` is unrate-limited; comment justifies omission with a non-existent middleware + +**File:** `src/app/api/auth/check-user/route.ts:28-32` + +**Root cause:** the route header says "No rate-limit middleware added here; the higher-level edge limit on `/api/auth/*` covers brute-force enumeration concerns". I grepped for that edge limit (`middleware.ts` at repo root, `src/middleware.ts`, any `/api/auth` allowlist in `src/lib/rate-limit.ts`, `next.config.*`) and could not find it. The comparable `/api/auth/passkey/login-options` (cited in the same header) does carry its own `checkRateLimit('auth:passkey-login-options:', 10, 15 * 60 * 1000)` block — that's a per-route limit, not an edge one. + +The functional outcome is acceptable per the W-IOS-COORD decision: the new endpoint discloses no more than `/api/auth/passkey/login-options` already does. But the rationale in the code is wrong, which means a future contributor reading this comment might confidently mirror the no-limit pattern in a route that *doesn't* have a comparable enumeration baseline. + +**Recommended fix:** either (a) add a per-route `checkRateLimit('auth:check-user:', 30, 15 * 60 * 1000)` block matching the passkey-login-options shape, or (b) rewrite the comment to cite the actual enumeration-equivalent (`/api/auth/passkey/login-options`) rather than a non-existent edge limit. (a) is the safer call given iOS onboarding is the only caller and 30 / 15 min is generous. + +### M2. ESLint `queryKey-factory` rule whitelist has drifted from the test-guard substitute + +**File:** `eslint-plugins/healthlog/queryKey-factory.js:48-57` (GUARDED_DIRECTORIES + GUARDED_FILES) + +**Root cause:** `eslint-plugins/healthlog/queryKey-factory.js` lists the ESLint rule's enforcement scope as `src/components/charts`, `src/components/comparison`, `src/app/page.tsx`, `src/hooks/use-auth.ts`. The companion test-guard substitute at `src/lib/__tests__/query-keys.test.ts:192-203` covers a wider scope — it also includes `src/app/auth`, `src/app/notifications`, and `src/components/settings/about-section.tsx` (the three surfaces W-FRONTEND-FACTORY migrated in `0bf07abd`). The ESLint rule's header comment claims "Extend in lockstep with the test-guard substitute's `guardedRoots`" but the two lists are already out of sync at landing. + +**Effect:** the ESLint rule won't catch a future bare-literal `queryKey: [...]` regression in the three v1.4.41-migrated dirs at IDE / `pnpm lint` time. The test-guard substitute will catch it (via `pnpm test`), but the whole point of promoting to ESLint per `.planning/phase-W-PROCESS-DOCS-v1441-report.md` was to fail fast. + +**Recommended fix:** add the three entries to `GUARDED_DIRECTORIES` / `GUARDED_FILES` in `queryKey-factory.js`: + +```js +const GUARDED_DIRECTORIES = [ + "src/components/charts", + "src/components/comparison", + "src/app/auth", + "src/app/notifications", +]; +const GUARDED_FILES = [ + "src/app/page.tsx", + "src/hooks/use-auth.ts", + "src/components/settings/about-section.tsx", +]; +``` + +## Low + +### L1. `pr-detection-worker.ts` doesn't filter soft-deleted measurements + +**File:** `src/lib/personal-records/pr-detection-worker.ts:219-221`, `:402-407` + +**Root cause:** the W-DELETED-2 sweep covered export, gamification, and doctor-report but did not touch the personal-records worker. `prisma.measurement.count({ where: { userId, type: metricType } })` (line 219) and `findBestMeasurement` (lines 402-407) both omit `deletedAt: null`. So a soft-deleted measurement can: +1. count toward the 30-row warm-up threshold (line 222); +2. be returned by `findBestMeasurement` as the user's best-ever value (line 402), which then writes a `PersonalRecord` row keyed to a tombstoned measurement. + +The worker is best-effort and the surfaces that read PR rows (`personal-record-badge.tsx`) just render the value — they don't re-validate against the measurement table — so the user sees a "best weight" that no longer exists on their list. + +**Recommended fix:** add `deletedAt: null` to both `where` clauses. Defer to v1.4.42 unless an iOS undo or admin delete is observed creating a stale PR in production. + +### L2. `offhost-backup.ts` includes soft-deleted measurements + +**File:** `src/lib/jobs/offhost-backup.ts:219` + +**Root cause:** `prisma.measurement.findMany({ where: { userId: user.id } })` omits `deletedAt: null`. This is the S3 nightly disaster-recovery dump, NOT a user-facing export. Per the W-DELETED-2 design intent (round-trip exports must not resurrect deletes) the user-initiated `/api/export/full-backup` correctly excludes them. The DR worker likely *should* keep them — restoring a tombstone is the safer DR semantic. But there's no documenting comment either way. + +**Recommended fix:** add an inline comment to `offhost-backup.ts:219` documenting the intent ("includes soft-deleted rows because this is the DR snapshot, not a user-facing export — see `/api/export/full-backup/route.ts` for the symmetric exclusion"). No behavioral change. + +### L3. Duplicate `DataSummary` import in `src/types/analytics.ts` + +**File:** `src/types/analytics.ts:37-38` + +**Root cause:** + +```ts +import type { DataSummary } from "@/lib/analytics/trends"; +import type { DataSummary as DataSummaryType } from "@/lib/analytics/trends"; +``` + +Same named import declared twice — once bare, once aliased. The aliased one is referenced exactly once at line 80 (`glucoseByContext?: Record`). The bare import is the canonical one used everywhere else in the file. + +**Recommended fix:** drop the aliased import and change line 80 to `Record`. Cosmetic / consistency. + +### L4. No dedicated unit test for `projectTodayIntakesAndRecompute` + +**File:** `src/lib/medications/scheduling/project-today-intakes.ts` + +**Root cause:** the helper is exercised only through the two route tests (`intake/route.test.ts`, `dashboard/summary/route.test.ts`). The W-SIMPLIFIER report at `.planning/phase-W-SIMPLIFIER-report.md` line 24-40 names the extraction as the main deliverable but doesn't add a `__tests__/project-today-intakes.test.ts` directly. The route tests do cover the happy path and the missing-row backfill, so coverage is structurally present, just not isolated. + +**Recommended fix:** v1.4.42 add a focused helper test (idempotent backfill, `Promise.allSettled` recompute-failure swallow, `skipDuplicates` no-op on duplicates). + +### L5. Helper `recomputeMedicationComplianceForEvent` swallows-on-failure but the helper now `await Promise.allSettled`s — explicit-redundant + +**File:** `src/lib/medications/scheduling/project-today-intakes.ts:127-142` + +**Root cause:** the doc-comment on line 122-126 says "the helper swallows internally today, but a future refactor that lets a throw escape would otherwise turn the parent POST into a 5xx". That is correct defense-in-depth, but the call site is also wrapped in `Promise.allSettled`. The intent is right; just flag that one of the two layers should remain documented as the explicit contract, otherwise a future contributor will simplify one assuming the other holds. + +Not a bug today. + +## Strengths + +- **W-DELETED-2 sweep is thorough.** All three new fixes (`a62b9498`, `cb8f74e4`, `5296a612`) land with the matching `deletedAt: null` filter in the right `where` clause, and every supporting raw SQL in `comprehensive-aggregator.ts`, `summaries-slice.ts`, `dashboard/summary/route.ts`, `measurements/route.ts`, `analytics/route.ts`, `bp-in-target-fast-path.ts`, `correlations-fast-path.ts`, `health-score-fast-path.ts`, `glp1-plateau.ts`, `features.ts`, `sync/state/route.ts`, `insights/cards/route.ts`, `insights/targets/route.ts`, `insights/generate/route.ts`, `ai/coach/snapshot.ts` carries the same `AND m."deleted_at" IS NULL` predicate. Eleven reader tiers covered. + +- **UNION arm retirement is safe.** The `mean * count` fallback in `dashboard/summary/route.ts:498-500` is intact and the legacy-NULL test pin at `summary/__tests__/route.test.ts:278-314` still asserts the fallback path. The Marc-tenant convergence is the right gate; the per-day discovery LEFT JOIN still surfaces any future legacy row on a self-hosted instance, even if its boot-time backfill needs to re-mint a row. + +- **Helper extraction is read-shape-neutral.** Both call sites consume the returned `{ projected, backfilled }` tuple correctly. The dashboard discards the values; the intake route forwards them onto `annotate(...)` for prod observability. The helper's `IntakeSource: "REMINDER"` literal preserves the doctor-report + analytics filter byte-stability. The `skipDuplicates: true` + the `@@unique([userId, medicationId, scheduledFor, source])` composite key give two layers of race protection. + +- **Type consolidation is clean.** `AnalyticsData` and `BackupRow` lifts kept the call-site contract via the `as AnalyticsData` aliases (`page.tsx:28`, `insights/page.tsx:100`, `getting-started-checklist.tsx:75`) so no consumer pays for the type relocation. The four-shape (`SubPage`/`Dashboard`/`Insights`/`Checklist`) split is the right design call — collapsing into one optional-everywhere shape would have lost the per-surface contract. No circular imports: both type modules consume `@/lib/analytics/trends` and `@/lib/insights/correlations`, neither of which import back from `@/types/*`. + +- **queryKey factory expansion has matching test pins.** The walker test at `src/lib/__tests__/query-keys.test.ts:55-69` covers all three new migrations (`authRegistrationStatus`, `notificationsPreferences`, `notificationsStatus`, plus the previously-unverified `apiVersion`). The bare-literal lint guard at lines 192-203 extends to the three new surfaces. No bare-literal `queryKey: [...]` survives in `src/app/auth`, `src/app/notifications`, or `src/components/settings/about-section.tsx`. + +- **`/api/auth/check-user` branching is correct.** All four documented branches (`not_found`, `passkey_only`, `email_fallback`, `exists`) are reachable from the `hasPasskey` / `hasPassword` truth table, the test pin at `src/__tests__/api/auth/check-user/route.test.ts` covers all four plus the 422 input-validation path, the response envelope matches the `apiSuccess({ branch, hasPasskey, hasPassword })` shape iOS expects, and the route never echoes the identifier (PII-safe). The OR-match on `username | email` mirrors the canonical login-credential resolver. + +- **Prompt-dir unification (`8a56f482`) is a pure move.** Every import path (`@/lib/ai/prompts/compact-sections`, `@/lib/ai/prompts/insight-system-prompt`) is consistent across the two consumers (`api/insights/generate/route.ts`, `ai/coach/snapshot.ts`) and the test file (`__tests__/prompt-comparison.test.ts`). No dangling `@/lib/insights/prompt*` references remain. + +- **W-SIMPLIFIER cleanup is conservative.** The `tx?` param drop on three rollup helpers (`mood-rollups`, `medication-compliance-rollups`) doesn't break any call site because no call site ever passed a tx. The 13 dead-export trims are the narrowing kind (drop `export`, keep the symbol), and the four reverted candidates (`CHART_RANGE_PRESETS`, `CHANNEL_TYPES`, `CHECKLIST_ITEM_IDS`, `moodLogSyncResponseSchema`) keep their export because they back `typeof X[number]` aliases — the right call. + +--- + +**Summary:** verdict APPROVE_WITH_FIXES, 0 Critical, 1 High, 2 Medium, 5 Low, 7 strengths. The only finding that should land before the v1.4.41 tag is H1 (port the persisted timeout-stub pattern to the four remaining status routes). Everything else is safe to defer to v1.4.42. diff --git a/.planning/round-v1441-QA-simplifier-findings.md b/.planning/round-v1441-QA-simplifier-findings.md new file mode 100644 index 000000000..755b531ac --- /dev/null +++ b/.planning/round-v1441-QA-simplifier-findings.md @@ -0,0 +1,190 @@ +# v1.4.41 Simplifier-residual findings + +## Verdict + +APPROVE_WITH_FIXES — the W-SIMPLIFIER wave delivered clean, well-scoped wins (helper extraction, knip −13, lint to zero, dead-export trim). The residual scan turned up three small clarity regressions introduced *this release* by sibling waves (W-INSIGHTS-HOT duplicated a 45-line block 3-way; W-ORG mis-ordered two `import type` statements; W-FRONTEND-FACTORY left a double comment block) plus two structural simplifications the simplifier wave didn't reach. None block the release. All five are mechanical follow-ups that could land as a single small commit on develop before tag, or roll into v1.4.42. + +## Critical + +None. + +## High (clarity regression introduced this release) + +### H1 — `src/lib/insights/blood-pressure-status.ts:544-590` + `src/lib/insights/weight-status.ts:442-488`: timeout-stub persist block duplicated 3-way + +W-INSIGHTS-HOT (phase report §"Fix") mirrors a 45-line `if (raced.timedOut || raced.value === null) { …prisma.auditLog.create({ … model: "timeout-stub", timeout: true …}) }` block verbatim from `bmi-status.ts` into both `blood-pressure-status.ts` and `weight-status.ts`. The three blocks are byte-identical except for the no-key fallback text getter name. The phase report itself describes the work as "Mirror the bmi-status stub-persist pattern verbatim into both routes" — that's a literal cue to extract. + +**Proposed shape** — `src/lib/insights/persist-timeout-stub.ts`: + +```ts +export async function persistTimeoutStubAndReturn(input: { + userId: string; + cacheAction: string; + todayKey: string; + locale: string; + providerType: string; + stubText: string; +}): Promise<{ hasProvider: true; text: string; cached: true; updatedAt: string | null }> { + let stubUpdatedAt: string | null = null; + try { + const stub = await prisma.auditLog.create({ + data: { + userId: input.userId, + action: input.cacheAction, + details: JSON.stringify({ + dateKey: input.todayKey, + locale: input.locale, + text: input.stubText, + providerType: input.providerType, + model: "timeout-stub", + tokensUsed: null, + timeout: true, + }), + }, + select: { createdAt: true }, + }); + stubUpdatedAt = stub.createdAt.toISOString(); + } catch { + // Best-effort persist — caller still gets the deterministic fallback text. + } + return { hasProvider: true, text: input.stubText, cached: true, updatedAt: stubUpdatedAt }; +} +``` + +Each of the 3 call sites collapses from ~40 lines to: + +```ts +if (raced.timedOut || raced.value === null) { + return persistTimeoutStubAndReturn({ + userId, cacheAction, todayKey, locale, + providerType: provider.type, + stubText: getNoKeyBloodPressureStatusText(locale), // metric-specific + }); +} +``` + +Net: ~120 LOC of duplicate `prisma.auditLog.create({ … timeout: true })` collapses to one helper + three 6-line invocations. The two pre-warm-worker-recognition comments shrink to one header on the helper. Cost: nil — the helper is a pure pass-through to the same `prisma.auditLog.create` call. + +### H2 — `src/components/onboarding/getting-started-checklist.tsx:75` + `src/app/insights/page.tsx:100`: `import type` placed after non-import statements + +The W-ORG wave hoisted shared analytics shapes into `src/types/analytics.ts` and added the `import type { … as AnalyticsData }` alias-back-import next to the old inline interface declaration — landing the import in the middle of the file, after non-import statements. + +`src/components/onboarding/getting-started-checklist.tsx`: +- Line 16: `import type { LucideIcon } from "lucide-react";` (top of file, with other imports) +- Lines 30-71: constants + `ITEM_ICONS` + `ITEM_LABEL_KEYS` declarations +- **Line 75**: `import type { ChecklistAnalyticsData as AnalyticsData } from "@/types/analytics";` ← mid-file import + +`src/app/insights/page.tsx`: +- Lines 1-18: import block +- Lines 88-95: `interface ComprehensiveData` declaration +- **Line 100**: `import type { InsightsAnalyticsData as AnalyticsData } from "@/types/analytics";` ← mid-file import + +**Proposed shape**: Move both `import type` lines into the top-of-file import block alongside their peers. The aliased name (`as AnalyticsData`) stays for code-churn minimisation. The "v1.4.41 W-ORG — shared shape lives in …" comment can collapse to a single inline trailing comment on the import. + +This is the kind of drift an ESLint `sort-imports` / `import/first` rule would have blocked at PR time. Worth checking whether the existing config already has `import/first` and just doesn't fire on the dynamic-import pattern in `src/app/page.tsx`. + +### H3 — `src/types/analytics.ts:37-38`: same symbol imported twice (one aliased) + +```ts +import type { DataSummary } from "@/lib/analytics/trends"; +import type { DataSummary as DataSummaryType } from "@/lib/analytics/trends"; +``` + +`DataSummary` is used on lines 44, 55, 100. `DataSummaryType` is used once on line 80 (`glucoseByContext?: Record;`). + +The duplicate-import shape was carried verbatim from `src/app/page.tsx` (lines 27 + 79 — same file still has both today) when the W-ORG wave consolidated `interface AnalyticsData` into `src/types/analytics.ts`. The alias was justified there because both names existed historically in `page.tsx`; in a fresh module there's no reason for two import lines. + +**Proposed shape**: + +```ts +import type { DataSummary } from "@/lib/analytics/trends"; +… +glucoseByContext?: Record; +``` + +Same fix for `src/app/page.tsx` lines 27 + 79 — collapse to a single `import type { DataSummary } from "@/lib/analytics/trends";` and use `DataSummary` on line 243 (the only `DataSummaryType` reference in that file). + +## Medium (clean wins the W-SIMPLIFIER wave missed) + +### M1 — `src/app/page.tsx:1403-1424`: two adjacent comment blocks describe the same `` + +The dashboard tile-strip Suspense boundary now carries two adjacent JSX comment blocks (v1.4.40 W-RSC rationale + v1.4.41 W-FRONTEND-FACTORY rationale). They overlap heavily — both explain "the body is synchronous today" + "future RSC hoist". Reads as historical archaeology rather than current intent. + +**Proposed shape**: consolidate into a single 5-6 line block keyed to v1.4.41 (the current behaviour) with a one-line "v1.4.40 W-RSC — boundary added; v1.4.41 W-FRONTEND-FACTORY — fallback hoisted to a layout-stable placeholder" trailer. + +Same comment-drift pattern *might* exist on the chart-row Suspense (lines 1441-1464); didn't audit deeply but worth a once-over in the same pass. + +### M2 — `src/lib/insights/{bmi,blood-pressure,weight,pulse,mood,general,medication-compliance}-status.ts`: `BERLIN_DAY_FORMATTER` + `toBerlinDayKey()` duplicated 7-way + +Each of the seven `*-status.ts` files carries an identical 20-LOC block: + +```ts +const BERLIN_DAY_FORMATTER = new Intl.DateTimeFormat("en-US", { + timeZone: "Europe/Berlin", year: "numeric", month: "2-digit", day: "2-digit", +}); + +function toBerlinDayKey(date: Date): string { /* identical body */ } +``` + +`src/lib/insights/bucket-series.ts` already declares an internal-only `toBerlinYmd` + the exported `dayOffsetToBerlinDayKey`. Adding a sibling `export function toBerlinDayKey(date: Date): string` to that module (or `src/lib/tz/resolver.ts`) and importing in 7 places drops ~120 LOC of identical helpers. + +This is a fair size, would benefit from being its own commit; not v1.4.41-introduced but the timeout-stub change in 3 of these 7 files made the duplication footprint conspicuous. Flagging as Medium because it's a clear, low-risk, follow-up. + +### M3 — `src/lib/insights/{mood,pulse,general,medication-compliance}-status.ts`: timeout fallback does NOT persist the stub + +Sibling to H1: while `bmi-status`, `blood-pressure-status`, and `weight-status` now all persist a `timeout: true` sentinel on stall, the other four `withTimeout`-wrapped insight routes still execute the original "return fallback text, do not persist" branch: + +```ts +// e.g. src/lib/insights/mood-status.ts:449-456 +if (raced.timedOut || raced.value === null) { + return { + hasProvider: true, + text: getNoKeyMoodStatusText(locale), + cached: true, + updatedAt: null, + }; +} +``` + +The behaviour gap means a provider stall on `mood-status` / `pulse-status` / `general-status` / `medication-compliance-status` still re-races every cold mount for the rest of the day — exactly the Marc-reported regression W-INSIGHTS-HOT was scoped to close, scoped narrowly to BP+weight because Marc reported only those two. + +**Status**: behavioural (not pure simplification), but if the H1 helper is extracted, wiring those four routes to it becomes a 1-line-per-route change with the same defensive `try/catch` semantics. Worth bundling. + +## Low (defer to v1.4.42) + +### L1 — `src/lib/doctor-report-data.ts` is checked in with literal control bytes inside a regex character class + +`grep -nP "[\x00\x1F\x7F]" src/lib/doctor-report-data.ts` returns line 171; `file` reports the file as `data`; `git diff` shows it as `Binary files differ` for the v1.4.41 W-DELETED-2 soft-delete fix (commit `5296a612`). This makes the file's diffs unreadable in code review, and obscures non-trivial changes (the W-DELETED-2 fix went in essentially unreviewable). + +Pre-existing — not introduced by v1.4.41 — but the v1.4.41 soft-delete fix exposed the cost. Replace the literal-byte regex `/[\x00-\x1F\x7F]/g` (written as actual NUL + US + DEL) with the escape-sequence form so the file is UTF-8-clean, diffs work, and reviewers can see what changed. Same behaviour at runtime. + +### L2 — `src/app/api/auth/check-user/route.ts:88-91`: branch resolver could use a switch or single expression + +```ts +let branch: CheckUserBranch; +if (hasPasskey && !hasPassword) branch = "passkey_only"; +else if (hasPassword) branch = "email_fallback"; +else branch = "exists"; +``` + +Reads fine today (no nested ternary, explicit), but a `function resolveCheckUserBranch(hasPasskey: boolean, hasPassword: boolean): CheckUserBranch` extraction would push the four-branch matrix into a unit-testable function next to the type declaration. Skipping for now — the inline shape is honest about the decision and works against the 4-line `if/else if/else` rule from the CLAUDE.md style guide. Pure preference. + +## Already-deferred items (acknowledge, do not re-flag) + +Per `.planning/phase-W-SIMPLIFIER-report.md` §"Follow-up for v1.4.42" — the simplifier wave deliberately deferred these and I am not re-flagging them: + +- `tokenKind`, `describeInjectionSite`, `PROGRESS_TICK_RECORDS`, `MAINTAINED_LOCALES`, `SUB_PAGE_METRIC`, `withBackgroundEventSafe`, `listSupportedTimezones` re-export at `tz/resolver.ts` +- `AlertDialogMedia / AlertDialogOverlay / AlertDialogPortal`, `AvatarBadge / AvatarGroup / AvatarGroupCount`, `badgeVariants`, `buttonVariants`, `CardFooter / CardAction / CardDescription`, `DialogOverlay / DialogPortal / DialogTrigger`, six unused `DropdownMenu*` exports, five unused `Select*` exports, `SheetTrigger`, `TableFooter / TableCaption`, `tabsListVariants` — shadcn surface-area exports needing Marc's call before removing +- `isRollupFresh` in `measurement-rollups.ts` — pending W-PERF-OPS-1 follow-up +- All 52 "unused exported types" — flag-by-flag audit (zod `z.infer<>` types, iOS native client contract, OpenAPI doc-gen) +- `recommendationSeveritySchema / aiCitationSchema / aiWarningSchema / storyboardAnnotation*` in `src/lib/ai/schema.ts` +- The `summaries-slice.ts` test exposure of `aggregateWmyBuckets` + `RollupBucketRow` — exports retained because the test suite imports them externally + +## Strengths + +- **W-SIMPLIFIER discipline**: 13 dead exports trimmed without churn-chasing the 4 cases (`CHART_RANGE_PRESETS`, `CHANNEL_TYPES`, `CHECKLIST_ITEM_IDS`, `moodLogSyncResponseSchema`) where the constant IS the source of truth for a `typeof X[number]` / `z.infer<>` type — the simplifier correctly reverted those after lint surfaced the dependency. That's the right call. +- **W-SERVER-FIX-2 helper extraction**: `projectTodayIntakesAndRecompute({ userId, userTz, todayStart, todayEnd })` shaves ~200 inline LOC across two routes into one 145-line tested helper. Comments document the `IntakeSource` "REMINDER" reuse, the `skipDuplicates: true` defense-in-depth rationale, the per-`(med, day)` coalescing, and the `Promise.allSettled` best-effort contract. Reads as Marc's authorship. +- **W-ORG type consolidation**: four named shapes (`SubPage…`, `Dashboard…`, `Insights…`, `Checklist…`) instead of one swiss-army `AnalyticsData` correctly preserves TypeScript control flow for the call-sites that need to demand fields. The "we keep four shapes (rather than collapsing into one with everything optional) because …" header on `src/types/analytics.ts` documents exactly why. +- **UNION discovery cleanup**: `9d2901aa` retires a now-no-op `sum_value IS NULL` UNION arm with one paragraph of audit-trail context for the next visitor. Test pruned alongside. Minimal, surgical. +- **Coverage of the simplification**: the new `query-keys` factory expansion (admin surfaces + 6 admin keys + insights settings + provider chain + GLP1 timeline + auth/profile) lands with a centralisation comment per key cluster and a fresh `src/lib/__tests__/query-keys.test.ts` that pins the contract. Marc voice throughout — no AI/agent/marathon language. +- **Auth check-user route** (`src/app/api/auth/check-user/route.ts`): 95 LOC, four branches, explicit `if/else if/else` (no nested ternary), test-pinned contract, zod-validated input, `annotate({ … branch })` for every branch, response shape constant across found/not-found (no enumeration leak). Reads exactly as the CLAUDE.md style guide prescribes.