Skip to content

fix(ai): preserve chat response for onFinish#16145

Closed
kiranmagic7 wants to merge 1 commit into
vercel:mainfrom
kiranmagic7:fix/chat-active-response-finish-15668
Closed

fix(ai): preserve chat response for onFinish#16145
kiranmagic7 wants to merge 1 commit into
vercel:mainfrom
kiranmagic7:fix/chat-active-response-finish-15668

Conversation

@kiranmagic7

Copy link
Copy Markdown

Summary

Closes #15668.

  • keep a request-local activeResponse reference for onFinish instead of reading the mutable instance field in finally
  • avoid clearing this.activeResponse when another overlapping request has already replaced it
  • add a regression test that clears activeResponse before stream finish and verifies onFinish still receives the assistant message without logging an error
  • add a patch changeset for ai

Tests

  • pnpm test:node src/ui/chat.test.ts
  • pnpm --filter ai type-check
  • pnpm check
  • pnpm --filter ai build

@kiranmagic7

Copy link
Copy Markdown
Author

Quick status check on current head 1a9d9cb86a0f680a1b229302b9b4e935010a1de8: the PR still reports mergeable from the PR view, Socket Security and Vercel Agent Review are green, and the only red item I can see is the Vercel preview/deployment authorization, which I cannot clear from this fork.

Review focus: whether preserving the response object for onFinish matches the intended chat callback contract. If you prefer a narrower shape or a different test location, I can adjust it.

@kiranmagic7

Copy link
Copy Markdown
Author

Friendly follow-up: current head 1a9d9cb86a0f680a1b229302b9b4e935010a1de8 is still mergeable with no conflicts. Socket and Vercel Agent Review checks pass; the only pending item is the Vercel deployment authorization from the fork.

Happy to rebase or adjust the approach if that would help. Let me know if there are questions or if there is a preferred path for the deployment authorization.

@aayush-kapoor aayush-kapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closing in favor of #16620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chat.makeRequest finally throws on this.activeResponse!.state when activeResponse is undefined (should be ?.)

2 participants