From ebf032158e17cc9bb90c0408680fa435b002e34d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 3 Aug 2026 18:47:42 +0200 Subject: [PATCH] Give the intake toast one decision, and stop admitting two writes nothing calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three surfaces record a dose and all three had to reach the same two conclusions inside somebody else's record: name the record, and drop an Undo the server refuses. Two learned it and the dose ledger did not, because the ternary was written out three times. It is written once now, in the file the other two already share, with its own test. POST /api/allergies and POST /api/family-history leave the frozen delegable write set. The argument that admitted them still stands — an allergy is the single most useful thing a caregiver can contribute — but the only surface that posts to either lives in Settings, which a switch closes, so no delegate could reach the form at any level. That is a permission frozen ahead of the caller for it, and the list is built the other way round on purpose. Both delegable READ arms stay. A caregiver reading the allergy list is what the feature is for; only the contribute step waits, and it comes back in the same diff as the surface that offers it. --- e2e/account-sharing.spec.ts | 29 +++++++ e2e/delegated-writes.spec.ts | 22 +++++ src/__tests__/delegable-surface-guard.test.ts | 21 +++-- .../success-affordance-guard.test.ts | 4 +- src/app/api/allergies/route.ts | 37 ++++++-- src/app/api/family-history/route.ts | 23 +++-- .../__tests__/use-medication-intake.test.ts | 65 ++++++++++++++ .../medications/dose-history-ledger.tsx | 39 ++++----- .../medications/use-medication-intake.ts | 87 +++++++++++++------ 9 files changed, 262 insertions(+), 65 deletions(-) diff --git a/e2e/account-sharing.spec.ts b/e2e/account-sharing.spec.ts index b7e0a7984..61fa91c58 100644 --- a/e2e/account-sharing.spec.ts +++ b/e2e/account-sharing.spec.ts @@ -386,6 +386,35 @@ test.describe("account sharing", () => { ).toBeVisible(); }); + test("a query parameter opens nothing the button withholds", async ({ + page, + }) => { + // Three deep links reach a create sheet without passing the control that + // normally opens it. A parameter that opens a sheet is the same affordance + // as the button, and the SSR suite cannot reach any of them: it holds the + // paint of a component, never a URL. + await page.goto("/measurements?add=WEIGHT"); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toBeVisible(); + await expect( + page.locator('[data-slot="responsive-sheet-content"]'), + ).toHaveCount(0); + // And the button it stands in for is gone too, so nothing on this page + // offers the form by either route. + await expect( + page.getByRole("button", { name: /add measurement/i }), + ).toHaveCount(0); + + await page.goto("/medications?new=1"); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toBeVisible(); + await expect( + page.locator('[data-slot="medication-wizard-dialog"]'), + ).toHaveCount(0); + }); + test("revoking from the owner's browser drops the delegate out", async ({ page, }) => { diff --git a/e2e/delegated-writes.spec.ts b/e2e/delegated-writes.spec.ts index 34bd0c476..e045f9448 100644 --- a/e2e/delegated-writes.spec.ts +++ b/e2e/delegated-writes.spec.ts @@ -180,6 +180,28 @@ test.describe("delegated writes", () => { ); }); + test("a deep link opens exactly what the level admits", async ({ page }) => { + // The gate binds to the level the server resolved, not to a blanket + // "somebody else's record" flag. Both halves matter and only a browser + // can show either: the SSR suite holds a component's paint, never a URL. + // + // Admitted: entering a reading, so `?add=` opens the same sheet the + // header button opens. + await page.goto("/measurements?add=WEIGHT"); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toBeVisible(); + await expect( + page.locator('[data-slot="responsive-sheet-content"]').first(), + ).toBeVisible(); + + // Also admitted: adding a medication with its schedule. + await page.goto("/medications?new=1"); + await expect( + page.locator('[data-slot="medication-wizard-dialog"]'), + ).toBeVisible(); + }); + test("the owner sees that somebody else was in their record", async () => { await ownerPage.goto("/settings/access"); const rows = ownerPage.locator('[data-slot="record-activity-row"]'); diff --git a/src/__tests__/delegable-surface-guard.test.ts b/src/__tests__/delegable-surface-guard.test.ts index 532eb962a..144a4c6b7 100644 --- a/src/__tests__/delegable-surface-guard.test.ts +++ b/src/__tests__/delegable-surface-guard.test.ts @@ -428,11 +428,11 @@ const DELEGABLE_ROUTES: Record = { "app/api/biomarkers/[id]/route.ts": "One biomarker of the record, fetch-then-guard against the resolved user.", "app/api/allergies/route.ts": - "The record's allergy list — the single most useful thing a caregiver can read, and a plain list of the owner's own rows.", + "The record's allergy list — the single most useful thing a caregiver can read, and a plain list of the owner's own rows. The POST beside it is NOT delegable and is deliberately absent from the write literal below: the only surface that posts to it lives under `/settings`, which a switch closes, so admitting the write would freeze a permission ahead of any caller for it. The route comment carries the argument.", "app/api/allergies/[id]/route.ts": "One allergy of the record, fetch-then-guard against the resolved user.", "app/api/family-history/route.ts": - "The record's family history. The payload describes the owner's relatives, so it is the one admitted read where third-party health information is present by design rather than by accident; a caregiver reading it is the use the feature exists for, and the row is stored as the owner's.", + "The record's family history. The payload describes the owner's relatives, so it is the one admitted read where third-party health information is present by design rather than by accident; a caregiver reading it is the use the feature exists for, and the row is stored as the owner's. Its POST is not delegable, for the reason its allergy sibling gives plus one of its own — see the route comment.", "app/api/family-history/[id]/route.ts": "One family-history entry of the record, fetch-then-guard against the resolved user.", "app/api/mental-health/assessments/route.ts": @@ -512,6 +512,17 @@ const DELEGABLE_ROUTES: Record = { * thirty-one read-only delegable modules do not do, and it is the difference * the identifier matcher above cannot see. * + * v1.36.x — `POST /api/allergies` and `POST /api/family-history` left this + * list, and the removal is worth reading before either is proposed again. The + * argument for admitting them was never wrong; what they lacked was a caller. + * The only surface in the product that posts to either lives in Settings → + * Anamnese, and a switch closes `/settings` — so a delegate could not reach + * the form at any grant level, and the two entries were a permission frozen + * ahead of the surface that would exercise it. Both delegable READ arms stay: + * a caregiver reading the allergy list is what the feature is for. Whoever + * builds a caregiver-reachable medical-history surface adds them back in the + * same diff, which is the two-ended change this list is meant to hold to. + * * Every member also has to call `auditLog`, asserted below. That is not a * stylistic preference. The decision not to add a `writtenBy` column to eleven * tables rests entirely on the audit trail carrying the actor, and `auditLog` @@ -528,10 +539,6 @@ const DELEGABLE_WRITE_ROUTES: Record = { "Entering a lab result. The free-text path may mint a biomarker, and mints it into the RECORD's catalogue — a result added to somebody's record that left the marker on the helper's account would be worse than useless to the owner.", "app/api/biomarkers/route.ts": "Adding an analyte to the record's catalogue. A name the record already tracks is the ordinary 409 from the same `(userId, name)` uniqueness the owner would hit themselves.", - "app/api/allergies/route.ts": - "Adding an allergy. The cleanest admission in the set: a plain statement about the record's own body, and the single most useful thing a caregiver can contribute.", - "app/api/family-history/route.ts": - "Adding a family-history entry. Third-party health data by design — the row describes the record's relatives — and it is stored as the record's own, which is exactly how the delegable read arm already serves it.", "app/api/illness/episodes/route.ts": "Opening an illness episode. The module gate runs against the RECORD before the write, so a delegate cannot create an episode inside a record whose owner switched the module off.", "app/api/custom-metrics/[id]/entries/route.ts": @@ -569,7 +576,7 @@ const ACTOR_ROUTES: Record = { * stay a formality by accident: every addition has to be counted here as well * as listed above, which is one more place a careless admission has to pass. */ -const FROZEN_ENTRY_COUNT = 57; +const FROZEN_ENTRY_COUNT = 55; /** * The two surfaces that authenticate a Bearer token outside `requireAuth` — diff --git a/src/__tests__/success-affordance-guard.test.ts b/src/__tests__/success-affordance-guard.test.ts index 23bc53a73..ac13ec592 100644 --- a/src/__tests__/success-affordance-guard.test.ts +++ b/src/__tests__/success-affordance-guard.test.ts @@ -256,7 +256,9 @@ const PINNED_AFFORDANCES: Record< "toast.success": 2, }, "src/components/medications/take-all-due.ts": { "toast.success": 1 }, - "src/components/medications/use-medication-intake.ts": { "toast.success": 5 }, + // v1.36.x — one fewer: the log-intake path's three-armed toast collapsed + // into the shared `intakeToastOptions` decision plus a single call pair. + "src/components/medications/use-medication-intake.ts": { "toast.success": 4 }, "src/components/medications/wizard/medication-wizard-dialog.tsx": { "toast.success": 1, }, diff --git a/src/app/api/allergies/route.ts b/src/app/api/allergies/route.ts index f29719ded..8225b7e0d 100644 --- a/src/app/api/allergies/route.ts +++ b/src/app/api/allergies/route.ts @@ -11,7 +11,7 @@ import { NextRequest } from "next/server"; import { prisma } from "@/lib/db"; -import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; +import { apiHandler, requireAuth, requireRecordAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { @@ -67,10 +67,37 @@ export const GET = apiHandler(async (request: NextRequest) => { export const POST = apiHandler(withIdempotency<[NextRequest]>(postAllergy)); async function postAllergy(request: NextRequest): Promise { - // v1.36.x — a delegated write, and the cleanest of them: the row is a plain - // statement about the record's own body, and the caller appears in the audit - // trail rather than in the row. - const { user } = await requireRecordAuth("write"); + // v1.36.x — the GET above is delegable and this is not, which is the + // opposite of where the classification landed and worth the paragraph. + // + // Nothing about the row changed: it is still a plain statement about the + // record's own body, still the single most useful thing a caregiver could + // contribute, and the argument for admitting it still holds. What it does + // not have is a caller. The only place in the product that posts here is the + // allergy manager in Settings → Anamnese, and `/settings/*` is not a + // shared-record destination — the shell shows the "not part of what was + // shared" panel there, so no delegate can reach the form at any level. + // + // An admitted write with no reachable surface is the one-ended change this + // repository keeps rediscovering (CLAUDE.md, "A two-ended change carries + // both ends"): the permission ships, the consumer is the follow-up, and + // nothing in the gate notices because every other check proves the other + // end. The frozen list is built the other way round on purpose — its own + // actor-surface note says the rest "arrive as their own diffs; naming them + // before they exist would freeze a guess." + // + // So this arm waits for the surface that would exercise it, and the two + // land together. Choosing that surface is design work, not a fix: allergies + // and family history have exactly one home today, that home is a personal + // account surface a switch rightly closes, and bolting a second copy onto a + // shared page would split one concept across two places. Re-admitting is + // one line here plus one entry in `delegable-surface-guard.test.ts` plus the + // paragraph that argues it — which is exactly the reviewed diff that guard + // exists to force. + // + // The half that was always the point is untouched: a caregiver can still + // READ the allergy list inside the record. + const { user } = await requireAuth(); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, diff --git a/src/app/api/family-history/route.ts b/src/app/api/family-history/route.ts index d560de87e..f3b3e22bb 100644 --- a/src/app/api/family-history/route.ts +++ b/src/app/api/family-history/route.ts @@ -12,7 +12,7 @@ import { NextRequest } from "next/server"; import { prisma } from "@/lib/db"; -import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; +import { apiHandler, requireAuth, requireRecordAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { @@ -62,11 +62,22 @@ export const POST = apiHandler( ); async function postFamilyHistory(request: NextRequest): Promise { - // v1.36.x — a delegated write, and the one where third-party health data is - // present by design: the row describes the record's relatives. It is stored - // as the record's own, exactly as the delegable READ arm above already - // serves it, and the audit trail names who entered it. - const { user } = await requireRecordAuth("write"); + // v1.36.x — not a delegated write, though the GET above is delegable. The + // reasoning is written out at the sibling arm in `api/allergies/route.ts` + // and is the same here: the only surface that posts to either route is the + // manager in Settings → Anamnese, and `/settings/*` is closed inside a + // shared record, so no delegate can reach the form at any level. An + // admitted write with no reachable caller is a permission frozen ahead of + // the surface that would exercise it. + // + // This arm carried an extra reason to wait. The row describes the record's + // RELATIVES, so it is the one admitted write where third-party health + // information is present by design — and frequently the delegate is that + // relative, asserting a condition about themselves into somebody else's + // record. The classification admitted it and named the discomfort. Landing + // it together with the surface that offers it means the copy on that surface + // can say whose statement the row is, which no route comment can. + const { user } = await requireAuth(); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, diff --git a/src/components/medications/__tests__/use-medication-intake.test.ts b/src/components/medications/__tests__/use-medication-intake.test.ts index 8a5572b38..b73ca1e8d 100644 --- a/src/components/medications/__tests__/use-medication-intake.test.ts +++ b/src/components/medications/__tests__/use-medication-intake.test.ts @@ -3,6 +3,7 @@ import type { QueryClient } from "@tanstack/react-query"; import { toast } from "sonner"; import { + intakeToastOptions, runLogIntake, runRecordIntake, runUndoIntake, @@ -475,3 +476,67 @@ describe("runUndoIntake — shared soft-delete", () => { expect(toast.success).not.toHaveBeenCalled(); }); }); + +/** + * v1.36.x — the one decision every intake surface shares. + * + * Three surfaces record a dose and all three showed the same success toast: + * the cards, the log-intake dialog, and the dose-history ledger. Two of them + * learned to name the record and drop Undo inside somebody else's; the ledger + * built its own copy of the ternary and learned neither, so a delegate met a + * dead Undo on every dose they marked. This is the copy that is left. + * + * Whole objects rather than field probes: when one person's record is being + * separated from another's, a failure should print what would have been shown + * rather than `false !== true`. + * + * Mutation check, run: making `intakeToastOptions` ignore `recordName` → the + * two shared-record legs go red, printing the Undo action they returned. + */ +describe("intakeToastOptions — the shared toast decision", () => { + const onUndo = vi.fn(); + + it("carries an Undo in the caller's own record", () => { + expect( + intakeToastOptions({ recordName: null, eventId: "evt-1", t, onUndo }), + ).toEqual({ + action: { + label: "medications.intakeUndo", + onClick: expect.any(Function), + }, + }); + }); + + it("names the record and offers no Undo inside somebody else's", () => { + expect( + intakeToastOptions({ + recordName: "Margarethe", + eventId: "evt-1", + t, + onUndo, + }), + ).toEqual({ description: "recordSharing.toast.savedTo:Margarethe" }); + }); + + it("still names the record when the write returned no event id", () => { + // The two arms are not alternatives: the receipt is owed either way, and + // an absent event id must not fall through to a bare "Saved". + expect( + intakeToastOptions({ + recordName: "Margarethe", + eventId: undefined, + t, + onUndo, + }), + ).toEqual({ description: "recordSharing.toast.savedTo:Margarethe" }); + }); + + it("carries nothing when there is neither a record to name nor an undo", () => { + expect( + intakeToastOptions({ recordName: null, eventId: undefined, t, onUndo }), + ).toBeUndefined(); + expect( + intakeToastOptions({ recordName: null, eventId: "evt-1", t }), + ).toBeUndefined(); + }); +}); diff --git a/src/components/medications/dose-history-ledger.tsx b/src/components/medications/dose-history-ledger.tsx index 5cd248af9..aa6b73e4f 100644 --- a/src/components/medications/dose-history-ledger.tsx +++ b/src/components/medications/dose-history-ledger.tsx @@ -94,7 +94,10 @@ import { refetchInactiveDailyReads, } from "@/lib/query-keys"; import type { DoseHistoryStatus } from "@/lib/medications/scheduling/dose-history"; -import { runUndoIntake } from "@/components/medications/use-medication-intake"; +import { + intakeToastOptions, + runUndoIntake, +} from "@/components/medications/use-medication-intake"; import { IntakeEditDialog } from "@/components/medications/intake-edit-dialog"; import { LedgerAddDialog } from "@/components/medications/dose-history-add-dialog"; import { @@ -291,26 +294,20 @@ export function DoseHistoryLedger({ : "medications.intakeToastTaken", { name: medicationName }, ), - recordName - ? { - description: t("recordSharing.toast.savedTo", { - name: recordName, - }), - } - : eventId - ? { - action: { - label: t("medications.intakeUndo"), - onClick: () => - void runUndoIntake({ - medication: { id: medicationId, name: medicationName }, - eventId, - t, - queryClient, - }), - }, - } - : undefined, + // The shared decision, not a fourth copy of it: name the record it + // landed in, and withhold an Undo the server would refuse there. + intakeToastOptions({ + recordName, + eventId, + t, + onUndo: (id) => + void runUndoIntake({ + medication: { id: medicationId, name: medicationName }, + eventId: id, + t, + queryClient, + }), + }), ); await invalidateKeys(queryClient, [ ...medicationDependentKeys, diff --git a/src/components/medications/use-medication-intake.ts b/src/components/medications/use-medication-intake.ts index f9ef5dfab..bf1abbdaa 100644 --- a/src/components/medications/use-medication-intake.ts +++ b/src/components/medications/use-medication-intake.ts @@ -29,6 +29,54 @@ interface MedicationIntakeIdentity { name: string; } +/** + * What rides alongside a successful intake toast — and the one place that + * decides it. + * + * Three surfaces record a dose: the medication cards through + * {@link runRecordIntake}, the log-intake dialog through {@link runLogIntake}, + * and the dose-history ledger, which builds its own POST because it also has + * an optimistic cache patch to make. All three showed the same success toast + * and all three had to reach the same two conclusions: + * + * - inside somebody else's record, name the record. "Saved" alone is the one + * confirmation a person acting for somebody else does not need. + * - and drop Undo there. A delegate may record a dose and may not remove + * one, so an Undo they can see is an Undo the server refuses. + * + * Two of the three learned that; the ledger did not, which is a delegate + * meeting a dead Undo on every dose they mark. Three copies of one ternary is + * how that happens, so there is one now. + */ +export function intakeToastOptions(input: { + /** The record the dose landed in, or null in the caller's own. */ + recordName: string | null | undefined; + /** The created event, when the POST returned one. */ + eventId: string | undefined; + t: Translator; + /** Reverse the write. Omitted where the caller offers no undo at all. */ + onUndo?: (eventId: string) => void; +}): + | { description: string } + | { action: { label: string; onClick: () => void } } + | undefined { + const { recordName, eventId, t, onUndo } = input; + if (recordName) { + return { + description: t("recordSharing.toast.savedTo", { name: recordName }), + }; + } + if (eventId && onUndo) { + return { + action: { + label: t("medications.intakeUndo"), + onClick: () => onUndo(eventId), + }, + }; + } + return undefined; +} + /** * v1.12.2 — the take / skip + Undo intake orchestration shared by the * generic {@link MedicationCard} and the {@link Glp1MedicationCard}. @@ -146,20 +194,12 @@ export async function runRecordIntake(deps: { : "medications.intakeToastTaken", { name: medication.name }, ), - recordName - ? { - description: t("recordSharing.toast.savedTo", { - name: recordName, - }), - } - : eventId - ? { - action: { - label: t("medications.intakeUndo"), - onClick: () => void undoIntake(eventId), - }, - } - : undefined, + intakeToastOptions({ + recordName, + eventId, + t, + onUndo: (id) => void undoIntake(id), + }), ); await invalidateMedicationReads(queryClient); onRecorded?.(eventId, skipped); @@ -248,17 +288,14 @@ export async function runLogIntake(deps: { // a real Undo action to attach; keeps the no-undo call signature // identical to the pre-fix behaviour (existing unit tests assert the // single-argument call). - if (recordName) { - toast.success(message, { - description: t("recordSharing.toast.savedTo", { name: recordName }), - }); - } else if (eventId && undoIntake) { - toast.success(message, { - action: { - label: t("medications.intakeUndo"), - onClick: () => void undoIntake(eventId), - }, - }); + const options = intakeToastOptions({ + recordName, + eventId, + t, + onUndo: undoIntake ? (id) => void undoIntake(id) : undefined, + }); + if (options) { + toast.success(message, options); } else { toast.success(message); }