diff --git a/src/__tests__/coach-stream-lifecycle-guard.test.ts b/src/__tests__/coach-stream-lifecycle-guard.test.ts new file mode 100644 index 000000000..c014cf97d --- /dev/null +++ b/src/__tests__/coach-stream-lifecycle-guard.test.ts @@ -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( + / { + 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); + }); +}); diff --git a/src/components/insights/coach-panel/coach-conversation.tsx b/src/components/insights/coach-panel/coach-conversation.tsx index e186a7b13..ee75a2e46 100644 --- a/src/components/insights/coach-panel/coach-conversation.tsx +++ b/src/components/insights/coach-panel/coach-conversation.tsx @@ -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(); }} /> } diff --git a/src/components/insights/coach-panel/use-coach.ts b/src/components/insights/coach-panel/use-coach.ts index 19b8ffbc3..05d6a972a 100644 --- a/src/components/insights/coach-panel/use-coach.ts +++ b/src/components/insights/coach-panel/use-coach.ts @@ -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); diff --git a/src/lib/measurements/__tests__/daily-series-read-tiered.test.ts b/src/lib/measurements/__tests__/daily-series-read-tiered.test.ts index c23d7030a..4fd85bec3 100644 --- a/src/lib/measurements/__tests__/daily-series-read-tiered.test.ts +++ b/src/lib/measurements/__tests__/daily-series-read-tiered.test.ts @@ -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(); }); diff --git a/src/lib/measurements/daily-series-read.ts b/src/lib/measurements/daily-series-read.ts index 7461b8dab..f63e6a163 100644 --- a/src/lib/measurements/daily-series-read.ts +++ b/src/lib/measurements/daily-series-read.ts @@ -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. })); }