Revert "Improve display of agent/tool cards in chat session"#1729
Revert "Improve display of agent/tool cards in chat session"#1729
Conversation
|
Important UI Tests need review – Review now🟡 UI Tests: 8 visual and accessibility changes must be accepted as baselines |
This reverts commit 4199089. Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
supreme-gg-gg
force-pushed
the
revert-1360-feat/tool-display-improvements
branch
from
d300d36 to
4d4a311
Compare
There was a problem hiding this comment.
Pull request overview
Reverts the prior UI change that “improved display of agent/tool cards in chat session”, restoring simpler preformatted rendering for tool/agent call inputs & outputs to address tool output formatting regressions.
Changes:
- Remove the
SmartContent/CollapsibleSectionformatted renderers (and associated Storybook stories). - Simplify
AgentCallDisplayinput/output rendering to plain<pre>blocks with basic expand/collapse UI. - Adjust
ToolDisplayto use plain<pre>blocks with scroll + add copy-to-clipboard on results; remove copy button fromChatMessage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/components/chat/SmartContent.tsx | Deleted formatted/recursive renderer used for tool output display. |
| ui/src/components/chat/SmartContent.stories.tsx | Deleted Storybook coverage for SmartContent. |
| ui/src/components/chat/CollapsibleSection.tsx | Deleted collapsible section helper previously used by tool/agent displays. |
| ui/src/components/chat/ChatMessage.tsx | Removes message-level copy button and related state/imports. |
| ui/src/components/chat/AgentCallDisplay.tsx | Reverts to simpler expand/collapse + <pre> rendering for inputs/outputs. |
| ui/src/components/ToolDisplay.tsx | Reverts away from SmartContent, uses <pre> + scroll for args/results and adds result copy button. |
| ui/src/components/ToolDisplay.stories.tsx | Removes long-id stories; keeps chat-layout stories and other long-content cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="flex items-center font-medium"> | ||
| <KagentLogo className="w-4 h-4 mr-2" /> | ||
| {agentDisplay} | ||
| </div> | ||
| <div className="font-light"> | ||
| {call.id} | ||
| </div> | ||
| <div className="font-light">{call.id}</div> |
There was a problem hiding this comment.
call.id is displayed without truncation in the card header. Long IDs can overflow the available width and break layout; consider adding min-w-0 + truncate (or similar) on the id element and its flex container so the header remains stable.
| import { useState } from "react"; | ||
| import { FunctionCall, TokenStats } from "@/types"; | ||
| import { FunctionSquare, CheckCircle, Clock, Code, Loader2, Text, AlertCircle, ShieldAlert } from "lucide-react"; | ||
| import { ScrollArea } from "@radix-ui/react-scroll-area"; |
There was a problem hiding this comment.
ScrollArea is being imported directly from @radix-ui/react-scroll-area, but the codebase already wraps Radix in @/components/ui/scroll-area (adds Viewport, ScrollBar, styling, and consistent API). Importing { ScrollArea } from Radix is likely incorrect (Radix typically exports Root/Viewport/... rather than a ScrollArea component) and can break scrolling/rendering here. Switch this import to the local UI wrapper and keep usage the same.
| import { ScrollArea } from "@radix-ui/react-scroll-area"; | |
| import { ScrollArea } from "@/components/ui/scroll-area"; |
| </div> | ||
| )} | ||
| <div className="font-light truncate min-w-0">{call.id}</div> | ||
| <div className="font-light">{call.id}</div> | ||
| </CardTitle> |
There was a problem hiding this comment.
call.id is rendered without truncation/min-width constraints in the header flex row. Long tool IDs can overflow the card and break the chat layout. Consider restoring min-w-0/truncate on the id element (and/or min-w-0 on the flex container) so long IDs don’t force horizontal overflow.
| {areInputsExpanded ? <ChevronUp className="w-4 h-4 ml-1" /> : <ChevronDown className="w-4 h-4 ml-1" />} | ||
| </button> | ||
| {areInputsExpanded && ( | ||
| <div className="mt-2 bg-muted/50 p-3 rounded"> |
There was a problem hiding this comment.
The expanded Input section renders the full JSON.stringify(call.args) in an unconstrained <pre> with no max height or scroll container. For large tool args this can massively expand the message list and degrade usability/performance. Consider wrapping the <pre> in the existing ScrollArea UI component (or add max-h-* + overflow-y-auto) similar to ToolDisplay’s behavior.
| <div className="mt-2 bg-muted/50 p-3 rounded"> | |
| <div className="mt-2 bg-muted/50 p-3 rounded max-h-96 overflow-y-auto"> |
| <pre className={`text-sm whitespace-pre-wrap break-words ${isError ? 'text-red-600 dark:text-red-400' : ''}`}> | ||
| {result?.content} | ||
| </pre> |
There was a problem hiding this comment.
The expanded Output section renders result.content in an unconstrained <pre> (no max height/scroll container). Tool outputs can be very large (logs, file contents), which can blow up the chat layout. Consider adding a bounded scroll area (e.g., reuse the shared ScrollArea wrapper or add max-h-* + overflow-y-auto) while keeping the error styling.
| <pre className={`text-sm whitespace-pre-wrap break-words ${isError ? 'text-red-600 dark:text-red-400' : ''}`}> | |
| {result?.content} | |
| </pre> | |
| <div className="max-h-96 overflow-y-auto"> | |
| <pre className={`text-sm whitespace-pre-wrap break-words ${isError ? 'text-red-600 dark:text-red-400' : ''}`}> | |
| {result?.content} | |
| </pre> | |
| </div> |
Reverts #1360
This seems to have messed up tool output formatting.
prev:
new:
