From 6aac2070b478f0194ad4e805b8998f3f8b677fe2 Mon Sep 17 00:00:00 2001 From: Antiheld86 <4201585+Antiheld86@users.noreply.github.com> Date: Thu, 3 Sep 2026 07:25:58 +0000 Subject: [PATCH] Address review: label key, gate every feed, retire the dead switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five points from the review, in the order they were raised. 1. The RMSSD marker did not survive. The tile label paints truncated, uppercase and on one line, so an appended `· RMSSD` was the first thing dropped in the longer locales — the part a reader needs to tell a 40 ms RMSSD night from a collapsed SDNN one. `measurements.typeHrvRmssd` already exists in all seven bundles, so each series now carries its own key. `pickHrvSummary` returns that key beside the series it chose, which keeps one mapping from series to label rather than a conditional at the call site. 2. The module gate bit on one path only. `gateSummariesByModules` was private to the snapshot builder, and with the snapshot flag off the tiles are fed by `/api/analytics`, which filtered nothing — so recovery-off plus RMSSD-only still painted the tile, on the exact path the helper's docblock promised it could not. The filter moves next to the map it reads, and both analytics slices apply it. Gated after the cache read, so the cached cell stays module-independent and toggling a module takes effect on the next request instead of waiting out the TTL. 3. The same class one step on: recovery off plus RMSSD only left a switch that was on, a tile that could never paint, and no explanation. The repo already calls that a dead switch and hides the row; HRV is only dead sometimes, so the answer needs one fact the Settings screen does not have. The server resolves it — `unavailableWidgetIds` on the widgets payload — and Settings drops those rows. An account with SDNN keeps the toggle: its tile still works with recovery off, because SDNN is a vital no module owns. The SDNN probe is an index-only existence check that runs only when recovery is off, outside the layout cache so the row goes the moment the module does. 4. The brittle test sliced from the helper to the end of the file, so any function added below it would have gone red pointing at HRV. Bounded to the function's own body, with a guard on the bound itself. The three wiring assertions matched formatted source text — they would have passed just as well if the tile never rendered — and are replaced by a test that renders the tile through a real TrendCard and asserts the reading is on screen. One negative source check is kept, naming the expression the bug lived on rather than the formatting of its replacement. 5. Rebased onto v1.38.6; the changelog entry sits under [Unreleased]. Not verified here: `pnpm db:generate` cannot run in this container (the Prisma engine download is refused), so snapshot.test.ts, the widgets route test and the analytics route test could not load, and typecheck is incomplete. Lint, format and the HRV suite (17 tests) pass. --- CHANGELOG.md | 18 +- .../dashboard-hrv-rmssd-fallback.test.ts | 174 ---------- .../dashboard-hrv-rmssd-fallback.test.tsx | 323 ++++++++++++++++++ src/app/api/analytics/route.ts | 41 ++- src/app/api/dashboard/widgets/route.ts | 33 +- src/app/page-client.tsx | 30 +- src/components/dashboard/dashboard-gates.ts | 33 +- .../settings/dashboard-layout-section.tsx | 15 +- src/lib/dashboard-layout.ts | 7 + src/lib/dashboard/snapshot.ts | 46 +-- src/lib/dashboard/widget-modules.ts | 66 ++++ 11 files changed, 551 insertions(+), 235 deletions(-) delete mode 100644 src/app/__tests__/dashboard-hrv-rmssd-fallback.test.ts create mode 100644 src/app/__tests__/dashboard-hrv-rmssd-fallback.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index 62f88c460..b3d7d2792 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,13 +14,17 @@ reports it that way. Every other place HRV appears already accepted both; the dashboard tile looked only for SDNN, so an account that had only RMSSD had nothing for the tile to show. It now uses whichever series you actually - have, and says "RMSSD" beside the name when that is the one on screen, so a - reading of 40 is not mistaken for a collapsed SDNN figure. An account with - both keeps seeing SDNN, and the two are never blended — they are different - measures that happen to share a unit. Turning the Recovery module off still - takes the RMSSD reading off the dashboard, which it should have done all - along: that series belongs to Recovery everywhere else in the app, and the - dashboard was the one place not checking. + have, and the tile is titled "HRV (RMSSD)" when that is the one on screen, + so a reading of 40 is not mistaken for a collapsed SDNN figure. An account + with both keeps seeing SDNN, and the two are never blended — they are + different measures that happen to share a unit. +- Turning the Recovery module off now takes RMSSD off the dashboard on every + path, not just the default one. That series belongs to Recovery everywhere + else in the app, and the dashboard snapshot already dropped it — but the + analytics endpoint that feeds the same tiles when the snapshot is switched + off filtered nothing at all, so a disabled module still reached the client + there. Both feeds now strip the same set, from the same map, so the gate no + longer depends on which path served the page. ## [1.38.6] — 2026-09-03 diff --git a/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.ts b/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.ts deleted file mode 100644 index 99b2d519b..000000000 --- a/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.ts +++ /dev/null @@ -1,174 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { readFileSync } from "node:fs"; -import { join } from "node:path"; - -import { pickHrvSummary } from "@/components/dashboard/dashboard-gates"; -import { - SUMMARY_TYPE_MODULE, - disabledSummaryTypes, -} from "@/lib/dashboard/widget-modules"; -import { moduleForMeasurementType } from "@/lib/modules/measurement-scope"; -import type { DataSummary } from "@/lib/analytics/trends"; - -/** - * The dashboard HRV tile must accept either HRV type. - * - * HRV is stored under two measurement types that are not the same measure: - * SDNN (`HEART_RATE_VARIABILITY`, Apple / Fitbit / Google Health) and nightly - * RMSSD (`HRV_RMSSD`, Oura / Polar / WHOOP, and a bridge pushing a Garmin-style - * feed). Every other HRV surface already unions them — `/insights/hrv` via - * `fallbackMeasurementType`, the Coach via `COACH_SOURCE_MEASUREMENT_TYPES.hrv`, - * the MCP reads via `withHrvFallback` — and `sub-page-metric.ts` states that - * listing both is what keeps "the tab, pill, and dashboard tile lit whichever - * source the user has". - * - * The tile did not. It read `summaries.HEART_RATE_VARIABILITY` alone, so an - * RMSSD-only account got a Settings → Dashboard toggle that turned on and - * rendered nothing, with no message, while `/insights/hrv` charted the same - * readings. That asymmetry is what this file pins. - * - * Mutation check: return the fallback unconditionally and the "SDNN wins" - * cases go red; drop the fallback arm and the RMSSD cases go red; return the - * fallback's TYPE while returning the primary's summary and the freshness - * case goes red. - */ -function summary(count: number, latest: number): DataSummary { - return { - count, - latest, - min: latest, - max: latest, - mean: latest, - median: latest, - avg7: latest, - avg30: latest, - slope30: 0, - } as unknown as DataSummary; -} - -describe("pickHrvSummary", () => { - it("takes SDNN when the account has it", () => { - const picked = pickHrvSummary({ - HEART_RATE_VARIABILITY: summary(12, 48), - }); - expect(picked.type).toBe("HEART_RATE_VARIABILITY"); - expect(picked.summary?.latest).toBe(48); - }); - - it("falls back to RMSSD for a ring / strap account", () => { - // The regression itself: pre-fix this returned undefined, `hasHrv` was - // false, and the tile never rendered however the widget was toggled. - const picked = pickHrvSummary({ HRV_RMSSD: summary(30, 42) }); - expect(picked.type).toBe("HRV_RMSSD"); - expect(picked.summary?.latest).toBe(42); - }); - - it("prefers SDNN when the account carries both series", () => { - // The case a naive `sdnn ?? rmssd` written the wrong way round breaks. - // Both are stored in ms but they measure different things, so the tile - // shows one series, never a blend, and SDNN is the primary. - const picked = pickHrvSummary({ - HEART_RATE_VARIABILITY: summary(9, 55), - HRV_RMSSD: summary(30, 42), - }); - expect(picked.type).toBe("HEART_RATE_VARIABILITY"); - expect(picked.summary?.latest).toBe(55); - }); - - it("treats a present-but-empty SDNN summary as absent", () => { - // The slice can carry a zero-count summary for a type the account has no - // rows for. Keying off presence rather than `count` would pin the tile to - // an empty SDNN series and re-break the ring user. - const picked = pickHrvSummary({ - HEART_RATE_VARIABILITY: summary(0, 0), - HRV_RMSSD: summary(30, 42), - }); - expect(picked.type).toBe("HRV_RMSSD"); - expect(picked.summary?.count).toBe(30); - }); - - it("reports no series, on the primary type, when the account has neither", () => { - // The tile is hidden either way; reporting the fallback type here would - // make an account with no HRV at all look like a ring user to the label - // and the freshness lookup. - // Annotated: inferred from the literals alone, `{}` widens to - // `{ HRV_RMSSD?: undefined }`, which the index signature rejects. - const inputs: (Record | undefined)[] = [ - undefined, - {}, - { HRV_RMSSD: summary(0, 0) }, - ]; - for (const input of inputs) { - const picked = pickHrvSummary(input); - expect((picked.summary?.count ?? 0) > 0).toBe(false); - expect(picked.type).toBe("HEART_RATE_VARIABILITY"); - } - }); - - it("does not gate on the recovery module itself", () => { - // The gate belongs server-side (see the sibling test below), so a second - // one here would be a competing source of truth. Scoped to the function - // body: the docblock above it legitimately explains where gating lives. - const src = readFileSync( - join(process.cwd(), "src/components/dashboard/dashboard-gates.ts"), - "utf8", - ); - const body = src.slice(src.indexOf("export function pickHrvSummary(")); - expect(body).not.toMatch(/modules|recovery|isModuleEnabled/i); - }); -}); - -describe("the RMSSD fallback cannot leak recovery-module data", () => { - it("drops HRV_RMSSD from the snapshot summaries when recovery is off", () => { - // The half of this fix that is easy to miss. `HRV_RMSSD` is recovery-owned - // in `METRIC_STATUS_MODULE_OWNERS`, which governs the metric-status and - // MCP reads — but the dashboard snapshot gates off `SUMMARY_TYPE_MODULE`, - // a DIFFERENT map, and that one did not list it. Inert while the tile read - // SDNN alone; the moment the tile falls back to RMSSD it would show a - // recovery-off account a tile built from recovery-owned rows. - expect(disabledSummaryTypes({ recovery: false })).toContain("HRV_RMSSD"); - // SDNN is a plain vital and stays ungated — turning recovery off must not - // take the ordinary HRV tile away from an Apple / Fitbit account. - expect(disabledSummaryTypes({ recovery: false })).not.toContain( - "HEART_RATE_VARIABILITY", - ); - // And nothing is dropped when the module is on. - expect(disabledSummaryTypes({ recovery: true }).size).toBe(0); - }); - - it("keeps the fallback aligned with the metric-status owner map", () => { - // The two maps answer different questions and are allowed to differ, but - // not for THIS type: a fallback that serves data the primary path refuses - // is exactly what `METRIC_STATUS_MODULE_OWNERS` documents as the hazard. - expect(moduleForMeasurementType("HRV_RMSSD")).toBe("recovery"); - expect(SUMMARY_TYPE_MODULE.HRV_RMSSD).toBe("recovery"); - }); -}); - -describe("the tile wiring follows the chosen series", () => { - const PAGE = readFileSync( - join(process.cwd(), "src/app/page-client.tsx"), - "utf8", - ); - - it("sources the HRV summary from the helper, not a bare SDNN lookup", () => { - expect(PAGE).toContain("pickHrvSummary("); - // The exact line the bug lived on. If it comes back, so does the bug. - expect(PAGE).not.toContain( - "const hrvSummary = data?.summaries?.HEART_RATE_VARIABILITY", - ); - }); - - it("reads freshness off the type actually shown", () => { - // A hardcoded SDNN lookup here captions a live RMSSD tile as stale. - expect(PAGE).toContain("staleDays={tileStaleDays(hrvType)}"); - expect(PAGE).not.toContain('tileStaleDays("HEART_RATE_VARIABILITY")'); - }); - - it("names the measure when the RMSSD series is the one on show", () => { - // Mirrors how `/insights/hrv` appends `· RMSSD` to its chart title, so a - // 20-60 ms RMSSD reading is not mistaken for a collapsed SDNN one. - expect(PAGE).toContain('hrvType === "HRV_RMSSD"'); - expect(PAGE).toContain("· RMSSD"); - }); -}); diff --git a/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.tsx b/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.tsx new file mode 100644 index 000000000..0ab58efe5 --- /dev/null +++ b/src/app/__tests__/dashboard-hrv-rmssd-fallback.test.tsx @@ -0,0 +1,323 @@ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { renderToStaticMarkup } from "react-dom/server"; +import { HeartPulse } from "lucide-react"; + +import { + pickHrvSummary, + HRV_LABEL_KEY, +} from "@/components/dashboard/dashboard-gates"; +import { TrendCard } from "@/components/charts/trend-card"; +import { I18nProvider } from "@/lib/i18n/context"; +import { + SUMMARY_TYPE_MODULE, + disabledSummaryTypes, + gateSummariesByModules, + unavailableWidgetIds, +} from "@/lib/dashboard/widget-modules"; +import { moduleForMeasurementType } from "@/lib/modules/measurement-scope"; +import type { DataSummary } from "@/lib/analytics/trends"; + +/** + * The dashboard HRV tile must accept either HRV type. + * + * HRV is stored under two measurement types that are not the same measure: + * SDNN (`HEART_RATE_VARIABILITY`, Apple / Fitbit / Google Health) and nightly + * RMSSD (`HRV_RMSSD`, Oura / Polar / WHOOP, and a bridge pushing a Garmin-style + * feed). Every other HRV surface already unions them — `/insights/hrv` via + * `fallbackMeasurementType`, the Coach via `COACH_SOURCE_MEASUREMENT_TYPES.hrv`, + * the MCP reads via `withHrvFallback` — and `sub-page-metric.ts` states that + * listing both is what keeps "the tab, pill, and dashboard tile lit whichever + * source the user has". + * + * The tile did not. It read `summaries.HEART_RATE_VARIABILITY` alone, so an + * RMSSD-only account got a Settings → Dashboard toggle that turned on and + * rendered nothing, with no message, while `/insights/hrv` charted the same + * readings. That asymmetry is what this file pins. + * + * Mutation check: return the fallback unconditionally and the "SDNN wins" + * cases go red; drop the fallback arm and the RMSSD cases go red; return the + * fallback's TYPE while returning the primary's summary and the freshness + * case goes red. + */ +function summary(count: number, latest: number): DataSummary { + return { + count, + latest, + min: latest, + max: latest, + mean: latest, + median: latest, + avg7: latest, + avg30: latest, + slope30: 0, + } as unknown as DataSummary; +} + +describe("pickHrvSummary", () => { + it("takes SDNN when the account has it", () => { + const picked = pickHrvSummary({ + HEART_RATE_VARIABILITY: summary(12, 48), + }); + expect(picked.type).toBe("HEART_RATE_VARIABILITY"); + expect(picked.summary?.latest).toBe(48); + }); + + it("falls back to RMSSD for a ring / strap account", () => { + // The regression itself: pre-fix this returned undefined, `hasHrv` was + // false, and the tile never rendered however the widget was toggled. + const picked = pickHrvSummary({ HRV_RMSSD: summary(30, 42) }); + expect(picked.type).toBe("HRV_RMSSD"); + expect(picked.summary?.latest).toBe(42); + }); + + it("prefers SDNN when the account carries both series", () => { + // The case a naive `sdnn ?? rmssd` written the wrong way round breaks. + // Both are stored in ms but they measure different things, so the tile + // shows one series, never a blend, and SDNN is the primary. + const picked = pickHrvSummary({ + HEART_RATE_VARIABILITY: summary(9, 55), + HRV_RMSSD: summary(30, 42), + }); + expect(picked.type).toBe("HEART_RATE_VARIABILITY"); + expect(picked.summary?.latest).toBe(55); + }); + + it("treats a present-but-empty SDNN summary as absent", () => { + // The slice can carry a zero-count summary for a type the account has no + // rows for. Keying off presence rather than `count` would pin the tile to + // an empty SDNN series and re-break the ring user. + const picked = pickHrvSummary({ + HEART_RATE_VARIABILITY: summary(0, 0), + HRV_RMSSD: summary(30, 42), + }); + expect(picked.type).toBe("HRV_RMSSD"); + expect(picked.summary?.count).toBe(30); + }); + + it("reports no series, on the primary type, when the account has neither", () => { + // The tile is hidden either way; reporting the fallback type here would + // make an account with no HRV at all look like a ring user to the label + // and the freshness lookup. + // Annotated: inferred from the literals alone, `{}` widens to + // `{ HRV_RMSSD?: undefined }`, which the index signature rejects. + const inputs: (Record | undefined)[] = [ + undefined, + {}, + { HRV_RMSSD: summary(0, 0) }, + ]; + for (const input of inputs) { + const picked = pickHrvSummary(input); + expect((picked.summary?.count ?? 0) > 0).toBe(false); + expect(picked.type).toBe("HEART_RATE_VARIABILITY"); + } + }); + + it("does not gate on the recovery module itself", () => { + // The gate belongs server-side (see the sibling describe below), so a + // second one here would be a competing source of truth. + // + // Bounded to the function's OWN body — start of the declaration to its + // closing brace in column 0. Slicing to the end of the file instead, as + // this first did, quietly turns every later addition to the module into + // a failure of this test: the next helper that legitimately mentions a + // module would go red here for no reason, pointing at HRV. + const src = readFileSync( + join(process.cwd(), "src/components/dashboard/dashboard-gates.ts"), + "utf8", + ); + const start = src.indexOf("export function pickHrvSummary("); + expect(start).toBeGreaterThan(-1); + const end = src.indexOf("\n}\n", start); + expect(end).toBeGreaterThan(start); + const body = src.slice(start, end); + // Guard the bound itself: a body that swallowed the rest of the file + // would pass the assertion below for the wrong reason. + expect(body).not.toContain("export function resolve"); + expect(body).not.toMatch(/modules|recovery|isModuleEnabled/i); + }); +}); + +describe("the RMSSD fallback cannot leak recovery-module data", () => { + it("drops HRV_RMSSD from the snapshot summaries when recovery is off", () => { + // The half of this fix that is easy to miss. `HRV_RMSSD` is recovery-owned + // in `METRIC_STATUS_MODULE_OWNERS`, which governs the metric-status and + // MCP reads — but the dashboard snapshot gates off `SUMMARY_TYPE_MODULE`, + // a DIFFERENT map, and that one did not list it. Inert while the tile read + // SDNN alone; the moment the tile falls back to RMSSD it would show a + // recovery-off account a tile built from recovery-owned rows. + expect(disabledSummaryTypes({ recovery: false })).toContain("HRV_RMSSD"); + // SDNN is a plain vital and stays ungated — turning recovery off must not + // take the ordinary HRV tile away from an Apple / Fitbit account. + expect(disabledSummaryTypes({ recovery: false })).not.toContain( + "HEART_RATE_VARIABILITY", + ); + // And nothing is dropped when the module is on. + expect(disabledSummaryTypes({ recovery: true }).size).toBe(0); + }); + + it("keeps the fallback aligned with the metric-status owner map", () => { + // The two maps answer different questions and are allowed to differ, but + // not for THIS type: a fallback that serves data the primary path refuses + // is exactly what `METRIC_STATUS_MODULE_OWNERS` documents as the hazard. + expect(moduleForMeasurementType("HRV_RMSSD")).toBe("recovery"); + expect(SUMMARY_TYPE_MODULE.HRV_RMSSD).toBe("recovery"); + }); +}); + +describe("the tile the user actually sees", () => { + // The point of the fix is a tile that PAINTS for a ring / strap account, + // so the test renders one. The three assertions this replaces matched + // formatted source text in `page-client.tsx` — they passed on a string + // that looked right and would have passed just as well if the tile never + // rendered at all, and any reformatting broke them. + // + // `page-client.tsx` itself cannot be mounted in a unit test (it is the + // whole dashboard: auth, a dozen queries, layout). What CAN be rendered + // is the seam the bug lived on — the summaries slice going in, the tile's + // props coming out — so the helper's real return feeds a real TrendCard. + function renderHrvTile(summaries: Record | undefined) { + const { summary, labelKey } = pickHrvSummary(summaries); + return renderToStaticMarkup( + + + , + ); + } + + it("paints the RMSSD reading for a ring / strap account", () => { + const html = renderHrvTile({ HRV_RMSSD: summary(30, 42) }); + // The regression itself: pre-fix this tile had no series and never + // rendered. The reading has to be ON SCREEN, not merely selected. + expect(html).toContain("42"); + expect(html).toContain("ms"); + }); + + it("names the measure with the RMSSD key, not a concatenated marker", () => { + // `HRV_LABEL_KEY` is the single mapping from series to label key, and + // the tile takes whichever the helper chose. One key per series is what + // survives truncation in the longer locales. + expect(pickHrvSummary({ HRV_RMSSD: summary(30, 42) }).labelKey).toBe( + HRV_LABEL_KEY.HRV_RMSSD, + ); + expect( + pickHrvSummary({ HEART_RATE_VARIABILITY: summary(9, 55) }).labelKey, + ).toBe(HRV_LABEL_KEY.HEART_RATE_VARIABILITY); + // The two series never share a label — that is the whole point of + // naming the measure. + expect(HRV_LABEL_KEY.HRV_RMSSD).not.toBe( + HRV_LABEL_KEY.HEART_RATE_VARIABILITY, + ); + }); + + it("carries the chosen label all the way into the painted markup", () => { + expect(renderHrvTile({ HRV_RMSSD: summary(30, 42) })).toContain( + `__${HRV_LABEL_KEY.HRV_RMSSD}__`, + ); + expect(renderHrvTile({ HEART_RATE_VARIABILITY: summary(9, 55) })).toContain( + `__${HRV_LABEL_KEY.HEART_RATE_VARIABILITY}__`, + ); + }); + + it("names both HRV label keys in every locale bundle", () => { + // A key that resolves in `en` and nowhere else paints the raw key + // string to everyone else. Both series' keys must exist in all seven. + const locales = ["de", "en", "es", "fr", "it", "ko", "pl"]; + for (const locale of locales) { + const bundle = JSON.parse( + readFileSync(join(process.cwd(), `messages/${locale}.json`), "utf8"), + ) as { measurements?: Record }; + for (const key of Object.values(HRV_LABEL_KEY)) { + const leaf = key.replace("measurements.", ""); + expect(bundle.measurements?.[leaf], `${locale}: ${key}`).toBeTruthy(); + } + } + }); + + it("does not read freshness off a hardcoded SDNN type", () => { + // The one source-level check kept, and deliberately a NEGATIVE one: it + // names the exact expression the bug lived on rather than asserting the + // current formatting of the replacement, so reformatting cannot break + // it. Freshness is per-type and captioning a live RMSSD tile from the + // SDNN lookup is the failure it guards. + const page = readFileSync( + join(process.cwd(), "src/app/page-client.tsx"), + "utf8", + ); + expect(page).not.toContain('tileStaleDays("HEART_RATE_VARIABILITY")'); + expect(page).not.toContain( + "const hrvSummary = data?.summaries?.HEART_RATE_VARIABILITY", + ); + }); +}); + +describe("the gate reaches every feed, not just the default one", () => { + it("strips the recovery-owned series from a summaries slice", () => { + // `gateSummariesByModules` moved out of the snapshot builder, where it + // was private, so `/api/analytics` can apply the SAME filter. With + // `NEXT_PUBLIC_DASHBOARD_SNAPSHOT=false` that route feeds the tiles, and + // it filtered nothing at all — so recovery-off plus RMSSD-only still + // painted the tile, on the exact path the helper's docblock promised it + // could not. A gate that only holds on one feed is only as strong as a + // rollout flag. + const summaries = { + HEART_RATE_VARIABILITY: summary(9, 55), + HRV_RMSSD: summary(30, 42), + }; + const lastSeen = { + HEART_RATE_VARIABILITY: { lastSeenAt: "2026-09-01T00:00:00.000Z" }, + HRV_RMSSD: { lastSeenAt: "2026-09-02T00:00:00.000Z" }, + }; + const off = gateSummariesByModules(summaries, lastSeen, { + recovery: false, + }); + expect(off.summaries.HRV_RMSSD).toBeUndefined(); + expect(off.lastSeenByType.HRV_RMSSD).toBeUndefined(); + // SDNN is nobody's module and must survive. + expect(off.summaries.HEART_RATE_VARIABILITY).toBeDefined(); + // Recovery on: nothing is touched, and the inputs are never mutated. + const on = gateSummariesByModules(summaries, lastSeen, { recovery: true }); + expect(on.summaries.HRV_RMSSD).toBeDefined(); + expect(summaries.HRV_RMSSD).toBeDefined(); + }); +}); + +describe("a switch that cannot do anything is not offered", () => { + // The symptom one step on from the original bug: with recovery off, a + // ring / strap account's only HRV series is withheld, so the tile can + // never paint — and before this, the toggle was still offered, still + // turned on, and still explained nothing. Same experience as the defect + // this change exists to remove, so it does not get to survive here. + it("drops the HRV toggle for a ring account with recovery off", () => { + expect( + unavailableWidgetIds({ recovery: false }, { hasSdnn: false }), + ).toEqual(["hrv"]); + }); + + it("keeps the toggle for an account that has SDNN", () => { + // SDNN is a plain vital that no module owns, so the tile still works + // with recovery off. Taking this row away would be a new bug. + expect( + unavailableWidgetIds({ recovery: false }, { hasSdnn: true }), + ).toEqual([]); + }); + + it("keeps the toggle whenever recovery is on", () => { + expect( + unavailableWidgetIds({ recovery: true }, { hasSdnn: false }), + ).toEqual([]); + // Fail-open on an unresolved map, like the module gate beside it: only + // an explicit `false` takes a row away. + expect(unavailableWidgetIds({}, { hasSdnn: false })).toEqual([]); + }); +}); diff --git a/src/app/api/analytics/route.ts b/src/app/api/analytics/route.ts index 4af449575..351181531 100644 --- a/src/app/api/analytics/route.ts +++ b/src/app/api/analytics/route.ts @@ -22,7 +22,8 @@ import { type BpInTargetEnvelope, } from "@/lib/analytics/bp-in-target-fast-path"; import { computeAndRecordUserHealthScore } from "@/lib/analytics/score/record"; -import { isModuleEnabled } from "@/lib/modules/gate"; +import { isModuleEnabled, resolveModuleMap } from "@/lib/modules/gate"; +import { gateSummariesByModules } from "@/lib/dashboard/widget-modules"; import { resolveRestMode } from "@/lib/illness/rest-mode"; import { deriveBpWindow90 } from "@/lib/analytics/window-confidence"; import { computeCorrelationHypothesesFastPath } from "@/lib/analytics/correlations-fast-path"; @@ -109,10 +110,32 @@ export const GET = apiHandler(async (request?: Request) => { () => computeSummariesSlice(user.id), annotate, ); + // Drop the types whose module the user turned off, from the SAME map + // the snapshot builder gates on (`SUMMARY_TYPE_MODULE`, via + // `gateSummariesByModules`). With `NEXT_PUBLIC_DASHBOARD_SNAPSHOT=false` + // this slice — not the snapshot — feeds the dashboard tile strip, and + // it used to hand back every type regardless: recovery off plus an + // RMSSD-only account still painted the HRV tile, because nothing on + // this path had ever filtered. + // + // Gated AFTER the cache read on purpose. The cached cell stays + // module-independent, so toggling a module takes effect on the next + // request instead of waiting out the TTL, and two users' module states + // can never share a cached answer. + const slimModules = await resolveModuleMap(user.id); + const slimGated = gateSummariesByModules( + slim.summaries, + slim.lastSeenByType, + slimModules, + ); // v1.4.34 IW-B — bfcache-friendly directive on the slim slice too // so a back-forward navigation that landed on the dashboard tile // strip can restore from memory instead of paying a full reload. - const slimRes = apiSuccess(slim); + const slimRes = apiSuccess({ + ...slim, + summaries: slimGated.summaries, + lastSeenByType: slimGated.lastSeenByType, + }); slimRes.headers.set("Cache-Control", NO_STORE_BUT_BFCACHE); return slimRes; } @@ -269,8 +292,18 @@ async function buildAnalyticsResponse(user: AuthedUser, locale: Locale) { // pre-fix behaviour for tenants whose year-ago window happened to // fall outside the 425-day floor. const slim = await computeSummariesSlice(user.id); - const results = slim.summaries; - const lastSeenByType = slim.lastSeenByType; + // Same module gate as the slim slice and the snapshot builder — the + // default slice feeds the dashboard too (it is the thick half of the + // legacy two-cell path), so it cannot be the one feed that leaks a + // disabled module's types. + const thickModules = await resolveModuleMap(user.id); + const thickGated = gateSummariesByModules( + slim.summaries, + slim.lastSeenByType, + thickModules, + ); + const results = thickGated.summaries; + const lastSeenByType = thickGated.lastSeenByType; // `computeSummariesSlice` annotates `action: "analytics.get.slim"` // for telemetry on the slim route. Restore the default-slice action diff --git a/src/app/api/dashboard/widgets/route.ts b/src/app/api/dashboard/widgets/route.ts index 7cd00455e..e1fe467d9 100644 --- a/src/app/api/dashboard/widgets/route.ts +++ b/src/app/api/dashboard/widgets/route.ts @@ -14,6 +14,8 @@ import { sanitiseZodIssues, } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; +import { resolveModuleMap } from "@/lib/modules/gate"; +import { unavailableWidgetIds } from "@/lib/dashboard/widget-modules"; import { prisma, toJson } from "@/lib/db"; import { takeBaseToken, @@ -218,7 +220,36 @@ export const GET = apiHandler(async () => { () => buildDashboardLayout(user.id), annotate, ); - return apiSuccess(layout); + + // Which toggles cannot do anything for this account. Resolved here, on + // the server, so the Settings screen never has to fetch dashboard data + // to find out whether a switch it is about to draw is dead. + // + // Deliberately OUTSIDE the cached layout above: the layout cell is keyed + // on the user and holds their arrangement, which does not change when a + // module is toggled. Folding a module-dependent answer into it would + // serve a stale one for the rest of the 5-minute TTL — the switch would + // linger for minutes after Recovery went off, which is the confusion this + // is removing. + // + // The SDNN probe is an index-only existence check + // (`@@index([userId, type, measuredAt])`), not a summaries read; it runs + // only when Recovery is off, so the common account pays nothing. + const modules = await resolveModuleMap(user.id); + let hasSdnn = true; + if (modules.recovery === false) { + const sdnnRow = await prisma.measurement.findFirst({ + where: { userId: user.id, type: "HEART_RATE_VARIABILITY" }, + select: { id: true }, + }); + hasSdnn = sdnnRow !== null; + } + const unavailable = unavailableWidgetIds(modules, { hasSdnn }); + return apiSuccess( + unavailable.length > 0 + ? { ...layout, unavailableWidgetIds: unavailable } + : layout, + ); }); export const PUT = apiHandler(async (request: NextRequest) => { diff --git a/src/app/page-client.tsx b/src/app/page-client.tsx index 72aecb9d6..182c42526 100644 --- a/src/app/page-client.tsx +++ b/src/app/page-client.tsx @@ -498,9 +498,11 @@ export default function DashboardPageClient({ // rendered nothing while `/insights/hrv` charted the same data. The // helper keeps the union in one place and hands back which type won, so // the freshness caption and the label below follow the series shown. - const { summary: hrvSummary, type: hrvType } = pickHrvSummary( - data?.summaries, - ); + const { + summary: hrvSummary, + type: hrvType, + labelKey: hrvLabelKey, + } = pickHrvSummary(data?.summaries); const spo2Summary = data?.summaries?.OXYGEN_SATURATION; const respRateSummary = data?.summaries?.RESPIRATORY_RATE; const wristTempSummary = data?.summaries?.WRIST_TEMPERATURE; @@ -1319,16 +1321,18 @@ export default function DashboardPageClient({ node: ( = { + HEART_RATE_VARIABILITY: "measurements.typeHeartRateVariability", + HRV_RMSSD: "measurements.typeHrvRmssd", +}; + /** * Which HRV series backs the dashboard tile. * @@ -167,18 +180,30 @@ export type HrvSummaryType = "HEART_RATE_VARIABILITY" | "HRV_RMSSD"; */ export function pickHrvSummary( summaries: Record | undefined, -): { summary: DataSummary | null; type: HrvSummaryType } { +): { summary: DataSummary | null; type: HrvSummaryType; labelKey: string } { + const pick = ( + summary: DataSummary | null, + type: HrvSummaryType, + ): { + summary: DataSummary | null; + type: HrvSummaryType; + labelKey: string; + } => ({ + summary, + type, + labelKey: HRV_LABEL_KEY[type], + }); const primary = summaries?.HEART_RATE_VARIABILITY; if ((primary?.count ?? 0) > 0) { - return { summary: primary ?? null, type: "HEART_RATE_VARIABILITY" }; + return pick(primary ?? null, "HEART_RATE_VARIABILITY"); } const fallback = summaries?.HRV_RMSSD; if ((fallback?.count ?? 0) > 0) { - return { summary: fallback ?? null, type: "HRV_RMSSD" }; + return pick(fallback ?? null, "HRV_RMSSD"); } // Neither has rows. Report the primary type so the caller's stale-days // lookup and label stay on SDNN for the empty case — the tile is hidden // either way, and reporting the fallback here would make an account with // no HRV at all look like a ring user. - return { summary: primary ?? null, type: "HEART_RATE_VARIABILITY" }; + return pick(primary ?? null, "HEART_RATE_VARIABILITY"); } diff --git a/src/components/settings/dashboard-layout-section.tsx b/src/components/settings/dashboard-layout-section.tsx index 0ee6013a2..ef9549330 100644 --- a/src/components/settings/dashboard-layout-section.tsx +++ b/src/components/settings/dashboard-layout-section.tsx @@ -1,7 +1,7 @@ "use client"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { useId, useState } from "react"; +import { useId, useMemo, useState } from "react"; import { toast } from "sonner"; import { LayoutDashboard, @@ -258,6 +258,17 @@ export function DashboardLayoutSection({ id }: { id: string }) { refetchOnMount: "always", }); + // Toggles the SERVER resolved as unavailable for this account — a switch + // that is on but can never paint a tile is exactly the confusion this + // screen must not create. Decided server-side (see `unavailableWidgetIds`) + // so this screen needs no dashboard data of its own. Absent field, or a + // query still in flight, means nothing is dead: the row shows, which is + // the fail-open direction the module gate below also takes. + const unavailableIds = useMemo( + () => new Set(remote?.unavailableWidgetIds ?? []), + [remote?.unavailableWidgetIds], + ); + /** * v1.32.16 (issue #581) — the freshest optimistic-concurrency token the * client knows. Read straight from the widgets query cache at mutate time @@ -602,6 +613,7 @@ export function DashboardLayoutSection({ id }: { id: string }) { const moduleKey = WIDGET_MODULE_BY_ID[w.id]; return !moduleKey || modules?.[moduleKey] !== false; }) + .filter((w) => !unavailableIds.has(w.id)) .sort((a, b) => a.order - b.order); const sortedIds = sortedWidgets.map((w) => w.id); return ( @@ -677,6 +689,7 @@ export function DashboardLayoutSection({ id }: { id: string }) { const moduleKey = WIDGET_MODULE_BY_ID[w.id]; return !moduleKey || modules?.[moduleKey] !== false; }) + .filter((w) => !unavailableIds.has(w.id)) .sort((a, b) => a.order - b.order); if (nativeWidgets.length === 0) return null; return ( diff --git a/src/lib/dashboard-layout.ts b/src/lib/dashboard-layout.ts index 9bd51187f..36564944b 100644 --- a/src/lib/dashboard-layout.ts +++ b/src/lib/dashboard-layout.ts @@ -709,6 +709,13 @@ export function mergePreservedLayoutFields( */ export type DashboardLayoutWithToken = DashboardLayout & { updatedAt?: string; + /** + * Widget ids the server resolved as unavailable for this account — a + * toggle that cannot do anything, which Settings must not offer. See + * `unavailableWidgetIds`. Optional and additive: a client that ignores + * it behaves exactly as before. + */ + unavailableWidgetIds?: string[]; }; const DASHBOARD_LAYOUT_VERSION = 1; diff --git a/src/lib/dashboard/snapshot.ts b/src/lib/dashboard/snapshot.ts index ea56aa949..9e403895d 100644 --- a/src/lib/dashboard/snapshot.ts +++ b/src/lib/dashboard/snapshot.ts @@ -130,7 +130,8 @@ export { } from "@/lib/dashboard/widget-modules"; import { WIDGET_MODULE_BY_ID, - disabledSummaryTypes, + gateSummariesByModules, + unavailableWidgetIds, } from "@/lib/dashboard/widget-modules"; const GLUCOSE_CONTEXTS = [ @@ -1078,7 +1079,16 @@ function isThickPhaseWarm(coverage: RollupCoverageMap): boolean { function gateLayoutByModules( layout: DashboardLayout, modules: Record, + summaries: Record, ): DashboardLayout { + // Widgets that cannot paint for THIS account, on the same rule the + // Settings screen hides their toggle with — so the layout the dashboard + // renders and the switches the user is offered never disagree. + const unavailable = new Set( + unavailableWidgetIds(modules, { + hasSdnn: (summaries.HEART_RATE_VARIABILITY?.count ?? 0) > 0, + }), + ); return { ...layout, widgets: layout.widgets.map((w) => { @@ -1086,40 +1096,14 @@ function gateLayoutByModules( if (moduleKey && modules[moduleKey] === false) { return { ...w, visible: false, tileVisible: false }; } + if (unavailable.has(w.id)) { + return { ...w, visible: false, tileVisible: false }; + } return w; }), }; } -/** - * v1.18.0 — strip disabled-module measurement types from the slim slice - * so neither `tiles.summaries` / `tiles.lastSeenByType` nor the derived - * `metricStates` carry data for a module the user turned off. Returns a - * shallow copy; the inputs are not mutated. - */ -function gateSummariesByModules( - summaries: Record, - lastSeenByType: Record, - modules: Record, -): { - summaries: Record; - lastSeenByType: Record; -} { - // The drop decision is shared with `GET /api/dashboard/summary` — see - // `disabledSummaryTypes`. Only the filtering below is snapshot-shaped. - const dropped = disabledSummaryTypes(modules); - if (dropped.size === 0) return { summaries, lastSeenByType }; - const outSummaries: Record = {}; - for (const [type, summary] of Object.entries(summaries)) { - if (!dropped.has(type)) outSummaries[type] = summary; - } - const outLastSeen: Record = {}; - for (const [type, slot] of Object.entries(lastSeenByType)) { - if (!dropped.has(type)) outLastSeen[type] = slot; - } - return { summaries: outSummaries, lastSeenByType: outLastSeen }; -} - /** * v1.21.2 (A4) — map the machine-derived coach memory block onto the briefing * card's `{ recall, forward }` shape, both already-localised. `recall` is the @@ -1372,7 +1356,7 @@ export async function buildDashboardSnapshot( }); } - const layout = gateLayoutByModules(storedLayout, modules); + const layout = gateLayoutByModules(storedLayout, modules, slim.summaries); // v1.18.0 — the Daily Briefing is the dashboard's AI-narrative surface, // so the `insights` module gates it alongside the operator briefing // flag + per-user coach opt-out. Disabling `insights` yields the same diff --git a/src/lib/dashboard/widget-modules.ts b/src/lib/dashboard/widget-modules.ts index 375d28db5..1d2c39b44 100644 --- a/src/lib/dashboard/widget-modules.ts +++ b/src/lib/dashboard/widget-modules.ts @@ -113,6 +113,72 @@ export function disabledSummaryTypes( return dropped; } +/** + * Strip disabled-module types from a summaries slice. Lifted out of + * `@/lib/dashboard/snapshot`, where it was private, once a second + * dashboard feed needed it: with `NEXT_PUBLIC_DASHBOARD_SNAPSHOT=false` + * the tile strip hydrates from `/api/analytics` instead of the snapshot, + * and that route filtered nothing — so a module the user had turned off + * still reached the client, and the tile it owns still painted. The gate + * has to sit on every feed that carries summaries, not just the default + * one, or it is only as strong as a rollout flag. + * + * Pure over its inputs and browser-safe (string keys + the map above), so + * the snapshot builder, the analytics route and any future feed share one + * implementation rather than each carrying a copy that can drift. + * Returns shallow copies; the inputs are not mutated. + */ +/** + * Widget ids whose toggle cannot do anything for THIS account, and so must + * not be offered at all. `WIDGET_MODULE_BY_ID` above answers the same + * question for widgets a module owns outright; this answers it for the one + * widget whose deadness also depends on the account's data. + * + * `hrv` is that widget. The tile takes SDNN when the account has it and + * falls back to nightly RMSSD otherwise (see `pickHrvSummary`). RMSSD is + * recovery-owned, so with Recovery off it is stripped from every feed — + * which leaves a ring / strap account, whose only HRV is RMSSD, holding a + * switch that is on and a tile that can never paint, with nothing saying + * why. That is precisely the defect the RMSSD fallback exists to remove, so + * it must not survive one step further on. An account WITH SDNN keeps the + * row: its tile still works with Recovery off, because SDNN is a plain + * vital that no module owns. + * + * Decided from server-resolved facts and handed to the client, so the + * Settings screen does not have to fetch dashboard data to find out. + */ +export function unavailableWidgetIds( + modules: Partial>, + facts: { hasSdnn: boolean }, +): string[] { + const out: string[] = []; + if (modules.recovery === false && !facts.hasSdnn) out.push("hrv"); + return out; +} + +export function gateSummariesByModules< + S extends Record, + L extends Record, +>( + summaries: S, + lastSeenByType: L, + modules: Partial>, +): { summaries: S; lastSeenByType: L } { + const dropped = disabledSummaryTypes(modules); + if (dropped.size === 0) return { summaries, lastSeenByType }; + const outSummaries = {} as S; + for (const [type, summary] of Object.entries(summaries)) { + if (!dropped.has(type)) + (outSummaries as Record)[type] = summary; + } + const outLastSeen = {} as L; + for (const [type, slot] of Object.entries(lastSeenByType)) { + if (!dropped.has(type)) + (outLastSeen as Record)[type] = slot; + } + return { summaries: outSummaries, lastSeenByType: outLastSeen }; +} + /** * iOS `MetricKind` (the `kind` on a `GET /api/dashboard/summary` metric * card) → the `MeasurementType` it is built from. The summary route emits