fix(share): improve shared messages tool handlers and add missing tools #195
Conversation
- Extract SharedMessagePartHandler into dedicated component for better maintainability - Fix terminal output extraction to handle nested result object structure (was showing [object Object] due to incorrect type assumption) - Add SharedTodoBlock component for rendering todo lists in shared chats - Add support for additional tool types: http_request, update_memory, match (glob/grep) - Add proper sidebar integration for viewing tool execution details - Refactor SharedMessages.tsx to use new component architecture - Update tests to match new action text in tool blocks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors SharedMessages component by extracting inline message part rendering logic into a new SharedMessagePartHandler component that centralizes handling for text, tools, and content types. Introduces SharedTodoBlock for standalone todo rendering. Updates test expectations for status text labels. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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
🤖 Fix all issues with AI agents
In `@app/share/`[shareId]/components/SharedTodoBlock.tsx:
- Around line 148-156: The SharedTodoBlock renders an actual <button> (toggled
by showAllTodos via handleToggleViewAll) inside a parent Button component,
creating invalid nested interactive elements; fix by removing the inner <button>
and either (a) replace it with a non-interactive element (e.g., a <span> or
<div> with role="button" or onClick handler) styled like a button, or (b) move
the toggle control out of the parent Button so handleToggleViewAll and the
showAllTodos text are rendered as a sibling element; update the JSX that checks
isExpanded && headerContent.showViewAll to use the non-nested element and ensure
it keeps the same styling and onClick behavior.
In `@app/share/`[shareId]/SharedMessages.tsx:
- Around line 56-66: The frontend separates file/image placeholders into
filePlaceholders and otherParts but only treats filePlaceholders specially for
user messages (see isUser check) causing assistant placeholders to stay in
otherParts and render right-aligned inside a left-aligned assistant bubble via
SharedMessagePartHandler; update the rendering logic in SharedMessages (use the
same extraction and special rendering for filePlaceholders regardless of message
role) by moving the filePlaceholders handling out of the isUser-only branch (or
modify the isUser condition to include assistant placeholders) so placeholders
are extracted from message.parts and rendered outside the bubble consistently
for both user and assistant messages.
🧹 Nitpick comments (5)
app/share/[shareId]/components/SharedTodoBlock.tsx (3)
9-12: UnusedblockIdprop.The
blockIdprop is declared in the interface but never used within the component. Either remove it from the props or use it (e.g., fordata-testidor as a key prefix).♻️ Proposed fix to remove unused prop
interface SharedTodoBlockProps { todos: Todo[]; - blockId: string; } -export const SharedTodoBlock = ({ todos, blockId }: SharedTodoBlockProps) => { +export const SharedTodoBlock = ({ todos }: SharedTodoBlockProps) => {
79-127: Consider memoizinggetVisibleTodos.Unlike
todoDataandheaderContent,getVisibleTodosis a plain function that gets recreated and executed on every render. For consistency and to avoid unnecessary recalculations, consider converting it to auseMemohook.♻️ Proposed refactor
- const getVisibleTodos = () => { + const visibleTodos = useMemo(() => { const { hasProgress, done, currentInProgress, byStatus } = todoData; if (!hasProgress || done === 0) { return todos; } if (showAllTodos) { return todos; } // Show collapsed view: last completed + current in-progress - const visibleTodos: Todo[] = []; + const result: Todo[] = []; // ... rest of logic using result instead of visibleTodos - return visibleTodos; - }; + return result; + }, [todoData, showAllTodos, todos]);Then update the render call:
- {getVisibleTodos().map((todo) => ( + {visibleTodos.map((todo) => (
30-33: Redundant condition ingetVisibleTodos.The condition
!hasProgress || done === 0on line 82 is redundant becausehasProgressis defined asdone > 0(line 33). This means!hasProgressis equivalent todone === 0, making the entire condition justdone === 0.♻️ Simplify the condition
- if (!hasProgress || done === 0) { + if (done === 0) { return todos; }Also applies to: 82-84
app/share/[shareId]/components/SharedMessagePartHandler.tsx (1)
33-42: Consider addingdatafield to MessagePart interface.The
data-summarizationrenderer (line 711) accessespart.datavia type assertion(part as any).data. Adding an optionaldatafield to the interface would improve type safety.♻️ Proposed interface update
interface MessagePart { type: string; text?: string; placeholder?: boolean; state?: string; input?: any; output?: any; toolCallId?: string; errorText?: string; + data?: { status?: string; message?: string }; }Then update the renderer:
-function renderSummarizationPart(part: MessagePart, idx: number) { - const data = (part as any).data as { status?: string; message?: string }; +function renderSummarizationPart(part: MessagePart, idx: number) { + const data = part.data;app/share/[shareId]/__tests__/SharedMessages.test.tsx (1)
289-309: Consider adding test coverage for SharedTodoBlock with actual todos.The current test only covers the fallback case when no
currentTodosare present (rendering "Updated todos"). Consider adding a test case wherecurrentTodosis populated to verifySharedTodoBlockrenders correctly with todo items.📝 Example test case
it("should render todo block with todos when currentTodos provided", () => { const messages = [ { id: "1", role: "assistant" as const, parts: [ { type: "tool-todo_write", state: "output-available", output: { currentTodos: [ { id: "t1", content: "First task", status: "completed" }, { id: "t2", content: "Second task", status: "in_progress" }, ], }, }, ], update_time: mockShareDate, }, ]; renderWithContext( <SharedMessages messages={messages} shareDate={mockShareDate} />, ); // Verify SharedTodoBlock renders (e.g., check for progress text or expand button) expect(screen.getByRole("button", { name: /todos/i })).toBeInTheDocument(); });
| {isExpanded && headerContent.showViewAll && ( | ||
| <button | ||
| type="button" | ||
| onClick={handleToggleViewAll} | ||
| className="text-[12px] text-muted-foreground/70 hover:text-muted-foreground transition-colors cursor-pointer p-1 ml-2 bg-transparent border-none" | ||
| > | ||
| {showAllTodos ? "Hide" : "View All"} | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Nested interactive elements: button inside Button.
The "View All" <button> is rendered inside the <Button> component, creating invalid HTML (button-in-button nesting). This causes accessibility issues and unpredictable click behavior. Consider restructuring to avoid nesting or moving the toggle outside the header button.
🛠️ Proposed fix
One approach is to use a non-interactive element with click handling, or restructure the layout:
{isExpanded && headerContent.showViewAll && (
- <button
- type="button"
+ <span
+ role="button"
+ tabIndex={0}
onClick={handleToggleViewAll}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ handleToggleViewAll(e);
+ }
+ }}
className="text-[12px] text-muted-foreground/70 hover:text-muted-foreground transition-colors cursor-pointer p-1 ml-2 bg-transparent border-none"
>
{showAllTodos ? "Hide" : "View All"}
- </button>
+ </span>
)}Alternatively, move the "View All" toggle outside the <Button> entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {isExpanded && headerContent.showViewAll && ( | |
| <button | |
| type="button" | |
| onClick={handleToggleViewAll} | |
| className="text-[12px] text-muted-foreground/70 hover:text-muted-foreground transition-colors cursor-pointer p-1 ml-2 bg-transparent border-none" | |
| > | |
| {showAllTodos ? "Hide" : "View All"} | |
| </button> | |
| )} | |
| {isExpanded && headerContent.showViewAll && ( | |
| <span | |
| role="button" | |
| tabIndex={0} | |
| onClick={handleToggleViewAll} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| handleToggleViewAll(e); | |
| } | |
| }} | |
| className="text-[12px] text-muted-foreground/70 hover:text-muted-foreground transition-colors cursor-pointer p-1 ml-2 bg-transparent border-none" | |
| > | |
| {showAllTodos ? "Hide" : "View All"} | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
In `@app/share/`[shareId]/components/SharedTodoBlock.tsx around lines 148 - 156,
The SharedTodoBlock renders an actual <button> (toggled by showAllTodos via
handleToggleViewAll) inside a parent Button component, creating invalid nested
interactive elements; fix by removing the inner <button> and either (a) replace
it with a non-interactive element (e.g., a <span> or <div> with role="button" or
onClick handler) styled like a button, or (b) move the toggle control out of the
parent Button so handleToggleViewAll and the showAllTodos text are rendered as a
sibling element; update the JSX that checks isExpanded &&
headerContent.showViewAll to use the non-nested element and ensure it keeps the
same styling and onClick behavior.
| const filePlaceholders = message.parts.filter( | ||
| (part) => | ||
| (part.type === "file" || part.type === "image") && part.placeholder, | ||
| (part.type === "file" || part.type === "image") && part.placeholder | ||
| ); | ||
| const otherParts = message.parts.filter( | ||
| (part) => | ||
| !( | ||
| (part.type === "file" || part.type === "image") && | ||
| part.placeholder | ||
| ), | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SharedMessages.tsx" -type fRepository: hackerai-tech/hackerai
Length of output: 106
🏁 Script executed:
head -100 app/share/\[shareId\]/SharedMessages.tsxRepository: hackerai-tech/hackerai
Length of output: 3143
🏁 Script executed:
sed -n '50,100p' app/share/\[shareId\]/SharedMessages.tsxRepository: hackerai-tech/hackerai
Length of output: 1990
🏁 Script executed:
rg -n "SharedMessagePartHandler" app/share/\[shareId\]/ -A 20Repository: hackerai-tech/hackerai
Length of output: 6261
🏁 Script executed:
cat app/share/\[shareId\]/components/SharedMessagePartHandler.tsxRepository: hackerai-tech/hackerai
Length of output: 20136
🏁 Script executed:
rg -n "part.type.*file|part.type.*image" app/share/\[shareId\]/components/SharedMessagePartHandler.tsx -A 10Repository: hackerai-tech/hackerai
Length of output: 3233
🏁 Script executed:
rg -n "placeholder.*true|placeholder:" . --type=ts --type=tsx -A 3 -B 3 | head -100Repository: hackerai-tech/hackerai
Length of output: 93
🏁 Script executed:
# Search for message part creation and placeholder handling
rg -n "parts.*placeholder|placeholder.*parts" . --type=ts --type=tsx -B 2 -A 2Repository: hackerai-tech/hackerai
Length of output: 93
🏁 Script executed:
# Check if there's any evidence of assistant messages with file/image placeholders
rg -n "assistant.*file|assistant.*image" . --type=ts --type=tsx -B 3 -A 3 | head -80Repository: hackerai-tech/hackerai
Length of output: 93
🏁 Script executed:
rg -n "placeholder.*true" . -A 3 -B 3 | head -150Repository: hackerai-tech/hackerai
Length of output: 2387
🏁 Script executed:
# Search for where message parts are created
rg -n "parts.*push|parts.*\=" . -B 2 -A 2 -t ts | grep -A 5 -B 5 "placeholder" | head -100Repository: hackerai-tech/hackerai
Length of output: 172
🏁 Script executed:
# Look at the message structure and where it's populated from API
rg -n "role.*assistant|assistant.*message" . -t ts -A 5 | head -80Repository: hackerai-tech/hackerai
Length of output: 4576
🏁 Script executed:
sed -n '1220,1260p' ./convex/messages.tsRepository: hackerai-tech/hackerai
Length of output: 1536
🏁 Script executed:
# Also check if there are any other places placeholders are set
rg -n "placeholder\s*:" . -t ts -B 5 -A 2Repository: hackerai-tech/hackerai
Length of output: 12224
🏁 Script executed:
sed -n '75,140p' app/share/\[shareId\]/SharedMessages.tsxRepository: hackerai-tech/hackerai
Length of output: 2753
Address placeholder rendering inconsistency for non-user messages.
File/image placeholders are converted to simple indicators by the backend for all message roles. However, the frontend separates and renders them specially (outside the message bubble) only for user messages (isUser check on line 81). For non-user (assistant) messages, placeholders remain in otherParts and are passed to SharedMessagePartHandler, which renders them with right-aligned styling inside the left-aligned assistant message bubble, creating a visual misalignment. Either extract and render assistant placeholders separately like user messages, or confirm this behavior is intentional.
🤖 Prompt for AI Agents
In `@app/share/`[shareId]/SharedMessages.tsx around lines 56 - 66, The frontend
separates file/image placeholders into filePlaceholders and otherParts but only
treats filePlaceholders specially for user messages (see isUser check) causing
assistant placeholders to stay in otherParts and render right-aligned inside a
left-aligned assistant bubble via SharedMessagePartHandler; update the rendering
logic in SharedMessages (use the same extraction and special rendering for
filePlaceholders regardless of message role) by moving the filePlaceholders
handling out of the isUser-only branch (or modify the isUser condition to
include assistant placeholders) so placeholders are extracted from message.parts
and rendered outside the bubble consistently for both user and assistant
messages.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.