mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-12 06:06:10 +00:00
Address review: label key, gate every feed, retire the dead switch
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.
This commit is contained in:
11 files changed
+551
-235
No files matched your search
@@ -134,6 +134,19 @@ export function resolveChartRowPlaceholderCount(
|
||||
/** The two measurement types the HRV surface reads, primary first. */
|
||||
export type HrvSummaryType = "HEART_RATE_VARIABILITY" | "HRV_RMSSD";
|
||||
|
||||
/**
|
||||
* The i18n key naming each HRV series. One key per series, never a base
|
||||
* label with the measure concatenated on: the tile label paints truncated,
|
||||
* uppercase and on a single line, so an appended marker is the first thing
|
||||
* dropped in the longer locales — and the marker is the part a reader needs
|
||||
* to tell a 40 ms RMSSD night from a collapsed SDNN one. Both keys are
|
||||
* translated in all seven locales.
|
||||
*/
|
||||
export const HRV_LABEL_KEY: Record<HrvSummaryType, string> = {
|
||||
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<string, DataSummary> | 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");
|
||||
}
|
||||
@@ -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 (
|
||||
|
||||
Reference in new issue
Block a user