-
Notifications
You must be signed in to change notification settings - Fork 2
Support multiple output formats in tool renderers #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- list_drives: map 'title' field to 'name' for DriveListRenderer - list_pages: parse flat 'paths' strings into tree structure for PageTreeRenderer - get_activity: flatten grouped 'drives[].activities' format into flat 'activities' array for ActivityRenderer https://claude.ai/code/session_01AKG5EjddKT1CVdkLYfnvX9
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughEnhances the ToolCallRenderer component to support multiple format variations for AI tool outputs: adds a tree-conversion utility for page paths, normalizes drive object fields, and extends activity handling to process both direct activity arrays and grouped drive formats with standardized metadata enrichment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 539453fd61
ℹ️ 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".
| const currentSegment = page.pathSegments[depth]; | ||
| const isDirectChild = page.pathSegments.length === depth + 1; | ||
|
|
||
| if (!seen.has(currentSegment)) { | ||
| seen.set(currentSegment, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve pages with duplicate titles in paths tree
The new parsePathsToTree groups nodes by currentSegment (the title segment) rather than by a stable identifier. If a drive has two sibling pages with the same title (which is allowed—titles aren’t unique), they will be merged into a single entry and one page will be dropped from the rendered tree. This only shows up for list_pages outputs that use the paths format. Consider keying the grouping by a unique identifier (e.g., include pageId alongside the segment in the map key) so distinct pages with identical titles don’t collapse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c7d3c31.
Changed the grouping logic to use a composite key ${segment}:${pageId} for direct children, so pages with identical titles are now preserved as separate entries in the tree. Intermediate folders still use just the segment name since they're synthetic nodes without their own pageId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ai/shared/chat/tool-calls/ToolCallRenderer.tsx`:
- Around line 49-123: buildTreeFromParsed currently seeds a non-null page for
every segment which lets synthetic/intermediate folder nodes inherit descendant
pageId/type; change the seen map entry so page is optional (e.g., { page?:
ParsedPage; children: ParsedPage[] }), only assign entry.page when isDirectChild
is true, and when constructing the TreeItem use page?.type ?? 'folder' (or a
folder-like type) and page?.pageId ?? undefined so synthetic nodes have no
pageId and use a folder type; make these changes inside parsePathsToTree /
buildTreeFromParsed (refer to ParsedPage, buildTreeFromParsed, and the TreeItem
construction).
- Around line 543-624: The renderer is synthesizing IDs instead of using
upstream CUID2 ids—add an id:string to the CompactActivity type in
activity-tools.ts, ensure the function that maps DB rows to CompactActivity
includes the source id field, and update the ToolCallRenderer.tsx mapping (the
code that builds flatActivities inside the get_activity branch) to use
activity.id for ActivityItem.id (and update the ActivityItem/props types if
needed) rather than constructing
`${group.drive.id}-${activity.ts}-${activity.pageId}`; keep the rest of
opToAction and sorting logic unchanged.
🧹 Nitpick comments (1)
apps/web/src/components/ai/shared/chat/tool-calls/ToolCallRenderer.tsx (1)
279-303: PreferdriveNamefallback to avoid slug-only labelsUsing only
driveSlugcan show a technical slug (or nothing) even when a friendly name exists. PreferdriveName ?? driveSlug.🔧 Suggested tweak
- <PageTreeRenderer - tree={tree} - driveName={parsedOutput.driveSlug as string | undefined} - driveId={parsedOutput.driveId as string | undefined} - /> + <PageTreeRenderer + tree={tree} + driveName={(parsedOutput.driveName ?? parsedOutput.driveSlug) as string | undefined} + driveId={parsedOutput.driveId as string | undefined} + />
- Fix synthetic folder nodes inheriting descendant IDs/types by making page optional in buildTreeFromParsed and defaulting to 'FOLDER' type - Fix duplicate titles collapsing by using composite key (pageId+segment) - Pass CUID2 activity IDs from tool output instead of synthesizing them - Prefer driveName over driveSlug for friendly display names - Remove unused DriveInfo import to fix lint error Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Response to Code Review FeedbackThank you for the thorough review! I've addressed all the feedback in commit c7d3c31: 1. Synthetic folder nodes inheriting descendant IDs/types (CodeRabbit)Fixed: Made 2. Duplicate titles collapsing (Codex)Fixed: Changed grouping key from just the segment title to a composite key 3. Synthesized activity IDs (CodeRabbit)Fixed: Added 4. DriveSlug-only labels (CodeRabbit nitpick)Fixed: Changed to prefer Also fixed lint errors (unused |
Summary
Enhanced tool call renderers to support multiple output formats from AI tools, improving compatibility with different response structures while maintaining backward compatibility.
Key Changes
pathsarray format to tree structure via newparsePathsToTree()helper function that parses path strings and builds a hierarchical treetitleto renderer's expectednamefield, with fallback logicdrivesformat in addition to flatactivitiesarray, with automatic flattening and operation-to-action mappingImplementation Details
"📁 [FOLDER](Task) ID: xxx Path: /drive/folder"These changes allow the renderers to work with both current and future tool output formats without breaking existing functionality.
https://claude.ai/code/session_01AKG5EjddKT1CVdkLYfnvX9
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes