From c38aba84b29f13c4d6ac940b47ce231e55b6558a Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Thu, 15 Oct 2026 08:44:04 +0900 Subject: feat(tool-call-cards): polish responsive headers and expanded details --- backend-handoff.md | 28 +++ biome.json | 2 + src/features/chat/logic/tool-summary.test.ts | 72 +++++- src/features/chat/logic/tool-summary.ts | 68 ++++-- src/features/chat/ui/ChatView.test.ts | 55 ++++- src/features/chat/ui/ToolCard.svelte | 316 +++++++++++++++++++-------- 6 files changed, 413 insertions(+), 128 deletions(-) diff --git a/backend-handoff.md b/backend-handoff.md index 4bc219c..4c46e7d 100644 --- a/backend-handoff.md +++ b/backend-handoff.md @@ -18,6 +18,34 @@ > ?archived=`) was made in the same branch and the FE consumes it via the `file:` deps (dists > rebuilt). Recorded below. +## Milestone: Tool-call card visual polish (`feature/tool-call-cards`, 2026-10-14) + +**Slice status:** COMPLETE (frontend-only follow-up to the user's headless-browser visual pass). + +- Card-width container queries prioritize call summaries: diffstats hide on narrow cards, + response summaries switch to complete compact facts (never mid-word CSS truncation), and + extremely narrow cards omit secondary response text. Error headers are simply `failed` + (or `timed out`); full errors remain in the response body. Wide response labels are also + budgeted by choosing a shorter whole fact instead of slicing arbitrary text. +- Edit badges now carry typed success/error tones. Orphans read “Call in earlier history” + in muted italic text, with a natural expanded explanation and de-emphasized success glyph. +- Output folding uses a high-contrast primary button; in full-output mode it moves ABOVE + the output and sticks to the transcript viewport while scrolling (no new scroll box). +- Call/Response are stronger heading elements with separators; generic input fields align + key/value columns. Short scalar arrays render inline; long/nested values retain full JSON. +- Regression coverage expanded for compact facts, error privacy in headers, orphan wording, + semantic badges/headings/fields, array formatting, and the output-control placement. +- Biome excludes local `scratch` fixtures and gitignored `reports` explicitly, so temporary + visual harnesses/artifacts do not affect the source-code check. User fixture files untouched. +- Verification: typecheck 0 errors/0 warnings; tests **1,929 passed ×2** (111 files); check + clean (existing 2 config infos only); build succeeds (existing app-store/bundle warnings). +- Browser: screenshots reviewed at **1280px and 420px**, collapsed and expanded, using the + user's running fixture harness. CDP measured no horizontal viewport overflow and confirmed + “Show fewer lines” remains at y=8px after scrolling 1,050px through the 111-line output; + actual mouse clicks fold it without closing the card at both widths. Local captures in + `reports/tool-call-polish/` (gitignored); scratch harness remains available on :24214. +- Backend, contract pins, mirrors, and open asks: **UNCHANGED; no backend asks**. + ## Milestone: Tool-call cards (`feature/tool-call-cards`, 2026-10-14) **Slice status:** COMPLETE (frontend-only, plan Steps 1–5 implemented; automated verification below). diff --git a/biome.json b/biome.json index 528c1a9..de73a49 100644 --- a/biome.json +++ b/biome.json @@ -11,6 +11,8 @@ "!**/node_modules", "!**/dist", "!**/build", + "!scratch", + "!reports", "!**/*.svelte", "!**/src/themes" ] diff --git a/src/features/chat/logic/tool-summary.test.ts b/src/features/chat/logic/tool-summary.test.ts index 5caa1f1..381842a 100644 --- a/src/features/chat/logic/tool-summary.test.ts +++ b/src/features/chat/logic/tool-summary.test.ts @@ -69,7 +69,10 @@ describe("tool summaries", () => { it("formats edit call badges and old/new details without hiding multiline text", () => { const input = { path: "a.ts", oldString: "a\nb\n", newString: "x\ny\nz\n" }; const card = view("edit_file", input); - expect(card.callBadges).toEqual(["+3 −2"]); + expect(card.callBadges).toEqual([ + { label: "+3", tone: "success" }, + { label: "−2", tone: "error" }, + ]); expect(card.callDetail).toEqual({ kind: "edit", path: "a.ts", @@ -77,7 +80,8 @@ describe("tool summaries", () => { newText: input.newString, }); expect(view("edit_file", { path: "a", oldString: "", newString: "x" }).callBadges).toEqual([ - "+1 −0", + { label: "+1", tone: "success" }, + { label: "−0", tone: "error" }, ]); expect(view("edit_file", { path: "a" }).callBadges).toEqual([]); }); @@ -97,6 +101,33 @@ describe("tool summaries", () => { ], }); }); + it("formats short scalar arrays inline, retaining quotes, escapes, nulls and empty arrays", () => { + expect( + view("mcp", { + labels: ["ui", "chat"], + scalars: [1, true, null], + empty: [], + escaped: ["a\nb"], + }).callDetail, + ).toEqual({ + kind: "fields", + fields: [ + { key: "labels", value: '["ui", "chat"]', multiline: false }, + { key: "scalars", value: "[1, true, null]", multiline: false }, + { key: "empty", value: "[]", multiline: false }, + { key: "escaped", value: '["a\\nb"]', multiline: false }, + ], + }); + }); + it("keeps long arrays and nested structures pretty-printed and complete", () => { + const input = { long: ["x".repeat(80)], nested: [{ a: 1 }], mixed: [1, { a: 1 }] }; + const card = view("mcp", input); + if (card.callDetail.kind !== "fields") throw new Error("expected fields"); + expect(card.callDetail.fields.map((field) => field.multiline)).toEqual([true, true, true]); + expect(card.callDetail.fields.map((field) => JSON.parse(field.value))).toEqual( + Object.values(input), + ); + }); it.each([ "command", "path", @@ -192,14 +223,12 @@ describe("tool summaries", () => { expect(viewToolEntry(e)).toMatchObject({ name: "read_file", status: "ok", - callSummary: "call unavailable", + callSummary: "Call in earlier history", resultSummary: "1 line", }); expect(viewToolEntry(e).callDetail).toEqual({ - kind: "fields", - fields: [ - { key: "call", value: "Call unavailable (earlier history unloaded)", multiline: false }, - ], + kind: "unavailable", + text: "The call is in earlier, unloaded history. Its response is shown below.", }); }); }); @@ -213,14 +242,15 @@ describe("tool response facts", () => { ["mcp", "", false, "no output"], ["mcp", " \n\t", false, "no output"], ["mcp", "a short response", false, "a short response"], + ["mcp", "A successful response with more than forty characters of text", false, "1 line"], ["mcp", "x".repeat(200), false, "1 line"], ["web_search", "a\nb\nc", false, "3 lines"], ["run_shell", "", true, "failed"], - ["mcp", "Permission denied", true, "failed · Permission denied"], - ["run_shell", "error\ndetail", true, "failed · 2 lines"], + ["mcp", "Permission denied", true, "failed"], + ["run_shell", "error\ndetail", true, "failed"], ["run_shell", "partial\n\n[Command timed out]", true, "timed out"], ["run_shell", "[Output truncated: 12000 bytes]", false, "1 line · truncated"], - ["run_shell", "bad\n[Output truncated: 12000 bytes]", true, "failed · 2 lines · truncated"], + ["run_shell", "bad\n[Output truncated: 12000 bytes]", true, "failed · truncated"], ["edit_file", 'Replaced 1 occurrence(s) in "a.ts".', false, "1 replacement"], [ "edit_file", @@ -237,6 +267,28 @@ describe("tool response facts", () => { expect(card.status).toBe(isError ? "error" : "ok"); expect(card.output?.isError).toBe(isError); }); + it.each([ + ["run_shell", "oops\ndetails", true, "failed"], + ["run_shell", "partial\n[Command timed out]", true, "timed out"], + ["write_file", 'Created "a.ts" (4200 bytes).', false, "created"], + ["write_file", 'Overwrote "a.ts" (10 bytes).', false, "overwrote"], + ["edit_file", 'Replaced 3 occurrences in "a.ts".\nDiagnostics: warning', false, "replaced"], + ["run_shell", "a\nb\nc\nd", false, "4 lines"], + ["mcp", "Created issue #42", false, "1 line"], + ["mcp", "", false, "no output"], + ["read_file", "(empty file: a.ts)", false, "empty file"], + ])("%s supplies a complete compact response fact", (name, content, isError, expected) => { + expect(view(name, {}, content, isError).compactResultSummary).toBe(expected); + }); + it("never puts error content in header summaries, but retains it verbatim in output", () => { + const content = 'Error: oldString not found in content of "src/missing.ts".'; + const card = view("edit_file", { path: "src/missing.ts" }, content, true); + expect(card.resultSummary).toBe("failed"); + expect(card.compactResultSummary).toBe("failed"); + expect(card.output).toEqual({ kind: "full", lines: [content], isError: true }); + expect(card.callSummary).toBe("src/missing.ts"); + expect(view("mcp", {}, null).compactResultSummary).toBeNull(); + }); it("does not infer success on an error that resembles a write result", () => { expect(view("write_file", {}, 'Created "a.ts" (10 bytes).', true).resultSummary).toMatch( /^failed/, diff --git a/src/features/chat/logic/tool-summary.ts b/src/features/chat/logic/tool-summary.ts index 3b75a6b..2e7680c 100644 --- a/src/features/chat/logic/tool-summary.ts +++ b/src/features/chat/logic/tool-summary.ts @@ -4,9 +4,14 @@ export interface ToolCardView { readonly name: string; readonly status: "pending" | "ok" | "error"; readonly callSummary: string; - readonly callBadges: readonly string[]; + readonly callBadges: readonly { + readonly label: string; + readonly tone: "success" | "error"; + }[]; readonly resultSummary: string | null; + readonly compactResultSummary: string | null; readonly callDetail: + | { readonly kind: "unavailable"; readonly text: string } | { readonly kind: "command"; readonly text: string } | { readonly kind: "edit"; @@ -68,6 +73,22 @@ function serialize(input: unknown, pretty = false): string { } } +/** Small scalar arrays are easier to scan inline; nested/long values stay pretty JSON. */ +function fieldValue(value: unknown): string { + if ( + Array.isArray(value) && + value.every((item) => item === null || ["string", "number", "boolean"].includes(typeof item)) + ) { + try { + const inline = `[${value.map((item) => JSON.stringify(item)).join(", ")}]`; + if (inline.length <= 80) return inline; + } catch { + // Non-wire values still use the safe serializer below. + } + } + return serialize(value, true); +} + function arg(args: Args, ...keys: string[]): string { for (const key of keys) { const value = Object.hasOwn(args, key) ? args[key] : undefined; @@ -146,7 +167,7 @@ function detail(name: string, input: unknown, args: Args): ToolCardView["callDet return { kind: "fields", fields: (entries.length > 0 ? entries : [["input", input]]).map(([key, value]) => { - const text = serialize(value, true); + const text = fieldValue(value); return { key: String(key), value: text, multiline: text.includes("\n") }; }), }; @@ -166,8 +187,7 @@ function responseSummary(name: string, content: string, isError: boolean): strin const truncated = /\[Output truncated[^\]]*\]/i.test(content); let summary: string; if (timeout) summary = "timed out"; - else if (isError) - summary = `failed${lines.length > 1 ? ` · ${count}` : first ? ` · ${first}` : ""}`; + else if (isError) summary = "failed"; else if (!content.trim()) summary = "no output"; else if (/^\(empty file\b/i.test(content)) summary = "empty file"; else { @@ -189,7 +209,19 @@ function responseSummary(name: string, content: string, isError: boolean): strin summary = count; } else summary = first; } - return oneLine(`${summary}${truncated ? " · truncated" : ""}`, 96); + const label = `${summary}${truncated ? " · truncated" : ""}`; + // Even the wide header must leave room for the call; prefer a whole short + // fact over chopping an arbitrary one-line response halfway through a word. + return label.length <= 40 ? label : compactResponseSummary(label, lines.length); +} + +/** Never CSS-clip response facts: swap to a whole short fact when space is scarce. */ +function compactResponseSummary(summary: string, count: number): string { + const fact = summary.split(" · ")[0] ?? summary; + if (/^(failed|timed out|no output|empty file|created|overwrote|\d+ lines?)$/.test(fact)) + return fact; + if (/^\d+ replacements?$/.test(fact)) return "replaced"; + return `${count} ${count === 1 ? "line" : "lines"}`; } export function viewToolEntry(entry: ToolEntry): ToolCardView { @@ -216,16 +248,21 @@ export function viewToolEntry(entry: ToolEntry): ToolCardView { const builtInSummary = summarize?.(args).trim(); const callSummary = entry.call === null - ? "call unavailable" + ? "Call in earlier history" : oneLine(builtInSummary || generic || serialize(input)); - const callBadges: string[] = []; + const callBadges: ToolCardView["callBadges"][number][] = []; if (entry.toolName === "edit_file" && entry.call !== null) { if (typeof args.oldString === "string" && typeof args.newString === "string") { - callBadges.push(`+${lineCount(args.newString)} −${lineCount(args.oldString)}`); + callBadges.push( + { label: `+${lineCount(args.newString)}`, tone: "success" }, + { label: `−${lineCount(args.oldString)}`, tone: "error" }, + ); } } const result = entry.result; const lines = result === null ? [] : outputLines(result.content); + const resultSummary = + result === null ? null : responseSummary(entry.toolName, result.content, result.isError); return { name: entry.toolName, status: result === null ? "pending" : result.isError ? "error" : "ok", @@ -234,18 +271,13 @@ export function viewToolEntry(entry: ToolEntry): ToolCardView { callDetail: entry.call === null ? { - kind: "fields", - fields: [ - { - key: "call", - value: "Call unavailable (earlier history unloaded)", - multiline: false, - }, - ], + kind: "unavailable", + text: "The call is in earlier, unloaded history. Its response is shown below.", } : detail(entry.toolName, input, args), - resultSummary: - result === null ? null : responseSummary(entry.toolName, result.content, result.isError), + resultSummary, + compactResultSummary: + resultSummary === null ? null : compactResponseSummary(resultSummary, lines.length), output: result === null ? null diff --git a/src/features/chat/ui/ChatView.test.ts b/src/features/chat/ui/ChatView.test.ts index d12088b..412bf2d 100644 --- a/src/features/chat/ui/ChatView.test.ts +++ b/src/features/chat/ui/ChatView.test.ts @@ -244,7 +244,8 @@ describe("ChatView — tool cards", () => { expect(box).not.toBeChecked(); expect(screen.getByLabelText("Failed")).toHaveClass("text-error"); expect(screen.getByLabelText("Failed")).toHaveTextContent("✗"); - expect(screen.getByText("→ failed · Permission denied")).toHaveClass("text-error"); + expect(screen.getByText("→ failed").closest(".tool-result")).toHaveClass("text-error"); + expect(container.querySelector(".tool-header")).not.toHaveTextContent("Permission denied"); expect(container.querySelector("pre")).toBeNull(); await userEvent.click(box); expect(screen.getByText("Permission denied").parentElement).toHaveClass("text-error"); @@ -253,10 +254,11 @@ describe("ChatView — tool cards", () => { const chunks = shellChunks("contents", false, true); const { container, rerender } = render(ChatView, { props: { chunks: chunks.slice(1) } }); expect(container.querySelectorAll(".chat-bubble")).toHaveLength(1); - expect(screen.getByText("call unavailable")).toBeInTheDocument(); + expect(screen.getByText("Call in earlier history")).toHaveClass("earlier-history"); + expect(screen.getByLabelText("Succeeded")).toHaveClass("opacity-60"); const box = screen.getByRole("checkbox", { name: "Toggle tool call run_shell" }); await userEvent.click(box); - expect(screen.getByText(/Call unavailable \(earlier history unloaded\)/)).toBeInTheDocument(); + expect(screen.getByText(/The call is in earlier, unloaded history/)).toBeInTheDocument(); expect(screen.getByText("contents")).toBeInTheDocument(); await rerender({ chunks }); expect(screen.getByRole("checkbox", { name: "Toggle tool call run_shell" })).toBe(box); @@ -280,6 +282,12 @@ describe("ChatView — tool cards", () => { expect(response).toHaveTextContent("output-7"); expect(response.querySelectorAll("pre")).toHaveLength(1); expect(response.querySelector("pre")?.textContent).toBe(output); + const fewer = screen.getByRole("button", { name: "Show fewer lines" }); + expect(fewer).toHaveClass("btn-primary", "btn-sm"); + expect(fewer.parentElement).toHaveClass("sticky-controls"); + const fullOutput = response.querySelector("pre"); + if (fullOutput === null) throw new Error("expected full output"); + expect(fewer.compareDocumentPosition(fullOutput)).toBe(Node.DOCUMENT_POSITION_FOLLOWING); expect(box).toBeChecked(); expect(screen.getByRole("button", { name: "Show fewer lines" })).toHaveAttribute( "aria-expanded", @@ -333,7 +341,8 @@ describe("ChatView — tool cards", () => { }, ]; const { container } = render(ChatView, { props: { chunks } }); - expect(screen.getByText("+2 −1")).toBeInTheDocument(); + expect(screen.getByText("+2")).toHaveClass("text-success"); + expect(screen.getByText("−1")).toHaveClass("text-error"); expect(screen.getByText("→ 1 replacement · diagnostics")).toBeInTheDocument(); await userEvent.click(screen.getByRole("checkbox", { name: "Toggle tool call edit_file" })); expect(screen.getByText("before")).toHaveClass("text-error"); @@ -342,3 +351,41 @@ describe("ChatView — tool cards", () => { expect(container).toHaveTextContent(""); }); }); + +it("renders compact, aligned generic fields with legible headings and inline scalar arrays", async () => { + const chunks: RenderedChunk[] = [ + { + seq: 1, + role: "assistant", + provisional: false, + chunk: { + type: "tool-call", + toolCallId: "issue", + toolName: "mcp__github__create_issue", + input: { + owner: "realtradam", + repo: "frontend", + title: "Tool cards", + labels: ["ui", "chat"], + options: { nested: true }, + }, + }, + }, + ]; + const { container } = render(ChatView, { props: { chunks } }); + await userEvent.click( + screen.getByRole("checkbox", { name: "Toggle tool call mcp__github__create_issue" }), + ); + for (const name of ["Call", "Response"]) { + expect(screen.getByRole("heading", { name })).toHaveClass("font-semibold", "text-base-content"); + } + const call = screen.getByRole("region", { name: "Call" }); + expect(call.querySelector("dl")).toHaveClass("call-fields"); + expect(screen.getByText("owner")).toHaveClass("font-semibold", "text-base-content/85"); + const labels = screen.getByText('["ui", "chat"]'); + expect(labels.tagName).toBe("PRE"); + expect(labels.textContent).not.toContain("\n"); + expect(container.querySelectorAll("dt")).toHaveLength(5); + expect(container.querySelectorAll("dd")).toHaveLength(5); + expect(screen.getByText("options").nextElementSibling).toHaveTextContent('"nested": true'); +}); diff --git a/src/features/chat/ui/ToolCard.svelte b/src/features/chat/ui/ToolCard.svelte index 8c6e325..f1a56cb 100644 --- a/src/features/chat/ui/ToolCard.svelte +++ b/src/features/chat/ui/ToolCard.svelte @@ -8,104 +8,228 @@ let showAll = $state(false); - - {#snippet title()} - {#if card.status === "pending"} - - {:else if card.status === "error"} - ✗ - {:else} - ✓ - {/if} - - {card.name} - {card.callSummary} - {#each card.callBadges as badge} - {badge} - {/each} - {#if card.resultSummary !== null} - - → {card.resultSummary} - - {/if} - {/snippet} -
-
-
Call
- {#if card.callDetail.kind === "command"} - 
$ {card.callDetail.text}
- {:else if card.callDetail.kind === "edit"} -
{card.callDetail.path}
-
-
− old
- 
{card.callDetail.oldText}
-
+ new
- 
{card.callDetail.newText}
-
- {:else} -
- {#each card.callDetail.fields as field} -
-
{field.key}
-
+
+ + {#snippet title()} +
+ {#if card.status === "pending"} + + {:else if card.status === "error"} + ✗ + {:else} + ✓ + {/if} + + {card.name} + + {card.callSummary} + + + {#each card.callBadges as badge} + {badge.label} + {/each} + + {#if card.resultSummary !== null} + + {#if card.resultSummary === card.compactResultSummary} + → {card.resultSummary} + {:else} + → {card.resultSummary} + → {card.compactResultSummary} + {/if} + + {/if} +
+ {/snippet} +
+
+

+ Call +

+ {#if card.callDetail.kind === "unavailable"} +

{card.callDetail.text}

+ {:else if card.callDetail.kind === "command"} + 
$ {card.callDetail.text}
+ {:else if card.callDetail.kind === "edit"} +
{card.callDetail.path}
+
+
− old
+ 
{card.callDetail.oldText}
+
+ new
+ 
{card.callDetail.newText}
+
+ {:else} +
+ {#each card.callDetail.fields as field} +
{field.key}
+ 
{field.value}
+ class:whitespace-pre-wrap={!field.multiline} + class:break-words={!field.multiline}>{field.value}
-
- {/each} -
- {/if} -
-
-
Response
- {#if card.output === null} - running… - {:else} -
- {#if card.output.kind === "full"} - 
{card.output.lines.join("\n") || "(no output)"}
- {:else} - {#if showAll} - 
{card.output.all.join("\n")}
+ {/each} + + {/if} +
+
+

+ Response +

+ {#if card.output === null} + running… + {:else} +
+ {#if card.output.kind === "full"} + 
{card.output.lines.join("\n") || "(no output)"}
{:else} - 
{card.output.head.join("\n")}
- {/if} - - {#if !showAll} - 
{card.output.tail.join("\n")}
+ {#if !showAll} + 
{card.output.head.join("\n")}
+ {/if} +
+ +
+ {#if showAll} + 
{card.output.all.join("\n")}
+ {:else} + 
{card.output.tail.join("\n")}
+ {/if} {/if} - {/if} -
- {/if} -
-
- + + {/if} + + + + + + -- cgit v1.2.3