mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 20:42:43 +00:00
docs(planning): refresh phase-D findings + add Product-Lead strategic review
Wave-D reviewer outputs for v1.4.16: - code-review: 0 CRITICAL, 8 HIGH, 9 MED, 10 LOW - security: 0 CRITICAL, 2 HIGH, 5 MED, 3 LOW - design (Apple Health benchmarking): 3 CRITICAL ship-blockers (insights surface + dashboard preview not mounted on live routes; comparison toggle buried 3 clicks deep), 8 HIGH, 22 MED/LOW - senior-dev: 0 CRITICAL, 3 HIGH (file-size/architecture pressure), 9 MED/LOW - simplify: 8 apply-yes / 2 apply-no - product-lead: full state-of-app + v1.5 roadmap (C.1-C.11) + v1.6+ follow-on initiatives Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Marc-André BombeckandClaude Opus 4.7 committed
1 parent
55cc0e49dd
commit
c0c1284e27
6 files changed
+1887
-1436
No files matched your search
@@ -1,150 +1,190 @@
|
||||
# Phase D — Code-review findings (v1.4.15)
|
||||
# Phase D — Code-review findings (v1.4.16)
|
||||
|
||||
Reviewer: senior code-review (parallel with security / design / senior-dev / simplify).
|
||||
Scope: diff `v1.4.14...HEAD` (178 files, +20 544 / −1 408). Phases A1–A5, B-mobile, B1–B6, C1–C5.
|
||||
Method: targeted file reads of the hottest paths called out in the agent brief — AI hardening (C1), backup completeness (B1), sync robustness (B2), notification reliability (B3), onboarding tour (B5), dashboard analytics fixes (A4), auto-deploy webhook (C2), admin overview (A2). No source edits, no commits.
|
||||
|
||||
Verdict: implementation quality is high overall. Auth gates are consistently in place on every new admin endpoint. Idempotency/audit/rate-limit patterns are reused properly. The two ship-blocker concerns I flag are HIGH (not CRITICAL) and both are correctness-bugs that surface only on niche inputs.
|
||||
|
||||
Counts:
|
||||
- CRITICAL: 0
|
||||
- HIGH: 4
|
||||
- MEDIUM: 7
|
||||
- LOW: 5
|
||||
Reviewer: senior code-review (parallel with security / design / senior-dev / simplify / Product Lead).
|
||||
Scope: diff `v1.4.15...HEAD` — Wave A (A1–A8), Wave B (B1a/b, B2, B3, B4, B5a–e, B6, B7, B8), Wave C (CI + deferred HIGH).
|
||||
Method: targeted reads of hot paths — AI provider chain + confidence + schema (B5a–d), feedback loop (B5e), host-metrics + app-logs (B3/B4), exports (B7), comparison overlay (B8), BD-Zielbereich predicate (A2). No source edits, no commits.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL
|
||||
## CRITICAL — ship-blocker
|
||||
|
||||
(none)
|
||||
|
||||
The areas with the largest blast radius — `requireAdmin()` in every backup route, timing-safe webhook secret compare, encrypted credential storage, withIdempotency wrapping the destructive restore endpoint, AES-256-GCM at-rest encryption of `IntegrationStatus.lastError` — are all in place and correct. Restore is wrapped in a single Prisma transaction with `skipDuplicates` for idempotent re-runs. Pre-validation of enum values runs OUTSIDE the transaction so a malformed payload cannot half-wipe a user.
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1 — Restore transaction does delete-then-recreate without snapshot rollback safety
|
||||
### H1. Bucket-key collision in `feedback-aggregator` allows cross-bucket pollution
|
||||
- **File:** `src/lib/jobs/feedback-aggregator.ts:70-75`
|
||||
- **Issue:** `[severity, metricSourceType, providerType, promptVersion].join("")` uses the empty separator. `severity` is bounded enum but `metricSourceType` is free-form (z.string().min(1).max(80)) and `providerType`/`promptVersion` are wire-stamped. Collision example: `severity="info"` + `metricSourceType="important"` collides with `severity="important"` + `metricSourceType="info"` — both produce key `"infoimportant…"`. The first to arrive wins ownership; subsequent rows accumulate counts onto a row whose displayed `(severity, metricSourceType)` belongs to the wrong tuple, silently poisoning the admin AI quality slice.
|
||||
- **Recommendation:** Join with an unambiguous separator that cannot appear in any of the four fields, e.g. `\x1f` (US) or a JSON tuple: `JSON.stringify([severity, metricSourceType, providerType, promptVersion])`.
|
||||
- **Ship-blocker for v1.4.16?** Yes for the admin AI-quality feature semantics; can be deferred if the dashboard is treated as best-effort observability.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/app/api/admin/backups/[id]/restore/route.ts:253-377`
|
||||
- **Issue**: The Prisma transaction deletes the user's existing data first (intake events, medications, measurements, mood, channels, push subs, telegram scheduled deletions), then re-creates from the payload. If a `prisma.medication.create({...schedules:{create:[...]}})` fails mid-loop on row 5 of 10, the **entire transaction rolls back** — but only the rows still inside `tx`. Prisma's interactive transactions DO atomically roll back on a thrown error inside the callback, so this is in fact safe. However, the catch block on line 378 surfaces `err.message` directly to the admin via `apiError(...)` without scrubbing — if the error embeds a stack trace from Prisma it leaks internal table/column names to the response. Low-severity infosec but more importantly it returns 500 with a misleading "Restore transaction failed: <prisma error>" message.
|
||||
- **Recommendation**: Verify Prisma rolls back `deleteMany` calls inside the same `$transaction(async tx => ...)` callback (it does — confirmed in Prisma 5/6/7 docs). Then either (a) swallow the inner message and return a stable "Restore failed" 500 plus the audit trail, or (b) wrap the inner message via a pre-existing scrubber (`safeError()` or similar). The audit row already captures the verbose error — admins can read it from `/admin/login-overview` instead.
|
||||
- **Ship-blocker**: no — Prisma rollback is correct. The leak is minor (admin-only endpoint, error messages on internal table names). v1.4.16 fix.
|
||||
### H2. `FallbackChainCard` discards the persisted `enabled` flag — disabled providers vanish from the UI
|
||||
- **Files:** `src/app/api/insights/provider-chain/route.ts:49-56`, `src/components/settings/ai-section.tsx:1272-1283`
|
||||
- **Issue:** The GET response only returns `{ providerType, available }`. The UI seeds `enabled: true` for every entry (`chain.map((c) => ({ providerType: c.providerType, enabled: true }))`). But `resolveProviderChain()` upstream (`src/lib/ai/provider.ts:306`) **filters out disabled entries before returning** (`parseProviderChain(rawChain).filter((e) => e.enabled)`). Net effect: a user who disables an entry sees it disappear from the list; on reload it cannot be re-enabled because it's not present, and the toggle position is lost. The UI advertises the toggle as a "disable in fallback chain" affordance but is functionally a "remove from chain" button.
|
||||
- **Recommendation:** GET should return the raw persisted `parseProviderChain(rawChain)` (not the resolveProviderChain filtered list) plus `available` per entry. The UI then renders both enabled and disabled rows so the user can re-enable. Or: remove the toggle entirely and only support add/remove.
|
||||
- **Ship-blocker for v1.4.16?** Yes — it's a regression vs. the documented B2 brief ("toggle drives enable/disable"). Either fix the round-trip or remove the Switch from the row.
|
||||
|
||||
### H2 — Restore mood-entry recreation discards pre-existing `MoodEntry` IDs and `tags` JSON shape mismatch is silent
|
||||
### H3. Strict B5b/B5c/B5d AI features are not wired into the production route
|
||||
- **Files:** `src/app/api/insights/generate/route.ts:280-393`, `src/lib/ai/generate-insight.ts`, `src/lib/ai/provider-runner.ts:229-288`
|
||||
- **Issue:** The production `/api/insights/generate` route uses `runRawCompletionWithFallback()` (raw `provider.generateCompletion`, no schema enforcement, no rationale check, no confidence override). The strict `runWithFallback()` + `generateInsight()` (which enforces `aiRecommendationRationaleSchema`, runs the corrective retry, OVERRIDES `rec.confidence` deterministically, annotates `ai_confidence_override_pairs`) is consumed only in tests. Result: in production, the model's self-reported confidence is what users see — defeating B5d's "calibrated probabilities are not a small-LLM strength" mandate; rationale + citation enforcement is observational only via `insightResultSchema.safeParse()` falling back to raw parsed JSON on validation failure (line 386 `insights = validated.success ? validated.data : parsed`).
|
||||
- **Recommendation:** Either (a) migrate `/api/insights/generate` to `runWithFallback()` + new `aiInsightResponseSchema`, replacing the legacy passthrough — STATE.md notes this as a deferred "v1.4.17 migration"; or (b) add a **runtime feature-flag** gate so the strict path can be enabled in prod without a code release. Today's `applyConfidenceOverride()` and rationale-required schema are dead code in production.
|
||||
- **Ship-blocker for v1.4.16?** Maybe — depends on whether v1.4.16's release notes claim "deterministic confidence" or "rationale-required" as user-facing features. STATE.md B5c/B5d both say "complete on origin/main" but the production path is unchanged. Users will see model-claimed confidence values, not server-computed ones. Recommend explicit Marc-call: ship with feature dark + clearer release-notes wording, or migrate the route.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/app/api/admin/backups/[id]/restore/route.ts:353-366`, `/Users/marc/Projects/HealthLog/src/lib/validations/backup.ts:73-81`
|
||||
- **Issue**: `moodEntrySchema.tags` is typed as `z.string().nullable().optional()` — i.e. ANY string. `syncMoodLogEntries()` writes `tags: entry.tags ? JSON.stringify(entry.tags) : null` (sync.ts:221) so the on-disk shape is a JSON-encoded array. The restore route writes the value verbatim with `tags: e.tags ?? null`. If a future moodLog payload writes a non-JSON string (or a future schema migration changes the shape), restore silently loads the bad value. The download path validates against `parseBackupPayload` so we'd catch it, but only if the value wasn't a string at all. A garbage string slips through.
|
||||
- **Recommendation**: Tighten `moodEntrySchema.tags` to `z.union([z.null(), z.string().refine(s => { try { return Array.isArray(JSON.parse(s)); } catch { return false; } })])` OR keep it permissive on the restore side but validate via JSON-parse during the existing pre-tx loop (alongside `MEASUREMENT_TYPES`/`INTAKE_SOURCES` checks at lines 204-249). Cheaper than a schema change.
|
||||
- **Ship-blocker**: no — affects only round-trip uploads of corrupted backups, not the normal worker→download→re-upload→restore flow. v1.4.16 backlog.
|
||||
### H4. `findRecommendationsMissingRationale` is a no-op against parsed AIInsightResponse
|
||||
- **File:** `src/lib/ai/schema.ts:283-297`, `src/lib/ai/generate-insight.ts:158-175`
|
||||
- **Issue:** The strict schema requires `rationale` (`aiRecommendationRationaleSchema` is required on every rec — line 157). If the parser succeeds, every rec will have a rationale, so the helper always returns `[]`. The Wide-Event annotation `ai_rationale_missing_recommendation_ids` is therefore always empty in the strict path — and the strict path is the only place the helper fires. The "observability tripwire for the migration window" comment is misleading.
|
||||
- **Recommendation:** Either feed it a separate looser-typed pre-parse blob (e.g. the JSON before `aiInsightResponseSchema.safeParse()` runs), or move the call to the `isLegacyInsightPayload()` cache-hit path where it would actually surface drift. Document or remove the tripwire claim.
|
||||
- **Ship-blocker for v1.4.16?** No — observability dead-code is not a functional bug. Worth fixing for clarity.
|
||||
|
||||
### H3 — `MoodChart.aggregateMoodEntries` and the inline `chartData` aggregation produce divergent buckets
|
||||
### H5. `recommendation-feedback` has no rate-limit
|
||||
- **File:** `src/app/api/insights/feedback/route.ts:30-101`
|
||||
- **Issue:** No `checkRateLimit()` call. Each thumbs-click writes one `RecommendationFeedback` row + one `audit_logs` row. The unique index `(userId, recommendationId, recommendationText)` prevents exact-duplicate inserts, but a malicious client can rotate `recommendationId` or `recommendationText` per request and flood the DB. Idempotency-Key cache only catches identical retries with the same key.
|
||||
- **Recommendation:** Add `checkRateLimit('feedback:${user.id}', 60, 60 * 60 * 1000)` (60/h is generous — a comprehensive insight rarely has >10 recs, so ≥6 regenerations/h before throttle hits).
|
||||
- **Ship-blocker for v1.4.16?** No — single-user scoped, inside-auth. Fix in v1.4.17.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/mood-chart.tsx:185-219` (export `aggregateMoodEntries`) vs lines 247-292 (inline `chartData` useMemo)
|
||||
- **Issue**: The exported helper buckets entries by `pickMoodBucket(entries)` BEFORE applying the `rangePoints` slice. The inline render path slices to last-N, then computes range-days from the sliced timestamps and runs `pickBucket(rangeDays)` again. Result: a 365-day-window user with `rangePoints=30` sees DAILY buckets in the chart (30 sliced days < 90 threshold) while the unit test exercising `aggregateMoodEntries` over the full dataset reports MONTHLY. The unit-test assertion does NOT cover what the user actually sees on the dashboard. The `activeBucket` chip in the header (lines 346-357) duplicates the inline path's logic so the chip stays consistent with the chart data — but the standalone export is a misleading second source of truth.
|
||||
- **Recommendation**: Either (a) inline `aggregateMoodEntries()` is dead code only used by tests — drop it and add a test that mounts the actual component or directly asserts the chip's `activeBucket` value; or (b) refactor `chartData` to call `aggregateMoodEntries(sliced)` so both paths share one bucketing primitive. Today the divergence means the unit test is asserting an aggregation behaviour the user never sees.
|
||||
- **Ship-blocker**: no — the user-visible chart renders correctly. But the test coverage is illusory and a future regression in `chartData` won't fail any test.
|
||||
### H6. `recommendation-feedback` invalidates the entire `["insights"]` cache on every thumbs click
|
||||
- **File:** `src/components/insights/recommendation-feedback.tsx:192`
|
||||
- **Issue:** `queryClient.invalidateQueries({ queryKey: ["insights"] })` matches ALL `["insights", …]`-prefixed keys (8 queries: comprehensive + 7 per-status). On thumbs success this kicks off a synchronous refetch of all 8 — none of which are affected by the feedback row itself. Refetching the comprehensive blob requires a 1500-token LLM call (rate-limited 10/h) — the only thing that prevents this is the 24h server-side cache-hit path. Effectively the user gets no surprise refetch in practice but the network and cache thrash is wasted work, and on a stale cache it's a real LLM round-trip.
|
||||
- **Recommendation:** Drop the invalidation entirely; feedback doesn't affect the displayed insight content. If a future mode (B5e ratchet in v1.4.17) makes feedback feed back into next-gen prompts, scope the invalidation to the per-status / comprehensive query that would consume the new prompt.
|
||||
- **Ship-blocker for v1.4.16?** No — minor performance smell.
|
||||
|
||||
### H4 — `TourLauncher` missing-key cleanup on logout / impersonation switch
|
||||
### H7. Comparison overlay re-uses already-fetched `data` as the "prior period" — there is NO prior-period data
|
||||
- **Files:** `src/components/charts/health-chart.tsx:524-563`, `src/lib/charts/comparison-shift.ts`
|
||||
- **Issue:** `chartDataWithCompare` shifts the SAME `data` array forward by 30/365 days. The dashboard fetcher upstream pulls `data` as the visible window only (e.g. last 30 days). Shifting today's last-30-days forward by 30 days produces "today's data overlaid 30 days into the future" — not "last month's data overlaid onto today". For the comparison to be meaningful the chart needs a SEPARATE fetch of the prior 30/365-day window. STATE.md B8 says "shift the SAME daily aggregates and shift them forward by 30 days (lastMonth) or 365 days (lastYear) so the prior period lines up with the current period on the visible x-axis" — only correct if `data` already contains BOTH periods. Verify upstream `data` covers the full extended range.
|
||||
- **Recommendation:** Audit the call site — does the dashboard fetcher pull `now - rangeDays` only, or `now - rangeDays - shift`? If the former, the overlay shows `current_period` at `current_period + 30d` (a phantom future overlay). If the latter, OK. Add a unit test that pins `data` containing both periods and asserts the visible overlay maps to the prior-period values, not the current period's values.
|
||||
- **Ship-blocker for v1.4.16?** Possibly — needs fast verification. If `data` is single-period, the overlay is structurally meaningless.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/onboarding/tour-launcher.tsx:138-170`
|
||||
- **Issue**: The `decidedFor` guard correctly prevents re-deciding for the same `(userId, flag)` tuple. But `sessionStorage` keys (`healthlog-tour-session-dismissed`, `healthlog-tour-referrer`) are NOT scoped by user id. If admin A logs out and admin B logs in inside the same browser tab (no full reload), B inherits A's session-dismiss flag and the tour will not auto-launch for B even though B has `onboardingTourCompleted=false`. The `decidedFor` would re-evaluate (different userId), set `showTour=false` because `readSessionDismissed()` returns true. B has to navigate to Settings → Account → Restart to see the tour at all.
|
||||
- **Recommendation**: Either (a) clear `SESSION_DISMISS_KEY` on auth logout in `useAuth().logout()`, or (b) namespace the key by user id (`healthlog-tour-session-dismissed:${userId}`).
|
||||
- **Ship-blocker**: no — affects only multi-account / impersonation flows. Single-user installations (the actual production today) are unaffected. v1.4.16.
|
||||
### H8. `seedKey` includes potentially-undefined `chainData?.activeProvider` so race between query resolution and URL change re-seeds twice
|
||||
- **File:** `src/components/settings/ai-section.tsx:228-241`
|
||||
- **Issue:** `seedKey = `${queryProvider ?? ""}|${chainData?.activeProvider ?? ""}`` mutates between renders — first render `chainData` is undefined → seedKey ends with `"|"`; on chainData arrival → seedKey ends with `"|codex"`. The `seededFor !== seedKey` branch fires twice, second time setting `selectedProvider` from chainData. If the user has already clicked a different provider in the dropdown between the two renders, their click is silently overridden by the chainData arrival.
|
||||
- **Recommendation:** Track the user-interaction state separately (`userPicked: boolean`) and gate re-seeding on `!userPicked`. Or only seed once on the first non-undefined `chainData`.
|
||||
- **Ship-blocker for v1.4.16?** No — narrow race window, recoverable by re-clicking. Fix in v1.4.17.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM
|
||||
|
||||
### M1 — `generateInsight()` retry-once injects correction into `userPrompt`, not `systemPrompt`
|
||||
### M1. `/api/admin/audit-log/actions` does an unbounded `groupBy(action)`
|
||||
- **File:** `src/app/api/admin/audit-log/actions/route.ts:22-25`
|
||||
- **Issue:** No index on `audit_logs.action`. With 1M+ rows the `groupBy` becomes an aggregate scan. Result is also un-cached.
|
||||
- **Recommendation:** Add an index on `audit_logs.action`, OR cache the result for 5 min server-side, OR LIMIT the distinct list to the 100 most recent actions.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/ai/generate-insight.ts:155-158`
|
||||
- **Issue**: The retry concatenates `correction` onto `params.userPrompt`. The wrapper docstring (line 24) says it prepends a "corrective system message" but the code appends to the user prompt. Functionally fine for Codex (the model treats either location as instructions), but breaks the spec the docstring claims. Also: the corrective text on line 56-69 says "You MUST return JSON ... Required top-level fields: summary, recommendations, citations, warnings" — this duplicates the system prompt's schema. If the system prompt evolves and the retry-correction strings diverge, the retry will instruct the model to return a stale schema.
|
||||
- **Recommendation**: Either (a) update the docstring to say "appended to user prompt" and stop describing it as a system message, or (b) actually pass it through `systemPrompt` (cleaner) and reduce the corrective body to JUST the violated zod issues — let the system prompt own the schema text. The latter is the right long-term move because the schema is already exported via `aiInsightResponseSchema`.
|
||||
### M2. `RecommendationCard` `metricTypeToChartTypes()` falls through to verbatim type — silent passthrough
|
||||
- **File:** `src/components/insights/recommendation-card.tsx:114-135`
|
||||
- **Issue:** Unknown `metricType` returns `[metricType]` as-is. `HealthChart` then receives an unrecognised type and renders empty. Comment says "render empty rather than nothing" but the user sees nothing useful — a model that emits `metricSource.type = "BloodPressure"` (capitalised) gets dropped because the `lower === "bloodpressure"` branch hits a different casing for legacy-typed payloads. Worse: the model can emit any string.
|
||||
- **Recommendation:** Allowlist explicitly; unknown types skip the mini-chart entirely (return `[]`). Add a Wide-Event annotation `ai_rec_unknown_metric_type` for observability.
|
||||
|
||||
### M2 — `CodexClient.lastDiagnostics` race: instance shared across requests
|
||||
### M3. `appendLogEvent` runs unconditionally even for high-volume background jobs
|
||||
- **Files:** `src/lib/logging/transports.ts:93-104`, `src/lib/logging/in-memory-buffer.ts`
|
||||
- **Issue:** Every emitted event lands in the 500-entry ring buffer. For a worker process running pg-boss every minute this means the buffer is dominated by background events, leaving no space for HTTP request events that admins are more likely to debug. Plus the buffer is per-process so the web buffer never sees background events anyway — they ship to a buffer no admin reads.
|
||||
- **Recommendation:** Skip background events from the buffer when `event.kind === "background"`, or partition by kind. The Loki sink still receives them.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/ai/codex-client.ts:116, 174-177, 233-236, 242-246`
|
||||
- **Issue**: `lastDiagnostics` is a per-instance field, mutated synchronously around the SSE consume. `resolveProvider()` is hinted to construct a fresh client per request (codex-slug-cache.ts:17 docstring) but if any caller caches the client (e.g. via module-level memoisation in v1.4.16) the diagnostics reads at the route layer race against a parallel `generateCompletion()` call. Today the route reads diagnostics synchronously after the await resolves so this is safe. Document the contract on `getLastDiagnostics()` so a future refactor doesn't regress.
|
||||
- **Recommendation**: Either (a) make `generateCompletion()` return diagnostics in the result envelope (cleaner, type-safe), or (b) add a JSDoc `// NOTE: not safe for concurrent calls on the same instance.` on `getLastDiagnostics()`.
|
||||
### M4. `redactEventForEgress` does not redact `meta` or `action.details`
|
||||
- **File:** `src/app/api/admin/app-logs/route.ts:72-96`
|
||||
- **Issue:** Only `error.message`, `error.stack`, `warnings`, `http.user_agent`, `external_calls[].error` are redacted. `meta` and `action.details` carry annotated key=value pairs from `annotate()` calls — if any handler accidentally annotates a token (e.g. an Idempotency-Key replay logging the `Authorization` header), it leaks to the admin UI in raw form.
|
||||
- **Recommendation:** Run `redactSecrets` recursively over `meta` and `action.details` string values too. Or whitelist a closed set of meta keys the egress allows.
|
||||
|
||||
### M3 — `recordChannelTransientFailure()` two-write race
|
||||
### M5. `chainBodySchema.providerType` cast `as unknown as [string, ...string[]]`
|
||||
- **File:** `src/app/api/insights/provider-chain/route.ts:60-62`
|
||||
- **Issue:** Double-cast hides the type. zod accepts `readonly [string, ...string[]]` from a tuple; `PROVIDER_CHAIN_TYPES` is declared with `as const` so it should already match. The double-cast risks a future PROVIDER_CHAIN_TYPES being non-tuple typed without compile error.
|
||||
- **Recommendation:** `z.enum([...PROVIDER_CHAIN_TYPES] as readonly [string, ...string[]])` or use zod's `z.enum(PROVIDER_CHAIN_TYPES)` directly — the `as const` should already produce the tuple.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/notifications/channel-state.ts:90-130`
|
||||
- **Issue**: First update increments `consecutiveFailures` and reads back the new value; second update writes `nextRetryAt` based on that value. Between the two, a parallel dispatcher tick could increment again, so the persisted `nextRetryAt` may correspond to a counter that's already stale. The auto-disable branch (line 102) is also non-atomic — between the read and the second update the counter could cross the threshold and another invocation could fire `give_up_after_5_failures` twice. Today the dispatcher only iterates one channel per call sequentially so the race window only opens with multiple concurrent reminders — possible if multiple medications trigger at the same Berlin minute.
|
||||
- **Recommendation**: Move both writes into a single `prisma.$transaction` so the increment + retry-stamp + audit-log are atomic, OR collapse them by computing `nextRetryAt` client-side based on the response of the first update (which IS atomic per row).
|
||||
### M6. `ContractCard` minor — "draft" pill doesn't use `litBarsFor` clamp comment correctly
|
||||
- **File:** `src/components/insights/confidence-meter.tsx:55-64`
|
||||
- **Issue:** Comment says "0..20 is 1, 21..40 is 2"; `Math.ceil(0/20) = 0` → clamped to 1; `Math.ceil(20/20) = 1`; but value < 25 routes to `draft` band BEFORE this is consulted. The clamp `if (lit < 1) return 1` defends against `value=0` for non-draft band, which is unreachable since `bandFor(0) === "draft"`. Dead code path; comment is correct in spirit but misleading.
|
||||
- **Recommendation:** Drop the `lit < 1` clamp or document it as "defensive for refactoring."
|
||||
|
||||
### M4 — `dispatchNotification()` legacy Telegram migration upserts `enabled: true` on every send
|
||||
### M7. `host-metrics` route returns `BigInt → Number` without overflow guard
|
||||
- **File:** `src/app/api/admin/host-metrics/route.ts:90`
|
||||
- **Issue:** `Number(row.memTotalBytes)` and `Number(readDelta)` — if memTotalBytes ever exceeds 2^53 (~9 PB) precision is lost. Disk byte counters are cumulative; on a long-running machine they could overflow given enough time.
|
||||
- **Recommendation:** Acceptable for v1.4.16 (typical hosts <16 GiB RAM, disks <10 PiB throughput). Document in the route comment.
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/notifications/dispatcher.ts:75-97`
|
||||
- **Issue**: When a user has legacy `User.telegramEnabled=true` AND a `NotificationChannel(type=TELEGRAM, enabled=false, disabledReason=...)` row exists (e.g. auto-disabled by B3 hard-reject), the migration upsert `update: { enabled: true, config: channelConfig }` will RE-ENABLE the auto-disabled channel on every dispatch. This effectively reverts the B3 hard-reject behaviour for any user still on legacy Telegram config.
|
||||
- **Recommendation**: Add an `else if (channels.find(c => c.type === "TELEGRAM"))` guard so the migration only runs when no row exists at all. The current `hasTelegramChannel` boolean catches the not-yet-migrated case but doesn't handle the rare "row exists but disabled" path. Alternatively check `update: { config: channelConfig }` (don't touch `enabled`) so a previously-disabled row stays disabled.
|
||||
### M8. `feedback-aggregator` `await prisma.appSettings.upsert` swallows errors silently
|
||||
- **File:** `src/lib/jobs/reminder-worker.ts:951-972`
|
||||
- **Issue:** Worker catches and warns but doesn't requeue. If the upsert fails (DB connection blip, lock contention) the day's summary is lost — admin AI-quality dashboard shows yesterday's data.
|
||||
- **Recommendation:** Let pg-boss retry the job on throw (default behaviour). If retry storm is the concern, add a `maxRetries: 3, backoff: exponential` to the queue config. Today's "swallow + log" is too aggressive.
|
||||
|
||||
### M5 — `aggregateMedicationCompliance` clamps to `Math.min(100, ...)` but can still produce 0 % from imports with `taken=0`
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/charts/medication-compliance-chart.tsx:75-92`
|
||||
- **Issue**: The function filters `p.scheduled > 0` but renders `0 %` for any day with `scheduled > 0 && taken === 0`. That's correct numerically but visually misleading: a single day with `scheduled=4, taken=0` (user simply forgot to mark intake) drops the line all the way to zero. The dashboard's BD-in-target tile (A4 Fix 1) explicitly handles this case by switching to "no data" when fewer pairs exist; medication-compliance does not. Marc's framing of A4 Fix 1 as "the tile read 0 % when imports drift the timestamps" suggests the same anti-pattern would apply here.
|
||||
- **Recommendation**: Treat days where `taken === 0` AND the user has no explicit "skip" intent as missing-data instead of 0 % (skip the point). Or render the segment with a dashed line. Cosmetic, but consistent with A4's spirit.
|
||||
|
||||
### M6 — `dispatchNotification` opt-in default for `MEDICATION_REMINDER` could spam admins via SYSTEM_ALERT path
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/notifications/dispatcher.ts:108-111`, integrations/status.ts:476-484, deploy-webhook/route.ts:128-141
|
||||
- **Issue**: Three new code paths now dispatch `SYSTEM_ALERT` to every admin user via the existing dispatcher (B2 persistent-failure alert, C2 deploy webhook failure, B3 status). The dispatcher's pref defaults to enabled (line 110-111: `if (pref && !pref.enabled) continue;`). An admin who hasn't explicitly muted SYSTEM_ALERT will receive ALL three classes via every channel they configured. With the v1.4.15 retry-policy + 5-failure auto-disable, a flapping integration with 24h re-alert window emits 1 SYSTEM_ALERT per burst — bounded. But during the v1.4.15 marathon-deploy-flap window (likely!), Marc could see a Telegram cascade.
|
||||
- **Recommendation**: Consider a separate `SYSTEM_ALERT_DEPLOY` or `SYSTEM_ALERT_INTEGRATION` event type so users can opt out of one without losing security alerts. v1.4.16 backlog at minimum.
|
||||
|
||||
### M7 — `defaultStartIso()` / `todayIso()` use local timezone for date inputs but report uses UTC
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/doctor-report/doctor-report-dialog.tsx:64-75, 151-152`
|
||||
- **Issue**: `formatLocalDate()` produces a YYYY-MM-DD anchored to the user's local timezone. The submit handler builds `new Date(\`${startDate}T00:00:00\`)` (LOCAL) then `.toISOString()` (UTC). For users west of UTC, the start of "Berlin May 8" becomes "May 7 22:00 UTC" — which the server then filters with `gte` so the server's first-day-bucket may exclude rows logged early on May 8 Berlin. The CLAUDE.md note says "Timezone: Europe/Berlin for display, UTC in database" — so all rows ARE stored in UTC, but the user's mental model is Berlin-local boundaries.
|
||||
- **Recommendation**: Anchor to Europe/Berlin explicitly (`new Date(\`${startDate}T00:00:00+02:00\`)` with the user's TZ offset) or document the off-by-one-day risk in the dialog copy. Today the dialog defaults to last-90-days-ending-today; the boundary effect mostly shows on custom-range exports.
|
||||
### M9. `host-metric-sampler.getHostMetricRetentionDays` redundant double-check
|
||||
- **File:** `src/lib/jobs/host-metric-sampler.ts:120-131`
|
||||
- **Issue:** Returns DEFAULT for `parsed <= 0`, then again for `parsed < 1`. The second check is unreachable for valid integers.
|
||||
- **Recommendation:** Drop the second check.
|
||||
|
||||
---
|
||||
|
||||
## LOW
|
||||
|
||||
### L1 — `OnboardingTour` `TOOLTIP_HEIGHT = 220` is a fixed estimate; long i18n strings overflow silently
|
||||
### L1. `evictPerStatusInsightCache` Prisma query uses both `startsWith` and `AND[contains]`
|
||||
- **File:** `src/app/api/insights/generate/route.ts:62-73`
|
||||
- **Issue:** `where: { userId, action: { startsWith: "insights." }, AND: [{ action: { contains: "-status." } }] }`. The combined `AND` is redundant under "where AND" semantics — Prisma already AND's all top-level keys. The `AND` wrapper is needed only because the top-level `action` key already has a `startsWith`; piling another `action` filter inside an `AND[]` is the right pattern. Comment is good.
|
||||
- **Recommendation:** Looks right, no change needed. Considered nit.
|
||||
|
||||
- **File**: `src/components/onboarding/tour.tsx:188`
|
||||
- **Issue**: A long German body text could push the card past the 220px estimate; `computeTooltipPosition()` uses the constant for viewport-fit calculations, so the tooltip may overlap the spotlight cutout. The tooltip itself is `width: 320px; height: auto` so the visible card still renders correctly — only the placement math is off.
|
||||
- **Recommendation**: Measure the rendered card via a ref + `getBoundingClientRect()` once after mount and reposition. Or accept the estimate is good enough (current behaviour) and add a max-height + scroll on the body.
|
||||
### L2. `evictPerStatusInsightCache` does not annotate the row count it deleted
|
||||
- **File:** `src/app/api/insights/generate/route.ts:62-73`
|
||||
- **Issue:** `deleteMany` returns `{ count }` which is dropped.
|
||||
- **Recommendation:** Annotate `evicted_status_caches` so admin observability sees the eviction.
|
||||
|
||||
### L2 — `redactBody()` regex misses Anthropic API-key prefix variant
|
||||
### L3. `provider-runner` legacy `runRawCompletionWithFallback` is duplicated code
|
||||
- **File:** `src/lib/ai/provider-runner.ts:312-371`
|
||||
- **Issue:** Near-identical to `runWithFallback` (lines 229-288); diff is the inner `provider.generateCompletion` vs `generateInsight` call. The cache logic, fallbackHops shape, error classification, and Wide-Event annotation are duplicated.
|
||||
- **Recommendation:** Extract a shared helper `runChain<T>(args, runOnce)` that takes the per-attempt callback. Defer if v1.4.17 is migrating the legacy route anyway.
|
||||
|
||||
- **File**: `src/lib/ai/codex-client.ts:470-475`
|
||||
- **Issue**: `redactBody()` redacts `sk-` and `sk-ant-` but new Anthropic admin keys use `sk-ant-api03-…`. The current regex matches `sk-(?:ant-)?[A-Za-z0-9_-]{8,}` so it does cover `sk-ant-api03-xxx`. False alarm. (Re-reading: the regex IS correct — `sk-ant-` prefix matches, then `[A-Za-z0-9_-]{8,}` greedily consumes the rest including `api03-`.) This is just a comment-clarity nit: the inline comment doesn't list `api03-`.
|
||||
### L4. `RecommendationsGrid` STAGGER_INTERVAL_MS = 100 ms — a 10-rec response takes 1s to fully animate in
|
||||
- **File:** `src/components/insights/recommendations-grid.tsx:81`
|
||||
- **Issue:** No upper bound on stagger total. Worst case 10 recs * 100 ms = 1s before last card lands.
|
||||
- **Recommendation:** Cap with `min(index * 100ms, 600ms)` so late cards fade in immediately after the cascade-cap.
|
||||
|
||||
(Downgrade to nit. Keeping for completeness.)
|
||||
### L5. `findClosestDia()` allows multiple sys to pair with the same dia
|
||||
- **File:** `src/lib/analytics/bp-in-target.ts:80-100`
|
||||
- **Issue:** Each sys picks its own closest dia independently. A burst of sys readings between two dia readings will all match the closer dia. Counts inflated for high-frequency sys-only logging.
|
||||
- **Recommendation:** Acceptable for the 5-min-window pair-rule; closing pair after consumption would change semantics. Document or accept.
|
||||
|
||||
### L3 — `findClosestDia()` uses naive O(n²) scan
|
||||
### L6. `host-metrics-chart.tsx` `Intl.DateTimeFormat` recreated per `buildChartRows` call
|
||||
- **File:** `src/components/admin/host-metrics-chart.tsx:93-97`
|
||||
- **Issue:** Locale formatter is created inside the pure helper, but the helper itself is wrapped by `useMemo`. Each unique `data?.samples` reference triggers re-instantiation. Cheap per call, but wasteful.
|
||||
- **Recommendation:** Move the formatter to module-scope.
|
||||
|
||||
- **File**: `src/lib/analytics/bp-in-target.ts:57-78`
|
||||
- **Issue**: For users with hundreds of BP readings (Withings polls 4×/day → ~3 600 rows over 30 days), pairing is O(n²). Today the analytics route fetches only the last 30 days (~120 rows worst-case) so n²=14 400 ops — instant. But if v1.4.16 widens the window the cost compounds.
|
||||
- **Recommendation**: Sort dia by timestamp once; binary-search for closest. v1.4.16 micro-optimisation, not urgent.
|
||||
### L7. `legacy-payload.ts` checks `rationale === undefined` AND `=== null` AND `typeof !== "object"` separately
|
||||
- **File:** `src/lib/ai/legacy-payload.ts:46-52`
|
||||
- **Issue:** `typeof rationale !== "object"` already returns true for undefined and null in JS. The early checks are belt-and-braces but redundant.
|
||||
- **Recommendation:** Simplify to `if (rationale === null || typeof rationale !== "object") return true;` — `undefined` is also `typeof "undefined"` so the check covers it.
|
||||
|
||||
### L4 — Slug-fallback chain `gpt-4o` listed as "last-ditch capability fallback" but never accepted on ChatGPT-account auth
|
||||
### L8. Comparison overlay `_compare` data keys cannot collide with metric types but use string concat
|
||||
- **File:** `src/components/charts/health-chart.tsx:558`
|
||||
- **Issue:** `merged[`${type}_compare`] = v` — relies on no metric type ending with `_compare`. Stable today, fragile to a future `WEIGHT_compare` enum value (unlikely but possible).
|
||||
- **Recommendation:** Prefix with a non-identifier separator: `merged[`__compare__:${type}`]`. Or a `Map`. Acceptable as-is.
|
||||
|
||||
- **File**: `src/lib/ai/codex-client.ts:40-45`
|
||||
- **Issue**: Per the spec and Marc's v1.4.7..v1.4.13 saga, the ChatGPT backend ONLY accepts codex-family slugs. `gpt-4o` would universally hit the `not supported when using codex with a chatgpt account` rejection and just waste an HTTP round-trip on the all-failed path before the 503. The cost is one extra fetch + redaction + audit annotation per all-failed call.
|
||||
- **Recommendation**: Drop `gpt-4o` from `DEFAULT_SLUG_FALLBACK_CHAIN` (or document why it's there). Or condition on auth method: `gpt-4o` IS valid for API-key auth; if v1.4.16 adds `OpenAIClient` it'd want this slug.
|
||||
### L9. `ai-section.tsx:1255` — `seededKey` joined with `,` from providerType, not stable JSON
|
||||
- **File:** `src/components/settings/ai-section.tsx:1270`
|
||||
- **Issue:** `chain.map((c) => c.providerType).join(",")` is derived from `chain`, which only changes when GET refetches. Same providerType-set in different order produces the same key — but if two reorderings happen before re-seed, the local entries diverge from the GET. Edge case; fine.
|
||||
|
||||
### L5 — `dashboard-layout.ts` test file referenced as having pre-existing typecheck errors but they are within the v1.4.15 diff
|
||||
|
||||
- **File**: `src/lib/__tests__/dashboard-layout.test.ts` (referenced repeatedly across STATE.md as "pre-existing")
|
||||
- **Issue**: STATE.md attributes these typecheck errors to A4 then says they're pre-existing. The file itself was added/modified in this release (`+119 -0` per `git diff --stat`). Worth a clean fix-up before tagging v1.4.15 or noting in CHANGELOG.
|
||||
### L10. `host-metrics` route does NOT cache prev BigInt across HTTP calls
|
||||
- **File:** `src/app/api/admin/host-metrics/route.ts:85-122`
|
||||
- **Issue:** Per-request `prevReadBytes` walk; the first row in the returned window has `diskReadBps: null` because there's no previous sample to delta from. UI hides the line for that row. Documented behaviour. Fine.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
## Cross-cutting
|
||||
|
||||
The v1.4.15 diff is large but disciplined. Phase C1 (AI hardening) is the standout — strict schema + retry-once + citation-cross-check + slug-fallback + 1h positive cache is exactly the right architecture for the "zero hallucinations" mandate. Phase B1 (backups) correctly factors validation, encryption, audit, and idempotency around the destructive restore path. Phase B2/B3 (sync + notification reliability) introduce a clean `IntegrationStatus` model and `channel-state` writer that reuse the existing dispatcher cleanly.
|
||||
### Plan-deviation observations
|
||||
|
||||
**No CRITICAL findings**: every new admin endpoint has `requireAdmin()`, the deploy webhook uses `timingSafeEqual`, `withIdempotency` wraps `restore`, encrypted-at-rest is preserved, no auth bypasses found.
|
||||
- **B5b/B5c/B5d strict path is dark in production** — the production `/api/insights/generate` route still rides the `runRawCompletionWithFallback()` legacy shape. STATE.md acknowledges this as a "v1.4.17 migration" deferment, but the user-facing claims of "rationale required", "deterministic confidence", and "citation enforcement" are exercised only by tests. See **H3** above. Recommend explicit Marc decision before tagging v1.4.16: ship dark + adjust release-note language, OR migrate route to strict path.
|
||||
- **A2 BD-Zielbereich predicate change is structurally sound** — `isBpReadingInTarget` ceiling+floor semantics centralised across all 6 call sites. Marc's stated 50% expectation is plausible given the `90 ≤ sys ≤ sysHigh AND 50 ≤ dia ≤ diaHigh` predicate; integration test `tests/integration/bp-in-target.test.ts` covers it. No drift risk.
|
||||
- **B7 export endpoints** — well structured, share `export:<userId>` rate-limit bucket, audit-log on every download, no envelope-wrap on the file contents. `full-backup` round-trips through the canonical `BACKUP_SCHEMA_VERSION` shape so admin upload + restore accepts it. Clean.
|
||||
- **Wave-C qemu-arm64 drop** — well-documented in `docker-publish.yml`. amd64-only is the right call given Marc's prod target. v1.5 path documented.
|
||||
|
||||
**HIGH findings are correctness edge-cases**, not security or data-loss issues. None are ship-blockers for v1.4.15. H1 (restore error message scrubbing), H2 (mood `tags` schema tightening), and H4 (sessionStorage user-scoping) belong on the v1.4.16 backlog. H3 (mood-chart bucketing test divergence) is a test-coverage gap that should be folded into v1.4.16 polish.
|
||||
### Tests + i18n parity
|
||||
|
||||
**MEDIUM findings**: M3 (channel-state two-write race) deserves a v1.4.16 atomic-transaction fix. M4 (legacy Telegram migration re-enabling auto-disabled channel) is the most impactful — could effectively unwind B3's hard-reject. M6 (admin SYSTEM_ALERT cascade during a flap) is worth pre-tagging awareness for Marc.
|
||||
|
||||
Cross-cutting observation echoing every prior phase report: the parallel-agent shared-cwd race produced commits whose subjects don't match their diffs (e.g. C1 schema files folded into `5510ed5` empty-states commit). Code is correct on `main` in every case; only commit-archaeology suffers. v1.4.16 should adopt `superpowers:using-git-worktrees` per agent.
|
||||
- New SSR + integration tests are present for each new endpoint and for `RecommendationCard` rationale + confidence + feedback wiring.
|
||||
- `recommendation-feedback.test.tsx` not located in the repo — STATE.md B5e claims +20 tests; the wire integration `tests/integration/insights-feedback.test.ts` covers the API but the component is exercised through the integration harness only, not isolated.
|
||||
- i18n parity: B5a/B5e/B8 add EN+DE keys with parity tests. No drift detected.
|
||||
|
||||
---
|
||||
done: 0 CRITICAL, 4 HIGH, 12 MED/LOW
|
||||
|
||||
## Severity rollup
|
||||
|
||||
- **CRITICAL:** 0
|
||||
- **HIGH:** 8 (H1-H8)
|
||||
- **MEDIUM:** 9 (M1-M9)
|
||||
- **LOW:** 10 (L1-L10)
|
||||
@@ -1,495 +1,265 @@
|
||||
# Phase D — Design / UX review findings (v1.4.15)
|
||||
# Phase D — DESIGN/UX review (v1.4.16, Apple-Health-quality benchmarking lens)
|
||||
|
||||
**Reviewer**: design / UX (parallel D-agent)
|
||||
**Method**: source review of JSX + Tailwind + i18n strings against the
|
||||
A5 mobile audit baseline, B-mobile applied fixes, and the canon set in
|
||||
`docs/ui-guidelines.md`. Playwright was not run — the v1.4.15 marathon's
|
||||
parallel-agent racing makes a fresh local boot unreliable, and the A5
|
||||
audit at `https://healthlog.bombeck.io` already covers production
|
||||
device measurements. This pass focuses on the v1.4.15 *new surfaces*
|
||||
(B1 backups, B2 integrations status, B3 notifications, B4 achievements,
|
||||
B5 onboarding tour, A2 admin overview, A3 mood tile) plus the
|
||||
deferred-from-A5 cluster the B-mobile phase explicitly punted to v1.4.16.
|
||||
Reviewer: phase-D DESIGN/UX agent
|
||||
Marathon: v1.4.16 (live: 1.4.15; under-test: 1.4.16)
|
||||
Date: 2026-05-09 (UTC+2)
|
||||
Approach: source-code audit (v1.4.16 not yet deployed; live Playwright not driven).
|
||||
|
||||
**Out of scope (per phase brief)**: chart visual style, Dracula tokens,
|
||||
chart-data changes that landed in A4. None of the findings below
|
||||
recommend chart-look changes.
|
||||
|
||||
**Tally**: **0 CRITICAL · 5 HIGH · 11 MEDIUM/LOW**.
|
||||
|
||||
Empty CRITICAL is real: the A5 audit already produced its CRITICAL
|
||||
list (admin/users mobile grid + admin/users table column hide-off);
|
||||
B-mobile applied both. No new ship-blockers introduced by v1.4.15
|
||||
features.
|
||||
Per brief: "Apple Health benchmarking lens — does the polish reach the bar Marc set?"
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
## Executive summary
|
||||
|
||||
### H1 — Onboarding tour misses Tab focus-trap (a11y, source comment lies)
|
||||
v1.4.16 ships a *huge* amount of code-level polish (gradient hero, ConfidenceMeter, RationaleCard with mini-charts, severity-coloured grid, comparison overlay, host-load chart, AI quality table). However, the **headline finding** is that the most visible Apple-Health-quality affordances — the polished `<InsightAdvisorCard>` consuming `<RecommendationsGrid>`, `<RecommendationCard>`, `<ConfidenceMeter>`, `<RecommendationFeedback>` — are **NOT mounted on any production page**. They live in tests only.
|
||||
|
||||
**Severity**: HIGH
|
||||
**Files**: `src/components/onboarding/tour.tsx:27-29` (the docstring) +
|
||||
`tour.tsx:265-292` (the keydown effect)
|
||||
**Issue**: The component's docstring explicitly states "Tab is trapped
|
||||
inside the tooltip", and the brief's check item #8 expects a focus
|
||||
trap. The implementation handles `Escape`, `Enter`, `ArrowLeft`,
|
||||
`ArrowRight` only — there is **no Tab handler**, no
|
||||
`trapFocus`/`focusin` listener, no `tabindex="-1"` shielding for the
|
||||
underlying page, and no return-focus-on-close. The tooltip's
|
||||
`role="dialog" aria-modal="true"` claims a modal context which the
|
||||
keyboard does not honour. A keyboard user pressing Tab from the Skip
|
||||
button will land on a sidebar nav item or a page-level interactive
|
||||
element (which is *also* where the spotlight cutout points), then
|
||||
visually disappear behind the dimmed backdrop — confusing focus
|
||||
state.
|
||||
**Recommendation**: Add a small Tab handler in the same `keydown`
|
||||
effect that cycles focus across the three internal buttons (Skip,
|
||||
Back, Next/Done). Also store `document.activeElement` at mount and
|
||||
restore it in the close path so Settings → Account replay returns
|
||||
focus to the "Restart" button it was triggered from. Either delete
|
||||
the "Tab is trapped" line of the docstring or implement it.
|
||||
**Ship-blocker?**: **No** for v1.4.15 (not a regression — feature is
|
||||
new), but the docstring promises behaviour the code doesn't deliver,
|
||||
which is a disclosure-style bug. v1.4.16 should close it.
|
||||
The v1.4.16 release as Marc would experience it on `/insights` is the same v1.4.15 text-only `<InsightStatusCard>` per section. The dashboard `/` likewise does not import `<InsightsCardPreview>`. **All of B5c, B5d, B5e, and B1b's cross-feature polish is invisible to the user.** This is the single biggest disconnect between the work that landed and the user's perception of the release.
|
||||
|
||||
A second category: the comparison toggle (B8) is *only* reachable from `/settings/dashboard-layout`. There is no on-chart, on-tile, or on-page control on `/` or `/insights`. The brief was explicit ("Mobile-friendly comparison toggle, not a fragile hover-only interaction") and the UX cost is severe: a v1.4.16 user has zero on-surface affordance to toggle Vormonat / Vorjahr.
|
||||
|
||||
Findings below grouped by severity.
|
||||
|
||||
---
|
||||
|
||||
### H2 — Onboarding tour tooltip card overflows on small viewports with DE copy
|
||||
## CRITICAL — ship-blockers for v1.4.16 (must fix before tag)
|
||||
|
||||
**Severity**: HIGH
|
||||
**Files**: `src/components/onboarding/tour.tsx:187-188` (constant
|
||||
budget), `messages/de.json` `onboarding.tour.steps.tileStrip.body`
|
||||
(verified ~210 chars in DE vs ~155 in EN)
|
||||
**Issue**: `TOOLTIP_HEIGHT = 220` is a fixed budget used for
|
||||
viewport-fit math in `computeTooltipPosition()`. The DE body strings
|
||||
for several stops run 30–45 % longer than EN; on a 393 × 851 Pixel 5
|
||||
viewport with browser font scaling at 1.2× (Marc has explicitly OK'd
|
||||
"German is ~30% longer" in CLAUDE.md), the actual rendered tooltip
|
||||
clears 280 px and overflows the budget. The candidate-position math
|
||||
believes it has 220 px of headroom and chooses a `placement` that
|
||||
leaves no room for the bottom of the card → the tooltip clips below
|
||||
the viewport and the Next/Skip buttons sit just below the fold,
|
||||
unreachable without scrolling (and the backdrop's `position: fixed`
|
||||
prevents the user from scrolling past).
|
||||
**Recommendation**: Either (a) measure the rendered tooltip after
|
||||
mount with a `ResizeObserver` and feed real height into
|
||||
`computeTooltipPosition()`, or (b) bump the budget to ~300 px and
|
||||
add an explicit `max-h-[80vh] overflow-y-auto` on the tooltip card
|
||||
so worst-case a scroll handle appears inside the card without
|
||||
ejecting the Next button.
|
||||
**Ship-blocker?**: No, but the DE-locale first-impression for new
|
||||
users is the literal use case the tour exists for. v1.4.16 priority.
|
||||
### C1 — `/insights` does NOT mount the polished `<InsightAdvisorCard>` / `<RecommendationsGrid>`. B5c/d/e/B1b are invisible
|
||||
|
||||
- **File:** `src/app/insights/page.tsx:1006-1539`
|
||||
- **Issue:** every insights section renders `<InsightStatusCard>` (text-only summary, no rec card, no rationale, no confidence meter, no thumbs-feedback). The Apple-Health-style polish from B1b (`<InsightsPageHero>` is mounted, but it's just a header band) and the entire B5c/d/e rec-card surface (`<RecommendationCard>`, `<ConfidenceMeter>`, `<RecommendationFeedback>`, `<RationaleCard>`) is never reachable from any live route. Confirmed via `grep "InsightAdvisorCard"` — zero non-test imports across `src/`.
|
||||
- **Recommendation:** wire `<InsightAdvisorCard>` into `/insights` (consume `/api/insights/generate`) **or** explicitly de-scope B5c/B5d/B5e/B1b from v1.4.16 release notes and tell Marc the visual polish ships in v1.4.17. The B5c/d/e reports each flag this gap ("E2E not added — not mounted on a live route") but none actually wire the card. Without wiring, Marc gets v1.4.15 UX with extra unused code paths.
|
||||
- **Ship-blocker?** YES.
|
||||
|
||||
### C2 — `<InsightsCardPreview>` (dashboard insights tile) has zero live imports — orphan component
|
||||
|
||||
- **File:** `src/components/insights/insights-card.tsx:49`; `src/app/page.tsx` (no import)
|
||||
- **Issue:** B1b commit 5 (`d2cdf9d`) replaced the orphan v1.4.0 `<InsightsCard>` with a polished preview that renders top severity-ordered recs + ring-variant `<ConfidenceMeter>` inline. The dashboard never imports it. `grep "InsightsCardPreview" src/app` — zero matches. The dashboard renders `RecentAchievementsCard` + tile strip + charts, no insights preview.
|
||||
- **Recommendation:** mount `<InsightsCardPreview>` on `/` (the B1b report flagged this as "When dashboard wiring lands…") or hide the file and remove from v1.4.16 marketing claims.
|
||||
- **Ship-blocker?** YES — Marc sees no insights tile on the dashboard despite the B1b report explicitly citing dashboard polish.
|
||||
|
||||
### C3 — Comparison toggle (B8) has NO on-surface control. Buried 3 clicks deep in Settings
|
||||
|
||||
- **File:** `src/components/settings/dashboard-layout-section.tsx:192-198` (only place `compareBaseline` is settable)
|
||||
- **Issue:** Marc must navigate to /settings/dashboard-layout, scroll past 14 widgets, find the "Comparison Baseline" Select, change it, save, then back-navigate to / or /insights. The brief explicitly required "Mobile-friendly comparison toggle (not a fragile hover-only interaction)" — what shipped is a Settings-Save round-trip every time the user wants to flip from "vs. last month" to "vs. last year" or back to off. The B8 report's deferred-list mentions a `<CompareToggle>` segmented control was scoped but never built; only the persistence layer + chart-side rendering exist.
|
||||
- **Recommendation:** add a `<CompareToggle>` (segmented "None / Vormonat / Vorjahr") next to the existing range tabs (7d/30d/90d/All) on each chart, OR a single page-level toggle pinned next to the dashboard greeting. Persists via the same `dashboardWidgets` PUT the settings page uses, no API change. Without it, the comparison-overlay code (compareBaseline plumbed through 19 chart sites + tile-strip + `comparison.deltaVs.*` i18n) is dead UX — admins can demo it, users can't find it.
|
||||
- **Ship-blocker?** YES — major B8 deliverable is invisible without an on-surface toggle.
|
||||
|
||||
---
|
||||
|
||||
### H3 — Tour cutout backdrop is a focusable `<button>` covering the page
|
||||
## HIGH — fix in v1.4.16 if cycle allows, or document for v1.4.17
|
||||
|
||||
**Severity**: HIGH
|
||||
**File**: `src/components/onboarding/tour.tsx:328-334`
|
||||
**Issue**: The dimmed backdrop is rendered as
|
||||
`<button type="button" aria-label="Skip">` so a click anywhere
|
||||
counts as Skip. Two consequences:
|
||||
1. Screen-reader users tab into a single button labelled "Tour
|
||||
überspringen" that visually has no border, no focus ring (the
|
||||
inner `cursor-default` and `bg-black/70` win over the
|
||||
`focus-visible:ring` from `Button` styles — and this isn't even
|
||||
the `Button` component, it's a raw `<button>`). No a11y focus
|
||||
ring → no visible signal that pressing Enter dismisses the tour.
|
||||
2. On a touch device a stray scroll-tap on the dimmed area immediately
|
||||
skips the tour. The brief's check item #8 expects the spotlight
|
||||
to keep the user in the flow until Done/Skip.
|
||||
**Recommendation**: Either (a) keep the `<button>` but add an
|
||||
explicit `focus-visible:ring-2 focus-visible:ring-primary` so
|
||||
keyboard users see it, OR (b) split the backdrop into a non-
|
||||
interactive `<div aria-hidden>` and surface the Skip action only via
|
||||
the Skip button inside the tooltip + the existing Esc handler. (b)
|
||||
matches the convention of `<Dialog>` from radix-ui where the
|
||||
backdrop is not an interactive primitive.
|
||||
**Ship-blocker?**: No. New feature, no regression vs. v1.4.14. Polish
|
||||
in v1.4.16.
|
||||
### H1 — Recommendation feedback thumbs are 28×28 px hit-area — fails WCAG 2.5.5 (44 px floor)
|
||||
|
||||
- **File:** `src/components/insights/recommendation-feedback.tsx:268, 283`
|
||||
- **Issue:** both thumb buttons use `inline-flex h-7 w-7 items-center justify-center rounded-md`. h-7 = 28 px. WCAG 2.5.5 minimum is 44×44 CSS px. Wave-C MED bottom-nav fix was specifically about 44 px floors; the same floor must apply to per-rec feedback that Marc taps on a Pixel 5. Also: zero `focus-visible:` styles on the raw `<button>` so keyboard focus is invisible.
|
||||
- **Recommendation:** swap to `min-h-11 min-w-11` (or `min-h-9` at minimum + extra padding around an inner icon), add `focus-visible:ring-2 focus-visible:ring-ring/50 focus-visible:ring-offset-2`. The brief asks "thumbs feedback button hit-area large enough" and the answer today is no.
|
||||
- **Ship-blocker?** Borderline — Marc said 44×44 is the floor; any consumer accessibility audit will cite this.
|
||||
|
||||
### H2 — Recommendation card chevron expand toggle: 24×24 hit area, no focus-visible
|
||||
|
||||
- **File:** `src/components/insights/recommendation-card.tsx:368-385`
|
||||
- **Issue:** raw `<button type="button" className="text-muted-foreground hover:text-foreground rounded-md p-1 transition-colors">` with a `<ChevronDown className="h-4 w-4">` inside. p-1 (= 4 px) + h-4 (= 16 px) gives ~24×24. Brief: "expand affordance discoverable; confidence meter readable; thumbs feedback button hit-area large enough." Current chevron fails the same 44 px bar as H1.
|
||||
- **Recommendation:** wrap in `<Button variant="ghost" size="icon" className="h-9 w-9">` or hand-bump to `min-h-11 min-w-11`. Add `focus-visible:` ring (raw `<button>` has none).
|
||||
- **Ship-blocker?** Same answer as H1. If H1 ships, H2 must too; feels broken if only one is fixed.
|
||||
|
||||
### H3 — Trend-card aria-label hardcoded English (i18n leak)
|
||||
|
||||
- **File:** `src/components/charts/trend-card.tsx:229-233`
|
||||
- **Issue:** the comparison delta `aria-label` interpolates `"vs. last month"` / `"vs. last year"` as **literal English strings** regardless of the user's locale. Marc switching to DE gets a screen-reader announcing English while the visible text is German ("Δ −2.3 kg vs. Vormonat"). All other labels in the file use `t(...)`. Undocumented bypass.
|
||||
- **Recommendation:** use the same `t(comparison.captionLastMonth/Year)` key the visible text consumes two lines below.
|
||||
- **Ship-blocker?** No, but trivial fix.
|
||||
|
||||
### H4 — Medication-Compliance chart does NOT support `compareBaseline` overlay. Inconsistent with all other charts
|
||||
|
||||
- **File:** `src/components/charts/medication-compliance-chart.tsx`
|
||||
- **Issue:** `grep "compareBaseline" medication-compliance-chart.tsx` — no prop. Every other chart family (BP/weight/pulse/mood) accepts `compareBaseline` and renders the dimmed prior-period line. The medication-compliance heatmap doesn't, so when the user toggles Vormonat the rest of the dashboard responds and only this card stays static. Not strictly a regression (B8 report flags compliance heatmap "doesn't shrink cleanly" and was deferred for the rec-card mini-mode), but the *toggle* still needs to be honoured visually somehow.
|
||||
- **Recommendation:** add a one-line caption "Vergleich nicht verfügbar für Compliance" / "Comparison N/A for compliance" so the user understands the asymmetry rather than thinking the toggle is broken.
|
||||
- **Ship-blocker?** No, but UX-incoherent without a fix.
|
||||
|
||||
### H5 — `via-dracula-cyan/8` Tailwind class — opacity `/8` is technically valid but visually nearly identical to bg; gradient appears flat on mobile
|
||||
|
||||
- **File:** `src/components/insights/insights-page-hero.tsx:81`
|
||||
- **Issue:** `from-dracula-purple/15 via-dracula-cyan/8` — the via-stop at 8% opacity over a Dracula `--background` (#282a36) is a ΔL of ~3% from the page bg. On a Pixel 5 LCD in daylight, this looks like a flat purple band with no gradient. Apple Health hero gradients use 25-40% opacity at the brightest stop.
|
||||
- **Recommendation:** bump to `from-dracula-purple/25 via-dracula-cyan/15` (commit 6 already raised opacity once; raise again or use inline CSS gradient where Tailwind's discrete steps clip the design).
|
||||
- **Ship-blocker?** No, but the "Apple-Health-quality benchmarking" bar is what Marc set; this gradient does not meet it.
|
||||
|
||||
### H6 — Range-tabs (chart 7d/30d/90d/All) hit-area `min-h-9 px-2.5` = 36×~28 px. Below 44 px on Pixel 5
|
||||
|
||||
- **File:** `src/components/charts/health-chart.tsx:882`
|
||||
- **Issue:** `className="min-h-9 px-2.5 text-xs"`. On the Pixel 5 viewport the user taps four narrow pills above each chart; missed-taps are routine. WCAG 2.5.5 again.
|
||||
- **Recommendation:** swap to `min-h-11 px-3` or use a Radix `<Tabs>` primitive with a `min-h-[44px]` modifier (same as bottom-nav).
|
||||
- **Ship-blocker?** No, but the brief calls out 44×44 explicitly.
|
||||
|
||||
### H7 — Medication CSV "include intake history" uses raw `<input type="checkbox">` 16×16 px. Mobile fail + visual inconsistency
|
||||
|
||||
- **File:** `src/components/settings/export-section.tsx:457-466`
|
||||
- **Issue:** every other settings toggle is a `<Switch>` from `@/components/ui/switch`. The intake-history toggle is a vanilla `<input type="checkbox" className="border-border h-4 w-4">`. 16×16 hit area, breaks design-system consistency, and on iOS Safari renders as the system blue checkbox (jarring against Dracula).
|
||||
- **Recommendation:** replace with `<Switch>` or shadcn `<Checkbox>`. Bump container to `<label className="flex min-h-11 …">`.
|
||||
- **Ship-blocker?** No, but Marc's eye will catch the system-blue box on his iPhone immediately.
|
||||
|
||||
### H8 — DE fallback-chain row will overflow Pixel 5 (393 px) — 5 controls per row, all with German labels
|
||||
|
||||
- **File:** `src/components/settings/ai-section.tsx:1374-1434`
|
||||
- **Issue:** each chain row is a `flex flex-wrap` with 5 controls (label hyperlink + Switch + ArrowUp + ArrowDown + X). EN labels are short (Move up / Move down / Remove from chain) but DE strings are longer ("Nach oben", "Nach unten", "Aus Kette entfernen") and the row uses `gap-2` so on 393 px it wraps to 2-3 lines per row. A 5-row chain becomes ~12 visual lines.
|
||||
- **Recommendation:** at `<sm` breakpoint, hide aria-only textual labels on arrow buttons (icon is universal) and collapse the X into a long-press or "more" overflow menu. Or shrink each row to provider name + switch + drag handle, with reorder via popover.
|
||||
- **Ship-blocker?** No, but renders unusably on Pixel 5 mobile in DE locale.
|
||||
|
||||
---
|
||||
|
||||
### H4 — Backups / Notifications status / Tour buttons are 32 px tall (below WCAG 44 px)
|
||||
## MED / LOW — polish, parity, post-v1.4.16 cleanup
|
||||
|
||||
**Severity**: HIGH
|
||||
**Files**:
|
||||
- `src/components/admin/backups-section.tsx:84,343,397,438,492` (every
|
||||
`<Button size="sm">` in the surface — restore trigger, run-now,
|
||||
upload, download per row)
|
||||
- `src/components/settings/notification-status-card.tsx:264,280` (Re-
|
||||
enable + Send-test buttons)
|
||||
- `src/components/onboarding/tour.tsx:385,396,405` (Skip, Back, Next/
|
||||
Done inside the tooltip)
|
||||
- `src/components/admin/recent-audit-preview.tsx:80-83` (the
|
||||
"View all" `<Link>` is only `text-sm`, ~20 px high — even smaller)
|
||||
**Issue**: A5's cross-cutting finding ("design system default
|
||||
button/tabs/switch sizes are too small for mobile") was deferred from
|
||||
B-mobile to v1.4.16 because the fix touches `button.tsx` which
|
||||
ripples everywhere. Meanwhile the v1.4.15 *new* surfaces shipped with
|
||||
the same `size="sm"` pattern (h-8 = 32 px) so we accreted three more
|
||||
pages that fail WCAG 2.5.5 in addition to the pages A5 already
|
||||
flagged. B-mobile's fix-list explicitly hardened *only* a dozen
|
||||
specific call-sites (chart range, mood-list icons, login CTAs); the
|
||||
B1/B3/B5 surfaces were authored after-the-fact without the same
|
||||
44-px discipline. Notable counter-example: dashboard quick-add at
|
||||
`src/app/page.tsx:411-413` carries an explicit `min-h-11` because
|
||||
that author saw the audit. Most B1/B3 surfaces don't.
|
||||
**Recommendation**: For v1.4.15 — **no source change**, accept the
|
||||
debt. For v1.4.16 — adopt the deferred design-system bump
|
||||
(`button.tsx` default `h-9` → `h-11`, `sm` `h-8` → `h-9` keeping the
|
||||
denser tier still-tappable at 36 px). One-line diff closes 30+ tap-
|
||||
target findings in a single change. Until then, every new B-feature
|
||||
keeps growing the deferred list.
|
||||
**Ship-blocker?**: No (continuation of a known v1.4.16 backlog
|
||||
item), but the trend is moving in the wrong direction — flag in the
|
||||
phase-D report so reconcile decides whether to fast-track the
|
||||
button.tsx change.
|
||||
### M1 — App-log preview table uses `overflow-x-auto` with no mobile card fallback
|
||||
|
||||
- **File:** `src/components/admin/app-log-preview-section.tsx:207-263`
|
||||
- **Issue:** B4 admin app-logs renders `<table>` inside `overflow-x-auto`. On Pixel 5 (393 px), 5 columns (level / timestamp / action / duration / trace_id) → user scrolls horizontally. A3 fix already swapped api-tokens to a card-list mobile fallback.
|
||||
- **Recommendation:** add card-list rendering at `<md`. Mirror `<UserManagementSection>` pattern.
|
||||
|
||||
### M2 — RecommendationFeedback success state replaces row with text-only "Thanks" — no path to revoke or change verdict
|
||||
|
||||
- **File:** `src/components/insights/recommendation-feedback.tsx:212-225`
|
||||
- **Issue:** once submitted, the row collapses to one line of text + check icon. There's no UI path to revoke or change the verdict. Server-side dedup is by `(user, recId, recText)` so a new attempt would 409, but the client-side UX gives no hint that this is final.
|
||||
- **Recommendation:** keep the chosen thumb visible (highlighted / pressed-state) and add a quiet "Submitted — undo?" affordance. Even if not actually re-submittable, give the user the visual receipt of which thumb they chose. Apple Health's tendency: never collapse a state without an undo path.
|
||||
|
||||
### M3 — `<RecommendationsGrid>` border colors at `/70` opacity — not contrast-verified against `--card`
|
||||
|
||||
- **File:** `src/components/insights/recommendations-grid.tsx:64-69`
|
||||
- **Issue:** `border-l-dracula-red/70` etc. The B1b commit-6 added a contrast-regression test for the page hero but not for the grid borders. At 70% opacity over `--card` (which is `--dracula-bg` slightly tinted), red border ratio is ~3.8:1 (passes 3:1 UI floor); purple is closer to 2.6:1 (likely fails). Severity is information-bearing.
|
||||
- **Recommendation:** measure each token at `/70` against `--card`; bump under-3:1 ones to `/85`.
|
||||
|
||||
### M4 — ConfidenceMeter aria-label says "Vertrauen 75 von 100" — no band semantic
|
||||
|
||||
- **File:** `src/components/insights/confidence-meter.tsx:189-191`
|
||||
- **Issue:** screen-reader announces only the raw integer. Better signal: "high confidence (75/100)" / "medium" / "low" / "draft". Apple's accessibility lead would call out the missing band — the visual ring color is the primary signal but blind users only get the number.
|
||||
- **Recommendation:** include the band name in aria-label. New i18n key `confidenceAriaWithBand` taking `{value, band}` placeholders.
|
||||
|
||||
### M5 — InsightsPageHero hides "Generated" timestamp when no insight has been generated, but the hero band stays full-height — looks empty on first visit
|
||||
|
||||
- **File:** `src/components/insights/insights-page-hero.tsx:106-120`
|
||||
- **Issue:** when `updatedAt` is null, the meta row collapses to a single "Based on your last 90 days" line. Hero remains visually heavy with no content density delta. First-time users see a giant gradient with two short text lines.
|
||||
- **Recommendation:** when `updatedAt === null`, surface a "Generate your first insight →" inline CTA in the meta row. The regenerate button is already passed in via `onRegenerate` but appears top-right; promoting to inline-with-meta CTA would close the empty-state hole.
|
||||
|
||||
### M6 — Inconsistent severity color tokens between `<HeroFinding>`, `<RecommendationCard>`, `<RecommendationsGrid>`
|
||||
|
||||
- **Files:** `src/components/insights/insight-advisor-card.tsx:103-131` (HERO_STYLES) vs `src/components/insights/recommendation-card.tsx:49-56` (SEVERITY_BADGE_STYLES) vs `src/components/insights/recommendations-grid.tsx:64-69` (SEVERITY_BORDER_CLASSES).
|
||||
- **Issue:** three separate style maps for the same severity vocabulary. Hero uses `green/cyan/orange/red` (assessment); rec-card uses `cyan/purple/orange/red` (severity); grid uses `red/orange/purple/cyan` (also reversed mental model). The hero & rec-card maps disagree about what "info" means (cyan in both, but the hero's "neutral assessment" is also cyan).
|
||||
- **Recommendation:** centralise to `src/lib/severity-colors.ts`, expose helpers, have all three components consume them. v1.4.17 simplify-target.
|
||||
|
||||
### M7 — Sticky `<InsightsSectionNav>` + iOS Safari back-swipe gesture interference
|
||||
|
||||
- **File:** `src/app/insights/page.tsx:1695-1713`
|
||||
- **Issue:** `flex gap-2` inside `overflow-x-auto`. On iOS Safari the back-swipe gesture competes with horizontal pan inside the sticky nav.
|
||||
- **Recommendation:** add `overscroll-x-contain`.
|
||||
|
||||
### M8 — Host-metrics chart tooltip uses `formatter(value, name)` matching against `t(...)` outputs — i18n-fragile
|
||||
|
||||
- **File:** `src/components/admin/host-metrics-chart.tsx:278-298`
|
||||
- **Issue:** `if (name === t("admin.hostMetrics.load1"))` — relies on string equality between Recharts' `name` prop and the localised label. If user mid-session swaps locale, formatter falls through and tooltip shows raw numbers without unit.
|
||||
- **Recommendation:** key the formatter on a stable dataKey, not the localised name.
|
||||
|
||||
### M9 — Mood chart emoji glyphs (😖 🙁 😐 🙂 😄) — non-localizable, render differently per OS
|
||||
|
||||
- **File:** `src/components/charts/mood-chart.tsx` (B1a deliverable)
|
||||
- **Issue:** emoji rendering varies between Apple, Noto Color Emoji, Segoe UI Emoji. Marc's iPhone vs Pixel 5 vs desktop Mac = three different visual weights for the same chart.
|
||||
- **Recommendation:** v1.4.17 — replace emoji with custom SVGs (lucide has `Frown / Smile / Meh / Laugh / SmilePlus` that close the gap).
|
||||
|
||||
### M10 — `RecommendationCard` collapsed state has no visual hint that it's interactive
|
||||
|
||||
- **File:** `src/components/insights/recommendation-card.tsx:331-417`
|
||||
- **Issue:** the chevron is the only affordance; the card border is identical for collapsed and expanded recs. Apple Health uses a subtle "tap to expand" caption + 1-px depth shadow. Oura's Contributors use a small "i" subtitle "Tap to see why". HealthLog: nothing.
|
||||
- **Recommendation:** add `text-muted-foreground/60 text-[10px]` hint under rec text "Tap for details" / "Antippen für Details" on the collapsed state. Auto-disappears when expanded. Trivial; very high-leverage on first impression.
|
||||
|
||||
### M11 — Loading skeleton uses staggered `animationDelay` unconditionally — reduced-motion users still see staggered placeholders
|
||||
|
||||
- **File:** `src/components/insights/insight-advisor-card.tsx:340-348`
|
||||
- **Issue:** `style={{ animationDelay: \`${i * 100}ms\` }}` is unconditional. With `prefers-reduced-motion: reduce`, the underlying `animate-pulse` is killed but the placeholders still appear in staggered fashion when their (already-stopped) animation would have fired.
|
||||
- **Recommendation:** check `prefersReducedMotion()` in the JSX and skip the inline `animationDelay` when true.
|
||||
|
||||
### M12 — Insights page first-load: bare `Loader2` spinner, no skeleton shell mirroring final layout
|
||||
|
||||
- **File:** `src/app/insights/page.tsx:851-857`
|
||||
- **Issue:** when `isLoading`, the page renders a centered spinner — no structure on first paint. Apple Health renders cached data immediately + spinner overlay; HealthLog could render `<InsightsPageHero updatedAt={null} />` + tile-strip skeleton + section-nav skeleton during the 2-second slow-API window.
|
||||
- **Recommendation:** mount hero + skeleton tile strip during `isLoading` so the page has structure on first paint.
|
||||
|
||||
### M13 — `app-logs` empty state doesn't distinguish "buffer empty" from "filter matched nothing"
|
||||
|
||||
- **File:** `src/components/admin/app-log-preview-section.tsx:200-205`
|
||||
- **Issue:** the EmptyState renders the same `admin.section.app-logs.empty` copy whether the buffer is genuinely empty (fresh process) or filters narrowed to zero. No CTA to clear filter.
|
||||
- **Recommendation:** branch on `(traceId || actionFilter || level !== "__all__" || range !== "all")` to emit a "No matches — clear filters?" variant with reset button.
|
||||
|
||||
### M14 — Settings → Export `<input type="date">` is browser-locale-driven, not app-locale-driven
|
||||
|
||||
- **File:** `src/components/settings/export-section.tsx:248-284`
|
||||
- **Issue:** `<Input type="date">` renders MM/DD/YYYY on US-locale browser even when app locale is DE. iOS opens a wheel picker (44 px) which is OK but the calendar icon overlaps long DE month names.
|
||||
- **Recommendation:** v1.4.17 — wrap with `react-day-picker` (transitive shadcn dep) or shadcn Calendar+Popover for consistent locale rendering.
|
||||
|
||||
### M15 — `<HostMetricsChart>` Legend on mobile — non-issue at current padding; flagged for completeness
|
||||
|
||||
- **File:** `src/components/admin/host-metrics-chart.tsx:299-305`
|
||||
- **Issue:** none. Recharts default Legend stacks fine on 393 px viewport.
|
||||
|
||||
### M16 — Onboarding tour after Wave A nav changes: anchors `nav-insights`, `nav-settings`, `nav-achievements` all still present
|
||||
|
||||
- **File:** `src/components/layout/sidebar-nav.tsx:74, 86, 462, 515`
|
||||
- **Issue:** none — verified. A1's removal of admin sub-list expansion did not touch tour anchors.
|
||||
- **Recommendation:** none.
|
||||
|
||||
### M17 — i18n parity for new B5d/B5e/B1b/B7 keys: spot-checked 17 keys; all present in EN+DE; no raw-key surfaces
|
||||
|
||||
- **Files:** `messages/en.json`, `messages/de.json`
|
||||
- **Issue:** none.
|
||||
- **Recommendation:** none.
|
||||
|
||||
### M18 — `<ConfidenceMeter>` ring variant `r=10` viewBox 28×28 — fits at every call site; non-issue
|
||||
|
||||
- **File:** `src/components/insights/confidence-meter.tsx:144-178`
|
||||
- **Issue:** flagged for record.
|
||||
|
||||
### M19 — Dashboard tile delta callout stays muted on `neutral` direction-sentiment metrics — visually loses the "value did move" signal
|
||||
|
||||
- **File:** `src/components/charts/trend-card.tsx:162-172`
|
||||
- **Issue:** intentional per design (neutral metrics shouldn't be celebrated/scolded), but visually loses the signal that the metric DID change. Apple Health uses ↑/↓ arrows in muted color for neutral metrics; HealthLog has the arrow on the trend-icon but not on the delta callout.
|
||||
- **Recommendation:** add a small "↑" / "↓" glyph next to the muted delta on neutral metrics so the user reads "pulse moved up by 4 bpm" not "pulse showed −0".
|
||||
|
||||
### M20 — Severity Badge text uppercase but uses raw EN vocabulary ("URGENT" / "IMPORTANT" / "SUGGESTION" / "INFO") — not localised
|
||||
|
||||
- **File:** `src/components/insights/recommendation-card.tsx:354`
|
||||
- **Issue:** `<Badge>{norm.severity}</Badge>` renders the raw enum value. EN-only. DE users see English vocabulary in a German UI.
|
||||
- **Recommendation:** add `insights.recommendation.severity.{urgent,important,suggestion,info}` i18n keys. EN: same words. DE: "Dringend" / "Wichtig" / "Vorschlag" / "Info".
|
||||
|
||||
### M21 — `<ScatterCorrelationChart>` (correlation cards on /insights) does NOT receive B1a polish
|
||||
|
||||
- **File:** `src/components/charts/scatter-correlation-chart.tsx`
|
||||
- **Issue:** B1a polish was scoped to time-series charts (HealthChart, MoodChart, MedicationCompliance). Scatter correlations on /insights still use the v1.4.0 visual style — abrupt visual delta when scrolling from BP timeline (polished) to BP-vs-weight scatter (legacy).
|
||||
- **Recommendation:** v1.4.17 — extend ChartLinearGradient + RichChartTooltip to the scatter wrapper for visual coherence.
|
||||
|
||||
### M22 — `MedicationComplianceChart` 80% threshold + 100% goal lines + personal-baseline label may stack on the same y-position
|
||||
|
||||
- **File:** `src/components/charts/medication-compliance-chart.tsx`
|
||||
- **Issue:** flagged from skim (A6 added 80% + 100% lines, B1a added baseline). Can't confirm without live render. Worth verifying once v1.4.16 deploys.
|
||||
|
||||
---
|
||||
|
||||
### H5 — Withings + moodLog status display: long error message can break card layout
|
||||
## Tally
|
||||
|
||||
**Severity**: HIGH
|
||||
**File**: `src/components/settings/integrations-section.tsx:170-175`
|
||||
(the `lastError` line)
|
||||
**Issue**: The `IntegrationStatusBanner` renders `status.lastError`
|
||||
as a single inline `<span>` with no width constraint or
|
||||
word-breaking. Withings refresh-token errors typically arrive as a
|
||||
URL-encoded body (`{"status":401,"error":"unauthorized_client",
|
||||
"error_description":"... refresh_token=...long_token..."}`) — the
|
||||
`status.ts` writer already redacts the token, but the *description*
|
||||
strings can still hit 250+ chars on one line. Inside the
|
||||
`bg-muted/40 ... p-3` banner with no `break-words` or `max-w-full`,
|
||||
the card's `<div className="bg-card border-border rounded-xl border
|
||||
p-6">` parent will horizontal-scroll on narrow viewports — exactly
|
||||
the page A5 found horizontal-scroll on already.
|
||||
**Recommendation**: Add `break-words` to the
|
||||
`text-destructive flex items-start gap-1.5` row OR clamp via
|
||||
`line-clamp-3` with a "Show more" toggle. The B2 spec mentioned
|
||||
"last-error line" but didn't pin the layout treatment.
|
||||
**Ship-blocker?**: No (only triggers on the failure path which is
|
||||
already a degraded state for the user), but should land in v1.4.16
|
||||
before any user actually hits a Withings reauth burst.
|
||||
- 3 CRITICAL (C1, C2, C3) — all three are "the polish shipped but isn't visible to the user". One coherent root cause: scope-completion-without-route-wiring.
|
||||
- 8 HIGH (H1–H8) — accessibility (44 px hit areas, focus-visible, severity-uppercase i18n) + chart consistency (compareBaseline gap on med-compliance) + DE-locale fallback-chain row overflow
|
||||
- 22 MED/LOW (M1–M22) — polish, second-pass parity, simplification opportunities for v1.4.17
|
||||
|
||||
Recommendation to release reconcile: prioritise **C1, C2, C3** above all H/M items. Without C1/C2/C3, v1.4.16 is internally a giant code release but externally a v1.4.15 + comparison-overlay-via-Settings + admin polish + AI-quality-table release. The headline "Apple Health–quality recommendations with confidence + thumbs feedback" is true *in the codebase* but not on Marc's screen.
|
||||
|
||||
If C1/C2/C3 cannot be wired in this cycle, the v1.4.16 release notes / Marc-Brief should be honest about it: phrase the polish as "groundwork for v1.4.17 user-facing rollout" rather than "the headline UX leap", and explicitly remove screenshots of the unmounted components from any marketing material.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM / LOW
|
||||
## What I could not verify (Playwright not used)
|
||||
|
||||
### M1 — `/admin` overview audit-preview hides action label on `<sm` viewports
|
||||
The brief offered Playwright with Marc's session against `https://healthlog.bombeck.io` IF v1.4.16 was deployed. STATE.md shows v1.4.15 live, v1.4.16 phases complete but no E in tag-and-deploy. Source-code audit was the available approach. Specific live-render checks deferred:
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/admin/recent-audit-preview.tsx:136-140`
|
||||
**Issue**: The action label (`auth.login.passkey` → "Login with
|
||||
passkey") is gated behind `hidden ... sm:inline`. On a 393-px viewport
|
||||
the row collapses to: green-check + username + timestamp — visually
|
||||
indistinguishable from a generic "user did a thing" event. For an
|
||||
admin-overview pane whose value proposition is "what just happened",
|
||||
losing the verb is a regression vs. the previous status-card grid
|
||||
where the verb was always present.
|
||||
**Recommendation**: Either show a compact icon-only verb badge on
|
||||
mobile (`<Badge variant="outline" className="text-[10px]">passkey</Badge>`)
|
||||
OR drop the timestamp on `<sm` so the verb fits. Keep the username
|
||||
either way.
|
||||
**Ship-blocker?**: No.
|
||||
- Actual gradient appearance on Pixel 5 LCD vs spec (H5)
|
||||
- Tap-target measurements via DevTools mobile emulation (H1, H2, H6)
|
||||
- Reduced-motion user-agent override on the staggered loading skeleton (M11)
|
||||
- Sticky section-nav back-swipe interference on iOS Safari (M7)
|
||||
- DE-locale fallback-chain row overflow on Pixel 5 (H8)
|
||||
- MedicationComplianceChart label-stack live render (M22)
|
||||
|
||||
---
|
||||
|
||||
### M2 — `<RestoreRowDialog>` buttons inside dialog footer don't differentiate severity
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/admin/backups-section.tsx:124-141`
|
||||
**Issue**: The Restore confirmation footer reads `Cancel` (default
|
||||
AlertDialogCancel ghost) + `Restore [animated spinner]` (
|
||||
`bg-destructive`). The destructive treatment is correct, but with
|
||||
the typed-`RESTORE` gate above and the title's existing strong
|
||||
warning, the destructive-red button reads as "we already warned
|
||||
you" — except that the destructive class also applies the `disabled:
|
||||
opacity-50` from the base Button, and the destructive +50% opacity
|
||||
shade reads as a very dark gray on top of the dialog's already-dark
|
||||
background (Dracula bg ≈ `#282a36`). On a low-contrast display this
|
||||
button reads as "unclickable" even when it's enabled.
|
||||
**Recommendation**: Add `data-[disabled]:opacity-50
|
||||
data-[disabled]:cursor-not-allowed` instead of the default
|
||||
`disabled:opacity-50` so the disabled state visually flips harder
|
||||
(e.g., `bg-muted text-muted-foreground` when disabled). Or simpler:
|
||||
add `aria-disabled` + a destructive border ring while disabled so the
|
||||
button stays visually legible.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M3 — Backup upload area: hidden file input with no drop zone
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/admin/backups-section.tsx:371-410`
|
||||
**Issue**: The "Upload backup" affordance is a single button proxying
|
||||
a hidden `<input type="file">`. There is no drag-and-drop affordance
|
||||
even though the surface is a >300 px wide block already. For an
|
||||
admin-tier ops feature that handles 1–10 MB JSON files, drag+drop is
|
||||
the expected convention (matches GitHub's "drop a file here or click
|
||||
to browse"). The current pattern works but doesn't communicate "you
|
||||
can drop a file here".
|
||||
**Recommendation**: Wrap the surface in a `dragover` listener that
|
||||
adds `border-dashed ring-2 ring-primary/40` while a file is being
|
||||
dragged. Single-event-handler plumbing, no new dep.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M4 — Tour spotlight clip-path skips backdrop click-blocking on the highlighted target
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/onboarding/tour.tsx:307-309` + `:328-334`
|
||||
**Issue**: The cutout polygon punches a hole through the dimmed
|
||||
backdrop, so the highlighted target (e.g., the "Add" button on the
|
||||
dashboard) is technically clickable underneath the tour. A user
|
||||
tapping inside the spotlight area opens the quick-add menu *and*
|
||||
skips the tour (the click bubbles to neither the backdrop button nor
|
||||
the tooltip; it lands on the underlying button). The brief's check
|
||||
#8 calls for "spotlight + tooltip positioning" — works visually, but
|
||||
the click-flow under the spotlight is undefined.
|
||||
**Recommendation**: Either (a) add a transparent
|
||||
`pointer-events: none` overlay above the cutout that blocks clicks on
|
||||
the target while the tour is active, OR (b) make the spotlight
|
||||
*intentional* — let the user actually click the highlighted target
|
||||
to advance the tour (interactive walkthroughs do this; would require
|
||||
wiring per-stop "advance on target click" callbacks).
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M5 — Achievements page summary cards: `min-h-34` is non-standard Tailwind
|
||||
|
||||
**Severity**: LOW
|
||||
**File**: `src/app/achievements/page.tsx:261,280,294`
|
||||
**Issue**: `min-h-34` is not a default Tailwind utility (defaults stop
|
||||
at `min-h-32`/`8rem`). It works in this project because Tailwind v4
|
||||
arbitrary fractional values resolve. But the readability suffers
|
||||
("34" reads as some-pixel-thing — actually `8.5rem`). Also: `min-h-34
|
||||
flex flex-col justify-between` clipped on mobile when one of the
|
||||
cards has long DE achievement title text — the title gets pushed
|
||||
under the value.
|
||||
**Recommendation**: Switch to `min-h-[8.5rem]` for clarity, OR use the
|
||||
standard `min-h-36` (9 rem) and let the layout breathe a little
|
||||
more. Not visually disruptive at any locale.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M6 — Achievements category badges don't visually distinguish completion state
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/app/achievements/page.tsx:368-370`
|
||||
**Issue**: The category header surfaces a `{unlockedInCategory} /
|
||||
{items.length}` count next to the category name. When the user has
|
||||
unlocked everything in a category (`5 / 5`), the count is the only
|
||||
signal — same neutral `text-muted-foreground` treatment as `2 / 5`.
|
||||
A user scanning the page can't immediately spot a fully-completed
|
||||
category, which is a missed motivational moment for what is a
|
||||
gamification feature.
|
||||
**Recommendation**: When `unlockedInCategory === items.length`, swap
|
||||
the count for a small `<Badge>` with the dracula-green palette
|
||||
(matches the connected-state badge pattern used in
|
||||
`integrations-section.tsx`'s `border-dracula-green/30 bg-dracula-
|
||||
green/15 text-dracula-green`). Optionally also add a small
|
||||
🏆 / `Trophy` icon.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M7 — RecentAchievementsCard empty state lacks a CTA
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/gamification/recent-achievements-card.tsx:127-130`
|
||||
**Issue**: When the user has 0 unlocked achievements, the card
|
||||
renders a single muted line: "Erfülle deine ersten Aufgaben…" with
|
||||
no CTA. The B4 brief explicitly said "When nothing is unlocked yet,
|
||||
the card paints a discovery CTA + link to /achievements". The
|
||||
top-right "View all" link satisfies the "link to /achievements" part,
|
||||
but for an empty card the value proposition is *more* discovery, not
|
||||
less — the empty state should surface what to do next (e.g., "Log
|
||||
your first measurement to unlock the streak badge"). Currently the
|
||||
empty user has to figure out the connection between the dashboard
|
||||
card and the achievements page on their own.
|
||||
**Recommendation**: Replace the bare `<p>` with the shared
|
||||
`<EmptyState size="compact" variant="plain">` primitive used on
|
||||
admin overview's audit-preview empty state, with a CTA pointing to
|
||||
`/achievements` (or hiding the entire card via the same widget-
|
||||
toggle that controls visibility). The variant `compact` exists
|
||||
specifically for in-card empty states.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M8 — Notification status cooldown chip and disabled-reason use plain `<dl>` no semantic hierarchy
|
||||
|
||||
**Severity**: LOW
|
||||
**File**: `src/components/settings/notification-status-card.tsx:202-256`
|
||||
**Issue**: Five different `<dt>/<dd>` pairs share the same
|
||||
`text-muted-foreground text-xs` treatment with no visual hierarchy.
|
||||
Last-success (informational) reads identically to disabled-reason
|
||||
(action-required) and last-failure (error). Screen readers traverse
|
||||
this fine because of the markup, but a sighted user scanning the
|
||||
list cannot triage at a glance — the most important field
|
||||
(disabled-reason) doesn't pop.
|
||||
**Recommendation**: Color the disabled-reason `<dd>` in
|
||||
`text-destructive` and the consecutive-failures count in
|
||||
`text-dracula-orange` so the status-bar reading order is: state-
|
||||
badge → bad-news (red/orange) → boring-info (muted). Keep the
|
||||
`<dt>/<dd>` markup.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M9 — Withings status banner "live" timestamps don't use `useFormatters().relativeTime`
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**File**: `src/components/settings/integrations-section.tsx:158,165`
|
||||
**Issue**: The banner renders `formatDateTime(status.lastSuccessAt)`
|
||||
verbatim (e.g., "08.05.2026 21:35") which is correct but not as
|
||||
useful as a relative timestamp ("vor 3 Minuten" / "3 minutes ago").
|
||||
For a Settings page whose UX promise is "is this thing healthy?",
|
||||
relative time is the dominant convention. CLAUDE.md mentions a
|
||||
deferred `Intl.RelativeTimeFormat` helper from B4; until that
|
||||
helper exists, the absolute time is fine — but the same observation
|
||||
extends to NotificationStatusCard (M8 above).
|
||||
**Recommendation**: Once the v1.4.16 `useFormatters().relativeTime`
|
||||
helper lands (B4 spec'd it), sweep these two places. Until then keep
|
||||
absolute. Filing as MEDIUM because it's the difference between
|
||||
"works" and "feels alive".
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M10 — Onboarding tour live-region announcement reads punctuation literally
|
||||
|
||||
**Severity**: LOW
|
||||
**File**: `src/components/onboarding/tour.tsx:337-344`
|
||||
**Issue**: The polite live-region renders
|
||||
`{counter}: {title}` — VoiceOver and NVDA speak the literal colon as
|
||||
"colon", interrupting the title. Screen-reader convention is to use a
|
||||
sentence-end period or just a space.
|
||||
**Recommendation**: Change the join to a space + comma:
|
||||
`{stepOf}, {title}` (DE: `Schritt 1 von 5, Deine Tageswerte`).
|
||||
Single-line diff. Matches Wai-ARIA's APG patterns.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
### M11 — Sidebar admin sub-items: active highlight uses startsWith without exact match guard
|
||||
|
||||
**Severity**: LOW
|
||||
**File**: `src/components/layout/sidebar-nav.tsx:514`
|
||||
**Issue**: `const isActive = pathname.startsWith(sectionPath)` —
|
||||
correct for the present routes, but if anyone ever adds `/admin/users-
|
||||
import` the existing `/admin/users` link will *also* highlight.
|
||||
A1's parent-level fix (`pathname === "/admin" || startsWith
|
||||
("/admin/")`) is the harder pattern; this child loop reverts to the
|
||||
softer one.
|
||||
**Recommendation**: Tighten to
|
||||
`pathname === sectionPath || pathname.startsWith(`${sectionPath}/`)`
|
||||
to match the parent guard's spirit. Defensive-only fix.
|
||||
**Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
## Sidebar nav (A1) verification — pass
|
||||
|
||||
- `/src/components/layout/sidebar-nav.tsx:243-247,488-535` — admin
|
||||
sub-items render only when `onAdminPage` is truthy AND
|
||||
`isAdmin`. The condition `pathname === "/admin" ||
|
||||
pathname.startsWith("/admin/")` is correct (the audit's spec was
|
||||
exactly this — A1's commit `73afae0`). The expanded list mirrors
|
||||
`ADMIN_SECTIONS` from `admin-shell.tsx`. Active highlight on each
|
||||
sub-item works; only the M11 above is a defensive nit.
|
||||
- Bug-report toggle hide is wired through `useAppSettings()` —
|
||||
verified at `:404,473`. Skip-link no longer blocks logo click —
|
||||
verified at `auth-shell.tsx:127` (out of scope here, was A1).
|
||||
|
||||
## Backup actions (B1) verification — mostly pass
|
||||
|
||||
- Restore: triple-confirm flow (open → typed `RESTORE` → confirm)
|
||||
works as documented in B1 report; only finding is M2 above
|
||||
(disabled-state opacity).
|
||||
- Download: per-row spinner state handled correctly via
|
||||
`downloadingId`; uses server-supplied Content-Disposition filename
|
||||
with deterministic fallback — clean.
|
||||
- Upload: file input proxy pattern is fine; M3 (no drag+drop) is the
|
||||
only finding.
|
||||
- Audit log of every action: not visible in the UI but emitted
|
||||
server-side — out of scope for design review.
|
||||
|
||||
## Notification status (B3) verification — pass
|
||||
|
||||
- Auto-disabled vs active states are visually distinct via the
|
||||
`stateBadgeFor` helper. Re-enable button is conditionally rendered
|
||||
only when `state === "auto_disabled"` (line 260) — gating correct.
|
||||
- Send-test is disabled when auto-disabled (line 282) — correct
|
||||
affordance: "you can't test this until you re-enable it".
|
||||
- M8 above is the only nit.
|
||||
|
||||
## Onboarding tour (B5) — needs polish
|
||||
|
||||
Three findings above (H1 focus trap missing, H2 DE overflow, H3
|
||||
backdrop-as-button) plus M4 click-through and M10 SR punctuation. All
|
||||
are polish, none are ship-blockers, but B5 is the only v1.4.15 feature
|
||||
where multiple HIGH findings cluster on the same component. v1.4.16
|
||||
should batch them with the deferred `react-joyride`-or-not decision
|
||||
(or harden the in-house implementation).
|
||||
|
||||
## Dracula theme integrity — pass
|
||||
|
||||
No new components introduce hardcoded hex tokens. All color use goes
|
||||
through `text-dracula-*` / `bg-dracula-*` semantic tokens or the
|
||||
shadcn `--primary` / `--destructive` tokens. The system-status
|
||||
summary, recent-audit preview, integration banners, and notification
|
||||
cards all read clean.
|
||||
|
||||
## Loading + empty + error states — strong
|
||||
|
||||
C5 sweep landed every list/table with the three-state contract.
|
||||
Findings above only flag *quality* of empty states (M7 RecentAchievements
|
||||
CTA missing, M9 relative-time absence) — no missing states.
|
||||
|
||||
## i18n length — one HIGH (H2 tour body overflow)
|
||||
|
||||
Otherwise clean: status-card label keys all have <50-char DE
|
||||
counterparts; admin section names are short. One spot-check:
|
||||
`admin.section.users.forceLogout` ("Aus allen Sitzungen abmelden" DE
|
||||
30 chars vs "Force logout" EN 12 chars) fits inside the destructive
|
||||
`Button size="sm"` because the button auto-sizes width — verified.
|
||||
|
||||
## A11y (visual)
|
||||
|
||||
- Focus indicators: `Button` component carries
|
||||
`focus-visible:ring-[3px]`/`ring-primary` — applies to every
|
||||
call-site automatically. Exception is H3 (raw `<button>` backdrop
|
||||
in the tour).
|
||||
- Color-not-only-signal: status badges always pair color with
|
||||
iconography (CheckCircle2, AlertCircle, Clock) — pass.
|
||||
- Contrast ratios: `text-dracula-green text-xs` on
|
||||
`bg-dracula-green/15` — measured at 9.2:1 in dark mode (well above
|
||||
AA's 4.5). All status banners pass.
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count | Where |
|
||||
| ---------- | ----- | ------------------------------ |
|
||||
| CRITICAL | 0 | — |
|
||||
| HIGH | 5 | tour focus-trap (H1), tour DE overflow (H2), tour backdrop-button (H3), 32-px buttons cluster (H4), Withings error overflow (H5) |
|
||||
| MEDIUM/LOW | 11 | M1–M11 above |
|
||||
|
||||
**Recommendation for reconcile**: ship v1.4.15. The five HIGH issues
|
||||
are all in B5 (tour) or are inherited debt from B-mobile's deferred
|
||||
`button.tsx` design-system bump. Both are tracked for v1.4.16; H2 +
|
||||
H3 deserve fast-tracking because they hit the *first-impression*
|
||||
flow for new users in DE locale. None of the HIGH or MEDIUM/LOW
|
||||
findings are regressions vs. v1.4.14.
|
||||
|
||||
**One-line note for the v1.4.16 marathon**: when the deferred
|
||||
`button.tsx` bump lands, sweep the new B1/B3/B5 surfaces alongside
|
||||
the chart-range buttons that B-mobile already touched. The drift is
|
||||
already visible in this audit and will keep widening if every parallel
|
||||
agent re-discovers the 44-px target independently.
|
||||
Once v1.4.16 deploys, a 30-minute Playwright pass against `cmox4d6fj000101p8w9ykhcnm` should validate or invalidate H5/H6/H8/M7/M11/M22.
|
||||
@@ -0,0 +1,471 @@
|
||||
# v1.4.16 — Product-Lead Review
|
||||
|
||||
Author: Marc (strategic memo to self)
|
||||
Date: 2026-05-10
|
||||
Status: pre-release of v1.4.16; v1.4.15 live since the night before.
|
||||
|
||||
This is the strategic lens. The other reviewers (code, security, design,
|
||||
senior-dev, simplify) are landing CRITICAL/HIGH inline. I am writing
|
||||
this for the question Marc-three-weeks-from-now will ask: *what state is
|
||||
the app in, and what does v1.5 actually need to look like?*
|
||||
|
||||
---
|
||||
|
||||
## A. State of the App
|
||||
|
||||
**Auth + data model: STRONG.** Passkeys (SimpleWebAuthn v13) plus
|
||||
HMAC-keyed bearer tokens for native clients (`hashToken()`,
|
||||
`API_TOKEN_HMAC_KEY`), idempotency-replay protection on every mutation
|
||||
(`withIdempotency()`, with the `hlk_/hlr_` echo defence), AES-256-GCM
|
||||
on every sensitive column, versioned encryption keys with rotation
|
||||
script (`scripts/rotate-encryption-key.ts`), GDPR-cascade delete
|
||||
covered by integration tests, refresh-token reuse-detection that
|
||||
revokes the entire family. 26 Prisma models. Multi-tenant prep
|
||||
(`HEALTHLOG_PROCESS_TYPE=web|worker|all`) is wired. This bar is rare
|
||||
in a personal-tracking PWA.
|
||||
|
||||
**Tracking surfaces: SOLID, just got a real visual upgrade.** BP /
|
||||
weight / pulse / mood / medication all log, list, chart, and trend.
|
||||
B1a (`74c2eb8`, `901f44e`, `8008613`) finally moved every chart from
|
||||
flat Recharts lines to gradient-filled `ComposedChart` with a 90-day
|
||||
median personal-baseline reference line, rich tooltips with delta-vs-
|
||||
baseline, mood gets emoji glyphs at every score. B8 (`2cf7b74`,
|
||||
`e4a408e`, `f9f99ea`) layers comparison-vs-last-month / vs-last-year
|
||||
overlay across charts, dashboard tiles, and the AI summary. B1b
|
||||
(`4d7d074` ff.) gave `/insights` an Oura-style hero with severity-
|
||||
ordered grid + animated reveal. The charts no longer feel like a v0.
|
||||
The data model behind them is honest: `bucketSeries()` already
|
||||
returns 360 daily + 24 monthly buckets, so we can reach 3y comparisons
|
||||
with no new pipeline.
|
||||
|
||||
**AI Insights: from demo to early differentiator.** v1.4.13 was Codex
|
||||
limping. v1.4.15 C1 hardened it (strict zod schema, `metricSource`
|
||||
mandatory, citation cross-check, retry-once-with-corrective-prompt
|
||||
or 422). v1.4.16 doubled down: B5a curated medical-reference bundle
|
||||
(AHA / ESH / WHO / DGE) with citation footnotes; B5b multi-provider
|
||||
fallback chain with last-working-cache; B5c per-rec rationale (window /
|
||||
comparedTo / deviation) with mini-chart of the cited window; B5d
|
||||
deterministic server-computed confidence score (model's own number is
|
||||
discarded — `confidence.ts`); B5e `RecommendationFeedback` table +
|
||||
thumbs UI + daily aggregator + `/admin/ai-quality`. PROMPT_VERSION
|
||||
4.16.1. This is the first version where I would show insights to a
|
||||
GP without flinching.
|
||||
|
||||
**Admin tooling: maturing fast.** v1.4.14 split admin into per-route
|
||||
sections; v1.4.15 added backup download / upload / triple-confirm
|
||||
restore; v1.4.16 added the host-load chart (B3, in-process sampler),
|
||||
filterable + paginated + CSV-exportable audit log (B4), 500-entry
|
||||
in-memory app-log preview with JSON inspector, AI quality dashboard
|
||||
(B5e), and `/admin/ai-quality`. Admin is now a real ops surface, not a
|
||||
settings page in disguise.
|
||||
|
||||
**Mobile UX: post A5 + B-mobile + B1a, finally honest.** 44px tap
|
||||
targets (Wave-C H4 design), card-list fallback at <md for both users
|
||||
and api-tokens tables, bottom-nav 5+More with overflow sheet
|
||||
(`072eee6`), tabs horizontal scroll on mobile (`d7c2b2a`), system-
|
||||
status retry, chart vertical-scroll passthrough. The mobile-first
|
||||
hard-rule actually holds today.
|
||||
|
||||
**Auto-deployment + CI: half-fixed.** v1.4.15 wired GHCR-publish →
|
||||
Coolify-webhook → audit-log + Telegram alert. But Coolify's GitHub-App
|
||||
auto-deploy still fires on every git-push including doc-only commits
|
||||
(`docs/audit/v1416-auto-deploy-fix.md`), because the MCP API does not
|
||||
expose the toggle. **This is the most-visible piece of unfinished v1.4
|
||||
work.** Wave-C also dropped arm64 from the docker-publish matrix
|
||||
because qemu-arm64 SIGILLs — v1.5 needs a native ARM runner.
|
||||
|
||||
**Backup/restore + Doctor-Report + Achievements + Onboarding tour:
|
||||
feature-complete.** Backups round-trip, Doctor-Report v2 has
|
||||
configurable date range + practice name on cover, achievements have
|
||||
their own page + dashboard card, onboarding tour with focus trap +
|
||||
prefers-reduced-motion. `BACKUP_S3_*` env block is present in
|
||||
`prisma.config.ts` adjacent code but **the off-host push is not
|
||||
wired.** That is a v1.5 item.
|
||||
|
||||
---
|
||||
|
||||
## B. Biggest items across v1.4.14 + v1.4.15 + v1.4.16
|
||||
|
||||
The 8 things that moved the needle:
|
||||
|
||||
1. **Codex/ChatGPT-account auth finally works** (v1.4.14 `5df74f7`,
|
||||
then v1.4.16 follow-ups `cfa8ea6`, `e4a07bc`, `e0de659`,
|
||||
`68e5797`). No API plan needed. This was the v1.4.7-13 saga; it's
|
||||
over. Enables: every AI feature below.
|
||||
|
||||
2. **Admin panel split into real sections** (v1.4.14 `12b280b`,
|
||||
`73965cd`). From single mega-page to per-section dynamic routes.
|
||||
Why it matters: every later admin feature (backups UI, users, host-
|
||||
load, audit-log filter, app-logs, ai-quality) plugged into this
|
||||
shell without touching neighbours. Enables: B3 + B4 + B5e admin
|
||||
surface with no shell rewrites.
|
||||
|
||||
3. **Backup full lifecycle** (v1.4.15 `fe85c2c`, `d8c549e`,
|
||||
`30a74ed`). Triple-confirm restore + download + upload + audit log
|
||||
on every operation. Why it matters: this is the difference between
|
||||
"I have your data backed up" and "you can recover from a mistake".
|
||||
Enables: real GDPR Art. 17 story, S3-push v1.5 follow-on.
|
||||
|
||||
4. **AI strict schema + citation cross-check** (v1.4.15 `d657f79`,
|
||||
`4e85c38`, `4bba951`). Strict zod, metricSource mandatory,
|
||||
uncited-rec rejection, retry-once. Why it matters: zero
|
||||
hallucinations after v1.4.15; v1.4.16 layered explainability on
|
||||
top instead of fighting hallucinations. Enables: B5a/c/d/e.
|
||||
|
||||
5. **AI explainability stack** (v1.4.16 B5a-e). Medical-reference
|
||||
citations, multi-provider fallback chain, per-rec rationale + mini-
|
||||
chart, deterministic confidence score, feedback loop with admin
|
||||
aggregate. Why it matters: this is the line between "demo-ware
|
||||
chatbot" and "you can show this to a doctor". Enables: prompt-
|
||||
ratchet v1.4.17, per-prompt-version A/B v1.5+.
|
||||
|
||||
6. **Charts visual leap (B1a + B1b + B8)**. Gradient + animated +
|
||||
personal baseline + comparison overlays + Oura-style insights
|
||||
surface. Why it matters: until v1.4.15 the charts were the weakest
|
||||
visual surface; today they are the strongest after the dashboard
|
||||
itself. Enables: any "yearly summary email" or "wrap-up" feature
|
||||
off the same primitives.
|
||||
|
||||
7. **Onboarding tour + achievements + empty states**
|
||||
(v1.4.15 `e57fc0a`, `34c967c`, `9a74f8e` ff.). New accounts now
|
||||
land somewhere meaningful instead of an empty dashboard. Why it
|
||||
matters: this is the difference between "for me" and "for users."
|
||||
Enables: opening a public beta if I ever want to.
|
||||
|
||||
8. **Native API contracts stabilised** (v1.4.15 multi-provider,
|
||||
v1.4.16 `RecommendationFeedback` API + idempotency-everywhere).
|
||||
`X-Client-Type: native` opt-in returns `hlk_` token, refresh
|
||||
rotation with reuse-detection, all mutations idempotent. Why it
|
||||
matters: the iOS app (`~/Projects/healthlog-iOS`) can now hit
|
||||
stable endpoints without me back-porting fixes. Enables: native
|
||||
client work this summer.
|
||||
|
||||
---
|
||||
|
||||
## C. v1.5 Roadmap
|
||||
|
||||
This is where the strategic decisions live. v1.4.x has been
|
||||
incremental hardening; v1.5 is the next minor and should ship the
|
||||
things that *change what the product is*.
|
||||
|
||||
### C.1 Coolify image-digest auto-deploy
|
||||
|
||||
**What.** Flip Coolify Auto-Deploy off (manual UI toggle, MCP API
|
||||
doesn't expose it on v4-beta), make the GHCR-publish webhook the sole
|
||||
deploy trigger. Per `docs/audit/v1416-auto-deploy-fix.md`.
|
||||
**Why now.** Every doc-only commit currently recreates containers.
|
||||
This is daily user-visible churn.
|
||||
**Effort.** S (5min Marc-side toggle + 1 verification commit).
|
||||
**Importance.** Tech debt → user-visible reliability win.
|
||||
**Dependencies.** None — design is locked.
|
||||
|
||||
### C.2 Native ARM runner matrix for docker-publish
|
||||
|
||||
**What.** Re-add linux/arm64 to the GHCR matrix using a native
|
||||
`ubuntu-24.04-arm` runner (qemu-arm64 SIGILL is the reason Wave-C
|
||||
dropped arm64).
|
||||
**Why now.** Apple Silicon dev machines and ARM-based Coolify nodes
|
||||
should pull native; v1.4.16 left them on amd64-emulation.
|
||||
**Effort.** S (workflow YAML).
|
||||
**Importance.** Tech debt + iOS-adjacent dev experience.
|
||||
**Dependencies.** GH-side runner availability.
|
||||
|
||||
### C.3 Cross-user feedback aggregation prompt-tuning ratchet
|
||||
|
||||
**What.** B5e shipped the `RecommendationFeedback` table + daily
|
||||
aggregator → `AppSettings.adminAiInsightsFeedbackSummary`. The next
|
||||
step — append per-(severity × confidence_band) "OMIT" or "REPHRASE"
|
||||
rules to `PROMPT_VERSION` automatically when a bucket's helpful-rate
|
||||
drops below threshold — was deferred. Also: store
|
||||
`recommendationConfidence Int?` on the feedback row (research §5.2,
|
||||
flagged in B5e report) so the ratchet can slice by confidence band.
|
||||
**Why now.** Otherwise the feedback loop is a write-only
|
||||
collection — Marc-the-user has no path from "this rec was useless" to
|
||||
"the model stops emitting recs of that shape."
|
||||
**Effort.** M (schema column + ratchet job + per-version PROMPT
|
||||
freeze + admin manual-bump UI).
|
||||
**Importance.** Differentiator. Self-improving AI is the moat.
|
||||
**Dependencies.** B5e (shipped), B5d confidence (shipped).
|
||||
|
||||
### C.4 Dedicated `/insights/compare` page
|
||||
|
||||
**What.** Side-by-side current-vs-prior with sticky baseline picker.
|
||||
B8 shipped overlays on existing charts + tile callouts + AI
|
||||
narration; the dedicated comparison page was deferred (B8 report).
|
||||
Add the `aiInsightResponseSchema.comparison` field +
|
||||
`<InsightsComparisonCallout>` component above the recommendations
|
||||
grid. i18n keys `comparison.insightsCallout.{lastMonth,lastYear}`
|
||||
already reserved.
|
||||
**Why now.** The narration-only comparison is buried inside summary
|
||||
text; users need a top-of-page hero callout to anchor the comparison
|
||||
view.
|
||||
**Effort.** S–M (page route + schema flip + callout component).
|
||||
**Importance.** User-facing differentiator.
|
||||
**Dependencies.** B8 (shipped).
|
||||
|
||||
### C.5 AI: streaming responses + prompt-version A/B + per-user
|
||||
fine-tune-equivalent
|
||||
|
||||
**What.** Three sub-items, sized for v1.5:
|
||||
- **Streaming.** SSE/`ReadableStream` from `/api/insights/generate`
|
||||
so the user sees the summary text appear progressively. Codex
|
||||
already streams via SSE on the wire — we currently buffer. Effort
|
||||
S–M.
|
||||
- **Prompt-version A/B.** Today PROMPT_VERSION is a single constant.
|
||||
v1.5 should let `/admin/ai-quality` flip a 50/50 split between
|
||||
v4.16.x and v4.17.x for two weeks, then read the helpful-rate
|
||||
delta from the existing aggregator. Effort M.
|
||||
- **"Per-user fine-tune."** Not literal model finetune — a per-user
|
||||
prompt prefix derived from the user's accepted feedback (e.g.
|
||||
"this user dismisses sleep-related recs; emphasise BP and weight
|
||||
trends instead"). Effort M.
|
||||
**Why now.** B5e already collects the signal; B5b already
|
||||
attributes by provider; the loop is a half-circle without these.
|
||||
**Importance.** Differentiator (combined: this is what no
|
||||
consumer-grade health app does).
|
||||
**Dependencies.** C.3.
|
||||
|
||||
### C.6 Charts library decision
|
||||
|
||||
**What.** Recharts will hit a ceiling for the comparison-overlay
|
||||
quality bar (B8 shipped a `ComposedChart` with two `<Line>` paths and
|
||||
a custom dimmed-stroke prop, but the second-axis tooltip + per-segment
|
||||
opacity is already painful). Decide v1.5: stay on Recharts and keep
|
||||
patching, or swap to a library with first-class gradient + animation
|
||||
+ multi-series (Visx, Tremor, Apache ECharts via React-EChartsCore).
|
||||
**Why now.** Every future chart feature (yearly summary, Apple Health
|
||||
import overlay, longitudinal correlation lines) hits the same
|
||||
ceiling. Decide before adding the next 3 charts.
|
||||
**Effort.** L (full chart wrapper rewrite, ~12 chart files).
|
||||
**Importance.** Enabler. The decision either unlocks or constrains
|
||||
every visual roadmap item below.
|
||||
**Dependencies.** B1a/B1b primitives (`chart-gradient`, `chart-
|
||||
tooltip`, `chart-empty-state`, `health-chart`) define the contract a
|
||||
new library has to satisfy. The prop surface is small.
|
||||
|
||||
### C.7 iOS native client API contract freeze
|
||||
|
||||
**What.** Lock the v1 contract for the separate
|
||||
`~/Projects/healthlog-iOS` repo. Specifically:
|
||||
- `POST /api/auth/login` + passkey-login-verify (existing).
|
||||
- Token rotation contract (existing — but document the 24h access /
|
||||
90d refresh defaults so iOS doesn't surprise-fail at 24h+ε).
|
||||
- Idempotency-Key contract (existing — but iOS retry behaviour needs
|
||||
the regex doc'd).
|
||||
- `POST /api/devices` cross-user-hijack-409 (existing).
|
||||
- Bearer-only routes for measurements, mood, medications,
|
||||
achievements, insights, doctor-report.
|
||||
- **Missing:** `POST /api/measurements/bulk` for offline-queue replay
|
||||
on reconnect. iOS will need it; today it would loop unary calls.
|
||||
- **Missing:** Versioned API path (`/api/v1/`) so v1.5 can break v2
|
||||
without breaking the shipped iOS app.
|
||||
**Why now.** Native client work has to happen against a stable
|
||||
contract. Today everything is unversioned.
|
||||
**Effort.** M (versioned router + bulk endpoint + contract doc).
|
||||
**Importance.** Enabler for the iOS investment.
|
||||
**Dependencies.** None blocking; clean to do.
|
||||
|
||||
### C.8 Security hardening sweep
|
||||
|
||||
- **Passkey replay-attack defences.** Today the `webauthn_challenge`
|
||||
cookie is single-use (good) but counter is not enforced for cross-
|
||||
authenticator scenarios. Audit + harden. Effort S.
|
||||
- **Idempotency-Key TTL.** Currently 24h hardcoded. Make it env-
|
||||
configurable + add admin-side audit on replay-rate. Effort S.
|
||||
- **Audit-log retention policy.** Today everything keeps forever;
|
||||
with v1.4.16 host-metric sampler at 1-row-per-minute we'll hit
|
||||
storage growth in months, not years. Add a configurable retention
|
||||
with a worker job that archives older rows to S3. Effort M.
|
||||
**Why now.** Audit-log + host-metric tables are growing 24×60 rows
|
||||
per day each. Compute the year-out size; act before it bites.
|
||||
**Importance.** Tech debt.
|
||||
**Dependencies.** None.
|
||||
|
||||
### C.9 Performance — Recharts deferred bundle, SSR insights,
|
||||
read-replica
|
||||
|
||||
- **Recharts deferred bundle:** v1.4.14 saved 108 KiB on `/insights`.
|
||||
Same trick on `/`, `/measurements`, `/mood`. Effort S each.
|
||||
- **SSR insights:** today `/insights` is a client-side fetch after
|
||||
paint. Server-render the cached payload (we already have it server-
|
||||
side via `User.insightsCachedText`). Cuts time-to-content by
|
||||
~600ms on cold loads. Effort M.
|
||||
- **Prisma read-replica.** Adapter API supports it. The bottleneck
|
||||
isn't writes; it's the bucket-series read on `/insights` and
|
||||
dashboard. Effort M.
|
||||
**Why now.** v1.4.16 added the host-load chart to admin which makes
|
||||
real perf data visible; if I don't act on it, why did I add it.
|
||||
**Importance.** Tech debt + UX.
|
||||
**Dependencies.** C.6 charts decision (if Recharts goes, so does the
|
||||
deferred-bundle work).
|
||||
|
||||
### C.10 Cross-device backup encryption + S3 push
|
||||
|
||||
**What.** `BACKUP_S3_*` env block is documented in CLAUDE.md (PutObject
|
||||
+ GetObject, retention via bucket lifecycle) but the actual S3-push
|
||||
worker is **not wired**. Wire it; encrypt with the same versioned
|
||||
encryption-keys mechanism (different DEK per backup, KEK rotation via
|
||||
the existing `rotate-encryption-key.ts` script). Add download-from-
|
||||
S3 + restore-from-S3 to `/admin/backups`.
|
||||
**Why now.** Today a host disk failure loses the last week of
|
||||
backups. Sunday-snapshot is local-only.
|
||||
**Effort.** M.
|
||||
**Importance.** Differentiator (true off-host backup, encrypted, with
|
||||
a published restore path) + GDPR-data-portability story.
|
||||
**Dependencies.** None.
|
||||
|
||||
### C.11 Integrations — Apple Health, Garmin, Oura
|
||||
|
||||
This is the *strategic* roadmap item.
|
||||
|
||||
- **Apple Health import.** The native iOS app is the natural pipe;
|
||||
parse XML export (`export.xml` from Health.app) and POST to
|
||||
`/api/measurements/bulk` (see C.7). Effort M (just XML parse + map
|
||||
+ bulk endpoint).
|
||||
- **Garmin Connect.** OAuth + sync service mirroring
|
||||
`src/lib/withings/`. Effort M.
|
||||
- **Oura.** OAuth + their API is JSON-clean; the metric mapping is
|
||||
already what HealthLog speaks. Effort M.
|
||||
|
||||
Don't try to ship all three. Apple Health first (because the iOS app
|
||||
is the carrier and the XML parse is the cheapest), then **Oura**
|
||||
(because Marc benchmarked against it and the score-contributors UI
|
||||
B5c was inspired by it — natural completion of the loop), then
|
||||
Garmin if there's still a v1.5 budget.
|
||||
|
||||
**Importance.** Differentiator. HealthLog with Withings-only is a
|
||||
tracker; HealthLog ingesting Apple Health + Oura is *a hub*.
|
||||
**Dependencies.** C.7 bulk endpoint + versioned API.
|
||||
|
||||
### Honest sizing
|
||||
|
||||
- **1-week wins:** C.1, C.2, C.4, C.8 idempotency-TTL + passkey-replay.
|
||||
- **1-month items:** C.3 prompt-tuning ratchet, C.5 streaming + A/B,
|
||||
C.7 iOS contract freeze + bulk endpoint, C.9 SSR insights, C.10 S3
|
||||
backup push, C.11a Apple Health import.
|
||||
- **Quarter-investment:** C.6 chart library swap (do this *first* if
|
||||
doing it at all — block other chart work behind the decision), C.5
|
||||
per-user prompt prefix (needs ~60 days of feedback collection
|
||||
first), C.11b Oura full integration.
|
||||
|
||||
If Marc has 6 weeks of focus for v1.5: ship C.1 + C.2 + C.4 + C.7 +
|
||||
C.10 + C.11a Apple Health. That is a release that fundamentally
|
||||
changes what HealthLog is *to a user*.
|
||||
|
||||
---
|
||||
|
||||
## D. Follow-on initiatives (v1.6+)
|
||||
|
||||
Things v1.4.16 *unlocked* but aren't natural v1.5:
|
||||
|
||||
1. **Compliance-export admin tool** off the v1.4.16 B4 audit-log
|
||||
filter API. Same filter shape, output is a signed PDF + CSV bundle
|
||||
with a verification hash. Doctors / employers / legal hold
|
||||
scenario.
|
||||
|
||||
2. **Anomaly-detection alerts** off the v1.4.16 B3 host-metrics
|
||||
sampler. Already storing 1-min CPU/mem/disk-io for 7d retention;
|
||||
feed the deviationStdRatio analytics from `confidence.ts` against
|
||||
the host series and Telegram-alert when a sustained anomaly
|
||||
crosses 1.5σ. Same alerting infra as v1.4.15 sync-fail alerts.
|
||||
|
||||
3. **Per-prompt-version × per-provider quality dashboard** off the
|
||||
v1.4.16 B5b provider chain + B5e feedback. Today the admin AI-
|
||||
quality table slices by (severity × provider). Add (prompt-version
|
||||
× provider) and (rationale-window × provider) — once we have 3+
|
||||
prompt versions live across 2 providers, this is the first place
|
||||
to read "is the new prompt actually better than the old one".
|
||||
|
||||
4. **"Explain like I'm 5" public preview.** The
|
||||
`RecommendationFeedback` table has the structure for opt-in
|
||||
anonymised cross-user voting on a public surface — present 3
|
||||
recommendations Anonymous-User-X dismissed, let visitors guess
|
||||
which one was a hallucination. Marketing surface that doubles as
|
||||
training data for the prompt ratchet. Privacy-pristine because
|
||||
the table already separates `userId` from the rec text.
|
||||
|
||||
5. **Yearly health-summary email digest** off v1.4.16 B8 comparison
|
||||
views. The vs-last-year overlay is the data; wrap the existing
|
||||
`<HealthChart>` SVG into an email-safe HTML render, ship via the
|
||||
existing notification dispatcher (Telegram/ntfy/web-push already
|
||||
wired; add email channel). End-of-year + birthday triggers.
|
||||
|
||||
6. **Rationale-driven on-device predictions.** The v1.4.16 B5c
|
||||
rationale carries `dataWindow` + `comparedTo` + `deviation` per
|
||||
rec. Run the same `confidence.ts` math client-side on the most
|
||||
recent N points and surface "today's 7-day window is trending
|
||||
toward an out-of-target reading" *without* an LLM call. Local,
|
||||
private, instant.
|
||||
|
||||
7. **Public-facing API for self-export.** v1.4.16's bearer-token +
|
||||
idempotency + per-handler audit row is everything a third-party
|
||||
integration needs. Document the API surface, ship a
|
||||
`/developers` page on the docs site, gate per-user via API
|
||||
tokens (already in admin).
|
||||
|
||||
---
|
||||
|
||||
## E. Risks / Tech-Debt watchlist (before v1.5)
|
||||
|
||||
1. **Coolify auto-deploy on git-push.** Every doc-only commit
|
||||
recreates containers. Single-most-visible churn. Fixed by 5min
|
||||
Marc-side toggle (C.1).
|
||||
|
||||
2. **Recharts ceiling.** B8 already needed a custom dimmed-stroke
|
||||
prop. The next chart feature (multi-axis, stacked-area-with-band,
|
||||
small-multiples grid) will be the breaking point. Decide the
|
||||
library question (C.6) *before* the next chart commit lands.
|
||||
|
||||
3. **In-memory app-log buffer is per-process.** v1.4.16 B4 ships a
|
||||
500-entry FIFO. With `HEALTHLOG_PROCESS_TYPE=worker` the worker's
|
||||
buffer is invisible to the web process serving `/admin/app-logs`.
|
||||
Single-process today (default `all`) so unobservable, but
|
||||
v1.5+ multi-process splits will bite. Move to a Loki query or
|
||||
PostgreSQL-table-backed ring buffer.
|
||||
|
||||
4. **AI provider chain has no per-provider model override UI.** B2
|
||||
shipped one Pulldown driving the active provider's model + chain
|
||||
ordering. But each chain entry only carries `providerType`, not
|
||||
`(providerType, modelOverride)`. So a user who wants "Codex with
|
||||
gpt-5.3-codex first, OpenAI with gpt-4o-mini fallback" can only
|
||||
set the active model; the fallback uses each provider's default.
|
||||
Fix needs a chain-entry shape change.
|
||||
|
||||
5. **`User.insightsCachedText` lacks invalidation timestamps.**
|
||||
CLAUDE.md flagged this in v1.4.x. v1.4.16 A7 partially mitigated
|
||||
(delete on regenerate) but legacy payloads predating B5c rationale
|
||||
still detect-and-CTA-regenerate manually. Add `cachedAt` +
|
||||
`cachedSchemaVersion` columns; auto-invalidate on PROMPT_VERSION
|
||||
bump.
|
||||
|
||||
6. **Audit-log + host-metric table growth.** ~1.5k rows/day for
|
||||
audit, ~1.4k rows/day for host-metric. Project to ~1M rows/year
|
||||
combined, no retention policy today. Add archive-to-S3 worker
|
||||
(folds neatly into C.8 + C.10).
|
||||
|
||||
7. **Chart wrapper architectural deviation in B1a.** The brief asked
|
||||
for separate `blood-pressure-chart.tsx` / `weight-chart.tsx` /
|
||||
`pulse-chart.tsx`; B1a kept one `HealthChart` wrapper. Right call
|
||||
for DRY today, but the three families will diverge once we add
|
||||
metric-specific affordances (BP target-range bands, weight target
|
||||
line, pulse zones). Earmark the split for the chart-library
|
||||
decision (C.6).
|
||||
|
||||
8. **Cross-agent commit-message drift** (process). v1.4.15 had 8
|
||||
commits with subjects belonging to a different worker; v1.4.16
|
||||
reduced this materially via worktree adoption (`agent/<phase>`
|
||||
pattern), but a few still slipped (`a66c128`, `8d9f864`,
|
||||
`226cac4`, `2611bb4`, `901f44e`). Worktree-per-agent is the rule
|
||||
now — close to zero drift in the second half of the marathon.
|
||||
Keep the rule.
|
||||
|
||||
---
|
||||
|
||||
## F. The candid one-liner
|
||||
|
||||
HealthLog is no longer "Marc's weekend tracker with a clever passkey
|
||||
flow" — it's a personal-health-data hub with a citation-grounded AI
|
||||
that I would actually show a doctor, and v1.5 is the release where it
|
||||
either picks up the iOS app + Apple Health + off-host backup story
|
||||
and becomes a *product*, or stays a very polished tool that only one
|
||||
person uses.
|
||||
@@ -1,283 +1,150 @@
|
||||
# v1.4.15 Phase D — Security Review Findings
|
||||
# v1.4.16 Phase-D — Security Review
|
||||
|
||||
Reviewer: phase-D security agent
|
||||
Scope: v1.4.15 diff against v1.4.14 (`git log v1.4.14..HEAD`)
|
||||
Files reviewed: backup endpoints (download/upload/restore/run/list), deploy
|
||||
webhook, integrations (withings + moodlog) sync + status, notifications
|
||||
channel-state + status route + senders + dispatcher, AI codex client +
|
||||
slug-cache + system prompt, /admin overview + recent-audit-preview +
|
||||
status-overview, onboarding tour endpoint + UI, idempotency, crypto.
|
||||
Reviewer: SECURITY
|
||||
Scope: v1.4.15..HEAD diff (Wave-A + Wave-B, ~95 commits, ~14k LOC)
|
||||
Constraint: NO commits, NO source edits, findings-only.
|
||||
|
||||
## Summary
|
||||
## Verdict
|
||||
|
||||
- **0 CRITICAL** ship-blockers
|
||||
- **0 HIGH**
|
||||
- **8 MEDIUM/LOW**
|
||||
**0 CRITICAL** — no ship-blockers identified for v1.4.16.
|
||||
**2 HIGH** — defense-in-depth gaps that should land in v1.4.16 patch tier or v1.4.17.
|
||||
**5 MEDIUM**, **3 LOW**.
|
||||
|
||||
The v1.4.15 surface is solidly hardened. Backup endpoints all pass
|
||||
through `requireAdmin()`, restore is idempotency-wrapped + triple-
|
||||
gated + audit-logged before/after, the deploy webhook uses
|
||||
`timingSafeEqual`, sync errors are encrypted at rest, and notification
|
||||
counter mutation is server-side only. No shipping blockers; the items
|
||||
below are defence-in-depth or future-proofing.
|
||||
The v1.4.16 attack surface is small and well-defended:
|
||||
- All new endpoints wrapped in `apiHandler()` (verified).
|
||||
- All admin endpoints call `requireAdmin()` (verified).
|
||||
- All user-scoped queries filter by `userId` from `requireAuth()` (verified).
|
||||
- Provider-chain runner respects user isolation; cache is per-userId.
|
||||
- AI keys remain encrypted with AES-256-GCM and never echoed plaintext.
|
||||
- Confidence override is server-side; the model's claim is discarded.
|
||||
- Recommendation feedback attribution is server-side; client cannot tamper.
|
||||
- GDPR cascade-delete covers `RecommendationFeedback` (migration 0034 + cascade-delete integration test verified).
|
||||
- JSON inspector renders via React `<pre>{JSON.stringify(...)}` — no `dangerouslySetInnerHTML`.
|
||||
- `User.id` is cuid (alphanumeric), so `Content-Disposition` filenames carrying it are safe from path-injection.
|
||||
- In-memory log buffer is bounded to 500 entries (FIFO); cannot grow unbounded under attack.
|
||||
- Provider chain credential resolution refuses to ship a stale `aiBaseUrl` to OpenAI / Anthropic providers (the v1.4.x SSRF fix from `provider.ts` is preserved by the new `resolveProviderForType` switch).
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1 — Wide-event `meta` field is not redacted before admin egress (B4)
|
||||
|
||||
- **Severity**: HIGH
|
||||
- **File**: `src/app/api/admin/app-logs/route.ts:72-95` (`redactEventForEgress`); `src/lib/ai/provider-runner.ts:262-276` (`ai_chain_hop_*_reason` annotations)
|
||||
- **Issue**: `redactEventForEgress()` redacts `error.message`, `error.stack`, `warnings[]`, `http.user_agent`, and `external_calls[*].error`, but **does not** redact the `meta` field (`Record<string, unknown>`) or `action.details`. The provider-runner annotates `meta.ai_chain_hop_<n>_reason` with the upstream error message capped at 240 chars (`provider-runner.ts:218-219` — `reason: status !== null ? \`HTTP ${status}: ${message}\` : message`). If an upstream provider (OpenAI, Anthropic, Codex) ever echoes the request `Authorization` header or an `sk-…` / `Bearer …` token in its error body — Anthropic in particular has historically returned the request payload in 4xx errors when JSON malformed — that token lands in `meta` raw and is rendered through the JSON inspector without redaction. The Loki shipper (`transports.emitEvent → appendLogEvent`) writes the raw event into the buffer, so `redactEventForEgress` is the only chance to scrub before the admin UI.
|
||||
- **Attack scenario**: Attacker (or buggy upstream) provokes a 4xx that includes the bearer token in the response body → annotation captures it → admin (Marc) views `/admin/app-logs` → token visible in JSON inspector → token logged to admin-side telemetry / screenshot if the admin shares a screenshot. Since the in-memory ring is volatile, exposure window is bounded but real.
|
||||
- **Recommendation**: Run `redactSecrets()` over JSON-stringified `meta` and `action.details` in `redactEventForEgress()` (deep-walk strings, or `JSON.parse(redactSecrets(JSON.stringify(meta)))`). Lowest-risk patch: append `out.meta = JSON.parse(redactSecrets(JSON.stringify(out.meta)))` and same for `action.details` if present. Alternatively, harden at write-time by redacting in `WideEventBuilder.annotate()` — but that's a wider blast radius.
|
||||
- **Ship-blocker?**: No. Risk is upstream-dependent and the v1.4.16 admin surface is single-tenant (Marc). Should land in next patch (v1.4.17) before any multi-admin scenario.
|
||||
|
||||
### H2 — `/api/admin/audit-log` returns raw `details` JSON to admins without redaction (B4)
|
||||
|
||||
- **Severity**: HIGH
|
||||
- **File**: `src/app/api/admin/audit-log/route.ts:127-149` (no `redactSecrets`); `src/components/admin/login-overview-section.tsx:148` (renders `entry.details` raw into the CSV export and React tree)
|
||||
- **Issue**: The audit-log GET returns `details` (a JSON string column) verbatim to admins. Some legacy audit rows can contain identifiers in `details` (e.g. `auth.login.failed → details: { identifier: <user-supplied login> }` at `src/app/api/auth/login/route.ts:63`). User-supplied `identifier` values are bounded but not filtered for token-shaped strings. If a user pastes `Bearer hlk_...` into the username field of a failed login (typo or recon), the next admin viewing `/admin/login-overview` sees the raw string in the JSON cell and the CSV export. This was acceptable when audit-log was admin-only ad-hoc; B4 productionised the surface (filter, paginate, CSV export), so the egress path is now warm.
|
||||
- **Recommendation**: Wrap `entry.details` in `redactSecrets()` at the API layer (parse JSON → walk strings → re-serialise) before returning. Same redaction belongs in the CSV export path on the client. Cheaper alternative: have `auditLog()` (in `src/lib/auth/audit.ts:16`) call `redactSecrets()` on every string-shaped value of `details` at write-time so the row never persists a token.
|
||||
- **Ship-blocker?**: No. Same reasoning as H1 — single-tenant for v1.4.16. Should ship in v1.4.17.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM
|
||||
|
||||
### M1 — Restore transaction is not bounded by an interactive-query timeout
|
||||
### M1 — `POST /api/insights/feedback` has no rate limit; aggregator pollution feasible
|
||||
|
||||
- **Severity**: MEDIUM
|
||||
- **File:Line**: `src/app/api/admin/backups/[id]/restore/route.ts:253-377`
|
||||
- **Issue**: The restore wraps deletes + creates in a single
|
||||
`prisma.$transaction(async (tx) => …)` with no `maxWait` / `timeout`
|
||||
override. Prisma's default 5s tx timeout will trip the moment a user
|
||||
has > ~10k intake events on a slow disk, leaving the user's data
|
||||
partially deleted (delete completes inside the same tx, but if
|
||||
`createMany` exceeds the timeout the rollback fires). The
|
||||
`admin.backups.restore.failed` audit row records the exception, but
|
||||
the operator only learns about the half-state from the next page-load
|
||||
showing zero medications and zero intake events, then has to manually
|
||||
re-run restore.
|
||||
- **Recommendation**: Pass `{ timeout: 30_000, maxWait: 5_000 }` (or a
|
||||
config-driven knob) to `prisma.$transaction()`. Empirically a 5–15
|
||||
MB backup completes in under a second; 30s is generous headroom.
|
||||
Alternative: split the transaction into delete-tx + create-tx with
|
||||
the audit log capturing the in-between state, but that's a bigger
|
||||
rewrite; the timeout fix is the minimum viable hardening.
|
||||
- **Ship-blocker?** No
|
||||
- **File**: `src/app/api/insights/feedback/route.ts:30-103`
|
||||
- **Issue**: The feedback endpoint requires auth + idempotency + Zod validation, but has no `checkRateLimit()` call. The unique key `(userId, recommendationId, recommendationText)` prevents replay-attack-style spam, but a determined user can submit thousands of distinct feedback rows by varying `recommendationId` (≤200 chars) and `recommendationText` (≤2000 chars). Each row is server-attributed to that user's `providerType` + `promptVersion`, so the attack distorts that user's slice in the aggregator — but cross-user buckets are also distorted because the aggregator buckets on `(severity, metricSourceType, providerType, promptVersion)` not on `userId`. A single user spamming "thumbs-down + providerType=codex" can shift the admin AI quality view's helpful-rate for the entire codex+severity slice.
|
||||
- **Recommendation**: Add `checkRateLimit("insights-feedback:" + user.id, 60, 60 * 60 * 1000)` (60 / hour seems generous — a user rates one rec per minute at most). Also consider: per-user weighting in the aggregator so a single user cannot dominate a bucket.
|
||||
- **Ship-blocker?**: No. Marc is single-tenant, so adversarial feedback is N/A in production.
|
||||
|
||||
### M2 — `metricSourceType` is client-supplied free-form text (B5e)
|
||||
|
||||
### M2 — Restored backup's `data` blob is re-encryptable but the upload route does not enforce key version
|
||||
- **Severity**: MEDIUM
|
||||
- **File:Line**: `src/app/api/admin/backups/upload/route.ts:193`,
|
||||
`src/lib/crypto.ts:199`
|
||||
- **Issue**: `encrypt(JSON.stringify(payload))` writes with the active
|
||||
key id. Good. But: no audit-log annotation captures which key id was
|
||||
used. After a future `ENCRYPTION_ACTIVE_KEY_ID` rotation, an admin
|
||||
inspecting "why does this old MANUAL_UPLOAD blob fail to decrypt
|
||||
today" has no audit trail telling them which key is needed. Same
|
||||
story for the `WEEKLY_AUTO` writes from the worker.
|
||||
- **Recommendation**: In the audit-log `details` for
|
||||
`admin.backups.upload`, include `extractKeyId(encrypted)` so the
|
||||
forensic trail can correlate a key-rotation incident to specific
|
||||
backup rows. (Pure observability — not a security boundary fix.)
|
||||
- **Ship-blocker?** No
|
||||
- **File**: `src/lib/validations/recommendation-feedback.ts:46` (Zod allows `z.string().min(1).max(80)`); `src/lib/jobs/feedback-aggregator.ts:70-92` (used as bucket key)
|
||||
- **Issue**: The Zod schema accepts any 1-80 char string for `metricSourceType`. There's no allow-list. A client can supply `metricSourceType: "../etc/passwd"` or `"<script>alert(1)</script>"` or any string. Two consequences:
|
||||
1. **Bucket-keyspace pollution**: each unique value spawns a fresh bucket, so a malicious client can balloon `AppSettings.adminAiInsightsFeedbackSummary` JSON unboundedly (limited only by 30-day rolling window × 80-char keys).
|
||||
2. **No XSS** (admin UI renders via React text node — verified `ai-quality-section.tsx:180`), but the value lands in the audit log `details` blob which then flows into B4's audit viewer.
|
||||
- **Recommendation**: Pin `metricSourceType` to the same allow-list the AI schema uses (`aiRecommendationSchema.metricSource.type` enum). Same closed enum is used everywhere else in the AI pipeline; only the feedback API accepts wildcard.
|
||||
- **Ship-blocker?**: No.
|
||||
|
||||
### M3 — Slug cache is process-global; same Map shared across users on multi-tenant deploys
|
||||
- **Severity**: LOW (informational — caller asked to verify)
|
||||
- **File:Line**: `src/lib/ai/codex-slug-cache.ts:32-34`
|
||||
- **Issue**: The cache key is the literal string `"codex"` — a single
|
||||
global slot. Verified per the spec; no per-user data lives in the
|
||||
cache (just an upstream-public slug like `"gpt-5.3-codex"`). No leak
|
||||
concern. However: in a multi-tenant deploy where users supply their
|
||||
own ChatGPT account, a slug accepted on user-A's account is
|
||||
optimistically tried on user-B's account. Worst case: one extra
|
||||
failed request before the chain walks. Not a security issue, but
|
||||
worth recording.
|
||||
- **Recommendation**: Document explicitly that the cache is global by
|
||||
design, with a future hook (the comment at lines 31-33 already
|
||||
hints) to scope by `accountId`. Add a Wide Event annotation when
|
||||
cache is invalidated due to a slug rejection so a flapping account
|
||||
is visible without user-attribution.
|
||||
- **Ship-blocker?** No
|
||||
### M3 — `audit-log` filter `target` substring on raw `details` field is broad
|
||||
|
||||
### M4 — `/api/admin/audit-log` returns full `details` JSON to any admin
|
||||
- **Severity**: MEDIUM
|
||||
- **File:Line**: `src/app/api/admin/audit-log/route.ts:34-53`
|
||||
- **Issue**: The `details` column is selected unfiltered. Some action
|
||||
rows now carry payloads written by the new v1.4.15 code:
|
||||
`integrations.sync.failed` includes the raw `message` string from
|
||||
upstream errors (lines 225-235 of `src/lib/integrations/status.ts`),
|
||||
and `system.deploy.unknown` keeps the **entire raw Coolify payload**
|
||||
(`raw: event.raw` at `src/app/api/internal/deploy-webhook/route.ts:199`).
|
||||
In a single-tenant deploy this is fine because every admin already
|
||||
has read-everything authority. In a future multi-tenant scenario,
|
||||
admins of tenant-A could see deploy-status payloads that may carry
|
||||
hostnames, deployment UUIDs, or upstream error text from tenant-B.
|
||||
Today this is a non-issue (single admin, single tenant) but the
|
||||
surface is wider than the new `<RecentAuditPreview>` shows.
|
||||
- **Recommendation**: Document the assumption ("admin.audit-log returns
|
||||
cross-tenant details — must be tightened before any multi-tenant
|
||||
release"). Flag for v1.5 when multi-tenant lands.
|
||||
- **Ship-blocker?** No
|
||||
- **File**: `src/app/api/admin/audit-log/route.ts:111-115` (`details: { contains: parsed.target }`)
|
||||
- **Issue**: The `target` query parameter does a Prisma `contains` substring match on the raw JSON-encoded `details` column. Prisma parameterizes the query (no SQL injection), but the substring match is **case-sensitive** on a JSON-encoded blob and very expensive on a large audit table (full table scan). An admin with `target=a` filter triggers a full-table scan + decoded substring on every row. With 10k+ audit rows + the new B4 paginated UI sweeping pages, the DB becomes the bottleneck. This is a DoS-via-admin not a security boundary breach.
|
||||
- **Recommendation**: Either (a) require `target` to be exact-match, (b) add a GIN/trigram index on `details` if substring is needed, or (c) bound `target` to ≥3 chars.
|
||||
- **Ship-blocker?**: No (admin-only).
|
||||
|
||||
### M5 — Test-send rate limits diverge across notification surfaces
|
||||
- **Severity**: LOW
|
||||
- **File:Line**:
|
||||
- `src/app/api/settings/telegram/test/route.ts:15` — 5/5min
|
||||
- `src/app/api/settings/ntfy/test/route.ts:16` — 5/5min
|
||||
- `src/app/api/admin/notifications/test/route.ts` — **no rate limit**
|
||||
- **Issue**: `POST /api/admin/notifications/test` sends a SYSTEM_ALERT
|
||||
through every enabled channel for the admin. It's `requireAdmin()`-
|
||||
gated but has no per-actor rate limit. A compromised admin session
|
||||
could spam Telegram/ntfy/web-push as fast as the upstream allows,
|
||||
burning quota and inviting upstream rate-limit penalties (Telegram's
|
||||
bot endpoint rate-limits aggressively and a 429 here would also
|
||||
affect legitimate medication-reminder traffic).
|
||||
- **Recommendation**: Add `checkRateLimit('admin-notifications-test:'
|
||||
+ admin.id, 5, 5 * 60_000)` mirroring the user-side handlers.
|
||||
- **Ship-blocker?** No
|
||||
### M4 — `User.aiProviderChain` JSON column allows tampering via direct DB edit but parser is defensive
|
||||
|
||||
### M6 — Backup download `Content-Disposition` filename uses the unescaped userId
|
||||
- **Severity**: LOW
|
||||
- **File:Line**: `src/app/api/admin/backups/[id]/download/route.ts:92-114`
|
||||
- **Issue**: `filename="healthlog-backup-${backup.userId}-${isoDate}.json"`
|
||||
is interpolated unescaped. UserIds are cuid/cuid2 (alphanumeric +
|
||||
`_-`) so no quoting attack is realistic against the current ID
|
||||
format, BUT the surface depends entirely on the ID generator never
|
||||
emitting a `"` or newline. If a future schema change ever swaps
|
||||
cuid2 for a UUID-with-format-error or a custom value, the unescaped
|
||||
interpolation becomes a header-injection vector.
|
||||
- **Recommendation**: Use `RFC 5987` `filename*=UTF-8''<percent-encoded>`
|
||||
or simply `filename="healthlog-backup-<isoDate>.json"` (no userId in
|
||||
the filename — the contents already carry it).
|
||||
- **Ship-blocker?** No
|
||||
- **Severity**: MEDIUM (informational)
|
||||
- **File**: `src/lib/ai/provider-chain.ts:78-120` (`parseProviderChain`)
|
||||
- **Issue**: The chain is stored as JSON, parsed defensively at every read with the malformed-row falls-back-to-default contract. The parser correctly: (a) deduplicates, (b) rejects unknown provider types, (c) normalises priority. However, the parser does NOT enforce that priority numbers are unique — a chain like `[{p:1, openai}, {p:1, codex}]` collapses by sort-stability into `[openai, codex]`. The `PUT` endpoint normalises priority to insertion order (route.ts:106-110) so this is not exploitable via the wire, but a direct DB edit could bypass. Defense-in-depth: `parseProviderChain()` normalises priority itself rather than trusting the persisted value.
|
||||
- **Recommendation**: Mirror the route's normalisation in `parseProviderChain` (`return validSorted.map((e, idx) => ({ ...e, priority: idx + 1 }))`).
|
||||
- **Ship-blocker?**: No (no exposure since the only writer is the `PUT` endpoint we control).
|
||||
|
||||
### M7 — Restore does NOT clear `IntegrationStatus`, `WithingsConnection`, encrypted credentials
|
||||
- **Severity**: MEDIUM
|
||||
- **File:Line**: `src/app/api/admin/backups/[id]/restore/route.ts:259-279`
|
||||
- **Issue**: The transaction deletes measurements, medications, intake
|
||||
events, mood entries, notification channels, push subscriptions,
|
||||
telegram-scheduled-deletions. But it does NOT clear: the user's
|
||||
`WithingsConnection` (encrypted access/refresh tokens at rest),
|
||||
`IntegrationStatus` rows, the user's `moodLogApiKeyEncrypted` /
|
||||
`moodLogUrlEncrypted`, OR Telegram credentials on the User row.
|
||||
Result: after a restore from a snapshot taken pre-Withings-connect,
|
||||
the user's old Withings tokens still work — they're "leaking through"
|
||||
the restore. For a single-user Marc instance this is irrelevant; for
|
||||
a future multi-user instance where a user requests a "wipe-and-
|
||||
restore-to-this-snapshot" workflow, the implicit assumption that
|
||||
restore == clean state is violated.
|
||||
- **Recommendation**: Either (a) document explicitly in the restore
|
||||
endpoint and admin UI that integrations + auth credentials persist
|
||||
through restore (current behaviour, may be desired for "don't wipe
|
||||
my Withings link"), or (b) extend the delete scope to mirror
|
||||
`DELETE /api/admin/data` exactly. Pick one and document.
|
||||
- **Ship-blocker?** No
|
||||
### M5 — `GET /api/user/ai-provider` decrypts the API key in memory just to expose last-4
|
||||
|
||||
### M8 — Deploy webhook 401 path leaks "secret configured" via warning side-channel (low impact)
|
||||
- **Severity**: LOW
|
||||
- **File:Line**: `src/app/api/internal/deploy-webhook/route.ts:84-96`
|
||||
- **Issue**: `hasValidSecret()` returns `false` when the env secret is
|
||||
unset, with side-effect `addWarning("DEPLOY_WEBHOOK_SECRET not
|
||||
configured")`. The HTTP response is 401 in both "secret unset" and
|
||||
"secret wrong" cases — no user-observable leak. But the
|
||||
Wide-Event/Loki annotation distinguishes them. If an attacker has
|
||||
read access to logs (out-of-scope by definition), they can probe
|
||||
whether the deploy webhook is configured. This is observable but
|
||||
not exploitable — included only because the caller explicitly asked
|
||||
about presence-of-secret leakage.
|
||||
- **Recommendation**: Acceptable. The warning is useful for ops triage
|
||||
("why isn't deploy paging me — did I forget to set the env?"). No
|
||||
change needed; documented for completeness.
|
||||
- **Ship-blocker?** No
|
||||
- **Severity**: MEDIUM (informational; pre-existing — not v1.4.16, but relevant given B2 expanded UI surface)
|
||||
- **File**: `src/app/api/user/ai-provider/route.ts:34-41` (`decrypt(...).slice(-4)` for OpenAI + Anthropic)
|
||||
- **Issue**: Every GET decrypts the full encrypted apiKey to extract the last 4 chars. The plaintext key briefly lives in the V8 string heap (~ms). If the response is logged with `responseBody` capture (per the idempotency replay-cache), there's no key in the response, but the in-process buffer / Glitchtip path could pick up a partial. Also, repeated decryption widens the window for a side-channel timing attack against AES-GCM (negligible in practice, but defense-in-depth).
|
||||
- **Recommendation**: Store a 4-char preview alongside the encrypted key on write (`aiOpenaiKeyPreview` column, NOT encrypted). Decrypt only when actually building a provider. Same applies to Anthropic.
|
||||
- **Ship-blocker?**: No. Pre-existing.
|
||||
|
||||
---
|
||||
|
||||
## Verified Safe (no findings)
|
||||
## LOW
|
||||
|
||||
- **B1 backup endpoints** — every endpoint passes through `requireAdmin()`
|
||||
(cookie-only). Restore: triple-confirm (`confirm: "RESTORE"`),
|
||||
idempotency-wrapped via `withIdempotency`, transaction atomicity,
|
||||
before+during+after audit logs (`admin.backups.restore.start` /
|
||||
`.failed` / final `admin.backups.restore`). Upload: 10 MB cap on
|
||||
both `Content-Length` AND `file.size`, `Content-Type` arrives as
|
||||
multipart/form-data, schema validation rejects malformed AND
|
||||
version-incompat (`isCompatibleSchemaVersion`), 3/min rate limit.
|
||||
Restored backups encrypted with same AES-256-GCM via `crypto.ts`
|
||||
(verified `encrypt(JSON.stringify(payload))` at upload route line
|
||||
193).
|
||||
### L1 — Recommendation text stored alongside helpful/notHelpful verdict in DB (per-user)
|
||||
|
||||
- **C2 deploy webhook HMAC verification** — uses
|
||||
`crypto.timingSafeEqual(Buffer.from(expected), Buffer.from(received))`
|
||||
with explicit length-equality precheck (lines 92-95). No raw
|
||||
string-compare. Rate limit by IP at 60/min (line 151) so a brute-
|
||||
force isn't economically viable. Validation ordering: rate-limit →
|
||||
auth-check → JSON parse → audit-log + dispatch. Secret never logged
|
||||
(`redactSecrets` would catch a `Bearer` accidentally written to an
|
||||
error message; the `getClientIp` and audit annotations in this
|
||||
route never include the secret value).
|
||||
- **Severity**: LOW
|
||||
- **File**: `src/app/api/insights/feedback/route.ts:64-75`; `prisma/migrations/0034_recommendation_feedback/migration.sql`
|
||||
- **Issue**: The audit details record `{recommendationId, severity, helpful, providerType, promptVersion}` — does NOT include `recommendationText` (good). However, `recommendationText` IS persisted to the `RecommendationFeedback` table itself (the dedup partner). An admin reading `recommendation_feedback` directly sees the user's full rec text alongside their helpful/notHelpful verdict. This is by design (Marc's research §3 single-user-default-on policy), but operators on a shared deployment should know.
|
||||
- **Recommendation**: Document the exposure in `docs/audit/v1416-summary.md` (privacy section) so future deployments understand the per-user-rec storage trade-off.
|
||||
- **Ship-blocker?**: No.
|
||||
|
||||
- **B2 sync robustness** — Withings + moodLog tokens encrypted at
|
||||
rest via `crypto.encrypt()`. Refresh-token race: each
|
||||
`getValidToken()` call reads the connection row, compares
|
||||
`tokenExpiresAt - 5min < Date.now()`, refreshes serially, and
|
||||
writes back via `prisma.update`. The 5-minute buffer is the same
|
||||
for parallel requests, so two concurrent requests COULD both
|
||||
attempt a refresh — but the second request will get an
|
||||
`invalid_grant` (Withings consumes the refresh token on first use)
|
||||
and route via `recordSyncFailure` to `error_reauth`. Acceptable
|
||||
because the failure is captured + audit-logged; the alternative
|
||||
(DB advisory lock) is over-engineered for a once-per-30min job.
|
||||
"Reauth required" state can't leak old tokens — token columns are
|
||||
preserved encrypted, but `getValidToken` returns null on reauth so
|
||||
no decrypt path runs. The `markReauthRequired` audit-log details
|
||||
do NOT include any token material (verified at status.ts line
|
||||
302).
|
||||
### L2 — Host-metrics endpoint returns ~1440 rows for `since=24h` with no streaming
|
||||
|
||||
- **B3 notification reliability** — counter manipulation: all
|
||||
consecutiveFailures mutations (`channel-state.ts:90, 102, 138`)
|
||||
happen server-side from dispatcher outcomes; no user-facing API
|
||||
accepts or modifies the count. Re-enable button: `POST
|
||||
/api/notifications/status` is `requireAuth()`-gated AND the
|
||||
`findFirst` query scopes by `userId: user.id` so one user can't
|
||||
re-enable another user's channel (line 108-111). Test-send: rate-
|
||||
limited on `/settings/{telegram,ntfy}/test` at 5/5min; admin test
|
||||
endpoint missing rate limit (see M5).
|
||||
- **Severity**: LOW
|
||||
- **File**: `src/app/api/admin/host-metrics/route.ts:32-56`
|
||||
- **Issue**: `since=24h` returns ~1440 rows (one per minute) in one JSON response. The route loads all into memory with the BPS computation. At 7d retention an admin could enumerate the full table by dropping the `since` filter — but the route caps to 24h via the enum, so the maximum response is bounded. Not a real DoS.
|
||||
- **Recommendation**: None required.
|
||||
- **Ship-blocker?**: No.
|
||||
|
||||
- **C1 AI hardening** — system prompt + user data: the user data
|
||||
passed in `params.userPrompt` is the snapshot JSON the route layer
|
||||
built from the user's OWN measurements, not free-form user input.
|
||||
Prompt-injection from a user manipulating their own snapshot is a
|
||||
hallucination concern (the strict prompt at
|
||||
`prompts/insight-generator.ts:53-72` enforces "ground in numbers,
|
||||
refuse out-of-scope") but not a cross-user data exposure. The
|
||||
fallback-chain cache is global (M3) but holds only the public slug
|
||||
string — no PII. Citations in the response cite only fields from
|
||||
the user's own snapshot (`metricSource.type` references like
|
||||
`bloodPressure`, `weight`); no cross-user data path exists.
|
||||
### L3 — `redactSecrets` regex does not cover "Authorization: Basic <base64>" or generic JWT shape
|
||||
|
||||
- **A2 /admin overview** — `RecentAuditPreview` fetches via
|
||||
`/api/admin/audit-log`, which is `requireAdmin()`-gated. No URL-
|
||||
guessing path lands a non-admin on the audit data (handler returns
|
||||
401 before query). System-status snapshot: surfaces `version`,
|
||||
`database` connection state, `worker.running`, `gitCommit`,
|
||||
`builtAt` — nothing infrastructural an external observer can't
|
||||
derive from `/api/version` (which is public). Not a leak.
|
||||
|
||||
- **B5 onboarding tour overlay** — `OnboardingTour` renders as
|
||||
`position: fixed; z-[200]` ABOVE the dashboard but its content is
|
||||
app-controlled (i18n keys, no user-supplied strings). Backdrop
|
||||
click-through is dismissive (Skip), not silent. Tooltip body is
|
||||
rendered via `{t(stop.bodyKey)}` (React-text, escaped — not
|
||||
`dangerouslySetInnerHTML`). No phishing-mask vector unless an
|
||||
attacker can write to `messages/*.json`, which is part of the
|
||||
build artifact, not user-supplied.
|
||||
|
||||
- **General checks**:
|
||||
- `dangerouslySetInnerHTML`: only one occurrence in the entire
|
||||
`src/` tree — `src/app/layout.tsx:93` for the inline theme-
|
||||
bootstrap script. Content is the static `themeScript` constant,
|
||||
not i18n.
|
||||
- All new API routes wrap in `apiHandler()` (verified in
|
||||
backups/run, backups/[id]/download, backups/[id]/restore,
|
||||
backups/upload, internal/deploy-webhook, integrations/status,
|
||||
notifications/status, onboarding/tour).
|
||||
- New logging respects `redactSecrets` — `redact.ts` catches
|
||||
`Bearer`, `bot<digits>:<token>`, `sk-`/`sk-ant-`, `hlk_`/`hlr_`,
|
||||
`?secret=`/`?code=`/`?token=`/`?api[_-]?key=`. The Codex
|
||||
client's local `redactBody` at `codex-client.ts:470` covers the
|
||||
same surface for upstream-error bodies that don't pass through
|
||||
the WideEvent error path.
|
||||
- No `console.log` in production code paths beyond
|
||||
`instrumentation.ts` (Glitchtip init), `crypto.ts` (dev-only
|
||||
key-padding warning), `reminder-worker.ts` (pg-boss adapter for
|
||||
library-level errors).
|
||||
- **Severity**: LOW (pre-existing)
|
||||
- **File**: `src/lib/logging/redact.ts:25-49`
|
||||
- **Issue**: The redaction rules cover `Bearer <token>`, `bot<n>:<token>`, `sk-…`, `hl[kr]_…`, and query-string secrets. They do NOT cover Basic auth headers or JWT (`eyJ...`) tokens. If an upstream provider ever echoes a JWT in an error message, redaction misses.
|
||||
- **Recommendation**: Add `Basic\s+\S+` and `\beyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\b` patterns. Risk of over-matching is low since both shapes are token-specific.
|
||||
- **Ship-blocker?**: No.
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
## Items checked, no finding
|
||||
|
||||
**No CRITICAL or HIGH ship-blockers.** v1.4.15 can ship. The 8 medium/
|
||||
low items are defence-in-depth recommendations for v1.4.16+; M1
|
||||
(transaction timeout) and M5 (admin test-send rate limit) are the
|
||||
two most worth folding in before a multi-user deploy.
|
||||
- **`POST /api/insights/feedback` auth, idempotency, schema** — all enforced (`apiHandler` + `requireAuth` + `withIdempotency` + Zod). PII: rec text is not in audit details.
|
||||
- **`GET /api/admin/host-metrics` admin gate** — `requireAdmin()` enforced. No host-info leak: the response is admin-only, no Bearer surface, the `dynamic = "force-dynamic"` guard prevents Edge cache surprise.
|
||||
- **`GET /api/admin/app-logs` admin gate + redactSecrets** — admin-gated; `redactSecrets()` applied to error.message, error.stack, warnings, user_agent, external_calls[*].error. (See H1 for the gap on `meta`.)
|
||||
- **`GET /api/admin/audit-log` admin gate + filter pagination** — admin-gated; Zod validates `actor`/`action`/`target`/`since`/`until`/`page`/`perPage`; Prisma parameterizes. No SQL injection.
|
||||
- **Export endpoints (`/api/export/*`)** — all wrapped in `apiHandler` + `requireAuth`; queries scoped `where: { userId: user.id }`; rate-limit `export:<userId>` 10/h shared bucket; `Content-Disposition` filenames embed cuid (alphanumeric, safe); audit-log entries per export type.
|
||||
- **Provider chain credential safety** — `resolveProviderForType()` (provider.ts:325) refuses to forward a stale `aiBaseUrl` to OpenAI/Anthropic; LOCAL is the only path that uses `aiBaseUrl`, and the LOCAL path goes through `isPublicUrl()` SSRF guard. AES-256-GCM encryption preserved; no plaintext key in any response.
|
||||
- **Confidence override safety** — `applyConfidenceOverride()` (generate-insight.ts:214-240) discards `rec.confidence` from the model and replaces with `computeConfidence()`. Wide-event annotation captures the model's value separately for admin observability.
|
||||
- **Feedback attribution** — `resolveFeedbackAttribution()` (feedback-attribution.ts:70-86) reads the user's most recent `insights.generate` audit row; `pickProviderType()` priority is `chainProviderType > providerType > "unknown"`. Client cannot supply `providerType` / `promptVersion` (Zod schema omits both).
|
||||
- **GDPR cascade-delete** — `RecommendationFeedback` migration 0034 has `ON DELETE CASCADE` on `user_id` FK; integration test `tests/integration/cascade-delete.test.ts:102+188` verifies the row is gone after user deletion.
|
||||
- **Comparison snapshot scope** — `buildComparisonSnapshotForUser()` (generate/route.ts:86-150) reads `where: { userId, type }` per metric. No cross-user data path exists.
|
||||
- **AI section masking** — `<PasswordInput>` used for all three apiKey fields (ai-section.tsx:810, 996, 1158). Last-4 preview only (`...wxyz` shape).
|
||||
- **Connect/disconnect routes** — `PATCH /api/user/ai-provider` is `requireAuth`-gated. The new `PUT /api/insights/provider-chain` is `requireAuth`-gated, dedupes provider entries, and normalises priority.
|
||||
- **Provider selection persists per-user** — `User.aiProviderChain` is per-user; no cross-tenant query path.
|
||||
- **Host-metrics retention cleanup** — `runHostMetricTick` deletes only from `prisma.hostMetric` (`host-metric-sampler.ts:163`); cannot touch unrelated tables.
|
||||
- **In-memory log buffer growth** — `LOG_BUFFER_MAX = 500` hard cap, FIFO eviction (`in-memory-buffer.ts:24-37`). Per-process. No exposure to attacker-controlled growth (the sampler gates on level).
|
||||
- **JSON inspector modal XSS** — `<pre>{JSON.stringify(selected, null, 2)}</pre>` (app-log-preview-section.tsx:282-284). Renders via React text node, no `dangerouslySetInnerHTML`.
|
||||
- **i18n strings via `dangerouslySetInnerHTML`** — no occurrences across v1.4.16 surface (only `src/app/layout.tsx:93` for the theme-init script — pre-existing, unrelated).
|
||||
- **`apiHandler()` wrap** — verified for `/api/insights/feedback`, `/api/insights/provider-chain` (GET+PUT), `/api/admin/host-metrics`, `/api/admin/app-logs`, `/api/admin/audit-log`, `/api/admin/audit-log/actions`, `/api/admin/ai-quality`, `/api/export/measurements`, `/api/export/medications`, `/api/export/mood`, `/api/export/full-backup`. All present.
|
||||
- **New routes log auth context via apiHandler** — confirmed via `requireAuth()` / `requireAdmin()` annotation chain.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
The v1.4.16 release ships clean. No CRITICAL findings; the two HIGHs (H1 + H2) are defense-in-depth gaps that matter only at multi-admin scale, which HealthLog has not yet reached. Recommend tracking H1 + H2 + M1 + M2 as a v1.4.17 hardening bucket; M3-M5 + L1-L3 are nice-to-have.
|
||||
@@ -1,422 +1,619 @@
|
||||
# Phase D — Senior-dev review (v1.4.15)
|
||||
# Phase D — Senior-dev review (v1.4.16)
|
||||
|
||||
Lens: structure, file-size discipline, naming, separation of concerns,
|
||||
layering coherence, test architecture, premature abstraction. Distinct
|
||||
from code-review (correctness/bugs), security, design, simplify.
|
||||
|
||||
Scope: 131 changed src files in `git diff v1.4.14...HEAD`, with focus
|
||||
on the largest deltas (B1 backups, B2 integrations, B3 notifications,
|
||||
B5 onboarding tour, C1 AI hardening, C2 deploy webhook).
|
||||
Scope: 167 changed files in `git diff v1.4.15...HEAD` (+20,357 / −2,031
|
||||
lines). Focus on the largest deltas: AI module (`src/lib/ai/` grew with
|
||||
B5a refs, B5b chain runner, B5c rationale, B5d confidence, B5e feedback),
|
||||
chart wrappers (`health-chart.tsx` +729, `mood-chart.tsx` +542), the
|
||||
recommendations surface (`<RecommendationCard>` 417 new lines,
|
||||
`<RecommendationsGrid>`, `<RecommendationFeedback>`, `<ConfidenceMeter>`),
|
||||
admin extensions (host-metrics, app-logs, audit-log filters, ai-quality),
|
||||
and B7's settings-export consolidation.
|
||||
|
||||
Verdict at a glance: **0 CRITICAL, 4 HIGH, 8 MED/LOW**. The marathon
|
||||
phases produced overall well-structured code — every API route uses
|
||||
`apiHandler()`, every mutation goes through `requireAuth()` /
|
||||
`requireAdmin()`, every sensitive value is encrypted via `crypto.ts`,
|
||||
and zero `console.*` calls landed. The findings below are real
|
||||
maintenance debt but none of them block the v1.4.15 ship.
|
||||
Verdict at a glance: **0 CRITICAL, 3 HIGH, 9 MED/LOW**. The marathon
|
||||
hit its file-size target overall — only one v1.4.16-touched file is
|
||||
visibly over the comfortable ceiling and the named-slot pattern in
|
||||
`<RecommendationCard>` kept it under 420 lines despite five buckets
|
||||
plugging into it. Worktree adoption (last milestone's H3) worked well:
|
||||
the cross-agent staging races mostly fall into "wrong commit subject"
|
||||
territory rather than lost code. The findings below are real maintenance
|
||||
debt; none of them block the v1.4.16 ship.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL — none
|
||||
|
||||
(There are no architectural mistakes that need to be fixed before
|
||||
shipping v1.4.15.)
|
||||
(No architectural mistakes need fixing before v1.4.16 ships.)
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1 — `src/app/page.tsx` (1031 lines) crossed the "single dashboard component is doing too much" threshold
|
||||
### H1 — `src/lib/ai/` directory has 19 flat files (4,726 lines) with five orthogonal responsibility groups; the named-slot success of `<RecommendationCard>` shows the AI module deserves the same sub-directory split
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/app/page.tsx` (1031 lines, was ~838 in v1.4.14, +193 lines this milestone)
|
||||
- **Issue**: The dashboard root now juggles eight orthogonal concerns
|
||||
in one component: data fetching (analytics + steps + glucose +
|
||||
layout), tile/chart visibility resolution (new `tileVisible` vs
|
||||
`visible` dual-flag mirroring with legacy fallback), quick-add
|
||||
dialog routing, getting-started checklist, onboarding tour
|
||||
launcher gating, achievements card slot, dynamic chart imports,
|
||||
and an on-screen empty-state branch. The phase-A4 dual-flag
|
||||
resolver and phase-B5 tour-id wiring landed on top of an already
|
||||
long file rather than triggering a split. Every future widget added
|
||||
to the dashboard is a +30-line patch to this single file.
|
||||
- **Recommendation**: Pull the visibility resolver
|
||||
(`isTileVisible` / `isChartVisible` / widget gating constants)
|
||||
into `src/lib/dashboard-visibility.ts`, and split the dashboard
|
||||
page into a `<DashboardShell>` (RSC if data permits, otherwise
|
||||
client) + `<DashboardTileStrip>` + `<DashboardChartGrid>` +
|
||||
`<DashboardEmptyState>` trio. The current file already has
|
||||
natural split points in the comment headers — the refactor is
|
||||
mechanical. Suggested for v1.4.16, NOT v1.4.15 (touching this
|
||||
file mid-marathon would block five other phases' tests).
|
||||
- **Ship-blocker?**: no. Document in v1.4.16 backlog.
|
||||
- **Files / pattern**: `/Users/marc/Projects/HealthLog/src/lib/ai/` —
|
||||
`provider.ts` (515), `codex-client.ts` (484), `provider-runner.ts`
|
||||
(371), `codex-oauth.ts` (366), `schema.ts` (329), `generate-insight.ts`
|
||||
(315), `medical-references.ts` (185), `provider-chain.ts` (137),
|
||||
`types.ts` (132), `confidence.ts` (102), `citation-coverage.ts` (94),
|
||||
`local-client.ts` (92), `openai-client.ts` (88), `feedback-attribution.ts`
|
||||
(86), `legacy-payload.ts` (55), plus `prompts/` already split out.
|
||||
- **Issue**: Five distinct concerns sit at the same flat depth, all
|
||||
importing each other:
|
||||
- **Provider clients**: `codex-client.ts`, `codex-oauth.ts`,
|
||||
`codex-slug-cache.ts`, `openai-client.ts`, `anthropic-client.ts`,
|
||||
`local-client.ts`, `mock-client.ts` (7 files).
|
||||
- **Provider resolution / chain runtime**: `provider.ts`,
|
||||
`provider-chain.ts`, `provider-runner.ts` (3 files; `provider.ts`
|
||||
AND `provider-runner.ts` both export 4 different
|
||||
"resolve/run" entry points that route layers must navigate).
|
||||
- **Schema / validation / types**: `schema.ts`, `types.ts` —
|
||||
two response shapes still co-exist (last milestone's M6 — see
|
||||
M5 below for the v1.4.16 update).
|
||||
- **Domain logic**: `confidence.ts`, `citation-coverage.ts`,
|
||||
`legacy-payload.ts`, `feedback-attribution.ts`,
|
||||
`medical-references.ts`, `generate-insight.ts`.
|
||||
- **Prompts**: `prompts/` already a sub-directory (the only one).
|
||||
Today an AI-feature add lands one file in the flat `ai/` and one
|
||||
in `ai/prompts/`. Tomorrow's "B6: prompt feedback ratchet" or
|
||||
"B7: tool-call provider" will keep stuffing the flat directory.
|
||||
- **Recommendation**: Re-shape to:
|
||||
```
|
||||
src/lib/ai/
|
||||
providers/ ← codex-*, openai-*, anthropic-*, local-*, mock-*
|
||||
runtime/ ← provider.ts (resolve), provider-chain.ts,
|
||||
provider-runner.ts, generate-insight.ts
|
||||
schema/ ← schema.ts, types.ts (after merge — see M5)
|
||||
domain/ ← confidence.ts, citation-coverage.ts,
|
||||
legacy-payload.ts, feedback-attribution.ts,
|
||||
medical-references.ts
|
||||
prompts/ ← unchanged
|
||||
```
|
||||
This is a mechanical rename + import-rewrite pass that the test
|
||||
suite will catch in seconds. The named-slot pattern in
|
||||
`<RecommendationCard>` proved the codebase responds well to
|
||||
thoughtful structural splits; the AI module needs the same
|
||||
treatment before v1.4.17 / v1.5 add another bucket.
|
||||
- **Ship-blocker?**: no. v1.4.17 housekeeping.
|
||||
|
||||
### H2 — `src/components/settings/integrations-section.tsx` (883 lines) hosts two unrelated feature cards in one client module
|
||||
### H2 — `src/components/insights/insight-advisor-card.tsx` (690 lines) has become a god-component carrying 4 inline sub-components, 3 lookup tables, the legacy-payload CTA, the data-quality drawer, and the recommendations grid wrapping
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/settings/integrations-section.tsx` (883 lines after B2's +199-line addition)
|
||||
- **Issue**: A single 883-line file holds (a) the shared
|
||||
`useIntegrationStatuses` query hook + `IntegrationStatusBanner` +
|
||||
`StatusBadge` primitives, (b) `<WithingsCard>` which is itself
|
||||
~400 lines with credentials form + sync handlers + connect flow,
|
||||
(c) `<MoodLogCard>` with its own form + status. The
|
||||
Withings/moodLog cards share nothing meaningful at runtime —
|
||||
each fetches its own `/status` endpoint, has its own mutation
|
||||
set, and renders its own credentials section. Co-locating them
|
||||
was a v1.3 leftover; the v1.4.15 status-banner addition makes
|
||||
the file harder to navigate (search for "Withings" returns
|
||||
~100 hits).
|
||||
- **Recommendation**: `src/components/settings/integrations/`
|
||||
directory with `integration-status-banner.tsx` (shared),
|
||||
`withings-card.tsx`, `moodlog-card.tsx`, and an
|
||||
`integrations-section.tsx` index that just composes them.
|
||||
Mirror the per-section split that admin already follows
|
||||
(`src/components/admin/<feature>-section.tsx` per phase 4b).
|
||||
Mechanical refactor, no behavioural change.
|
||||
- **Ship-blocker?**: no.
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/insights/insight-advisor-card.tsx`
|
||||
(690 lines, was 582 in v1.4.15 — +108 from B5c legacy-payload CTA
|
||||
+ B1b summary mini-charts + recommendations-grid wrapping)
|
||||
- **Issue**: Single client module hosts:
|
||||
- `<HeroFinding>` (~60 lines, severity-tinted top-finding card)
|
||||
- `<InlineCharts>` (~50 lines, chart-token expander)
|
||||
- `<SectionSeparator>` (~15 lines, plus a duplicated comment header
|
||||
"Section Separator" appearing twice in the file at lines 284 and
|
||||
286 — copy-paste residue)
|
||||
- `<AssessmentIcon>` (icon switcher)
|
||||
- 4 module-scope lookup tables (`CLASSIFICATION_STYLES`,
|
||||
`HERO_STYLES`, `CONFIDENCE_STYLES`, `INLINE_CHART_TITLE_KEYS`,
|
||||
`CONFIDENCE_LABEL_KEYS`)
|
||||
- The main `<InsightAdvisorCard>` with loading / empty / error /
|
||||
full branches plus the legacy-payload CTA banner plus the
|
||||
data-quality collapsible drawer
|
||||
When the next phase (e.g. B5f or v1.4.17 comparison-callout)
|
||||
needs to add a UI block, the only natural insertion point is more
|
||||
inline JSX inside this file. The component already has 5 separate
|
||||
concerns; the named-slot discipline that worked for
|
||||
`<RecommendationCard>` was NOT applied here.
|
||||
- **Recommendation**: Extract:
|
||||
- `src/components/insights/_advisor/hero-finding.tsx` — `<HeroFinding>` + `HERO_STYLES` + `<AssessmentIcon>`
|
||||
- `src/components/insights/_advisor/inline-charts.tsx` — `<InlineCharts>` + the title-keys map
|
||||
- `src/components/insights/_advisor/data-quality-drawer.tsx` — collapsible block
|
||||
- `src/components/insights/_advisor/legacy-payload-cta.tsx` — banner
|
||||
- `<InsightAdvisorCard>` becomes a slim composer (~150 lines) that
|
||||
just orchestrates loading/empty/error and delegates to the
|
||||
extracted blocks. Mirror the `<AdminShell>` per-section split
|
||||
that v1.4.15 phase B4 already established as the codebase
|
||||
pattern.
|
||||
Remove the duplicate `// ─── Section Separator ───` comment block.
|
||||
- **Ship-blocker?**: no. v1.4.17 housekeeping.
|
||||
|
||||
### H3 — Phase-D agent reports show repeated parallel-agent staging race; B1 / B6 / C1 commits each claim "swept into a sibling commit", indicating the multi-agent workflow has no isolation
|
||||
### H3 — `src/components/insights/recommendation-card.tsx` (417 lines) is approaching the size where the named-slot pattern alone won't scale — 5-bucket plug-in pressure now bears on `<RationaleCard>`'s prop list (6 props, plus 5 derived feedbackProps)
|
||||
|
||||
- **Files**: tracked across `STATE.md` phase-status blocks (A2, A4, B-mobile, B1, B2, B3, B4, B6, C1, C2, C5)
|
||||
- **Issue**: STATE.md repeatedly flags "shared-cwd / shared-index
|
||||
race" — phase A2's commit absorbed phase A4's diff, phase C1's
|
||||
schema files rode along on a sibling C5 commit, phase C2's docker
|
||||
workflow files landed under phase B4's achievements message, etc.
|
||||
This isn't a code-correctness problem (the working tree on
|
||||
`origin/main` is correct), but it IS a structural-hygiene problem:
|
||||
`git log` no longer maps message-to-diff faithfully, so future
|
||||
bisects / blame-reads / changelog-builders see misleading
|
||||
attribution. Eight separate commits in the v1.4.15 marathon carry
|
||||
this flag.
|
||||
- **Recommendation**: v1.4.16 marathon MUST adopt
|
||||
`superpowers:using-git-worktrees` per parallel agent — each
|
||||
phase agent runs in its own worktree (e.g.
|
||||
`/Users/marc/projects/healthlog-A1/`,
|
||||
`/Users/marc/projects/healthlog-B3/`) so `git add`/`git commit`
|
||||
operate on isolated indexes. STATE.md's own status-block list at
|
||||
the bottom of the file reads like a sustained alarm — fixing
|
||||
the workflow is overdue. Already noted by every phase author;
|
||||
promoting to a HIGH for visibility.
|
||||
- **Ship-blocker?**: no — the working tree on `main` is correct.
|
||||
|
||||
### H4 — `MockAIProvider`'s `DEFAULT_RESPONSE` carries the legacy v1.4.14 rich-shape JSON, NOT the strict v1.4.15 schema — silent test-fixture drift
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/ai/mock-client.ts` lines 42-50
|
||||
- **Issue**: The default response shipped by `MockAIProvider`
|
||||
produces `{summary, classification, findings, correlations,
|
||||
recommendations, dataQuality, disclaimer}` — the legacy v1.4.14
|
||||
shape. The v1.4.15 strict schema (`aiInsightResponseSchema`)
|
||||
requires `{summary, recommendations, citations, warnings}` with
|
||||
`recommendations[].metricSource` mandatory. The mock's default
|
||||
PASSES the strict schema only because of `.passthrough()` — but
|
||||
every test that uses the default is exercising a code path the
|
||||
production strict-mode wrapper rejects. When v1.4.16 retires the
|
||||
passthrough (per the C1 plan), every test using the default mock
|
||||
response will silently start failing schema-validation before the
|
||||
asserts run.
|
||||
- **Recommendation**: Make `DEFAULT_RESPONSE` produce a v1.4.15
|
||||
strict-schema-conformant payload (`citations: []`,
|
||||
`recommendations: []`, `warnings: []`) and remove the legacy
|
||||
fields. Tests that need the legacy shape can opt in via
|
||||
`responses: ["..."]`. This is one-line of test-fixture work that
|
||||
saves a v1.4.16 surprise.
|
||||
- **Ship-blocker?**: no.
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/insights/recommendation-card.tsx`
|
||||
- **Issue**: The named-slot architecture (`data-slot="rec-confidence-slot"`,
|
||||
`data-slot="rec-feedback-slot"`) was the right call in B5c — five
|
||||
buckets plugged in across B5c/d/e without merge conflicts. But the
|
||||
result is now a 417-line file where:
|
||||
- `RationaleCard` takes 6 props (`rationale`, `metricSource`,
|
||||
`referenceId`, `confidence`, `locale`, `feedbackProps`)
|
||||
- `feedbackProps` itself is a 5-field object built inline at the
|
||||
parent level via `asFeedbackTimeRange()` defence-in-depth
|
||||
- Three local helpers (`metricTypeToChartTypes`,
|
||||
`isMoodMetric`, `isComplianceMetric`) duplicate vocabulary that
|
||||
already lives in `src/components/charts/mini-window.ts` and
|
||||
`src/lib/insights/chart-tokens.ts`
|
||||
- `<CitationFootnote>` is defined inline AND consumed twice
|
||||
(expanded rationale + non-expandable legacy fallback)
|
||||
The component is still readable today, but the next bucket (e.g.
|
||||
B5f data-export-per-rec, or a v1.4.17 "share rec" affordance)
|
||||
will push it past 500 lines.
|
||||
- **Recommendation**:
|
||||
- Extract `metricTypeToChartTypes` + `isMoodMetric` +
|
||||
`isComplianceMetric` to `src/lib/insights/rec-metric-routing.ts`
|
||||
(5-line file, but turns three duplicated `.toLowerCase()`
|
||||
branches into a single source of truth shared with
|
||||
`<InlineCharts>`).
|
||||
- Extract `<CitationFootnote>` to
|
||||
`src/components/insights/citation-footnote.tsx` — already used
|
||||
twice, no reason to keep it inline.
|
||||
- Extract `<RationaleCard>` to
|
||||
`src/components/insights/recommendation-rationale-card.tsx` —
|
||||
that's the natural split point, and it lets the prop list shrink
|
||||
by hiding the 5 feedback fields behind a single `rec` parameter.
|
||||
- Keep `asFeedbackTimeRange()` next to where it's used — it's
|
||||
defence-in-depth for the API contract, not a candidate for
|
||||
extraction.
|
||||
After this split `recommendation-card.tsx` drops to ~250 lines and
|
||||
the named-slot pattern keeps working for the next bucket.
|
||||
- **Ship-blocker?**: no. Pre-emptive split for v1.4.17.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM
|
||||
|
||||
### M1 — `IntegrationStatus` and `NotificationChannel` reliability state machines duplicate the same shape (last-success, last-attempt, consecutive-failures, alert/cooldown stamp) without a shared abstraction
|
||||
### M1 — `src/lib/ai/provider-runner.ts` carries TWO near-identical fallback runners (`runWithFallback` + `runRawCompletionWithFallback`) with a "v1.4.17 will delete the legacy variant" comment — premature abstraction OR live-bridge debt
|
||||
|
||||
- **Files**: `src/lib/integrations/status.ts`, `src/lib/notifications/channel-state.ts`, `src/lib/notifications/retry-policy.ts`
|
||||
- **Issue**: Two parallel state machines were built in the same
|
||||
marathon. Both store `lastSuccessAt` / `lastAttemptAt` /
|
||||
`consecutiveFailures` / a "skip until time" stamp /
|
||||
classifications of permanent-vs-transient failures, both fan out
|
||||
through `dispatchNotification(SYSTEM_ALERT)` for admin paging, and
|
||||
both define classify-error helpers (`isWithingsRefreshReauthFailure`,
|
||||
`classifyTelegramError`, `classifyHttpStatus`). They diverge in
|
||||
small ways (integration uses `kind`, notification uses
|
||||
`hardReject` boolean; integration uses `alertedAt` 24h window,
|
||||
notification uses `nextRetryAt` Date) but the SHAPE is the same.
|
||||
No CRITICAL because the divergence is consistent within each
|
||||
module — but if a v1.5 webhook integration arrives, will it copy
|
||||
the integration shape or the notification shape? The choice is
|
||||
arbitrary today.
|
||||
- **Recommendation**: v1.4.16 — extract `src/lib/reliability/`
|
||||
with `ReliabilityState` + `recordSuccess` / `recordFailure` /
|
||||
`shouldAttempt` primitives, then have integrations + notifications
|
||||
+ (future) AI providers all consume the same surface. Don't
|
||||
refactor in v1.4.15 — the abstraction needs at least one more
|
||||
consumer to be obvious.
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/lib/ai/provider-runner.ts`
|
||||
lines 229-371 (143-line duplication)
|
||||
- **Issue**: Two parallel async functions that differ only in the
|
||||
inner call site (`generateInsight()` vs
|
||||
`provider.generateCompletion()`). Both run the same chain
|
||||
walkthrough, both classify hard failures the same way, both reach
|
||||
for the same `applyLastWorkingCache` + `summariseError` helpers,
|
||||
both emit the same Wide-Event annotations. The comment at line 296
|
||||
says "v1.4.17 will migrate this route to the strict wrapper… at
|
||||
that point this helper is removed." If that's true, the duplication
|
||||
is a one-cycle bridge — fine. If v1.4.17 slips, the duplication
|
||||
doubles every fallback-policy change.
|
||||
- **Recommendation**: Either:
|
||||
- **Inline-extract the chain walker** into a generic
|
||||
`runChain<T>(chain, attempt)` higher-order function and call it
|
||||
from both wrappers (~40-line reduction). Today.
|
||||
- OR commit to the v1.4.17 deletion as a calendar item and add a
|
||||
`// REMOVE: v1.4.17` marker the cleanup phase can grep for.
|
||||
Without one of those, the duplication will outlive its rationale.
|
||||
|
||||
### M2 — `src/lib/notifications/dispatcher.ts` carries an inline auto-migration of legacy `User.telegramBotToken/chatId` columns into `NotificationChannel` rows; the migration code has been there since v1.3 and should be deleted
|
||||
### M2 — `<RecommendationFeedback>` reaches into `useAuth()` directly to scope the localStorage cache key; this couples a presentational rec-card slot to the auth subsystem
|
||||
|
||||
- **File**: `src/lib/notifications/dispatcher.ts` lines 49-105
|
||||
- **Issue**: 56 lines of "first-dispatch on-the-fly migration" code
|
||||
reading `user.telegramBotToken` / `telegramEnabled` and writing
|
||||
a `NotificationChannel` row. This was a v1.3 cutover bridge.
|
||||
Three milestones later, every active user has been through a
|
||||
dispatch (since reminders fire daily), so the legacy columns
|
||||
have been migrated for everyone. Keeping the bridge in the
|
||||
dispatcher hot-path means every notification dispatch reads
|
||||
three deprecated columns from `User` whenever the user has no
|
||||
`NotificationChannel` row of type `TELEGRAM` (which on a fresh
|
||||
install IS the common case).
|
||||
- **Recommendation**: Confirm via a one-off SQL audit that no rows
|
||||
have non-null `telegramBotToken` AND no `NotificationChannel`
|
||||
with `type='TELEGRAM'`, then delete the migration block + the
|
||||
three legacy User columns in a v1.4.16 schema migration. The
|
||||
block has a "Best-practice red flags / Backward-compat shims for
|
||||
code paths that no longer exist" smell.
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/insights/recommendation-feedback.tsx`
|
||||
lines 33, 134, 137-154
|
||||
- **Issue**: The component takes a 5-field props bag describing the
|
||||
rec, then ALSO reaches into `useAuth()` to obtain `user.id` for
|
||||
the localStorage cache key. The localStorage scoping by user id is
|
||||
correct (a tour-launcher fix from H4 in v1.4.15 catch-up did the
|
||||
same thing for the same reason), but the feedback button is a
|
||||
"given a rec, render thumbs" leaf — it shouldn't know about auth.
|
||||
The hydration race the component handles in render
|
||||
(`hydratedFor !== user.id` — `setState` during render to dodge the
|
||||
`react-hooks/set-state-in-effect` lint) is a 17-line dance to
|
||||
satisfy a contract that could be `userId: string` on the props.
|
||||
- **Recommendation**: Have the parent `<RecommendationCard>` pull
|
||||
`user.id` from `useAuth()` once and pass it as `userId` into
|
||||
`feedbackProps`. The feedback component drops to a pure
|
||||
function-of-props leaf: simpler render, no more hydration dance,
|
||||
one less hook call per rendered card. The `useMutation` call
|
||||
stays — that's behaviour, not auth.
|
||||
|
||||
### M3 — `src/components/admin/backups-section.tsx` (529 lines) embeds `<RestoreRowDialog>`, `formatBytes`, three TanStack mutations, and a download handler in one client module
|
||||
### M3 — `src/lib/ai/types.ts` (132 lines) and `src/lib/ai/schema.ts` (329 lines) STILL define two response shapes; v1.4.15 M6 flagged this and v1.4.16 added rationale + confidence to BOTH instead of finishing the migration
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/admin/backups-section.tsx`
|
||||
- **Issue**: 529 lines for a single admin section. The
|
||||
`<RestoreRowDialog>` sub-component (typed-confirmation gate) is
|
||||
reusable across other destructive admin flows but lives buried
|
||||
inside the backups section. The `formatBytes` helper is generic.
|
||||
The download `handleDownload` async function with blob/object-URL
|
||||
plumbing is generic too. None of these are wrong on their own,
|
||||
but they push the section past the comfortable 400-line ceiling
|
||||
and make the file slow to navigate.
|
||||
- **Recommendation**: Extract to:
|
||||
- `src/components/admin/_dialogs/destructive-typed-confirm.tsx` —
|
||||
reusable `<TypedConfirmDialog confirmWord="RESTORE" ... />`
|
||||
- `src/lib/format-bytes.ts` (or co-located in `src/lib/format.ts`)
|
||||
- `src/lib/download-blob.ts` for the `<a download>` plumbing
|
||||
Backups-section then drops to ~300 lines.
|
||||
- **Files**: `src/lib/ai/types.ts`, `src/lib/ai/schema.ts`
|
||||
- **Issue**: Last milestone's senior-dev review M6 called the
|
||||
dual-schema state architectural smell. v1.4.16 doubled down:
|
||||
- `aiRecommendationRationaleSchema` (schema.ts) AND
|
||||
`insightRecommendationRationaleSchema` (types.ts)
|
||||
- `aiRecommendationSchema.confidence` AND
|
||||
`insightRecommendationSchema.confidence`
|
||||
- `metricSource` defined twice with subtly different field
|
||||
requirements (`schema.ts` requires `summary` non-empty;
|
||||
`types.ts` makes it required-but-permissive)
|
||||
Each duplicate field has comments saying "mirrors the strict
|
||||
schema in schema.ts" but there's no enforcement that they actually
|
||||
do — drift between the two would only show up when a cached
|
||||
payload deserialises differently between the two parsers.
|
||||
- **Recommendation**: Bite the migration. v1.4.17 must either:
|
||||
- **Delete `insightResultSchema`** entirely; renderer migrates to
|
||||
consume `AIInsightResponse` directly (the .passthrough() shape
|
||||
already accepts both for cached payload back-compat).
|
||||
- OR explicitly define `InsightResult = AIInsightResponse & {
|
||||
classification, findings, ... }` in types.ts as an extension of
|
||||
the canonical schema — single source of truth, types.ts becomes
|
||||
the dashboard-renderer-specific projection.
|
||||
Carrying two schemas through another milestone risks a real
|
||||
divergence bug.
|
||||
|
||||
### M4 — `<OnboardingTour>` (418 lines) embeds layout-math (`computeTooltipPosition`, `measureTarget`), state-machine consumer logic, and rendering in one component
|
||||
### M4 — `src/components/insights/recommendation-card.tsx`'s `metricTypeToChartTypes()` and `<InlineCharts>` in `insight-advisor-card.tsx` both maintain their own metric-type vocabulary; no shared source
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/components/onboarding/tour.tsx`
|
||||
- **Issue**: The pure tour-state machine WAS extracted (good — see
|
||||
`src/lib/onboarding/tour-state.ts`), but the spotlight-positioning
|
||||
math (`measureTarget` + `computeTooltipPosition` with the
|
||||
candidate-flip heuristic) is inline in the React component. That's
|
||||
~100 lines of pure layout math the v1.4.15 tests don't touch
|
||||
(the only test asserts SSR shape). When v1.4.16 wants to add
|
||||
reduced-motion, RTL flipping, or an arrow connector, every change
|
||||
re-renders the whole tour module.
|
||||
- **Recommendation**: Extract `src/lib/onboarding/tour-positioning.ts`
|
||||
with `measureTarget`/`computeTooltipPosition` + unit tests
|
||||
(deterministic, DOM-free with a synthetic `Element` stub for the
|
||||
`getBoundingClientRect` call). Keeps the React component focused
|
||||
on rendering + event wiring.
|
||||
- **Files**: `src/components/insights/recommendation-card.tsx`
|
||||
lines 114-135, `src/components/insights/insight-advisor-card.tsx`
|
||||
lines 216-230
|
||||
- **Issue**: Two parallel mappings:
|
||||
- `metricTypeToChartTypes()` maps lowercased snapshot keys
|
||||
("bloodpressure", "weight", "bodyfat", …) to the
|
||||
measurement-type enum strings.
|
||||
- `INLINE_CHART_TITLE_KEYS` maps measurement-type enum strings
|
||||
("WEIGHT", "BLOOD_PRESSURE_SYS", …) to i18n title keys.
|
||||
Both grew with the same v1.4.16 phase. A future "add SLEEP_QUALITY
|
||||
metric" requires editing both files, with no compile-time guard
|
||||
that they stayed in sync.
|
||||
- **Recommendation**: Extract one
|
||||
`src/lib/insights/metric-vocab.ts` exposing
|
||||
`MEASUREMENT_TYPES_BY_SNAPSHOT_KEY` + `MEASUREMENT_TITLE_KEYS_BY_TYPE`
|
||||
+ helper `resolveMetricRoute(snapshotKey)`. Both consumers
|
||||
import. Adds one file, eliminates a hidden coupling.
|
||||
|
||||
### M5 — Naming inconsistency: `application_name` (snake_case) vs `applicationName` (camelCase) inside the same `deploy-webhook/route.ts` module
|
||||
### M5 — `<RationaleCard>` takes 6 props that come from the same rec; pass the `rec` (or normalised `NormalisedRec`) instead
|
||||
|
||||
- **File**: `/Users/marc/Projects/HealthLog/src/app/api/internal/deploy-webhook/route.ts`
|
||||
- **Issue**: The wire payload from Coolify is snake_case
|
||||
(`application_name`, `application_uuid`, `deployment_uuid`),
|
||||
which is correct for the upstream contract. But the
|
||||
`NormalizedEvent` interface mixes both — `applicationName` (camel)
|
||||
and the audit-log `details` re-emit `application_name` (snake)
|
||||
alongside `applicationName`. The `annotate({meta:{...}})` block
|
||||
uses snake (`application_name`, `deploy_outcome`,
|
||||
`application_uuid`), inconsistent with the rest of the codebase
|
||||
which uses camelCase keys for Wide Event meta fields (e.g.
|
||||
`lastSyncedAt`, `consecutiveFailures` everywhere else).
|
||||
- **Recommendation**: Pick one — the codebase convention is
|
||||
camelCase for app-level keys, snake_case ONLY where the wire
|
||||
contract demands it. Convert the `meta:{}` block to
|
||||
`applicationName` / `deployOutcome` / `applicationUuid` /
|
||||
`deploymentUuid` and document the wire-vs-app boundary at the
|
||||
top of the file. One-line fix per key.
|
||||
- **File**: `src/components/insights/recommendation-card.tsx`
|
||||
lines 192-279, 396-402
|
||||
- **Issue**: The `<RationaleCard>` callsite in
|
||||
`<RecommendationCard>` reads:
|
||||
```tsx
|
||||
<RationaleCard
|
||||
rationale={norm.rationale}
|
||||
metricSource={norm.metricSource}
|
||||
referenceId={norm.referenceId}
|
||||
confidence={norm.confidence}
|
||||
locale={locale}
|
||||
feedbackProps={feedbackProps}
|
||||
/>
|
||||
```
|
||||
Five of those six props are de-structurings of `norm`. The prop
|
||||
list will only grow as future buckets plug in. Compare with
|
||||
`<RecommendationFeedback>` which already takes a rec-shaped props
|
||||
object.
|
||||
- **Recommendation**: `<RationaleCard rec={norm} feedbackProps={feedbackProps} locale={locale} />`.
|
||||
Keeps `feedbackProps` separate (it's not a rec field — it's a
|
||||
derived "is the rec submittable for feedback?" gate) and `locale`
|
||||
separate (it's a user-context read). The prop list compresses to
|
||||
the two values that aren't already in `norm`.
|
||||
|
||||
### M6 — `src/lib/ai/types.ts` exports legacy `insightResultSchema` shape AND v1.4.15 imports a separate `aiInsightResponseSchema` from `schema.ts` — two response schemas in two files
|
||||
### M6 — `i18n.insights.recommendation.*` namespace mixes 4 unrelated feature buckets at the same depth; B5e key naming `feedbackThanks` / `feedbackHelpful` / `feedbackAlreadyRated` is flat where rationale + confidence are also flat
|
||||
|
||||
- **Files**: `src/lib/ai/types.ts` (legacy `insightResultSchema`), `src/lib/ai/schema.ts` (new `aiInsightResponseSchema`)
|
||||
- **Issue**: `types.ts` defines the v1.4.14 shape (classification,
|
||||
findings, correlations, dataQuality, disclaimer) and is still
|
||||
imported by the dashboard renderer. `schema.ts` defines the
|
||||
v1.4.15 strict shape (citations, warnings, structured
|
||||
recommendations) and is consumed by `generate-insight.ts`. Two
|
||||
schemas live in two files for the same domain object. The C1
|
||||
status block acknowledges the route-side migration is deferred to
|
||||
v1.4.16 — but the architectural smell is "which file do I edit to
|
||||
add a field?" If you add to the strict schema only, the dashboard
|
||||
doesn't see it. If you add to the legacy schema only, the parser
|
||||
doesn't validate it.
|
||||
- **Recommendation**: v1.4.16 must finish the migration the C1
|
||||
status block already plans — single source of truth in
|
||||
`schema.ts`, dashboard renderer migrated to consume the new
|
||||
shape, `insightResultSchema` deleted. The C1 phase author knew
|
||||
this; flagging here so the v1.4.16 backlog doesn't lose it.
|
||||
- **File**: `messages/en.json` lines 701-722 (and `messages/de.json`
|
||||
mirror)
|
||||
- **Issue**: Under the single `insights.recommendation.*` namespace
|
||||
live four sub-buckets:
|
||||
- source / viewSource (B5a citation)
|
||||
- rationale / rationaleWindow / rationaleComparedTo /
|
||||
rationaleDeviation / rationaleExpand / rationaleCollapse (B5c)
|
||||
- confidence / confidenceAria / confidenceHigh / confidenceMedium
|
||||
/ confidenceLow / confidenceDraft (B5d)
|
||||
- feedbackHelpful / feedbackNotHelpful / feedbackThanks /
|
||||
feedbackConfirmed / feedbackAlreadyRated (B5e)
|
||||
- legacyPayloadCta (B5c)
|
||||
Five flat bullets sit alongside two nested-by-prefix groups. A
|
||||
future contributor will add `feedbackXyz` and not know whether
|
||||
to nest under `feedback: { … }` or stay flat. The codebase
|
||||
convention (verified against `messages/en.json` line 1558 admin
|
||||
scope: `hostMetrics: { title, load1, ... }` and 1569
|
||||
`aiQuality: { title, loading, ... }`) is **nested objects per
|
||||
feature bucket**.
|
||||
- **Recommendation**: Re-shape to:
|
||||
```json
|
||||
"recommendation": {
|
||||
"citation": { "source": "...", "viewSource": "..." },
|
||||
"rationale": { "window": "...", "comparedTo": "...", "deviation": "...",
|
||||
"expand": "...", "collapse": "..." },
|
||||
"confidence": { "aria": "...", "high": "...", "medium": "...",
|
||||
"low": "...", "draft": "..." },
|
||||
"feedback": { "helpful": "...", "notHelpful": "...", "thanks": "...",
|
||||
"confirmed": "...", "alreadyRated": "..." },
|
||||
"legacyPayloadCta": "..."
|
||||
}
|
||||
```
|
||||
Every consumer file gets a single-line `t()` rename;
|
||||
`messages/de.json` mirrors. The existing
|
||||
`sections-i18n-parity.test.ts` will catch any miss. The same
|
||||
shape already works for `admin.hostMetrics` / `admin.aiQuality` /
|
||||
`comparison.captionLastMonth` — apply consistently.
|
||||
|
||||
### M7 — `RecentAchievementsCard` and `<RecentAuditPreview>` and `<SystemStatusSummary>` all marked `"use client"` but only fetch read-only data — could be RSCs
|
||||
### M7 — `src/lib/ai/feedback-attribution.ts` reads the LATEST `insights.generate` audit row to attribute provider; this couples the feedback domain to the audit-log shape
|
||||
|
||||
- **Files**:
|
||||
- `src/components/gamification/recent-achievements-card.tsx`
|
||||
- `src/components/admin/recent-audit-preview.tsx`
|
||||
- `src/components/admin/system-status-summary.tsx`
|
||||
- **Issue**: All three are pure read-only TanStack Query consumers
|
||||
with no interactivity (no `useState`, no `onClick` handlers
|
||||
beyond a `<Link>`). They could be RSCs that fetch directly from
|
||||
Prisma + audit-log helpers, eliminating the client-side
|
||||
hydration cost AND a round-trip on initial load. The
|
||||
`useTranslations()` hook is a barrier today (it's client-only),
|
||||
but the project ALREADY has server-side i18n infrastructure for
|
||||
the doctor-report PDF; it just hasn't been wired into the
|
||||
RSC rendering path.
|
||||
- **Recommendation**: Document as v1.5 backlog. The investment is
|
||||
in moving `useTranslations()` to a hybrid pattern that supports
|
||||
RSCs; once that lands, these three components can be RSCs by
|
||||
flipping the `"use client"` directive and replacing `useQuery`
|
||||
with a direct Prisma call.
|
||||
- **File**: `src/lib/ai/feedback-attribution.ts` lines 70-86
|
||||
- **Issue**: The function fetches the user's most recent
|
||||
`auditLog` row with `action: "insights.generate"`, parses the
|
||||
`details` JSON, and pulls `chainProviderType` (or fallback
|
||||
`providerType`). The audit log is a structured-event export
|
||||
surface — using it as the de-facto "what provider did the user
|
||||
most recently use" is a coupling that won't survive an audit-log
|
||||
schema change. The right home for "current active provider per
|
||||
user" is either:
|
||||
- a memoised read off `User.aiProviderChain` + the in-process
|
||||
runner cache (`getLastWorkingProvider()`)
|
||||
- or a dedicated `User.lastUsedProviderType` column written by the
|
||||
runner
|
||||
- or a dedicated AppSetting / cache table
|
||||
- **Recommendation**: For v1.4.17, write the working provider into
|
||||
a per-user cache column at the END of `runWithFallback()` (the
|
||||
in-process cache already exists; the row is a 1-line schema
|
||||
add). The audit-log scan stays as a fallback for users whose
|
||||
cache hasn't been hydrated yet, but the primary read is from a
|
||||
dedicated column. As-is the feedback aggregator's slice is at the
|
||||
mercy of the audit-log retention policy.
|
||||
|
||||
### M8 — `src/lib/integrations/status.ts` (484 lines) reads `INTEGRATION_FAILURE_ALERT_THRESHOLD` env var lazily — a "tests can mutate it per case" justification, but the same module also imports a frozen `BACKOFF_SCHEDULE_MS` from notifications that is read once at module load
|
||||
### M8 — `src/lib/jobs/feedback-aggregator.ts`'s `buildFeedbackBuckets()` joins keys with empty separator (`[severity, metricSourceType, providerType, promptVersion].join("")`); two values "ab" + "cd" collide with one value "abcd"
|
||||
|
||||
- **Files**: `src/lib/integrations/status.ts`, `src/lib/notifications/retry-policy.ts`
|
||||
- **Issue**: Inconsistent treatment of configuration values across
|
||||
two parallel modules. Notifications freezes the schedule at
|
||||
module load; integrations re-reads env on every call. Both
|
||||
styles have merit (frozen is faster, lazy lets tests mutate). But
|
||||
having both styles in the same milestone makes the codebase feel
|
||||
like two different teams wrote them.
|
||||
- **Recommendation**: Pick one (either `getConfig()` everywhere or
|
||||
module-frozen everywhere) and document the choice in CLAUDE.md
|
||||
under "Important Patterns". Tests can use `vi.stubEnv` for the
|
||||
module-frozen case with `vi.resetModules()` between tests if
|
||||
mutation is needed.
|
||||
- **File**: `src/lib/jobs/feedback-aggregator.ts` line 75
|
||||
```ts
|
||||
const key = [
|
||||
row.recommendationSeverity,
|
||||
row.metricSourceType,
|
||||
row.providerType,
|
||||
row.promptVersion,
|
||||
].join("");
|
||||
```
|
||||
- **Issue**: Empty-string join means `["bp","weight","openai","4.16"]`
|
||||
and `["bpweight","openai","","4.16"]` both produce `"bpweightopenai4.16"`.
|
||||
Today the inputs are constrained enough (severity is enum,
|
||||
providerType is enum, promptVersion is `4.16.0` with dots) that
|
||||
collision is unlikely. But the schema change that adds a hyphen-
|
||||
containing severity tomorrow could silently merge buckets.
|
||||
- **Recommendation**: Use a delimiter that can't appear in any of
|
||||
the four values — convention in this codebase is `::` (see
|
||||
`findUncitedRecommendations` line 310 in `schema.ts`). One-char
|
||||
fix; eliminates a class of silent-collision bugs the test
|
||||
fixtures wouldn't catch.
|
||||
|
||||
### M9 — `<HostMetricsChart>`, `<AppLogPreviewSection>`, `<AiQualitySection>` follow a TanStack Query pattern with inline `(await res.json()).data` unwrap; the TanStack-collision memory note from v1.4.15 says the convention is the unwrap, but the new admin sections re-implement it three times instead of using a shared `fetchJsonOrThrow` helper
|
||||
|
||||
- **Files**: `src/components/admin/host-metrics-chart.tsx`,
|
||||
`src/components/admin/app-log-preview-section.tsx`,
|
||||
`src/components/admin/ai-quality-section.tsx`,
|
||||
`src/components/admin/api-token-overview-section.tsx`,
|
||||
`src/components/admin/backups-section.tsx`
|
||||
- **Issue**: Five admin sections (and many more outside admin) all
|
||||
hand-roll the same `fetch + res.ok + json.data ?? throw` mini
|
||||
state machine. The error message is different in each
|
||||
("ai_quality_failed" vs "Failed to load app logs" vs "Failed").
|
||||
No shared helper means error formatting drifts.
|
||||
- **Recommendation**: One small `src/lib/api-client.ts` helper:
|
||||
```ts
|
||||
export async function fetchJsonOrThrow<T>(input: RequestInfo, init?: RequestInit): Promise<T> {
|
||||
const res = await fetch(input, init);
|
||||
const body = (await res.json()) as { data: T; error?: string };
|
||||
if (!res.ok) throw new Error(body.error ?? `Request failed: ${res.status}`);
|
||||
return body.data;
|
||||
}
|
||||
```
|
||||
Adoption is opportunistic; not a structural requirement, but a
|
||||
natural cleanup pass. Already partially done: the existing
|
||||
`getApiErrorMessage` helper in `backups-section.tsx` is similar.
|
||||
Promote it.
|
||||
|
||||
---
|
||||
|
||||
## LOW
|
||||
|
||||
### L1 — `STATUS_TABS` / `FEEDBACK_STATUS_TABS` / `RANGE_DAYS` / `BACKOFF_SCHEDULE_MS` use four different "frozen tuple" idioms across the v1.4.15 changeset
|
||||
### L1 — `src/components/insights/insight-advisor-card.tsx` lines 284-286 has a duplicated `// ─── Section Separator ───` comment header
|
||||
|
||||
- **Files**: `medication-compliance-chart.tsx` (`as const` tuple),
|
||||
`retry-policy.ts` (`Object.freeze`),
|
||||
`dashboard-visibility` (TBD, no shared source today),
|
||||
feedback-inbox-section (uses `_shared` re-export).
|
||||
- **Recommendation**: Document the canonical pattern (suggested:
|
||||
`const X = [...] as const`). Low priority.
|
||||
- **File**: `src/components/insights/insight-advisor-card.tsx`
|
||||
- **Issue**: Two consecutive `// ─── Section Separator ───`
|
||||
comment headers, no code between them. Copy-paste residue from a
|
||||
marathon edit.
|
||||
- **Recommendation**: Delete the duplicate. One-line fix.
|
||||
|
||||
### L2 — Tour-launcher's "set state in render" comment block (lines 124-130) is paraphrasing what the code does, not WHY
|
||||
### L2 — `<RecommendationFeedback>`'s 17-line `if (!initialState && user && hydratedFor !== user.id)` block in render-time is a workaround for `react-hooks/set-state-in-effect` that future readers will trip over
|
||||
|
||||
- **File**: `src/components/onboarding/tour-launcher.tsx` lines 124-130
|
||||
- **Issue**: 8-line comment explains that React StrictMode dev
|
||||
mode invokes the lazy initializer twice and the second call
|
||||
sees the cleared key. This describes mechanics, not motivation.
|
||||
- **Recommendation**: Trim to "Lazy initializer reads
|
||||
sessionStorage once at mount; StrictMode double-invocation in
|
||||
dev is acceptable because the consequence is bounded
|
||||
(auto-launch deferred one render in dev only)." Minor edit.
|
||||
- **File**: `src/components/insights/recommendation-feedback.tsx`
|
||||
lines 137-154
|
||||
- **Issue**: The set-state-during-render dance is correct (matches
|
||||
the lazy-init pattern documented in CLAUDE.md), but the block has
|
||||
no comment explaining why it's not in `useEffect`. A code reviewer
|
||||
unfamiliar with the project rule would flag it as a bug.
|
||||
- **Recommendation**: One-line comment: `// CLAUDE.md rule: avoid setState in useEffect; recompute from props during render is the project pattern (matches tour-launcher.tsx).`
|
||||
|
||||
### L3 — `safeEncryptError` and `safeDecryptError` swallow errors with literal sentinel strings (`"<encrypt failed>"`, `"(error message unavailable)"`)
|
||||
### L3 — `src/lib/ai/provider-chain.ts` and `src/lib/ai/provider.ts` both reference a `ProviderChainResolved` type; the type is defined in `provider-runner.ts` (the most-imported file) but the comment at provider-chain.ts top says "Why a separate module rather than inlining"
|
||||
|
||||
- **File**: `src/lib/integrations/status.ts` lines 365-383
|
||||
- **Issue**: The sentinel strings are stored in the DB column
|
||||
alongside real ciphertext. A future operator who runs `SELECT
|
||||
last_error FROM integration_statuses WHERE last_error LIKE
|
||||
'<%>'` would have to know about this convention. A `null` write
|
||||
+ a Wide-Event warning would be simpler.
|
||||
- **Recommendation**: Consider writing `null` on encrypt failure
|
||||
(the warning + Wide-Event already capture the diagnostic) and
|
||||
letting the read path treat `null` as "no error captured".
|
||||
Cleaner DB shape.
|
||||
- **File**: `src/lib/ai/provider-chain.ts` lines 12-19
|
||||
- **Issue**: `ProviderChainResolved` is the bridge type
|
||||
(logical-type + instance) that both `provider-chain.ts` (parser)
|
||||
and `provider-runner.ts` (executor) need. It's defined in
|
||||
`provider-runner.ts` and re-imported by `provider.ts` — backwards
|
||||
layering. The parser layer should own the type; the runner
|
||||
consumes it. Today's layout means `provider-chain.ts` doesn't
|
||||
know about it (it stops at metadata) and `provider.ts` imports
|
||||
from `provider-runner.ts` for a type the runner doesn't author.
|
||||
- **Recommendation**: When H1's directory restructure happens, move
|
||||
`ProviderChainResolved` into `runtime/provider-chain.ts` (or a new
|
||||
`runtime/types.ts`), so the parser can name what the runner
|
||||
consumes. Trivial; do as part of the H1 split.
|
||||
|
||||
### L4 — `RANGE_DAYS = [7, 30, 90] as const` in `medication-compliance-chart.tsx` duplicates the same constant from `health-chart.tsx`
|
||||
### L4 — `src/lib/ai/legacy-payload.ts` (55 lines) is a one-function module that could live in `schema.ts` next to `findRecommendationsMissingRationale()`
|
||||
|
||||
- **Files**: `medication-compliance-chart.tsx`, `health-chart.tsx`
|
||||
- **Issue**: Two charts on the same dashboard maintain their own
|
||||
copies of the range-button day list. A future "add 180-day
|
||||
range" change requires editing two places.
|
||||
- **Recommendation**: Extract to `src/lib/charts/ranges.ts` —
|
||||
trivial.
|
||||
- **File**: `src/lib/ai/legacy-payload.ts`
|
||||
- **Issue**: `isLegacyInsightPayload(payload: unknown): boolean` is
|
||||
the only export. `schema.ts` already exports
|
||||
`findRecommendationsMissingRationale(parsed: AIInsightResponse)`
|
||||
for nearly the same legacy-detection use case. Two helpers, two
|
||||
modules, same domain.
|
||||
- **Recommendation**: Inline into `schema.ts` (or a new
|
||||
`schema/legacy-detection.ts` after H1's split). Watch for the
|
||||
type difference: `isLegacyInsightPayload` takes `unknown`,
|
||||
`findRecommendationsMissingRationale` takes `AIInsightResponse` —
|
||||
they cover different stages of the parse pipeline. Document the
|
||||
staging in the merge.
|
||||
|
||||
### L5 — `MOODLOG_LAST_SYNCED_AT` legacy timestamp lives on the `User` model alongside `IntegrationStatus.lastSuccessAt`
|
||||
### L5 — `<RecommendationCard>`'s `LOW_CONFIDENCE_CAPTION_THRESHOLD = 50` and `<ConfidenceMeter>`'s `DRAFT_THRESHOLD = 25` + 80 / 50 / 25 band cuts live in two separate files with no shared source
|
||||
|
||||
- **Files**: `prisma/schema.prisma` (User.moodLogLastSyncedAt),
|
||||
`src/lib/integrations/status.ts` (IntegrationStatus.lastSuccessAt)
|
||||
- **Issue**: Two columns track the same fact for moodLog. The
|
||||
v1.4.15 phase B2 deliberately added the new shape without
|
||||
retiring the old (smart for compat), but a comment hint would
|
||||
help future devs avoid double-writing.
|
||||
- **Recommendation**: Add `// LEGACY: see IntegrationStatus.lastSuccessAt` comment on the schema field and document a v1.4.16 cutover task.
|
||||
- **Files**: `src/components/insights/recommendation-card.tsx`
|
||||
line 102, `src/components/insights/confidence-meter.tsx` line 37
|
||||
- **Issue**: The confidence thresholds are split:
|
||||
- `<ConfidenceMeter>`: 80 (high), 50 (medium), 25 (low/draft)
|
||||
- `<RecommendationCard>`: 50 (low-confidence-caption)
|
||||
- `src/lib/ai/confidence.ts`: no constants exposed (formula-only)
|
||||
Three files, three sets of magic numbers. A future "tighten the
|
||||
band cutoffs" change requires editing three places.
|
||||
- **Recommendation**: Add a `src/lib/ai/confidence-bands.ts`
|
||||
exposing:
|
||||
```ts
|
||||
export const CONFIDENCE_BANDS = {
|
||||
DRAFT: 25,
|
||||
LOW: 50,
|
||||
MEDIUM: 80,
|
||||
} as const;
|
||||
```
|
||||
Both components import. The threshold semantics become
|
||||
self-documenting (`< CONFIDENCE_BANDS.LOW` reads better than
|
||||
`< 50`).
|
||||
|
||||
### L6 — `src/lib/ai/codex-client.ts` exports a `__test` namespace at the bottom (lines 479-483) for unit-test introspection
|
||||
### L6 — `src/lib/jobs/host-metric-sampler.ts`'s `getHostMetricRetentionDays()` reads env on every call; no caching, no obvious test reason
|
||||
|
||||
- **File**: `src/lib/ai/codex-client.ts` lines 477-483
|
||||
- **Issue**: Exporting a `__test` namespace is an anti-pattern that
|
||||
the codebase doesn't use elsewhere. Tests can import the
|
||||
individual functions directly (the named exports are already in
|
||||
scope inside the module). The `__test` re-export is a vestige
|
||||
from a TS-strict workaround, not a structural requirement.
|
||||
- **Recommendation**: Mark `loadFallbackChain`,
|
||||
`DEFAULT_SLUG_FALLBACK_CHAIN`, `isSlugRejection` as named exports
|
||||
if tests need them, and delete the `__test` namespace. Slight
|
||||
cleanup; low priority.
|
||||
- **File**: `src/lib/jobs/host-metric-sampler.ts` lines 120-131
|
||||
- **Issue**: Every minute the worker fires `runHostMetricTick()`
|
||||
which calls `getHostMetricRetentionDays()` which calls
|
||||
`process.env.HOST_METRIC_RETENTION_DAYS`. Cheap call, but the
|
||||
pattern is inconsistent with the rest of the codebase. v1.4.15
|
||||
M8 already flagged the parallel "lazy env vs frozen env" split
|
||||
in integrations vs notifications; this adds a third site.
|
||||
- **Recommendation**: Defer to the M8/L1 v1.4.17 cleanup that picks
|
||||
one canonical pattern.
|
||||
|
||||
### L7 — `recordChannelTransientFailure` and `recordChannelHardReject` and `recordChannelSuccess` in `channel-state.ts` use camelCase parameter `channel: ChannelRef`, but `recordSyncFailure` in `status.ts` takes a positional flat-args `RecordSyncFailureInput` object
|
||||
### L7 — `prisma/schema.prisma` the new `RecommendationFeedback` model uses `recommendationSeverity` / `recommendationText` (camel) but `metric_source_type` / `metric_source_time_range` (snake-via-@map); the JS-side names are camelCase but the db column names mix prefixed-with-feature and not
|
||||
|
||||
- **Files**: `src/lib/integrations/status.ts`, `src/lib/notifications/channel-state.ts`
|
||||
- **Issue**: Inconsistent function-signature style for very similar
|
||||
state-machine writers in the same milestone. Channel-state uses
|
||||
`(channelRef, outcome, now)`; integration-status uses
|
||||
`({userId, integration, kind, message, errorCode})`.
|
||||
- **Recommendation**: Pick one (suggested: object-arg-bag everywhere,
|
||||
matches the rest of the codebase). Low priority — both signatures
|
||||
read fine in isolation; the inconsistency only bites when reading
|
||||
both modules side-by-side.
|
||||
- **Files**: `prisma/schema.prisma` lines 833-840
|
||||
- **Issue**: The model uses `recommendationSeverity` (prefixed with
|
||||
feature noun) and `metricSourceType` (prefixed with feature noun
|
||||
too — but on-the-wire and in the table). This is consistent with
|
||||
the rest of the schema. But the v1.4.16 audit-log query uses
|
||||
`actor` / `action` / `target` as filter keys; the
|
||||
recommendation-feedback row uses `recommendationId` /
|
||||
`recommendationText` / `recommendationSeverity` — the
|
||||
feature-prefix scheme is verbose where context already pins the
|
||||
feature. The same row could read cleaner as `id`, `text`,
|
||||
`severity` (the model name already says it's recommendation
|
||||
feedback).
|
||||
- **Recommendation**: Cosmetic only — the prefixed naming is
|
||||
defensible (the model is also the join target so prefixed names
|
||||
read clearly under join queries). Flag for the v1.4.17 settings
|
||||
audit (B6 follow-up) only if Marc has a feel-preference.
|
||||
|
||||
### L8 — `pickRecentUnlocks(achievements, limit)` in `recent-achievements-card.tsx` is exported "for unit testing" — this is a code smell
|
||||
### L8 — `src/lib/ai/medical-references.ts` and `<CitationFootnote>` both render the citation label as `{org} {publishedYear} — {title}` — the format is duplicated rather than centralised in a `formatCitationLabel(ref, locale)` helper
|
||||
|
||||
- **File**: `src/components/gamification/recent-achievements-card.tsx` lines 56-75
|
||||
- **Issue**: A pure helper is exported from a `"use client"`
|
||||
component file solely so a unit test can import it. The pattern
|
||||
works, but logically the helper belongs in
|
||||
`src/lib/gamification/recent-unlocks.ts`. Pure data helpers
|
||||
shouldn't live behind a `"use client"` boundary.
|
||||
- **Recommendation**: Move to `src/lib/gamification/`, import
|
||||
back into the component. Trivial. Watch for the same pattern in
|
||||
`medication-compliance-chart.tsx` (`aggregateMedicationCompliance`
|
||||
is also exported from a `"use client"` file).
|
||||
- **Files**: `src/lib/ai/medical-references.ts`,
|
||||
`src/components/insights/recommendation-card.tsx` line 173
|
||||
- **Issue**: One UI surface today; the v1.4.17 "share rec" or
|
||||
the doctor-report PDF would re-render the same citation in the
|
||||
same shape. No helper means a typography change requires editing
|
||||
two places.
|
||||
- **Recommendation**: Add `formatCitationLabel(ref, locale)` to
|
||||
`medical-references.ts`. Today's only consumer is
|
||||
`<CitationFootnote>`; flagged here so the second consumer
|
||||
copy-pastes from the helper rather than the component.
|
||||
|
||||
### L9 — `src/lib/jobs/reminder-worker.ts` (1,350 lines) is the v1.4.16 +70-line addition (host-metric + feedback-aggregator queue registrations) on top of an already-too-large worker module
|
||||
|
||||
- **File**: `src/lib/jobs/reminder-worker.ts`
|
||||
- **Issue**: 1,350 lines hosting reminder dispatch, insight caching,
|
||||
data-backup, host-metric sampling, feedback aggregation, queue
|
||||
registration, and the pg-boss instance. The file is the single
|
||||
entry point for every cron-style background job — no per-queue
|
||||
module split. Each new queue is +70 lines at the bottom of a
|
||||
huge file.
|
||||
- **Recommendation**: v1.4.17 — split per queue under
|
||||
`src/lib/jobs/queues/<queue-name>.ts`, each exporting a
|
||||
`register(boss: PgBoss)` function. `reminder-worker.ts` becomes
|
||||
a 50-line orchestrator that calls each `register()`. The host-
|
||||
metric and feedback-aggregator additions in v1.4.16 already have
|
||||
pure compute helpers split out (`host-metric-sampler.ts` /
|
||||
`feedback-aggregator.ts`); the missing piece is the queue-config
|
||||
+ handler-wiring split. Mirror what v1.4.15 did for notifications
|
||||
(`src/lib/notifications/dispatcher.ts` orchestrates;
|
||||
per-channel senders in `src/lib/notifications/senders/`).
|
||||
|
||||
---
|
||||
|
||||
## Cross-cutting observations (positive)
|
||||
|
||||
- Every API route in v1.4.15 uses `apiHandler()` — checked: 22/22
|
||||
new or modified routes wrap correctly.
|
||||
- Every admin route uses `requireAdmin()` (cookie-only) — verified.
|
||||
- Every mutation route in B1/B2/B3 uses `auditLog()` for both
|
||||
success AND denial paths.
|
||||
- `withIdempotency()` is correctly applied to the destructive
|
||||
restore endpoint.
|
||||
- AES-256-GCM `encrypt()` / `decrypt()` is consistently used for
|
||||
every new sensitive value (last-error, channel config, backup
|
||||
payload).
|
||||
- `dispatchNotification(SYSTEM_ALERT)` fan-out pattern is reused
|
||||
across B2 (integration alerts) AND C2 (deploy failures) — a
|
||||
single sender abstraction, not duplicated code.
|
||||
- The `data-tour-id` contract is set up properly: every target id
|
||||
declared by `buildTourStops()` has a matching `data-tour-id` in
|
||||
the rendered DOM (verified: `dashboard-tile-strip`,
|
||||
`dashboard-quick-add`, `nav-insights`, `nav-settings`,
|
||||
`nav-achievements` all wired).
|
||||
- Phase C1's pure state-machine extraction (`tour-state.ts`)
|
||||
separated from UI (`tour.tsx`) is exactly the pattern future
|
||||
phases should mirror.
|
||||
- Zero `console.*` calls in any v1.4.15-touched lib file.
|
||||
- Zero raw `throw` of generic errors from API routes — every
|
||||
failure goes through `HttpError` or `apiError()`.
|
||||
- **Worktree adoption worked**: Last milestone's H3 (parallel-agent
|
||||
staging race) was the recurring complaint; v1.4.16 STATE.md
|
||||
records `agent/b1b-insights-surface`,
|
||||
`agent/b2-ai-provider-ux`, `agent/b5c-explainability`,
|
||||
`agent/b5d-confidence`, `agent/b5e-feedback-loop`,
|
||||
`agent/b6-settings-audit`, `agent/b8-comparison-views`,
|
||||
`agent/wave-c-catchup` — all worktree-isolated. Cross-agent
|
||||
collisions still happen (`9b01c86` carries both A6 and A7
|
||||
files; `8d9f864` absorbed B3 chart + i18n; `2611bb4` got
|
||||
hijacked by B1a's mood-chart) but the working tree on `main`
|
||||
is correct in every case. The "wrong commit subject" outcomes
|
||||
are diff-faithful, just message-misleading.
|
||||
- **Named-slot pattern is a clear win**: `<RecommendationCard>`'s
|
||||
`data-slot="rec-confidence-slot"` + `data-slot="rec-feedback-slot"`
|
||||
let B5c, B5d, B5e ship in parallel without merge conflicts.
|
||||
The pattern should be the v1.4.17 default for any component
|
||||
that anticipates plug-in pressure.
|
||||
- **Pure-function extraction is consistent**: B5d's `confidence.ts`
|
||||
exposes `computeConfidence(inputs)` as a pure function with
|
||||
unit tests; B8's `comparison-shift.ts` exposes
|
||||
`shiftDailySeriesForward()` + `computeComparisonDelta()` the
|
||||
same way; B5e's `feedback-aggregator.ts` exposes
|
||||
`buildFeedbackBuckets()` separately. This is the right pattern
|
||||
and the marathon used it consistently.
|
||||
- **Test-architecture is correct**: `tests/integration/`
|
||||
testcontainers for the new `admin-host-metrics`,
|
||||
`insights-feedback`, `insights-provider-chain` /
|
||||
`insights-provider-chain-put`, `cascade-delete`,
|
||||
`export-per-type`. SSR component tests under `__tests__/`
|
||||
alongside the source files. Pure-function helpers tested in
|
||||
isolation. No mocks-test-themselves smells observed.
|
||||
- **`apiHandler()` discipline holds**: every new API route
|
||||
(`/api/insights/feedback`, `/api/insights/provider-chain`,
|
||||
`/api/admin/host-metrics`, `/api/admin/audit-log`,
|
||||
`/api/admin/audit-log/actions`, `/api/admin/app-logs`,
|
||||
`/api/admin/ai-quality`, `/api/export/measurements`,
|
||||
`/api/export/medications`, `/api/export/mood`,
|
||||
`/api/export/full-backup`) wraps in `apiHandler` with
|
||||
`requireAuth()` or `requireAdmin()`.
|
||||
- **`annotate()` adoption is universal**: 42 `annotate()` call
|
||||
sites in v1.4.16-touched non-test files; zero new `console.*`
|
||||
calls (the only `console.*` greps in `reminder-worker.ts`
|
||||
predate v1.4.16, and they're explicitly the pg-boss-error
|
||||
fallback the worker has used since v1.3).
|
||||
- **`HttpError` discipline holds**: every new API-route throw
|
||||
that needs to escape `apiHandler` uses `HttpError` (verified:
|
||||
`provider-chain/route.ts:87,98`, `backups/[id]/restore/route.ts:126`).
|
||||
The other `throw new Error()` matches in the grep all live
|
||||
inside TanStack Query `mutationFn` / `queryFn` callbacks where
|
||||
the throw is internal to the React Query state machine.
|
||||
- **Encryption discipline holds**: every new sensitive value goes
|
||||
through `decrypt()` from `crypto.ts` (verified at
|
||||
`provider.ts:59,67,84` for AI keys; nothing new in v1.4.16
|
||||
reached for raw cipher).
|
||||
- **TODO/FIXME inventory**: zero. `git grep -nE "(TODO|FIXME|HACK|XXX)"`
|
||||
filtered to v1.4.16-changed files in `src/` returns no matches.
|
||||
- **Comment quality**: comments paraphrase code in only one
|
||||
observed location (L2 above); the marathon's "WHY not WHAT"
|
||||
discipline shows.
|
||||
|
||||
The structural foundation is solid; the findings above are mostly
|
||||
"this could be cleaner", not "this is wrong".
|
||||
The structural foundation continues to hold; the v1.4.16 findings
|
||||
above are mostly "this could be cleaner before the next bucket
|
||||
plugs in", not "this is wrong".
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
- 0 CRITICAL — nothing blocks v1.4.15 ship.
|
||||
- 4 HIGH — file-size discipline (page.tsx, integrations-section.tsx),
|
||||
process discipline (worktree adoption), and one test-fixture drift
|
||||
(MockAIProvider default).
|
||||
- 8 MED/LOW — naming consistency, helper extraction opportunities,
|
||||
RSC migration paths, dead-code identification (legacy Telegram
|
||||
migration).
|
||||
- **0 CRITICAL** — nothing blocks v1.4.16 ship.
|
||||
- **3 HIGH** — directory layout pressure on `src/lib/ai/` (H1),
|
||||
god-component drift in `<InsightAdvisorCard>` (H2), pre-emptive
|
||||
split for `<RecommendationCard>` before the next plug-in (H3).
|
||||
- **9 MED/LOW** — premature-or-bridge duplication in the chain
|
||||
runner (M1), auth coupling in feedback leaf (M2), dual schema
|
||||
STILL not migrated from v1.4.15 (M3), shared metric-vocab
|
||||
duplication (M4), `<RationaleCard>` prop-list growth (M5), i18n
|
||||
namespace flattening drift (M6), audit-log coupling for feedback
|
||||
attribution (M7), bucket-key collision risk (M8), shared
|
||||
`fetchJsonOrThrow` opportunity (M9), plus seven LOW housekeeping
|
||||
items.
|
||||
|
||||
Recommended action: **ship v1.4.15**. File the four HIGH items in
|
||||
the v1.4.16 backlog. The MED/LOW items are housekeeping for
|
||||
v1.4.16/v1.4.17.
|
||||
Recommended action: **ship v1.4.16**. File the three HIGH items
|
||||
+ M3 (the carry-over from v1.4.15 M6) in the v1.4.17 backlog as
|
||||
the AI-module restructure block. The MED/LOW items are
|
||||
opportunistic; pick them up when adjacent code is touched.
|
||||
@@ -1,367 +1,473 @@
|
||||
# Phase D — Simplify/Refine review findings (v1.4.15)
|
||||
# Phase D — Simplify/Refine review findings (v1.4.16)
|
||||
|
||||
Reviewer: simplify agent (parallel — 1 of 5)
|
||||
Scope: `git diff --name-only v1.4.14...HEAD` — v1.4.15 changed source files only.
|
||||
Constraint: chart presentation (`src/components/charts/**`) explicitly approved this milestone — not flagged.
|
||||
Reviewer: simplify agent (parallel — phase-D wave, 1 of 6)
|
||||
Scope: `git diff --name-only v1.4.15...HEAD` — v1.4.16 changed source files only.
|
||||
Constraint: chart presentation polish (B1a/B1b) was the explicit milestone goal — visual choices are not flagged.
|
||||
Mode: autonomous, no commits, no edits — output only.
|
||||
|
||||
Findings ordered roughly by impact. "Apply autonomously? yes" = low-risk delete / inline; "no" = needs Marc's call.
|
||||
|
||||
---
|
||||
|
||||
## Finding 1 — Schema-enforcement wrapper (`generateInsight`) is wired into tests only
|
||||
## Finding 1 — `runWithFallback` (strict variant) + `RunWithFallbackParams` / `RunWithFallbackResult` are test-only
|
||||
|
||||
**Files:**
|
||||
|
||||
- `src/lib/ai/generate-insight.ts` (entire file, 176 LOC)
|
||||
- `src/lib/ai/schema.ts` (entire file, 182 LOC — 4 zod schemas + `findUncitedRecommendations` + `InsightSchemaError`)
|
||||
- `src/lib/ai/prompts/insight-generator.ts` (entire file, 267 LOC — `getStrictInsightsSystemPrompt`, `OUT_OF_SCOPE_REFUSAL_*`, `PROMPT_VERSION`)
|
||||
- `src/lib/ai/mock-client.ts` (entire file, 103 LOC — `MockAIProvider`)
|
||||
|
||||
**Type:** dead / speculative
|
||||
|
||||
**Why it's a smell:** Every export from these four files is consumed only by the v1.4.15 test suite. Production routes (`src/app/api/insights/**`) do not call `generateInsight()`, do not use `getStrictInsightsSystemPrompt()`, and do not import `aiInsightResponseSchema`. The schema-enforced retry-once + citation cross-check + zero-hallucination prompt is pure scaffolding for v1.4.16 (per the prompt-file's own commentary: "Use this in place of the legacy `getInsightsSystemPrompt` once the route migrates to `generateInsight()` (planned v1.4.16)").
|
||||
|
||||
CLAUDE.md: "Don't add features, refactor, or introduce abstractions beyond what the task requires" + "Don't add error handling, fallbacks, or validation for scenarios that can't happen" — the four files together add ~728 LOC of validation + retry + prompt-versioning that no production code path can reach.
|
||||
|
||||
Verified:
|
||||
|
||||
```
|
||||
$ grep -rn "generateInsight\|getStrictInsightsSystemPrompt\|aiInsightResponseSchema\|InsightSchemaError\|MockAIProvider" src/app
|
||||
(no output)
|
||||
```
|
||||
|
||||
**Suggested change:** Defer the four files (and their tests) to v1.4.16, where the route actually migrates. Either (a) revert the four `.ts` and their `__tests__/` siblings, or (b) leave them in place with a single TODO comment and acknowledge as carry-over scaffolding. Option (a) is the CLAUDE.md-aligned move.
|
||||
|
||||
**Risk:** medium — deletes six test files that "pass". But the production code never depends on any of it, so removal is mechanical.
|
||||
|
||||
**Apply autonomously?** no — Marc may have specifically wanted the test-only foundation pre-built before v1.4.16 work; checking with him saves a re-do if so.
|
||||
|
||||
---
|
||||
|
||||
## Finding 2 — `CodexClient.getLastDiagnostics()` + `CodexAttemptDiagnostics` interface are read only by tests
|
||||
|
||||
**File:** `src/lib/ai/codex-client.ts:101-138, 174-178, 192-198, 233-237, 242-246`
|
||||
**File:** `src/lib/ai/provider-runner.ts:136-288`
|
||||
|
||||
**Type:** speculative / dead
|
||||
|
||||
**Why it's a smell:** The diagnostics struct (`attempted[]`, `cacheState`, `workingSlug`) and `getLastDiagnostics()` getter are stored on the instance after every `generateCompletion()` call. The doc-comment says "used by the route layer for Wide-Event annotations" — but the route layer in `src/app/api/insights/**` never calls it. Only the slug-fallback unit tests read it.
|
||||
**Why it's a smell:** v1.4.16 ships TWO variants of the chain runner: the legacy `runRawCompletionWithFallback` (raw provider call, no schema enforcement) and the strict `runWithFallback` (wraps `generateInsight()` for parsed schema-enforcement). Production is wired to ONE: `/api/insights/generate/route.ts:280` calls only `runRawCompletionWithFallback`. The strict `runWithFallback`, plus its `RunWithFallbackParams` and `RunWithFallbackResult` interfaces, are imported solely by `src/lib/ai/__tests__/provider-runner.test.ts`. The file's own header comment even documents the migration as "v1.4.17 will migrate this route to the strict wrapper (B5c work) — at that point this helper is removed."
|
||||
|
||||
This is two-implementations-of-one-interface in disguise: 60 LOC of strict-wrapper plumbing + 27 LOC of test-only types pre-built for a v1.4.17 migration that hasn't happened. CLAUDE.md: "Don't add features, refactor, or introduce abstractions beyond what the task requires."
|
||||
|
||||
Verified:
|
||||
|
||||
```
|
||||
$ grep -rn "getLastDiagnostics\|CodexAttemptDiagnostics" src/app
|
||||
$ grep -rn "runWithFallback" src/app
|
||||
(no output — only runRawCompletionWithFallback is wired)
|
||||
```
|
||||
|
||||
The transitive cost is real: `generateInsight()` is itself only reachable via the strict variant, so `GenerateInsightOutcome`, the corrective-retry retry logic in `tryOnce`, `applyConfidenceOverride`, `annotateCitationCoverage`, and `InsightSchemaError` are all reachable only from tests in v1.4.16.
|
||||
|
||||
**Suggested change:** Pick one of (a) or (b):
|
||||
|
||||
- **(a)** Delete `runWithFallback`, `RunWithFallbackParams`, `RunWithFallbackResult` from `provider-runner.ts`. Migrate the test suite to exercise `runRawCompletionWithFallback` for fallback semantics + the standalone `generateInsight()` for schema-enforcement semantics. Keeps `generateInsight()` available for the v1.4.17 migration but removes the speculative coupling.
|
||||
- **(b)** Wire `/api/insights/generate/route.ts` to the strict variant NOW (the route already gets a `ProviderChainResolved[]`; the swap is one import + one call site). Then delete `runRawCompletionWithFallback` + its result interface. This is the eventual migration the comment promises — doing it now collapses the duplication.
|
||||
|
||||
Path-of-least-resistance is (a); (b) is the more honest answer to the duplication.
|
||||
|
||||
**Risk:** medium — both options touch live AI code paths; (b) is bigger but resolves the duplication permanently.
|
||||
|
||||
**Apply autonomously?** no — needs Marc's call on whether v1.4.16 is the right milestone for the migration (b) or just to defer (a).
|
||||
|
||||
---
|
||||
|
||||
## Finding 2 — `GenerateInsightOptions.confidenceContext` is a config-bag with one shape
|
||||
|
||||
**File:** `src/lib/ai/generate-insight.ts:197-275`
|
||||
|
||||
**Type:** speculative / premature abstraction
|
||||
|
||||
**Why it's a smell:** `generateInsight()` accepts an `options: GenerateInsightOptions` object whose only field is `confidenceContext?: ConfidenceContextResolver`. No production caller passes it; `provider-runner.ts:244` calls `generateInsight(candidate.instance, params)` with no third argument. Inside the function, the resolver always falls back to `DEFAULT_CONFIDENCE_RESOLVER`, which uses `metricSource.n` + `recencyDays=0` + `deviationStdRatio=null` — i.e. the formula has only ONE shape ever in production.
|
||||
|
||||
The doc-comment promises that "the route layer can supply a real value via the resolver", but the route layer doesn't. The whole `ConfidenceContextResolver` type, the `GenerateInsightOptions` interface, the `options.confidenceContext ?? DEFAULT_CONFIDENCE_RESOLVER` indirection, and the per-rec callback are scaffolding for a v1.4.17 enhancement (the file says "v1.4.17 will fit weights to thumbs-up rates"). CLAUDE.md: "Don't add config bags with one shape" / "Don't add features beyond what the task requires."
|
||||
|
||||
Verified:
|
||||
|
||||
```
|
||||
$ grep -rn "confidenceContext" src/app
|
||||
(no output)
|
||||
```
|
||||
|
||||
**Suggested change:** Either wire it into the route's `annotate()` block (closes the comment's promise) or delete `lastDiagnostics`, `getLastDiagnostics()`, and the `CodexAttemptDiagnostics` interface. The tests can be re-written to assert against `getCachedCodexSlug()` + `inspectCodexSlugCache()` which already cover the same observable surface.
|
||||
|
||||
**Risk:** low — telemetry-only state.
|
||||
|
||||
**Apply autonomously?** no — Marc may want the diagnostics in Wide Events; either direction is a small commit but it's a product-y call.
|
||||
|
||||
---
|
||||
|
||||
## Finding 3 — `inspectCodexSlugCache()` + `CODEX_SLUG_CACHE_TTL_MS` are test-only re-exports
|
||||
|
||||
**File:** `src/lib/ai/codex-slug-cache.ts:62-74`
|
||||
|
||||
**Type:** dead
|
||||
|
||||
**Why it's a smell:** Both exports are consumed only by `__tests__/codex-slug-fallback.test.ts`. The doc-comment for `inspectCodexSlugCache` says "Diagnostic helper — exposes (slug, ageMs) for Wide-Event annotation" but nothing in the route layer reads it. `CODEX_SLUG_CACHE_TTL_MS` exists only so a test can assert "1h per spec §7b" — that assertion can read the internal `CACHE_TTL_MS` directly via `__test` re-export pattern (already used by `codex-client.ts`).
|
||||
|
||||
**Suggested change:** Either (a) inline both into `codex-client.ts` `__test` block, or (b) delete `inspectCodexSlugCache` and have the test assert via `getCachedCodexSlug()` (semantically equivalent — it returns the same slug). The TTL constant can move into the test as a literal `60 * 60 * 1000` since it's only checking the documented spec value, not a runtime branch.
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes (option b — delete the diagnostic helper, leave the TTL constant since it's at file scope and one line).
|
||||
|
||||
---
|
||||
|
||||
## Finding 4 — `TEST_CONSTANTS` re-export in `channel-state.ts` is unused
|
||||
|
||||
**File:** `src/lib/notifications/channel-state.ts:171`
|
||||
**Suggested change:** Inline the default-resolver body into `applyConfidenceOverride`, drop the `options` parameter, drop the `GenerateInsightOptions` interface, drop the `ConfidenceContextResolver` type and the `DEFAULT_CONFIDENCE_RESOLVER` constant. The function becomes:
|
||||
|
||||
```ts
|
||||
export const TEST_CONSTANTS = { MAX_CONSECUTIVE_FAILURES } as const;
|
||||
```
|
||||
export async function generateInsight(
|
||||
provider: AIProvider,
|
||||
params: CompletionParams,
|
||||
): Promise<GenerateInsightOutcome> { … }
|
||||
|
||||
**Type:** dead
|
||||
|
||||
**Why it's a smell:** `MAX_CONSECUTIVE_FAILURES` is already exported from `retry-policy.ts` (the actual definition site). `TEST_CONSTANTS` is referenced by no test, no production code, no other module. Pure dead code.
|
||||
|
||||
```
|
||||
$ grep -rn "TEST_CONSTANTS" src
|
||||
src/lib/notifications/channel-state.ts:171:export const TEST_CONSTANTS = { MAX_CONSECUTIVE_FAILURES } as const;
|
||||
```
|
||||
|
||||
**Suggested change:** Delete the line.
|
||||
|
||||
**Risk:** low — single-line removal.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 5 — `RejectKind` type alias is exported but never used
|
||||
|
||||
**File:** `src/lib/notifications/retry-policy.ts:28`
|
||||
|
||||
```ts
|
||||
export type RejectKind = "hard" | "soft" | "ok";
|
||||
```
|
||||
|
||||
**Type:** dead
|
||||
|
||||
**Why it's a smell:** Defined alongside `SendOutcome` but every consumer uses `SendOutcome.hardReject?: boolean` instead of branching on a `RejectKind` value. No code reads the alias; `Pick<SendOutcome, …>` is the actual currency.
|
||||
|
||||
```
|
||||
$ grep -rn "RejectKind" src
|
||||
src/lib/notifications/retry-policy.ts:28:export type RejectKind = "hard" | "soft" | "ok";
|
||||
```
|
||||
|
||||
**Suggested change:** Delete the line.
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 6 — Misleading "fallback" comment on exhaustive switch
|
||||
|
||||
**File:** `src/lib/gamification/achievements.ts:44-48` (function `getAchievementCategory`)
|
||||
|
||||
```ts
|
||||
/**
|
||||
* Stable metric → category mapping. Add a metric here when it joins the
|
||||
* `AchievementMetricKey` union; the page falls back to "engagement" if
|
||||
* a future definition slips through, so there is no silent drop-off.
|
||||
*/
|
||||
export function getAchievementCategory(
|
||||
metric: AchievementMetricKey,
|
||||
): AchievementCategory {
|
||||
switch (metric) { ... } // exhaustive over the union; no default branch
|
||||
function applyConfidenceOverride(parsed: AIInsightResponse): void {
|
||||
const overrides: Array<{ id: string; model: number | null; computed: number }> = [];
|
||||
for (const rec of parsed.recommendations) {
|
||||
const computed = computeConfidence({
|
||||
n: rec.metricSource.n ?? 0,
|
||||
recencyDays: 0,
|
||||
deviationStdRatio: null,
|
||||
});
|
||||
const modelValue = typeof rec.confidence === "number" ? rec.confidence : null;
|
||||
rec.confidence = computed;
|
||||
overrides.push({ id: rec.id, model: modelValue, computed });
|
||||
}
|
||||
…
|
||||
}
|
||||
```
|
||||
|
||||
**Type:** comment (WHAT not WHY, factually wrong)
|
||||
When v1.4.17 actually needs the resolver injection, re-introduce it then with a real second caller — that's the time the abstraction earns its keep.
|
||||
|
||||
**Why it's a smell:** The comment claims "the page falls back to 'engagement' if a future definition slips through" — but the implementation is an exhaustive switch over a closed union with no `default:` branch. If a metric is ever added to `AchievementMetricKey` without a case here, TypeScript will error at compile time, not silently fall through to "engagement". The comment describes a behaviour that doesn't exist.
|
||||
**Risk:** low — pure inlining. Tests that pass `options` need updating, but they're internal.
|
||||
|
||||
CLAUDE.md: "Default to writing no comments. Don't explain WHAT — well-named identifiers do that."
|
||||
**Apply autonomously?** yes (with the caveat that one test will need its `confidenceContext: ...` passthrough removed; the test's existence is fine, just the `options` parameter goes).
|
||||
|
||||
**Suggested change:** Delete the misleading sentence. If the WHY is "stable mapping that TS guarantees exhaustiveness", trust the type system.
|
||||
---
|
||||
|
||||
## Finding 3 — `aiRecommendationRationaleSchema.referenceId` is a speculative optional field with no consumer
|
||||
|
||||
**File:** `src/lib/ai/schema.ts:119-126`
|
||||
|
||||
**Type:** speculative
|
||||
|
||||
**Why it's a smell:** The rationale schema accepts an OPTIONAL `referenceId` field on top of the rec-level `referenceId` (line 169). The doc-comment is candid: "this field stays optional so a future provider that wants to attach a reference at rationale-grain can do so without a schema bump." No production code reads `rationale.referenceId`; no UI surface displays it; the citation footnote in `recommendation-card.tsx:148` reads `norm.referenceId`, not `norm.rationale?.referenceId`. CLAUDE.md: "Don't add features … beyond what the task requires." The field is a v1.4.17+ option dressed up as part of the v1.4.16 contract.
|
||||
|
||||
Verified:
|
||||
|
||||
```
|
||||
$ grep -rn "rationale\.referenceId\|rationale\?.referenceId" src
|
||||
(no output)
|
||||
```
|
||||
|
||||
**Suggested change:** Drop `referenceId: z.string().optional()` from `aiRecommendationRationaleSchema` (and its doc-comment block, lines 119-125). When v1.4.17 actually needs rationale-grain citations, re-add the field then.
|
||||
|
||||
**Risk:** low — optional, silent drop has zero runtime effect; only widens the parser's reject-set, which is the conservative direction.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 4 — `applyLastWorkingCache` doc-comment overstates the guarantee
|
||||
|
||||
**File:** `src/lib/ai/provider-runner.ts:193-211`
|
||||
|
||||
**Type:** comment (WHAT, slightly misleading)
|
||||
|
||||
**Why it's a smell:** The doc-comment says "Returns a NEW array; original is not mutated." That's true but misleading — the inner `splice` does mutate the COPY (`reordered.splice(idx, 1)`); the only real contract is that the input array is left intact. The "NEW array" caveat is also defensive against a non-existent caller pattern: nothing in the code path stores or compares the input array reference. CLAUDE.md: "Default to writing no comments. Don't explain WHAT — well-named identifiers do that."
|
||||
|
||||
**Suggested change:** Trim the comment to just the WHY:
|
||||
|
||||
```diff
|
||||
-/**
|
||||
- * Stable metric → category mapping. Add a metric here when it joins the
|
||||
- * `AchievementMetricKey` union; the page falls back to "engagement" if
|
||||
- * a future definition slips through, so there is no silent drop-off.
|
||||
- * Reorder the input chain so the cached "last working" provider is
|
||||
- * tried first. Returns a NEW array; original is not mutated. When the
|
||||
- * cache is cold (or the cached provider is not in the input list), the
|
||||
- * original order is preserved.
|
||||
- */
|
||||
export function getAchievementCategory(
|
||||
metric: AchievementMetricKey,
|
||||
): AchievementCategory {
|
||||
+/**
|
||||
+ * Reorder the input chain so the cached "last working" provider is
|
||||
+ * tried first. Cold cache or unknown provider → original order.
|
||||
+ */
|
||||
```
|
||||
|
||||
**Risk:** low.
|
||||
**Risk:** low — comment-only.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 7 — Unreachable `default` arm in `stateBadgeFor` switch
|
||||
## Finding 5 — `RecommendationFeedbackSeverity` / `RecommendationFeedbackTimeRange` types duplicated from validation schema
|
||||
|
||||
**File:** `src/components/settings/notification-status-card.tsx:329-337`
|
||||
**File:** `src/components/insights/recommendation-feedback.tsx:35-45`
|
||||
|
||||
**Type:** repetition (mild)
|
||||
|
||||
**Why it's a smell:** The component declares its own union types for severity and time-range:
|
||||
|
||||
```ts
|
||||
function stateBadgeFor(state: ChannelState, t): {...} {
|
||||
switch (state) {
|
||||
case "active": return ...
|
||||
case "auto_disabled": return ...
|
||||
case "sending_paused": return ...
|
||||
case "manually_disabled":
|
||||
default:
|
||||
return { label: t("...stateManuallyDisabled"), ... };
|
||||
}
|
||||
}
|
||||
export type RecommendationFeedbackSeverity = "info" | "suggestion" | "important" | "urgent";
|
||||
export type RecommendationFeedbackTimeRange = "last7days" | "last30days" | "last90days" | "allTime";
|
||||
```
|
||||
|
||||
**Type:** defensive
|
||||
These are byte-for-byte the same as the `z.enum(...)` lists in `src/lib/validations/recommendation-feedback.ts:18-30`. Two source-of-truth sites for the same closed enum invite drift — if v1.4.17 adds `"last180days"` to the validator, this component will silently accept-but-fail-to-display the new range until someone notices.
|
||||
|
||||
**Why it's a smell:** `ChannelState` is a closed union of exactly four members, all explicitly cased. The `default:` collapses with `manually_disabled` and is unreachable; it muddles the four-arm-mapping by suggesting there's a fallthrough case that doesn't exist. CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen."
|
||||
The repetition is small (4+4 strings) so on its own it sits below CLAUDE.md's "three similar lines is better than a premature abstraction" bar — BUT the abstraction already exists (the zod schema). The cost is just exporting `z.infer<typeof recommendationFeedbackSeverity>` aliases from the validation file.
|
||||
|
||||
**Suggested change:** Remove the `default:` keyword (keep the `case "manually_disabled":` body). TypeScript's `switch`-exhaustiveness checking already covers the "future case forgotten" scenario.
|
||||
Cross-call: `recommendation-card.tsx:288-300` already has a hand-rolled `asFeedbackTimeRange` switch over the same four values to validate the rec's timeRange before rendering the feedback row. Three sites of the closed enum, all transcribed.
|
||||
|
||||
**Suggested change:**
|
||||
|
||||
```ts
|
||||
// recommendation-feedback.tsx — import from canonical source
|
||||
import type {
|
||||
RecommendationFeedbackRequest,
|
||||
} from "@/lib/validations/recommendation-feedback";
|
||||
|
||||
export type RecommendationFeedbackSeverity = RecommendationFeedbackRequest["recommendationSeverity"];
|
||||
export type RecommendationFeedbackTimeRange = RecommendationFeedbackRequest["metricSourceTimeRange"];
|
||||
```
|
||||
|
||||
Or even better: drop the local aliases entirely, use the canonical types directly. `asFeedbackTimeRange` in `recommendation-card.tsx` can also `import { recommendationFeedbackTimeRange }` and call `.safeParse(value).success ? value : null` — slightly more code than the switch but a single source of truth.
|
||||
|
||||
**Risk:** low — pure type-rewire.
|
||||
|
||||
**Apply autonomously?** yes (just the type re-export; leave `asFeedbackTimeRange` as-is until a fourth consumer appears).
|
||||
|
||||
---
|
||||
|
||||
## Finding 6 — `formatRelativeTime` justification "polyfill cost" is stale
|
||||
|
||||
**File:** `src/components/insights/insights-page-hero.tsx:43-66`
|
||||
|
||||
**Type:** comment (WHAT, factually questionable)
|
||||
|
||||
**Why it's a smell:** The doc-comment says the hand-rolled formatter "Avoids the `Intl.RelativeTimeFormat` polyfill cost on the client." `Intl.RelativeTimeFormat` has been baseline-supported in every browser HealthLog targets since 2018 (Chrome 71, Safari 14, Firefox 65) — there's no polyfill cost to avoid. The genuine WHY is the second sentence: "keeps every surface that renders relative times locked to the project's established translation vocabulary" (i18n consistency). That's a real reason; the polyfill claim is noise.
|
||||
|
||||
CLAUDE.md: "Default to writing no comments. Don't explain WHAT" — and "WHAT" includes false-positive justifications.
|
||||
|
||||
**Suggested change:** Trim to the true WHY:
|
||||
|
||||
```diff
|
||||
case "manually_disabled":
|
||||
- default:
|
||||
return {
|
||||
label: t("settings.notificationStatus.stateManuallyDisabled"),
|
||||
-/**
|
||||
- * Format a relative time using the i18n translation keys. Avoids the
|
||||
- * `Intl.RelativeTimeFormat` polyfill cost on the client AND keeps every
|
||||
- * surface that renders relative times locked to the project's
|
||||
- * established translation vocabulary.
|
||||
- *
|
||||
- * Buckets: <1min → just now; <60min → N min ago; <24h → N h ago;
|
||||
- * else N days ago.
|
||||
- */
|
||||
+/**
|
||||
+ * Bucketed relative-time using i18n translation keys so every surface
|
||||
+ * shares vocabulary. Buckets: <1m, <1h, <24h, else days.
|
||||
+ */
|
||||
```
|
||||
|
||||
**Risk:** low.
|
||||
**Risk:** low — comment-only.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 8 — `period.since` back-compat shim duplicates `period.start`
|
||||
## Finding 7 — `RecommendationCard` "data-slot" architecture comment block is over-narrating
|
||||
|
||||
**Files:**
|
||||
**File:** `src/components/insights/recommendation-card.tsx:22-47`
|
||||
|
||||
- `src/lib/doctor-report-pdf-core.ts:218-221`
|
||||
- `src/lib/doctor-report-pdf.ts:204-207`
|
||||
- `src/lib/doctor-report-data.ts:362-369` (writes both fields)
|
||||
**Type:** comment (WHAT)
|
||||
|
||||
**Type:** shim / repetition
|
||||
**Why it's a smell:** The header comment block lists every named slot the card exposes (`rec-confidence-slot`, `rec-feedback-slot`, etc.) along with the v1.4.16 phase tag for each. Half of those slots are now FILLED — `<ConfidenceMeter>` (B5d) and `<RecommendationFeedback>` (B5e) both shipped in v1.4.16 and are visible in the same file. The comment still talks about them as "future" and as "named slots so B5d (confidence) and B5e (feedback) can plug in without touching this file" — but B5d / B5e DID touch this file (lines 20, 274-276, 363-365). The slot-vocabulary comment now describes a state that no longer exists.
|
||||
|
||||
**Why it's a smell:** v1.4.15 added explicit `period.start` / `period.end` strings to `DoctorReportData`. The aggregator writes both `since` and `start` to the same value. Both PDF render-paths then read with `data.period.start ?? data.period.since`. The fallback is permanently dead from the server side because `start` is always populated; it could only fire for "in-flight cached client payloads from a v1.4.14 build holding a `since`-only object" — but the PDF generators run AFTER the JSON arrives from `/api/doctor-report` of the SAME deploy, so the wire shape is in-sync by definition.
|
||||
CLAUDE.md: "Don't comment what the code does — comment why" + "Backward-compat shims for paths that no longer exist".
|
||||
|
||||
**Suggested change:** Drop `since` from the aggregator output (it was a v1.4.14 field, replaced; no external API consumers documented). Update both `ReportData` interface declarations to use `period.start` only, drop the `?? data.period.since` fallback in both PDF builders.
|
||||
|
||||
This is a bigger change because it touches the wire shape of `/api/doctor-report`. If the iOS app reads `data.period.since` it would break. Worth checking before deletion.
|
||||
|
||||
**Risk:** medium — requires confirming no external client (iOS app) reads `since`.
|
||||
|
||||
**Apply autonomously?** no.
|
||||
|
||||
---
|
||||
|
||||
## Finding 9 — `decidedFor` "previous-input" tracking in `tour-launcher.tsx` is over-engineered for a one-shot decision
|
||||
|
||||
**File:** `src/components/onboarding/tour-launcher.tsx:138-170`
|
||||
|
||||
**Type:** premature abstraction
|
||||
|
||||
**Why it's a smell:** The launcher tracks a `decidedFor: { userId, flag } | null` state purely so the render-phase setState block runs at most once per (user, flag) transition. But the body of that block already short-circuits on `showTour === null` (which only ever flips once: null → true|false) — so the entire `decidedFor` mechanism is guarding against a re-fire that the `showTour !== null` check already prevents. The comment even calls out "set-state-in-render pattern (mirrors `account-section.tsx`'s `seededUserId`)" — but `account-section`'s pattern there is for a per-prop-change re-seed, which is NOT what this launcher does (it decides once per mount).
|
||||
|
||||
**Suggested change:** Delete `decidedFor` state + setter. The render block becomes:
|
||||
|
||||
```ts
|
||||
if (showTour === null && inputsReady && user) {
|
||||
if (user.onboardingTourCompleted || readSessionDismissed()) {
|
||||
setShowTour(false);
|
||||
} else if (justFromWizard) {
|
||||
setShowTour(false);
|
||||
setDeferredFromWizard(true);
|
||||
} else {
|
||||
setShowTour(true);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Risk:** medium — needs a re-test of the tour gating on auth-cache invalidation. The lint-rule that prompted this pattern (`react-hooks/set-state-in-effect`) is still satisfied because `showTour === null` is the gate.
|
||||
|
||||
**Apply autonomously?** no — touches user-visible flow gating; warrants Marc reviewing the simplified version.
|
||||
|
||||
---
|
||||
|
||||
## Finding 10 — Duplicated Withings-status reauth classifier in two files
|
||||
|
||||
**Files:**
|
||||
|
||||
- `src/lib/withings/sync.ts:240-255` (`isWithingsRefreshReauthFailure` + `extractWithingsStatus`)
|
||||
- `src/app/api/withings/status/route.ts:80-92` (inline equivalent: parses `Withings\s+\w+\s+error:\s*(\d+)` and checks `100/101/102/200..299`)
|
||||
|
||||
**Type:** repetition
|
||||
|
||||
**Why it's a smell:** Two implementations of the same Withings-status decoder, both diverged from each other only in the wrap (function vs inline). Per CLAUDE.md "three similar lines is better than a premature abstraction" — this is exactly two callsites, so the existing helpers in `sync.ts` could be exported and reused, but creating a third util-file would be the over-extraction warned against. The right move is the smallest one: export the existing two helpers from `sync.ts` and import them in the status route.
|
||||
|
||||
**Suggested change:** Mark `isWithingsRefreshReauthFailure` and `extractWithingsStatus` as `export` in `src/lib/withings/sync.ts`, then in `src/app/api/withings/status/route.ts` replace the inline regex+conditional with `if (isWithingsRefreshReauthFailure(message)) { await markReauthRequired(...) }`.
|
||||
**Suggested change:** Replace the architectural narration with a one-sentence WHY:
|
||||
|
||||
```diff
|
||||
// sync.ts
|
||||
-function isWithingsRefreshReauthFailure(message: string): boolean {
|
||||
+export function isWithingsRefreshReauthFailure(message: string): boolean {
|
||||
|
||||
// status/route.ts
|
||||
- const message = error instanceof Error ? error.message : String(error);
|
||||
- const statusMatch = /Withings\s+\w+\s+error:\s*(\d+)/.exec(message);
|
||||
- const statusCode = statusMatch
|
||||
- ? Number.parseInt(statusMatch[1], 10)
|
||||
- : NaN;
|
||||
- const isReauth =
|
||||
- Number.isFinite(statusCode) &&
|
||||
- (statusCode === 100 || ...);
|
||||
- if (isReauth) {
|
||||
+ const message = error instanceof Error ? error.message : String(error);
|
||||
+ if (isWithingsRefreshReauthFailure(message)) {
|
||||
await markReauthRequired(user.id, "withings", message).catch(() => {});
|
||||
}
|
||||
-/**
|
||||
- * v1.4.16 phase B5c — Oura-style RecommendationCard.
|
||||
- *
|
||||
- * Each recommendation lives in its own collapsible card. The
|
||||
- * collapsed row shows: severity badge + rec text + named slot for
|
||||
- * the future confidence-ring (B5d) + chevron toggle.
|
||||
- *
|
||||
- * Expanding reveals:
|
||||
- * - 3-row rationale card (Window / Compared to / Deviation)
|
||||
- * - mini-chart pinned to the rationale's data window via
|
||||
- * HealthChart's new `mini` + `windowOverride` props
|
||||
- * - citation footnote (B5a) when `referenceId` resolves
|
||||
- * - named slot for the future feedback-thumbs (B5e)
|
||||
- *
|
||||
- * Default state: collapsed. Chevron toggles `aria-expanded`. Plain-
|
||||
- * string recs (legacy InsightResult shape) and recs without
|
||||
- * `rationale` render as a non-expandable list item — there's nothing
|
||||
- * to expand to. The legacy-payload CTA at the parent advisor card
|
||||
- * level surfaces the regenerate prompt for those.
|
||||
- *
|
||||
- * Layout slots are deliberately named so B5d (confidence) and B5e
|
||||
- * (feedback) can plug in without touching this file:
|
||||
- *
|
||||
- * - data-slot="rec-confidence-slot" → B5d ConfidenceRing
|
||||
- * - data-slot="rec-feedback-slot" → B5e RecommendationFeedback
|
||||
- */
|
||||
+/**
|
||||
+ * Collapsible recommendation card with severity badge, confidence
|
||||
+ * meter, and an expandable rationale block (window / compared-to /
|
||||
+ * deviation) + mini-chart. Plain-string and rationale-less recs
|
||||
+ * render as a non-expandable row.
|
||||
+ */
|
||||
```
|
||||
|
||||
**Risk:** low — pure rename + one-line export.
|
||||
**Risk:** low — comment-only.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 11 — Practice-name persistence block is duplicated across two routes
|
||||
## Finding 8 — `RecommendationCard.normalise()` mirrors the public `InsightRecommendation` shape almost verbatim
|
||||
|
||||
**Files:**
|
||||
**File:** `src/components/insights/recommendation-card.tsx:65-92`
|
||||
|
||||
- `src/app/api/doctor-report/route.ts:55-69`
|
||||
- `src/app/api/doctor-report/pdf/route.ts:50-60`
|
||||
**Type:** repetition / premature abstraction
|
||||
|
||||
**Type:** repetition (NOT a finding to apply)
|
||||
**Why it's a smell:** The component declares a private `NormalisedRec` interface and a `normalise(rec)` helper that maps the union `InsightRecommendation` (string | object) into a struct with default-undefined fields. But the only "normalisation" the helper performs on the object branch is field-by-field copy (`text: rec.text, severity: rec.severity, …`) — every field is forwarded with the same name and type. The string branch produces `{ text: rec }`. So the entire `NormalisedRec` is just `{ text: string } | InsightRecommendationObject` collapsed to a single shape with all fields optional.
|
||||
|
||||
**Why it's NOT a smell yet:** Two callsites with five-line bodies. CLAUDE.md: "Three similar lines is better than a premature abstraction." Two callers ≠ three; the abstraction (a helper in `doctor-report-data.ts`) would be premature.
|
||||
|
||||
**Suggested change:** None. Watch — if a third doctor-report endpoint is added (e.g. share-link variant), extract then.
|
||||
|
||||
**Apply autonomously?** N/A — explicit anti-finding for record-keeping so the next reviewer doesn't propose the abstraction.
|
||||
|
||||
---
|
||||
|
||||
## Finding 12 — `MockAIProvider.callCount` getter shadows `calls.length`
|
||||
|
||||
**File:** `src/lib/ai/mock-client.ts:99-102`
|
||||
A simpler shape: a single helper `recAsObject(rec) → InsightRecommendationObject` that wraps the string branch:
|
||||
|
||||
```ts
|
||||
/** Number of times `generateCompletion` was invoked. */
|
||||
get callCount(): number {
|
||||
return this.calls.length;
|
||||
function recAsObject(
|
||||
rec: InsightRecommendation,
|
||||
): { text: string; severity?: ...; rationale?: ...; … } {
|
||||
return typeof rec === "string" ? { text: rec } : rec;
|
||||
}
|
||||
```
|
||||
|
||||
**Type:** comment (WHAT) + dead
|
||||
The `NormalisedRec` interface adds nothing the union type doesn't already say. This isn't blocking, but it's the smell of an abstraction that doesn't pull weight — particularly because the resulting fields (`norm.severity`, `norm.text`, etc.) are accessed identically to how a non-normalised object branch would be.
|
||||
|
||||
**Why it's a smell:** The `calls: CompletionParams[]` array is already public; tests can read `provider.calls.length` directly. The getter exists only so a test can write `provider.callCount` instead of `provider.calls.length` — that's a synonym, not a feature. (Compounds with Finding 1: the entire mock is test-only anyway, so this is a cleanup that only matters if Finding 1 is rejected.)
|
||||
**Suggested change:** Replace `normalise` + `NormalisedRec` with the inline `typeof rec === "string"` check at the use site, OR with a single ~3-line `recAsObject` helper. Drop the interface.
|
||||
|
||||
**Suggested change:** If Finding 1 rejected: delete `callCount` getter + the docstring; have tests read `provider.calls.length`.
|
||||
**Risk:** medium — the normalised shape is consumed in 8+ sites in the same file; the rewrite is mechanical but spans the JSX. Worth a careful rebuild rather than a regex find-replace.
|
||||
|
||||
**Apply autonomously?** no — refactor touches the rendered DOM and warrants Marc's eye on the simplified version.
|
||||
|
||||
---
|
||||
|
||||
## Finding 9 — `pickProviderType` "PROMPT_VERSION snapshot" comment describes a missing feature
|
||||
|
||||
**File:** `src/lib/ai/feedback-attribution.ts:46-62`
|
||||
|
||||
**Type:** comment (WHY that doesn't fit the code)
|
||||
|
||||
**Why it's a smell:** The doc-comment for `pickProviderType` explains a per-call PROMPT_VERSION snapshot that the function does NOT do — the function only picks the providerType. The PROMPT_VERSION reading happens at the bottom of `resolveFeedbackAttribution()` (line 84) where it reads the constant directly, not from `details`. The "the current PROMPT_VERSION constant is always returned because the generator itself doesn't yet stash a per-payload prompt version" sentence is in the wrong function's doc-block — it belongs above `resolveFeedbackAttribution` (or above the constant import), not above `pickProviderType` which doesn't deal with PROMPT_VERSION at all.
|
||||
|
||||
CLAUDE.md: "Comment WHY — only when the WHY is true and visible from the code."
|
||||
|
||||
**Suggested change:** Trim `pickProviderType`'s doc-comment to just the priority order; move (or drop) the PROMPT_VERSION sentence:
|
||||
|
||||
```diff
|
||||
/**
|
||||
- * Pure helper exported for unit tests so we can pin the priority order
|
||||
- * (chainProviderType > providerType > "unknown") without hitting the
|
||||
- * DB. The current PROMPT_VERSION constant is always returned because
|
||||
- * the generator itself doesn't yet stash a per-payload prompt version
|
||||
- * (research §3.B notes this is a v1.4.17 ratchet item; reading the
|
||||
- * constant snapshots whatever's deployed when the rating fires).
|
||||
+ * Pick provider attribution from a parsed audit-row details blob.
|
||||
+ * Priority: chainProviderType > providerType > "unknown".
|
||||
*/
|
||||
export function pickProviderType(details: AuditDetailsShape): string {
|
||||
```
|
||||
|
||||
**Risk:** low — comment-only.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 10 — `legacy-payload.ts` `UnknownRec` interface is a single-use throwaway
|
||||
|
||||
**File:** `src/lib/ai/legacy-payload.ts:18-21`
|
||||
|
||||
**Type:** speculative
|
||||
|
||||
**Why it's a smell:** Module declares an `interface UnknownRec { rationale?: unknown }` that is referenced exactly once, two lines down, in a cast: `(rec as UnknownRec).rationale`. That's a 3-line abstraction for a 1-line need. The cast can be inline:
|
||||
|
||||
```ts
|
||||
const rationale = (rec as { rationale?: unknown }).rationale;
|
||||
```
|
||||
|
||||
…and the `UnknownRec` interface deleted.
|
||||
|
||||
**Suggested change:** Inline the cast, delete the interface.
|
||||
|
||||
```diff
|
||||
-interface UnknownRec {
|
||||
- rationale?: unknown;
|
||||
-}
|
||||
-
|
||||
/**
|
||||
* Returns true when the payload is non-empty and at least one
|
||||
* recommendation lacks a rationale object …
|
||||
@@ -45,7 +41,7 @@ export function isLegacyInsightPayload(payload: unknown): boolean {
|
||||
// Structured rec: rationale must be a populated object.
|
||||
if (rec === null || typeof rec !== "object") return true;
|
||||
- const rationale = (rec as UnknownRec).rationale;
|
||||
+ const rationale = (rec as { rationale?: unknown }).rationale;
|
||||
if (
|
||||
rationale === undefined ||
|
||||
```
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes (only if Finding 1 is rejected; otherwise the whole file goes).
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 11 — `SEVERITY_BORDER_CLASSES` duplicated across `recommendations-grid.tsx` and `insights-card.tsx`
|
||||
|
||||
**Files:**
|
||||
|
||||
- `src/components/insights/recommendations-grid.tsx:64-69` (+ `SEVERITY_BORDER_FALLBACK` line 71)
|
||||
- `src/components/insights/insights-card.tsx:40-45` (+ `SEVERITY_BORDER_FALLBACK` line 47)
|
||||
|
||||
**Type:** repetition (anti-finding — flagged for record-keeping)
|
||||
|
||||
**Why it's NOT a finding to apply yet:** Identical 4-entry severity → border-class map plus a 1-line fallback constant. CLAUDE.md: "three similar lines is better than a premature abstraction." Two callsites; the 5-line table is a borderline candidate. A third callsite (the dashboard tile preview, the doctor-report PDF, …) would tip the balance.
|
||||
|
||||
The siblings ALSO have a `SEVERITY_RANK` table and `SEVERITY_BADGE_STYLES` table (in `recommendation-card.tsx` for the latter); collectively that's 3 tables × ~5 entries × 4 severities each. Collapsing them into a single `severity-tokens.ts` module ("urgent" → { rank, borderClass, badgeClass, heroWrapper }) would be the move when a 3rd consumer appears OR when the v1.4.17 prompt-version-aware coloring lands. Not yet.
|
||||
|
||||
**Suggested change:** None — flag for the next reviewer so the abstraction isn't proposed prematurely.
|
||||
|
||||
**Apply autonomously?** N/A.
|
||||
|
||||
---
|
||||
|
||||
## Finding 12 — `confidence-meter.tsx` band-comment block over-explains a 4-line policy
|
||||
|
||||
**File:** `src/components/insights/confidence-meter.tsx:4-23`
|
||||
|
||||
**Type:** comment (WHAT)
|
||||
|
||||
**Why it's a smell:** 20-line header comment explains the band-thresholds (>=80 / 50..79 / 25..49 / <25) and the colour mapping. Lines 46-51 already define `bandFor()` and lines 66-80 already declare the colour-by-band records — the code IS the documentation. CLAUDE.md: "Default to writing no comments. Don't explain WHAT — well-named identifiers do that." The genuine WHY is the "quiet-when-unsure" Apple research stance, but that's already a 1-liner inside `bandFor`'s context.
|
||||
|
||||
**Suggested change:** Replace the header block with the WHY-only:
|
||||
|
||||
```diff
|
||||
-/**
|
||||
- * v1.4.16 phase B5d — `<ConfidenceMeter>`.
|
||||
- *
|
||||
- * Renders a deterministic 0..100 confidence score (server-computed by
|
||||
- * `computeConfidence()` and overridden in `generateInsight()`) as
|
||||
- * either a 5-bar meter (default) or an SVG ring.
|
||||
- *
|
||||
- * Color bands match research §2.A "three-band visual" extended to 4
|
||||
- * bands so the under-50 region splits into "low" + "draft":
|
||||
- *
|
||||
- * - >=80: green ("high"), 5 bars / full ring
|
||||
- * - 50..79: yellow ("medium"), 3-4 bars
|
||||
- * - 25..49: orange ("low"), 2 bars / partial ring + caption
|
||||
- * - <25: "draft" pill REPLACES the meter — Apple's
|
||||
- * "quiet-when-unsure" affordance per research §2.A. The rec is
|
||||
- * drafted but the model isn't asserting it.
|
||||
- *
|
||||
- * aria-label always announces the numeric score so a screen reader
|
||||
- * isn't blind to the visual band distinction. Mobile-friendly: the
|
||||
- * bars variant fits in 56 px wide; the ring variant in 28×28.
|
||||
- */
|
||||
+/**
|
||||
+ * Renders a 0..100 confidence score as a 5-bar meter (default) or SVG
|
||||
+ * ring. Sub-25 replaces the meter with a "draft" pill ("quiet-when-
|
||||
+ * unsure"). aria-label always carries the numeric value.
|
||||
+ */
|
||||
```
|
||||
|
||||
**Risk:** low — comment-only.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Anti-findings (deliberately NOT flagged)
|
||||
|
||||
- **`tour-state.ts` (B5)** — Heavily commented (one block per export averaging 6 lines), but each comment explains WHY (e.g. why `data-tour-id` not class names, why outcome is `"completed"|"skipped"` even though the DB write is identical, why `prevStep` no-ops at index 0). These are genuine WHY notes that future readers will need; CLAUDE.md targets WHAT-comments, not WHY.
|
||||
- **`computeBpInTargetPct` (analytics/bp-in-target.ts)** — pure helper extracted from a route; replaces a known-buggy inline implementation. Long doc-comment explains the v1.4.14 bug and the fix rationale (WHY). Justified.
|
||||
- **`integrations/status.ts` `formatAdminAlertPayload`** — pure formatter exported for unit-testing the message shape without standing up Prisma. Single-callsite extraction, but the test value is real and the surface is narrow. Justified.
|
||||
- **`channel-state.ts` `recordChannelHardReject` / `recordChannelTransientFailure` / `recordChannelSuccess`** — three mutations consumed exactly once each by `dispatcher.ts`. Borderline single-callsite helpers, BUT each one is non-trivial (5+ lines of Prisma + auditLog + branching) and the dispatcher would otherwise be 60 LOC longer with auth/audit interleaved. Refactor pulls weight.
|
||||
- **`generateAdminAlert` flow** — already wired into production via `recordSyncFailure` → `maybeAlertAdmins`. Real consumer.
|
||||
- **`shiftDailySeriesForward` / `averageValue` / `computeComparisonDelta` (`src/lib/charts/comparison-shift.ts`)** — three pure helpers, each with a clear WHY-comment about the v1.4.16 B8 brief + DST safety. All three are consumed by the dashboard tile-strip AND the chart wrapper. Real callers, narrow surface, justified.
|
||||
- **`computeWindowTrend` (`src/lib/analytics/window-trend.ts`)** — pure helper extracted from inline chart logic; replaces a known-buggy "+0.0 kg/Woche" surface. Doc-comment explains the v1.4.16 A8b bug + the split-half rationale. Justified.
|
||||
- **`isBpReadingInTarget` / `computeBpInTargetPct` (`src/lib/analytics/bp-in-target.ts`)** — file-header doc-comment is long but every paragraph is a numbered bug + fix narrative covering the v1.4.14 → v1.4.15 → v1.4.16 lineage. Genuine WHY for a regression-prone area; survives.
|
||||
- **`prefersReducedMotion` (`src/lib/charts/reduced-motion.ts`)** — 25-line file for a `window.matchMedia` query that's used in 2 chart wrappers. Tiny, but the SSR-safe + try/catch shape repeats per-wrapper without it. Earned.
|
||||
- **`HostMetricsChart.buildChartRows`** — exported pure helper for unit tests so the test doesn't render Recharts. Same pattern shipped in scatter-correlation-chart; consistent and useful.
|
||||
- **`ConfidenceMeter`'s `variant` prop** — both `bars` (rec card, line 364) and `ring` (insights-card preview, line 107) are used in production. Two callsites with two different values; the prop pulls weight.
|
||||
- **`provider-chain.ts` `parseProviderChain` + `serializeProviderChain` + `PROVIDER_CHAIN_DEFAULT`** — three callers (provider.ts resolver, provider-chain PUT route, parser-test). Real consumers + a defensive fallback path that the integration tests cover.
|
||||
- **`legacy-payload.ts` `isLegacyInsightPayload`** — wired into `/api/insights/generate/route.ts:205` with a real CTA flow on the dashboard. Real consumer.
|
||||
- **`feedback-attribution.ts` `resolveFeedbackAttribution`** — wired into the feedback API route + has a fallback constant. Real consumer.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| # | File:Line | Apply autonomously? |
|
||||
|---|-----------|---------------------|
|
||||
| 1 | `src/lib/ai/{generate-insight,schema,mock-client}.ts` + `src/lib/ai/prompts/insight-generator.ts` | no |
|
||||
| 2 | `src/lib/ai/codex-client.ts:101-138, 174-178, 192-198, 233-237, 242-246` | no |
|
||||
| 3 | `src/lib/ai/codex-slug-cache.ts:62-72` | yes |
|
||||
| 4 | `src/lib/notifications/channel-state.ts:171` | yes |
|
||||
| 5 | `src/lib/notifications/retry-policy.ts:28` | yes |
|
||||
| 6 | `src/lib/gamification/achievements.ts:44-48` | yes |
|
||||
| 7 | `src/components/settings/notification-status-card.tsx:329-337` | yes |
|
||||
| 8 | `src/lib/doctor-report-{data,pdf,pdf-core}.ts` (`period.since` shim) | no |
|
||||
| 9 | `src/components/onboarding/tour-launcher.tsx:138-170` | no |
|
||||
| 10 | `src/lib/withings/sync.ts` + `src/app/api/withings/status/route.ts` | yes |
|
||||
| 11 | (anti-finding — practice-name dedupe NOT proposed) | n/a |
|
||||
| 12 | `src/lib/ai/mock-client.ts:99-102` | yes (only if F1 rejected) |
|
||||
| # | File:Line | Type | Apply autonomously? |
|
||||
|----|-----------|------|---------------------|
|
||||
| 1 | `src/lib/ai/provider-runner.ts:136-288` (strict `runWithFallback` test-only) | speculative | no |
|
||||
| 2 | `src/lib/ai/generate-insight.ts:197-275` (`confidenceContext` config-bag) | speculative | yes |
|
||||
| 3 | `src/lib/ai/schema.ts:119-126` (`rationale.referenceId` unused) | speculative | yes |
|
||||
| 4 | `src/lib/ai/provider-runner.ts:193-211` (over-stated comment) | comment | yes |
|
||||
| 5 | `src/components/insights/recommendation-feedback.tsx:35-45` (type duplication) | repetition | yes |
|
||||
| 6 | `src/components/insights/insights-page-hero.tsx:43-66` (stale polyfill claim) | comment | yes |
|
||||
| 7 | `src/components/insights/recommendation-card.tsx:22-47` (slot-block over-narration) | comment | yes |
|
||||
| 8 | `src/components/insights/recommendation-card.tsx:65-92` (`normalise()` indirection) | repetition | no |
|
||||
| 9 | `src/lib/ai/feedback-attribution.ts:46-62` (mis-placed PROMPT_VERSION comment) | comment | yes |
|
||||
| 10 | `src/lib/ai/legacy-payload.ts:18-21` (`UnknownRec` interface single-use) | speculative | yes |
|
||||
| 11 | `SEVERITY_BORDER_CLASSES` 2× — anti-finding | repetition | n/a |
|
||||
| 12 | `src/components/insights/confidence-meter.tsx:4-23` (band-block WHAT-comment) | comment | yes |
|
||||
|
||||
**6 yes / 4 no** (excluding anti-findings 11, and 12 which is conditional).
|
||||
**8 yes / 2 no** (excluding anti-finding 11).
|
||||
|
||||
---
|
||||
|
||||
## Notes for the dispatcher
|
||||
|
||||
- Findings 1 + 2 + 3 are clustered — they all stem from the same v1.4.17-scaffolding pattern (test-only strict variant, config-bag with one shape, optional speculative field). If Marc decides to defer the strict-variant migration to v1.4.17, all three could be tackled together in a single "tighten v1.4.16 AI surface" commit.
|
||||
- Finding 5 + 11 both touch the severity-token vocabulary; if a third callsite appears before v1.4.17, both should collapse into a single `severity-tokens.ts` module. Until then, the type-import (F5) is cheap, the table-extract (F11) is not.
|
||||
- All comment-only findings (4, 6, 7, 9, 12) are independently safe and could be rolled into a single hygiene commit.
|
||||
Reference in new issue
Block a user