From 42100d94a71bc5a2f56c8dd55367544293fc0b70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 05:07:02 +0200 Subject: [PATCH] fix(api): finish the pagination tiebreaker, and state the tie premise correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment on the measurements list justified the tiebreaker with an Apple Health import stamping long runs with one instant, and said the unique index forbids two live measurements of the same type at that instant. The index is `(userId, type, measuredAt, source, sleepStage)`, so the named example is the one case it does block — while ties survive across `type` (the `stats:` daily rollups), across `source` (two writers landing together) and across `sleepStage`. Same conclusion, honest premise. The per-day drill-down on the same route still sorted on `measuredAt` alone. It slices in memory rather than with `skip`, so nothing drifts between pages, but two identical requests were free to render a tied block in different orders, and a list that reshuffles on refresh reads as data changing. Three of the tiebreakered routes had nothing pinning the order: custom-metric entries, the per-medication intake history (both sort arms, so the NULLS LAST pin cannot be lost to the tiebreaker) and the mental-health assessment history. Each now asserts the `orderBy` the way the illness day-log test does. --- .../[id]/entries/__tests__/route.test.ts | 10 ++++++- src/app/api/measurements/route.ts | 17 +++++++---- .../[id]/intake/__tests__/route.test.ts | 21 +++++++++++++ .../assessments/__tests__/route.test.ts | 30 +++++++++++++++++-- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts b/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts index d02cd4a08..c13967317 100644 --- a/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts +++ b/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts @@ -90,10 +90,18 @@ describe("GET /api/custom-metrics/[id]/entries", () => { expect(body.data.meta.total).toBe(5); expect(body.data.meta.limit).toBe(1); const findCall = vi.mocked(prisma.customMetricEntry.findMany).mock - .calls[0][0] as { where: Record; take: number }; + .calls[0][0] as { + where: Record; + take: number; + orderBy: unknown; + }; expect(findCall.where.userId).toBe("user-1"); expect(findCall.where.customMetricId).toBe("cm-1"); expect(findCall.take).toBe(1); + // `measuredAt` is not unique on a custom metric — a batch of readings can + // share one instant — so offset paging over it alone can repeat a row on + // one page and drop another. + expect(findCall.orderBy).toEqual([{ measuredAt: "desc" }, { id: "desc" }]); }); it("404s a cross-user / unknown metric", async () => { diff --git a/src/app/api/measurements/route.ts b/src/app/api/measurements/route.ts index 87b22745d..6dcfe5456 100644 --- a/src/app/api/measurements/route.ts +++ b/src/app/api/measurements/route.ts @@ -190,7 +190,12 @@ export const GET = apiHandler(async (request: NextRequest) => { measuredAt: { gte: dayStart, lt: dayEnd }, deletedAt: null, }, - orderBy: { measuredAt: sortDir }, + // Same tiebreaker as the paged list. This one slices in memory rather + // than with `skip`, so there is no page drift to prevent — but two + // identical requests would still be free to render a tied block in + // different orders, and a drill-down that reshuffles on refresh reads + // as data changing. + orderBy: [{ measuredAt: sortDir }, { id: sortDir }], // v1.4.38 — the 1000-row cap now lives on the validator (refine // on `(limit, dayKey)` returning 422 when a caller asks for // more). The route reads `limit` straight through because the @@ -545,10 +550,12 @@ export const GET = apiHandler(async (request: NextRequest) => { prisma.measurement.findMany({ where, // `{ id: sortDir }` is the tiebreaker, not decoration. `measuredAt` - // is not unique here and ties are the normal case — an Apple Health - // import stamps long runs with one instant, and the - // `stats::YYYY-MM-DD` daily-rollup convention puts every daily - // total at the same one. Without a unique secondary key Postgres is + // is not unique here and ties are the normal case. Only the exact + // tuple `(userId, type, measuredAt, source, sleepStage)` is unique, + // so a tie survives across `type` (the `stats::YYYY-MM-DD` + // daily-rollup convention puts every daily total at one instant), + // across `source` (two writers landing on the same instant) and + // across `sleepStage`. Without a unique secondary key Postgres is // free to order the tied block differently between the query for page // N and the query for page N+1, so a client paging its own history // silently gets one row twice and never gets another. diff --git a/src/app/api/medications/[id]/intake/__tests__/route.test.ts b/src/app/api/medications/[id]/intake/__tests__/route.test.ts index 2555f8d7b..60569dcb6 100644 --- a/src/app/api/medications/[id]/intake/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/intake/__tests__/route.test.ts @@ -208,6 +208,27 @@ describe("GET /api/medications/[id]/intake — status filter", () => { }); }); + it("orders on a unique tiebreaker, keeping NULLS LAST on the takenAt arm", async () => { + // Scheduled slots share an instant by construction — a twice-daily + // medication puts every morning dose on the same `scheduledFor` — so + // offset paging over the sort key alone can repeat a row on one page and + // drop another. The `takenAt` arm keeps its NULLS LAST pin as well: the + // tiebreaker must not push skipped rows back to the top. + await GET(makeRequest("sortBy=takenAt&sortDir=desc"), ROUTE_PARAMS); + expect( + vi.mocked(prisma.medicationIntakeEvent.findMany).mock.calls[0][0] + ?.orderBy, + ).toEqual([{ takenAt: { sort: "desc", nulls: "last" } }, { id: "desc" }]); + + vi.mocked(prisma.medicationIntakeEvent.findMany).mockClear(); + + await GET(makeRequest("sortBy=scheduledFor&sortDir=asc"), ROUTE_PARAMS); + expect( + vi.mocked(prisma.medicationIntakeEvent.findMany).mock.calls[0][0] + ?.orderBy, + ).toEqual([{ scheduledFor: "asc" }, { id: "asc" }]); + }); + it("rejects unknown status values with a 422", async () => { const res = await GET(makeRequest("status=junk"), ROUTE_PARAMS); expect(res.status).toBe(422); diff --git a/src/app/api/mental-health/assessments/__tests__/route.test.ts b/src/app/api/mental-health/assessments/__tests__/route.test.ts index 1d17b58a0..b6311acfc 100644 --- a/src/app/api/mental-health/assessments/__tests__/route.test.ts +++ b/src/app/api/mental-health/assessments/__tests__/route.test.ts @@ -15,7 +15,11 @@ import { NextRequest } from "next/server"; vi.mock("@/lib/db", () => ({ prisma: { appSettings: { findUnique: vi.fn().mockResolvedValue(null) }, - mentalHealthAssessment: { create: vi.fn(), findFirst: vi.fn() }, + mentalHealthAssessment: { + create: vi.fn(), + findFirst: vi.fn(), + findMany: vi.fn(), + }, measurement: { create: vi.fn().mockResolvedValue({}) }, }, })); @@ -60,7 +64,7 @@ vi.mock("next/headers", () => ({ })), })); -import { POST } from "../route"; +import { GET, POST } from "../route"; import { enqueueReminderSatisfy } from "@/lib/jobs/reminder-satisfy"; import { getSession } from "@/lib/auth/session"; import { requireModuleEnabled } from "@/lib/modules/gate"; @@ -449,3 +453,25 @@ describe("POST /api/mental-health/assessments — external-id stability floor", expect(prisma.mentalHealthAssessment.create).toHaveBeenCalledTimes(1); }); }); + +describe("GET /api/mental-health/assessments", () => { + it("orders the history on a unique tiebreaker", async () => { + vi.mocked(prisma.mentalHealthAssessment.findMany).mockResolvedValue( + [] as never, + ); + + const res = await (GET as unknown as (r: NextRequest) => Promise)( + new NextRequest( + new URL("http://localhost/api/mental-health/assessments"), + ), + ); + expect(res.status).toBe(200); + + // `takenAt` is day-resolution, so two instruments answered in one sitting + // tie; without the unique secondary key offset paging can repeat a row on + // one page and drop another. + const findArgs = vi.mocked(prisma.mentalHealthAssessment.findMany).mock + .calls[0][0] as { orderBy: unknown }; + expect(findArgs.orderBy).toEqual([{ takenAt: "desc" }, { id: "desc" }]); + }); +});