From 889add734e700f6a930a6b9bb25df842aa97294e Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 20 May 2026 21:58:19 +0100 Subject: [PATCH] fix(site): use ToolCollapsible for thinking blocks (#25445) --- .../AgentsPage/AgentChatPage.stories.tsx | 20 ++ .../ConversationTimeline.stories.tsx | 197 ++++++++++++++++-- .../ChatConversation/ConversationTimeline.tsx | 96 +++------ .../StreamingOutput.stories.tsx | 33 ++- .../ChatConversation/StreamingOutput.tsx | 13 +- .../ChatConversation/messageHelpers.test.ts | 108 +++++++++- .../ChatConversation/messageHelpers.ts | 41 +++- .../components/ChatElements/TranscriptRow.tsx | 23 ++ .../tools/AskUserQuestionTool.tsx | 23 +- .../ChatElements/tools/ExecuteTool.tsx | 35 ++-- .../ChatElements/tools/ProcessOutputTool.tsx | 8 +- .../ChatElements/tools/ProposePlanTool.tsx | 9 +- .../ChatElements/tools/ReadTemplateTool.tsx | 5 +- .../ChatElements/tools/SubagentTool.tsx | 95 +++++---- .../ChatElements/tools/Tool.stories.tsx | 1 + .../components/ChatElements/tools/Tool.tsx | 81 +------ .../ChatElements/tools/ToolCollapsible.tsx | 60 ++++-- .../ChatElements/tools/toolVisibility.test.ts | 122 +++++++++++ .../ChatElements/tools/toolVisibility.ts | 112 ++++++++++ .../components/ChatPageContent.stories.tsx | 51 ++++- 20 files changed, 847 insertions(+), 286 deletions(-) create mode 100644 site/src/pages/AgentsPage/components/ChatElements/TranscriptRow.tsx create mode 100644 site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.test.ts create mode 100644 site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts diff --git a/site/src/pages/AgentsPage/AgentChatPage.stories.tsx b/site/src/pages/AgentsPage/AgentChatPage.stories.tsx index e510b4cba9..aeac0227b1 100644 --- a/site/src/pages/AgentsPage/AgentChatPage.stories.tsx +++ b/site/src/pages/AgentsPage/AgentChatPage.stories.tsx @@ -2433,7 +2433,27 @@ export const WithEveryTool: Story = { expect(canvas.getByText(/Editing 2 files/)).toBeInTheDocument(); expect(canvas.getByText(/Reading CHANGELOG\.md/)).toBeInTheDocument(); expect(canvas.getByText(/Writing CHANGELOG\.md/)).toBeInTheDocument(); + expect(canvas.getByText(/Attached auth-split\.md/)).toBeInTheDocument(); + expect( + canvas.getByRole("button", { name: /Spawned Workspace diagnostics/i }), + ).toBeInTheDocument(); + expect( + canvas.getByRole("button", { name: /Read skill deep-review/i }), + ).toBeInTheDocument(); }); + + const rowHeights = [ + canvas.getByText(/Attached auth-split\.md/), + canvas.getByRole("button", { + name: /Spawned Workspace diagnostics/i, + }), + canvas.getByRole("button", { name: /Read skill deep-review/i }), + ].map((label) => { + const row = label.closest("[data-transcript-row]"); + expect(row).toBeInstanceOf(HTMLElement); + return Math.round((row as HTMLElement).getBoundingClientRect().height); + }); + expect(new Set(rowHeights)).toEqual(new Set([24])); }, }; diff --git a/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx b/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx index d086f81eb8..6996912738 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx @@ -1869,32 +1869,69 @@ export const SourcesOnlyAssistantSpacing: Story = { }, }; -export const NoRenderableContentFallbackSpacing: Story = { +/** + * Regression: assistant messages whose only tool row resolves to null + * must not leave behind an empty transcript wrapper or an extra gap. + */ +export const HiddenAssistantToolMessageDoesNotRenderGap: Story = { args: { ...defaultArgs, parsedMessages: buildMessages([ { ...baseMessage, - id: 101, - role: "assistant", - content: [], + id: 201, + role: "user", + content: [{ type: "text", text: "Run the command" }], }, { ...baseMessage, - id: 102, + id: 202, + role: "assistant", + content: [{ type: "text", text: "Done." }], + }, + { + ...baseMessage, + id: 203, + role: "assistant", + content: [ + { + type: "tool-call", + tool_call_id: "hidden-execute", + tool_name: "execute", + args: {}, + }, + ], + }, + { + ...baseMessage, + id: 204, role: "user", - content: [{ type: "text", text: "Thanks for trying!" }], + content: [{ type: "text", text: "Thanks!" }], }, ]), }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); expect( - canvas.getByText("Message has no renderable content."), - ).toBeInTheDocument(); - expect( - document.querySelector('[data-testid="assistant-bottom-spacer"]'), - ).toBeInTheDocument(); + canvas.queryByText("Message has no renderable content."), + ).not.toBeInTheDocument(); + + for (const el of canvasElement.querySelectorAll( + '[data-testid="message-actions"]', + )) { + if (el instanceof HTMLElement) { + el.style.opacity = "1"; + } + } + + const timeline = canvas.getByTestId("conversation-timeline"); + const renderedRows = Array.from( + timeline.querySelectorAll('[data-role="user"], [data-role="assistant"]'), + ); + expect(renderedRows).toHaveLength(3); + expect(renderedRows[1]).toHaveAttribute("data-role", "assistant"); + expect(renderedRows[1]).toHaveTextContent("Done."); + expect(canvas.getAllByTestId("message-actions")).toHaveLength(3); }, }; @@ -2211,12 +2248,144 @@ export const ThinkingBlockWithToolCall: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( - canvas.getByRole("button", { name: /thinking/i }), - ).toBeInTheDocument(); + const thinkingButton = canvas.getByRole("button", { name: /thinking/i }); + expect(thinkingButton).toBeInTheDocument(); expect( canvas.getByRole("button", { name: /read package\.json/i }), ).toBeInTheDocument(); + + const toolButton = canvas.getByRole("button", { + name: /read package\.json/i, + }); + const thinkingContainer = thinkingButton.closest("[data-transcript-row]"); + const toolContainer = toolButton.closest("[data-transcript-row]"); + expect(thinkingContainer).toBeInstanceOf(HTMLElement); + expect(toolContainer).toBeInstanceOf(HTMLElement); + expect(toolContainer?.firstElementChild).not.toHaveAttribute("data-state"); + expect(thinkingContainer?.firstElementChild).not.toHaveAttribute( + "data-state", + ); + expect( + canvas.queryByTestId("assistant-bottom-spacer"), + ).not.toBeInTheDocument(); + }, +}; + +/** Shell-style tool rows should keep the same collapsed height as Thinking. */ +export const ThinkingBlockWithShellTools: Story = { + parameters: { + queries: [ + { + key: ["me", "preferences"], + data: { + task_notification_alert_dismissed: false, + thinking_display_mode: "always_collapsed" as const, + shell_tool_display_mode: "always_collapsed" as const, + code_diff_display_mode: "auto" as const, + agent_chat_send_shortcut: "enter" as const, + }, + }, + ], + }, + args: { + ...defaultArgs, + parsedMessages: buildMessages([ + { + ...baseMessage, + id: 1, + role: "assistant", + content: [ + { + type: "reasoning", + text: "I should inspect the current chat spacing before patching it.", + }, + { + type: "tool-call", + tool_call_id: "tool-1", + tool_name: "execute", + args: { command: "pnpm test" }, + }, + { + type: "tool-call", + tool_call_id: "tool-2", + tool_name: "process_output", + args: { process_id: "process-1" }, + }, + ], + }, + { + ...baseMessage, + id: 2, + role: "tool", + content: [ + { + type: "tool-result", + tool_call_id: "tool-1", + tool_name: "execute", + result: { output: "", wall_duration_ms: "667" }, + }, + ], + }, + { + ...baseMessage, + id: 3, + role: "tool", + content: [ + { + type: "tool-result", + tool_call_id: "tool-2", + tool_name: "process_output", + result: { output: "Spacing looks stable." }, + }, + ], + }, + ]), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const thinkingButton = canvas.getByRole("button", { name: /thinking/i }); + const executeButton = canvas.getByRole("button", { + name: /expand command/i, + }); + const processOutputButton = canvas.getByRole("button", { + name: /expand process output/i, + }); + + const thinkingRow = thinkingButton.closest( + "[data-transcript-row]", + )?.firstElementChild; + const executeRow = executeButton.closest( + "[data-transcript-row]", + )?.firstElementChild; + const processOutputRow = processOutputButton.closest( + "[data-transcript-row]", + )?.firstElementChild; + + expect(thinkingRow).toBeInstanceOf(HTMLElement); + expect(executeRow).toBeInstanceOf(HTMLElement); + expect(processOutputRow).toBeInstanceOf(HTMLElement); + + const rowHeights = [thinkingRow, executeRow, processOutputRow].map((row) => + Math.round((row as HTMLElement).getBoundingClientRect().height), + ); + expect(new Set(rowHeights)).toHaveLength(1); + + const wrappers = [ + thinkingButton.closest("[data-transcript-row]"), + executeButton.closest("[data-transcript-row]"), + processOutputButton.closest("[data-transcript-row]"), + ].map((row) => row as HTMLElement); + const gaps = [ + Math.round( + wrappers[1].getBoundingClientRect().top - + wrappers[0].getBoundingClientRect().bottom, + ), + Math.round( + wrappers[2].getBoundingClientRect().top - + wrappers[1].getBoundingClientRect().bottom, + ), + ]; + expect(gaps).toEqual([8, 8]); }, }; diff --git a/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx b/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx index d78fb89bf8..1c19543a1a 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx +++ b/site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx @@ -1,9 +1,4 @@ -import { - ChevronDownIcon, - ChevronLeftIcon, - ChevronRightIcon, - PencilIcon, -} from "lucide-react"; +import { ChevronLeftIcon, ChevronRightIcon, PencilIcon } from "lucide-react"; import { type FC, Fragment, @@ -20,11 +15,6 @@ import type * as TypesGen from "#/api/typesGenerated"; import type { ThinkingDisplayMode } from "#/api/typesGenerated"; import { Button } from "#/components/Button/Button"; -import { - Collapsible, - CollapsibleContent, - CollapsibleTrigger, -} from "#/components/Collapsible/Collapsible"; import { CopyButton } from "#/components/CopyButton/CopyButton"; import { Tooltip, @@ -43,6 +33,7 @@ import { } from "../ChatElements"; import { WebSearchSources } from "../ChatElements/tools"; import type { SubagentVariant } from "../ChatElements/tools/subagentDescriptor"; +import { ToolCollapsible } from "../ChatElements/tools/ToolCollapsible"; import { ImageLightbox } from "../ImageLightbox"; import { TextPreviewDialog } from "../TextPreviewDialog"; import { @@ -152,61 +143,39 @@ const ReasoningDisclosure = memo<{ }, [displayTextLength, isPreviewConstrained]); return ( -
- setManualToggle(open)} +
+ - - {isStreaming ? ( + expanded={expanded} + onExpandedChange={(open) => setManualToggle(open)} + header={ + isStreaming ? ( Thinking ) : ( Thinking - )} - - + ) + } + > {hasText && ( - -
+ - - {displayText} - -
- + {displayText} + +
)} - +
); }, @@ -552,10 +521,6 @@ const ChatMessageItem = memo<{ return null; } - const hasRenderableContent = - parsed.blocks.length > 0 || - parsed.tools.length > 0 || - parsed.sources.length > 0; const conversationItemProps: { role: "user" | "assistant" } = { role: isUser ? "user" : "assistant", }; @@ -580,8 +545,8 @@ const ChatMessageItem = memo<{ ) : ( - {/* Keep consecutive shell tools tighter because execute/process_output pairs read as one terminal interaction. */} -
+ {/* Keep assistant content spacing consistent by letting the parent stack own every top-level gap. */} +
- {!hasRenderableContent && ( -
- Message has no renderable content. -
- )}
diff --git a/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.stories.tsx b/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.stories.tsx index ee6984291b..44922f69c2 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.stories.tsx @@ -279,13 +279,34 @@ export const ThinkingDuringStreamingWithToolCalls: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); // Tool-only stream chunks can otherwise clear the activity indicator before text arrives. - const matches = canvas.getAllByText("Thinking"); - expect(matches.length).toBeGreaterThanOrEqual(1); + expect(canvas.getAllByText("Thinking").length).toBeGreaterThanOrEqual(1); - const thinkingBlock = matches[0].parentElement; - expect(thinkingBlock).toBeInstanceOf(HTMLElement); - expect(getComputedStyle(thinkingBlock as HTMLElement).marginTop).toBe( - "8px", + const toolCallWrappers = Array.from( + canvasElement.querySelectorAll("[data-transcript-row]"), ); + expect(toolCallWrappers).toHaveLength(3); + + const thinkingWrapper = toolCallWrappers.at(-1); + const previousToolWrapper = toolCallWrappers.at(-2); + expect(thinkingWrapper).toBeInstanceOf(HTMLElement); + expect(previousToolWrapper).toBeInstanceOf(HTMLElement); + expect(thinkingWrapper).toHaveTextContent("Thinking"); + + const gap = Math.round( + (thinkingWrapper as HTMLElement).getBoundingClientRect().top - + (previousToolWrapper as HTMLElement).getBoundingClientRect().bottom, + ); + expect(gap).toBe(8); + + // The placeholder inner row must match the committed collapsed + // Thinking row height so the transition from streaming to settled + // does not jump. ToolCollapsible enforces min-h-6 (24px). + const placeholderRow = (thinkingWrapper as HTMLElement).firstElementChild; + expect(placeholderRow).toBeInstanceOf(HTMLElement); + expect( + Math.round( + (placeholderRow as HTMLElement).getBoundingClientRect().height, + ), + ).toBe(24); }, }; diff --git a/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.tsx b/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.tsx index 822e7cfab7..d3ce729db2 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.tsx +++ b/site/src/pages/AgentsPage/components/ChatConversation/StreamingOutput.tsx @@ -7,6 +7,7 @@ import { MessageContent, Shimmer, } from "../ChatElements"; +import { TranscriptRow } from "../ChatElements/TranscriptRow"; import type { SubagentVariant } from "../ChatElements/tools/subagentDescriptor"; import { ChatStatusCallout } from "./ChatStatusCallout"; import { BlockList } from "./ConversationTimeline"; @@ -33,10 +34,12 @@ const hasTextOrReasoningBlock = (blocks: readonly RenderBlock[]): boolean => * as the ChatStatusCallout status placeholder. */ const StreamingThinkingPlaceholder: FC = () => ( -
- - Thinking - +
+ + + Thinking + +
); @@ -92,7 +95,7 @@ export const StreamingOutput: FC<{ -
+
{shouldShowBlocks && ( + parseMessagesWithMergedTools([message])[0].parsed; + const getDisplayState = ( message: ChatMessage, overrides: Partial[0]> = {}, ) => deriveMessageDisplayState({ message, - parsed: parseMessageContent(message.content), + parsed: getParsedMessage(message), hideActions: false, hasActiveStream: false, ...overrides, @@ -63,7 +67,7 @@ describe("deriveMessageDisplayState", () => { expect(getDisplayState(message).hasCopyableContent).toBe(false); }); - it("shows the assistant spacer for reasoning messages when no suppressing flags apply", () => { + it("shows the assistant spacer for thinking-only messages when no suppressing flags apply", () => { const message = buildMessage( [{ type: "reasoning", text: "I should think before answering." }], "assistant", @@ -72,6 +76,40 @@ describe("deriveMessageDisplayState", () => { expect(getDisplayState(message).needsAssistantBottomSpacer).toBe(true); }); + it("hides the assistant spacer when thinking is followed by a tool call", () => { + const message = buildMessage( + [ + { type: "reasoning", text: "I should think before acting." }, + { + type: "tool-call", + tool_call_id: "tool-1", + tool_name: "execute", + args: { command: "pnpm storybook --no-open" }, + }, + ], + "assistant", + ); + + expect(getDisplayState(message).needsAssistantBottomSpacer).toBe(false); + }); + + it("shows the assistant spacer when thinking is followed by a hidden execute tool", () => { + const message = buildMessage( + [ + { type: "reasoning", text: "I should think before acting." }, + { + type: "tool-call", + tool_call_id: "tool-1", + tool_name: "execute", + args: {}, + }, + ], + "assistant", + ); + + expect(getDisplayState(message).needsAssistantBottomSpacer).toBe(true); + }); + it("suppresses the assistant spacer while awaiting the first stream chunk", () => { const message = buildMessage( [{ type: "reasoning", text: "I should think before answering." }], @@ -113,4 +151,68 @@ describe("deriveMessageDisplayState", () => { expect(getDisplayState(message).needsAssistantBottomSpacer).toBe(false); }); + + it("hides assistant messages with no renderable content", () => { + const message = buildMessage([], "assistant"); + + expect(getDisplayState(message).shouldHide).toBe(true); + }); + + it("hides assistant messages whose execute tool renders nothing", () => { + const message = buildMessage( + [ + { + type: "tool-call", + tool_call_id: "tool-1", + tool_name: "execute", + args: {}, + }, + ], + "assistant", + ); + + expect(getDisplayState(message).shouldHide).toBe(true); + }); + + it("keeps assistant messages visible when execute shows a real command", () => { + const message = buildMessage( + [ + { + type: "tool-call", + tool_call_id: "tool-1", + tool_name: "execute", + args: { command: "pnpm test" }, + }, + ], + "assistant", + ); + + expect(getDisplayState(message).shouldHide).toBe(false); + }); + + it("hides running wait_agent messages until the chat id is available", () => { + const message = buildMessage([], "assistant"); + const parsed: ParsedMessageContent = { + ...getParsedMessage(message), + blocks: [{ type: "tool", id: "wait-1" }], + tools: [ + { + id: "wait-1", + name: "wait_agent", + args: {}, + isError: false, + status: "running", + }, + ], + }; + + expect( + deriveMessageDisplayState({ + message, + parsed, + hideActions: false, + hasActiveStream: false, + }).shouldHide, + ).toBe(true); + }); }); diff --git a/site/src/pages/AgentsPage/components/ChatConversation/messageHelpers.ts b/site/src/pages/AgentsPage/components/ChatConversation/messageHelpers.ts index e85c1c8c25..c86624416e 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/messageHelpers.ts +++ b/site/src/pages/AgentsPage/components/ChatConversation/messageHelpers.ts @@ -1,4 +1,5 @@ import type * as TypesGen from "#/api/typesGenerated"; +import { shouldRenderTool } from "../ChatElements/tools/toolVisibility"; import type { ParsedMessageContent, RenderBlock } from "./types"; export type UserInlineRenderBlock = @@ -37,6 +38,33 @@ const isMetadataOnlyMessage = ( parts.length > 0 && parts.every((part) => part.type === "context-file" || part.type === "skill"); +const getRenderableContentState = (parsed: ParsedMessageContent) => { + const visibleTools = parsed.tools.filter((tool) => + shouldRenderTool({ + name: tool.name, + status: tool.status, + args: tool.args, + result: tool.result, + }), + ); + const visibleToolIds = new Set(visibleTools.map((tool) => tool.id)); + const visibleBlocks = parsed.blocks.filter( + (block) => block.type !== "tool" || visibleToolIds.has(block.id), + ); + const hasRenderableContent = + visibleBlocks.length > 0 || + visibleTools.length > 0 || + parsed.sources.length > 0; + const hasThinkingOnlyContent = + visibleBlocks.length > 0 && + visibleBlocks.every((block) => block.type === "thinking"); + + return { + hasRenderableContent, + hasThinkingOnlyContent, + }; +}; + export const deriveMessageDisplayState = ({ message, parsed, @@ -61,19 +89,15 @@ export const deriveMessageDisplayState = ({ const hasFileBlocks = userFileBlocks.length > 0; const hasCopyableContent = Boolean(parsed.markdown.trim()) && !hasFileAttachments; - const hasRenderableContent = - parsed.blocks.length > 0 || - parsed.tools.length > 0 || - parsed.sources.length > 0; + const { hasRenderableContent, hasThinkingOnlyContent } = + getRenderableContentState(parsed); const needsAssistantBottomSpacer = !hideActions && !hasActiveStream && !isAwaitingFirstStreamChunk && !isUser && !hasCopyableContent && - (Boolean(parsed.reasoning) || - parsed.sources.length > 0 || - !hasRenderableContent); + (hasThinkingOnlyContent || parsed.sources.length > 0); const hasToolResultsOnly = parsed.toolResults.length > 0 && parsed.toolCalls.length === 0 && @@ -85,7 +109,8 @@ export const deriveMessageDisplayState = ({ shouldHide: hasToolResultsOnly || isProviderToolResultOnlyMessage(parts) || - isMetadataOnlyMessage(parts), + isMetadataOnlyMessage(parts) || + (!isUser && !hasRenderableContent), userInlineContent, userFileBlocks, hasUserMessageBody, diff --git a/site/src/pages/AgentsPage/components/ChatElements/TranscriptRow.tsx b/site/src/pages/AgentsPage/components/ChatElements/TranscriptRow.tsx new file mode 100644 index 0000000000..c4dacabe18 --- /dev/null +++ b/site/src/pages/AgentsPage/components/ChatElements/TranscriptRow.tsx @@ -0,0 +1,23 @@ +import { Slot } from "radix-ui"; +import type { ComponentPropsWithRef, FC } from "react"; +import { cn } from "#/utils/cn"; + +type TranscriptRowProps = ComponentPropsWithRef<"div"> & { + asChild?: boolean; +}; + +/** + * Some transcript rows bypass ToolCollapsible, so they need one shared place + * to keep the collapsed row height aligned across the chat timeline. + */ +export const TranscriptRow: FC = ({ + asChild = false, + className, + ...props +}) => { + const Comp = asChild ? Slot.Root : "div"; + + return ( + + ); +}; diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/AskUserQuestionTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/AskUserQuestionTool.tsx index 4743532040..59562edd25 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/AskUserQuestionTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/AskUserQuestionTool.tsx @@ -9,6 +9,7 @@ import { Button } from "#/components/Button/Button"; import { Input } from "#/components/Input/Input"; import { RadioGroup, RadioGroupItem } from "#/components/RadioGroup/RadioGroup"; import { cn } from "#/utils/cn"; +import { TranscriptRow } from "../TranscriptRow"; import type { ToolStatus } from "./utils"; export type AskUserQuestion = { @@ -536,16 +537,16 @@ export const AskUserQuestionTool: FC = ({ if (isError) { return (
-
{errorMessage || "Failed to ask questions"} -
+
); } @@ -554,11 +555,7 @@ export const AskUserQuestionTool: FC = ({ return (
{isRunning ? ( -
+ Asking for clarification... @@ -566,7 +563,7 @@ export const AskUserQuestionTool: FC = ({ data-testid="ask-user-question-loading-icon" className="h-3.5 w-3.5 shrink-0 animate-spin text-content-secondary motion-reduce:animate-none" /> -
+ ) : (

No questions available. @@ -680,11 +677,7 @@ export const AskUserQuestionTool: FC = ({ return (

{isRunning && ( -
+ Asking for clarification... @@ -692,7 +685,7 @@ export const AskUserQuestionTool: FC = ({ data-testid="ask-user-question-loading-icon" className="h-3.5 w-3.5 shrink-0 animate-spin text-content-secondary motion-reduce:animate-none" /> -
+ )} {isInteractive ? ( diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx index 22d5ced009..1c2524d478 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx @@ -20,6 +20,7 @@ import { TooltipTrigger, } from "#/components/Tooltip/Tooltip"; import { cn } from "#/utils/cn"; +import { TranscriptRow } from "../TranscriptRow"; import { type AgentDisplayState, isAgentDisplayOpen, @@ -93,21 +94,25 @@ const ExecuteToolInner: React.FC = ({ return (
- -
+ + + {isRunning && ( )} @@ -157,7 +162,7 @@ const ExecuteToolInner: React.FC = ({ label="Copy command" className="-my-0.5 size-6 p-0 opacity-0 transition-opacity hover:bg-surface-tertiary group-hover/exec:opacity-100" /> -
+ {outputOpen && ( = ({ exit {exitCode} )} - {hasOutput && } + {hasOutput && ( + + )} ) : undefined } diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.tsx index 523670a6a4..d3c2e41fc4 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.tsx @@ -10,6 +10,7 @@ import { TooltipTrigger, } from "#/components/Tooltip/Tooltip"; import { Response } from "../Response"; +import { TranscriptRow } from "../TranscriptRow"; import type { ToolStatus } from "./utils"; export const ProposePlanTool: React.FC<{ @@ -72,7 +73,7 @@ export const ProposePlanTool: React.FC<{ return (
-
+ {isRunning ? `Proposing ${filename}…` : `Proposed ${filename}`} @@ -92,7 +93,7 @@ export const ProposePlanTool: React.FC<{ {isRunning && ( )} -
+ {hasDisplayContent ? ( <> {displayContent} @@ -137,10 +138,10 @@ export const ProposePlanTool: React.FC<{ ) )} {fetchLoading && ( -
+ Loading plan… -
+ )}
); diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ReadTemplateTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ReadTemplateTool.tsx index 3a158430a4..6bcf356ddf 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ReadTemplateTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ReadTemplateTool.tsx @@ -5,6 +5,7 @@ import { TooltipContent, TooltipTrigger, } from "#/components/Tooltip/Tooltip"; +import { TranscriptRow } from "../TranscriptRow"; import type { ToolStatus } from "./utils"; /** @@ -26,7 +27,7 @@ export const ReadTemplateTool: React.FC<{ : "Read template"; return ( -
+ {label} {isError && ( @@ -41,6 +42,6 @@ export const ReadTemplateTool: React.FC<{ {isRunning && ( )} -
+ ); }; diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx index ac818bcadb..2e7386b2bb 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx @@ -15,6 +15,7 @@ import { cn } from "#/utils/cn"; import { safeBuildAgentChatPath } from "../../../utils/navigation"; import { Response } from "../Response"; import { Shimmer } from "../Shimmer"; +import { TranscriptRow } from "../TranscriptRow"; import { useDesktopPanel } from "./DesktopPanelContext"; import { InlineDesktopPreview } from "./InlineDesktopPreview"; import { RecordingPreview } from "./RecordingPreview"; @@ -192,58 +193,60 @@ export const SubagentTool: React.FC<{ return (
- + {hasExpandableContent && ( + + )} + {durationLabel && ( + + {`Worked for ${durationLabel}`} + + )} + + {showDesktopPreview && desktopChatId && toolStatus !== "completed" && (
diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx index dc376696ac..18144e7d84 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx @@ -848,6 +848,7 @@ export const CloseAgentRunningWithoutChatId: Story = { await waitFor(() => { expect(canvasElement.textContent?.trim()).toBe(""); }); + expect(canvasElement.querySelector("[data-transcript-row]")).toBeNull(); expect(canvas.queryByRole("button")).toBeNull(); expect(canvas.queryByRole("link", { name: "View agent" })).toBeNull(); }, diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx index 017995a820..ca3864f9c5 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx @@ -42,6 +42,7 @@ import { import { ToolCollapsible } from "./ToolCollapsible"; import { ToolIcon } from "./ToolIcon"; import { ToolLabel } from "./ToolLabel"; +import { getExecuteRenderData, shouldRenderTool } from "./toolVisibility"; import { asNumber, asRecord, @@ -215,53 +216,6 @@ const parseAskUserQuestionResult = ( return null; }; -type ExecuteRenderData = { - command: string; - output: string; - durationMs?: number; - isBackgrounded: boolean; - authenticateURL: string; - providerLabel: string; -}; - -const getExecuteRenderData = ( - args: unknown, - result: unknown, -): ExecuteRenderData => { - const parsedArgs = parseArgs(args); - const command = parsedArgs ? asString(parsedArgs.command) : ""; - const rec = asRecord(result); - const output = rec ? asString(rec.output).trim() : ""; - const durationMs = rec - ? (asNumber(rec.wall_duration_ms, { parseString: true }) ?? - asNumber(rec.duration_ms, { parseString: true })) - : undefined; - const isBackgrounded = Boolean( - rec && asString(rec.background_process_id).trim(), - ); - const authenticateURL = rec?.auth_required - ? asString(rec.authenticate_url).trim() - : ""; - const providerLabel = toProviderLabel( - rec ? asString(rec.provider_display_name).trim() : "", - rec ? asString(rec.provider_id).trim() : "", - rec ? asString(rec.provider_type).trim() : "", - ); - - return { - command, - output, - durationMs, - isBackgrounded, - authenticateURL, - providerLabel, - }; -}; - -const shouldHideExecuteTool = (data: ExecuteRenderData): boolean => { - return data.command.trim().length === 0 && !data.authenticateURL; -}; - const ExecuteRenderer: FC = ({ status, args, @@ -273,10 +227,6 @@ const ExecuteRenderer: FC = ({ }) => { const data = getExecuteRenderData(args, result); - if (shouldHideExecuteTool(data)) { - return null; - } - if (data.authenticateURL) { return ( = ({ } } - // Postpone rendering wait_agent, message_agent, and close_agent - // until the chat_id has been parsed from the streaming args. - // Without it we cannot determine variant or title, which causes - // a brief flash of the generic lifecycle copy. - if (!chatId && status === "running") { - if ( - descriptor.action === "wait" || - descriptor.action === "message" || - descriptor.action === "close" - ) { - return null; - } - } - return ( void; ariaLabel?: ToolCollapsibleAriaLabel; className?: string; headerClassName?: string; @@ -49,47 +52,60 @@ export const ToolCollapsible: FC = ({ headerActions, hasContent = true, defaultExpanded = false, + expanded: expandedProp, + onExpandedChange, ariaLabel, className, headerClassName, }) => { - const [expanded, setExpanded] = useState(defaultExpanded); + const [uncontrolledExpanded, setUncontrolledExpanded] = + useState(defaultExpanded); + const expanded = expandedProp ?? uncontrolledExpanded; const renderedHeader = typeof header === "function" ? header(expanded) : header; + const toggleExpanded = () => { + const nextExpanded = !expanded; + if (expandedProp === undefined) { + setUncontrolledExpanded(nextExpanded); + } + onExpandedChange?.(nextExpanded); + }; const headerButton = hasContent ? ( - + + ) : ( -
{renderedHeader} -
+ ); return ( diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.test.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.test.ts new file mode 100644 index 0000000000..03db7e4c80 --- /dev/null +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it } from "vitest"; +import { getExecuteRenderData, shouldRenderTool } from "./toolVisibility"; + +describe("toolVisibility", () => { + describe("getExecuteRenderData", () => { + it("parses execute output and auth metadata from result payloads", () => { + expect( + getExecuteRenderData( + { command: "git fetch origin" }, + { + output: " fetched ", + wall_duration_ms: "47200", + background_process_id: "process-1", + auth_required: true, + authenticate_url: "https://example.com/auth", + provider_display_name: "GitHub", + }, + ), + ).toEqual({ + command: "git fetch origin", + output: "fetched", + durationMs: 47200, + isBackgrounded: true, + authenticateURL: "https://example.com/auth", + providerLabel: "GitHub", + }); + }); + }); + + describe("shouldRenderTool", () => { + it("hides execute rows with neither a command nor an auth prompt", () => { + expect( + shouldRenderTool({ + name: "execute", + status: "completed", + args: {}, + result: { output: "ignored" }, + }), + ).toBe(false); + }); + + it("keeps execute rows when auth is required even without a command", () => { + expect( + shouldRenderTool({ + name: "execute", + status: "completed", + args: {}, + result: { + auth_required: true, + authenticate_url: "https://example.com/auth", + }, + }), + ).toBe(true); + }); + + it("hides running wait_agent rows until chat_id is available", () => { + expect( + shouldRenderTool({ + name: "wait_agent", + status: "running", + args: {}, + result: { status: "pending" }, + }), + ).toBe(false); + }); + + it("hides running message_agent rows until chat_id is available", () => { + expect( + shouldRenderTool({ + name: "message_agent", + status: "running", + args: { message: "continue" }, + result: { status: "pending" }, + }), + ).toBe(false); + }); + + it("hides running close_agent rows until chat_id is available", () => { + expect( + shouldRenderTool({ + name: "close_agent", + status: "running", + args: {}, + result: { status: "running" }, + }), + ).toBe(false); + }); + + it("renders running lifecycle rows once args provide the chat_id", () => { + expect( + shouldRenderTool({ + name: "wait_agent", + status: "running", + args: { chat_id: "child-chat-1" }, + result: { status: "pending" }, + }), + ).toBe(true); + }); + + it("renders completed lifecycle rows even if chat_id is absent", () => { + expect( + shouldRenderTool({ + name: "close_agent", + status: "completed", + args: {}, + result: { status: "completed" }, + }), + ).toBe(true); + }); + + it("keeps unrelated tools visible", () => { + expect( + shouldRenderTool({ + name: "read_file", + status: "completed", + args: { path: "README.md" }, + result: { content: "docs" }, + }), + ).toBe(true); + }); + }); +}); diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts new file mode 100644 index 0000000000..daaca9e269 --- /dev/null +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts @@ -0,0 +1,112 @@ +import { getSubagentChatId, getSubagentDescriptor } from "./subagentDescriptor"; +import { + asNumber, + asRecord, + asString, + parseArgs, + type ToolStatus, + toProviderLabel, +} from "./utils"; + +type ExecuteRenderData = { + command: string; + output: string; + durationMs?: number; + isBackgrounded: boolean; + authenticateURL: string; + providerLabel: string; +}; + +/** + * Execute payloads can arrive partially populated, so visibility and rendering + * share one defensive, normalized interpretation of args and results here. + */ +export const getExecuteRenderData = ( + args: unknown, + result: unknown, +): ExecuteRenderData => { + const parsedArgs = parseArgs(args); + const command = parsedArgs ? asString(parsedArgs.command) : ""; + const rec = asRecord(result); + const output = rec ? asString(rec.output).trim() : ""; + const durationMs = rec + ? (asNumber(rec.wall_duration_ms, { parseString: true }) ?? + asNumber(rec.duration_ms, { parseString: true })) + : undefined; + const isBackgrounded = Boolean( + rec && asString(rec.background_process_id).trim(), + ); + const authenticateURL = rec?.auth_required + ? asString(rec.authenticate_url).trim() + : ""; + const providerLabel = toProviderLabel( + rec ? asString(rec.provider_display_name).trim() : "", + rec ? asString(rec.provider_id).trim() : "", + rec ? asString(rec.provider_type).trim() : "", + ); + + return { + command, + output, + durationMs, + isBackgrounded, + authenticateURL, + providerLabel, + }; +}; + +const shouldRenderExecuteTool = (data: ExecuteRenderData): boolean => { + return data.command.trim().length > 0 || Boolean(data.authenticateURL); +}; + +const shouldRenderSubagentLifecycleTool = ({ + name, + status, + args, + result, +}: { + name: string; + status: ToolStatus; + args?: unknown; + result?: unknown; +}): boolean => { + const descriptor = getSubagentDescriptor({ name, args, result }); + if (!descriptor || status !== "running") { + return true; + } + + if ( + descriptor.action !== "wait" && + descriptor.action !== "message" && + descriptor.action !== "close" + ) { + return true; + } + + // Wait, message, and close rows can stream before their target chat_id + // arrives. Hiding them until that id exists avoids flashing generic + // lifecycle copy before the transcript can resolve the real title. + return Boolean(getSubagentChatId({ args, result })); +}; + +/** + * Centralize tool-row visibility so transcript message hiding stays in sync + * with row rendering and hidden rows never leave empty gaps behind. + */ +export const shouldRenderTool = ({ + name, + status, + args, + result, +}: { + name: string; + status: ToolStatus; + args?: unknown; + result?: unknown; +}): boolean => { + if (name === "execute") { + return shouldRenderExecuteTool(getExecuteRenderData(args, result)); + } + + return shouldRenderSubagentLifecycleTool({ name, status, args, result }); +}; diff --git a/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx b/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx index 066f4de4fd..ab327d1c83 100644 --- a/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx @@ -71,6 +71,22 @@ const buildRegressionStore = () => { return store; }; +const buildThinkingSpacerStore = () => { + const store = createChatStore(); + + store.replaceMessages([ + buildMessage(1, "user", [{ type: "text", text: "Read the source files" }]), + buildMessage(2, "assistant", [ + { + type: "reasoning", + text: "I should think before answering.", + }, + ]), + ]); + + return store; +}; + export const StreamingToolCallGapRegression: Story = { render: () => { const store = buildRegressionStore(); @@ -121,7 +137,7 @@ export const StartingPhaseToolCallGapRegression: Story = { export const SpacerVisibleWhenNotStreaming: Story = { render: () => { - const store = buildRegressionStore(); + const store = buildThinkingSpacerStore(); return ( { const canvas = within(canvasElement); + canvas.getByRole("button", { name: /thinking/i }); expect(canvas.getByTestId("assistant-bottom-spacer")).toBeInTheDocument(); }, }; + +export const HiddenAssistantPlaceholderDoesNotRender: Story = { + render: () => { + const store = createChatStore(); + + store.replaceMessages([ + buildMessage(1, "user", [{ type: "text", text: "Run the command" }]), + buildMessage(2, "assistant", [{ type: "text", text: "Done." }]), + buildMessage(3, "assistant", []), + buildMessage(4, "user", [{ type: "text", text: "Thanks!" }]), + ]); + + return ( + + ); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.queryByText("Message has no renderable content.")).toBeNull(); + + const rows = canvasElement.querySelectorAll( + '[data-role="user"], [data-role="assistant"]', + ); + expect(rows).toHaveLength(3); + expect(rows[1]).toHaveAttribute("data-role", "assistant"); + expect(rows[1]).toHaveTextContent("Done."); + }, +};