From 1f5ad68033044e6d95e066d3cd12ab7b513059ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 10 May 2026 14:30:57 +0200 Subject: [PATCH] docs(planning): record Wave D reconcile + reviewer findings STATE update + reconcile report captures the six reviewer briefs and the inline triage outcome (one CRITICAL fixed, seven HIGH applied, three HIGH carried over). Backlogs updated: the v1.4.20 file gains a "Phase D reconcile carry-over" block with file:line references; the v1.5 file inherits the strategic risk watchlist (conversation persistence eviction, correlation FDR control, streaming UX, prompt-injection refusal, safeParse legacy-payload audit pattern). Co-Authored-By: Claude Opus 4.7 (1M context) --- .planning/STATE.md | 48 +- .planning/phase-D-code-review-findings.md | 948 +++++++----------- .planning/phase-D-design-findings.md | 698 ++++++------- .planning/phase-D-reconcile-report.md | 149 ++- .planning/phase-D-security-findings.md | 609 +++++------ .planning/phase-D-senior-dev-findings.md | 766 ++++++++------ .planning/phase-D-simplify-findings.md | 457 +++++---- .../phase-D-v1419-product-lead-review.md | 377 +++++++ .planning/v1420-backlog.md | 129 +++ .planning/v15-backlog.md | 48 + 10 files changed, 2281 insertions(+), 1948 deletions(-) create mode 100644 .planning/phase-D-v1419-product-lead-review.md diff --git a/.planning/STATE.md b/.planning/STATE.md index bf590d190..b89f0bd56 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -211,16 +211,44 @@ Last update: 2026-05-10T12:48+02:00 ## Wave D — Multi-agent QA + Product-Lead review -- [ ] code-reviewer -- [ ] security review -- [ ] design / UX review -- [ ] senior-dev review -- [ ] simplify -- [ ] Product Lead — strategic for v1.4.20 (Insights redesign roadmap, - AI Coach feasibility) -- [ ] Reconcile applies CRITICAL/HIGH inline -- Detailed report: `.planning/phase-D-report.md` + - `product-lead-review.md` +- [x] code-reviewer (1 CRIT + 5 HIGH + 12 MED/LOW) +- [x] security review (0 CRIT, 0 HIGH, 3 MED/LOW) +- [x] design / UX review (0 CRIT, 2 HIGH, 8 MED/LOW) +- [x] senior-dev review (0 CRIT, 3 HIGH, 13 MED/LOW) +- [x] simplify (5 apply-yes, 2 apply-no) +- [x] Product Lead — v1.4.20 redesign plan filed at + `phase-D-v1419-product-lead-review.md` +- [x] Reconcile applied CRITICAL + 7/10 HIGH + all 5 simplify-yes; 3 + HIGH deferred to `.planning/v1420-backlog.md` under "Phase D — + v1.4.19 reconcile carry-over". Strategic items pulled into + `.planning/v15-backlog.md`. 1672/1672 unit + 67/67 integration + green; typecheck clean; lint baseline (0 errors / 12 + pre-existing warnings). +- Detailed report: `.planning/phase-D-reconcile-report.md` + +### Status block — D (v1.4.19) + +- 2026-05-09T14:30+02:00 — Phase D reconcile complete. Six atomic + commits on origin/main: `ef74241` (CRITICAL C-01 mobile Sys/Dia + badge enum mismatch + TDD guard), `6b35cad` (5 simplify-yes: + useAuthActionLabels hook, IntegrationStatusPill chipClass dead + branches, DateInput/DateTimeInput wrappers ×14 callsites, + icon-wrapper cleanup ×3, narration-comment strip), `1258b24` + (HIGH H-01 token-name UTC→Berlin), `5a8ad3d` (HIGH H-02 + useViewportWidth setState removal), `977f124` (HIGH H-04 tabs + ring clip), `1f0d9ad` (prettier on danger-zone-section). + Simplify F3 + F4 also resolved senior-dev H-1 + code-review + H-03 transitively. Three HIGH deferred to + `.planning/v1420-backlog.md`: D-CR-H-05 (insights `data?.` + narrowing — large refactor), D-DSGN-H-01 (api-tokens touch + tooltip — needs Popover swap), D-DSGN-H-02 (insights hero + density — folded into v1.4.20 redesign), D-SR-H-3 (Withings/ + MoodLog card chrome dedup — v1.5 Apple Health prep). Strategic + items from product-lead-review pulled into + `.planning/v15-backlog.md`. No `--no-verify` / no + `--no-gpg-sign`. Final verification: pnpm typecheck clean, + pnpm lint 0 errors / 12 baseline warnings (no new), pnpm test + --run 1672/1672, pnpm test:integration 67/67. ## Phase E — Release v1.4.19 diff --git a/.planning/phase-D-code-review-findings.md b/.planning/phase-D-code-review-findings.md index c1198458a..000c550a6 100644 --- a/.planning/phase-D-code-review-findings.md +++ b/.planning/phase-D-code-review-findings.md @@ -1,568 +1,380 @@ -# v1.4.18 Phase-D Code Review Findings - -Reviewer: code-reviewer agent -Run: 2026-05-10 (parallel with 5 other reviewers) -Scope: v1.4.17 → origin/main (22 commits) -DO NOT commit — findings only. - -Diff scope (from `git log --oneline v1.4.17..origin/main`): - -- A1 BD-Zielbereich tile sub-values (commits 23363ca, 15c852f) -- A2 painted-scrollbar fix (commits 3e16074, 874878f) -- A3 chart visual revert + per-chart toggles (commits 008a8fb, 06f18df, b388fc0, 75f340e, d70a9b1, 3d58557, d6e87c3) -- B1 achievements expansion + hidden Easter-eggs (commits e73813a, 19b0844, c41f9a4, 23514b4, b88f2de, 6912d4e, 75c74f1, 178c6dc) - -## CRITICAL - -### C1 — Hidden achievement triggers leak via the API response (and the client bundle) - -- **Severity**: CRITICAL -- **File**: `src/app/api/gamification/achievements/route.ts:611-655` and - `src/lib/gamification/achievements.ts:706-739` (evaluator) + - `src/lib/i18n/context.tsx:15-22` (bundle import) -- **Issue**: The reviewer brief lists this exact failure mode as ship- - blocker: _"the 'Hidden' placeholder must not reveal predicate - conditions to a snooping user"_. The achievements page UI does the - right thing — `` for hidden+locked entries renders - an opaque card with only a generic "Hidden achievement" label and - the help-circle icon. **However, the wire response from - `GET /api/gamification/achievements` ships the full achievement - definition for every entry including hidden+locked**: - - ```json - { - "id": "hidden-night-owl", - "metric": "nightOwlCount", - "category": "hidden", - "titleKey": "achievements.badges.hiddenNightOwl.title", - "descriptionKey": "achievements.badges.hiddenNightOwl.description", - "icon": "Moon", - "format": "count", - "target": 1, - "current": 0, - "points": 25, - "unlocked": false, - "progressPercent": 0, - "completedAt": null, - "isHidden": true - } - ``` - - Three independent leak vectors a "snooping user" can use without any - privileged access: - 1. The metric name itself (`nightOwlCount`, `earlyBirdCount`, - `leapDayCount`, `doctorPdfCount`, `localeFlipCount`) leaks the - trigger semantically. - 2. The `titleKey` / `descriptionKey` strings reference the i18n - bucket whose translated values describe the trigger verbatim - (`"Logged an entry between 02:00 and 04:00 in the morning."`). - 3. `messages/en.json` and `messages/de.json` are statically - `import`-ed in `src/lib/i18n/context.tsx` (lines 15-16) and - therefore bundled into the client JS — a `Cmd-F` for - `"hiddenNightOwl"` in the chunk reveals every trigger and - reward. - Items 1+2 are the ship-blocker the brief explicitly asks for. Item 3 - is a related but separable concern that needs a different fix. - - Critically, the `` opaque-placeholder branch makes - this leakage _less obvious to casual users_ but does nothing to - prevent a determined snooper — Marc, who specifically asked for - hidden Easter-eggs to be a surprise, would discover them in 30 - seconds with the Network tab open. - - Existing test coverage (`src/app/achievements/__tests__/page.test.tsx` - lines 254-270) only asserts that the rendered HTML doesn't contain - the strings — _that test passes_ even though the strings are still - in the JSON response and the bundle. The test is not actually - guarding what the brief asked us to guard. - -- **Recommendation**: Strip identifying fields from hidden+locked - achievement entries server-side before returning them. The wire - shape for a hidden+locked entry should look like: - - ```json - { - "id": "hidden-night-owl", - "category": "hidden", - "isHidden": true, - "unlocked": false, - "completedAt": null - } - ``` - - i.e. drop `metric`, `titleKey`, `descriptionKey`, `icon`, `target`, - `current`, `progressPercent`, `points`. A stable `id` is fine to - expose because the i18n key is namespaced under `hidden*` (the user - can still infer that hidden achievements exist; that's the whole - point) but the trigger details must not be transmitted. - - Concretely, in `src/app/api/gamification/achievements/route.ts` - around line 611 (where `applyDiscoveryFilter` is called), add a - second pass that scrubs hidden+locked entries before serialising: - - ```ts - const sanitized = visibleAchievements.map((a) => { - if (a.isHidden && !a.unlocked) { - return { - id: a.id, - category: "hidden" as const, - isHidden: true, - unlocked: false, - completedAt: null, - // Synthetic placeholders for type-stability so the client - // doesn't need an `?.` everywhere. The real strings only - // ship once the achievement unlocks. - titleKey: "achievements.hiddenCard.title", - descriptionKey: "achievements.hiddenCard.description", - icon: "HelpCircle", - format: "count" as const, - target: 0, - current: 0, - points: 0, - metric: "totalTakenIntakes" as const, // sentinel - progressPercent: 0, - }; - } - return a; - }); - ``` - - And add an explicit test at the API/route level that asserts the - response body does NOT contain `nightOwlCount`, `earlyBirdCount`, - `leapDayCount`, `doctorPdfCount`, `localeFlipCount` while the - achievement is locked, and DOES contain those strings once - unlocked. - - Bundle leak (item 3) is a separate fix: gate hidden i18n strings - behind a server-side translator and return the resolved title / - description in the unlock toast payload only when the unlock - actually occurs (or move them to a server-only translation file - loaded by `getServerTranslator`). Lower-effort interim is a comment - in `messages/*.json` explaining the trade-off and accepting that - any client-bundle leak below the API leak is "acceptable risk for - v1.4.18" — but that's a Marc-call, not a reviewer-call. - -- **Ship-blocker?**: YES. The brief explicitly calls this out as - CRITICAL ("hidden achievement leakage in DOM/network responses"). - The DOM leak is fixed (the card renders correctly); the **network - response leak is not**. This must be fixed before v1.4.18 ships. - -## HIGH - -### H1 — Read-modify-write race when two charts toggle overlays simultaneously - -- **Severity**: HIGH (correctness — concurrent writes lose data) -- **File**: `src/app/api/dashboard/chart-overlay-prefs/route.ts:55-79` - and `src/app/api/dashboard/widgets/route.ts:96-115` -- **Issue**: Both routes do an unwrapped read-modify-write against - `User.dashboardWidgetsJson`: - - ```ts - const row = await prisma.user.findUnique({ - where: { id: user.id }, - select: { dashboardWidgetsJson: true }, - }); - const current = resolveDashboardLayout(row?.dashboardWidgetsJson); - const next: DashboardLayout = { - ...current, - chartOverlayPrefs: { - ...(current.chartOverlayPrefs ?? {}), - [parsed.data.chartKey]: parsed.data.prefs, - }, - }; - await prisma.user.update({ - where: { id: user.id }, - data: { dashboardWidgetsJson: normalized }, - }); - ``` - - If a user opens the dashboard, flips the BP chart's "Target range" - toggle, and 50ms later flips the weight chart's "Trend" toggle, the - two `PUT` calls can interleave: BP-PUT reads the current layout, - weight-PUT reads the _same_ current layout (concurrent), BP-PUT - writes the merged BP prefs, weight-PUT then writes its merged - weight prefs back **on top of the layout it read** — which still - has the old BP prefs — silently dropping the BP toggle. The user - sees the BP toggle revert when the page next refreshes. - - The optimistic update in `useChartOverlayPrefs` (`onMutate`) - partially masks this for the originating client because the local - cache stays correct until the route returns; once the route - returns successful and `onSettled` invalidates the query, the next - fetch reveals the dropped toggle. - - The same race exists for `/api/dashboard/widgets` PUT (already had - this defect in v1.4.16 + B8 comparison baseline) but the new chart- - overlay-prefs route makes it worse because the surface for - per-chart toggles is exactly the kind of thing users will - rapid-fire (5 charts × 3 toggles = 15 settings to flip in a - session). - -- **Recommendation**: Wrap the read-modify-write in a transaction - with `SERIALIZABLE` isolation OR rewrite the merge to use Postgres' - JSON path operators so the SQL itself is the merge. Cheapest fix - is the transaction wrapper: - - ```ts - await prisma.$transaction( - async (tx) => { - const row = await tx.user.findUnique({ - where: { id: user.id }, - select: { dashboardWidgetsJson: true }, - }); - const current = resolveDashboardLayout(row?.dashboardWidgetsJson); - const next = { - ...current, - chartOverlayPrefs: { - ...(current.chartOverlayPrefs ?? {}), - [parsed.data.chartKey]: parsed.data.prefs, - }, - }; - await tx.user.update({ - where: { id: user.id }, - data: { - dashboardWidgetsJson: serializeDashboardLayout( - next, - ) as Prisma.InputJsonValue, - }, - }); - }, - { isolationLevel: "Serializable" }, - ); - ``` - - Add a vitest race-condition test against the existing testcontainer - setup (mirror `tests/integration/rate-limit-race.test.ts`). The - existing v1.4.18 integration test - (`tests/integration/chart-overlay-prefs.test.ts`) only exercises - serial writes and would not catch this defect. - -- **Ship-blocker?**: NO — single-user, single-tab usage will not - trigger this (the optimistic update masks it). But a user with the - dashboard open in two tabs (a common pattern with PWAs) WILL hit - it. Fix in v1.4.18 if Marc wants; safe to defer to v1.4.19 with a - backlog entry. - -### H2 — `useChartOverlayPrefs` subscribes to "bp" prefs even when no chartKey is supplied - -- **Severity**: HIGH (correctness — wrong overlay state when chartKey is omitted) -- **File**: `src/components/charts/health-chart.tsx:321-324` -- **Issue**: - - ```ts - const overlayPrefs = useChartOverlayPrefs(chartKey ?? "bp"); - const showMA = chartKey ? overlayPrefs.prefs.showTrendIndicator : false; - const showTrend = chartKey ? overlayPrefs.prefs.showTrendArrow : false; - const showBands = chartKey ? overlayPrefs.prefs.showTargetRange : false; - ``` - - When `chartKey` is undefined (mini mode / ad-hoc render), the hook - still subscribes to the dashboard-layout query and reads the - user's `bp` chart prefs — which are then ignored by the ternaries. - This is mostly wasteful (an extra fetch + reactive subscription), - but it has two real consequences: - 1. A render thrash when the user updates BP chart prefs from the - dashboard: every mini chart re-renders even though their state - doesn't change. - 2. If a user has no `bp` prefs but has `weight` prefs, the - defaulted-empty `bp` prefs leak through the optimistic-update - cache as "every flag false" — usually equivalent to the - correct default but conceptually wrong. - -- **Recommendation**: Either pass `chartKey` to the hook only when - defined and short-circuit inside the hook when undefined, or move - the hook call inside a wrapper that early-exits before the hook is - called (the latter is a hooks-rule violation though, so prefer the - former): - - ```ts - // In use-chart-overlay-prefs.ts: - export function useChartOverlayPrefs( - chartKey: ChartOverlayKey | undefined, - ): { prefs: ChartOverlayPrefs; setPrefs: ...; isSaving: boolean } { - const queryClient = useQueryClient(); - const { data: layout } = useQuery({ - queryKey: ["dashboard-layout"], - enabled: chartKey !== undefined, // skip when no key - ... - }); - const prefs = useMemo(() => { - if (!chartKey) return DEFAULT_CHART_OVERLAY_PREFS; - return layout?.chartOverlayPrefs?.[chartKey] ?? DEFAULT_CHART_OVERLAY_PREFS; - }, [layout, chartKey]); - ... - } - ``` - -- **Ship-blocker?**: NO — the user-visible behaviour is the clean-line - default (which is correct). Wasted bandwidth + re-render only. - -### H3 — Hidden bug-buddy achievement shares `bugReportCount` with public bugreport-1 — order-of-evaluation matters - -- **Severity**: HIGH (correctness around discovery) -- **File**: `src/lib/gamification/achievements.ts:354-375` (public - `bugreport-1`) and lines 638-647 (hidden `hidden-bug-buddy`) -- **Issue**: Both achievements use `metric: "bugReportCount"`. The - public one fires at target 1; the hidden one fires at target 5. - When the API response is serialised, both entries have - `metric: "bugReportCount"` but different `target` values. A snooper - comparing the two entries discovers that **the hidden achievement - shares the same metric as the public one but with a higher - threshold** — i.e. the trigger is "submit 5 bug reports". - - This is an even-worse leak than the metric-name leak (C1) because - the cross-reference is trivial: open the page, see one public - bugreport badge with `target: 1`, and one hidden badge with - `metric: "bugReportCount"` and `target: 5` — game over for the - surprise. - -- **Recommendation**: Fix subsumed by C1 (don't ship `metric` or - `target` for hidden+locked entries). With C1 fixed, this leak goes - away. - - As a separate consideration: the design choice of reusing a public - metric for a hidden Easter-egg is questionable to begin with — - consider giving the hidden bug-buddy its own metric - (`bugReportCountHidden` or similar), so even server-side audit - trails don't accidentally reveal that "5 bug reports" is the - trigger via shared metric counting. - -- **Ship-blocker?**: NO once C1 ships. Tracked under C1 fix. - -### H4 — `getMoodMetrics` improvement-window slices distinct days, not 7 calendar days - -- **Severity**: HIGH (correctness — diverges from "7-day window" semantics) -- **File**: `src/lib/gamification/expansion-metrics.ts:104-138` -- **Issue**: `moodImprovementHit` slides a 7-distinct-day window - rather than a 7-calendar-day window: - - ```ts - for (let i = 13; i < sortedDays.length; i++) { - const recent = sortedDays.slice(i - 6, i + 1); // 7 distinct days - const prior = sortedDays.slice(i - 13, i - 6); // 7 distinct days - ... - } - ``` - - If a user logs daily for a month, that's fine. But a user who logs - on April 1, 2, 3, 4, 5, 6, 7, then nothing for a month, then - May 1, 2, 3, 4, 5, 6, 7 will have the comparison fire across the - one-month gap — comparing two arbitrarily-spaced 7-entry sets, not - two contiguous 7-day windows. - - The doc-comment claims _"any 7-day window had a mean score at least - 1.0 higher than the preceding 7-day window"_ (line 96) which is - **NOT** what the code does. - - Practical impact: the achievement may unlock too eagerly for users - with sparse logging patterns. Not coercive (Marc's design rule is - satisfied), but the semantics are wrong. - -- **Recommendation**: Either: - - Update the doc-comment to say "compares the last 7 entries to the - 7 preceding entries" and accept the simplification, OR - - Make the window calendar-day-based: build a daily array including - no-data days (or a `null` for missing), require a minimum number - of populated days in each window, then compute the means. - - The first option is cheaper and probably what Marc actually wants - ("user is moving in the right direction"); the second is more - rigorous but adds complexity. - - Either way, add a unit test for the cross-month-gap case to lock - in the chosen semantics. - -- **Ship-blocker?**: NO — the achievement still functions and it's - user-positive (not coercive). Document or fix in a follow-up. - -### H5 — `consistentMonthCount` keeps growing — wasted DB churn for once-fired achievement - -- **Severity**: HIGH (DB wear on long-tenured users) -- **File**: `src/lib/gamification/expansion-metrics.ts:175-182` and - `src/app/api/gamification/achievements/route.ts:601-610` - (persistence flow) -- **Issue**: `consistentMonthCount` returns the _total_ number of - consistent months over the user's entire history. The achievement - unlocks at target 1 (first consistent month), but the metric keeps - incrementing every month thereafter. Each new consistent month - causes: - 1. Recompute on every `/api/gamification/achievements` call - (every 2 minutes via `` polling) - 2. The `metrics.consistentMonthCount` column grows unboundedly - in the response payload (a small int, but conceptually unbounded) - 3. No corresponding state in the UI exposes this count - - Compare to `moodEntryCount`, which does have multi-tier - achievements at 1 / 50 / 200 etc. — so the unbounded growth there - is justified. - -- **Recommendation**: Either: - - Add tiered achievements at consistent-month 1 / 3 / 6 / 12 to - justify the growing count, OR - - Cap the count at the highest threshold (`min(count, 1)`) inside - `getEngagementMetrics()` to make the metric effectively boolean - and stop the growth. - - Same review applies to `moodImprovementHit` (already capped at 1 - by the `break;`), `nightOwlCount` (counted but no tiered targets - beyond 1), `earlyBirdCount`, `leapDayCount`, `doctorPdfCount`, - `localeFlipCount`. Consider capping all single-target hidden - metrics at 1 to limit the wire-payload growth and avoid a future - "did the user trigger this 100 times?" question that the data - doesn't actually answer. - -- **Ship-blocker?**: NO. Pure efficiency / hygiene, no incorrect - user-facing behaviour. - -## MEDIUM / LOW - -### M1 — Earnability flags treat `BLOOD_PRESSURE_DIA`-only users as having no BP data - -- **Severity**: MEDIUM -- **File**: `src/lib/gamification/expansion-metrics.ts:74-83` -- **Issue**: `countMeasurementsByType` counts only `BLOOD_PRESSURE_SYS` - toward `bpCount`. If a user (or a sync source) ever wrote - `BLOOD_PRESSURE_DIA` rows without paired `BLOOD_PRESSURE_SYS` rows, - the BP achievements would not become earnable. The comment at - lines 26-28 of the test file explains the design choice ("avoid - double-counting") but doesn't justify why DIA-only data invalidates - the user from BP achievements. -- **Recommendation**: Either: - - `bpCount = max(sys, dia)` so DIA-only users still earn, OR - - Document the assumption "BP measurements always include a SYS - row" and add a guard that drops DIA-only writes upstream. - No action probably needed for v1.4.18 — Withings + manual entry - always pair them — but worth a backlog note. -- **Ship-blocker?**: NO. - -### M2 — Hidden hint text is revealed in the unlock toast description - -- **Severity**: MEDIUM -- **File**: `src/components/gamification/achievement-unlock-notifier.tsx:120-129` -- **Issue**: When a hidden achievement unlocks, the toast description - is `${t(achievement.titleKey)} — ${t(achievement.descriptionKey)}` - which combines the **real** title and description (e.g. - `"Night owl — Logged an entry between 02:00 and 04:00 in the morning."`). - This is correct _for the toast itself_ (the unlock IS the moment - to reveal the surprise). However, after the toast dismisses, the - notifier persists `seenIdsRef` so the hidden ID is no longer - treated as new. If the user switched their device clock back, or - cleared localStorage, the toast would re-fire — but that's an - acceptable edge case. -- **Recommendation**: No action needed. Behaviour is correct; flagging - here only because the brief asked the reviewer to look for hidden- - hint leakage. The unlock-toast reveal is by design. -- **Ship-blocker?**: NO. - -### M3 — `/admin/api-tokens` mobile spec covers AdminShell but not SettingsShell - -- **Severity**: MEDIUM (parity) -- **File**: `e2e/admin-api-tokens-mobile.spec.ts` -- **Issue**: A2 fixed the painted-scrollbar in BOTH `AdminShell` and - `SettingsShell` (same `no-scrollbar` utility). The Playwright - e2e regression guard at `e2e/admin-api-tokens-mobile.spec.ts` only - asserts the AdminShell path. SettingsShell has unit-level - coverage in `src/components/settings/__tests__/settings-shell.test.tsx` - (14 lines added) which checks the className. If `globals.css` ever - removes the `.no-scrollbar` utility, the unit test still passes - (it asserts the className, not the rendered scrollbar). -- **Recommendation**: Add a 5-line Playwright spec hitting - `/settings/profile` at Pixel-5 viewport asserting no horizontal - scroll. Mirror the AdminShell spec exactly. -- **Ship-blocker?**: NO. - -### M4 — `chart-overlay-prefs` route doesn't annotate the previous values for audit trail - -- **Severity**: LOW -- **File**: `src/app/api/dashboard/chart-overlay-prefs/route.ts:81-92` -- **Issue**: The `annotate()` call at the end captures the new flags - but not the previous ones, so a session log can't reconstruct the - full toggle history. Compare to `applyProfileUpdate` in - `src/lib/auth/profile-update.ts:89-94` which captures `from` / - `to` for the locale change. -- **Recommendation**: If audit forensics matters for chart overlay - prefs (probably not — Marc said this is a UX prefs blob not an - analytical attribute), capture both old and new flag sets in the - annotation. Otherwise drop this finding. -- **Ship-blocker?**: NO. - -### M5 — `health-chart.tsx` no longer renders the personal-baseline ReferenceLine — confirm via grep - -- **Severity**: LOW (verification — the test already enforces this) -- **File**: `src/components/charts/health-chart.tsx`, - `src/components/charts/__tests__/health-chart-overlay-defaults.test.tsx` -- **Issue**: The default-overlay test reads the source file and - asserts no unconditional baseline ReferenceLine. This is a brittle - source-string assertion (`readFileSync` + regex) that will break - if the file is reformatted or split. It's a reasonable - short-term guard, but it's not the same as testing the rendered - output. -- **Recommendation**: Replace with an SSR render assertion: render - `` with default props, assert no element has - `data-slot="chart-personal-baseline-line"` (or whatever the - ReferenceLine's identity ends up being). Mirror the medication - chart's overlay-toggle test pattern. -- **Ship-blocker?**: NO. - -### L1 — `parseDayKey` redundantly sets UTC hours after constructor - -- **Severity**: LOW -- **File**: `src/lib/gamification/expansion-metrics.ts:230-232` (used - inside `getEngagementMetrics`) -- **Issue**: `parseDayKey` constructs the date with - `new Date(Date.UTC(year, month - 1, day, 12, 0, 0))` (already - noon UTC). The next line `cursor.setUTCHours(12, 0, 0, 0)` is a - no-op. -- **Recommendation**: Drop the redundant line. -- **Ship-blocker?**: NO. - -### L2 — `de.json` / `en.json` symmetry not asserted for the v1.4.18 expansion strings - -- **Severity**: LOW -- **File**: `messages/de.json`, `messages/en.json`, - `src/lib/__tests__/i18n*.test.ts` (per CLAUDE.md the symmetry - guard exists) -- **Issue**: Couldn't verify in this review whether the existing - i18n parity test enumerates the 22 new keys (mood × 3 + measurement - counts × 7 + engagement × 4 + hidden × 6 + hiddenCard placeholder - - hiddenUnlockToast + 2 category labels). The expansion adds ~30+ - new keys; if the parity test is data-driven (read both files, - assert keys match), this is automatic. If it's hand-rolled, some - may have slipped. -- **Recommendation**: Quick sanity run of `pnpm test -src/lib/__tests__/i18n*` to confirm 0 failures. Already in CI most - likely. -- **Ship-blocker?**: NO. - -## SUMMARY - -- 1 CRITICAL (C1 — hidden achievement trigger leakage via API response) -- 5 HIGH (H1 race, H2 hook subscription, H3 bug-buddy metric leak [subsumed by C1], H4 mood-window semantics, H5 unbounded count growth) -- 7 MED/LOW (M1-M5, L1-L2) - -Ship-blocker count: **1** (C1). - -Recommended action before v1.4.18 release: scrub `metric` / -`titleKey` / `descriptionKey` / `target` / `current` / -`progressPercent` / `points` from hidden+locked entries in the API -response body. Add a route-level test that asserts hidden trigger -strings are not present in the response while the achievement is -locked, and ARE present once unlocked. Item H3 is auto-resolved by -the C1 fix. - -The H1 race condition is real but masked in single-user usage; safe -to defer with a backlog entry. H2-H5 are correctness / hygiene -issues; defer to v1.4.19 absent regression reports. M/L items are -follow-on hygiene work. - -Strong work overall — A1 BD-tile fix is clean and well-tested -(`computeBpInTargetWindows` reuses the v1.4.16 ceiling predicate -correctly with explicit null-handling for sparse data), A2 painted- -scrollbar fix correctly identifies that the same defect lives in -both shells and uses a single utility class to fix both, A3 chart -revert is thorough (all three reversions land + the toggle UI -persists per-chart cleanly), and B1 achievements expansion has -strong unit + integration test coverage for the visible cases. The -discovery filter design is correct. The remaining work is the wire- -shape hardening for the hidden category and the race-condition -guard for concurrent overlay toggles. +# v1.4.19 Phase-D Code Review Findings + +Reviewer: code-reviewer agent (parallel with 5 other reviewers) +Run: 2026-05-10 +Scope: v1.4.18 → origin/main (47 commits, 91 files, +6623/-1739) +Verified: `pnpm test --run` 1669/1669 green at HEAD; spot-checked source diffs against +deployed contracts. + +--- + +## CRITICAL (ship-blockers — must fix before tag) + +### C-01 — F-13 BP visual disambiguation badge never renders (broken just-shipped feature) + +- File: `/Users/marc/Projects/HealthLog/src/components/measurements/measurement-list.tsx:478,483` +- Commit: `876e074` ("fix(measurements,settings): visual disambiguation polish") +- Severity: CRITICAL · Ship-blocker: YES + +Issue: The mobile-list BP-type badge introduced for F-13 checks the WRONG enum +values. Code does: + + {(m.type === "BP_SYS" || m.type === "BP_DIA") && ( + + {m.type === "BP_SYS" ? t("measurements.typeBpSys") : t("measurements.typeBpDia")} + + )} + +But the canonical `MeasurementType` enum in `prisma/schema.prisma:189-190` and the +sibling `measurement-list-meta.ts:32-33` use `BLOOD_PRESSURE_SYS` / `BLOOD_PRESSURE_DIA`. +`m.type` is whatever the API returns — i.e. always the long form. The condition is +permanently false; the badge never paints. The user-visible regression Marc +reported (a 117 mmHg value with no Sys / Dia label on Pixel-5) is therefore NOT +fixed in v1.4.19 despite the commit message claiming so. + +The desktop table on the same component uses `TYPE_LABEL_KEYS[m.type]` +(`measurement-list-meta.ts`, correct mapping) and is unaffected. Only the +mobile card branch is broken. + +Recommendation: + + {(m.type === "BLOOD_PRESSURE_SYS" || m.type === "BLOOD_PRESSURE_DIA") && ( + + {m.type === "BLOOD_PRESSURE_SYS" + ? t("measurements.typeBpSys") + : t("measurements.typeBpDia")} + + )} + +Add a vitest-level guard so a future enum rename can't re-break this: + + expect(BLOOD_PRESSURE_SYS).toBe(MeasurementType.BLOOD_PRESSURE_SYS) + +Or, better, route through the same `TYPE_LABEL_KEYS[m.type]` table the desktop +row uses; then the entire enum is single-sourced. + +--- + +## HIGH (should fix; ship at reviewer's discretion) + +### H-01 — `formatTokenName` UTC clock breaks Berlin-time convention + +- File: `/Users/marc/Projects/HealthLog/src/components/admin/api-token-overview-section.tsx:69-82` +- Commit: `713b494` (F-18 token-name humanization) +- Severity: HIGH · Ship-blocker: NO + +The new `formatTokenName` helper extracts the trailing ISO suffix and renders +it as `dd.MM.yyyy HH:mm` using `getUTCDate / getUTCMonth / getUTCHours / +getUTCMinutes`. The rest of the app renders timestamps in `Europe/Berlin` +(per `CLAUDE.md` "Timezone: Europe/Berlin for display" + `format-locale.ts:18 +DISPLAY_TIMEZONE = "Europe/Berlin"`). For a token issued at +`2026-05-05T19:46:20.603Z` (UTC), Marc reading the admin viewer in Berlin +(UTC+2) would expect `21:46` but sees `19:46`. The phase-A8 spec for F-18 +explicitly said "locale-aware" — UTC is neither. + +Recommendation: replace the manual UTC string-build with the existing +`formatDateTime(d)` helper or `Intl.DateTimeFormat("de-DE", { timeZone: +"Europe/Berlin", … })`. Single line change. + +### H-02 — `useViewportWidth` hook calls `setState` inside `useEffect` post-mount sync + +- File: `/Users/marc/Projects/HealthLog/src/hooks/use-viewport-width.ts:21-32` +- Severity: HIGH · Ship-blocker: NO + +Project lints with `react-hooks/set-state-in-effect` (CLAUDE.md). The hook does: + + useEffect(() => { + const handler = () => setWidth(getViewportWidth()); + window.addEventListener(...); + handler(); // ← unconditional setState on mount + return () => { ... }; + }, []); + +The lazy initializer (`useState(() => getViewportWidth())`) already returns +the correct browser value when SSR + hydration produce a client mount, so +the `handler()` call right before the cleanup return is redundant under +normal conditions. It can also race React's commit phase on first paint — +in StrictMode the effect fires twice, both times calling `setWidth` with +the same value (no-op but allocates). + +The comment claims "Sync once after mount in case the SSR default differs" +— but the lazy initializer already runs in the browser on hydration, so +the first `useState()` invocation gets the live value. The post-mount +`handler()` is dead code unless `getViewportWidth()` returns a different +value between the lazy initializer and the effect run, which can only +happen if `window.innerWidth` mutates between them (a non-issue). + +Recommendation: drop the unconditional `handler()` call; rely on resize + +orientationchange listeners alone. + +### H-03 — Feedback inbox + danger-zone cards now render an orphan icon + +- Files: + - `/Users/marc/Projects/HealthLog/src/components/admin/feedback-inbox-section.tsx:81-83` + - `/Users/marc/Projects/HealthLog/src/components/admin/danger-zone-section.tsx:73-78` +- Commit: `70ebe32` (F-08 drop duplicate card title on single-card pages) +- Severity: HIGH · Ship-blocker: NO + +The F-08 dedup deleted the card-level CardTitle but kept the leading icon +in a `
` with no companion text. That row is now a single 20×20 px +icon hovering above the body content — visually it reads as either a +broken render or a stray decoration. The intent (the page-level header +already provides title + description) is correct; the implementation +leaves dead structural noise behind. + +Recommendation: drop the wrapper div entirely on both files since the +icon is now decorative-only. If a card-level marker is desired, fold the +icon into the first content row (e.g. ` Open` ahead +of the tab strip) or use a left-border accent like the danger-zone card +already has via `border-destructive/30`. + +### H-04 — Tabs `overflow-y-hidden` clips focus-visible ring on triggers + +- File: `/Users/marc/Projects/HealthLog/src/components/ui/tabs.tsx:48,86` +- Commit: `088832a` (A7 admin feedback tab strip) +- Severity: HIGH · Ship-blocker: NO + +The A7 fix locks `overflow-y-hidden` on `TabsList` to suppress a 1-2 px +painted vertical scrollbar Marc reported. The `TabsTrigger` carries +`focus-visible:ring-[3px]` — a Tailwind box-shadow ring drawn 3 px outside +the trigger's box. The parent `TabsList` has `p-[3px]` padding, so the +3 px ring lands exactly at the parent's content edge; with +`overflow-y-hidden`, the bottom of the ring is clipped at the parent's +content box. Keyboard users tabbing through the strip will see a +partially-clipped focus ring — minor, but a WCAG 2.4.7 (focus visible) +nit. + +Also affects the `data-[variant=line]` `after:bottom-[-5px]` active-bar +indicator — that 5 px element extends 5 px below the trigger, which is +2 px past the parent's padding and would be clipped. The `line` variant +isn't currently used anywhere in the codebase (`grep -rn 'variant="line"'` +returns 0), so this is theoretical until someone adopts it. Worth flagging +in the comment so a future consumer doesn't add the variant and lose its +active-state cue silently. + +Recommendation: bump `TabsList` padding from `p-[3px]` → `p-1` (4 px) so +the 3 px focus ring fits inside, OR replace `overflow-y-hidden` with a +`scrollbar-track-transparent` + `scrollbar-h-0` variant that hides the +painted bar without clipping the y-axis. + +### H-05 — Insights page reads `data?.moodSummary` post early-return where `data` is non-null + +- File: `/Users/marc/Projects/HealthLog/src/app/insights/page.tsx:756,758,1036,1046,1064-1065,1235,1245,1248-1252,1263-1273` +- Severity: HIGH · Ship-blocker: NO (but maintenance debt) + +After `if (!data) return ` at line 820, `data` is provably +non-null for the entire JSX tree below. Yet several reads still use +`data?.moodSummary`, `data?.moodBpScatterData`, etc. — sometimes mixed +with non-optional `data.medications` reads on the same line. This is a +TypeScript narrowing artefact from the v1.4.19 A3 refactor that pruned +the tile strip. Mostly harmless but inconsistent and obscures the +contract: a reader cannot tell from line 758 +(`(data?.moodSummary?.count ?? 0) > 0`) that the optional chain is +unreachable. Lines 720-790 (the `*SectionStatus` precomputes) DO need +`data?.` because they sit BEFORE the early return — the rest do not. + +Recommendation: hoist the `if (!data) return` above the SectionStatus +calls so the entire body has narrowed `data: ComprehensiveData`, then +strip the redundant `?` operators. Reduces cognitive load and keeps a +future migration to `assertExists()` patterns honest. + +--- + +## MED (nice to fix; not ship-blockers) + +### M-01 — `/insights` queries don't share a stale window with the dashboard + +- File: `/Users/marc/Projects/HealthLog/src/app/insights/page.tsx:553-643` +- Severity: MED + +The 7 per-section status queries (`general-status`, `blood-pressure-status`, +`weight-status`, `pulse-status`, `bmi-status`, `mood-status`, +`medication-compliance-status`) all carry `staleTime: 60 * 1000`, but the +top-level `comprehensive` and `analytics` queries have no `staleTime` +override. On a navigation back to `/insights` after 30 s, the two +top-level queries refetch but the section caches don't, leaving the page +in a state where the headline numbers update but the AI prose stays on +the old data. Cosmetic — the page reconciles within 60 s — but worth +unifying. + +### M-02 — `chartKey` typed as optional on `` but UI surfaces drop the chartKey for /insights charts + +- File: `/Users/marc/Projects/HealthLog/src/app/insights/page.tsx:948-1480` +- Severity: MED + +Every `` on `/insights` (BP, Weight, Pulse, BMI) is mounted +without a `chartKey`. The HealthChart contract treats missing chartKey as +"don't paint overlay controls + use clean-line defaults" — which is the +intentional design — but the cog dropdown is therefore invisible on +`/insights` while it IS visible on the dashboard. Marc may rediscover +this as an inconsistency. Either: + - document it (the per-chart prefs are dashboard-only by design), OR + - mount overlay controls on `/insights` too (would require new + chart-key entries in the dashboard layout). + +### M-03 — `formatTokenName` regex doesn't cover non-ISO suffixes + +- File: `/Users/marc/Projects/HealthLog/src/components/admin/api-token-overview-section.tsx:66-67` +- Severity: MED + +The regex `^(.+?)\s+(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z)(.*)$` +requires the trailing `Z` (UTC). If a future client emits offset-based +ISO (`+02:00`) the regex misses and the row falls back to the raw label, +which is the v1.4.18 dev-ugly state F-18 set out to fix. Cheap fix: +broaden to `(?:Z|[+-]\d{2}:?\d{2})$`. + +### M-04 — Tabs comment claims overflow-y-hidden is from A7 but that fix is from `088832a` + +- File: `/Users/marc/Projects/HealthLog/src/components/ui/tabs.tsx:38-46` +- Severity: MED (doc nit) + +Comment is accurate for the SOURCE of the fix but mis-cites the rationale — +"Combined with the fixed `h-9` strip and badge children that ride ~1 px +taller than the strip on some glyph stacks, a tiny painted vertical +scrollbar appeared on the right edge of short strips". The actual root +cause is the `` child inside each `TabsTrigger` (line 99 of +`feedback-inbox-section.tsx`), which is a `text-xs` + `py-0.5` + `border` +pill — its computed height + `align-baseline` does push the line-box past +the parent's 36 px content area. Worth explicitly mentioning the badge +in the comment so a future "let me delete this className" pass keeps the +context. + +### M-05 — `STATUS_CATEGORY_KEY` map is open to drift from server + +- File: `/Users/marc/Projects/HealthLog/src/app/targets/page.tsx:115-166` +- Severity: MED + +The static map duplicates every category string the server emits from +`lib/analytics/classifications.ts` + `pulse-targets.ts` + +`targets/route.ts`. The fallback (line 174 — `if (!key) return category`) +silently surfaces the English category if the server adds a new +classification. Worth adding a vitest case that diffs the union of the +server's emitted strings against the map's keyset. + +### M-06 — `analytics/route.ts` fetches ALL paired BP rows for the all-time aggregate + +- File: `/Users/marc/Projects/HealthLog/src/app/api/analytics/route.ts:71-86` +- Severity: MED (perf) + +The v1.4.19 A1 fix removes the 30-day filter at the DB layer to compute +a true all-time average. For Marc (572 rows since 2022) this is two +small queries — negligible. For a 5-year power user with 5 daily +readings it'd be ~9 000 rows × 2; still under 100 ms in Postgres but +worth a Wide-Event annotation (`bpSysCount`, `bpDiaCount`) so a future +slow-query review can attribute the latency. The route already calls +`annotate()` once for `analytics.get` — just extend the same call. + +### M-07 — `chooseTickInterval` returns 0 for `pointCount <= target` but Recharts may still paint dense ticks + +- File: `/Users/marc/Projects/HealthLog/src/lib/charts/x-axis-density.ts:80-87` +- Severity: MED + +Helper returns 0 ("render every tick") when the dataset is small. Combined +with `interval={0}` and `preserveStartEnd` (Recharts default), Recharts +may still draw all 6 ticks for a 6-point dataset, which is fine. But on +viewport widths between 360-480 px (target=6) and a dataset of exactly 6 +points, the labels can still collide because Recharts measures each +label against the available width and doesn't drop overlapping labels +unless `interval="preserveStartEnd"` is replaced with +`interval="preserveStartEndAndCollide"` (Recharts 3+) — the legacy +`preserveStartEnd` doesn't drop. Worth pinning the exact Recharts version +behaviour with a smoke e2e on the 393-px viewport. + +--- + +## LOW (cosmetic / safe to defer) + +### L-01 — Insights page comments duplicate a `// ── Section Separator ────` line + +- File: `/Users/marc/Projects/HealthLog/src/components/insights/insight-advisor-card.tsx:293-295` + +The duplicate `// ── Section Separator ────────` comment (lines 293 + 295) +landed in an earlier marathon and isn't from v1.4.19, but the file was +not touched by this milestone. Harmless. + +### L-02 — `` `state="error"` ignores its `chipClass` (`""`) since `chipClass` is only set for the "connected" branch in the switch + +- File: `/Users/marc/Projects/HealthLog/src/components/settings/integration-status-pill.tsx:88-97,121-125` + +For `state==="error"` and `state==="disconnected"` the `chipClass` is set +to `""`, then the `cn(...)` call drops it because the `state === "connected"` +gate at line 123 short-circuits to `false`. The component falls back to +the Badge `variant` ("destructive" / "outline") which is correct, but the +code reads like the chipClass might apply for non-connected states too. +Pure-code clarity nit; behaviour is correct. + +### L-03 — `e2e/charts-mobile.spec.ts` thresholds are tied to `getViewportWidth` desktop default of 1280 + +- File: `/Users/marc/Projects/HealthLog/e2e/charts-mobile.spec.ts` + +The test pins `≤ 7 visible ticks per axis` for mobile. If a future +viewport-width bucket boundary moves (e.g. iPad mini becomes its own +bucket), the assertion may flake. Consider parameterizing via the +`VIEWPORT_BUCKETS` constant the helper exports. + +### L-04 — `formatTokenName` regex's `(?:\.\d+)?Z` doesn't permit milliseconds without a decimal + +- File: `/Users/marc/Projects/HealthLog/src/components/admin/api-token-overview-section.tsx:66` + +`Date.now().toISOString()` always emits milliseconds (`.603Z`), so the +regex matches in production. A token issued without milliseconds +(`2026-05-05T19:46:20Z` from a different code path) would still match +because `(?:\.\d+)?` is optional. Fine. + +### L-05 — `targets-i18n.test.tsx` and `targets-spacing.test.tsx` mock the same query key but only one of them — a future test that runs both serially might leak `data` across them + +- Files: `/Users/marc/Projects/HealthLog/src/app/__tests__/targets-i18n.test.tsx`, `/Users/marc/Projects/HealthLog/src/app/__tests__/targets-spacing.test.tsx` + +vitest module isolation handles this today; cosmetic. + +--- + +## Summary + +- 1 CRITICAL (F-13 mobile BP badge enum mismatch — feature shipped broken) +- 5 HIGH (admin token UTC, useViewportWidth setState, orphan icon, tabs ring clip, insights data narrowing) +- 7 MED +- 5 LOW + +Plan-alignment: A1, A2, A3, A4, A5, A6, A7, B Wave (6 CRIT + 21 HIGH F-#) all +match the planning intent. The single broken implementation is the v1.4.19 F-13 +visual-disambiguation badge — code wrote the wrong enum literal but no test +caught it because no test renders the mobile card list with a `BLOOD_PRESSURE_*` +fixture. Test gap to backfill once C-01 is patched. + +PROMPT_VERSION ratchet 4.16.1 → 4.19.0 looks good. The relaxed assertion +`/4\.\d+\.\d+/` in `medical-reference-prompt.test.ts` is the right call — +fully-pinned versioning would require a test edit on every prompt revision, +defeating the version's purpose as a deploy-attribution tag. + +i18n parity: every new key in `messages/en.json` has a matching `messages/de.json` +entry; `i18n-locale-integrity.test.ts` and `achievements-no-insults.test.ts` +guard against future copy-paste regressions. + +A2 mobile chart fix is solid: helper is pure + tested (13 cases); +`useViewportWidth` is SSR-safe; `chooseTickInterval` math is correct; the +header layout `flex-col → sm:flex-row` is the right responsive pattern. +The H-02 `setState`-in-effect concern is the only blemish. + +A3 raw-token-leak fix (STRIP_TOKEN_REGEX permissive, PARSE_TOKEN_REGEX strict) +is the cleanest possible split; trailing-junk apostrophe still cleaves +correctly; tests pin the contract. + +A5 IntegrationStatusPill is reusable and locale-aware; ready for the v1.5 +Apple Health integration card without changes. + +A7 truncate-with-tooltip on api-tokens is the canonical fix for that whole +family of width-overflow bugs; the `table-fixed` + colgroup widths give the +desktop branch a hard upper bound. Solid. + +Wave-B 27 fixes — every commit I spot-checked landed cleanly with adequate +test coverage, save F-13 (C-01) and F-18 (H-01). + +No security concerns surfaced in this pass — leave that to the security-review +agent. No regressions found in the v1.4.18 surfaces (admin shell, achievements, +analytics aggregator). + +Recommendation: patch C-01 (1-line enum fix + a test) before tagging v1.4.19. +H-01 / H-02 / H-03 / H-04 are reasonable to fold into the same release if a +re-spin is cheap; otherwise they queue for v1.4.20 alongside the Insights +redesign. diff --git a/.planning/phase-D-design-findings.md b/.planning/phase-D-design-findings.md index 26a0ed2f1..5cdd0bf89 100644 --- a/.planning/phase-D-design-findings.md +++ b/.planning/phase-D-design-findings.md @@ -1,445 +1,335 @@ -# Phase D — DESIGN/UX review (v1.4.18) +# Phase D — Design / UX review (v1.4.19) -Reviewer: design / UX (parallel run with 5 other reviewers) -Mode: source audit (production was on v1.4.17 at audit time, v1.4.18 -not yet deployed; `/api/version` returned 1.4.17 at probe) -Branch state: origin/main pulled, 7 untracked stale files left in -place (4 v1.4.16-era export route dirs + 3 v1.4.16 phase-E reports); -none touch the surfaces under review. +Marathon: v1.4.19 Wave-D +Reviewer: design / UX agent (parallel with code, security, senior-dev, +simplify, product-lead) +Date: 2026-05-10 +Method: source-code review against the local working tree at +`origin/main` (`14eff96`); production probe deferred — `/api/version` +still reports `1.4.18`, the v1.4.19 image hasn't deployed yet, so +Playwright probes against prod would not exercise the changes under +review. Findings keyed to file + line numbers in the working tree. -Scope of audit: +## Scope -- `/dashboard` charts after v1.4.18 A3 chart revert + per-chart toggle -- `/insights` (no regression check on RecommendationCard) -- `/admin/api-tokens` + other admin/settings sections (no-scrollbar - utility from A2) -- `/achievements` (B1 expansion + hidden cards) -- BD-tile 7T/30T sub-values (A1) -- A11y, contrast, focus, mobile-friendliness, EN/DE i18n parity +Pages audited: +- `/dashboard` (`src/app/page.tsx`) +- `/insights` (`src/app/insights/page.tsx`, + `src/components/insights/insights-page-hero.tsx`) +- Settings shell — `/settings/{account,profile,dashboard,ai, + notifications,integrations,export}` + (`src/components/settings/*.tsx`) +- `/admin/feedback`, `/admin/api-tokens` + (`src/components/admin/{feedback-inbox-section, + api-token-overview-section}.tsx`) +- Zielwerte (`src/app/targets/page.tsx`) +- Charts (`src/components/charts/{health,mood, + medication-compliance}-chart.tsx`, + `src/components/charts/chart-overlay-controls.tsx`) +- Comparison toggle (`src/components/comparison/compare-toggle.tsx`) -## What's clean (no findings) +## Cross-cutting checks (all passed) -- **Chart revert is solid.** No `linearGradient` defs, no `chart-gradient` - module (deleted), no emoji glyphs at mood data points (`MOOD_EMOJI` - map removed at `mood-chart.tsx:471` with explanatory comment), no - always-on personal-baseline reference line — the baseline is now - gated behind the `showTrend` toggle on both `HealthChart` and - `MoodChart`. Tests pin all three reverts (`health-chart-polish.test.tsx:146`, - `mood-chart-polish.test.tsx:66/81`, `medication-chart-polish.test.tsx:51`). - v1.4.16's smooth interpolation + rich tooltip + animation-on-render - are preserved as the brief required. -- **Default overlay state is OFF for all three toggles** — - `DEFAULT_CHART_OVERLAY_PREFS = { showTrendIndicator: false, -showTrendArrow: false, showTargetRange: false }` at - `chart-overlay-controls.tsx:165`. Pinned by - `health-chart-overlay-defaults.test.tsx`. -- **EN+DE i18n parity for new strings.** `chart.overlay.controls.{title, -trendIndicator,trendArrow,targetRange,tooltip.openSettings}` exist - in both `messages/en.json:652-664` and `messages/de.json:652-664`. - `achievements.{hiddenCard,hiddenUnlockToast,categories.hidden}` exist - in both at the same offsets. -- **Hidden-achievement DOM redaction works.** Locked hidden cards - render via the early-return path at `achievements/page.tsx:146-174` - with no `titleKey` / `descriptionKey` / `metric` / `target` / `current` - in the rendered output. Test pins: - `achievements/__tests__/page.test.tsx:252-262` asserts neither - "Night owl" nor "Logged an entry between 02:00 and 04:00" nor - "nightOwlCount" appears in the DOM. Once unlocked, real strings - appear (`page.test.tsx:264-269`). -- **No-scrollbar utility scope is appropriate.** Used only on the - intended `