fix(workouts): collapse a re-sent session on every surface, not just the list

The canonical picker keyed a session on sport, start, end and duration. Only the
workout list selected the end timestamp; the Coach narration, the per-sport
baseline and the deep-link cluster passed rows without it, so those rows carried
no session identity at all and the duplicate survived. One record counted once in
the list and two or three times everywhere else, including in the averages a
person is compared against.

Drop the end timestamp from the identity. On a workout with pauses it measures
wall-clock time while the duration counts only moving time, so two re-sends of
one session can differ in that field alone and the stricter key rejected a
genuine match. It also bought nothing in exchange: nobody starts a second
workout of the same sport in the same instant with the same duration.

The deep-link cluster now selects the duration it needs. A structural guard
freezes the call-site set and records which file owns each caller's select, so a
new surface cannot join the set without fetching the identity, and the identity
cannot grow a field only some callers fetch.
This commit is contained in:
Marc-André Bombeck committed 2026-07-30 11:54:58 +02:00
1 parent 3811d4592e
commit 00c282d311
4 files changed
+216 -12

No files matched your search

@@ -0,0 +1,120 @@
/**
* Structural guard on the canonical-workout session identity.
*
* `pickCanonicalWorkoutRows()` collapses a HealthKit session that arrived more
* than once under different external UUIDs. It can only do that when the row it
* is handed carries the identity fields, and the row shape comes from each
* caller's own Prisma `select`. Nothing in the type system connects the two:
* `durationSec` is optional on `WorkoutPickerRow`, so a caller that omits it
* gets silent passthrough rather than a compile error.
*
* That is exactly what happened. When the collapse first shipped, the key was
* `(sportType, startedAt, endedAt, durationSec)` and only the workout list
* selected `endedAt`. The list showed one workout while the Coach narrated the
* duplicate, the per-sport baseline averaged it twice and the deep-link cluster
* kept both rows. One record, four surfaces, two answers.
*
* These are tripwires, not proofs. They cannot show the collapse is correct —
* that is what the unit tests do. What they can do is refuse to let a new call
* site join the set without someone editing this file, and refuse to let the
* identity quietly grow a field that only some callers fetch.
*/
import { describe, it, expect } from "vitest";
import { readFileSync } from "node:fs";
import { globSync } from "node:fs";
import { join, sep } from "node:path";
const SRC = join(process.cwd(), "src");
/** The picker module itself — declares the identity, does not consume it. */
const PICKER = "lib/measurements/pick-canonical-workout-rows.ts";
/**
* Every production call site of the read-time picker, frozen.
*
* Adding a surface that collapses workouts is a deliberate act: it must fetch
* the identity fields or it silently disagrees with every other surface. Adding
* the file here is the acknowledgement.
*/
const IDENTITY_OWNER: Record<string, string> = {
"app/api/workouts/[id]/route.ts": "app/api/workouts/[id]/route.ts",
"lib/jobs/workout-insight-generate.ts":
"lib/jobs/workout-insight-generate.ts",
"lib/workouts/list-read.ts": "lib/workouts/list-read.ts",
"lib/workouts/sport-context.ts": "lib/workouts/sport-context.ts",
// The only indirection in the set. The Coach block is handed rows it does not
// read itself; the snapshot builder fetches them in parallel with every other
// block's data. Recording the owner here rather than assuming the call site
// owns its own select is the whole point of this map.
"lib/ai/coach/snapshot-blocks/workouts-block.ts": "lib/ai/coach/snapshot.ts",
};
const EXPECTED_CALL_SITES = Object.keys(IDENTITY_OWNER).sort();
function sourceFiles(): string[] {
return globSync("**/*.{ts,tsx}", { cwd: SRC })
.map((p) => p.split(sep).join("/"))
.filter((p) => !p.startsWith("generated/"))
.filter((p) => !p.includes("__tests__"))
.filter((p) => !p.endsWith(".test.ts") && !p.endsWith(".test.tsx"))
.sort();
}
function read(rel: string): string {
return readFileSync(join(SRC, rel), "utf8");
}
/**
* Lines that actually invoke the picker. Comment lines are excluded because the
* write-time sibling (`lib/workouts/canonical-rows.ts`) names the read-time
* picker in its docblock to explain how the two layer, and that mention is
* documentation rather than a call.
*/
function invokesPicker(source: string): boolean {
return source
.split("\n")
.some(
(line) =>
line.includes("pickCanonicalWorkoutRows(") &&
!/^\s*(\*|\/\/|\/\*)/.test(line),
);
}
describe("the picker's call-site set is frozen", () => {
it("has exactly the call sites this file acknowledges", () => {
const found = sourceFiles()
.filter((rel) => rel !== PICKER)
.filter((rel) => invokesPicker(read(rel)));
expect(found).toEqual(EXPECTED_CALL_SITES);
});
});
describe("every call site fetches the session identity", () => {
it.each(EXPECTED_CALL_SITES)("%s gets rows carrying durationSec", (rel) => {
// File-level rather than per-select: a caller that fetches workout rows
// without their duration cannot collapse a re-send, whichever select the
// rows came from. Coarse in the safe direction — it can only fail to catch
// a second unrelated select in the same file, never pass a caller that
// fetches no duration at all.
expect(read(IDENTITY_OWNER[rel])).toContain("durationSec: true");
});
});
describe("the identity cannot quietly grow a field callers do not fetch", () => {
it("keys the session on sportType, startedAt and durationSec only", () => {
const picker = read(PICKER);
const body = picker.slice(
picker.indexOf("function exactSessionKey"),
picker.indexOf("function rowRichness"),
);
expect(body).toContain("row.sportType");
expect(body).toContain("row.startedAt");
expect(body).toContain("row.durationSec");
// `endedAt` is the specific field that split the callers. On a workout with
// pauses it disagrees with `durationSec` by design, so it rejected genuine
// re-sends while forcing every caller to fetch a fourth column.
expect(body).not.toContain("endedAt");
});
});
+5
View File
@@ -114,6 +114,11 @@ export const GET = apiHandler(
source: true,
startedAt: true,
sportType: true,
// Part of the canonical session identity consulted by
// `pickCanonicalWorkoutRows` below — without it a HealthKit re-send of
// this very workout stays in the cluster and the deep link can resolve
// to the duplicate instead of the row the list shows.
durationSec: true,
avgHeartRate: true,
maxHeartRate: true,
metadata: true,
@@ -277,6 +277,68 @@ describe("pickCanonicalWorkoutRows", () => {
expect(result[0].id).toBe("duplicate-b");
});
it("collapses a re-send whose endedAt disagrees with the first arrival", () => {
// A workout with pauses has a wall-clock end that is longer than the
// counted duration, and the two sources of that number do not always agree
// across re-sends. `endedAt` was part of the session key when the collapse
// first shipped, so this exact pair survived as two rows.
const base = {
startedAt: new Date("2026-07-29T12:54:00Z"),
durationSec: 480,
sportType: "cycling",
source: "APPLE_HEALTH" as const,
};
const result = pickCanonicalWorkoutRows<RowFixture>([
{
...base,
id: "first-arrival",
endedAt: new Date("2026-07-29T13:02:00Z"),
},
{
...base,
id: "resend",
endedAt: new Date("2026-07-29T13:09:00Z"),
avgHeartRate: 132,
},
]);
expect(result).toHaveLength(1);
expect(result[0].id).toBe("resend");
});
it("keeps two same-source sessions whose durations differ", () => {
// The counterpart guarantee: the looser key must not merge genuinely
// distinct sessions that happen to share a bucket slot and a sport.
const base = {
startedAt: new Date("2026-07-29T12:54:00Z"),
sportType: "strength",
source: "APPLE_HEALTH" as const,
};
const result = pickCanonicalWorkoutRows<RowFixture>([
{ ...base, id: "short", durationSec: 480 },
{ ...base, id: "long", durationSec: 1800 },
]);
expect(result.map((row) => row.id).sort()).toEqual(["long", "short"]);
});
it("passes rows without a duration through rather than guessing", () => {
// Fixture-only path: a row with no duration carries no session identity, so
// it cannot be collapsed. Every production caller selects the field and
// `workout-canonical-identity-guard.test.ts` keeps that true.
const base = {
startedAt: new Date("2026-07-29T12:54:00Z"),
sportType: "yoga",
source: "APPLE_HEALTH" as const,
};
const result = pickCanonicalWorkoutRows<RowFixture>([
{ ...base, id: "no-duration-a" },
{ ...base, id: "no-duration-b" },
]);
expect(result).toHaveLength(2);
});
it("does not collapse or enrich rows belonging to different users", () => {
const result = pickCanonicalWorkoutRows<RowFixture>([
{
@@ -46,8 +46,12 @@ import {
export interface WorkoutPickerRow {
userId?: string;
startedAt: Date;
/** Optional exact-session fields used to collapse repeated same-source sync rows. */
endedAt?: Date;
/**
* Session length. Together with `sportType` + `startedAt` this is the
* exact-session identity used to collapse repeated same-source sync rows.
* Optional only so fixtures can omit it; every production call site MUST
* select it, and `workout-canonical-identity-guard.test.ts` freezes that set.
*/
durationSec?: number;
sportType: string;
source: MeasurementSource;
@@ -60,18 +64,31 @@ export interface WorkoutPickerRow {
* HealthKit can resend the same session with a new external UUID. The source
* ladder correctly chooses Apple over WHOOP, but previously kept every Apple
* row in that winning bucket, which made the web list show the same workout
* two or three times. Only collapse rows when the exact session identity is
* available; fixtures and legacy callers without end/duration remain
* governed by the original bucket contract.
* two or three times.
*
* The identity is `(sportType, startedAt, durationSec)`. `endedAt` was part of
* this key when the collapse first shipped and has been dropped deliberately:
*
* - It is not a free extra check. On a workout with pauses, `endedAt` is the
* wall-clock end while `durationSec` counts only moving time, so the two
* legitimately disagree, and a stored row can carry either convention. Two
* re-sends of one session can therefore differ in `endedAt` alone, and the
* stricter key let that duplicate through.
* - It bought nothing in exchange. Nobody starts a second workout of the
* same sport in the same instant with the same duration, so start plus
* duration already identifies the session uniquely.
* - Requiring it split the callers. Four of the five call sites select
* `durationSec` but not `endedAt`, so the collapse ran on the workout list
* and silently did nothing for the Coach narration, the per-sport baseline
* and the deep-link cluster. One list, one number, two answers.
*
* A row without `durationSec` cannot be identified and passes through
* uncollapsed. That is a fixture-only path: every production call site selects
* the field and `workout-canonical-identity-guard.test.ts` keeps it that way.
*/
function exactSessionKey(row: WorkoutPickerRow): string | null {
if (row.endedAt == null || row.durationSec == null) return null;
return [
row.sportType,
row.startedAt.getTime(),
row.endedAt.getTime(),
row.durationSec,
].join(":");
if (row.durationSec == null) return null;
return [row.sportType, row.startedAt.getTime(), row.durationSec].join(":");
}
function rowRichness(row: WorkoutPickerRow): number {