⏯️ feat: Resume Streams and Send AI SDK Messages Through the Chat Facade - #16609
Conversation
berry-13
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 094097c080
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * `resumeStream`). `useResumeOnLoad` answers each increase with the same status re-check it runs | ||
| * when a job is announced, so a request never builds a second resume path. | ||
| */ | ||
| export const resumeRequestFamily = atomFamily((_conversationId: string) => atom<number>(0)); |
There was a problem hiding this comment.
This atom is written by the chat facade and read only by the chat resume path, so placing it under the app-global ~/store makes feature-owned state part of the shell and preserves the coupling that prevents the chat feature from moving to its own workspace. Define the signal within the chat feature, or pass it through a host-supplied context, rather than adding it to the global store.
AGENTS.md reference: AGENTS.md:L161-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
codegraph-librechat
Bot
added
the
🗺️ Chat State Mgmt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 592cdaeb3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!id || id === Constants.NEW_CONVO) { | ||
| return; | ||
| } | ||
| jotaiStore.set(resumeRequestAtom, ({ count }) => ({ conversationId: id, count: count + 1 })); |
There was a problem hiding this comment.
When two multi-conversation panes call resumeStream() before React flushes effects—for example via Promise.all or consecutive awaited calls—both writes update the count, but this single slot retains only the second conversationId. The first pane then observes the new count, marks it answered, rejects it because the conversation ID belongs to the other pane, and never rechecks its stream status even though its promise resolved. Store pending counts per conversation or otherwise queue requests so one pane cannot overwrite another's request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ffc3634: pending resume requests are now a set of conversation ids. The facade adds its chat, and the pane showing that conversation consumes only its own entry, so a second pane's request no longer overwrites the first. transport.spec covers one pane answering while another pane's request stays pending; reviewctl verify passed on that head.
ffc3634 to
968d601
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 968d6010c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jotaiStore.set(resumeRequestsAtom, (pending) => | ||
| pending.has(id) ? pending : new Set(pending).add(id), | ||
| ); |
There was a problem hiding this comment.
When comparison mode displays the same conversation in two panes, this conversation-only key makes both mounted useResumeOnLoad instances observe the request before either passive effect removes it, so both pane indices can build submissions and open duplicate resumable SSE connections even though only one facade called resumeStream(). Fresh evidence beyond the earlier overwritten-request report is useSteerRecovery.spec.tsx:610-620, which explicitly establishes that comparison mode can hold the same conversation in two panes; include the pane/run index in the request identity or otherwise route the request to its originating pane.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not applicable on f9e181f: useResumeOnLoad has one mount site (ChatView.tsx:125), and ChatRoute renders a single ChatView with index hard-coded to 0 (ChatRoute.tsx:63). The comparison pane is the added response, which never mounts the resume path, so only one pane can consume a request for a conversation.
| ...(conversationId !== undefined && { conversationId }), | ||
| ...(parentMessageId !== undefined && { parentMessageId }), |
There was a problem hiding this comment.
When an AI SDK-shaped message carries metadata.parentMessageId: null—the value emitted for a stored root message—this forwards the null even though useChatFunctions.ask treats parentMessageId == null as “unspecified” and replaces it with the currently viewed tail (useChatFunctions.ts:561-563). Consequently, resending or constructing a message explicitly attached to the root instead appends it to the active branch, despite SendMessageInput documenting this metadata as where the turn attaches; translate an explicit null to the root sentinel or distinguish omission from null before invoking ask.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f9e181f: an explicit null parentMessageId now maps to the no-parent sentinel before ask, so a root-attached message stays at the root; facade.spec covers it. reviewctl verify passed on that head.
f9e181f to
a5ffcf7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ffcf7455
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!resumableEnabled || conversationId === Constants.NEW_CONVO) { | ||
| return; |
There was a problem hiding this comment.
When navigating from an Assistants conversation to a resumable agent conversation, ChatRoute keeps the old ChatView mounted while the new conversation loads, so resumeStream() is already keyed to the new route via messagesKey while resumableEnabled still reflects the old Assistants endpoint. This branch returns after the request was deleted above, and the request is not retried when the target conversation arrives, so a running generation remains detached. Retain the request until currentConversation.conversationId matches conversationId, or otherwise avoid consuming it for this transient mismatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0bb672c: a resume request now waits until the pane has loaded the conversation the route names, so a pane still holding the previous Assistants conversation no longer consumes it. transport.spec covers that switch (request held, then answered and reattached) and fails without the guard; reviewctl verify passed on that head.
| const { conversationId, parentMessageId } = message.metadata ?? {}; | ||
| return { | ||
| text: message.parts.map((part) => part.text).join(''), | ||
| ...(conversationId !== undefined && { conversationId }), |
There was a problem hiding this comment.
When an AI SDK-shaped message names a conversationId different from the facade's current chat, forwarding it here makes ask load that target conversation's messages but still construct the request from the facade's active immutableConversation (useChatFunctions.ts:384,409,610-639). The turn can therefore use the wrong endpoint/model/agent settings, and when parentMessageId is omitted it also derives the parent from the current chat's latestMessage rather than the target history. Either require this metadata ID to equal the facade ID or resolve the complete target conversation before sending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0bb672c: the AI SDK input's metadata now carries only parentMessageId, so a sendMessage call goes to the facade's own chat, as AI SDK useChat does. TAskProps callers are unchanged.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Message metadata handling and unmounted-pane resume behavior contradict the documented contract.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds AI SDK-compatible sending and resumable streams to the client chat facade.
Changes:
- Maps text-part messages into LibreChat ask requests.
- Adds pane-mediated
resumeStream. - Adds facade and transport integration tests.
| File | Description |
|---|---|
client/src/hooks/Chat/facade.ts |
Extends facade actions and message mapping. |
client/src/hooks/Chat/resume.ts |
Stores pending resume requests. |
client/src/hooks/SSE/useResumeOnLoad.ts |
Consumes explicit resume requests. |
client/src/hooks/Chat/__tests__/facade.spec.tsx |
Tests facade actions. |
client/src/hooks/Chat/__tests__/transport.spec.tsx |
Tests resume transport behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| export type SendMessageInput = { | ||
| parts: UITextPart[]; | ||
| metadata?: Partial<Pick<UIMessageMetadata, 'parentMessageId'>>; |
There was a problem hiding this comment.
Not applicable on a1b9dd6: the facade sends to its own chat, as AI SDK useChat does. A cross-chat conversationId would make ask load one conversation while building the request from another (the earlier finding on this PR), so it was removed on purpose; the PR body's stale sentence is corrected.
| jotaiStore.set(resumeRequestsAtom, (pending) => | ||
| pending.has(id) ? pending : new Set(pending).add(id), | ||
| ); |
There was a problem hiding this comment.
Accepted on a1b9dd6: a request for a conversation no pane shows stays pending, and opening that conversation runs the same status check, so answering it changes nothing. Entries exist only for chats a host asked to resume. The PR body now says this instead of claiming a no-op.
| const store = getDefaultStore(); | ||
| store.set(resumeRequestsAtom, new Set(['convo-2'])); | ||
| const { result } = renderChat(createContract()); | ||
|
|
||
| await result.current.resumeStream(); | ||
| await result.current.resumeStream(); | ||
|
|
||
| expect([...store.get(resumeRequestsAtom)]).toEqual(['convo-2', 'convo-1']); |
There was a problem hiding this comment.
Fixed in a1b9dd6: both resume tests render under IsolatedAtomStore and assert against that store.
useChat and useChatActions gain resumeStream, which asks the pane's resume-on-load path to re-check the stream status and reattach through the host transport, and sendMessage now also takes an AI SDK user message of text parts, sent as the ask call it describes. The facade spec covers the new members and the refused-send and failed-stop paths; the transport spec resumes a running generation against a fake transport, stays detached when nothing runs, and reports a reattached stream that fails.
The resume-request atom is written by the facade and read by the chat's resume path, so it moves from the app store to hooks/Chat.
A request only matters to the pane showing its conversation, so a single atom with the conversation id and a count replaces the per-conversation family, which kept an atom for every conversation a session ever asked to resume.
A single request slot let a second pane's resumeStream overwrite the first before either pane's effect ran, so the first request was dropped. Requests are now a set of conversation ids: the facade adds its chat, and the pane showing that conversation consumes the entry and answers it.
A root message's view carries a null parentMessageId, which ask reads as unspecified and replaces with the branch tail. sendMessage now passes the no-parent sentinel for an explicit null, so the turn attaches where its metadata says.
…the Facade's Chat A pane navigating away from an Assistants conversation still reads that endpoint while the route already names the next one, so it consumed a resume request it could not answer. The request now waits until the pane has loaded the route's conversation. sendMessage's AI SDK input also stops accepting a conversationId: the turn goes to the facade's own chat.
0bb672c to
8da1638
Compare
The resume-request tests wrote to jotai's module-global default store and left their entries for later tests; they now render under IsolatedAtomStore and read that store.
Pull Request
Summary
The chat facade (
useChat,useChatActions) covered seven of the eight AI SDKuseChatmembers but had noresumeStream, and itssendMessageonly took LibreChat'saskarguments. This covers chat audit criterion C6.resumeStreamasks the pane to reattach to its running generation. It does not open a second resume path: it bumps a per-conversation request thatuseResumeOnLoadanswers the way it answers an announced job. It re-reads the stream status and builds the resume submission, whichuseResumableSSEattaches through the host transport from #16592. It does nothing for a new chat or one already attached, and waits while the pane is still loading the conversation the route names. A request for a conversation no chat view shows stays pending until one does; opening it runs the same status check either way.sendMessagenow also takes an AI SDK user message of text parts (with optionalparentMessageIdmetadata;nullattaches at the root) and sends it as theaskcall it describes. Like AI SDKuseChat, it goes to the facade's own chat, so it names no conversation.{ text }was already valid for both. File parts are left out of the type because a turn takes its files from the composer.TAskFunctioncallers are unchanged.Type of change
Testing
Tested environments/configuration:
lcdev servers on this stack, Chromium (Playwright, headless), an agent on Anthropicclaude-sonnet-4-6: send, regenerate, stop (abort 200), steer, queue, and reload mid-stream (reattached withresume=trueand finished). No page errors.Automated tests:
facade.spec.tsx: AI SDK message toask, a refused send returnsfalse, a failed stop rejects, andresumeStreamrequests a resume for the chat and none for a new chat (39 tests).transport.spec.tsx: with realuseChatHelpers,useResumeOnLoad,useResumableSSEanduseChatover a fake transport,resumeStreamreattaches to a running generation, re-reads the status and stays detached when nothing runs, and reports a reattached stream that fails asstatus: 'error'with its message (16 tests). These fail without theuseResumeOnLoadchange.npx jest --findRelatedTestson the changed files: 492 of 493 suites pass. The one failure,DeploymentTheme.spec.tsx("resolves a bundled theme name without touching stored preferences"), fails the same way on canary.npx tsc --noEmit -p client/tsconfig.json: clean.npm run static-checks -- --against origin/canary: passed.Screenshots / recordings
No user-facing change.
Risk / compatibility
sendMessageis no longer theaskfunction itself but a stable wrapper over it (same dependencies), so memoized consumers see the same identity stability.resumeStreamresolves when the request is made, not when the stream ends.Checklist