Camand/tools display#605
Open
camilleAND wants to merge 1 commit into
Open
Conversation
Show expandable tool cards with loader
WalkthroughTool invocation rendering now uses formatted, expandable cards arranged in timelines. Visibility and document-parsing handling are centralized, previews and statuses are generated by utilities, and the loader supports configurable sizing. ChangesTool invocation display
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant MessageItem
participant ToolInvocationTimeline
participant ToolInvocationItem
participant ToolCard
MessageItem->>MessageItem: Filter visible tool invocations
MessageItem->>ToolInvocationTimeline: Render visible invocations
ToolInvocationTimeline->>ToolInvocationItem: Render each timeline item
ToolInvocationItem->>ToolCard: Pass summarized tool invocation
ToolCard->>ToolCard: Compute status and readable previews
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts`:
- Around line 50-54: Update the translation mock in
src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts:50-54
to interpolate both count and query placeholders, producing “Query: climate
news”; update the t stub in
src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx:6-8
to interpolate options.query, producing “Query: latest news”.
In
`@src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts`:
- Around line 201-213: Update formatGenericOutputPreview and its callers,
including getToolExpandedOutputPreview, to accept and pass the existing
translation function t. Replace the hardcoded results/items preview labels with
localized translations, and update getToolErrorFromResult to use t for its
failure message while preserving the current preview and error behavior.
🪄 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
Review profile: CHILL
Plan: Pro
Run ID: c2906e83-ddb1-4f53-841f-c50ff61a504d
📒 Files selected for processing (10)
CHANGELOG.mdsrc/frontend/apps/conversations/src/components/Loader.tsxsrc/frontend/apps/conversations/src/features/chat/components/MessageItem.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolCard.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolInvocationItem.tsxsrc/frontend/apps/conversations/src/features/chat/components/ToolInvocationTimeline.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ToolInvocationTimeline.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.tssrc/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts
Comment on lines
+50
to
+54
| (key, options) => | ||
| key.replace('{{count}}', String(options?.count ?? '')), | ||
| ); | ||
|
|
||
| expect(content.inputPreview).toBe('Query: climate news'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test files and implementation around the referenced lines.
for f in \
src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts \
src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx \
src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts
do
echo "===== $f ====="
wc -l "$f"
echo
done
echo "===== toolCardUtils.test.ts (lines 1-140) ====="
sed -n '1,140p' src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts
echo "===== ToolCard.test.tsx (lines 1-180) ====="
sed -n '1,180p' src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx
echo "===== toolCardUtils.ts (lines 1-220) ====="
sed -n '1,220p' src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.tsRepository: suitenumerique/conversations
Length of output: 11982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts
sed -n '1,180p' src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx
sed -n '1,220p' src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.tsRepository: suitenumerique/conversations
Length of output: 11277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rest of toolCardUtils.ts where getToolReadableContent is defined.
sed -n '220,360p' src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts
# Also inspect ToolCard for how the input preview is rendered.
sed -n '1,260p' src/frontend/apps/conversations/src/features/chat/components/ToolCard.tsxRepository: suitenumerique/conversations
Length of output: 8830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,360p' src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts
sed -n '1,260p' src/frontend/apps/conversations/src/features/chat/components/ToolCard.tsxRepository: suitenumerique/conversations
Length of output: 8830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,360p' src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts
sed -n '1,260p' src/frontend/apps/conversations/src/features/chat/components/ToolCard.tsxRepository: suitenumerique/conversations
Length of output: 8830
Interpolate {{query}} in the translation mocks Both tests expect rendered query text, but the mocked t functions leave {{query}} untouched. Update the stubs so Query: {{query}} becomes Query: climate news / Query: latest news in these suites.
src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts: extend the mock to handlequeryas well ascount.src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx: changet: (key: string) => keyto interpolateoptions.query.
📍 Affects 2 files
src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts#L50-L54(this comment)src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx#L6-L8
🤖 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
`@src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts`
around lines 50 - 54, Update the translation mock in
src/frontend/apps/conversations/src/features/chat/components/__tests__/toolCardUtils.test.ts:50-54
to interpolate both count and query placeholders, producing “Query: climate
news”; update the t stub in
src/frontend/apps/conversations/src/features/chat/components/__tests__/ToolCard.test.tsx:6-8
to interpolate options.query, producing “Query: latest news”.
Comment on lines
+201
to
+213
| if (Array.isArray(output.results)) { | ||
| return truncateText( | ||
| `${output.results.length} results`, | ||
| maxLength, | ||
| ); | ||
| } | ||
|
|
||
| const keys = Object.keys(output); | ||
| if (keys.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return truncateText(`${keys.length} items`, maxLength); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hardcoded English in generic previews bypasses i18n.
formatGenericOutputPreview renders "${n} results" and "${n} items" as literals (and getToolErrorFromResult at Line 82 returns literal 'Tool execution failed'), while the rest of the module localizes via t. Non-English users will see these untranslated. Thread t into this helper and wrap the strings, mirroring formatWebSearchOutputPreview.
🌐 Suggested direction
-const formatGenericOutputPreview = (
- output: unknown,
- maxLength: number,
-): string | undefined => {
+const formatGenericOutputPreview = (
+ output: unknown,
+ t: (key: string, options?: Record<string, unknown>) => string,
+ maxLength: number,
+): string | undefined => {
@@
- if (Array.isArray(output.results)) {
- return truncateText(
- `${output.results.length} results`,
- maxLength,
- );
- }
+ if (Array.isArray(output.results)) {
+ return truncateText(
+ t('{{count}} results', { count: output.results.length }),
+ maxLength,
+ );
+ }
@@
- return truncateText(`${keys.length} items`, maxLength);
+ return truncateText(t('{{count}} items', { count: keys.length }), maxLength);Callers at Lines 283/290 (and the getToolExpandedOutputPreview path) will need to pass t.
🤖 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
`@src/frontend/apps/conversations/src/features/chat/components/toolCardUtils.ts`
around lines 201 - 213, Update formatGenericOutputPreview and its callers,
including getToolExpandedOutputPreview, to accept and pass the existing
translation function t. Replace the hardcoded results/items preview labels with
localized translations, and update getToolErrorFromResult to use t for its
failure message while preserving the current preview and error behavior.
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
1 participant
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.
Purpose
Make tool calls in the chat clearer and less noisy: compact expandable cards with readable status, instead of a generic "Search..." / "Summarizing..." loader while tools run.
Proposal
ToolCardwith collapsed header (tool name + status) and expandable input/output/error detailsToolInvocationTimelineto stack multiple tool calls with a visual connectortoolCardUtilsLoaderwhile a tool is runningconversation_resumeanddocument_parsingerrorsToolCard,ToolInvocationTimeline, andtoolCardUtils