diff --git a/.planning/phase-D-code-review-findings.md b/.planning/phase-D-code-review-findings.md index 18cf8c417..9d0942afc 100644 --- a/.planning/phase-D-code-review-findings.md +++ b/.planning/phase-D-code-review-findings.md @@ -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: " 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(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:` 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) diff --git a/.planning/phase-D-design-findings.md b/.planning/phase-D-design-findings.md index 7f376dbcb..6b1bfb912 100644 --- a/.planning/phase-D-design-findings.md +++ b/.planning/phase-D-design-findings.md @@ -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 `` consuming ``, ``, ``, `` — 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 `` per section. The dashboard `/` likewise does not import ``. **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 `` / ``. B5c/d/e/B1b are invisible + +- **File:** `src/app/insights/page.tsx:1006-1539` +- **Issue:** every insights section renders `` (text-only summary, no rec card, no rationale, no confidence meter, no thumbs-feedback). The Apple-Health-style polish from B1b (`` is mounted, but it's just a header band) and the entire B5c/d/e rec-card surface (``, ``, ``, ``) is never reachable from any live route. Confirmed via `grep "InsightAdvisorCard"` — zero non-test imports across `src/`. +- **Recommendation:** wire `` 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 — `` (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 `` with a polished preview that renders top severity-ordered recs + ring-variant `` inline. The dashboard never imports it. `grep "InsightsCardPreview" src/app` — zero matches. The dashboard renders `RecentAchievementsCard` + tile strip + charts, no insights preview. +- **Recommendation:** mount `` 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 `` segmented control was scoped but never built; only the persistence layer + chart-side rendering exist. +- **Recommendation:** add a `` (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 `