Uh oh!
There was an error while loading. Please reload this page.
perf(plugin-chatbot): stop rebuilding the chat transport on every render (#4187) - #5603
Merged
Merged
Conversation
…der (#4187) `useObjectChat` memoized its `DefaultChatTransport` on a dep list that included the caller's `body` and `headers`. Both are object props and every caller passes a fresh literal each render, so the memo never hit: a transport was constructed on every render of every chat surface, which during a streaming turn is once per token batch. Both are now read through refs inside `prepareSendMessagesRequest` and are gone from the dep list -- the idiom this hook already uses for the live model (`modelRef`) and the handoff conversation id (`parentConvRef`), and the only one of the two candidate fixes a future caller cannot silently undo by forgetting a `useMemo` of its own. Premise re-measured against the installed `@ai-sdk/react@4.0.68` rather than inherited from the 4.0.59 the card was written against: `useChat` still keeps the transport in `latestRef` and delegates through `getTransport()`, and `shouldRecreateChat` is still keyed only on `chat`/`id`, neither of which this hook passes. So the thread was never at risk and the rebuild was pure waste, exactly as filed. The one behavioural difference is when the two values are sampled: a send reads them at send time, so it observes the most recent render's values rather than those of the last render that happened to rebuild the transport -- never staler than before, and now unconditional. Merge order is preserved in both directions: the hook-owned body keys still win over the caller's body, and a per-send header still wins over the caller's headers. `useObjectChat.transportIdentity.test.tsx` pins one construction across renders, the rebuild that a real dep change must still cause, the sampling timing, and the key precedence. Co-authored-by: Claude <noreply@anthropic.com>
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
…ransport site (#4187) The PR body is not what the next person reads. Moving `headers` out of the `DefaultChatTransport` constructor also took them off `reconnectToStream`, which reads the constructor's `headers` through `prepareReconnectToStreamRequest` and never runs `prepareSendMessagesRequest`. No consumer in this repo resumes a stream today, so nothing is missing them -- but whoever wires up `resumeStream` later would get an unauthenticated resume with no reason to suspect this hook. Comment only, at the exact line the `headers` option used to occupy. No behaviour, no test, nothing executable to pin. Co-authored-by: Claude <noreply@anthropic.com>
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
commented
Aug 21, 2026
CollaboratorAuthor
PM review — ACCEPT (card #4187)Gates. 22 named check runs read individually for The measurement this card actually turned onThe card's entire "dormant, waste-only" framing was written against Measured against the installed build rather than assumed:
So the restart condition fired on the version string, not on a behaviour change, and the severity is unchanged. That is the honest answer rather than the dramatic one, and it is what lets this land as a hygiene fix instead of a hotfix. Three things the implementer got right that I did not ask for
The one thing I asked forThe NotesBundle. Lint. Two new #5605 filed for the Generated by Claude Code |
os-sales
marked this pull request as ready for review
August 21, 2026 17:32
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#4187
1. Premise re-measured against the installed 4.0.68 — still dormant, severity unchanged
The card's whole "waste-only" framing was written against
@ai-sdk/react@4.0.59; the package now carries^4.0.68, which is what fired the hold's restart condition. I read the installed build rather than inheriting the analysis:node_modules/.pnpm/@ai-sdk+react@4.0.68_react@19.2.8_zod@4.4.3/node_modules/@ai-sdk/react/dist/index.js, version field4.0.68.Q: does
useChatstill store the transport in a ref and delegate through agetTransport()indirection? Yes, verbatim (dist/index.js:301-322):latestRef.currentis reassigned during every render andgetTransport()is called at send time, so the transport is late-bound.Q: is
Chatrecreation still keyed only onchat/id, or does transport identity now participate? Still onlychat/id— transport identity does not participate (dist/index.js:347):useObjectChatpassestransport,messagesandonErrorand neitherchatnorid, so theChatobject — and the message list with it — survives every transport rebuild.Conclusion: this is NOT a live per-render thread reset. 4.0.68 behaves exactly as the card described 4.0.59, so the severity is unchanged: construction waste only, nothing a user hits. The restart condition fired on the version string, not on a behaviour change. What the bump does confirm is the card's own argument for fixing it anyway — the safety is a property of the SDK's internals that a future release can withdraw silently, and the memo gives a reviewer no warning because it is already written as though it works.
2. The fix
packages/plugin-chatbot/src/useObjectChat.ts. The caller'sbodyandheadersmove out of the transportuseMemodep list and are read through refs at send time, the idiom the hook already uses two dozen lines above for the live model (modelRef) and the handoff conversation id (parentConvRef):bodyRef/headersRefare assigned on every render, beside the existing refs;conversationId,model,systemPrompt,stream) and no longer takesheaders;prepareSendMessagesRequestspreadsbodyRef.currentunder the request body and returns the caller's headers merged into the SDK's base headers;[isApiMode, api, model, systemPrompt, streamingEnabled, conversationId].Call-site memoization was considered and rejected upstream of this PR as unenforceable; this is the only one of the two a future caller cannot silently undo by forgetting a
useMemoof its own.Merge order is preserved in both directions, which is the part of the diff most able to regress quietly:
prepareSendMessagesRequestits ownbodyalready merged as constructor-body over per-send body, so spreading the caller's live body underneath it reproduces the old precedence exactly: caller body loses to the hook-owned keys, which lose to a per-send body. Pinned by the "hook-owned keys winning over the caller body" test.prepareSendMessagesRequestREPLACES the SDK's base headers rather than merging with them, so the caller's headers are re-merged underneath the base set. A per-send header still wins, as it did when the caller's headers were the transport's ownheadersoption.One consequence, and it is recorded in the file rather than only here:
headersleaving the constructor also takes them offreconnectToStream, which reads the constructor'sheadersthroughprepareReconnectToStreamRequestand never runsprepareSendMessagesRequest. That path has no consumer in this repo —grep -rn "resumeStream\|reconnectToStream"overpackages/andapps/returns nothing outsidenode_modules— so nothing regresses today. Because a PR body is not what the next person reads, the invariant now sits as a comment on the exact line theheadersoption used to occupy, naming what a futureresumeStreamconsumer has to merge and where.3. The sampling-timing answer (the one real behavioural difference)
Which values does a send observe after the change? Those of the most recent completed render, read at send time.
bodyRef.current/headersRef.currentare assigned during every render;prepareSendMessagesRequestruns synchronously at the top ofHttpChatTransport.sendMessages, which is itself reached throughgetTransport()at that instant. So the pair a send serializes is always the last-rendered pair, spread at send time.What did it observe before? The transport spread
body/headersinto itself at CONSTRUCTION time, so a send serialized the values of the last render that rebuilt the transport. With both in the dep list and every caller passing a fresh literal, that was in practice also the last render — which is exactly why nothing changes for any caller that exists today. But the guarantee was incidental: it held only because the memo never hit. A caller with a stablebodyidentity got a value frozen at the first construction.Net: the new read is never staler than the old one and is now unconditional rather than a side effect of the memo missing. It is pinned by
useObjectChat.transportIdentity.test.tsx, whose fourth case holds one object identity across the whole test — with a stable identity the memo never re-ran even before this change, so the value a send observes is decided purely by when it is read. That case fails onmainwithexpected 1 to be 2and passes here.4. Tests
New:
packages/plugin-chatbot/src/__tests__/useObjectChat.transportIdentity.test.tsx, five cases, countingnew DefaultChatTransport(...)with a construct-trap Proxy around the real class so the hook still talks to the genuine transport.bodyat SEND time, not at transport-construction time — the sampling change;Reverse-verification
Both ablations restored through
trap ... EXIT INT TERM, each mutation proved on disk with an anchoredgrep -cin both directions before reading any result.A — the deps put back on the memo list (
}, [isApiMode, api, headers, body, model, ...]);): fixed-form count 1 then 0, ablated-form count 0 then 1,git diff --stat1 file changed, 1 insertion(+), 1 deletion(-). Case 1 fails withAssertionError: expected 4 to be 1— four renders, four constructions — and 4 of 5 still pass. Restored,git status --porcelainempty.B — the whole hook reverted to
origin/main(git checkout origin/main -- packages/plugin-chatbot/src/useObjectChat.ts): fix markerbodyRef.current = body;count 1 then 0, pre-fix markerheaders: { ...headers },count 0 then 1. Cases 1 and 4 fail (expected 4 to be 1,expected 1 to be 2— the frozen construction-time body), cases 2, 3 and 5 pass, which is the predicted split.git diff --statprinted nothing for this leg becausegit checkout ref -- pathwrites the index too, so that diff was staged; the two-directional grep is the load-bearing proof. Restored, and the tree verified byte-identical to HEAD afterwards:git status --porcelainandgit diff HEAD --name-onlyboth empty.No ablation for the invariant comment in section 2 — a comment has nothing executable to pin.
Gates, all at
72bd9df02with a clean treeExit codes captured before any pipe; each line is the gate's own verdict. The three gates that can see a comment change were re-run on the new head after the review addition:
type-check,lintand the package's vitest suite, all reported below at72bd9df02. The remaining rows were measured at0cd066a7c, whose only difference from this head is six lines of comment in one file.pnpm exec vitest run packages/plugin-chatbot/Test Files 23 passed (23)/Tests 326 passed (326)--reporter=verbosepackages/plugin-chatbot/src/__tests__/useObjectChat.transportIdentity.test.tsxnamed on all 5 lines,Tests 5 passed (5)Test Files 7 passed (7)/Tests 75 passed (75)pnpm --filter @object-ui/plugin-chatbot type-checktsc --noEmit && tsc -p tsconfig.test.jsoncleanpnpm --filter @object-ui/plugin-chatbot lint86 problems (0 errors, 86 warnings)node scripts/check-control-bytes.mjscheck-control-bytes: OK (scanned 4655 tracked text file(s); skipped 85 binary)node scripts/check-changeset-presence.mjs2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjsNo changeset declares a major bump.node scripts/check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.node scripts/check-type-check-coverage.mjstype-check coverage: 45/46 ... 41/41 packages compile their testsnode scripts/check-lint-coverage.mjslint coverage: 46/46 packages linted, 0 with outstanding errorsnode scripts/check-phantom-dependencies.mjsEvery in-scope import is declared by the package that publishes it.node scripts/check-package-self-import.mjsNo package names itself inside its own src/.node scripts/check-spec-symbol-derivation.mjs1289 files scanned against 4912 spec export namesnode scripts/check-i18n-call-site-keys.mjsEvery in-scope call-site key resolves against the en pack (2918 keys)node scripts/check-i18n-en-drift.mjsNo en value changed in this range.node scripts/check-skills-paths.mjs95/96 stated path(s) resolve across 18 guide file(s); 1 baselinednode scripts/check-node-esm-load.mjs --specifiers-onlyno un-ledgered package emits an extensionless relative specifierDerived from the workflows against this diff rather than from a handed-down list;
check-changeset-fixed,check-phantom-dependencies,check-package-self-import,check-node-esm-load --specifiers-only,check-spec-symbol-derivation, both i18n gates andcheck-skills-pathsare additions the dispatch list did not name.The 86 lint warnings are the package's pre-existing count plus two of a class this very file already emits four times:
react-hooks/refs"Cannot access refs during render" atuseObjectChat.ts:460and:462, the same warning the sibling idiommodelRef.current = modeldraws at:423. Left unsuppressed for consistency with the refs beside them, and lint still exits 0.eslint . --format jsoninside the package: 55 files, 0 errors, 86 warnings; the new test file contributes 0 of each.Narrowing declared. Two repo-wide runs were left to CI.
pnpm lintisturbo run lint, which fans the sameeslint .out to each of 46 packages; I ran the one package whose files changed, and the flat config extendstseslint.configs.recommendedwith noprojectorprojectService, so type-aware linting is off and this diff cannot move the verdict on a file it does not contain.pnpm testis sharded repo-wide in CI; I ran the changed package in full plus every test file outside it that namesplugin-chatbotoruseObjectChat.check-eager-closure-budgetexits 2 here for a reason unrelated to this change — it readsapps/console/dist/eager-closure.json, written by a consolevite build, and reports itself asa broken gauge, not a passing budgetwhen absent; it belongs to the performance-budget job. The doc-snippet and doc-component gates were not run: no exported signature, option or type changed, so no documented snippet can compile differently.One changeset, patch,
@object-ui/plugin-chatbot. No release notes touched.Generated by Claude Code