From 7dc51a217753337bc2c90c089d3a77a4818070c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 15 May 2026 20:57:13 +0200 Subject: [PATCH] fix(workouts): correct pagination under canonical dedup Reading rows with skip/take and then deduping inside the window double-counted cluster boundaries: a cluster split across two pages would be dropped on page N-1 and surface again on page N, while meta.total reflected the per-window canonical count rather than the user's true canonical total. Pull the full filtered set, dedupe once, then slice. Workout volume for a single user stays well within budget for an unbounded read; if the workload ever grows past that, switch to a (startedAt, id) cursor with a deterministic over-fetch window. A new test case exercises a window with eight twin clusters split across two pages to pin the contract. --- .../__tests__/canonical-dedup.test.ts | 44 +++++++++++++++++ src/app/api/workouts/route.ts | 47 ++++++++++++------- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/app/api/workouts/__tests__/canonical-dedup.test.ts b/src/app/api/workouts/__tests__/canonical-dedup.test.ts index 3308ff327..35e93c174 100644 --- a/src/app/api/workouts/__tests__/canonical-dedup.test.ts +++ b/src/app/api/workouts/__tests__/canonical-dedup.test.ts @@ -164,4 +164,48 @@ describe("GET /api/workouts — canonical dedup", () => { expect(body.data.workouts).toHaveLength(3); expect(body.data.meta.limit).toBe(3); }); + + it("paginates across cluster boundaries without double-counting deduped rows", async () => { + // Eight twin clusters: each cluster has an APPLE_HEALTH + WITHINGS + // row within the 5 min window. The picker collapses each cluster to + // its Apple row, so the canonical projection has 8 rows. Page 1 + // (offset=0, limit=4) must yield Apple rows for clusters 7–4 + // (descending). Page 2 (offset=4, limit=4) must yield Apple rows + // for clusters 3–0 with no overlap and no gaps. `meta.total` must + // reflect the deduped count (8), not the raw row count (16). + const rows: FakeWorkoutRow[] = []; + for (let i = 0; i < 8; i++) { + const start = new Date(2026, 4, 15, 7 + i, 0, 0).toISOString(); + const start2 = new Date(2026, 4, 15, 7 + i, 1, 30).toISOString(); + rows.push(makeRow(`apple-${i}`, "APPLE_HEALTH", start)); + rows.push(makeRow(`withings-${i}`, "WITHINGS", start2)); + } + // Mock returns the full filtered set; the route paginates after + // dedup. + vi.mocked(prisma.workout.findMany).mockResolvedValue(rows as never); + + const page1 = await GET(makeRequest("?limit=4&offset=0")); + const page1Body = await page1.json(); + const page2 = await GET(makeRequest("?limit=4&offset=4")); + const page2Body = await page2.json(); + + expect(page1Body.data.workouts).toHaveLength(4); + expect(page2Body.data.workouts).toHaveLength(4); + expect(page1Body.data.meta.total).toBe(8); + expect(page2Body.data.meta.total).toBe(8); + expect(page1Body.data.meta.droppedDuplicates).toBe(8); + expect(page2Body.data.meta.droppedDuplicates).toBe(8); + + const page1Ids = page1Body.data.workouts.map((w: { id: string }) => w.id); + const page2Ids = page2Body.data.workouts.map((w: { id: string }) => w.id); + // No overlap across the page boundary. + expect(page1Ids.filter((id: string) => page2Ids.includes(id))).toEqual([]); + // All eight clusters are covered between the two pages. + expect([...page1Ids, ...page2Ids].sort()).toEqual( + Array.from({ length: 8 }, (_, i) => `apple-${i}`).sort(), + ); + // Descending order is preserved within each page. + expect(page1Ids).toEqual(["apple-7", "apple-6", "apple-5", "apple-4"]); + expect(page2Ids).toEqual(["apple-3", "apple-2", "apple-1", "apple-0"]); + }); }); diff --git a/src/app/api/workouts/route.ts b/src/app/api/workouts/route.ts index 994062c4c..9dfd2bbe8 100644 --- a/src/app/api/workouts/route.ts +++ b/src/app/api/workouts/route.ts @@ -27,10 +27,22 @@ * - `since` / `until` — ISO timestamps, inclusive. Optional. * - `sportType` — narrow to one HKWorkoutActivityType. Optional. * + * Pagination contract: + * Dedup runs over the full filtered row set, then `offset` / `limit` + * slice the canonical projection. A naive `skip` + per-window dedup + * double-counts boundary clusters across pages because a cluster + * whose first member sits in page N-1 and whose later member sits in + * page N would be dropped on N-1 and surface again on N. Workout + * volume for HealthLog is bounded (single user, tens of workouts per + * week — single-digit MB at the table's busiest), so reading the + * full filtered set per request is well within budget. If the + * workload ever grows to where this matters, a `(startedAt, id)` + * cursor with a deterministic over-fetch window is the next step. + * * v1.4.27 B7 / BL-P2-3 — wires `pickCanonicalWorkout()` into the read - * path so the workout dedup contract finally has a consumer. The - * picker stays a pure function — the route owns the query window and - * the response shaping. + * path. v1.4.27 R4 RC3 — corrects the pagination so `meta.total` + * reflects the deduped count and cluster boundaries no longer leak + * across pages. */ import { NextRequest } from "next/server"; @@ -47,10 +59,6 @@ import { const DEFAULT_LIMIT = 50; const MAX_LIMIT = 200; -// The picker can collapse heavy two-source ingest by ~50%; fetching a -// 2× window ahead of pagination keeps the page full without leaving -// the server fetching unbounded. -const FETCH_MULTIPLIER = 2; export const GET = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); @@ -92,16 +100,14 @@ export const GET = apiHandler(async (request: NextRequest) => { where.sportType = sportType; } - const rawTake = Math.min( - limit * FETCH_MULTIPLIER, - MAX_LIMIT * FETCH_MULTIPLIER, - ); - + // Read the full filtered set then dedupe once. See the file-level + // "Pagination contract" note for the reason this is preferred over + // a per-page over-fetch + slice. The picker is O(n) on cluster + // building plus O(k) per cluster (k = open clusters of the same + // sport type) and the workout table is bounded. const rows = await prisma.workout.findMany({ where, orderBy: { startedAt: "desc" }, - take: rawTake, - skip: offset, select: { id: true, source: true, @@ -127,15 +133,22 @@ export const GET = apiHandler(async (request: NextRequest) => { proximityMinutes: DEFAULT_WORKOUT_PROXIMITY_MINUTES, }); - const page = canonical.slice(0, limit); + // The picker sorts ascending for deterministic cluster building. + // Restore descending order on the canonical projection so the API + // matches the orderBy contract above. + const canonicalDesc = [...canonical].sort( + (a, b) => b.startedAt.getTime() - a.startedAt.getTime(), + ); + + const page = canonicalDesc.slice(offset, offset + limit); return apiSuccess({ workouts: page, meta: { - total: canonical.length, + total: canonicalDesc.length, limit, offset, - droppedDuplicates: rows.length - canonical.length, + droppedDuplicates: rows.length - canonicalDesc.length, clusters: clusters.length, }, });