mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 21:10:59 +00:00
Never render a failed read as an absence of data
"No data yet" is a claim about the user's record. Making it when the read failed states something false about their health history. The mental-wellbeing surface was the one that mattered: the assessment history read dropped isError entirely, so a failure left both instrument cards showing "not taken yet" and the detail sheet showing an empty trend. Someone with months of screening results was told, on a mental-health surface, that the results did not exist. It now tells loading, failed and genuinely empty apart, with a retry on the failure. Also swept: - the workouts insights page read isEmpty but not isError, so a failed list read fell through every branch to null and painted a titled shell with nothing in it; - the micronutrients card (not in the original list) rendered its "no nutrient data" empty state on a failed read, while its sibling hydration card already used the shared error card; - the medication detail page and the custom-metric values list showed an honest error with no way out. Both keep retry: false — right for a deleted row, wrong for a transient failure — and gain a user-initiated retry instead, which cannot storm; - /achievements rendered a blank page on a direct hit with the module disabled. It now bounces to /insights like every other gated page. The achievements query error branch, the custom-metric error branch and the chronotype notice were already correct and are left alone.
This commit is contained in:
1 parent
c23f2545cc
commit
c87ce161ba
13 files changed
+330
-53
No files matched your search
@@ -34,6 +34,8 @@ import { EmptyState } from "@/components/ui/empty-state";
|
||||
import { PageHeader } from "@/components/ui/page-header";
|
||||
import { PageAuthGate } from "@/components/ui/page-auth-gate";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import { useEffect } from "react";
|
||||
import { useRouter } from "next/navigation";
|
||||
import { useAuth } from "@/hooks/use-auth";
|
||||
import { useTranslations } from "@/lib/i18n/context";
|
||||
import { formatDate } from "@/lib/format";
|
||||
@@ -260,6 +262,7 @@ function AchievementCard({ achievement, t }: AchievementCardProps) {
|
||||
export default function AchievementsPage() {
|
||||
const { user, isAuthenticated, isLoading: authLoading } = useAuth();
|
||||
const { t } = useTranslations();
|
||||
const router = useRouter();
|
||||
|
||||
// v1.18.0 — the achievements module gate. When the account has the
|
||||
// module turned off the whole page disappears: an empty state stands in
|
||||
@@ -276,14 +279,26 @@ export default function AchievementsPage() {
|
||||
enabled: isAuthenticated && achievementsEnabled,
|
||||
});
|
||||
|
||||
useEffect(() => {
|
||||
if (!authLoading && isAuthenticated && !achievementsEnabled) {
|
||||
router.push("/insights");
|
||||
}
|
||||
}, [authLoading, isAuthenticated, achievementsEnabled, router]);
|
||||
|
||||
if (authLoading || (achievementsEnabled && isLoading)) {
|
||||
return <PageAuthGate label={t("common.loading")} />;
|
||||
}
|
||||
|
||||
// v1.18.0 — module off ⇒ the surface disappears entirely. The nav entry
|
||||
// is hidden by the same gate, so a direct hit on /achievements renders
|
||||
// nothing rather than an orphaned shell. No new i18n string is minted
|
||||
// (the disabled state has no copy of its own).
|
||||
// v1.18.0 — module off ⇒ the surface disappears entirely. The nav entry is
|
||||
// hidden by the same gate, so this only catches a direct URL hit. It used to
|
||||
// render `null` and stop there, which left a blank page with no explanation
|
||||
// and no route anywhere — a dead end rather than a gate. It now bounces to
|
||||
// /insights, matching every other module-gated page (`useModulePageGuard`);
|
||||
// the brief `null` is the pre-redirect frame, not the destination.
|
||||
//
|
||||
// The shared guard is not used wholesale here because it also redirects an
|
||||
// unauthenticated visitor to /auth/login, and this page deliberately answers
|
||||
// that case with a "sign in to see your achievements" header instead.
|
||||
if (isAuthenticated && !achievementsEnabled) {
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ import { MetricEmptyState } from "@/components/insights/metric-empty-state";
|
||||
import { SubPageShell } from "@/components/insights/sub-page-shell";
|
||||
import { WorkoutList } from "@/components/insights/workout-list";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
|
||||
/**
|
||||
* v1.4.32 — `/insights/workouts`.
|
||||
@@ -36,7 +37,13 @@ import { Skeleton } from "@/components/ui/skeleton";
|
||||
export default function InsightsWorkoutsPageClient() {
|
||||
const { t } = useTranslations();
|
||||
const { ready } = useModulePageGuard("workouts");
|
||||
const { data, isLoading, isEmpty } = useWorkouts({ limit: 100 });
|
||||
// `isError` / `refetch` were already on the hook and simply not read here, so
|
||||
// a failed list read fell through every branch to `null` and painted a titled
|
||||
// shell with nothing in it — no message, no way back. Not the empty state,
|
||||
// but just as opaque.
|
||||
const { data, isLoading, isEmpty, isError, refetch } = useWorkouts({
|
||||
limit: 100,
|
||||
});
|
||||
|
||||
// v1.18.0 B1 — bounce a direct URL hit on a disabled-workouts account.
|
||||
if (!ready) {
|
||||
@@ -57,7 +64,12 @@ export default function InsightsWorkoutsPageClient() {
|
||||
{/* No `<MetricRangeControls>` here: workouts are session records, not a
|
||||
MeasurementType series, so the period-over-period range read has
|
||||
nothing to aggregate. */}
|
||||
{isLoading ? (
|
||||
{isError ? (
|
||||
<QueryErrorCard
|
||||
title={t("insights.workouts.loadError")}
|
||||
onRetry={() => refetch()}
|
||||
/>
|
||||
) : isLoading ? (
|
||||
<div data-slot="workouts-loading" className="space-y-2">
|
||||
<Skeleton className="h-14 w-full rounded-lg" />
|
||||
<Skeleton className="h-14 w-full rounded-lg" />
|
||||
|
||||
@@ -22,7 +22,7 @@ import { useTranslations } from "@/lib/i18n/context";
|
||||
import { queryKeys } from "@/lib/query-keys";
|
||||
|
||||
import { BackLink } from "@/components/ui/back-link";
|
||||
import { Card } from "@/components/ui/card";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import {
|
||||
MedicationDetailTabs,
|
||||
type MedicationDetailSnapshot,
|
||||
@@ -49,6 +49,7 @@ export default function MedicationDetailPage({
|
||||
data: medication,
|
||||
isLoading,
|
||||
isError,
|
||||
refetch,
|
||||
} = useQuery<MedicationDetailSnapshot>({
|
||||
queryKey: queryKeys.medicationDetail(id),
|
||||
queryFn: async () => {
|
||||
@@ -71,20 +72,21 @@ export default function MedicationDetailPage({
|
||||
return <PageAuthGate label={t("common.loading")} />;
|
||||
}
|
||||
|
||||
// `retry: false` above stays: a deleted medication 404s here and automatic
|
||||
// backoff against a row that no longer exists is pure waste. But that flag
|
||||
// also stranded the page on a single transient failure, with no way forward
|
||||
// short of navigating away and back. A user-initiated retry is the right
|
||||
// granularity — it costs one request per click and cannot storm.
|
||||
if (isError || !medication) {
|
||||
return (
|
||||
<div className="space-y-6">
|
||||
<BackLink href="/medications" label={t("medications.back")} />
|
||||
<Card
|
||||
className="p-6"
|
||||
role="alert"
|
||||
aria-live="polite"
|
||||
data-slot="medication-detail-error-card"
|
||||
>
|
||||
<p className="text-destructive text-sm">
|
||||
{t("medications.detail.shell.loadFailed")}
|
||||
</p>
|
||||
</Card>
|
||||
<div data-slot="medication-detail-error-card">
|
||||
<QueryErrorCard
|
||||
description={t("medications.detail.shell.loadFailed")}
|
||||
onRetry={() => void refetch()}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,174 @@
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { renderToStaticMarkup } from "react-dom/server";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
|
||||
import { I18nProvider } from "@/lib/i18n/context";
|
||||
import { queryKeys } from "@/lib/query-keys";
|
||||
import en from "../../../messages/en.json";
|
||||
|
||||
/**
|
||||
* A failed read must never render as an absence of data.
|
||||
*
|
||||
* "No data yet" is a factual claim about the user's record. Making it when the
|
||||
* read failed is a false statement about their health history — worst of all on
|
||||
* the mental-wellbeing surface, where a user with months of screening results
|
||||
* was told, in a mental-health context, that they had never taken an
|
||||
* assessment.
|
||||
*
|
||||
* Each surface must tell three states apart: loading, failed, and genuinely
|
||||
* empty. These tests pin the failed/empty split; the loading branch is asserted
|
||||
* where it is SSR-observable.
|
||||
*
|
||||
* SSR-only convention (`renderToStaticMarkup`, no `@testing-library/react`), so
|
||||
* the error state is produced by seeding the query cache with a settled failure
|
||||
* rather than by driving a real fetch.
|
||||
*/
|
||||
|
||||
vi.mock("@/lib/api/api-fetch", () => ({
|
||||
apiGet: () => new Promise(() => {}),
|
||||
apiPost: () => new Promise(() => {}),
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/use-auth", () => ({
|
||||
useAuth: () => ({
|
||||
user: { gender: "FEMALE", modules: {} },
|
||||
isAuthenticated: true,
|
||||
isLoading: false,
|
||||
}),
|
||||
}));
|
||||
|
||||
import { MentalWellbeing } from "@/components/mental-health/mental-wellbeing";
|
||||
import { MicronutrientsCard } from "@/components/insights/nutrients/micronutrients-card";
|
||||
|
||||
function makeClient() {
|
||||
return new QueryClient({
|
||||
defaultOptions: {
|
||||
queries: {
|
||||
retry: false,
|
||||
// An errored query would otherwise refetch on mount, and the observer
|
||||
// reports that intent as an optimistic pending state during the server
|
||||
// render — which would hide the very branch under test.
|
||||
retryOnMount: false,
|
||||
staleTime: Infinity,
|
||||
},
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Drive `queryKey` to a settled failure through the public prefetch path, so
|
||||
* the SSR render below observes exactly the state a real failed read leaves in
|
||||
* the cache.
|
||||
*/
|
||||
async function seedFailure(
|
||||
client: QueryClient,
|
||||
queryKey: readonly unknown[],
|
||||
): Promise<void> {
|
||||
await client.prefetchQuery({
|
||||
queryKey: queryKey as unknown[],
|
||||
queryFn: () => Promise.reject(new Error("read failed")),
|
||||
retry: false,
|
||||
});
|
||||
}
|
||||
|
||||
function render(node: React.ReactNode, client: QueryClient): string {
|
||||
return renderToStaticMarkup(
|
||||
<QueryClientProvider client={client}>
|
||||
<I18nProvider initialLocale="en">{node}</I18nProvider>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
describe("mental wellbeing — screening history", () => {
|
||||
const mh = en.mentalHealth;
|
||||
|
||||
it("renders the error state with a retry when the history read fails", async () => {
|
||||
const client = makeClient();
|
||||
await seedFailure(client, queryKeys.mentalHealthAssessments());
|
||||
const html = render(<MentalWellbeing />, client);
|
||||
|
||||
expect(html).toContain('data-slot="query-error-card"');
|
||||
expect(html).toContain(mh.historyLoadError);
|
||||
expect(html).toContain(en.common.retry);
|
||||
// The load-bearing assertion: the instrument cards' "not taken yet" copy
|
||||
// must NOT appear. That sentence is a claim about the user's record.
|
||||
expect(html).not.toContain(mh.noResultYet);
|
||||
});
|
||||
|
||||
it("still renders the instrument cards when the record is genuinely empty", () => {
|
||||
const client = makeClient();
|
||||
client.setQueryData(queryKeys.mentalHealthAssessments(), {
|
||||
assessments: [],
|
||||
});
|
||||
const html = render(<MentalWellbeing />, client);
|
||||
|
||||
expect(html).not.toContain('data-slot="query-error-card"');
|
||||
// An honest empty record: the cards render and invite a first check-in.
|
||||
expect(html).toContain(mh.noResultYet);
|
||||
});
|
||||
|
||||
it("shows neither the empty copy nor an error while the read is in flight", () => {
|
||||
const html = render(<MentalWellbeing />, makeClient());
|
||||
expect(html).not.toContain('data-slot="query-error-card"');
|
||||
expect(html).not.toContain(mh.noResultYet);
|
||||
});
|
||||
});
|
||||
|
||||
describe("micronutrients card", () => {
|
||||
const mn = en.nutrients.micronutrients;
|
||||
|
||||
it("renders the error state with a retry when the nutrient read fails", async () => {
|
||||
const client = makeClient();
|
||||
await seedFailure(client, queryKeys.nutrientIntake(30));
|
||||
const html = render(<MicronutrientsCard />, client);
|
||||
|
||||
expect(html).toContain('data-slot="query-error-card"');
|
||||
expect(html).toContain(mn.loadError);
|
||||
expect(html).toContain(en.common.retry);
|
||||
expect(html).not.toContain(mn.emptyTitle);
|
||||
});
|
||||
|
||||
it("still renders the empty state when the record is genuinely empty", () => {
|
||||
const client = makeClient();
|
||||
client.setQueryData(queryKeys.nutrientIntake(30), { nutrients: [] });
|
||||
const html = render(<MicronutrientsCard />, client);
|
||||
|
||||
expect(html).not.toContain('data-slot="query-error-card"');
|
||||
expect(html).toContain(mn.emptyTitle);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The remaining surfaces in the sweep are pages whose render depends on the
|
||||
* router / module gate, which the SSR-only convention cannot drive. Pin their
|
||||
* contract structurally instead: each must read `isError` from its query and
|
||||
* hand a retry to the shared error card, so a later edit cannot quietly drop
|
||||
* the branch and fall back through to the empty state.
|
||||
*/
|
||||
describe("read-failure branch is wired on every swept surface", () => {
|
||||
const surfaces = [
|
||||
"src/components/mental-health/mental-wellbeing.tsx",
|
||||
"src/app/insights/workouts/page-client.tsx",
|
||||
"src/app/medications/[id]/page.tsx",
|
||||
"src/components/custom-metrics/custom-metric-values-list.tsx",
|
||||
"src/app/achievements/page.tsx",
|
||||
"src/components/insights/nutrients/micronutrients-card.tsx",
|
||||
"src/components/insights/sleep/chronotype-section.tsx",
|
||||
] as const;
|
||||
|
||||
for (const rel of surfaces) {
|
||||
it(`${rel} reads isError and offers a retry`, () => {
|
||||
const source = readFileSync(join(process.cwd(), rel), "utf8");
|
||||
expect(source).toMatch(/isError/);
|
||||
// chronotype-section is the one surface that legitimately renders an
|
||||
// inline notice rather than the card (it sits inside a chart section),
|
||||
// so it is exempt from the retry affordance but not from the branch.
|
||||
if (!rel.endsWith("chronotype-section.tsx")) {
|
||||
expect(source).toMatch(/onRetry=/);
|
||||
expect(source).toMatch(/refetch/);
|
||||
}
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -6,6 +6,7 @@ import { useInfiniteQuery, useQuery } from "@tanstack/react-query";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Card, CardContent } from "@/components/ui/card";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import { apiGet } from "@/lib/api/api-fetch";
|
||||
import { useTranslations } from "@/lib/i18n/context";
|
||||
import { queryKeys } from "@/lib/query-keys";
|
||||
@@ -41,6 +42,7 @@ export function CustomMetricValuesList({
|
||||
data: list,
|
||||
isLoading,
|
||||
isError,
|
||||
refetch,
|
||||
fetchNextPage,
|
||||
hasNextPage,
|
||||
isFetchingNextPage,
|
||||
@@ -68,11 +70,15 @@ export function CustomMetricValuesList({
|
||||
);
|
||||
const total = list?.pages[0]?.meta.total ?? 0;
|
||||
|
||||
// The error state was honest but a dead end: one red sentence, no way to try
|
||||
// again short of navigating away and back. QueryErrorCard carries the same
|
||||
// message with a retry, matching the sibling custom-metric list.
|
||||
if (isError) {
|
||||
return (
|
||||
<p className="text-destructive py-8 text-center text-sm">
|
||||
{t("customMetrics.loadError")}
|
||||
</p>
|
||||
<QueryErrorCard
|
||||
description={t("customMetrics.loadError")}
|
||||
onRetry={() => void refetch()}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@ import { Leaf } from "lucide-react";
|
||||
|
||||
import { Card, CardContent, CardHeader } from "@/components/ui/card";
|
||||
import { EmptyState } from "@/components/ui/empty-state";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
import { TileHeader } from "@/components/insights/tile-header";
|
||||
import { useAuth } from "@/hooks/use-auth";
|
||||
@@ -53,7 +54,11 @@ export function MicronutrientsCard() {
|
||||
const sex =
|
||||
user?.gender === "MALE" || user?.gender === "FEMALE" ? user.gender : null;
|
||||
|
||||
const { data, isLoading } = useQuery({
|
||||
// A failed read left `data` undefined, `rows` empty, and the card rendered
|
||||
// its "no micronutrient data" EmptyState — the same output as an account that
|
||||
// genuinely logs no nutrients. The sibling hydration card already routes
|
||||
// through QueryErrorCard; this one now matches it.
|
||||
const { data, isLoading, isError, refetch } = useQuery({
|
||||
queryKey: queryKeys.nutrientIntake(WINDOW_DAYS),
|
||||
queryFn: () =>
|
||||
apiGet<NutrientIntakeOverview>(`/api/nutrients?days=${WINDOW_DAYS}`),
|
||||
@@ -72,7 +77,12 @@ export function MicronutrientsCard() {
|
||||
<TileHeader icon={Leaf} title={t("nutrients.micronutrients.title")} />
|
||||
</CardHeader>
|
||||
<CardContent className="space-y-3">
|
||||
{isLoading ? (
|
||||
{isError ? (
|
||||
<QueryErrorCard
|
||||
title={t("nutrients.micronutrients.loadError")}
|
||||
onRetry={() => void refetch()}
|
||||
/>
|
||||
) : isLoading ? (
|
||||
<div className="space-y-1" aria-hidden="true">
|
||||
<Skeleton className="h-9 w-full rounded-md" />
|
||||
<Skeleton className="h-9 w-full rounded-md" />
|
||||
|
||||
@@ -38,6 +38,8 @@ import { queryKeys } from "@/lib/query-keys";
|
||||
import { INSTRUMENTS, INSTRUMENT_ORDER } from "@/lib/mental-health/instruments";
|
||||
import { PageHeader } from "@/components/ui/page-header";
|
||||
import { ResponsiveSheet } from "@/components/ui/responsive-sheet";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import { Skeleton } from "@/components/ui/skeleton";
|
||||
|
||||
import { AssessmentResult } from "./assessment-result";
|
||||
import { CheckInWizard } from "./check-in-wizard";
|
||||
@@ -63,7 +65,20 @@ export function MentalWellbeing() {
|
||||
null,
|
||||
);
|
||||
|
||||
const { data: history } = useQuery({
|
||||
// The three states are kept apart deliberately. This read carries the user's
|
||||
// screening history; when it fails, `history` is undefined and every
|
||||
// downstream consumer (`lastByInstrument`, the detail sheet's `rows`) sees an
|
||||
// empty array — which used to render the instrument cards' "not taken yet"
|
||||
// copy. On a mental-health surface that is not a missing error message, it is
|
||||
// a false statement about the user's record: someone with months of
|
||||
// assessments was told the assessments do not exist. Error is now rendered as
|
||||
// error, loading as loading, and only a genuinely empty result as empty.
|
||||
const {
|
||||
data: history,
|
||||
isPending: historyPending,
|
||||
isError: historyError,
|
||||
refetch: refetchHistory,
|
||||
} = useQuery({
|
||||
queryKey: queryKeys.mentalHealthAssessments(),
|
||||
queryFn: () =>
|
||||
apiGet<{ assessments: AssessmentRow[] }>(
|
||||
@@ -131,18 +146,37 @@ export function MentalWellbeing() {
|
||||
|
||||
<section aria-label={t("mentalHealth.choosePrompt")}>
|
||||
<h2 className="sr-only">{t("mentalHealth.choosePrompt")}</h2>
|
||||
<ul className="grid list-none gap-4 p-0 sm:grid-cols-2">
|
||||
{INSTRUMENT_ORDER.map((id) => (
|
||||
<li key={id} className="contents">
|
||||
<InstrumentCard
|
||||
instrument={id}
|
||||
last={lastByInstrument.get(id)}
|
||||
onStart={() => begin(id)}
|
||||
onOpenDetail={() => setDetailInstrument(id)}
|
||||
/>
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
{historyError ? (
|
||||
<QueryErrorCard
|
||||
title={t("mentalHealth.historyLoadError")}
|
||||
description={t("mentalHealth.historyLoadErrorHint")}
|
||||
onRetry={() => void refetchHistory()}
|
||||
/>
|
||||
) : historyPending ? (
|
||||
<ul
|
||||
className="grid list-none gap-4 p-0 sm:grid-cols-2"
|
||||
aria-busy="true"
|
||||
>
|
||||
{INSTRUMENT_ORDER.map((id) => (
|
||||
<li key={id} className="contents">
|
||||
<Skeleton className="h-44 w-full rounded-xl" />
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
) : (
|
||||
<ul className="grid list-none gap-4 p-0 sm:grid-cols-2">
|
||||
{INSTRUMENT_ORDER.map((id) => (
|
||||
<li key={id} className="contents">
|
||||
<InstrumentCard
|
||||
instrument={id}
|
||||
last={lastByInstrument.get(id)}
|
||||
onStart={() => begin(id)}
|
||||
onOpenDetail={() => setDetailInstrument(id)}
|
||||
/>
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
)}
|
||||
</section>
|
||||
|
||||
{/* 2026-07-17 UX/IA audit M9 — mood tracking, this screener surface,
|
||||
|
||||
Reference in new issue
Block a user