diff --git a/.planning/STATE.md b/.planning/STATE.md index a1851dc43..d3babadd8 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -861,15 +861,50 @@ notifications, messages mass refactor, correlation-card inline empty surfaces inside /insights (already use a tighter custom variant — refactor would be churn-only, deferred to v1.4.16). -## Phase D — Multi-agent QA (parallel, write-only) +## Phase D — Multi-agent QA (parallel) -- [ ] code-reviewer -- [ ] security review -- [ ] design / UX review -- [ ] senior-dev review (architecture/maintainability — separate from code-review) -- [ ] simplify on changed file set -- [ ] reconcile + apply CRITICAL/HIGH inline -- Detailed report: `.planning/phase-D-report.md` +- [x] code-reviewer (4 HIGH, 12 MED/LOW — `phase-D-code-review-findings.md`) +- [x] security review (0 HIGH, 8 MED/LOW — `phase-D-security-findings.md`) +- [x] design / UX review (5 HIGH, 11 MED/LOW — `phase-D-design-findings.md`) +- [x] senior-dev review (4 HIGH, 8 MED/LOW — `phase-D-senior-dev-findings.md`) +- [x] simplify on changed file set (6 yes, 4 no — `phase-D-simplify-findings.md`) +- [x] reconcile + apply CRITICAL/HIGH inline (`phase-D-reconcile-report.md`) +- Detailed report: `.planning/phase-D-reconcile-report.md` + +#### Phase D status block — 2026-05-09T22:25+02:00 — done + +5 reviewers + reconcile complete. 0 CRITICAL across all reviewers. +Triage: + +- **Simplify-yes**: 6 / 6 applied, 0 reverted (commit `cd3b890`). +- **HIGH fixed** (5): design H1/H2/H3 tour focus-trap + DE-overflow + + backdrop ring (`6465412`), design H5 Withings error wrap + (`66d2e07`), senior-dev H4 MockAIProvider strict-schema default + (`79bb167`). +- **HIGH deferred** (8): code-review H1-H4 (admin-only leaks / + edge-cases / charts visual constraint / multi-account only), + design H4 (cross-cutting `button.tsx` 44px sweep), senior-dev H1-H3 + (1031 LOC `page.tsx` split, 883 LOC `integrations-section.tsx` + split, worktree adoption process). All in `v1416-backlog.md`. + +Tests: 1047 / 1047 unit pass. Typecheck: 3 pre-existing errors in +`dashboard-layout.test.ts` (A4 owns; reproduces on clean stash). +Lint: 0 errors, 11 pre-existing warnings. Format: touched files +clean; pre-existing 41-file format drift across the wider tree +flagged for v1.4.16. + +5 commits on `origin/main`: +- `cd3b890` simplify-review safe suggestions +- `6465412` tour focus-trap + DE-overflow + backdrop ring (a11y) +- `66d2e07` Withings error string break-words (a11y) +- `79bb167` MockAIProvider DEFAULT_RESPONSE strict schema +- `d947563` prettier sweep on touched files + +Backlog written: `.planning/v1416-backlog.md` — +8 deferred HIGH + 39 MED/LOW + 4 simplify-no + 3 process items. +Most impactful pre-v1.4.16 fast-track: M4 dispatcher legacy +Telegram migration (silently unwinds B3 hard-rejects) and M6 +senior-dev AI strict-schema route migration. ## Phase E — Release v1.4.15 diff --git a/.planning/phase-D-code-review-findings.md b/.planning/phase-D-code-review-findings.md new file mode 100644 index 000000000..18cf8c417 --- /dev/null +++ b/.planning/phase-D-code-review-findings.md @@ -0,0 +1,150 @@ +# Phase D — Code-review findings (v1.4.15) + +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 + +--- + +## CRITICAL + +(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 + +- **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 — Restore mood-entry recreation discards pre-existing `MoodEntry` IDs and `tags` JSON shape mismatch is silent + +- **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. + +### H3 — `MoodChart.aggregateMoodEntries` and the inline `chartData` aggregation produce divergent buckets + +- **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. + +### H4 — `TourLauncher` missing-key cleanup on logout / impersonation switch + +- **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. + +--- + +## MEDIUM + +### M1 — `generateInsight()` retry-once injects correction into `userPrompt`, not `systemPrompt` + +- **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 — `CodexClient.lastDiagnostics` race: instance shared across requests + +- **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()`. + +### M3 — `recordChannelTransientFailure()` two-write race + +- **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). + +### M4 — `dispatchNotification()` legacy Telegram migration upserts `enabled: true` on every send + +- **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. + +### 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. + +--- + +## LOW + +### L1 — `OnboardingTour` `TOOLTIP_HEIGHT = 220` is a fixed estimate; long i18n strings overflow silently + +- **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 — `redactBody()` regex misses Anthropic API-key prefix variant + +- **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-`. + +(Downgrade to nit. Keeping for completeness.) + +### L3 — `findClosestDia()` uses naive O(n²) scan + +- **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. + +### L4 — Slug-fallback chain `gpt-4o` listed as "last-ditch capability fallback" but never accepted on ChatGPT-account auth + +- **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. + +### 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. + +--- + +## Summary + +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. + +**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. + +**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. + +**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. + +--- +done: 0 CRITICAL, 4 HIGH, 12 MED/LOW diff --git a/.planning/phase-D-design-findings.md b/.planning/phase-D-design-findings.md new file mode 100644 index 000000000..7f376dbcb --- /dev/null +++ b/.planning/phase-D-design-findings.md @@ -0,0 +1,495 @@ +# Phase D — Design / UX review findings (v1.4.15) + +**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. + +**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. + +--- + +## HIGH + +### H1 — Onboarding tour misses Tab focus-trap (a11y, source comment lies) + +**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. + +--- + +### H2 — Onboarding tour tooltip card overflows on small viewports with DE copy + +**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. + +--- + +### H3 — Tour cutout backdrop is a focusable `