mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 21:10:59 +00:00
docs(planning): record phase D multi-agent QA + reconcile
Phase D wrap-up: - 5 parallel reviewer findings recorded (code-review / security / design / senior-dev / simplify) - reconcile report at `.planning/phase-D-reconcile-report.md` - v1.4.16 backlog written at `.planning/v1416-backlog.md` (8 deferred HIGH + 39 MED/LOW + 4 simplify-no + 3 process items) - STATE.md phase-D status block updated 0 CRITICAL across all reviewers. 5 HIGH fixed in commitscd3b890/6465412/66d2e07/79bb167. 8 HIGH deferred to v1.4.16 (admin-only leaks, charts visual constraint, cross-cutting design-system bump, large-file refactors, multi-account hardening, process change). 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
d94756389f
commit
1b1214bcb3
8 files changed
+2112
-8
No files matched your search
+43
-8
@@ -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
|
||||
|
||||
|
||||
@@ -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: <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 — 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
|
||||
@@ -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 `<button>` covering the page
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### H4 — Backups / Notifications status / Tour buttons are 32 px tall (below WCAG 44 px)
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### H5 — Withings + moodLog status display: long error message can break card layout
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM / LOW
|
||||
|
||||
### M1 — `/admin` overview audit-preview hides action label on `<sm` viewports
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### 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.
|
||||
@@ -0,0 +1,100 @@
|
||||
# Phase D — Reconcile report (v1.4.15)
|
||||
|
||||
Reviewer: phase-D reconcile agent (sequential after the 5 parallel
|
||||
reviewers).
|
||||
Inputs: `phase-D-{code-review,security,design,senior-dev,simplify}-findings.md`.
|
||||
Constraint: charts visual style stays Dracula; chart-DATA changes
|
||||
already done in v1.4.15. No new dependencies. Pre-commit hooks must
|
||||
run; no `--no-verify`, no `--no-gpg-sign`.
|
||||
|
||||
Verdict: 0 CRITICAL across all five reviewers. 13 HIGH + 6 simplify-yes
|
||||
items triaged below.
|
||||
|
||||
---
|
||||
|
||||
## Simplify — autonomous-yes (6 items)
|
||||
|
||||
| # | Description | Result |
|
||||
| --- | --- | --- |
|
||||
| F3 | drop `inspectCodexSlugCache()` (test-only diag) | applied |
|
||||
| F4 | drop `TEST_CONSTANTS` re-export in `channel-state.ts` | applied |
|
||||
| F5 | drop `RejectKind` type alias in `retry-policy.ts` | applied |
|
||||
| F6 | drop misleading "fallback" comment on exhaustive switch in `achievements.ts` | applied |
|
||||
| F7 | drop unreachable `default:` arm in `stateBadgeFor` | applied |
|
||||
| F10 | export `isWithingsRefreshReauthFailure` and dedupe inline classifier in `/api/withings/status` | applied |
|
||||
|
||||
**6 applied, 0 reverted.** All landed in commit `cd3b890`
|
||||
(`refactor(v1.4.15): apply simplify-review safe suggestions`).
|
||||
|
||||
---
|
||||
|
||||
## HIGH triage
|
||||
|
||||
| # | Reviewer | Finding | Decision | Commit / reason |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| code-review H1 | code-review | restore catch surfaces raw Prisma error | DEFER | admin-only leak; v1.4.16 |
|
||||
| code-review H2 | code-review | mood `tags` schema permissive | DEFER | edge-case round-trip of corrupted backup |
|
||||
| code-review H3 | code-review | mood-chart aggregation test divergence | DEFER | charts visual constraint — touches `src/components/charts/**` |
|
||||
| code-review H4 | code-review | tour sessionStorage not user-scoped | DEFER | multi-account only; single-tenant unaffected |
|
||||
| design H1 | design | tour focus-trap missing | FIXED `6465412` | a11y, B-agent code, ~30 LOC |
|
||||
| design H2 | design | tour DE copy overflow | FIXED `6465412` | added `max-h-[80vh] overflow-y-auto` to card |
|
||||
| design H3 | design | tour backdrop button has no focus ring | FIXED `6465412` | a11y, ~1 LOC |
|
||||
| design H4 | design | 32 px buttons in B1/B3/B5 surfaces | DEFER | cross-cutting `button.tsx` design-system bump; v1.4.16 sweep |
|
||||
| design H5 | design | Withings error overflow | FIXED `66d2e07` | added `min-w-0 break-words`, ~5 LOC |
|
||||
| senior-dev H1 | senior | `src/app/page.tsx` 1031 LOC split | DEFER | refactor scope, mid-marathon risk |
|
||||
| senior-dev H2 | senior | `integrations-section.tsx` 883 LOC split | DEFER | refactor scope |
|
||||
| senior-dev H3 | senior | adopt git worktrees per agent | DEFER | process change for next marathon |
|
||||
| senior-dev H4 | senior | `MockAIProvider` DEFAULT_RESPONSE drift | FIXED `79bb167` | low risk, ~10 LOC |
|
||||
|
||||
**5 HIGH fixed, 8 HIGH deferred.**
|
||||
|
||||
Plus `d947563` for prettier-sweep on the touched files.
|
||||
|
||||
---
|
||||
|
||||
## Final verify status
|
||||
|
||||
- `pnpm typecheck`: 3 pre-existing errors in
|
||||
`src/lib/__tests__/dashboard-layout.test.ts` (lines 89/102/114 →
|
||||
`DashboardWidgetId` narrow-string mismatch). Reproduces on a clean
|
||||
`git stash` of the working tree per STATE.md A4 / B-mobile / C3
|
||||
attribution. Not introduced by this reconcile.
|
||||
- `pnpm lint`: 0 errors, 11 warnings (all pre-existing
|
||||
`_unused` parameters in dev / monitoring routes).
|
||||
- `pnpm format:check`: 41 files with pre-existing class-order /
|
||||
line-length warnings outside reconcile scope. Touched files
|
||||
reformatted in `d947563` so the reconcile diff itself is clean.
|
||||
- `pnpm test`: **1047 / 1047 passing.**
|
||||
|
||||
---
|
||||
|
||||
## v1.4.16 backlog
|
||||
|
||||
Severity-grouped, file:line, terse:
|
||||
[`/Users/marc/Projects/HealthLog/.planning/v1416-backlog.md`](./v1416-backlog.md).
|
||||
|
||||
Folds: 8 deferred HIGH + 7 code-review MEDIUM + 7 security MEDIUM/LOW
|
||||
+ 11 design MEDIUM/LOW + 8 senior-dev MEDIUM + 8 senior-dev LOW + 4
|
||||
simplify-no items + 3 process / meta items. Two single most
|
||||
impactful entries to fast-track in v1.4.16:
|
||||
|
||||
1. **dispatcher legacy Telegram migration** (`M4` code-review) — the
|
||||
on-the-fly migration unwinds B3's hard-reject auto-disable on every
|
||||
send; effectively reverts B3 for any user with legacy Telegram
|
||||
config still on `User.telegramBotToken`.
|
||||
2. **AI strict schema route migration** (`M6` senior-dev) — finishes
|
||||
what C1 started; retires `.passthrough()`; deletes legacy
|
||||
`insightResultSchema`.
|
||||
|
||||
---
|
||||
|
||||
## Pointer
|
||||
|
||||
- Backlog: `.planning/v1416-backlog.md`
|
||||
- Findings sources: `.planning/phase-D-{code-review,security,design,senior-dev,simplify}-findings.md`
|
||||
- Commits this phase (5):
|
||||
- `cd3b890` simplify
|
||||
- `6465412` tour a11y
|
||||
- `66d2e07` Withings error wrap
|
||||
- `79bb167` MockAIProvider strict default
|
||||
- `d947563` prettier sweep
|
||||
@@ -0,0 +1,283 @@
|
||||
# v1.4.15 Phase D — Security Review Findings
|
||||
|
||||
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.
|
||||
|
||||
## Summary
|
||||
|
||||
- **0 CRITICAL** ship-blockers
|
||||
- **0 HIGH**
|
||||
- **8 MEDIUM/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.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM
|
||||
|
||||
### M1 — Restore transaction is not bounded by an interactive-query timeout
|
||||
- **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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
### 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
|
||||
|
||||
---
|
||||
|
||||
## Verified Safe (no findings)
|
||||
|
||||
- **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).
|
||||
|
||||
- **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).
|
||||
|
||||
- **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).
|
||||
|
||||
- **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).
|
||||
|
||||
- **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.
|
||||
|
||||
- **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).
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
**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.
|
||||
@@ -0,0 +1,422 @@
|
||||
# Phase D — Senior-dev review (v1.4.15)
|
||||
|
||||
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).
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL — none
|
||||
|
||||
(There are no architectural mistakes that need to be fixed before
|
||||
shipping v1.4.15.)
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1 — `src/app/page.tsx` (1031 lines) crossed the "single dashboard component is doing too much" threshold
|
||||
|
||||
- **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.
|
||||
|
||||
### H2 — `src/components/settings/integrations-section.tsx` (883 lines) hosts two unrelated feature cards in one client module
|
||||
|
||||
- **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.
|
||||
|
||||
### 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
|
||||
|
||||
- **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.
|
||||
|
||||
---
|
||||
|
||||
## 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
|
||||
|
||||
- **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.
|
||||
|
||||
### 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
|
||||
|
||||
- **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.
|
||||
|
||||
### M3 — `src/components/admin/backups-section.tsx` (529 lines) embeds `<RestoreRowDialog>`, `formatBytes`, three TanStack mutations, and a download handler in one client module
|
||||
|
||||
- **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.
|
||||
|
||||
### M4 — `<OnboardingTour>` (418 lines) embeds layout-math (`computeTooltipPosition`, `measureTarget`), state-machine consumer logic, and rendering in one component
|
||||
|
||||
- **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.
|
||||
|
||||
### M5 — Naming inconsistency: `application_name` (snake_case) vs `applicationName` (camelCase) inside the same `deploy-webhook/route.ts` module
|
||||
|
||||
- **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.
|
||||
|
||||
### 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
|
||||
|
||||
- **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.
|
||||
|
||||
### M7 — `RecentAchievementsCard` and `<RecentAuditPreview>` and `<SystemStatusSummary>` all marked `"use client"` but only fetch read-only data — could be RSCs
|
||||
|
||||
- **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.
|
||||
|
||||
### 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
|
||||
|
||||
- **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.
|
||||
|
||||
---
|
||||
|
||||
## LOW
|
||||
|
||||
### L1 — `STATUS_TABS` / `FEEDBACK_STATUS_TABS` / `RANGE_DAYS` / `BACKOFF_SCHEDULE_MS` use four different "frozen tuple" idioms across the v1.4.15 changeset
|
||||
|
||||
- **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.
|
||||
|
||||
### L2 — Tour-launcher's "set state in render" comment block (lines 124-130) is paraphrasing what the code does, not WHY
|
||||
|
||||
- **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.
|
||||
|
||||
### L3 — `safeEncryptError` and `safeDecryptError` swallow errors with literal sentinel strings (`"<encrypt failed>"`, `"(error message unavailable)"`)
|
||||
|
||||
- **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.
|
||||
|
||||
### L4 — `RANGE_DAYS = [7, 30, 90] as const` in `medication-compliance-chart.tsx` duplicates the same constant from `health-chart.tsx`
|
||||
|
||||
- **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.
|
||||
|
||||
### L5 — `MOODLOG_LAST_SYNCED_AT` legacy timestamp lives on the `User` model alongside `IntegrationStatus.lastSuccessAt`
|
||||
|
||||
- **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.
|
||||
|
||||
### L6 — `src/lib/ai/codex-client.ts` exports a `__test` namespace at the bottom (lines 479-483) for unit-test introspection
|
||||
|
||||
- **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.
|
||||
|
||||
### 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
|
||||
|
||||
- **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.
|
||||
|
||||
### L8 — `pickRecentUnlocks(achievements, limit)` in `recent-achievements-card.tsx` is exported "for unit testing" — this is a code smell
|
||||
|
||||
- **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).
|
||||
|
||||
---
|
||||
|
||||
## 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()`.
|
||||
|
||||
The structural foundation is solid; the findings above are mostly
|
||||
"this could be cleaner", 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).
|
||||
|
||||
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.
|
||||
@@ -0,0 +1,367 @@
|
||||
# Phase D — Simplify/Refine review findings (v1.4.15)
|
||||
|
||||
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.
|
||||
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
|
||||
|
||||
**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`
|
||||
|
||||
**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.
|
||||
|
||||
Verified:
|
||||
|
||||
```
|
||||
$ grep -rn "getLastDiagnostics\|CodexAttemptDiagnostics" 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`
|
||||
|
||||
```ts
|
||||
export const TEST_CONSTANTS = { MAX_CONSECUTIVE_FAILURES } as const;
|
||||
```
|
||||
|
||||
**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
|
||||
}
|
||||
```
|
||||
|
||||
**Type:** comment (WHAT not WHY, factually wrong)
|
||||
|
||||
**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.
|
||||
|
||||
CLAUDE.md: "Default to writing no comments. Don't explain WHAT — well-named identifiers do that."
|
||||
|
||||
**Suggested change:** Delete the misleading sentence. If the WHY is "stable mapping that TS guarantees exhaustiveness", trust the type system.
|
||||
|
||||
```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.
|
||||
- */
|
||||
export function getAchievementCategory(
|
||||
metric: AchievementMetricKey,
|
||||
): AchievementCategory {
|
||||
```
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 7 — Unreachable `default` arm in `stateBadgeFor` switch
|
||||
|
||||
**File:** `src/components/settings/notification-status-card.tsx:329-337`
|
||||
|
||||
```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"), ... };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Type:** defensive
|
||||
|
||||
**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."
|
||||
|
||||
**Suggested change:** Remove the `default:` keyword (keep the `case "manually_disabled":` body). TypeScript's `switch`-exhaustiveness checking already covers the "future case forgotten" scenario.
|
||||
|
||||
```diff
|
||||
case "manually_disabled":
|
||||
- default:
|
||||
return {
|
||||
label: t("settings.notificationStatus.stateManuallyDisabled"),
|
||||
```
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 8 — `period.since` back-compat shim duplicates `period.start`
|
||||
|
||||
**Files:**
|
||||
|
||||
- `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:** shim / repetition
|
||||
|
||||
**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.
|
||||
|
||||
**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(...) }`.
|
||||
|
||||
```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(() => {});
|
||||
}
|
||||
```
|
||||
|
||||
**Risk:** low — pure rename + one-line export.
|
||||
|
||||
**Apply autonomously?** yes.
|
||||
|
||||
---
|
||||
|
||||
## Finding 11 — Practice-name persistence block is duplicated across two routes
|
||||
|
||||
**Files:**
|
||||
|
||||
- `src/app/api/doctor-report/route.ts:55-69`
|
||||
- `src/app/api/doctor-report/pdf/route.ts:50-60`
|
||||
|
||||
**Type:** repetition (NOT a finding to apply)
|
||||
|
||||
**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`
|
||||
|
||||
```ts
|
||||
/** Number of times `generateCompletion` was invoked. */
|
||||
get callCount(): number {
|
||||
return this.calls.length;
|
||||
}
|
||||
```
|
||||
|
||||
**Type:** comment (WHAT) + dead
|
||||
|
||||
**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:** If Finding 1 rejected: delete `callCount` getter + the docstring; have tests read `provider.calls.length`.
|
||||
|
||||
**Risk:** low.
|
||||
|
||||
**Apply autonomously?** yes (only if Finding 1 is rejected; otherwise the whole file goes).
|
||||
|
||||
---
|
||||
|
||||
## 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.
|
||||
|
||||
---
|
||||
|
||||
## 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) |
|
||||
|
||||
**6 yes / 4 no** (excluding anti-findings 11, and 12 which is conditional).
|
||||
@@ -0,0 +1,252 @@
|
||||
# v1.4.16 backlog (deferred from v1.4.15 phase D)
|
||||
|
||||
Severity-grouped, file:line, terse. Items here came out of the parallel
|
||||
phase-D code-review / security / design / senior-dev / simplify
|
||||
agents.
|
||||
|
||||
---
|
||||
|
||||
## HIGH — deferred (correctness / coupling)
|
||||
|
||||
### code-review (4)
|
||||
|
||||
- **H1** — `src/app/api/admin/backups/[id]/restore/route.ts:253-377` —
|
||||
scrub raw Prisma error in catch block; return stable "Restore failed"
|
||||
500 + audit row carries verbose text. Admin-only leak, low blast.
|
||||
- **H2** — `src/app/api/admin/backups/[id]/restore/route.ts:353-366` +
|
||||
`src/lib/validations/backup.ts:73-81` — tighten `moodEntrySchema.tags`
|
||||
to `z.union([z.null(), z.string().refine(JSON.parse → array)])` OR
|
||||
validate via JSON-parse in pre-tx loop.
|
||||
- **H3** — `src/components/charts/mood-chart.tsx:185-219` vs `:247-292`
|
||||
— exported `aggregateMoodEntries` and inline `chartData` aggregation
|
||||
diverge; unit test exercises a bucketing user never sees. Either drop
|
||||
the export or refactor `chartData` to call it. Charts visual style
|
||||
constraint applies — function-extract only, no visual change.
|
||||
- **H4** — `src/components/onboarding/tour-launcher.tsx:138-170` —
|
||||
scope sessionStorage keys (`healthlog-tour-session-dismissed`,
|
||||
`healthlog-tour-referrer`) by user id OR clear on logout.
|
||||
Multi-account / impersonation only.
|
||||
|
||||
### design / UX (1 of 5 — others fixed)
|
||||
|
||||
- **H4 design** — Button/Tabs/Switch default heights below 44 px on
|
||||
the new B1/B3/B5 surfaces:
|
||||
`src/components/admin/backups-section.tsx:84,343,397,438,492`,
|
||||
`src/components/settings/notification-status-card.tsx:264,280`,
|
||||
`src/components/onboarding/tour.tsx:385,396,405`,
|
||||
`src/components/admin/recent-audit-preview.tsx:80-83`.
|
||||
Cross-cutting `button.tsx` `h-9 → h-11` bump, sweep call-sites.
|
||||
|
||||
### senior-dev (4)
|
||||
|
||||
- **H1 senior** — `src/app/page.tsx` (1031 LOC) — split
|
||||
`<DashboardShell>` + `<DashboardTileStrip>` + `<DashboardChartGrid>`
|
||||
+ `<DashboardEmptyState>`; pull visibility resolver into
|
||||
`src/lib/dashboard-visibility.ts`. Mechanical.
|
||||
- **H2 senior** — `src/components/settings/integrations-section.tsx`
|
||||
(883 LOC) — split into
|
||||
`src/components/settings/integrations/{integration-status-banner,
|
||||
withings-card, moodlog-card}.tsx` + composing index. Mirror admin
|
||||
per-section pattern.
|
||||
- **H3 senior** — Process — adopt
|
||||
`superpowers:using-git-worktrees` per parallel agent so commit
|
||||
messages match diffs. Eight v1.4.15 commits had drift.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM — deferred
|
||||
|
||||
### code-review
|
||||
|
||||
- **M1** — `src/lib/ai/generate-insight.ts:155-158` — retry-once
|
||||
injects correction into `userPrompt`, not `systemPrompt`; either fix
|
||||
the path or fix the docstring.
|
||||
- **M2** — `src/lib/ai/codex-client.ts:101-138` — document
|
||||
`getLastDiagnostics()` not concurrent-safe OR move into
|
||||
`generateCompletion()` result envelope.
|
||||
- **M3** — `src/lib/notifications/channel-state.ts:90-130` — collapse
|
||||
the two Prisma writes into one transaction so increment +
|
||||
retry-stamp + audit are atomic.
|
||||
- **M4** — `src/lib/notifications/dispatcher.ts:75-97` — legacy
|
||||
Telegram migration upserts `enabled: true` on every send; will
|
||||
re-enable channels B3 auto-disabled. Add guard "only when no row
|
||||
exists at all" or stop touching `enabled`.
|
||||
- **M5** — `src/components/charts/medication-compliance-chart.tsx:75-92`
|
||||
— render days where `taken === 0 && scheduled > 0` as missing-data
|
||||
(skip / dashed) instead of 0 %. Charts visual constraint applies —
|
||||
semantic-only, no Dracula change.
|
||||
- **M6** — `src/lib/notifications/dispatcher.ts:108-111`,
|
||||
`src/lib/integrations/status.ts:476-484`,
|
||||
`src/app/api/internal/deploy-webhook/route.ts:128-141` — split
|
||||
`SYSTEM_ALERT_DEPLOY` / `SYSTEM_ALERT_INTEGRATION` event types so
|
||||
admins can opt out of one without losing security alerts.
|
||||
- **M7** — `src/components/doctor-report/doctor-report-dialog.tsx:64-75,
|
||||
151-152` — anchor date range to Europe/Berlin explicitly OR document
|
||||
the off-by-one boundary risk.
|
||||
|
||||
### security
|
||||
|
||||
- **M1 sec** — `src/app/api/admin/backups/[id]/restore/route.ts:253-377`
|
||||
— pass `{ timeout: 30_000, maxWait: 5_000 }` to `prisma.$transaction()`;
|
||||
default 5s trip leaves user partially restored on slow disk / large
|
||||
payload.
|
||||
- **M2 sec** — `src/app/api/admin/backups/upload/route.ts:193`,
|
||||
`src/lib/crypto.ts:199` — capture `extractKeyId(encrypted)` in
|
||||
`admin.backups.upload` audit `details` for forensic key-rotation
|
||||
trace.
|
||||
- **M4 sec** — `src/app/api/admin/audit-log/route.ts:34-53` — document
|
||||
`details` JSON returned cross-tenant, must be tightened before any
|
||||
multi-tenant release.
|
||||
- **M5 sec** — `src/app/api/admin/notifications/test/route.ts` —
|
||||
add `checkRateLimit('admin-notifications-test:' + admin.id, 5, 5min)`
|
||||
mirroring user-side test endpoints.
|
||||
- **M6 sec** — `src/app/api/admin/backups/[id]/download/route.ts:92-114`
|
||||
— RFC 5987 filename or drop userId from filename; defensive against
|
||||
future ID-format change.
|
||||
- **M7 sec** — `src/app/api/admin/backups/[id]/restore/route.ts:259-279`
|
||||
— decide + document: restore preserves WithingsConnection,
|
||||
IntegrationStatus, encrypted credentials, Telegram creds. Either
|
||||
extend delete scope to mirror `DELETE /api/admin/data` OR document
|
||||
current preservation behaviour.
|
||||
|
||||
### design
|
||||
|
||||
- **M1 design** — `src/components/admin/recent-audit-preview.tsx:136-140`
|
||||
— show compact verb badge on `<sm` instead of hiding action label.
|
||||
- **M2 design** — `src/components/admin/backups-section.tsx:124-141`
|
||||
— destructive button disabled-state legibility on dark bg.
|
||||
- **M3 design** — `src/components/admin/backups-section.tsx:371-410`
|
||||
— drag-and-drop affordance on backup upload area.
|
||||
- **M4 design** — `src/components/onboarding/tour.tsx:307-309` +
|
||||
`:328-334` — define click-flow under spotlight cutout (block clicks
|
||||
OR advance on target click).
|
||||
- **M6 design** — `src/app/achievements/page.tsx:368-370` — visually
|
||||
distinguish category-completed (`5/5`) from in-progress (`2/5`) with
|
||||
dracula-green badge or trophy icon.
|
||||
- **M7 design** — `src/components/gamification/recent-achievements-card.tsx:127-130`
|
||||
— empty card needs CTA pointing to first-unlock action. Use
|
||||
`<EmptyState size="compact">`.
|
||||
- **M9 design** — `src/components/settings/integrations-section.tsx:158,165`
|
||||
+ `notification-status-card.tsx` — switch absolute timestamps to
|
||||
relative ("vor 3 Min.") once `useFormatters().relativeTime` helper
|
||||
exists. Helper itself is also a v1.4.16 task.
|
||||
|
||||
### senior-dev
|
||||
|
||||
- **M1 senior** — Reliability state-machine duplication
|
||||
(`src/lib/integrations/status.ts` + `src/lib/notifications/channel-state.ts`
|
||||
+ `src/lib/notifications/retry-policy.ts`); extract
|
||||
`src/lib/reliability/` once a third consumer exists.
|
||||
- **M2 senior** — `src/lib/notifications/dispatcher.ts:49-105` —
|
||||
delete legacy `User.telegramBotToken/chatId` migration block + the
|
||||
three deprecated User columns in a v1.4.16 schema migration. Verify
|
||||
via SQL audit first.
|
||||
- **M3 senior** — `src/components/admin/backups-section.tsx` (529 LOC)
|
||||
— extract `<TypedConfirmDialog>` to
|
||||
`src/components/admin/_dialogs/`, `formatBytes` to
|
||||
`src/lib/format.ts`, `download-blob` plumbing to
|
||||
`src/lib/download-blob.ts`.
|
||||
- **M4 senior** — `src/components/onboarding/tour.tsx` — extract
|
||||
`measureTarget` + `computeTooltipPosition` to
|
||||
`src/lib/onboarding/tour-positioning.ts` for unit-test isolation.
|
||||
- **M5 senior** — `src/app/api/internal/deploy-webhook/route.ts` —
|
||||
unify `meta:{}` keys to camelCase (`applicationName`,
|
||||
`deployOutcome`); document wire-vs-app boundary.
|
||||
- **M6 senior** — `src/lib/ai/types.ts` (`insightResultSchema`) +
|
||||
`src/lib/ai/schema.ts` (`aiInsightResponseSchema`) — single source of
|
||||
truth, migrate route + UI to strict schema, drop `.passthrough()`,
|
||||
delete legacy.
|
||||
- **M8 senior** — Pick one config pattern (lazy env-read vs
|
||||
module-frozen) across `integrations/status.ts` + `retry-policy.ts`;
|
||||
document in CLAUDE.md.
|
||||
|
||||
---
|
||||
|
||||
## LOW — deferred
|
||||
|
||||
### code-review
|
||||
|
||||
- **L1** — `src/components/onboarding/tour.tsx:188` — `TOOLTIP_HEIGHT
|
||||
= 220` fixed estimate; measure rendered card via ResizeObserver. (H2
|
||||
fix above gave a `max-h-[80vh] overflow-y-auto` band-aid for v1.4.15.)
|
||||
- **L2** — `src/lib/ai/codex-client.ts:470-475` — comment-clarity nit
|
||||
on `redactBody()` regex (downgraded to nit).
|
||||
- **L3** — `src/lib/analytics/bp-in-target.ts:57-78` —
|
||||
`findClosestDia()` O(n²); binary-search after sort.
|
||||
- **L4** — `src/lib/ai/codex-client.ts:40-45` — drop `gpt-4o` from
|
||||
`DEFAULT_SLUG_FALLBACK_CHAIN` OR condition on auth method.
|
||||
- **L5** — `src/lib/__tests__/dashboard-layout.test.ts` — reclassify
|
||||
the typecheck errors (currently called pre-existing in STATE.md but
|
||||
the file is +119 in this milestone).
|
||||
|
||||
### design
|
||||
|
||||
- **M5 design** — `src/app/achievements/page.tsx:261,280,294` —
|
||||
`min-h-34` non-standard; switch to `min-h-[8.5rem]` or `min-h-36`.
|
||||
- **M8 design** — `src/components/settings/notification-status-card.tsx:202-256`
|
||||
— colour disabled-reason `text-destructive`, consecutive-failures
|
||||
`text-dracula-orange` for at-a-glance triage.
|
||||
- **M10 design** — `src/components/onboarding/tour.tsx:337-344` —
|
||||
live-region `: ` → `, ` so screen readers don't speak punctuation.
|
||||
- **M11 design** — `src/components/layout/sidebar-nav.tsx:514` —
|
||||
tighten admin-sub-item active match to
|
||||
`pathname === sectionPath || startsWith(`${sectionPath}/`)`.
|
||||
|
||||
### security
|
||||
|
||||
- **M3 sec** — `src/lib/ai/codex-slug-cache.ts:32-34` — document the
|
||||
global cache; add Wide Event annotation on slug-rejection
|
||||
invalidation.
|
||||
- **M8 sec** — `src/app/api/internal/deploy-webhook/route.ts:84-96` —
|
||||
acknowledged trade-off, no change needed.
|
||||
|
||||
### senior-dev
|
||||
|
||||
- **L1 senior** — Frozen-tuple style inconsistency across charts /
|
||||
retry-policy / dashboard-visibility; document canonical pattern.
|
||||
- **L2 senior** — `src/components/onboarding/tour-launcher.tsx:124-130`
|
||||
— trim 8-line StrictMode comment.
|
||||
- **L3 senior** — `src/lib/integrations/status.ts:365-383` — write
|
||||
`null` on encrypt failure instead of sentinel string.
|
||||
- **L4 senior** — `RANGE_DAYS = [7,30,90]` duplicated across
|
||||
`medication-compliance-chart.tsx` + `health-chart.tsx`; extract.
|
||||
- **L5 senior** — `MOODLOG_LAST_SYNCED_AT` + `IntegrationStatus.lastSuccessAt`
|
||||
duplication; document v1.4.16 cutover.
|
||||
- **L6 senior** — `src/lib/ai/codex-client.ts:477-483` — replace
|
||||
`__test` namespace with named exports.
|
||||
- **L7 senior** — Function-signature consistency (`channel-state.ts`
|
||||
positional vs `status.ts` object-arg-bag).
|
||||
- **L8 senior** — `src/components/gamification/recent-achievements-card.tsx:56-75`
|
||||
— move `pickRecentUnlocks` to `src/lib/gamification/recent-unlocks.ts`;
|
||||
same for `aggregateMedicationCompliance` in
|
||||
`medication-compliance-chart.tsx`.
|
||||
|
||||
---
|
||||
|
||||
## Simplify — deferred (4 of 6 yes-items not applied autonomously)
|
||||
|
||||
- **F1** — `src/lib/ai/{generate-insight,schema,mock-client}.ts` +
|
||||
`prompts/insight-generator.ts` — schema-enforcement wrapper wired
|
||||
into tests only. Ship-it call: keep as v1.4.16 scaffolding OR revert.
|
||||
- **F2** — `src/lib/ai/codex-client.ts:101-138, 174-178, 192-198,
|
||||
233-237, 242-246` — `getLastDiagnostics()` + `CodexAttemptDiagnostics`
|
||||
read only by tests. Either wire into `annotate()` or delete.
|
||||
- **F8** — `src/lib/doctor-report-{data,pdf,pdf-core}.ts` —
|
||||
`period.since` back-compat shim duplicates `period.start`. Drop
|
||||
after confirming iOS app does not read `since`.
|
||||
- **F9** — `src/components/onboarding/tour-launcher.tsx:138-170` —
|
||||
delete `decidedFor` over-engineered guard; `showTour === null`
|
||||
already prevents re-fire.
|
||||
- **F12** — `src/lib/ai/mock-client.ts:99-102` — drop `callCount`
|
||||
getter; conditional on F1.
|
||||
|
||||
---
|
||||
|
||||
## Process / meta
|
||||
|
||||
- Adopt `superpowers:using-git-worktrees` per parallel agent for the
|
||||
v1.4.16 marathon.
|
||||
- Migrate route + UI for AI insights to the strict schema; retire
|
||||
`.passthrough()`.
|
||||
- Resolve the dashboard-layout.test.ts typecheck regression (currently
|
||||
blocking `pnpm typecheck`).
|
||||
Reference in new issue
Block a user