Fix tool execution bug #327
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatInput
participant Messages
participant Message
participant FunctionCalling
participant useToolExecution
participant executeFunctionAPI
User->>ChatInput: Submit message / trigger tool call
ChatInput->>Messages: Update messages
Messages->>Message: Render message parts
Message->>FunctionCalling: Render tool-invocation part
FunctionCalling->>useToolExecution: Initialize with toolInvocation, API key, etc.
useToolExecution->>executeFunctionAPI: Call tool with args and credentials
executeFunctionAPI-->>useToolExecution: Return result or error
useToolExecution->>FunctionCalling: Provide error state (if any)
useToolExecution->>Messages: addToolResult callback with result
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
frontend/src/app/playground/message.tsx (1)
127-136: 💡 Verification agent🧩 Analysis chain
Consider updating the memo comparison function.
The memo implementation doesn't compare the new props (
linkedAccountOwnerId,apiKey, andaddToolResult). If these props could change between renders, consider updating the comparison function.
🏁 Script executed:
#!/bin/bash # Check if linkedAccountOwnerId or apiKey could change during component lifecycle # by looking for state updates or context changes related to these values echo "Searching for code that might update linkedAccountOwnerId or apiKey:" rg -A 2 -B 2 "setState.*linkedAccountOwnerId|setState.*apiKey|useEffect.*linkedAccountOwnerId|useEffect.*apiKey" --type=tsx --type=ts echo "Searching for components that might provide these values via context:" rg -A 2 -B 2 "Provider.*value.*linkedAccountOwnerId|Provider.*value.*apiKey" --type=tsx --type=tsLength of output: 587
I’m not seeing those props defined in the comparator’s file—let’s confirm whether
PurePreviewMessageactually receiveslinkedAccountOwnerId,apiKey, oraddToolResult. If it doesn’t, the original suggestion may be unnecessary. Let’s extract the component signature and look for those props:
🏁 Script executed:
#!/bin/bash # Locate PurePreviewMessage and any occurrences of the queried props rg -n "PurePreviewMessage" -C3 frontend/src/app/playground/message.tsx rg -n "linkedAccountOwnerId|apiKey|addToolResult" -C3 frontend/src/app/playground/message.tsxLength of output: 1400
Update the memo comparator to include all props
The
PreviewMessagememo only checksisLoading,message.id, andmessage.parts, butPurePreviewMessagealso takeslinkedAccountOwnerId,apiKey, andaddToolResult. If any of those change between renders, the component will be skipped incorrectly.Please update
frontend/src/app/playground/message.tsxaround lines 127–136:export const PreviewMessage = memo( PurePreviewMessage, - (prevProps, nextProps) => { - if (prevProps.isLoading !== nextProps.isLoading) return false; - if (prevProps.message.id !== nextProps.message.id) return false; - if (!equal(prevProps.message.parts, nextProps.message.parts)) return false; + (prev, next) => { + if (prev.isLoading !== next.isLoading) return false; + if (prev.message.id !== next.message.id) return false; + if (!equal(prev.message.parts, next.message.parts)) return false; + if (prev.linkedAccountOwnerId !== next.linkedAccountOwnerId) return false; + if (prev.apiKey !== next.apiKey) return false; + if (prev.addToolResult !== next.addToolResult) return false; return true; }, );
- Compare
linkedAccountOwnerIdandapiKeyby value.- Compare the callback
addToolResultby reference.frontend/src/app/playground/messages.tsx (1)
55-62: 🛠️ Refactor suggestionPotential issue in memo comparison function.
The memo implementation has two concerns:
- It doesn't compare the new props (
linkedAccountOwnerId,apiKey, andaddToolResult)- The condition
if (prevProps.status && nextProps.status) return false;will always return false (triggering re-renders) when both statuses are truthy, which may be inefficientexport const Messages = memo(PureMessages, (prevProps, nextProps) => { if (prevProps.status !== nextProps.status) return false; - if (prevProps.status && nextProps.status) return false; if (prevProps.messages.length !== nextProps.messages.length) return false; if (!equal(prevProps.messages, nextProps.messages)) return false; + // Only re-render if linkedAccountOwnerId or apiKey changes + if (prevProps.linkedAccountOwnerId !== nextProps.linkedAccountOwnerId) return false; + if (prevProps.apiKey !== nextProps.apiKey) return false; + // addToolResult is likely a stable function reference, so skipping that comparison return true; });
🧹 Nitpick comments (6)
frontend/src/app/playground/function-calling.tsx (1)
21-23: Added error handling.Good addition of error handling, though the error message display could be improved.
if (error) { - return <div>Function Calling Error: {error}</div>; + return ( + <div className="text-red-500 border border-red-200 rounded-md p-2 flex items-center space-x-2"> + <svg xmlns="http://www.w3.org/2000/svg" className="h-5 w-5" viewBox="0 0 20 20" fill="currentColor"> + <path fillRule="evenodd" d="M10 18a8 8 0 100-16 8 8 0 000 16zM8.707 7.293a1 1 0 00-1.414 1.414L8.586 10l-1.293 1.293a1 1 0 101.414 1.414L10 11.414l1.293 1.293a1 1 0 001.414-1.414L11.414 10l1.293-1.293a1 1 0 00-1.414-1.414L10 8.586 8.707 7.293z" clipRule="evenodd" /> + </svg> + <span>Tool execution failed: {error}</span> + </div> + ); }frontend/src/app/playground/page.tsx (1)
59-62: MemoisehandleAddToolResultto prevent needless child re-rendersBecause
handleAddToolResultis created on every render, React will treat it as a new
prop and causeMessages, and anything further down, to re-render even when nothing
else changed. Wrapping the callback inuseCallback(or at leastuseMemo) gives it
stable identity and avoids extra work.- const handleAddToolResult = ({ toolCallId, result }: { toolCallId: string; result: object }) => { - addToolResult({ toolCallId, result }); - }; + const handleAddToolResult = useCallback( + ({ toolCallId, result }: { toolCallId: string; result: object }) => { + addToolResult({ toolCallId, result }); + }, + [addToolResult], + );frontend/src/hooks/use-tool-execution.ts (2)
26-35: Current duplicate-call guard is scoped per component instance
hasStartedCheckinglives inside each hook instance, so two components rendering the
sametoolCallIdat roughly the same time will still both fire the network request.
If the intent is to guarantee global deduplication, consider moving theSet
outside the hook or into a Recoil/Zustand atom.No code change mandatory, but worth verifying the requirement.
70-75:useLayoutEffectis overkill for network requests
useLayoutEffectblocks the browser from painting; it’s meant for DOM
measurements/patches that must happen before paint.
A plainuseEffectis adequate (and avoids React v18 SSR warnings).- useLayoutEffect(() => { + useEffect(() => {frontend/src/app/playground/chat-input.tsx (2)
30-34:setMessages((messages) => messages)is a no-opCalling the setter with the previous state unmodified does nothing besides a wasted
render pass.
If you meant to refresh the UI afterstop(), fetch the latest messages from the
server instead; otherwise remove the call.- stop(); - setMessages((messages) => messages); + stop();
95-110: Hard-coded status literals scattered in the key handler
"submitted","streaming"and"ready"are compared in several places. A small
enum (or union type) exported from the chat hook would:
- avoid typos,
- make autocomplete work, and
- keep the UI consistent when new states are added.
Example:
export const ChatStatus = { READY: "ready", SUBMITTED: "submitted", STREAMING: "streaming", } as const; export type ChatStatus = typeof ChatStatus[keyof typeof ChatStatus];Then use
ChatStatus.READY, etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/package.json(1 hunks)frontend/src/app/appconfigs/[appName]/page.tsx(1 hunks)frontend/src/app/linked-accounts/page.tsx(1 hunks)frontend/src/app/playground/chat-input.tsx(1 hunks)frontend/src/app/playground/function-calling.tsx(1 hunks)frontend/src/app/playground/message.tsx(2 hunks)frontend/src/app/playground/messages.tsx(2 hunks)frontend/src/app/playground/page.tsx(3 hunks)frontend/src/hooks/use-tool-execution.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Compose Tests
- GitHub Check: Format & Lint
🔇 Additional comments (15)
frontend/src/app/linked-accounts/page.tsx (1)
406-406: Trivial whitespace-only change: the trailing newline at EOF was removed. No functional impact.frontend/src/app/appconfigs/[appName]/page.tsx (1)
340-340: No-op formatting change
The removal of the trailing newline at the end of the file is purely cosmetic and has no functional impact.frontend/package.json (1)
63-63: Addedusehooks-tslibrary to support the new tool execution functionality.Good addition of the
usehooks-tspackage which provides useful React hooks like debounce functionality that will help prevent duplicate API requests during tool execution.frontend/src/app/playground/message.tsx (3)
18-20: Props added to support tool execution functionality.These new props will be passed down to the FunctionCalling component, enabling proper tool execution handling.
24-26: Well-defined prop types for the new parameters.The TypeScript definitions are clear and properly typed for the new functionality.
112-114: Properly passing required props to FunctionCalling component.These props will enable the FunctionCalling component to execute tools and report results back through the callback.
frontend/src/app/playground/messages.tsx (3)
12-14: Added necessary props to MessagesProps interface.These new props will enable tool execution functionality throughout the message components.
17-17: Updated PureMessages component to receive new props.Properly updated the function signature to include the new parameters.
37-39: Correctly passing new props to PreviewMessage components.This ensures the tool execution functionality flows down to individual messages.
frontend/src/app/playground/function-calling.tsx (5)
1-1: Added import for the new useToolExecution hook.This hook will handle the logic for executing tools and prevent duplicate API calls.
6-8: Updated FunctionCallingProps with necessary parameters.Added the required props to properly type the component.
11-11: Updated component signature to include new props.Properly destructured the new parameters for use within the component.
14-19: Implemented useToolExecution hook.The hook is properly configured with all necessary parameters to handle tool execution.
26-28: Improved UI during tool execution.The loading spinner provides good visual feedback when a tool is being executed.
frontend/src/app/playground/page.tsx (1)
40-57: Guard against an emptyapiKeybefore calling the chat endpoint
apiKeyis derived fromgetApiKey(activeProject)and may be an empty string when a
project has no key yet.
useChatwill still issue the request with an emptyx-api-keyheader, which will
(1) fail every call and (2) still count against rate limits on some back-ends.
Consider bailing out early or disabling the input until a valid key is present:if (!apiKey) { return <div>Please configure an API key for this project.</div>; }or pass
enabled: Boolean(apiKey)to the hook (if supported).
| }, [ | ||
| toolInvocation.toolCallId, | ||
| toolName, | ||
| args, | ||
| addToolResult, | ||
| linkedAccountOwnerId, | ||
| apiKey, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
args in dependency list may trigger needless re-creations
Objects coming from props often get recreated on every render, so including args
directly in the dependency array causes executeToolCallback to be regenerated and
debounced work to restart even when the content is unchanged.
Either
- rely on
toolInvocation.toolCallId(which is unique and already present), or - stabilise
argswithuseMemo/JSON.stringify.
- args,
+ /* args intentionally omitted – toolCallId uniqueness is sufficient */,📝 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.
| }, [ | |
| toolInvocation.toolCallId, | |
| toolName, | |
| args, | |
| addToolResult, | |
| linkedAccountOwnerId, | |
| apiKey, | |
| ]); | |
| }, [ | |
| toolInvocation.toolCallId, | |
| toolName, | |
| /* args intentionally omitted – toolCallId uniqueness is sufficient */, | |
| addToolResult, | |
| linkedAccountOwnerId, | |
| apiKey, | |
| ]); |
jiwei-aipolabs
force-pushed
the
fix_tool_execution_bug
branch
from
1278202 to
4ba3582
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/app/playground/chat-input.tsx (1)
98-99: Remove redundant condition check.The
linkedAccountOwnerIdis already checked at the beginning of the handler (line 91).if ( event.key === "Enter" && - !event.shiftKey && - linkedAccountOwnerId + !event.shiftKey ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
frontend/package.json(1 hunks)frontend/src/app/playground/chat-input.tsx(1 hunks)frontend/src/app/playground/function-calling.tsx(1 hunks)frontend/src/app/playground/message.tsx(2 hunks)frontend/src/app/playground/messages.tsx(2 hunks)frontend/src/app/playground/page.tsx(3 hunks)frontend/src/hooks/use-tool-execution.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/package.json
- frontend/src/app/playground/message.tsx
- frontend/src/app/playground/function-calling.tsx
- frontend/src/hooks/use-tool-execution.ts
- frontend/src/app/playground/messages.tsx
- frontend/src/app/playground/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Compose Tests
- GitHub Check: Format & Lint
- GitHub Check: Format, Lint, and Test
🔇 Additional comments (10)
frontend/src/app/playground/chat-input.tsx (10)
4-6: Good import organization.The new imports are well-selected for the enhanced functionality. Using
lucide-reacticons provides consistent visual elements, while the React optimization hooks will help with performance.
14-15: Proper typing for new optional props.Adding optional stop controls is a good approach for supporting the new functionality while maintaining backward compatibility.
18-39: Well-structured StopButton component.The PureStopButton component is properly typed with clear props and includes testing attributes. The implementation correctly prevents default form behavior and handles stopping.
41-41: Good use of memoization.Using memo for the StopButton is appropriate for performance optimization when props rarely change.
43-53: Well-designed SendButton component.The button properly disables when the input is empty and maintains consistent styling with the StopButton.
55-59: Effective custom memoization comparison.The custom comparison for SendButton optimization is a good performance enhancement, only re-rendering when input empty state changes.
70-76: Good form handling optimization.Using useCallback for the form submission handler prevents unnecessary re-renders and properly manages form events.
90-110: Improved keydown handler with proper validation.The keydown handler now properly validates the linked account and prevents submission during ongoing requests, addressing the duplicate requests issue mentioned in the PR objectives.
111-114: Enhanced textarea styling and behavior.The increased right padding (pr-20) accommodates the absolutely positioned button overlay, and the disabled state prevents interaction during processing.
115-123: Elegant button positioning and conditional rendering.The UI implementation for the buttons is well-executed with proper positioning and conditional rendering based on the current status.
| <div className="pointer-events-none"> | ||
| <div className="absolute top-1/2 right-4 -translate-y-1/2 flex flex-row justify-end pointer-events-auto"> | ||
| {status === "submitted" || status === "streaming" ? ( | ||
| <StopButton stop={stop!} setMessages={setMessages!} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-null assertions on optional props.
The use of non-null assertions (!) on optional props could lead to runtime errors if these props aren't provided.
- <StopButton stop={stop!} setMessages={setMessages!} />
+ {stop && setMessages && <StopButton stop={stop} setMessages={setMessages} />}Or provide default no-op functions:
- <StopButton stop={stop!} setMessages={setMessages!} />
+ <StopButton stop={stop || (() => {})} setMessages={setMessages || (cb => cb([]))} />📝 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.
| <StopButton stop={stop!} setMessages={setMessages!} /> | |
| {stop && setMessages && <StopButton stop={stop} setMessages={setMessages} />} |
| <StopButton stop={stop!} setMessages={setMessages!} /> | |
| <StopButton | |
| stop={stop || (() => {})} | |
| setMessages={setMessages || (cb => cb([]))} | |
| /> |
🏷️ Ticket
https://www.notion.so/duplicate-chat-call-after-tool-execution-1e38378d6a4780f48cb5dca16e11ec21?pvs=4
📝 Description
fix bug duplicate
/chatapi requestimprove ux(add stop button for agent playground)
🎥 Demo (if applicable)
20250506192128-ACI.DEV.Platform-.-1.mp4
📸 Screenshots (if applicable)
✅ Checklist
Summary by CodeRabbit
New Features
Improvements
Chores