Rich tool call rendering: readable arguments and responses, post previews, path-identical metadata - #881
Open
crspeller wants to merge 29 commits into
Open
Rich tool call rendering: readable arguments and responses, post previews, path-identical metadata#881crspeller wants to merge 29 commits into
crspeller wants to merge 29 commits into
Conversation
…Call.Schema Capture MCP title/annotations (sanitized) in mcp GetTools, resolve effective display title (title > annotations.title). Add Title/Description to ToolCall, ContentBlock, and both block writers; keep them visible to non-requesters on both the live (redactToolCalls) and persisted (FilterForNonRequester) paths so a call renders identically live and after reload. Remove the unused ToolCall.Schema from the wire (providers build tool defs from Tool.Schema). Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…l_identity Map server_origin, mcp_bare_name, title, description on persisted rounds (toolUseBlockToToolCall). Add tool_identity helpers (originKind, canonicalToolKey, toolDisplayName) and use toolDisplayName in ToolCard, preferring the MCP-supplied title and ellipsizing long titles; built-in and embedded display names are unchanged. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…dentity Add streaming/tool_call_parity_test.go: a per-field policy table over llm.ToolCall enforced by reflection (exhaustiveness), plus writer-completeness and live-vs-persisted redaction-parity tests (verified to catch the historical Description drop-by-omission). Add mcp GetTools title/annotation mapping + Unicode-sanitization test; conversation SanitizeForDisplay + FilterForNonRequester title/description coverage; ContentBlock title/description round-trip; TS tool_identity.test.ts and turn_content_utils new-field mappings. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTool-call metadata now includes resolved titles, descriptions, annotations, and bare names across MCP resolution, persistence, redaction parity enforcement, and frontend rendering. The webapp adds structured argument views and rich cards for supported embedded tools. ChangesTool identity metadata and rich rendering
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServer
participant UserClients
participant llmTools
participant Conversation
participant Streaming
participant Webapp
MCPServer->>UserClients: tool title, description, annotations
UserClients->>llmTools: sanitized, precedence-resolved metadata
llmTools->>Conversation: persist title, description
llmTools->>Streaming: redacted title, description (payload cleared)
Conversation->>Webapp: rehydrate filtered title, description
Streaming->>Webapp: stream redacted tool call (title, description intact)
Webapp->>Webapp: identity mapping: origin, bare name, canonical key
Webapp->>Webapp: route to rich card or generic fallback
Webapp->>Webapp: render structured arguments + approval chrome
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
conversation/content_block.go (1)
151-184: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize
MCPBareNamebefore display
MCPBareNamecomes from the server-supplied tool name, and the webapp prettifies it whenTitleis empty. It should go through the same bidi/spoofing sanitization here asInput,Title, andDescription.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conversation/content_block.go` around lines 151 - 184, Update SanitizeForDisplay to sanitize the MCPBareName field for BlockTypeToolUse blocks before returning the copied slice, using the same llm.SanitizeNonPrintableChars helper as Input, Title, and Description. Preserve the existing nil handling and non-mutating behavior.conversation/convert.go (1)
83-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve persisted tool titles during rehydration.
enrichToolCallFromStorecurrently overwritestoolCall.TitlewheneverOverwriteDescriptionis set, andconversation/convert.goalways takes that path for non-redacted tool uses. That can replace a title captured at call time with a newer catalog value if the MCP server changes later. LeaveTitleuntouched when already set, or add a separate title-overwrite flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conversation/convert.go` around lines 83 - 97, Update enrichToolCallFromStore and its invocation in the toolCall rehydration path so an existing toolCall.Title is preserved, even when OverwriteDescription is enabled. Only populate the title from the current tool catalog when the persisted title is empty; keep description enrichment and redacted-tool behavior unchanged.
🧹 Nitpick comments (3)
llm/tools.go (1)
67-80: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument the MCP spec's pessimistic default for
destructiveHint.
DestructiveHint *boolis documented as "nil pointer means the server did not declare the hint," which is correct for capture, but the MCP spec's stated default whendestructiveHintis omitted istrue(destructive), notfalse. SinceAnnotationsisn't consumed yet, this isn't a live bug, but a future consumer naively doinghint.DestructiveHint != nil && *hint.DestructiveHintwould silently treat "undeclared" as "safe," inverting the spec's pessimistic default. Worth a one-line comment now so this doesn't get missed when annotations are actually surfaced.📝 Suggested doc addition
// DestructiveHint indicates the tool may perform destructive updates. - // A nil pointer means the server did not declare the hint. + // A nil pointer means the server did not declare the hint. Per the MCP + // spec, an undeclared destructiveHint defaults to true (assume + // destructive) — future consumers must not treat nil as "safe." DestructiveHint *bool🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/tools.go` around lines 67 - 80, Update the DestructiveHint field comment in ToolAnnotations to state that nil means the server omitted the hint and the MCP specification treats omitted destructiveHint as true (destructive), while preserving the existing capture semantics.mcp/user_clients_test.go (1)
137-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider table-driven form for this multi-case test.
This test exercises three distinct tool configurations (top-level title + annotations, annotations-only fallback, and no title/annotations at all) inside one monolithic function rather than as table-driven subtests.
As per coding guidelines,
**/*_test.go: "Go tests must be table-driven when they contain more than one case."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/user_clients_test.go` around lines 137 - 203, Refactor TestUserClientsGetToolsMapsAndSanitizesTitleAndAnnotations into table-driven subtests, with separate cases for top-level title and annotations, annotations-only fallback, and missing title/annotations. Define each case’s input and expected values in the table, run them via t.Run, and preserve all existing assertions for sanitization, annotations, fallback behavior, and empty title handling.Source: Coding guidelines
webapp/src/components/tool_card.tsx (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTruncated titles have no way to reveal the full text.
Long MCP titles are ellipsized but there's no tooltip/
titleattribute to let users see the full name on hover.💡 Suggested fix
- <ToolName>{displayName}</ToolName> + <ToolName title={displayName}>{displayName}</ToolName>Also applies to: 560-560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/tool_card.tsx` around lines 59 - 71, Update the ToolName styled span usage to expose the complete tool title through a hover tooltip, such as a title attribute, while preserving the existing single-line ellipsis styling. Apply the same change to the corresponding title rendering at the other affected location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@conversation/content_block.go`:
- Around line 151-184: Update SanitizeForDisplay to sanitize the MCPBareName
field for BlockTypeToolUse blocks before returning the copied slice, using the
same llm.SanitizeNonPrintableChars helper as Input, Title, and Description.
Preserve the existing nil handling and non-mutating behavior.
In `@conversation/convert.go`:
- Around line 83-97: Update enrichToolCallFromStore and its invocation in the
toolCall rehydration path so an existing toolCall.Title is preserved, even when
OverwriteDescription is enabled. Only populate the title from the current tool
catalog when the persisted title is empty; keep description enrichment and
redacted-tool behavior unchanged.
---
Nitpick comments:
In `@llm/tools.go`:
- Around line 67-80: Update the DestructiveHint field comment in ToolAnnotations
to state that nil means the server omitted the hint and the MCP specification
treats omitted destructiveHint as true (destructive), while preserving the
existing capture semantics.
In `@mcp/user_clients_test.go`:
- Around line 137-203: Refactor
TestUserClientsGetToolsMapsAndSanitizesTitleAndAnnotations into table-driven
subtests, with separate cases for top-level title and annotations,
annotations-only fallback, and missing title/annotations. Define each case’s
input and expected values in the table, run them via t.Run, and preserve all
existing assertions for sanitization, annotations, fallback behavior, and empty
title handling.
In `@webapp/src/components/tool_card.tsx`:
- Around line 59-71: Update the ToolName styled span usage to expose the
complete tool title through a hover tooltip, such as a title attribute, while
preserving the existing single-line ellipsis styling. Apply the same change to
the corresponding title rendering at the other affected location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c278c1e7-f3f9-4018-9d10-c44541a15483
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
conversation/content_block.goconversation/content_block_test.goconversation/convert.goconversation/convert_test.goconversation/helpers.goconversation/helpers_test.gollm/tools.gollm/tools_test.gomcp/user_clients.gomcp/user_clients_test.gostreaming/streaming.gostreaming/tool_call_parity_test.gotoolrunner/toolrunner.gotoolrunner/toolrunner_extended_test.gowebapp/src/components/llmbot_post/turn_content_utils.test.tswebapp/src/components/llmbot_post/turn_content_utils.tswebapp/src/components/tool_card.tsxwebapp/src/components/tool_types.tswebapp/src/types/conversation.tswebapp/src/utils/tool_identity.test.tswebapp/src/utils/tool_identity.ts
Split the undefined-origin case out of the test.each table into its own test so ESLint's no-undefined rule passes. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Replace the pretty-printed JSON blob in ToolCard with a labeled, readable field list driven purely by the arguments object (insertion key order, so the layout is identical on the live and persisted paths). Values render by JS type: strings as plain wrapped text, numbers/booleans/primitive-arrays as compact value pills, nested objects/arrays as a small inline JSON block. All values render as plain text via styled-components, never through formatText/markdown. A single card-level Show more expands all clamped long values, and a required View raw toggle reveals the exact pretty-printed JSON payload so the approval surface always lets the user inspect what they are approving. Empty-object args keep the verbatim No parameters required message; null/redacted args render nothing. Value pills never carry an icon/avatar (reserved for Phase 3 entity chips). Header, status, buttons, result rendering, and the result-review callout are untouched. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Extract the shared approval chrome (header, collapse, decision buttons, result section, result-review callout) into tool_renderers/tool_card_shell.tsx, and move the View raw affordance onto the shell so rich cards inherit it. ToolCard becomes a thin shell + ToolArguments field-list body. ToolArguments keeps the field list + Show more; its raw view is exposed via ToolArgumentsRaw. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Add tool_renderers/registry.tsx routing each tool call to a rich card or QuestionCard (migrated as the proving entry), falling back to the generic ToolCard; matching is on canonical identity (origin kind + bare name) plus a strict parse. Add entity_chips.tsx (ChannelChip/UserChip: resolve via store or API, icon/avatar on success, plain-text raw id/username on failure). Add rich cards for create_post, dm, group_message, search_posts, search_users, read_post, get_channel_info with strict parse-or-fallback. tool_approval_set routes through the registry. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ritance Extract pure parsers into rich_card_parsers.ts (testable without React deps). Add rich_card_parsers.parsers.test (valid/malformed/redacted -> null), entity chip tests (store + API resolution, failure degradation to plain text), and registry routing tests (QuestionCard, embedded rich cards, external/unknown/ malformed fall back to generic; rich cards expose View raw with the exact payload). Migrate tool_approval_set.test to mock the registry. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Add tests/tool-config/mock-api/rich-create-post-card.spec.ts asserting the rich create_post card renders the resolved channel and message body and that View raw exposes the exact payload; assign it to the lightest e2e shard (shard-1). Mirrors the existing mock-api tool-config specs. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ange) The lockfile was inadvertently rewritten by a local npm install (npm dropped "peer" markers); no webapp dependency actually changed. Restore master's lockfile so the CI lockfile-drift check passes. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ange) Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…ange) Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Completes the drift-guard's writer-completeness check for the conversation-side auto-run writer (toolUseBlocks is unexported, so streaming/tool_call_parity_test cannot reach it): assert toolUseBlocks and PostToBlocks emit every persisted tool_use identity/metadata field for a fully-populated call. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…tool-call-rendering-phase-2-f0d9 Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…or/rich-tool-call-rendering-phase-3-f0d9 Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
conversation/tool_use_writer_parity_test.go (1)
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoop over
requireassertions hides all-but-the-first drifted field.Both tests loop over
persistedToolUseFieldsusingrequire.Containsf/require.NotEmptyf. SincerequirecallsFailNowon the first failure, a regression that drops/empties multiple fields will only ever report the first one per test run, weakening this drift-guard's diagnostic value across CI reruns. Isolating each field in at.Runsubtest (optionally withassertinside) would let every dropped field surface in a single run and better matches the table-driven pattern for tests with more than one case.♻️ Suggested refactor (illustrative)
m := toolUseBlockJSONMap(t, blocks[0]) for _, field := range persistedToolUseFields { - require.Containsf(t, m, field, "toolUseBlocks dropped persisted tool_use field %q", field) - require.NotEmptyf(t, m[field], "toolUseBlocks emitted an empty persisted tool_use field %q", field) + field := field + t.Run(field, func(t *testing.T) { + assert.Containsf(t, m, field, "toolUseBlocks dropped persisted tool_use field %q", field) + assert.NotEmptyf(t, m[field], "toolUseBlocks emitted an empty persisted tool_use field %q", field) + }) }As per coding guidelines, "Go tests must be table-driven when they contain more than one case."
Also applies to: 76-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conversation/tool_use_writer_parity_test.go` around lines 60 - 70, Refactor the loops in TestToolUseBlocksPersistsPolicyFields and the corresponding test around the shared persistedToolUseFields assertions into table-driven t.Run subtests, using each field as the subtest name. Keep the existing field-presence and non-empty validations, but isolate failures per field so all drifted fields are reported in one run.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@conversation/tool_use_writer_parity_test.go`:
- Around line 60-70: Refactor the loops in TestToolUseBlocksPersistsPolicyFields
and the corresponding test around the shared persistedToolUseFields assertions
into table-driven t.Run subtests, using each field as the subtest name. Keep the
existing field-presence and non-empty validations, but isolate failures per
field so all drifted fields are reported in one run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7c0065c6-c749-4dff-825c-0b28e0c7c723
📒 Files selected for processing (1)
conversation/tool_use_writer_parity_test.go
This was referenced Jul 13, 2026
crspeller
marked this pull request as ready for review
July 20, 2026 15:32
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765acff55a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+269
to
+270
| if llmTool.Title == "" { | ||
| llmTool.Title = llmTool.Annotations.Title |
There was a problem hiding this comment.
When an MCP tool declares a top-level or annotation title containing only spaces or allowed whitespace such as tabs, this equality check treats it as a valid effective title. toolDisplayName then returns that truthy title verbatim, so the tool card renders with a blank name instead of falling back to the prettified bare tool name. Trim titles, or test strings.TrimSpace when resolving the effective title.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
webapp/src/components/tool_arguments.tsx (1)
228-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
ToolArgumentsRawinstead of re-implementing the raw-JSON block.The top-level array/primitive fallback (lines 291-297) duplicates the exact markup already defined in
ToolArgumentsRaw(lines 231-240). Delegate to the component to keep the raw view in one place.♻️ Proposed dedup
- if (entries.length === 0) { - return ( - <Container> - <RawJson>{JSON.stringify(args, null, 2)}</RawJson> - </Container> - ); - } + if (entries.length === 0) { + return <ToolArgumentsRaw arguments={args}/>; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/tool_arguments.tsx` around lines 228 - 297, Update the top-level array/primitive fallback in ToolArguments to return ToolArgumentsRaw with the existing args value instead of duplicating the Container and RawJson markup. Keep the current entries.length === 0 condition and preserve the existing raw JSON rendering behavior.webapp/src/components/tool_renderers/tool_card_shell.tsx (1)
399-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the plugin-state selector.
(state: any) => state['plugins-' + manifest.id]?.allowUnsafeLinks ?? falseloses type safety. Define a minimal local interface for the plugin state slice instead of casting toany.As per coding guidelines, "keep TypeScript strictly typed" for webapp
.ts/.tsxfiles.♻️ Proposed fix
- const allowUnsafeLinks = useSelector<GlobalState, boolean>((state: any) => state['plugins-' + manifest.id]?.allowUnsafeLinks ?? false); + interface PluginState { + allowUnsafeLinks?: boolean; + } + const allowUnsafeLinks = useSelector<GlobalState & Record<string, PluginState | undefined>, boolean>( + (state) => state['plugins-' + manifest.id]?.allowUnsafeLinks ?? false, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/tool_renderers/tool_card_shell.tsx` around lines 399 - 401, Replace the any-typed selector callback for allowUnsafeLinks with a minimal typed interface describing the plugin state slice, and use that interface in the selector while preserving the dynamic manifest.id lookup and false fallback. Update the selector near allowUnsafeLinks; leave the siteURL and team selectors unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/components/tool_renderers/rich_card_parsers.ts`:
- Around line 99-106: Replace hardcoded English labels with i18n-backed values
across the affected renderers: in
webapp/src/components/tool_renderers/rich_card_parsers.ts lines 99-106, store
stable message keys in searchFilterKeys and localize them where LabeledPill is
rendered; in lines 137-151, apply the same approach to parseSearchUsers’ “Limit”
label; in webapp/src/components/tool_renderers/rich_cards.tsx lines 245-249 and
288-297, localize ReadPostCard’s “ID” and GetChannelInfoCard’s “Name” and “Team”
labels using the surrounding formatMessage/FormattedMessage pattern.
In `@webapp/src/components/tool_renderers/tool_card_shell.tsx`:
- Around line 550-558: Make the canExpand ToolCallHeader keyboard-accessible by
adding an appropriate interactive role, tabIndex, and keyboard handler that
invokes onToggleCollapse for Enter and Space while preserving the existing click
behavior. Update the ToolCallHeader usage in the tool card shell so
non-expandable cards remain non-interactive.
---
Nitpick comments:
In `@webapp/src/components/tool_arguments.tsx`:
- Around line 228-297: Update the top-level array/primitive fallback in
ToolArguments to return ToolArgumentsRaw with the existing args value instead of
duplicating the Container and RawJson markup. Keep the current entries.length
=== 0 condition and preserve the existing raw JSON rendering behavior.
In `@webapp/src/components/tool_renderers/tool_card_shell.tsx`:
- Around line 399-401: Replace the any-typed selector callback for
allowUnsafeLinks with a minimal typed interface describing the plugin state
slice, and use that interface in the selector while preserving the dynamic
manifest.id lookup and false fallback. Update the selector near
allowUnsafeLinks; leave the siteURL and team selectors unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9b3a082d-caa5-44ea-a49d-371bb09e43fc
📒 Files selected for processing (19)
e2e/scripts/ci-test-groups.mjse2e/tests/tool-config/mock-api/rich-create-post-card.spec.tswebapp/src/client.tsxwebapp/src/components/tool_approval_set.test.tsxwebapp/src/components/tool_approval_set.tsxwebapp/src/components/tool_arguments.test.tsxwebapp/src/components/tool_arguments.tsxwebapp/src/components/tool_card.test.tsxwebapp/src/components/tool_card.tsxwebapp/src/components/tool_renderers/entity_chips.test.tsxwebapp/src/components/tool_renderers/entity_chips.tsxwebapp/src/components/tool_renderers/registry.test.tsxwebapp/src/components/tool_renderers/registry.tsxwebapp/src/components/tool_renderers/rich_card_parsers.tswebapp/src/components/tool_renderers/rich_card_parts.tsxwebapp/src/components/tool_renderers/rich_cards.parsers.test.tswebapp/src/components/tool_renderers/rich_cards.tsxwebapp/src/components/tool_renderers/tool_card_shell.tsxwebapp/src/i18n/en.json
Comments now target the future reader concisely; no development-phase narrative. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/components/tool_renderers/entity_chips.tsx`:
- Around line 76-77: Update the module-level entity caches used by the renderer
to avoid retaining null failure entries indefinitely: add a bounded retry/TTL
for failed fetches, or invalidate those entries when authentication or server
context changes. Preserve successful entity caching while allowing transient
failures and permission changes to be retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c09ca2d8-b45f-46f2-83df-fec2d77c662e
📒 Files selected for processing (21)
conversation/content_block.goconversation/content_block_test.goconversation/tool_use_writer_parity_test.goe2e/tests/tool-config/mock-api/rich-create-post-card.spec.tsllm/tools.gomcp/user_clients.gostreaming/streaming.gostreaming/tool_call_parity_test.gowebapp/src/components/llmbot_post/turn_content_utils.tswebapp/src/components/tool_approval_set.test.tsxwebapp/src/components/tool_approval_set.tsxwebapp/src/components/tool_arguments.tsxwebapp/src/components/tool_renderers/entity_chips.tsxwebapp/src/components/tool_renderers/registry.tsxwebapp/src/components/tool_renderers/rich_card_parsers.tswebapp/src/components/tool_renderers/rich_card_parts.tsxwebapp/src/components/tool_renderers/rich_cards.tsxwebapp/src/components/tool_renderers/tool_card_shell.tsxwebapp/src/components/tool_types.tswebapp/src/types/conversation.tswebapp/src/utils/tool_identity.ts
💤 Files with no reviewable changes (1)
- webapp/src/components/llmbot_post/turn_content_utils.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- webapp/src/components/tool_approval_set.tsx
- mcp/user_clients.go
- conversation/tool_use_writer_parity_test.go
- webapp/src/utils/tool_identity.ts
- webapp/src/types/conversation.ts
- conversation/content_block.go
- webapp/src/components/tool_types.ts
- llm/tools.go
- webapp/src/components/tool_renderers/rich_card_parts.tsx
- webapp/src/components/tool_arguments.tsx
- streaming/streaming.go
- webapp/src/components/tool_renderers/rich_card_parsers.ts
- webapp/src/components/tool_renderers/rich_cards.tsx
- e2e/tests/tool-config/mock-api/rich-create-post-card.spec.ts
- webapp/src/components/tool_renderers/tool_card_shell.tsx
- conversation/content_block_test.go
- streaming/tool_call_parity_test.go
Registry matchers now match on identity only — the cards already fall back to the generic ToolCard when their strict parse fails, so arguments were parsed twice per render. Replace the field-by-field toRichProps copy with rest destructuring. Remove unused canonicalToolKey, the EmbeddedServerOrigin export, and the speculative channel-icon branch. Use the UserInteractionSelect constant instead of a string literal. Rename the parser test to match its source file. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
cursor
Bot
changed the title
- Treat whitespace-only MCP titles as absent so cards never render a blank header (Codex); covered in the GetTools test, now table-driven (CodeRabbit). - Localize the remaining hardcoded pill labels (filter names, ID, Name, Team): parsers emit stable keys, labels render via FormattedMessage. - Make the card header toggle keyboard-accessible (role/tabIndex/Enter/Space, aria-expanded) and expose ellipsized titles via a hover tooltip. - Expire cached entity-chip failures after 30s so transient fetch errors recover without a reload; note that omitted destructiveHint means destructive per the MCP spec. - Parity drift-guards report every drifted field via per-field subtests instead of stopping at the first failure. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Bordered rounded card container with a larger header: chevron, tool name, and a muted plain-text context (target channel, recipients, or query) supplied by the rich cards — the name element keeps its exact text. Arguments render as a two-column grid (bold labels left, values right) in both the generic field list and the rich cards. Accept is now the filled primary action. All existing behavior is unchanged: entity chips, value pills, Show more, View raw, result section, review callout, statuses, and redaction. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
The redesigned header shows the channel context and the chip shows 'name · team'; assert each specifically instead of a substring that now matches both. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Channel chips use the globe/lock icons like the channel UI (no '#', which is not a Mattermost convention); the card container matches QuestionCard's border radius and shadow. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
Tool results now use the same generic treatment as arguments: JSON-object results render as the labeled field list, everything else as clamped plain text — replacing the markdown code-block pipeline (and its formatText dependency) in the card shell. Replace the seven thin per-tool cards with one that adds real value: read_post renders a permalink-style preview of the referenced post (via PostPreview / PostMessagePreview), falling back to the generic card when the arguments don't parse or the post can't be fetched. The registry keeps QuestionCard routing. Prune code that had no consumer: MCP annotation safety-hint capture, the header-context slot, entity chips, filter-label i18n strings, and getProfilesByUsernames. Swap the create_post e2e spec for a read_post preview spec. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
…onse The read_post card now shows a permalink-style preview, so the referenced post's text legitimately appears twice on the expanded card. The image-count and image-request assertions are unchanged and now also cover the preview. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
The permalink-style preview renders the post with Mattermost's normal markdown, which is redundant once the call has executed (the response shows the content) and would sit next to the deliberately-unrendered result text. Executed calls render the generic card, restoring the original guarantees of the unsafe-post result spec, which is reverted to its strict assertions. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
While a create_post call awaits approval, render its message as a permalink-style post preview (authored by the requesting user, whose session the embedded tool executes with) — the same rendering the message gets once posted — so the user sees exactly what they are approving. Executed or malformed calls render the generic card. Extend the preview e2e spec to cover both cards with a shared container. Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
The previewed post does not exist yet, so its permalink/avatar/username affordances cannot lead anywhere; suppress pointer events on the preview. The read_post preview stays interactive (its post is real). Co-authored-by: Christopher Speller <crspeller@users.noreply.github.com>
cursor
Bot
changed the title
|
Test server destroyed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the raw-JSON display of LLM tool calls with readable, safe rendering: a generic labeled field list for arguments and responses, permalink-style post previews for
read_postandcreate_post, and the server plumbing that makes a call render identically live and after reload.Foundation: path-identical tool metadata
llm.ToolCallgainsTitle(resolved MCP display name); the unusedSchemafield is removed from the wire (providers build tool definitions fromllm.Tool.Schema, and requests fromid/name/arguments/result—ToolCall.Schemanever reached a provider and the webapp ignored it).mcpGetToolsresolves the effective display title from MCP metadata (title>annotations.title, whitespace-only treated as absent) and Unicode-sanitizesTitle/Descriptionat capture (bidi/invisible-char defense).ContentBlockgainsTitle/Description; both block writers andPostToBlocks/BlocksToPostcarry them;SanitizeForDisplaysanitizes them;FilterForNonRequesterkeeps them visible (tool identity, likeName).redactToolCallsnow keepsTitle/Descriptiontoo — fixing a live↔persisted asymmetry whereDescriptionwas dropped by omission on the live path.buildContentBlocksalso gains the previously missingMCPBareName.streaming/tool_call_parity_test.go+conversation/tool_use_writer_parity_test.go) maps everyllm.ToolCallJSON field to a persistence + redaction policy and fails (per-field subtests) if any writer or redactor diverges.Card UX (per design, Mattermost styling)
formatText/markdown — so neither LLM-generated arguments nor tool responses can become a link-spoofing surface (the old code-block pipeline for results is gone).arguments == null(redacted) renders nothing;{}keeps the verbatim "No parameters required". The header toggle is keyboard-accessible (role='button', Enter/Space,aria-expanded), and ellipsized titles expose a hover tooltip.Renderer registry + post preview cards
tool_renderers/registry.tsxroutes each call by canonical identity (origin kind + bare name); first match wins; no match → generic card.QuestionCardis routed through it too.read_postshows a Mattermost permalink-style preview of the referenced post, andcreate_postshows a preview of the post-to-be — the message rendered exactly as it will appear once posted, authored by the requesting user (whose session the embedded tool executes with). So the user sees precisely what the tool will read or create before approving. The create_post preview is non-interactive (pointer-events: none): the post doesn't exist yet, so its permalink/avatar affordances can't lead anywhere; the read_post preview stays clickable since its post is real. Both fall back to the generic field list when the arguments don't parse, the post can't be fetched, or the call has already executed.External MCP tools that declare a
titlenow display it (sanitized, ellipsized); built-in/embedded names are unchanged. External-server render overrides are out of scope.Before / after
Rendered with the real components at the current head of this branch (before =
master'sToolCard). The "after" shows both post previews (pending approval), the executed-call generic fallback, plain-text and JSON response rendering, and collapsed cards. (Screenshots are hosted on thecursor/rich-tool-call-rendering-assets-f0d9branch — not part of the plugin code.)Testing
Title/Descriptionare dropped fromredactToolCalls.tests/tool-config/mock-api/tool-preview-cards.spec.ts(mocked LLM, shared container) covers both cards — read_post shows the seeded post's preview and create_post shows the post-to-be's message before approval, with View raw exposing the exact payloads; assigned toe2e-shard-1. Existing tool-config specs pass unchanged (display names, button labels, status text, and the expand target are unchanged).Ticket Link
NONE
Release Note
Summary by CodeRabbit