-
Notifications
You must be signed in to change notification settings - Fork 327
fix(ai-react): prevent stale agent capture in aiFetch; ensure active connection is used #465
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
fix(ai-react): prevent stale agent capture in aiFetch; ensure active connection is used #465
Conversation
🦋 Changeset detectedLatest commit: 7df04c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
|
@BeiXiao before we review, could you please fix the build? thank you |
packages/agents/src/ai-react.tsx
Outdated
|
|
||
| return new Response(stream); | ||
| } | ||
| }, [agentUrlString]); |
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.
if we have this, why do we need to put agent on a ref?
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.
if we have this, why do we need to put agent on a ref?
The agentRef ensures we always use the latest agent object inside the callback. Without it, the callback would be stuck with the old agent from when it was first created.
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.
but doesn't agentUrlString changing also capture the new agent instance?
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.
but doesn't agentUrlString changing also capture the new agent instance?
OK, latest commit has removed agentUrlString from the dependency array.
The main issue here is that the agent captured in aiFetch is unconnected, so we used agentRef to preserve the agent instance.
…useCallback and memoize transport to prevent capturing outdated connections
8bc7e8e to
763d12c
Compare
Hi, fixed it, all checks have passed 🎉 |
|
Does this really fix the issue? It seems a bit strange. In the ai-sdk, the transport is used in AbstractChat #L214C2-L214C3, and the Chat instance is created in useChat #L61. The logic for ai-sdk to determine whether to update the chat instance is in useChat #L65. It appears that this change doesn't update the transport in the ai-sdk. To update the instance, you would need to pass a new chat or a new id, like in #440. |
|
FYI just straight up removing |
|
We encountered something similar, I think this issue is related to this #440 Removing strict-mode prevents the "change of ID" from happening Or if you do something like this: const agent = useAgent({
agent: 'chat',
name: conversationId,
})
const agentChat = useAgentChat({
agent,
id: agent._pk,
})Not sure what approach folks want to go with, but would be great to unify the conversations: |
super useful feedback, thanks for this, we just need to really dive deeply into whats happening and discuss the best available fix, should be not too long |
threepointone
left a 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.
ok I see now the problem, since useChat doesn't use the latest fetch passed to it. this PR looks good.
|
I don't recommend using _pk as the chat identifier since it's unique per connection, I suspect .name might be better... I need to dig into that. in the meanwhile, this PR seems ok. |
Description
Reproduction
Implementation