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 👍 / 👎.
| 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 👍 / 👎.
| 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 👍 / 👎.
| 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 👍 / 👎.
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.
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".
| 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 a caller passes a valid UIMessage with role: 'system' to setMessages, this conversion silently stores it with isCreatedByUser: false, because fromUIMessage distinguishes only role === 'user'; subsequent history therefore treats the system instruction as an assistant response. Either exclude system messages from this facade's accepted type or preserve/reject that role explicitly rather than changing its semantics.
Useful? React with 👍 / 👎.
| 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 👍 / 👎.
This branch has not been deployed
1 participant
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