fix(medications): identify an imported dose by when it was taken

The intake import built its replay key from the row's optional `zaehler`
field. That reads as a per-row counter — the documented example showed one —
but nothing required it to be unique and nothing checked. A reported export
carried the dose quantity there instead: 1 tablet, on all 28 rows. Every entry
queued under the same key, one dose reached the ledger, and the other 27 came
back reported as duplicates of a dose they had nothing to do with.

The key is the instant now, and `zaehler` is out of the contract rather than
accepted-and-ignored. Nothing is lost by dropping it: the live-row unique on
`(user_id, medication_id, scheduled_for, source)` already makes an imported
dose unique per instant, so a re-import of a file first imported under the old
key still dedups.

The telling is the other half. Two different facts were counted as one number:
entries of a submission that collide with each other, and doses the ledger
already holds. They are separate reasons now, grouped with a count, and the
dialog renders them through the same classifier and the same outcome line as
the CSV and JSON import cards — so an import that wrote nothing can no longer
answer in success tone. That rule shipped for the settings import panel and
this surface was not part of it; the guard now lists every import surface by
path, so a fourth one has to be added deliberately. `skippedInvalid` is gone:
it was hardcoded to zero on every path and reported a capability the importer
never had.

Alongside it, the other half of the environment report — a scheduled job that
fails every run has to say so. `recordError()` increments one unnamed counter,
so the admin status page could show a number and not which queue produced it,
and the module's own screen showed a zero day count with nothing to
distinguish "nothing stored yet" from "every run failed". pg-boss has held the
answer the whole time: a job that exhausts its retries keeps its queue name,
its error and its timestamp. The admin status now names the failing queues
with counts and the last error, the environment overview reports its own
account's failed runs, and neither needed a change at any of the thirty
`recordError()` call sites — a visibility fix that has to be remembered per
call site is the same failure one level up.

Refs #650, #646
This commit is contained in:
Marc-André Bombeck committed 2026-07-26 14:37:57 +02:00
1 parent a8d798442f
commit f0c5b5ae94
27 files changed
+1626 -154

No files matched your search

+15
View File
@@ -45,6 +45,14 @@ export interface WorkerStatus {
errors: number;
}
/** One queue with exhausted-retry failures, as `/api/admin/status` reports it. */
export interface FailingQueue {
queue: string;
failures: number;
lastFailedAt: string;
lastError: string;
}
export interface SystemStatus {
version: string;
nodeVersion: string;
@@ -53,6 +61,13 @@ export interface SystemStatus {
startTime: string;
database: string;
worker: WorkerStatus;
/**
* Queues whose jobs exhausted their retries recently. `null` means the
* deployment has no pg-boss schema to ask (web-only); an empty `queues`
* array means it was asked and nothing is failing. The two are rendered
* differently on purpose — absence is not health.
*/
failingJobs: { windowHours: number; queues: FailingQueue[] } | null;
counts: {
users: number;
measurements: number;
+84 -1
View File
@@ -23,7 +23,12 @@ import { formatDateTime } from "@/lib/format";
import { SettingsCard } from "@/components/settings/settings-card";
import { SettingsCardHeader } from "@/components/settings/_card-header";
import { useFormatters, useTranslations } from "@/lib/i18n/context";
import { StatusItem, usePublicVersion, useSystemStatus } from "./_shared";
import {
StatusItem,
usePublicVersion,
useSystemStatus,
type FailingQueue,
} from "./_shared";
// v1.4.16 phase B3: Recharts is ~108 KiB Brotli — defer-load the host
// metrics chart so the system-status page only ships it when an admin
@@ -43,6 +48,83 @@ const HostMetricsChart = dynamic(
},
);
/**
* The queues whose jobs gave up, named.
*
* Before this, a background job that threw on every single run showed up as one
* digit in an unnamed `errors` counter, and the module it fed rendered an empty
* screen as if empty were the answer. A broken queue has to say its own name,
* so this card is rendered whenever there is anything to report and stays
* silent otherwise — an operator should be able to trust that a quiet card
* means quiet queues, which is why the "cannot ask" case renders its own line
* rather than looking identical to health.
*/
function FailingJobsCard({
failingJobs,
}: {
failingJobs: { windowHours: number; queues: FailingQueue[] } | null;
}) {
const { t, tCount } = useTranslations();
if (failingJobs === null) {
return (
<SettingsCard>
<SettingsCardHeader
icon={AlertTriangle}
title={t("admin.failingJobs.title")}
description={t("admin.failingJobs.description")}
/>
<p className="text-muted-foreground mt-4 pl-7 text-sm">
{t("admin.failingJobs.unavailable")}
</p>
</SettingsCard>
);
}
return (
<SettingsCard>
<SettingsCardHeader
icon={AlertTriangle}
title={t("admin.failingJobs.title")}
description={t("admin.failingJobs.description")}
/>
{failingJobs.queues.length === 0 ? (
<p
data-testid="failing-jobs-none"
className="text-muted-foreground mt-4 pl-7 text-sm"
>
{t("admin.failingJobs.none", { hours: failingJobs.windowHours })}
</p>
) : (
<ul data-testid="failing-jobs-list" className="mt-4 space-y-3 pl-7">
{failingJobs.queues.map((queue) => (
<li key={queue.queue} className="text-sm">
<p className="text-foreground font-medium">
<span className="font-mono">{queue.queue}</span>{" "}
<span className="text-destructive font-normal">
{tCount("admin.failingJobs.count", queue.failures, {
count: queue.failures,
})}
</span>
</p>
<p className="text-muted-foreground text-xs">
{t("admin.failingJobs.lastFailed", {
when: formatDateTime(queue.lastFailedAt),
})}
</p>
{queue.lastError.length > 0 && (
<p className="text-muted-foreground mt-0.5 font-mono text-xs break-all">
{queue.lastError}
</p>
)}
</li>
))}
</ul>
)}
</SettingsCard>
);
}
export function SystemStatusSection() {
const { t } = useTranslations();
const fmt = useFormatters();
@@ -216,6 +298,7 @@ export function SystemStatusSection() {
</div>
)}
</SettingsCard>
{status && <FailingJobsCard failingJobs={status.failingJobs} />}
</div>
);
}
@@ -0,0 +1,145 @@
/**
* #650 — the intake import says what actually happened.
*
* The report was a file of 28 doses where 27 were "skipped" and the dialog
* answered in success tone with a duplicate count, so nothing on screen said
* the history had not landed. The cause is fixed elsewhere; this pins the
* telling, which is what turned a recoverable problem into a silent one.
*
* Project convention is SSR-only component tests (vitest runs `node`;
* `@testing-library/react` is not installed), so the view takes its state as a
* prop and each case is rendered directly.
*/
import { describe, expect, it } from "vitest";
import { renderToStaticMarkup } from "react-dom/server";
import { I18nProvider } from "@/lib/i18n/context";
import {
IntakeImportResultView,
type IntakeImportResultState,
} from "../intake-import-result";
/** `<li>` elements only — the lucide icon markup carries `<line>` tags. */
function countListItems(html: string): number {
return html.split("<li>").length - 1;
}
function render(state: IntakeImportResultState): string {
return renderToStaticMarkup(
<I18nProvider initialLocale="en">
<IntakeImportResultView result={state} />
</I18nProvider>,
);
}
/** What the reported run would have produced before the key was fixed. */
const ALL_SKIPPED: IntakeImportResultState = {
kind: "outcome",
imported: 0,
skipped: 27,
skipReasons: [{ reason: "duplicate_in_file", count: 27 }],
};
describe("<IntakeImportResultView> — an import that wrote nothing", () => {
it("renders as a failure, never a success", () => {
const html = render(ALL_SKIPPED);
expect(html).toContain('data-outcome="failed"');
expect(html).not.toContain('data-outcome="success"');
expect(html).toContain("Nothing was imported");
expect(html).toContain("27");
});
it("names the skip reason once with its count, not once per entry", () => {
const html = render(ALL_SKIPPED);
expect(html).toContain("27 entries skipped");
expect(html).toContain("same date and time");
// One list item for one reason — 27 entries must not become 27 lines.
expect(countListItems(html)).toBe(1);
});
it("separates the two skip reasons instead of calling both duplicates", () => {
const html = render({
kind: "outcome",
imported: 4,
skipped: 5,
skipReasons: [
{ reason: "already_recorded", count: 3 },
{ reason: "duplicate_in_file", count: 2 },
],
});
expect(html).toContain("3 entries skipped");
expect(html).toContain("already recorded");
expect(html).toContain("2 entries skipped");
expect(html).toContain("another entry in the same file");
expect(countListItems(html)).toBe(2);
});
});
describe("<IntakeImportResultView> — the other outcomes", () => {
it("renders a mixed run as a warning, not a tick", () => {
const html = render({
kind: "outcome",
imported: 26,
skipped: 2,
skipReasons: [{ reason: "already_recorded", count: 2 }],
});
expect(html).toContain('data-outcome="partial"');
expect(html).toContain("26 imported, 2 skipped");
});
it("renders a clean run as a success", () => {
const html = render({
kind: "outcome",
imported: 28,
skipped: 0,
skipReasons: [],
});
expect(html).toContain('data-outcome="success"');
expect(html).toContain("28 intakes imported");
expect(countListItems(html)).toBe(0);
});
it("gives a file with no intakes its own message rather than either", () => {
const html = render({
kind: "outcome",
imported: 0,
skipped: 0,
skipReasons: [],
});
expect(html).toContain('data-outcome="empty"');
expect(html).not.toContain('data-outcome="success"');
expect(html).toContain("no intakes");
});
it("falls back to a stated absence for an unrecognised reason", () => {
const html = render({
kind: "outcome",
imported: 0,
skipped: 1,
skipReasons: [{ reason: "something_new", count: 1 }],
});
expect(html).toContain("reason not recorded");
});
});
describe("<IntakeImportResultView> — notices are not outcomes", () => {
it("reports a loaded file in neutral tone, not as an import success", () => {
const html = render({
kind: "notice",
tone: "info",
text: 'File "intakes.json" loaded',
});
expect(html).toContain('data-tone="info"');
expect(html).not.toContain("text-success");
expect(html).not.toContain("data-outcome");
});
it("reports a refusal in destructive tone", () => {
const html = render({
kind: "notice",
tone: "error",
text: "Import failed",
});
expect(html).toContain("text-destructive");
});
});
@@ -21,6 +21,11 @@ import { useState } from "react";
import { useQueryClient } from "@tanstack/react-query";
import { Loader2, MoreHorizontal, RotateCcw, Upload } from "lucide-react";
import {
IntakeImportResultView,
type IntakeImportResultState,
} from "@/components/medications/intake-import-result";
import { Button } from "@/components/ui/button";
import { ResponsiveSheet } from "@/components/ui/responsive-sheet";
import {
@@ -51,10 +56,7 @@ export function IntakeImportDialog({
const { t } = useTranslations();
const [jsonText, setJsonText] = useState("");
const [importing, setImporting] = useState(false);
const [result, setResult] = useState<string | null>(null);
const [resultType, setResultType] = useState<"success" | "error" | null>(
null,
);
const [result, setResult] = useState<IntakeImportResultState | null>(null);
const [selectedFileName, setSelectedFileName] = useState<string | null>(null);
const [fileInputKey, setFileInputKey] = useState(0);
const queryClient = useQueryClient();
@@ -62,7 +64,6 @@ export function IntakeImportDialog({
function resetImportForm() {
setJsonText("");
setResult(null);
setResultType(null);
setSelectedFileName(null);
setFileInputKey((prev) => prev + 1);
}
@@ -77,18 +78,25 @@ export function IntakeImportDialog({
if (!file) return;
setResult(null);
setResultType(null);
try {
const content = await file.text();
JSON.parse(content);
setJsonText(content);
setSelectedFileName(file.name);
setResult(t("medications.importFileLoaded", { name: file.name }));
setResultType("success");
// A readable file is a step, not an import. It says so in neutral tone —
// the success affordance belongs to a run that actually wrote rows.
setResult({
kind: "notice",
tone: "info",
text: t("medications.importFileLoaded", { name: file.name }),
});
} catch {
setResult(t("medications.importInvalidJson"));
setResultType("error");
setResult({
kind: "notice",
tone: "error",
text: t("medications.importInvalidJson"),
});
}
}
@@ -96,7 +104,6 @@ export function IntakeImportDialog({
if (!medicationId || !jsonText.trim()) return;
setImporting(true);
setResult(null);
setResultType(null);
try {
let data = JSON.parse(jsonText.trim());
@@ -114,37 +121,41 @@ export function IntakeImportDialog({
statusUrl: string;
}>(`/api/medications/${medicationId}/intake/import`, data);
const d = await waitForMedicationIntakeImport(kickoff.statusUrl);
setResult(
t("medications.importResult", { imported: d.imported }) +
(d.skippedDuplicates > 0
? `, ${t("medications.importDuplicatesSkipped", { count: d.skippedDuplicates })}`
: "") +
(d.skippedInvalid > 0
? `, ${t("medications.importInvalidSkipped", { count: d.skippedInvalid })}`
: ""),
);
setResultType("success");
setResult({
kind: "outcome",
imported: d.imported,
skipped: d.skipped,
skipReasons: d.skipReasons,
});
// An import can land today-dated doses, so clear the Today hero +
// dashboard through the blessed helper (they are unmounted behind the
// import dialog on the detail page).
void invalidateMedicationReads(queryClient);
} catch (err) {
if (err instanceof MedicationIntakeImportError) {
setResult(t("medications.importFailed"));
setResultType("error");
setResult({
kind: "notice",
tone: "error",
text: t("medications.importFailed"),
});
return;
}
if (!(err instanceof ApiError)) throw err;
setResult(err.message || t("medications.importFailed"));
setResultType("error");
setResult({
kind: "notice",
tone: "error",
text: err.message || t("medications.importFailed"),
});
}
} catch (err) {
setResult(
err instanceof SyntaxError
? t("medications.importInvalidFormat")
: (err as Error).message || t("medications.importFailed"),
);
setResultType("error");
setResult({
kind: "notice",
tone: "error",
text:
err instanceof SyntaxError
? t("medications.importInvalidFormat")
: (err as Error).message || t("medications.importFailed"),
});
} finally {
setImporting(false);
}
@@ -226,8 +237,8 @@ export function IntakeImportDialog({
</div>
<pre className="bg-muted text-muted-foreground rounded-lg p-3 text-xs">
{`[
{"datum": "2026-02-14", "uhrzeit": "10:27:43", "zaehler": 523},
{"datum": "2026-02-14", "uhrzeit": "23:33:42", "zaehler": 524}
{"datum": "2026-02-14", "uhrzeit": "10:27:43"},
{"datum": "2026-02-14", "uhrzeit": "23:33:42"}
]`}
</pre>
<Textarea
@@ -239,15 +250,9 @@ export function IntakeImportDialog({
spellCheck={false}
className="font-mono"
/>
{result && (
<p
className={`text-sm ${resultType === "success" ? "text-success" : "text-destructive"}`}
role="status"
aria-live="polite"
>
{result}
</p>
)}
<div role="status" aria-live="polite">
{result && <IntakeImportResultView result={result} />}
</div>
</div>
</ResponsiveSheet>
);
@@ -0,0 +1,126 @@
"use client";
/**
* What the intake-import dialog says once an attempt has finished.
*
* Split out of the dialog so the outcome can be rendered and asserted on its
* own — the dialog itself is a sheet with a query client and a file input, and
* the thing that has to be right is the sentence at the end of the run.
*
* The rules are the ones the settings import panel already follows, reused
* rather than restated: `classifyImportOutcome` decides the tone from the
* counts, so an import that wrote nothing can never carry the success tick,
* and `ImportOutcomeLine` owns the tick and the success colour. The skips are
* grouped by reason with a count, because a file that trips one rule trips it
* on every row and a per-row list is the same sentence a hundred times.
*
* This dialog was not covered when that work landed for the CSV and JSON
* cards; the medication intake import is the third import surface and it now
* answers the same way.
*/
import {
classifyImportOutcome,
type ImportOutcome,
} from "@/components/settings/import-panel/import-result-state";
import { ImportOutcomeLine } from "@/components/settings/import-panel/import-result-view";
import { useTranslations } from "@/lib/i18n/context";
/** A skip group exactly as the import job reports it. */
export interface IntakeImportSkipGroup {
reason: string;
count: number;
}
/**
* `notice` covers everything that happened before the importer had an opinion —
* a file was read, the JSON did not parse, the request was refused. `outcome`
* is the import itself.
*/
export type IntakeImportResultState =
| { kind: "notice"; tone: "info" | "error"; text: string }
| {
kind: "outcome";
imported: number;
skipped: number;
skipReasons: IntakeImportSkipGroup[];
};
/**
* Plain language per machine-readable skip reason. Written as literal `t(…)`
* calls rather than a computed key so the i18n call-site coverage guard sees
* every one of them.
*/
function useSkipReasonLabel(): (reason: string) => string {
const { t } = useTranslations();
const labels: Record<string, string> = {
duplicate_in_file: t("medications.import.reasonDuplicateInFile"),
already_recorded: t("medications.import.reasonAlreadyRecorded"),
};
const fallback = t("medications.import.reasonUnknown");
return (reason: string) => labels[reason] ?? fallback;
}
export function IntakeImportResultView({
result,
}: {
result: IntakeImportResultState;
}) {
const { t, tCount } = useTranslations();
const labelFor = useSkipReasonLabel();
if (result.kind === "notice") {
return (
<p
data-testid="intake-import-notice"
data-tone={result.tone}
className={`text-sm ${result.tone === "error" ? "text-destructive" : "text-muted-foreground"}`}
>
{result.text}
</p>
);
}
const outcome: ImportOutcome = classifyImportOutcome({
written: result.imported,
skipped: result.skipped,
});
const message = (() => {
if (outcome === "empty") return t("medications.import.resultEmpty");
if (outcome === "failed") {
return t("medications.import.resultNothing", { skipped: result.skipped });
}
if (outcome === "partial") {
return t("medications.import.resultPartial", {
imported: result.imported,
skipped: result.skipped,
});
}
return t("medications.import.resultSuccess", { imported: result.imported });
})();
return (
<div data-testid="intake-import-result" className="space-y-1.5">
<ImportOutcomeLine
outcome={outcome}
message={message}
testId="intake-import-outcome"
/>
{result.skipReasons.length > 0 && (
<ul
data-testid="intake-import-skip-groups"
className="text-muted-foreground space-y-0.5 text-xs"
>
{result.skipReasons.map((group) => (
<li key={group.reason}>
{tCount("medications.import.skipGroup", group.count, {
count: group.count,
reason: labelFor(group.reason),
})}
</li>
))}
</ul>
)}
</div>
);
}
@@ -28,6 +28,7 @@ import { Input } from "@/components/ui/input";
import { Label } from "@/components/ui/label";
import { useAuth } from "@/hooks/use-auth";
import { ConfirmButton } from "@/components/settings/confirm-button";
import { formatDateTime } from "@/lib/format";
import { useTranslations } from "@/lib/i18n/context";
import { apiDelete, apiGet, apiPost, apiPut } from "@/lib/api/api-fetch";
import { queryKeys } from "@/lib/query-keys";
@@ -64,6 +65,10 @@ interface EnvironmentOverview {
latestDate: string | null;
latestFetchedAt: string | null;
};
/** Set when the background weather fetch for this account recently gave up.
* A zero day count alone cannot distinguish "nothing to store yet" from
* "every run failed", so the count is never rendered on its own. */
lastFetchFailure: { lastFailedAt: string; failures: number } | null;
attribution: string;
}
@@ -72,7 +77,7 @@ function todayKey(): string {
}
export function EnvironmentSection() {
const { t } = useTranslations();
const { t, tCount } = useTranslations();
const { user } = useAuth();
const queryClient = useQueryClient();
@@ -500,6 +505,22 @@ export function EnvironmentSection() {
})}
</p>
)}
{data?.lastFetchFailure && (
<p
role="status"
data-testid="environment-fetch-failed"
className="text-destructive mt-1 pl-7 text-xs"
>
{tCount(
"settings.sections.environment.fetchFailed",
data.lastFetchFailure.failures,
{
count: data.lastFetchFailure.failures,
when: formatDateTime(data.lastFetchFailure.lastFailedAt),
},
)}
</p>
)}
</SettingsCard>
{/* Attribution (CC BY 4.0, required) */}
@@ -7,6 +7,13 @@
* exactly one key of the outcome table, and no card reaches for them
* directly. A future card that hand-rolls a green tick fails here rather than
* shipping and lying to a self-hoster about their sensor history.
*
* #650 widened it. The medication intake import is a third import surface, in
* another directory, and it was not covered when the rule landed — it kept its
* own success/error tone and reported a run that wrote one row out of
* twenty-eight as a success. "Every import surface" now means every one of
* them, listed by path, so a fourth surface has to be added here deliberately
* rather than inheriting the old behaviour by being somewhere else.
*/
import { describe, it, expect } from "vitest";
import { readFileSync, readdirSync } from "node:fs";
@@ -14,6 +21,34 @@ import { join } from "node:path";
const PANEL_DIR = join(__dirname, "..");
const PRESENTATION_FILE = "import-result-view.tsx";
const REPO_ROOT = join(__dirname, "..", "..", "..", "..", "..");
/**
* Every surface that reports an import result, and the module each one must
* render its outcome through. Paths from the repository root.
*/
const IMPORT_SURFACES: ReadonlyArray<{ path: string; through: RegExp }> = [
{
path: "src/components/settings/import-panel/csv-import-card.tsx",
through: /CsvImportResultView|ImportOutcomeLine/,
},
{
path: "src/components/settings/import-panel/json-import-card.tsx",
through: /CsvImportResultView|ImportOutcomeLine/,
},
{
path: "src/components/settings/import-panel/apple-health-import-card.tsx",
through: /CsvImportResultView|ImportOutcomeLine/,
},
{
path: "src/components/medications/intake-import-result.tsx",
through: /ImportOutcomeLine/,
},
{
path: "src/components/medications/intake-import-dialog.tsx",
through: /IntakeImportResultView/,
},
];
/** The success affordance: the tick component and the success colour token. */
const SUCCESS_MARKERS = ["CheckCircle2", "text-success"] as const;
@@ -25,10 +60,16 @@ function panelSourceFiles(): string[] {
}
/** Code only — a comment naming the marker is prose, not an affordance. */
function stripComments(source: string): string {
return source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^\s*\/\/.*$/gm, "");
}
function readCode(name: string): string {
return readFileSync(join(PANEL_DIR, name), "utf8")
.replace(/\/\*[\s\S]*?\*\//g, "")
.replace(/^\s*\/\/.*$/gm, "");
return stripComments(readFileSync(join(PANEL_DIR, name), "utf8"));
}
function readRepoCode(relativePath: string): string {
return stripComments(readFileSync(join(REPO_ROOT, relativePath), "utf8"));
}
describe("import panel — success affordance is owned by the outcome table", () => {
@@ -63,13 +104,27 @@ describe("import panel — success affordance is owned by the outcome table", ()
}
});
it("routes every card's result line through the shared outcome line", () => {
for (const name of [
"csv-import-card.tsx",
"json-import-card.tsx",
"apple-health-import-card.tsx",
]) {
expect(readCode(name)).toMatch(/CsvImportResultView|ImportOutcomeLine/);
it("routes every import surface's result line through the shared outcome line", () => {
for (const surface of IMPORT_SURFACES) {
expect(
readRepoCode(surface.path),
`${surface.path} must render its outcome through the shared line`,
).toMatch(surface.through);
}
});
it("keeps the success markers out of every import surface", () => {
const offenders = IMPORT_SURFACES.filter((surface) =>
SUCCESS_MARKERS.some((marker) =>
readRepoCode(surface.path).includes(marker),
),
).map((surface) => surface.path);
expect(
offenders,
offenders.length > 0
? `Import surface(s) hand-rolling the success affordance instead of ` +
`classifying the counts: ${offenders.join(", ")}`
: undefined,
).toEqual([]);
});
});