Repository navigation
🐛 fix(client): keep each send and run on the chat it started in - #89
Conversation
handleSendMessage awaits attachment processing, which can take a long time,
before calling forwardedPropsProvider with no arguments. A provider that
resolves 'the current chat' at call time then sees whichever chat the user
switched to in the meantime, so per-chat props land on the wrong conversation.
The provider now receives { chatId }, captured when the send starts. Zero-arg
providers keep working.
…sagree If the user switches chats while attachments are still processing, forwardedPropsProvider gets the original chat id. But sendPrompt reads this.threadId when the send finishes, so the run goes to the new chat. This test fails on purpose. It shows that the request holds two different chat ids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I wasn't too sure about the design, so I investigated a little. Basically, I'm not a big fan of there being two conflicting sources of truth, one which could be wrong in certain cases. It's a footgun for a client of the library. Digging into it, I think this fix doesn't quite work anyway, I added a test illustrating the error case. I'm wondering, could we just block chat switching while attachment is uploading? This seems the simplest, and likely the user doesn't need to switch chats while uploading attachments anyway. |
|
@mm-zacharydavison
I'll take some time to think about it. |
handleSendMessage awaits attachment processing, the user-message save and forwardedPropsProvider before dispatching the run, and the run then read the client's live thread id, message buffer and selected agent. A chat switch in that window sent the message on the other chat's thread, without its own history, with the agent selected later, and showed it in the other chat. The send now captures its chat id, history and agent before the first await and passes them to sendPrompt as a RunTarget. forwardedPropsProvider and the attachment pipeline receive that chat, loaded once by id. saveUserMessage only appends to the displayed list when its chat is displayed, and activatePendingChat always reloads the chat's messages into the client, so a run that finished after a switch does not leak into the next chat's send.
The agent of a run came from the global selection at send time, so opening an
older chat sent it to whatever agent was selected now. Apps that needed a
stable route per conversation had to infer it from the conversation text.
Chat and CreateChatOptions gain an optional agent field, persisted by
LocalStorageChatRepository. sendMessage({ newChat: true, agent }) creates a
bound chat, and the blank-chat reuse check compares the agent. A send in a
bound chat uses that agent; unbound chats keep following the global
selection, and message-level forwardedProps.agent still wins.
KakkoiDev
changed the title
setThreadId and clearConversation cleared the client's tool-call and assistant assembly state on every chat switch, including while a run was streaming. A TOOL_CALL_END that arrived after the switch found no tool call, so the client tool never ran, no tool_result was sent, and the server waited for it until the run was aborted. The run's turn messages were also pushed into the buffer of the chat the user switched to. While a run is in flight, a thread change now keeps that run's assembly state and moves its message buffer aside; only the visible thread's buffer is reset. The run appends its turn to its own buffer, exposed as client.runMessages, and useServerEvents extracts the persisted turn from it. Idle switches reset state as before.
KakkoiDev
changed the title
An agent returned by forwardedPropsProvider is merged into the run's forwarded props like a message-level one, so it overrides the chat's bound agent too. The README only named the message-level override.
The test asserts that a bound chat with history keeps its agent after the global selection changes. Its name and fixture text described a text-based routing mechanism the library does not have.
|
@KakkoiDev let me know when this is ready for review and what the conclusion was after: |
…le is sent from chat.updateMetadata takes an optional chatId. Without it the write still goes to the chat on screen. With attachments, forwardedPropsProvider's ctx.chat is reloaded by the send's chat id after they are processed, so it sees metadata written by transformers during the same send.
KakkoiDev
changed the title
|
@mm-zacharydavison On the conclusion: I didn't block chat switching. I had several issues with multiple chats in one window.
Your test is unchanged and passes. Every new test fails when its fix is reverted. |
|
@masudahiroto can you review this? |
mm-zacharydavison
left a comment
mm-zacharydavison
left a comment
There was a problem hiding this comment.
Looks like your AI has overfit a little bit on the frozen metadata request and decided it needs to mention it all over the place. Please check comments and simplify.
| * Implementations can store chats locally (localStorage, IndexedDB) | ||
| * or remotely (REST API, GraphQL, etc.) | ||
| * | ||
| * Implementations must persist every {@link Chat} field, including `agent`. A chat whose |
There was a problem hiding this comment.
It seems redundant to say that agent must be saved, if it is required to save every field.
| const updateMetadata = useCallback(async (metadata: ChatMetadata, overwrite = false): Promise<void> => { | ||
| /** | ||
| * Gets the current chat object (including metadata). | ||
| * Metadata is frozen to prevent accidental mutation. |
There was a problem hiding this comment.
I don't think the comment should say anything about metadata being frozen. If the implementation wants to prevent mutation, it should freeze or copy.
| // Load existing messages into client for conversation history | ||
| if (clientRef.current && messages.length > 0) { | ||
| // Load the chat's messages into the client for conversation history, even when there are | ||
| // none: the client buffer may still hold a run that finished after the user switched away. |
There was a problem hiding this comment.
what does none mean? note typo?
Also, what is the impact of this happening, if it happens?
There was a problem hiding this comment.
🏁 Updated wording to:
Load the chat's messages into the client for conversation history, also when the chat has no messages, so the client's history is always the activated chat's.
clearConversation keeps an in-flight run's tool-call and assistant state and moves its buffer to runMessages, like setThreadId. No test covered it: with the in-flight branch removed, every client test still passed. The new test fails on both transports without that branch.
The comment said the client buffer may still hold a run that finished after a switch. Since runs for another thread write to runMessages, it cannot; the empty load keeps the client's history equal to the activated chat's.
KakkoiDev
changed the title
🧭 Review walkthrough / レビューガイドGenerated for
Action items / 対応事項
|
…is sent from The provider kept one file processing state for all chats, so switching chats while a send-time transformer ran showed its progress in the other chat. It is now passed to the UI only while the displayed chat is the one the send started in, like the streaming answer.
…at the run belongs to The provider-wide loading flag drew the "..." bubble and the "Thinking..." placeholder in every chat while one chat's run was in flight. They now follow the run's chat, like the streaming answer. Sending stays blocked in every chat while a run is loading.
…nking (#91) * test(example): record a multi-chat walkthrough with a visible cursor * chore(example): add a launcher that records the multi-chat e2e video * test(example): detect an in-flight run by its stop button in the multi-chat walkthrough A provider that delivers the answer in one burst renders the streaming message for less than a frame, so waiting for it timed out after the reply had arrived. Longer answers keep each run in flight across the chat switch, and the stop button is checked again after the switch. * chore(example): accept the AI gateway key for e2e runs The example server runs its Claude agent through the gateway when AI_GATEWAY_API_KEY is set, so ANTHROPIC_API_KEY is not required then. * chore(example): add a Playwright config that runs e2e tests against the mock agent Blanks every provider key and loads no .env file, so no run with it can make a paid call. * test(example): show the multi-chat checks on screen and switch chats while a PDF is processed The walkthrough moves to the File Transformers page, whose PDF transformer delays the send by about 4 seconds; switching chats in that window put the message in the other chat before the chat fixes. Each check is captioned and the walkthrough continues past a failed one, so a run without the fixes records where it goes wrong. The test still fails at the end if any check failed. * chore(example): record the multi-chat walkthrough before and after the branch's changes The launcher extracts the merge-base with origin/main (git archive, no worktree), builds it once, runs the same spec against it and against the checkout, labels both videos, joins them into one mp4 and prints the failed checks of each. MOCK=1 records both with the mock agent at no cost. * test(example): check that chat B shows no file processing indicator while chat A's PDF is processed * test(example): check that chat B shows no pending bubble or "Thinking..." while chat A runs * test(example): sample the pending bubble check once, with the run's in-flight state A failed check holds its caption for 2.5 seconds, so a separate in-flight check after it failed on the base build because the run had ended meanwhile. * fix(server): derive the mock agent's reasoning and reply from the prompt The mock agent answered every prompt with the same text, and with "long reasoning" the same factorial of 15. A demo or test on two chats then showed that calculation in both, for prompts that never asked for it, and the reply could not tell which prompt or which chat it answered. The reasoning now quotes the last user message, and the reply follows its request: a count, a single word, or the last word of the previous reply in the prompt. Any other prompt gets a reply that quotes it. createMockReasoningModel takes chunkDelayInMs for fast tests. * test(client): check that a run's thinking and answer show only in its own chat * test(example): check each chat's thinking in the multi-chat walkthrough The walkthrough no longer appends "Use long reasoning." with the mock agent and no longer skips the reply checks there: the mock now answers the prompt it gets. New checks: chat B shows no thinking or answer from chat A's runs while they stream, and each chat's thinking is about its own questions. With the mock agent, BEFORE runs today's mock too, so only the client differs.
KakkoiDev
left a comment
KakkoiDev
left a comment
There was a problem hiding this comment.
Review: keep each send and run on the chat it started in
Reviewed at head 755b334a1135626b90d1357d4497b8e0d7974970. Unit tests at this head: packages/client 574 pass, 0 fail; packages/server 370 pass, 1 skip, 0 fail (bun run test, after building packages/core and packages/client). I did not run the e2e suites.
The latest commit (755b334) changes no client code: it adds a multi-chat e2e walkthrough (apps/example/test/multi-chat.e2e.test.ts), a script that records it before and after the fix, a Playwright config for the server's mock agent, a mock model that answers the prompt it gets, and one more provider test for the thinking and the streamed answer.
What this PR does: handleSendMessage awaits attachment processing, the user-message save and forwardedPropsProvider before it dispatches the run, and the run used to read the client's live thread id, history and agent. A chat switch in that window sent the message on the other chat. The PR captures a RunTarget (thread id, history copy, agent) before the first await and passes it to sendPrompt. A run in flight keeps its own buffer (_runMessages) when the thread changes, so its tool calls still run and its turn is not written into the new chat. Chats can be bound to an agent (Chat.agent). The last two commits show the file progress, the pending bubble and "Thinking..." only in the run's chat; sending stays blocked in every chat while any run is in flight.
Where to look first: useAIProvider.tsx (the capture before the first await, loadingInCurrentChat), then client.ts (sendPrompt with a target, setThreadId and clearConversation during a run, the runMessages getter).
One send, before and after
Before:
flowchart TD
UI["chat UI send"] --> Send["handleSendMessage"]
Send --> Await["awaits: attachments, save user message, forwardedPropsProvider"]
Switch["chat switch during the awaits"] -.-> Live
Await --> Prompt["client.sendPrompt"]
Live["live client threadId, messages, selectedAgent"] --> Prompt
Prompt --> Server["server run"]
Server --> Events["useServerEvents: tool calls, persist turn"]
Events --> Buf["client._messages"]
After:
flowchart TD
UI["chat UI send"] --> Send["handleSendMessage"]
Send --> Target["RunTarget: threadId, messages copy, agent"]
Repo[("ChatRepository: Chat.agent")] --> Target
Target --> Await["awaits: attachments, save user message, forwardedPropsProvider with chatId and chat"]
Await --> Prompt["client.sendPrompt with target"]
Prompt --> Server["server run"]
Server --> Events["useServerEvents: tool calls, persist turn"]
Events --> RunBuf["client.runMessages"]
Switch["setThreadId or clearConversation during a run"] --> RunBuf
Events --> Ind["chat panel: pending bubble, Thinking, file progress only in the run's chat"]
classDef added fill:#dafbe1,stroke:#1a7f37,color:#000
classDef changed fill:#fff8c5,stroke:#9a6700,color:#000
class Target,Repo,RunBuf added
class Send,Await,Prompt,Events,Switch,Ind changed
| Step of a send | Before | After |
|---|---|---|
| Thread id of the run | live client.threadId at dispatch |
captured before the first await |
| History of the run | live client.messages at dispatch |
copy captured before the first await |
| Agent of the run | global selection at dispatch | chat's bound agent, else global selection at capture; forwardedProps.agent still wins |
forwardedPropsProvider argument |
none | { chatId, chat } of the captured chat |
| Chat switch while a run streams | tool calls and turn state cleared | run keeps its own buffer and tool calls |
| Pending bubble, "Thinking...", file progress in another chat | shown in every chat | shown only in the run's chat |
Findings
High: 0. Medium: 0. Low: 3.
- Low, UX (
packages/client/src/components/UseAIChatPanel.tsx:322,:325): the placeholder now followsloadingInCurrentChat, butcanSendstill followsloading. In a chat with no run of its own, while another chat's run is in flight, the composer shows the idle placeholder and the send button is disabled, with nothing on screen saying why. Suggest a placeholder or hint for "a reply is in progress in another chat", or document the global block. - Low, compatibility (
packages/client/src/providers/chatRepository/types.ts:152):Chat.agentis optional andLocalStorageChatRepositorystores it. A customChatRepositorythat copies known fields only drops it, and a bound chat silently falls back to the global agent. The PR body says this, and the code comment that said it was dropped inc8fc5b4at review request, so the release notes are the one place left to tell custom-repository authors. - Low, test suite (
apps/example/test/multi-chat.e2e.test.ts:36): the walkthrough sits inapps/example/test, so the defaultplaywright.config.tsruns it in everytest:e2erun, against the real model. It forcesvideo: 'on', which overrides the config'sretain-on-failure, and its first prompt asks for 300 numbers. Suggest turning video on only when the record script runs (it already setsMULTI_CHAT_LABEL), so a normal e2e run keeps the regression test without the recording.
Nit: apps/example/playwright.config.ts:6 now accepts AI_GATEWAY_API_KEY or ANTHROPIC_API_KEY, but the hint at :9 still names only ANTHROPIC_API_KEY.
Checked, no finding
- Switch away from a chat and back while its run streams, then send:
saveAIResponsesaves tocurrentChatId(still the run's chat, since activating another chat needs a send, which is blocked) and appends the turn to the displayed list when that chat is shown.activatePendingChatthen loads that list into the client beforeRunTargetis captured, so the next send carries the run's turn. forwardedPropsProvidersignature change: a provider written as() => propsstill type-checks.ForwardedPropsProviderContextandRunTargetare exported fromindex.ts._runMessagesis reset only by the nextsendPrompt, so after a cross-thread run ends,runMessagesreturns that run's buffer until the next send. Its two readers inuseServerEventsread it only during a run, and the getter's doc says "latest run", so this matches.- Mock model rewrite: the existing mock-agent e2e test (
extended-thinking.e2e.test.ts) checks only that reasoning shows and is stored with a signature, not the old fixed texts, and no test refers to the removed factorial reply. The walkthrough's thinking-quotes-the-prompt checks run only withMULTI_CHAT_MOCK_AGENT, so the real-model run does not depend on the mock's wording. - The three earlier inline threads (agent-persistence wording on
types.ts, frozen-metadata wording and thenonetypo onuseChatManagement.ts) are addressed byc8fc5b4andc090646. The "Metadata is frozen" lines still in the code predate this PR, andgetChatdoesObject.freezethe metadata.
Before you approve
- Author: decide what a chat without its own run shows while another chat's run blocks sending (finding 1).
- Author: note in the release notes that a custom
ChatRepositorymust storeagentfor bound chats to keep their agent (finding 2). - Author: decide whether the walkthrough records video in every e2e run (finding 3).
| : loadingInCurrentChat | ||
| ? `${executingTool?.displayText ?? strings.input.thinking}...` | ||
| : strings.input.placeholder; | ||
| const canSend = !!( |
There was a problem hiding this comment.
Low, UX: the placeholder now follows loadingInCurrentChat, but canSend below still follows loading. In a chat with no run of its own, while another chat's run is in flight, the composer shows the idle placeholder and the send button is disabled, with nothing on screen saying why. A placeholder such as "A reply is in progress in another chat" when loading && !loadingInCurrentChat would explain the block; or document that sending is blocked globally.
| * Agent this chat is bound to. Every message sent in the chat goes to this agent, | ||
| * whatever the global agent selection is. When unset, the chat follows the global selection. | ||
| */ | ||
| agent?: string; |
There was a problem hiding this comment.
Low, compatibility: a custom ChatRepository that copies known fields only drops agent, and a bound chat then silently falls back to the global agent. The code comment that said so was removed in review, so a line in the release notes is the remaining place to tell custom-repository authors.
|
|
||
| test.use({ | ||
| viewport: { width: 1280, height: 720 }, | ||
| video: { mode: 'on', size: { width: 1280, height: 720 } }, |
There was a problem hiding this comment.
Low, test suite: this file sits in apps/example/test, so the default playwright.config.ts runs it in every test:e2e run, against the real model. video: 'on' here overrides the config's retain-on-failure, so every run records it. Suggest turning video on only when the record script runs (it already sets MULTI_CHAT_LABEL), so a normal e2e run keeps the regression test without the recording.
|
@mm-zacharydavison |
…the forwardedPropsProvider paragraph
🧭 Review walkthrough / レビューガイドOpen the review page / レビューページを開く (generated for
@mm-zacharydavison generated this page with the |
A chat switch while a send or run was in progress mixed two chats.
handleSendMessageawaits attachment processing, the user-message save andforwardedPropsProviderbefore it dispatches the run, and the run read the client's live thread id, history and agent. A switch in that window sent the message on the other chat's thread, without its own history, with the agent picked later, and showed it in the other chat. A switch while a run streamed dropped its pending tool calls, so the server waited until the run was aborted.✅ What this PR does
forwardedPropsProvider({ chatId, chat })and the attachment pipeline all use that capture, so the run'sthreadIdand thechatIdgiven to the provider cannot disagree.chat.create({ agent })orsendMessage(text, { newChat: true, agent }). Persisted byLocalStorageChatRepository. Unbound chats follow the global selection; anagentin forwarded props overrides both.setThreadId/clearConversation). Its tools still run and send results; an abort records{ aborted: true }in the run's own chat.chat.updateMetadata(metadata, overwrite, chatId): a file transformer passescontext.chat.idso its write lands on the chat the file is sent from.UseAIChatPanelgains an optionalloadingInCurrentChatprop (defaults toloading).✔️ Already correct, no change needed
API: additive only.
forwardedPropsProvidergains a context argument (providers without arguments still work);sendPromptgains an optionaltarget;chat.updateMetadatagains an optionalchatId; new optionalChat.agent,CreateChatOptions.agent,SendMessageOptions.agent; new typesForwardedPropsProviderContext,RunTarget; new getterUseAIClient.runMessages; new optionalUseAIChatPanelProps.loadingInCurrentChat. DirectUseAIClientusers read a run's turn fromclient.runMessagesafter a switch mid-run. A customChatRepositorymust persistagent, or bound chats fall back to the global selection.Tests:
bun run test(client 573, server 363, workflows 27 pass) andbun run buildpass. Each behavior test inuseAIProvider.forwardedPropsProvider.test.tsx,LocalStorageChatRepository.test.tsandclient.test.tsfails with its fix reverted; the review test from f09d558 is unchanged and passes.Before / After
The example app's File Transformers page, run against the server's mock agent (no model calls), at 1.7x speed.
pr89-multi-chat-before.mp4
pr89-multi-chat-after.mp4
Manual testing
Two chats, one run in flight: steps, expected vs leak
Two chats, one run in flight while you switch between them. Each step says what you should see and what a leak looks like.
Setup
bun install && bun run buildat the repo root.bun run kill(3000, 3002, 8081).AI_GATEWAY_API_KEY=...works instead ofANTHROPIC_API_KEY.)The user asked: "<your prompt>". Its reply follows the prompt: a count, a single word, or the last word of the chat's previous reply. If you see thinking about a factorial ("15! = ..."), the server is running an old build. Rebuild.cd apps/example && bun --no-env-file run dev, then open http://localhost:3000.localStorage.clear(), then reload.printf '%%PDF-1.4\nQuarterly report.' > /tmp/quarterly-report.pdf.Prompts:
Count from 1 to 300, one number per line. After the last number, write the single word ALPHA.Reply with the single word BRAVO and nothing else.Which word did you write at the end of your previous answer in this chat? Reply with that word only.Steps
BRAVO. Expand Thinking complete: it is about the BRAVO request (mock:The user asked: "Reply with the single word BRAVO...).ALPHAstreaming in B.BRAVO.ALPHA. Expand both thinking blocks: neither mentions BRAVO.JSON.parse(localStorage.getItem('use-ai:chat-index')).map(id => JSON.parse(localStorage.getItem('use-ai:chat:' + id)).messages.map(m => m.role))[user, assistant, user, assistant](A), one[user, assistant](B).The reported symptoms, and what they mean
ALPHAstream in B (steps 2, 3, 6, 7).Automated version
MOCK=1 apps/example/scripts/record-multi-chat.shruns the same walkthrough against the code before and after the branch. It records both runs and prints every check. WithoutMOCK=1it needsENV_FILE=...with a key, and it makes paid calls.