fix(ai-react): sync tools to ChatClient when tools prop changes#776
fix(ai-react): sync tools to ChatClient when tools prop changes#776citizen204 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesTools sync in useChat
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| useEffect(() => { | ||
| // Conditional spread: `updateOptions` declares strict-optional | ||
| // fields and rejects explicit `undefined` under EOPT. | ||
| client.updateOptions({ | ||
| body: options.body, | ||
| ...(options.forwardedProps !== undefined && { | ||
| forwardedProps: options.forwardedProps, | ||
| }), | ||
| ...(options.tools !== undefined && { | ||
| tools: options.tools, | ||
| }), | ||
| context: options.context, | ||
| }) | ||
| }, [client, options.body, options.forwardedProps, options.context]) | ||
| }, [client, options.body, options.forwardedProps, options.tools, options.context]) | ||
|
|
There was a problem hiding this comment.
wouldn't it be better to have separate useEffect to avoid updating other fields repeatedly when one changes
the underlying code
ai/packages/ai-client/src/chat-client.ts
Line 1439 in 2cbf4f8
just updates all fields present in the provided options.
With separate useEffects, updates in let's say forwardedRef wouldn't update the tools
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and show the updateOptions implementation in chat-client.ts
fd 'chat-client.ts' --type f | xargs grep -n 'updateOptions' | head -30
fd 'chat-client.ts' packages/ai-client --type f | xargs -I{} sed -n '1420,1480p' {}Length of output: 2435
🏁 Script executed:
#!/bin/bash
# Get more of the updateOptions implementation, especially the tools handling
fd 'chat-client.ts' packages/ai-client --type f | xargs -I{} sed -n '1480,1540p' {}Length of output: 1697
@manish-baghel Your concern is well-founded. Looking at the updateOptions implementation, the tools branch is not idempotent — it always rebuilds the tool registry and fires notifyToolsChanged():
if (options.tools !== undefined) {
this.clientToolsRef.current = new Map() // new Map every time
for (const tool of options.tools) {
this.clientToolsRef.current.set(tool.name, tool)
}
this.devtoolsBridge.notifyToolsChanged() // devtools event every time
}So with the current single combined effect, changing body or forwardedProps will trigger the effect, and if options.tools is defined it will unnecessarily rebuild the tool registry and fire a devtools notification. The same applies in reverse — changing tools re-sends body/forwardedProps/context even if those haven't changed (though those assignments are cheaper no-ops for body/forwardedProps since they only set a ref).
Separate effects are the cleaner fix:
useEffect(() => {
client.updateOptions({ body: options.body })
}, [client, options.body])
useEffect(() => {
if (options.forwardedProps !== undefined) {
client.updateOptions({ forwardedProps: options.forwardedProps })
}
}, [client, options.forwardedProps])
useEffect(() => {
if (options.tools !== undefined) {
client.updateOptions({ tools: options.tools })
}
}, [client, options.tools])
useEffect(() => {
client.updateOptions({ context: options.context })
}, [client, options.context])This ensures each field update is surgical — a body change doesn't touch the tool registry, and a tools change doesn't re-send body or context. Would you like me to plan this refactor?
2 participants
Summary
useChatinitialises theChatClientwith thetoolsoption passed at construction time, but never callsclient.updateOptions({ tools })when thetoolsprop changes during the component lifecycle. This means the client-side tool registry becomes stale when callers swap tools — e.g. a project-management app whereupdateTaskToolcloses overprojectIdand the user switches projects.ChatClient.updateOptions()already acceptstoolsand rebuildsclientToolsRefwhen it receives them. The fix is to addtoolsto the existingupdateOptionseffect that already syncsbody,forwardedProps, andcontext.Changes
packages/ai-react/src/use-chat.ts: conditionally spreadoptions.toolsinto theupdateOptionscall and add it to the dependency array, matching the existing pattern forforwardedProps.Fixes #775
Test plan
useChatwith atoolsarray that closes over a value (e.g.projectId).projectIdwhile the component stays mounted.useMemotheir tools array (as shown in the issue) to avoid unnecessary re-registrations.Summary by CodeRabbit