From ff99778e5d008e6af1f9a844e350d20c762f8f45 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Mon, 5 Oct 2026 09:49:39 +0000 Subject: [PATCH] fix: print the running tool once, not twice (#17) --- README.md | 4 ++- src/__tests__/markdown.test.ts | 58 +++++++++++++++++++++++++++++++++ src/__tests__/thread.test.ts | 47 +++++++++++++++++++++++++- src/lib/components/ChatView.tsx | 38 +++++++++------------ src/lib/markdown.ts | 37 ++++++++++++++++----- src/lib/thread.ts | 20 +++++++++++- src/lib/types.ts | 28 ++++++++++++++++ 7 files changed, 198 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index c2cda82..4f69072 100644 --- a/README.md +++ b/README.md @@ -225,7 +225,9 @@ The cap is a preference rather than a constant, because how many rounds a questi Arguments are compared with their keys sorted, because models are not consistent about key order and a call that differs only by key order is still the same call. -**What Lam is doing is visible while it is doing it, even with tool calls hidden.** `Show Tool Calls` governs the finished transcript, where a list of names adds little. The live activity line is separate and always shown, because during a turn the difference between working and circling is the entire question: `_Reading a page…_` for a first call, and `_Reading a page · the same call again (2)_` once it has come round. Hiding that would leave the user watching a spinner they cannot interpret, which is the situation the repeat guard exists to fix. A call that failed, or that was refused for repeating, is also listed in the transcript even with tool calls hidden: those are the two cases where hiding the line leaves an answer the user cannot account for. +**What Lam is doing is visible while it is doing it, even with tool calls hidden.** `Show Tool Calls` governs the finished transcript, where a list of names adds little. The live activity line is separate and always shown, because during a turn the difference between working and circling is the entire question: `_Reading a page…_` for a first call, and `_Reading a page · the same call again (2)…_` once it has come round. Hiding that would leave the user watching a spinner they cannot interpret, which is the situation the repeat guard exists to fix. A call that failed, or that was refused for repeating, is also listed in the transcript even with tool calls hidden: those are the two cases where hiding the line leaves an answer the user cannot account for. + +**That line is rendered in exactly one place, on the live message.** It is not a view-level note above the transcript, and it is not a substitute for missing text either: it is shown whether or not tokens have arrived, because a tool that starts mid-answer would otherwise be invisible for the rest of the turn. The first version had both, a note in the view and the placeholder in the transcript, and a tool running before any text arrived printed "Reading a page" twice, one line above the other. The cursor rides on whichever line is written last, so it always trails the text actually arriving. ## Auto-deletion diff --git a/src/__tests__/markdown.test.ts b/src/__tests__/markdown.test.ts index 27b7322..dfde2bd 100644 --- a/src/__tests__/markdown.test.ts +++ b/src/__tests__/markdown.test.ts @@ -116,6 +116,64 @@ describe("buildThreadMarkdown", () => { expect(markdown).not.toContain("Thinking"); }); + it("says what a running tool is doing exactly once, before any text arrives", () => { + // The reported bug, from a screenshot: the activity line and the answer + // placeholder both printed "Reading a page", one directly above the other. + const markdown = buildThreadMarkdown([ + message({ + role: "assistant", + isStreaming: true, + text: "", + activity: { label: "Reading a page", count: 1 }, + }), + ]); + + expect(markdown.match(/Reading a page/g)).toHaveLength(1); + // With nothing to read yet, the activity line is the stand-in, so the cursor + // rides on it rather than sitting on a line of its own. + expect(markdown).toContain(`_Reading a page…_ ${STREAMING_CURSOR}`); + }); + + it("keeps the activity line once the answer has started arriving", () => { + // The other half of the fix: a tool that starts mid-answer has to stay on + // screen, which is why the line is not just a stand-in for missing text. + const markdown = buildThreadMarkdown([ + message({ + role: "assistant", + isStreaming: true, + text: "Here is what I found.", + activity: { label: "Reading a page", count: 1 }, + }), + ]); + + expect(markdown).toContain("_Reading a page…_"); + expect(markdown).toContain("Here is what I found."); + // And the cursor follows the answer, not the activity line. + expect(markdown).toContain(`Here is what I found. ${STREAMING_CURSOR}`); + expect(markdown).not.toContain(`_Reading a page…_ ${STREAMING_CURSOR}`); + }); + + it("names a repeat rather than leaving a bare count to be interpreted", () => { + const markdown = buildThreadMarkdown([ + message({ + role: "assistant", + isStreaming: true, + activity: { label: "Searching the web", count: 2 }, + }), + ]); + + expect(markdown).toContain("the same call again (2)"); + }); + + it("drops the activity line when the message is no longer streaming", () => { + const markdown = buildThreadMarkdown([ + message({ role: "assistant", text: "done", activity: { label: "Searching the web", count: 1 } }), + ]); + + expect(markdown).not.toContain("Searching the web"); + expect(markdown).not.toContain(STREAMING_CURSOR); + }); + it("shows a placeholder while an answer has no text yet", () => { expect(buildThreadMarkdown([message({ role: "assistant", isStreaming: true })])).toContain("Thinking"); expect(buildThreadMarkdown([message({ role: "assistant" })])).toContain("No response"); diff --git a/src/__tests__/thread.test.ts b/src/__tests__/thread.test.ts index 9106f9b..04a2d2e 100644 --- a/src/__tests__/thread.test.ts +++ b/src/__tests__/thread.test.ts @@ -1,9 +1,11 @@ import { describe, expect, it } from "vitest"; -import { buildThread, type LiveState } from "../lib/thread"; +import { buildThread, describeActivity, type LiveState } from "../lib/thread"; import type { ChatTurn } from "../lib/types"; const idle: LiveState = { isStreaming: false, response: "", thinking: "" }; +const activity = { label: "Reading a page", count: 1 }; + function turn(id: string, prompt: string, response: string, overrides: Partial = {}): ChatTurn { return { id, @@ -133,3 +135,46 @@ describe("buildThread", () => { expect(thread[0].error).toBe("Request failed"); }); }); + +describe("carrying the running tool", () => { + const live: LiveState = { isStreaming: true, response: "", thinking: "", activity }; + + it("puts it on the message being streamed", () => { + const thread = buildThread([turn("t1", "q", "a")], live); + const assistant = thread.find((message) => message.role === "assistant"); + + expect(assistant?.activity).toEqual(activity); + }); + + it("keeps it off every earlier message", () => { + // Only the newest turn can be mid-flight, and an older answer carrying a + // "Searching the web" line would be a claim about a finished turn. + const thread = buildThread([turn("t1", "q", "a"), turn("t2", "q2", "a2")], live); + + expect(thread.slice(2).every((message) => message.activity === undefined)).toBe(true); + }); + + it("drops it once the turn is over", () => { + const thread = buildThread([turn("t1", "q", "a")], { ...idle, activity }); + + expect(thread.every((message) => message.activity === undefined)).toBe(true); + }); + + it("never puts one on a question", () => { + const thread = buildThread([turn("t1", "q", "a")], live); + + expect(thread.find((message) => message.role === "user")?.activity).toBeUndefined(); + }); +}); + +describe("describeActivity", () => { + it("is just the label for a first call", () => { + expect(describeActivity({ label: "Reading a page", count: 1 })).toBe("Reading a page"); + }); + + it("says a repeat is a repeat, because a bare number reads as progress", () => { + expect(describeActivity({ label: "Reading a page", count: 3 })).toBe( + "Reading a page · the same call again (3)", + ); + }); +}); diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index 42abce6..bd89895 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -2,7 +2,7 @@ import { Action, ActionPanel, Icon, LaunchType, List, launchCommand } from "@vic import { useCallback, useEffect, useMemo, useState } from "react"; import { readAttachment } from "../attachments"; import { runAutoDelete } from "../cleanup"; -import { useChat, type ToolActivity } from "../hooks/useChat"; +import { useChat } from "../hooks/useChat"; import { usePreferences } from "../hooks/usePreferences"; import { useVisionSupport } from "../hooks/useVision"; import { buildThreadMarkdown, renderAttachments, type TranscriptOptions } from "../markdown"; @@ -63,18 +63,6 @@ const DEFAULT_IMAGE_PROMPT = "What is in this image?"; /** The reminder shown while a snip is attached but not yet sent. */ const PENDING_NOTE = "_Ask a question about it, or press Return to send it as it is._"; -/** - * The line that shows what Lam is doing right now. - * - * A count on its own reads as progress, so the second time round says what it is: - * the same call, asked again. That is the visible half of the loop guard in - * `agent.ts`, and it is the only warning the user gets before a turn is cut short. - */ -function describeActivity(activity: ToolActivity): string { - if (activity.count <= 1) return `_${activity.label}…_`; - return `_${activity.label} · the same call again (${activity.count})_`; -} - export interface ChatViewProps { /** Opens a specific conversation instead of resuming the latest one. */ initialSession?: Session; @@ -138,8 +126,6 @@ function ChatSession({ const chat = useChat({ preferences, initialSession, vision: vision.support }); const { replaceSession } = chat; const { canSeeImages } = vision; - const { activity } = chat; - const [input, setInput] = useState(initialPrompt ?? ""); const [summaries, setSummaries] = useState(undefined); // One flag, not two. See the startup effect below for why that matters. @@ -232,8 +218,16 @@ function ChatSession({ response: chat.streamingResponse, thinking: chat.streamingThinking, status: chat.status, + activity: chat.activity, }), - [chat.session.turns, chat.isStreaming, chat.streamingResponse, chat.streamingThinking, chat.status], + [ + chat.session.turns, + chat.isStreaming, + chat.streamingResponse, + chat.streamingThinking, + chat.status, + chat.activity, + ], ); const transcriptOptions = useMemo( @@ -255,12 +249,10 @@ function ChatSession({ // The preview comes first so the captured image is visible before the user // decides what to ask about it, rather than only after sending. pending ? `${renderAttachments([pending])}\n\n${PENDING_NOTE}` : undefined, - // Deliberately not gated on the Show Tool Calls preference. That setting is - // about the finished transcript, where a list of names adds nothing. This - // is about the turn in flight, where the same tool coming round a second - // time is the difference between a long answer and a stuck one, and hiding - // it would leave the user watching a spinner they cannot interpret. - activity ? describeActivity(activity) : undefined, + // Deliberately nothing about the running tool here. That line is rendered by + // the transcript, on the live message, so that one fact is printed once: + // adding it here as well put "Reading a page" on screen twice, one line + // above the other, while a tool ran before any text had arrived. ].filter((note): note is string => Boolean(note)); // The placeholder is what tells a user that the search bar is the composer. @@ -271,7 +263,7 @@ function ChatSession({ thread.length === 0 ? empty : buildThreadMarkdown(thread, transcriptOptions); return [...notes, transcript].filter((part) => part.length > 0).join("\n\n"); - }, [launchNote, snipNote, pending, activity, thread, transcriptOptions]); + }, [launchNote, snipNote, pending, thread, transcriptOptions]); const rows: SessionRow[] = useMemo(() => { const base = summaries ?? []; diff --git a/src/lib/markdown.ts b/src/lib/markdown.ts index 2998031..05d4992 100644 --- a/src/lib/markdown.ts +++ b/src/lib/markdown.ts @@ -1,5 +1,6 @@ import { imageSource } from "./attachments"; import { repairCodeBlocks } from "./code"; +import { describeActivity } from "./thread"; import type { Attachment, ChatTurn, ThreadMessage } from "./types"; /** @@ -82,17 +83,37 @@ function renderMessage(message: ThreadMessage, options: TranscriptOptions): stri } // While a tool is running there is no answer text yet, so the activity line - // stands in and the user can see what Lam is actually doing. - const placeholder = message.status ? `_${message.status}…_` : "_Thinking…_"; - const body = message.text.trim() + // stands in and the user can see what Lam is actually doing. It is shown + // whether or not tokens have arrived, because a tool that starts after the + // answer has begun would otherwise be invisible for the rest of the turn. + const activityLine = + message.isStreaming && message.activity ? `_${describeActivity(message.activity)}…_` : undefined; + + /** The answer, or whatever stands in for it while there is none. */ + const answered = message.text.trim(); + const body = answered ? // Fences without a language render as a plain dark panel, and a lone // inline command renders as a chip with no Copy button at all. Both are // repaired before the markdown reaches the renderer. - repairCodeBlocks(message.text.trim()) - : message.isStreaming - ? placeholder - : "_No response._"; - const sections = [message.isStreaming ? `${body} ${STREAMING_CURSOR}` : body]; + repairCodeBlocks(answered) + : !message.isStreaming + ? "_No response._" + : activityLine + ? // The activity line above is already saying what is being waited on, so + // printing the placeholder as well put the same sentence on screen + // twice: once as a note above the transcript and once as the stand-in. + undefined + : message.status + ? `_${message.status}…_` + : "_Thinking…_"; + + // The cursor trails whichever of the two is written last, so it always sits at + // the end of the text arriving rather than on a line of its own. + const written = [activityLine, body].filter((line): line is string => Boolean(line)); + const last = written.length - 1; + const sections = written.map((line, index) => + message.isStreaming && index === last ? `${line} ${STREAMING_CURSOR}` : line, + ); // The one thing allowed to break the plain-text rule, because it means // something went wrong and should not be skimmed past. diff --git a/src/lib/thread.ts b/src/lib/thread.ts index c7a6326..67714a6 100644 --- a/src/lib/thread.ts +++ b/src/lib/thread.ts @@ -1,4 +1,4 @@ -import type { ChatTurn, ThreadMessage } from "./types"; +import type { ChatTurn, ThreadMessage, ToolActivity } from "./types"; /** * Flattens stored turns into the flat message list the transcript renders. @@ -15,6 +15,20 @@ export interface LiveState { thinking: string; /** Activity line, e.g. "Searching the web". Only meaningful mid-turn. */ status?: string; + /** The tool being waited on right now, which supersedes the plain status when present. */ + activity?: ToolActivity; +} + +/** + * One line describing a tool that is running. + * + * Says "the same call again" rather than showing a bare count, because a number + * on its own reads as progress. This is the visible half of the loop guard in + * `agent.ts`, and the only warning the user gets before a turn is cut short. + */ +export function describeActivity(activity: ToolActivity): string { + if (activity.count <= 1) return activity.label; + return `${activity.label} · the same call again (${activity.count})`; } export function buildThread(turns: ChatTurn[], live: LiveState): ThreadMessage[] { @@ -43,6 +57,10 @@ export function buildThread(turns: ChatTurn[], live: LiveState): ThreadMessage[] // Only while there is nothing to read yet. Once tokens land, the // placeholder it feeds is gone anyway. status: isLive && text.length === 0 ? live.status : undefined, + // The tool, on the other hand, is worth naming even after the answer has + // started arriving: a tool that begins mid-answer would otherwise be + // invisible, which was the original complaint about hidden tool calls. + activity: isLive ? live.activity : undefined, // Only a question carries attachments, so the answer never has one. attachments: [], }); diff --git a/src/lib/types.ts b/src/lib/types.ts index b642c75..d5d8b0a 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -158,6 +158,24 @@ export interface SessionSummary { */ export type VisionSupport = "yes" | "no" | "unknown"; +/** + * The tool running right now, and how stale it is getting. + * + * Lives here rather than beside the chat state machine so that `thread.ts` can + * carry it into the transcript, which is the only place it is now rendered. + */ +export interface ToolActivity { + /** Human-readable label, e.g. "Searching the web". */ + label: string; + /** + * How many times this turn has now started this *identical* call. + * + * Counted by the agent over name and arguments together, so two different + * searches are two tools doing their job rather than a repeat. + */ + count: number; +} + /** A model as reported by `GET /api/tags`. */ export interface ModelInfo { name: string; @@ -204,6 +222,16 @@ export interface ThreadMessage { error?: string; /** Activity line shown while this message is streaming, e.g. "Searching the web". */ status?: string; + /** + * The tool this message is waiting on, when there is one. + * + * Separate from {@link ThreadMessage.status} because the two are rendered from + * different triggers: the status is a stand-in used only while there is no text + * to read, while a running tool is worth saying whether or not tokens have + * started arriving. Carrying both is what lets one line be printed for one + * fact, rather than the same sentence appearing twice at the top of a turn. + */ + activity?: ToolActivity; /** Images attached to this question. Only ever non-empty on a user message. */ attachments: Attachment[]; }