Conversation
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: f845e796ef
ℹ️ 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".
| const hasStreamed = (message: TMessage) => | ||
| (message.content?.length ?? 0) > 0 || (message.text?.length ?? 0) > 0; |
There was a problem hiding this comment.
When an Assistants turn starts, useChatFunctions seeds the response with an empty text part, and dual-conversation turns seed type: '' lane placeholders. Testing only content.length therefore reports streaming before any model output arrives, so consumers skip the submitted loading state. Treat empty text/reasoning/tool placeholders as non-streamed content; the existing isEmptyContentPart helper already defines these cases.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b49de6e. status reads streamed output through isEmptyContentPart, so empty text/think parts and type '' lane placeholders keep the turn submitted. Covered by 'stays submitted while the response holds only placeholder parts'. The lane placeholder also exposed a crash in toUIMessage, fixed on the base in 16aa523.
| const current = getMessages() ?? []; | ||
| const next = typeof update === 'function' ? update(current.map(toUIMessage)) : update; | ||
| const byId = new Map(current.map((message) => [message.messageId, message])); | ||
| setStoredMessages(next.map((message) => fromUIMessage(message, byId.get(message.id)))); | ||
| }, |
There was a problem hiding this comment.
When an updater changes only a non-tail message, this cache write does not cause useChat to re-render. The provider's React Query subscriptions select only the active tail and its ID, which remain referentially unchanged in that scenario, while this facade reads the full cache imperatively and owns no subscription. As a result, the returned messages remains stale until some unrelated context update occurs, breaking common operations such as editing an earlier message through setMessages; the facade needs to subscribe to the message cache or otherwise schedule an update after the write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b49de6e. messages now reads getMessages through useSyncExternalStore, subscribed to message query cache events, so a setMessages write re-renders without an unrelated context change. No state is added. Covered by 're-reads messages when the message cache is written'.
| const part = message.content?.find((item) => item?.type === ContentTypes.ERROR); | ||
| return part?.type === ContentTypes.ERROR ? (part.error ?? '') : ''; |
There was a problem hiding this comment.
When an Assistants run fails, both Assistants error handlers append a part shaped like { type: ContentTypes.ERROR, text: { value: message } }, without an error property or populated top-level message text. This expression consequently returns an empty string, so the facade reports status: 'error' but exposes new Error('') instead of the server's failure message. Extract the error part's text/TextData value as well as its legacy error field.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b49de6e. The error text falls back to the error part's error field, then its text (string or TextData value). Covered by 'reads the error text of an Assistants error part'.
| const byId = new Map(current.map((message) => [message.messageId, message])); | ||
| setStoredMessages(next.map((message) => fromUIMessage(message, byId.get(message.id)))); |
There was a problem hiding this comment.
When setMessages is used to append a valid UIMessage that was not produced by this facade, its optional metadata will commonly omit LibreChat's conversationId and parentMessageId. Because there is no matching base, fromUIMessage then stores both fields as null, creating a disconnected root that branch selection and regenerate({ messageId }) cannot operate on. Populate these fields from the active conversation and neighboring message when converting newly inserted messages, or explicitly reject insertion rather than accepting an orphaned message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b49de6e. A message with no stored counterpart gets the active conversation's id and, unless its metadata names a parent, the previous message as parent. Covered by 'joins an inserted message to the conversation under the one before it'.
f845e79 to
6a8a16b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a8a16be2d
ℹ️ 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".
| for (const message of stored ?? []) { | ||
| views.push(toUIMessage(message)); |
There was a problem hiding this comment.
When this hook is mounted under the active ChatView, every streamed cache write produces a new stored array and causes this loop to reconvert the entire transcript; toUIMessage also traverses each message's content, files, and attachments. Long conversations therefore incur O(total history) render work and allocations for every streamed update, rather than remapping only the changed tail, which can visibly degrade token streaming. Cache conversions by TMessage reference or select the mapped messages incrementally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b49de6e. Views are cached in a WeakMap keyed by the stored TMessage, so a streamed cache write remaps only the messages it replaced. Covered by 'keeps unchanged message views across a streamed update', which asserts the untouched view is the same object.
useChat reads ChatContext and presents it as @ai-sdk/react@4.0.117 does: UI-shaped messages, a submitted/streaming/ready/error status derived from the in-flight flag and the latest message, and sendMessage, regenerate, stop and setMessages forwarding to the contract. It holds no state and changes no existing consumer.
The parts mapping reads only the markers stored on a tool call; the facade supplies getToolMeta as its resolveToolFailure, so memory failure prose and background task status attachments reach the UI parts view the same way they reach the tool cards.
…er message messages re-reads getMessages whenever the message query cache is written, through useSyncExternalStore, so setMessages and other cache writes show without an unrelated context update. Views are cached per message reference, so a streamed chunk remaps only the messages it replaced. status ignores placeholder parts through isEmptyContentPart, the error reads an Assistants error part's text, and a message inserted through setMessages joins the active conversation under the message before it.
6a8a16b to
b49de6e
Compare
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b49de6ee83
ℹ️ 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".
| let view = views.get(message); | ||
| if (!view) { | ||
| view = toUIMessage(message, mappingOptions); | ||
| views.set(message, view); |
There was a problem hiding this comment.
On the second and later content frame for a response, this returns the first cached UIMessage instead of remapping its updated content. Fresh evidence against the prior caching resolution is in client/src/hooks/SSE/useContentHandler.ts:55-82: messageMap deliberately reuses the same response object and mutates response.content for each frame, so message identity does not change as the comment assumes. Consequently, consumers of this facade can remain stuck on the first streamed chunk until the final server message replaces the object; cache against a content/version identity or make the stream producer immutable.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dc26613. Confirmed against useContentHandler: the response object is reused across frames and its content array replaced. The view cache now checks the fields a view reads (content, text, files, attachments, error, unfinished), not only the object, so a frame that replaces content remaps that response and unchanged messages keep their views. Covered by 'remaps a response whose content the stream replaced in place'.
| byToolCall = mapAttachments(message.attachments ?? []); | ||
| attachmentsByMessage.set(message, byToolCall); | ||
| } | ||
| const meta = getToolMeta({ type: ContentTypes.TOOL_CALL, tool_call: toolCall }, byToolCall); |
There was a problem hiding this comment.
When a message contains repeated provider tool-call IDs and the newer call has not received a stepId yet, calling getToolMeta without its sibling-step set lets filterAttachmentsForPart associate an older step's attachment with the new call. This is the exact repeated-ID case handled by summarizeSpan in outcome.ts: an earlier memory-error artifact or failed/cancelled background-status attachment can make the newer call appear as output-error even though it is still running. Derive the owned sibling step IDs from the message content and pass them as getToolMeta's third argument.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dc26613. The resolver builds, per message snapshot, the step ids each provider tool-call id already owns, and passes them to getToolMeta as siblingStepIds for a call without a stepId, the way summarizeSpan does. Covered by 'keeps a stepless call clear of attachments its repeated id owns elsewhere', which fails without the fix.
…cade The content handler reuses a response object across frames and replaces its content, so views are cached per message and per snapshot of the fields a view reads rather than by the object alone. The tool outcome resolver passes getToolMeta the step ids a repeated provider id already owns, so a call without a step is not failed by another step's attachment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc26613757
ℹ️ 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".
| const readMessages = useCallback(() => getMessages(), [getMessages]); | ||
| const stored = useSyncExternalStore(subscribe, readMessages, readMessages); |
There was a problem hiding this comment.
On the second and subsequent useContentHandler frame, client/src/hooks/SSE/useContentHandler.ts:79-97 replaces response.content on the same response object and writes an array containing the same message references. React Query's structural sharing can therefore retain the existing outer array, and although the subscription fires, readMessages() returns the identical snapshot, so useSyncExternalStore suppresses the render before isSameSource can inspect the changed content. Consumers remain on the first chunk until a later immutable or final write. Fresh evidence beyond the earlier view-cache fix is the unchanged outer snapshot identity at this external-store boundary; expose a per-write version/query state or make the stream write immutable.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2254d21. Confirmed: the content handler replaces content on the same response object, so structural sharing hands back the same cached array and the array-identity snapshot suppressed the render. The external-store snapshot is now keyed by the message cache write count (successful updates and removals of messages queries), so every frame re-renders and the per-field view cache remaps the response. Regression: 'follows each stream frame written to the cache with the same message references' in facade.spec.tsx runs a real QueryClient, asserts the cached array is retained, and fails on dc26613.
| if (cached && isSameSource(cached, message)) { | ||
| return cached.view; | ||
| } | ||
| const view = toUIMessage(message, mappingOptions); |
There was a problem hiding this comment.
When a live/provider message contains an empty tool-call slot such as { type: 'tool_call' } or a null payload, this unconditional conversion throws because toToolPart passes undefined to readToolCall, which immediately reads .type. These slots are recognized transient input—isEmptyContentPart explicitly classifies tool calls missing their payload, while server compaction removes them only before persistence—so mounting the facade during such a stream can crash instead of returning an empty/step placeholder. Guard or normalize malformed tool calls before mapping, including in getToolContext.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The facade's own getToolContext loop now tolerates a missing tool_call (2254d21). The throw you traced sits in the base mapping (toToolPart/readToolCall, plus the agentId read in toUIMessage), which belongs to #16374 and is fixed there at 6a3685f with a parts.spec regression mapping the empty slot to step-start. It reaches this branch when #16374 merges and this stack rebases.
danny-avila
added
the
🗺️ Chat State Mgmt
… in the facade The content handler replaces a response's content on the same object, so structural sharing keeps the cached message array and the external store saw no change. The snapshot is now keyed by the message cache write count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
This branch has not been deployed
2 participants
Summary
The chat context is already an explicit contract, but consumers still read it in LibreChat terms (
ask,isSubmitting,getMessages). This addsuseChatinclient/src/hooks/Chat/facade.ts, which presents the same contract the way@ai-sdk/react@4.0.117does. It sits on top of the parts mapping from the base PR.It returns
id,messages(getMessages()mapped throughtoUIMessage),status,error,sendMessage,regenerate,stopandsetMessages.statusissubmittedwhile a turn is in flight and its response has no content yet, thenstreaming. Once the turn ends it iserrorif the latest message failed andreadyotherwise, which includes a stopped turn.abortScrollonly holds the scroll position, so a stop is read from the settled message instead.sendMessageisaskitself,stopisstopGenerating,regenerate({ messageId })resolves the target the contract expects, andsetMessageswrites UI messages back onto the stored ones. The hook holds no state and reads no store, and no existing consumer changes.Depends on #16374, which this is stacked on.
Type of change
Testing
Tested environments/configuration:
Unit level only; no component uses the facade yet.
Automated tests:
client/src/hooks/Chat/__tests__/facade.spec.tsx: renders under the realChatContextwith a stubbed contract; status through submit, stream, finish, abort and error;sendMessageforwards toaskwith the same arguments;regenerate,stopandsetMessagesforwardcd client && npx jest hooks/Chat: 14 suites, 365 passedcd client && npm run typecheck: cleannpm run static-checks -- --against origin/canary: all affected checks passedScreenshots / recordings
No user-facing change.
Risk / compatibility
None.
messagesfollowsgetMessages(), so it is only as fresh as the host's re-render, the same as the contract it reads.Checklist