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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing-conversation navigation still drops the marker, and response-resolution races can strand queued follow-ups.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Fixes queued follow-ups not draining after leaving a conversation during generation.
Changes:
- Tracks detached and explicitly stopped runs.
- Reconstructs run completion from persisted history.
- Adds unit, hook, and end-to-end coverage.
| File | Description |
|---|---|
e2e/specs/mock/scenarios/queue-owners.spec.ts |
Tests draining after returning. |
client/src/hooks/SSE/useResumeOnLoad.ts |
Resolves detached run endings. |
client/src/hooks/SSE/useResumableSSE.ts |
Records detached active runs. |
client/src/hooks/SSE/__tests__/useResumeOnLoad.spec.tsx |
Tests run-end restoration. |
client/src/hooks/SSE/__tests__/useResumableSSE.spec.ts |
Tests detachment conditions. |
client/src/hooks/Chat/useChatHelpers.ts |
Records explicit Stop requests. |
client/src/hooks/Chat/queue.ts |
Adds detached-run state and resolution. |
client/src/hooks/Chat/__tests__/queue.spec.ts |
Tests response outcome resolution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const target = run.responseMessageId?.replace(/_+$/, ''); | ||
| const responses = (messages ?? []).filter( | ||
| (message) => message.isCreatedByUser === false && message.parentMessageId === run.userMessageId, | ||
| ); | ||
| const response = | ||
| responses.find((message) => message.messageId === target) ?? | ||
| (responses.length === 1 ? responses[0] : undefined); |
There was a problem hiding this comment.
Fixed in 16e5b7b: an explicit response id is matched exactly first, then without its trailing underscores; the single-reply fallback applies only when the run named no response (queue.spec covers an id ending in _).
| if (submission != null && detachCandidate != null) { | ||
| jotaiStore.set(detachedRunByConvoId(detachCandidate.conversationId), detachCandidate.run); | ||
| } |
There was a problem hiding this comment.
Fixed in 16e5b7b: the cleanup now records any run it closes while still generating, whatever cleared the submission (null from useNavigateToConvo, {} from a new chat, or an unmount); scenario parked-run-end-drains-after-switching-chats covers the sidebar switch.
| if (detachedRun != null) { | ||
| jotaiStore.set(detachedFamily, null); | ||
| const end = resolveDetachedRunEnd(conversationId, detachedRun, getMessages()); |
There was a problem hiding this comment.
Fixed in 16e5b7b: the marker is cleared only once its response is found; an unresolved run keeps it and gets one history refetch (useResumeOnLoad.spec: keeps the detached run until a history refetch shows its response).
![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: cb3d653edf
ℹ️ 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".
| * the queue drain; remember the run, and its end is read from history on return. */ | ||
| const detachCandidate = detachCandidateRef.current; | ||
| detachCandidateRef.current = null; | ||
| if (submission != null && detachCandidate != null) { |
There was a problem hiding this comment.
When the user leaves via the normal saved-conversation navigation, useNavigateToConvo clears the submission with setSubmission(null) (client/src/hooks/Conversations/useNavigateToConvo.tsx:238), rather than with the empty object used by the new-chat path. The cleanup has already captured the live run in detachCandidateRef, but this condition rejects and then discards that candidate, so returning after the run finishes cannot publish its end and the queued follow-up remains stuck. The new test only exercises navigation to /c/new, so it misses the standard chat-to-chat flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16e5b7b: switching to a saved chat (setSubmission(null)) now records the detached run; covered by parked-run-end-drains-after-switching-chats and the null-clear case in useResumableSSE.spec.
| jotaiStore.set(detachedFamily, null); | ||
| const end = resolveDetachedRunEnd(conversationId, detachedRun, getMessages()); |
There was a problem hiding this comment.
If the user returns while the run is finishing, the messages request can read history before the response is persisted, while the subsequent status request observes that the job has already become inactive. In that case resolveDetachedRunEnd returns null, but the marker is cleared first and processedConvoRef is then latched, so no later history refresh can retry the resolution and the follow-up remains queued indefinitely. Clear the marker only after a terminal response has actually been resolved, or explicitly refetch/re-arm when it is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16e5b7b: the marker stays until a response resolves it, with one history refetch for a response saved after the first read.
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: 16e5b7b798
ℹ️ 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".
| response = | ||
| responses.find((message) => message.messageId === exact) ?? | ||
| responses.find((message) => message.messageId === unpadded); |
There was a problem hiding this comment.
When a regeneration is left and its history request completes before terminal persistence, the messages still contain the previous response under the unpadded ID. This fallback immediately accepts that old row as the detached run's result and usually classifies it as completed, so a regeneration that subsequently aborts or fails can auto-send the queued follow-up instead of leaving it for manual send. The fresh evidence beyond the earlier missing-response race is that regenerations deliberately use existingId_ while the pre-run existingId row already exists, meaning parkDetachedEnd does not take the new refetch/retry path at all. Require evidence that this row was updated by the detached generation before publishing its outcome.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8269cc: a detached regeneration is no longer resolved from history (resolveDetachedRunEnd returns null for isRegenerate), so the follow-up stays queued for a manual send; covered in queue.spec and useResumableSSE.spec.
| return { | ||
| conversationId, | ||
| outcome, | ||
| endedAt: Date.now(), | ||
| ...(outcome === 'completed' && { responseMessageId: response.messageId }), |
There was a problem hiding this comment.
When an Agent server-queued turn is admitted while the predecessor chat is detached, its receipt records the consumed predecessor in effectivePredecessorCreatedAt, but this synthesized end omits generationCreatedAt. useQueueDrain therefore cannot match and consume that admission receipt; once receipt reconciliation removes the admitted server row, any remaining local row is treated as eligible on this predecessor end and can be submitted while the server-started successor already owns the conversation. Capture the detached run's active generation epoch and include it in the reconstructed RunEnd so the existing admission-boundary guard remains effective.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8269cc: the pane captures the run's live generation epoch when it leaves and the parked end carries it as generationCreatedAt, so the drain still matches admission receipts; a detached run with no epoch whose queue the server shares is left for a manual send (useResumeOnLoad.spec covers both).
Leaving a chat mid-run closes its stream without stopping the run, and the server deletes the job when it finishes, so no run end ever reached the queue drain and the follow-up stayed in the rail. The stream hook now remembers a run it closed while still generating, and on return the resume path reads the run's persisted response and parks its end, so a completed run drains the queue as an attached one would. A run the user stopped, or whose end already reached the drain, is not remembered.
…as It Switching to a saved chat clears the submission to null, and unmounting runs no later effect body, so the detached run is now recorded in the cleanup itself whenever the stream closes on a run that is still going. A new run in the conversation clears it. On return the marker is kept until its response is in history, with one history refetch for a response saved after the first read, and an explicit response id is matched exactly before its unpadded form, never by a sibling fallback.
A regeneration rewrites a response history already holds, so a detached one is no longer resolved from history. The run's generation epoch is captured when the pane leaves it and carried into the parked run end, so the drain still matches server admission receipts; a run with no epoch whose queue the server shares is left for a manual send.
16e5b7b to
071d82a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 071d82a38e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 071d82a38e
ℹ️ 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(detachedFamily, null); | ||
| return true; | ||
| } | ||
| const end = resolveDetachedRunEnd(conversationId, detachedRun, getMessages()); |
There was a problem hiding this comment.
When the user leaves after the created event, the optimistic assistant placeholder remains in the React Query cache, whose messages query has refetchOnMount: false. If the server later aborts or errors, this inactive path passes that cached row to resolveDetachedRunEnd before performing any refetch; because the optimistic row lacks the persisted unfinished/error flags, it is classified as completed and the queued follow-up is sent unexpectedly. Unlike the earlier missing-response race, the stale cache supplies a non-null response, so the new fallback refetch never runs; require fresh persisted history or other terminal evidence before publishing the completion.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0dfc5d2: a detached run is now resolved only after a successful history refetch (throwOnError), never from the optimistic row left in cache; a failed refetch keeps the marker for the next visit. useResumeOnLoad.spec adds an aborted-while-away case and a failed-refetch case, both failing on 071d82a.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dfc5d26cd
ℹ️ 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".
Summary
If you queue a follow-up while a reply is generating, then switch to another chat before the reply finishes, the follow-up never sends. When you come back, the finished reply loads from history, but the follow-up stays in the queue rail until you send it by hand. If you stay in the chat, it sends automatically. #16615 exposed this while writing queue coverage, and it reproduces on code from before that change.
The cause: starting another chat clears the pane's submission. The stream hook then closes the stream without stopping the server run, which is deliberate. The server deletes the job when it finishes. So no terminal event reaches the queue drain, and the resume-on-load path that reloads the finished reply does not publish a run end.
The stream hook now remembers a run it closed while that run was still generating. On return, if the server reports nothing active, the resume path reads the run's persisted response and parks its end for the drain. A completed reply then drains the queue, as it would for an attached run. A failed reply leaves the queue for a manual send. A run the user stopped, or whose end already reached the drain, is not remembered. A stopped reply persists like a completed one, so the stop request itself is what keeps a stopped run from auto-sending.
Closes berry-13#225.
Type of change
Testing
Tested environments/configuration:
reviewctl verify(desktop light, desktop dark, mobile).Automated tests:
parked-run-end-drains-on-returninqueue-owners.spec.ts: queue a follow-up during a slow run, leave the chat, wait for the server to finish the run, then return. The follow-up is sent. The same scenario failed on canary before this fix, which is why 🧺 refactor: Move the Chat Follow-Up Queue State to Jotai #16615 dropped it.useResumeOnLoad.spec: a detached run that finished parks a completed run end and clears the marker. A conversation with no detached run parks nothing. The first test fails with the fix removed.useResumableSSE.spec: navigation records the detached run. A terminal clear, a run end that already reached the drain, and a stopped run record nothing.queue.spec: outcome resolution covers completed, failed, unfinished, missing and sibling responses.npx jest --findRelatedTests <changed files> --maxWorkers=2: 499 suites, 6593 tests.npx tsc --noEmit -p client/tsconfig.json: clean. ESLint and Prettier: clean.Screenshots / recordings
No visual change; the e2e scenario covers the behavior.
Risk / compatibility
A run that started as a brand-new conversation is not tracked, because its submission carries no concrete conversation id; it keeps today's behavior. The marker is session memory only, like the queue it serves.
Checklist