From b0ac267af9309c1453f1ce583bf2fcb31644d612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 5 Oct 2026 17:31:38 +0200 Subject: [PATCH] fix(coach): stop offering a view chip the result panel already toggles A table the answer displays with a chart renders its own chart/table toggle, and the "Show as a chart" / "Show as a table" chips under it repeated that control. The server no longer derives a view chip for such a table; a table the answer only used, folded under "Data used", still gets "Show as a chart", since that brings it into the answer. A reuse turn shows its table with the toggle and so offers no view chip either. The rule is one predicate in a leaf module, applied where chips are derived and where a stored reply is read back, so a reply stored before the change offers the same chips as a new one on every client. --- .../chat/__tests__/route-followups.test.ts | 20 ++++--- .../__tests__/persistence-results.test.ts | 54 +++++++++++++++++++ .../coach/follow-ups/__tests__/derive.test.ts | 35 +++++++----- src/lib/ai/coach/follow-ups/catalog.ts | 8 +-- src/lib/ai/coach/follow-ups/derive.ts | 27 +++++----- src/lib/ai/coach/follow-ups/view-chip.ts | 42 +++++++++++++++ src/lib/ai/coach/persistence.ts | 9 +++- .../turn/__tests__/follow-up-turn.test.ts | 26 +++------ 8 files changed, 158 insertions(+), 63 deletions(-) create mode 100644 src/lib/ai/coach/follow-ups/view-chip.ts diff --git a/src/app/api/insights/chat/__tests__/route-followups.test.ts b/src/app/api/insights/chat/__tests__/route-followups.test.ts index 8110489ee..fbfc92f84 100644 --- a/src/app/api/insights/chat/__tests__/route-followups.test.ts +++ b/src/app/api/insights/chat/__tests__/route-followups.test.ts @@ -212,17 +212,17 @@ describe("a reuse chip", () => { messageId: h.LAST_ASSISTANT_ID, ref: "r1", }); - const [chips] = framesOf<{ followUps: CoachFollowUp[] }>( - run.frames, - "followUps", - ); - expect(chips.followUps.map((c) => c.kind)).toEqual(["as_chart"]); + // The result renders with its own chart/table toggle, so no chip + // offers the chart again. + expect( + framesOf<{ followUps: CoachFollowUp[] }>(run.frames, "followUps"), + ).toEqual([]); expect(m.appendMessage).toHaveBeenCalledWith( expect.objectContaining({ role: "assistant", providerType: "reuse" }), ); }); - it("answers 'as a chart' with the stored chart and offers the table view", async () => { + it("answers 'as a chart' with the stored chart and leaves the table to its toggle", async () => { conversationWithChips(); const run = await tap("f3"); expect(providerCalls).toHaveLength(0); @@ -231,11 +231,9 @@ describe("a reuse chip", () => { "result", ).map((f) => f.result); expect(result.chart).toEqual(world.storedTables[0].chart); - const [chips] = framesOf<{ followUps: CoachFollowUp[] }>( - run.frames, - "followUps", - ); - expect(chips.followUps.map((c) => c.kind)).toEqual(["as_table"]); + expect( + framesOf<{ followUps: CoachFollowUp[] }>(run.frames, "followUps"), + ).toEqual([]); }); it("still passes the rate limit", async () => { diff --git a/src/lib/ai/coach/__tests__/persistence-results.test.ts b/src/lib/ai/coach/__tests__/persistence-results.test.ts index ae1d2a582..6b5e28a23 100644 --- a/src/lib/ai/coach/__tests__/persistence-results.test.ts +++ b/src/lib/ai/coach/__tests__/persistence-results.test.ts @@ -201,6 +201,60 @@ describe("appendMessage — the dialog fields", () => { }); }); + it("reads a stored view chip back only where the answer's panel has no toggle for it", async () => { + // Stored before the rule: "as a table" for the shown chart (r1), whose + // panel already toggles, and "as a chart" for a table only used (r2). + const view = (id: string, kind: "as_chart" | "as_table", ref: string) => ({ + ...FULL.followUps![0], + id, + kind, + labelKey: `coach.followUp.${kind === "as_chart" ? "asChart" : "asTable"}`, + label: kind === "as_chart" ? "Show as a chart" : "Show as a table", + anchor: { ref, domain: "bp" as const, window: "last30days" as const }, + reuse: true, + }); + const used = { ...SLEEP_META, displayed: false }; + stubEchoCreate( + JSON.stringify({ + windows: [], + metrics: [], + results: [META, used], + followUps: [ + view("f0", "as_table", "r1"), + FULL.followUps![0], + view("f3", "as_chart", "r2"), + ], + }), + ); + const out = await appendMessage({ + conversationId: "c1", + role: "assistant", + content: "reply", + metricSource: { windows: [], metrics: [] }, + }); + expect(out.metricSource?.followUps?.map((c) => [c.id, c.kind])).toEqual([ + ["f1", "previous_period"], + ["f3", "as_chart"], + ]); + + // Only the redundant chip stored: the list reads back absent, not empty. + stubEchoCreate( + JSON.stringify({ + windows: [], + metrics: [], + results: [META], + followUps: [view("f1", "as_table", "r1")], + }), + ); + const bare = await appendMessage({ + conversationId: "c1", + role: "assistant", + content: "reply", + metricSource: { windows: [], metrics: [] }, + }); + expect(bare.metricSource).not.toHaveProperty("followUps"); + }); + it("strips a key the stored blob carries that the contract does not know", async () => { stubEchoCreate( JSON.stringify({ diff --git a/src/lib/ai/coach/follow-ups/__tests__/derive.test.ts b/src/lib/ai/coach/follow-ups/__tests__/derive.test.ts index 048be7aea..a7a3234c2 100644 --- a/src/lib/ai/coach/follow-ups/__tests__/derive.test.ts +++ b/src/lib/ai/coach/follow-ups/__tests__/derive.test.ts @@ -165,20 +165,26 @@ describe("deriveFollowUps — the rules", () => { expect(all).toEqual([]); }); - it("offers the chart back for a table shown as a table", () => { - const chips = derive({ - results: [ - table("bp", { chartKind: "line", displayed: true, view: "table" }), - ], - }); - expect(chips[0]).toMatchObject({ kind: "as_chart", reuse: true }); + it("offers no view chip for a table the answer shows with its chart", () => { + // The result panel renders its own chart/table toggle; a chip asking for + // either view would repeat it, whichever view the panel opens on. + for (const view of [undefined, "table" as const]) { + const chips = derive({ + results: [ + table("bp", { + chartKind: "line", + displayed: true, + ...(view ? { view } : {}), + }), + ], + }); + expect(chips.some((c) => c.reuse)).toBe(false); + } }); - it("offers the other view of a table that has a chart, as a reuse chip", () => { - const shown = derive({ - results: [table("bp", { chartKind: "line", displayed: true })], - }); - expect(shown[0]).toMatchObject({ kind: "as_table", reuse: true }); + it("offers the chart of a table the answer only used, as a reuse chip", () => { + // Folded under "Data used" as a table, its toggle is out of sight; the + // chip brings it into the answer. const tucked = derive({ results: [table("bp", { chartKind: "line", displayed: false })], }); @@ -404,6 +410,11 @@ describe("deriveFollowUps — property: every chip's domain was read this turn", } if (!chip.reuse) { expect(held.has(domain!), `seed ${seed}`).toBe(true); + } else { + // A view chip never repeats the toggle of a table the answer + // shows with its chart. + const meta = results.find((m) => m.ref === chip.anchor?.ref); + expect(meta?.displayed && meta.chartKind, `seed ${seed}`).toBeFalsy(); } } } diff --git a/src/lib/ai/coach/follow-ups/catalog.ts b/src/lib/ai/coach/follow-ups/catalog.ts index d9e774757..6c7206535 100644 --- a/src/lib/ai/coach/follow-ups/catalog.ts +++ b/src/lib/ai/coach/follow-ups/catalog.ts @@ -22,6 +22,8 @@ import { } from "@/lib/ai/coach/dialog-keys"; import { METRIC_TABLE_EXCLUDED_SOURCES } from "@/lib/ai/coach/results/metric-table-tool"; +import { REUSE_FOLLOW_UP_KINDS } from "./view-chip"; + /** At most this many chips under one reply. */ export const MAX_FOLLOW_UPS = 3; @@ -36,12 +38,6 @@ export const FOLLOW_UP_KINDS: readonly CoachFollowUpKind[] = [ "continue", ]; -/** Kinds answered from a table the conversation already holds. */ -export const REUSE_FOLLOW_UP_KINDS: ReadonlySet = new Set([ - "as_chart", - "as_table", -]); - /** * Kinds the model may propose in a `---FOLLOWUPS---` block. `continue` is * the server's alone: it follows from the loop hitting its round cap, which diff --git a/src/lib/ai/coach/follow-ups/derive.ts b/src/lib/ai/coach/follow-ups/derive.ts index 7334898ef..7a3b6f29b 100644 --- a/src/lib/ai/coach/follow-ups/derive.ts +++ b/src/lib/ai/coach/follow-ups/derive.ts @@ -6,8 +6,10 @@ * * A chip is offered only when what it asks for is grounded in this turn: * - * - `as_chart` / `as_table`: a table this turn produced (or copied) that - * has a chart; the chip names the view it is not shown in yet. + * - `as_chart`: a table this turn produced that has a chart and that the + * answer only used, so it sits folded under "Data used" as a table. A + * table the answer displays with a chart carries its own chart/table + * toggle, so it gets no view chip (`isRedundantViewChip`). * - `previous_period`: a time series table of the current period, for a * metric the table tool reads and the record holds. * - `year_ago`: the same, over a window of at most 90 days, when the @@ -59,6 +61,7 @@ import { widerWindow, } from "./catalog"; import type { FollowUpProposal } from "./parse-sentinel"; +import { isRedundantViewChip } from "./view-chip"; import { exceedsReach, fitsReach, @@ -195,18 +198,14 @@ export function deriveFollowUps(args: { ...(source.granularity ? { granularity: source.granularity } : {}), period: source.period, }; - if (meta.chartKind !== null) { - // The other view of what the answer shows: a table shown as a table - // offers its chart, a shown chart offers its table. - const showsChart = meta.displayed && meta.view !== "table"; - candidates.push( - buildFollowUp({ - kind: showsChart ? "as_table" : "as_chart", - anchor, - origin: "server", - locale, - }), - ); + const viewChip = buildFollowUp({ + kind: "as_chart", + anchor, + origin: "server", + locale, + }); + if (meta.chartKind !== null && !isRedundantViewChip(viewChip, [meta])) { + candidates.push(viewChip); } } diff --git a/src/lib/ai/coach/follow-ups/view-chip.ts b/src/lib/ai/coach/follow-ups/view-chip.ts new file mode 100644 index 000000000..8b77b7a47 --- /dev/null +++ b/src/lib/ai/coach/follow-ups/view-chip.ts @@ -0,0 +1,42 @@ +/** + * The view chips ("as a chart", "as a table") and when one would repeat a + * control the answer already shows. + * + * A leaf module on purpose: the stored-reply reader (`persistence.ts`) uses + * it, and that reader sits on the fenced document chat's module graph, which + * must not reach the Coach tool registry the rest of the chip catalog + * imports. + */ +import type { + CoachFollowUp, + CoachFollowUpKind, + CoachResultMeta, +} from "@/lib/ai/coach/types"; + +/** Kinds answered from a table the conversation already holds. */ +export const REUSE_FOLLOW_UP_KINDS: ReadonlySet = new Set([ + "as_chart", + "as_table", +]); + +/** + * True when a view chip ("as a chart", "as a table") would repeat a control + * the answer already shows: a table the answer displays with a chart renders + * its own chart/table toggle right under the prose, so the chip adds + * nothing. A view chip still counts for a table the answer only used (it + * sits folded under "Data used", and the chip brings it into the answer) + * and every other kind is never redundant this way. One definition for the + * chips a turn derives and the chips a stored reply is read back with, so a + * reply stored before the rule offers the same as a new one, on every + * client. + */ +export function isRedundantViewChip( + chip: Pick, + results: readonly Pick[], +): boolean { + if (!REUSE_FOLLOW_UP_KINDS.has(chip.kind)) return false; + const ref = chip.anchor?.ref; + return results.some( + (meta) => meta.ref === ref && meta.displayed && meta.chartKind !== null, + ); +} diff --git a/src/lib/ai/coach/persistence.ts b/src/lib/ai/coach/persistence.ts index db1c05d4c..9b529c26f 100644 --- a/src/lib/ai/coach/persistence.ts +++ b/src/lib/ai/coach/persistence.ts @@ -23,6 +23,7 @@ import { coachStepSchema, } from "./stream-events"; import { COACH_CONVERSATION_TITLE_MAX } from "./types"; +import { isRedundantViewChip } from "./follow-ups/view-chip"; import { RESULTS_MAX_BYTES, fitResultsToStorage } from "./results/refs"; import { isCheckupIntervalId, @@ -323,7 +324,11 @@ function provenanceFromJson(raw: string | null): CoachProvenance | null { const steps = restoreSteps(parsed.steps); const method = restoreMethod(parsed.method); const results = restoreResultMetas(parsed.results); - const followUps = restoreFollowUps(parsed.followUps); + // A reply stored before view chips were withheld for a table the answer + // already shows with its own toggle reads back without them. + const followUps = restoreFollowUps(parsed.followUps)?.filter( + (chip) => !isRedundantViewChip(chip, results ?? []), + ); const clarification = restoreClarification(parsed.clarification); const forcedFinal = parsed.forcedFinal === true; const continuationOf = @@ -345,7 +350,7 @@ function provenanceFromJson(raw: string | null): CoachProvenance | null { ...(steps ? { steps } : {}), ...(method ? { method } : {}), ...(results ? { results } : {}), - ...(followUps ? { followUps } : {}), + ...(followUps && followUps.length > 0 ? { followUps } : {}), ...(clarification ? { clarification } : {}), ...(forcedFinal ? { forcedFinal: true as const } : {}), ...(continuationOf ? { continuationOf } : {}), diff --git a/src/lib/ai/coach/turn/__tests__/follow-up-turn.test.ts b/src/lib/ai/coach/turn/__tests__/follow-up-turn.test.ts index 139fb6def..0dd1b1a3b 100644 --- a/src/lib/ai/coach/turn/__tests__/follow-up-turn.test.ts +++ b/src/lib/ai/coach/turn/__tests__/follow-up-turn.test.ts @@ -290,14 +290,7 @@ describe("a reuse chip", () => { const order = out .map((f) => f.type) .filter((type, i, all) => type !== "token" || all[i - 1] !== "token"); - expect(order).toEqual([ - "step", - "token", - "provenance", - "result", - "followUps", - "done", - ]); + expect(order).toEqual(["step", "token", "provenance", "result", "done"]); expect( out.flatMap((f) => (f.type === "token" ? [f.token] : [])).join(""), ).toBe("Here are the same figures from my last answer, unchanged."); @@ -310,11 +303,9 @@ describe("a reuse chip", () => { chart: STORED.chart, reusedFrom: { messageId: "m-last", ref: "r1" }, }); - const followUps = out.find((f) => f.type === "followUps"); - // Shown as a chart now, so the other view is what is offered. - expect( - followUps?.type === "followUps" && followUps.followUps.map((c) => c.kind), - ).toEqual(["as_table"]); + // The table is shown with its chart, and the panel carries its own + // chart/table toggle, so no chip offers the other view again. + expect(out.some((f) => f.type === "followUps")).toBe(false); const done = out.at(-1); expect(done).toMatchObject({ type: "done", @@ -327,7 +318,7 @@ describe("a reuse chip", () => { expect(annotated("coach.followUp.reused")).toHaveLength(1); }); - it("shows the table first on 'as a table' and offers the chart back", async () => { + it("shows the table first on 'as a table', leaving the chart to the panel's toggle", async () => { const asTable = { ...AS_CHART, kind: "as_table" as const }; m.findMany.mockResolvedValue(latestReply([asTable])); m.readMessageResults.mockResolvedValue([STORED]); @@ -338,10 +329,9 @@ describe("a reuse chip", () => { chart: STORED.chart, rows: STORED.rows, }); - const chips = out.find((f) => f.type === "followUps"); - expect( - chips?.type === "followUps" && chips.followUps.map((c) => c.kind), - ).toEqual(["as_chart"]); + // The chart stays on the result, one tap away on its toggle; a chip + // for it would repeat that toggle. + expect(out.some((f) => f.type === "followUps")).toBe(false); }); it("goes to the model with the chip's hint when the table is withheld", async () => {