fix(measurements,coach): restore the live-fallback spread band and the stream lifecycle

The tiered rollup read emits minValue/maxValue; readLiveDaily omitted them, so
on a rollup-cold account or a coverage miss the chart silently dropped the
min/max spread band for the same underlying data — contradicting the file's own
byte-for-byte parity claim. The live aggregate now selects MIN/MAX alongside the
average.

useSendCoachMessage never aborted its in-flight reader on unmount. The drawer
works around that through handleOpenChange, but the full-page /coach surface
passes no registerReset, so navigating away mid-stream left the SSE loop alive:
the provider kept generating and billing, and trailing setState fired on an
unmounted component.

Switching conversations reset every piece of local state except the send state,
so the previous thread's last assistant or error bubble rendered appended to the
newly selected thread until the next send.

The two lifecycle invariants are pinned structurally, not behaviourally — the
repo ships no renderHook. That is weaker, and the header says so; both defects
were a missing call, which is what a structural guard does catch.
This commit is contained in:
Marc-André Bombeck committed 2026-07-19 14:23:03 +02:00
1 parent 9d03fe13f0
commit d103699099
5 files changed
+145 -2

No files matched your search

@@ -0,0 +1,89 @@
/**
* Structural guard for two Coach streaming-lifecycle invariants.
*
* These are STRUCTURAL assertions over the source text, not behavioural ones.
* The repo ships no `@testing-library/react` / `renderHook`, so an unmount
* cleanup and a tray-switch callback cannot be driven behaviourally here. A
* structural guard is weaker than a behavioural test — it proves the code is
* present, not that it works — but both defects were *deletions of a call*,
* which is exactly what a structural guard does catch. Matches the existing
* `*-guard.test.ts` precedent in this directory.
*
* Invariant 1 — `useSendCoachMessage` aborts its in-flight reader on unmount.
* Without it, the full-page `/coach` surface (which passes no `registerReset`,
* so the reset effect early-returns) left the SSE reader loop alive on a
* route change: the provider kept generating and billing, and trailing
* setState fired on an unmounted component. The drawer worked around this via
* `handleOpenChange`; route-change unmount had no exit at all.
*
* Invariant 2 — switching conversations resets the send state. Without it the
* previous thread's last assistant/error bubble rendered appended to the newly
* selected thread until the next send, because `streamingActive` stays true
* while that messageId is absent from the new thread's messages.
*/
import { readFileSync } from "node:fs";
import { join } from "node:path";
import { describe, expect, it } from "vitest";
const ROOT = join(__dirname, "..");
function read(relative: string): string {
return readFileSync(join(ROOT, relative), "utf8");
}
describe("Coach streaming lifecycle", () => {
it("useSendCoachMessage registers an unmount cleanup that aborts the reader", () => {
const source = read("components/insights/coach-panel/use-coach.ts");
// An effect with an empty dependency array whose cleanup aborts.
const hasUnmountAbort =
/useEffect\([\s\S]*?return\s*\(\)\s*=>\s*\{[^}]*abortRef\.current\?\.abort\(\)/.test(
source,
);
expect(
hasUnmountAbort,
"use-coach.ts must abort abortRef in an unmount cleanup effect",
).toBe(true);
// And it must be mount-scoped — a dependency-carrying effect would abort
// mid-stream on an unrelated re-render.
const effectWithDeps =
/return\s*\(\)\s*=>\s*\{\s*abortRef\.current\?\.abort\(\);\s*\};\s*\},\s*\[\]\s*\)/.test(
source,
);
expect(
effectWithDeps,
"the unmount abort effect must have an empty dependency array",
).toBe(true);
});
it("the conversation-switch handler resets the send state", () => {
const source = read(
"components/insights/coach-panel/coach-conversation.tsx",
);
// Isolate the HistoryRail onSelect callback body.
const match = source.match(
/<HistoryRail[\s\S]*?onSelect=\{\([\s\S]*?\n\s*\}\}/,
);
expect(match, "HistoryRail onSelect handler not found").not.toBeNull();
const onSelectBody = match![0];
expect(
onSelectBody.includes("send.reset()"),
"selecting another conversation must call send.reset() so the previous " +
"thread's trailing stream state does not bleed into the new thread",
).toBe(true);
});
it("handleNewChat still resets too — the two paths must not diverge again", () => {
const source = read(
"components/insights/coach-panel/coach-conversation.tsx",
);
expect(source).toContain("send.reset()");
// Both call sites present: new-chat and conversation-switch.
const resetCalls = source.match(/send\.reset\(\)/g) ?? [];
expect(resetCalls.length).toBeGreaterThanOrEqual(2);
});
});
@@ -1216,6 +1216,12 @@ export function CoachConversation({
// seed so a pending attachment can't leak across a thread switch.
setPendingAttachmentIds([]);
dispatchGuided({ type: "RESET" });
// Drop the finished stream's trailing state, as `handleNewChat`
// already does. Without it the previous thread's last assistant
// or error bubble renders appended to the newly selected thread
// — `streamingActive` stays true because that messageId is
// absent from the new thread's messages — until the next send.
send.reset();
}}
/>
}
@@ -597,6 +597,29 @@ export function useSendCoachMessage(opts: UseSendCoachMessageOptions = {}) {
optsRef.current = opts;
}, [opts]);
// Abort any in-flight stream when the hook unmounts. The drawer works around
// this via `handleOpenChange`, but the full-page `/coach` surface passes no
// `registerReset`, so navigating away mid-stream left the SSE reader loop
// alive: the provider kept generating (and billing), and trailing setState
// fired on an unmounted component. The route-change path had no other exit.
useEffect(() => {
return () => {
abortRef.current?.abort();
};
}, []);
// Abort any in-flight stream when the hook unmounts. The drawer works around
// this via `handleOpenChange`, but the full-page `/coach` surface passes no
// `registerReset`, so navigating away mid-stream left the SSE reader loop
// alive: the provider kept generating (and billing), and trailing setState
// fired on an unmounted component. The route-change path had no other exit.
// Abort any in-flight stream when the hook unmounts. The drawer works around
// this via `handleOpenChange`, but the full-page `/coach` surface passes no
// `registerReset`, so navigating away mid-stream left the SSE reader loop
// alive: the provider kept generating (and billing), and trailing setState
// fired on an unmounted component. The route-change path had no other exit.
const reset = useCallback(() => {
setStreaming(EMPTY_STREAMING);
setOptimisticUser(null);
@@ -175,6 +175,8 @@ describe("readDailySeries — rollup read throw falls back to live SQL (F-DB-2)"
bucket_start: new Date("2026-06-10T00:00:00.000Z"),
avg: 80.5,
cnt: 2,
min_value: 79.8,
max_value: 81.2,
},
]);
@@ -189,6 +191,10 @@ describe("readDailySeries — rollup read throw falls back to live SQL (F-DB-2)"
// The read did NOT throw — it fell through to live SQL and served a row.
expect(result).toHaveLength(1);
expect(result[0]).toMatchObject({ type: "WEIGHT", value: 80.5, count: 2 });
// The live fallback must carry the min/max spread the rollup path emits —
// otherwise a rollup-cold account silently loses the chart's spread band
// for the same underlying data.
expect(result[0]).toMatchObject({ minValue: 79.8, maxValue: 81.2 });
expect(mocks.queryRaw).toHaveBeenCalled();
});
+21 -2
View File
@@ -253,7 +253,14 @@ async function readLiveDaily(opts: {
buildSourceRankCase(priorityJson, 'm."type"', 'm."source"'),
);
const buckets = await prisma.$queryRaw<
Array<{ type: string; bucket_start: Date; avg: number; cnt: number }>
Array<{
type: string;
bucket_start: Date;
avg: number;
cnt: number;
min_value: number | null;
max_value: number | null;
}>
>`
WITH canon AS (
SELECT DISTINCT ON (m."type", date_trunc(${truncUnitLiteral}, m."measured_at"))
@@ -272,7 +279,9 @@ async function readLiveDaily(opts: {
m."type"::text AS type,
date_trunc(${truncUnitLiteral}, m."measured_at") AS bucket_start,
${aggregator} AS avg,
COUNT(*)::int AS cnt
COUNT(*)::int AS cnt,
MIN(m."value") AS min_value,
MAX(m."value") AS max_value
FROM measurements m
JOIN canon c
ON c.t = m."type"
@@ -296,5 +305,15 @@ async function readLiveDaily(opts: {
value: Number(b.avg),
measuredAt: b.bucket_start.toISOString(),
count: Number(b.cnt),
// The rollup path emits these; the live fallback used to omit them, so a
// rollup-cold account silently lost the min/max spread band on the chart
// for the same underlying data — contradicting this file's own
// byte-for-byte parity claim.
minValue: b.min_value === null ? undefined : Number(b.min_value),
maxValue: b.max_value === null ? undefined : Number(b.max_value),
// The rollup path emits these; the live fallback used to omit them, so a
// rollup-cold account silently lost the min/max spread band on the chart
// for the same underlying data — contradicting this file's own
// byte-for-byte parity claim.
}));
}