🔌 refactor: Drive useSSE Through the ChatTransport - #16336
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. |
a725610 to
19c7ba7
Compare
![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: 19c7ba7c77
ℹ️ 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 (signal.aborted) { | ||
| return; |
There was a problem hiding this comment.
When an Assistants request receives a 401 and the user presses Stop while refreshToken() is pending, the underlying SSE is already closed, so the abort listener does not emit abort; this early return then suppresses the refreshed/error path as well. The Stop path only clears submissions, while handleAbort/handleError are responsible for resetting isSubmitting and the stop button, leaving the chat permanently shown as generating until another reset or reload. Ensure aborting this refresh still settles the caller's UI state without reopening the stream.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 861e222. The transport now treats a pending 401 refresh as open, so an abort during it dispatches cancel and useSSE's abort path settles the submitting state; the retry still never re-streams. Covered by sse.spec.ts 'cancels, and stays closed, when aborted while a 401 refresh succeeds/fails'.
Move the sse.js connection, frame parsing, the named attachment and error events, the 401 refresh-and-retry and the cancel-on-close signal into client/src/hooks/SSE/transport. normalizeFrame tags each message frame by the keys the hooks already checked, in the same order, and createSSETransport emits the resulting ChatEvent union. useSSE now switches on event.type and calls the same handlers with the same arguments; its signature is unchanged. A frame that is not JSON is now logged and skipped. Previously the parse error escaped the listener and dropped every frame behind it in the same chunk. createdHandler never read its first argument, so it is typed as the created frame it actually receives instead of TResData.
A refresh that resolved after the caller aborted called sse.stream() again, reopening the request with the unmounted consumer's handlers; a refresh that failed after abort dispatched the error path. Both now return once the signal is aborted.
A retried connection that drew another 401 refreshed and reconnected again with no bound, so a server rejecting the refreshed token looped. The second 401 now reaches the error path.
The content handler never read userMessageId, which Assistants content frames do not send, so it now takes ChatContentFrame.
The handlers accept narrower payload types than the wire frames now admit. useSSE narrows at the call, where these frames already reached the handlers unchanged before the transport existed.
sse.js marks the connection closed on the 401, so an abort during the refresh skipped the cancel path and nothing reset the submitting state. The stream now counts as open until the refresh settles.
861e222 to
c3efa04
Compare
1 participant
Summary
useSSEparsed frames, retried a 401 and synthesized cancel in the same closure that ran the chat handlers, so the wire could not be tested apart from React state. This moves it intoclient/src/hooks/SSE/transport/, stacked on #16335.normalizeFrametags each frame by the keys the hooks already checked, in the same order, andcreateSSETransportemitsChatEvents, including attachments, errors, the 401 retry andabort.useSSEswitches onevent.typeand calls the same handlers with the same arguments; its signature is unchanged and the existing specs pass unmodified. One difference: a malformed frame is logged and skipped, where before its parse error dropped every frame behind it in the chunk.createdHandler, which never read its argument, is retyped fromTResDatato the created frame.useResumableSSEis next.Type of change
Testing
Tested environments/configuration:
lcdev servers on this branch (sharedlibrechat.yamlstarted withCONFIG_BYPASS_VALIDATION=true, since canary's schema rejects a key in it). No provider key in the dev environment is valid (OpenAI and Anthropic return 401), so no live streamed turn could run. An agents turn reached the provider and rendered the provider's 401 as the error card, which exercises the untouched resumable path and the retyped handlers. The Assistants endpoint, the only oneuseSSEserves, isuser_providedwith no key on the dev account, souseSSEitself is covered by specs only.Automated tests:
__tests__/frames.spec.ts(key order, side channels, unknown events as steps, legacy content and text frames, empty frames) and__tests__/sse.spec.ts(realsse.jsover a fake XHR: created, sync and final in order; named and in-band attachments; malformed JSON; server error event; non-JSON failure body; 401 refresh and retry; abort mid-stream; abort after close; already-aborted signal; abort during a 401 refresh; a second 401)cd client && npx jest hooks/SSE data-provider/SSE: 16 suites, 466 passed, including theuseResumableSSE.spec.tscases that driveuseSSEthrough final, cancel and errorcd client && npm run typecheck: cleannpx eslint,npx prettier --check,node scripts/sort-imports.mts --checkon every touched file: cleanScreenshots / recordings
No user-facing change
Risk / compatibility
The transport also refreshes the token at most once per stream (a second 401 now reaches the error path instead of looping), and stopping a turn while its refresh is pending now cancels it, where before the retry reopened the stream and sent the turn anyway. Both behaviors predate this change.
useSSEonly serves the Assistants endpoints. It now ignores steering and label side channels instead of passing them to the step handler, which only warned about them; only agent runs emit those, and agent runs stream throughuseResumableSSE. For the same reason, an in-band{ event: 'attachment' }frame now reaches the attachment handler on this path too, though only resumable streams send that shape.Checklist