Repository navigation
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).
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".
0dfc5d2 to
4062008
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40620082cb
ℹ️ 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".
codegraph-librechat
Bot
added
🗺️ Chat Features
4062008 to
3fdb25a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fdb25ae6c
ℹ️ 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".
| } | ||
|
|
||
| /** Still generating: the stream this resume attaches will deliver the run's own end. */ | ||
| jotaiStore.set(detachedRunByConvoId(conversationId), null); |
There was a problem hiding this comment.
When the status request observes the detached job as active but the job finishes and is deleted before the SSE subscription attaches, this clears the only detached marker before a terminal event is guaranteed. The resulting stream 404 follows useResumableSSE's conservative recovery path, which publishes an aborted end even when history contains a completed response, so the queued follow-up remains stuck. Retain the marker until attachment/terminal reconciliation succeeds, or let the 404 recovery resolve the persisted outcome.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
| if (run.isRegenerate === true) { | ||
| return null; |
There was a problem hiding this comment.
For every detached regeneration, this branch returns null permanently because persisted history cannot distinguish the old response from the regenerated one. parkDetachedEnd consequently never clears the marker, so each later visit to the conversation performs another forced history refetch even though the marker can never resolve; the follow-up can remain queued for manual send without retaining this unrecoverable marker. Clear it after the authoritative refetch establishes that this is a regeneration.
AGENTS.md reference: AGENTS.md:L59-L61
Useful? React with 👍 / 👎.
| if (submission.conversation?.conversationId != null) { | ||
| /** A run starting here owns the conversation's end from now on: an earlier run this pane | ||
| * left is superseded, and a Stop belonged to that earlier run. */ | ||
| jotaiStore.set(stopRequestedByConvoId(submission.conversation.conversationId), false); |
There was a problem hiding this comment.
When a user presses Stop, leaves before the abort terminal frame, returns while that same generation is still active, and then leaves again, the resumed submission reaches this unconditional reset and forgets that the generation was stopped. The second cleanup can therefore record it as detached; stopped compaction turns are persisted with unfinished: false, so later history recovery classifies such a turn as completed and auto-sends the queued follow-up despite the explicit Stop. Preserve the marker when resumeGenerationCreatedAt identifies the stopped generation, and reset it only for a genuinely newer run.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
codegraph-librechat
Bot
added
the
🗺️ Chat State
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.
The detached-run marker, the Stop marker and the resolved outcome were keyed by conversation and inferred from which rows existed, so every race at the edges produced a new wrong answer. Each now names the run by its generation epoch: - A persisted response counts as the run's only when the server wrote it at or after the run's epoch, which rejects the reply a regeneration replaces and resolves the regenerated sibling. - Stop records the epoch it stopped. Reattaching to that run keeps it, a different run resets it, and a settled abort releases it. - An active status or a resume of the same run keeps the marker. It is retired once that run's end is published, by a newer run, or at once when history can never date it, and the resume's 404 recovery resolves it from the history it just read instead of reporting an abort.
3fdb25a to
3b4aa1a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b4aa1ae46
ℹ️ 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 detachedRun = jotaiStore.get(detachedRunByConvoId(recoveryConvoId)); | ||
| const detachedEnd = | ||
| detachedRun != null && persistedMessages != null | ||
| ? resolveDetachedRunEnd(recoveryConvoId, detachedRun, persistedMessages) |
There was a problem hiding this comment.
When a detached marker has no generationCreatedAt (for example, during mixed-version operation) and an Agent server-queued turn has already consumed the predecessor, this 404 recovery path publishes a completed end without the server-queue check used by useResumeOnLoad. useQueueDrain cannot match that end to the admission receipt, so after the server row is reconciled away it can use the same boundary to submit a remaining local row while the server-started successor owns the conversation. Fresh evidence beyond the earlier epoch finding is that the newly added 404 recovery bypasses the explicit epochless guard at useResumeOnLoad.ts:1025-1035; apply the same guard here before resolving the detached end.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
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.
Each piece of state about a left run is keyed to its generation epoch: a persisted response counts as the run's only when the server wrote it after the run started, a Stop belongs to the run it was pressed on (a reattach keeps it, a different run or a settled abort releases it), and a left run yields exactly one end, including when its job disappears between the status read and the reattach.
Closes berry-13#225. Fixes berry-13#257, where a Stop that lost the race to natural completion kept suppressing the left run's outcome.
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. A job deleted between the status read and the reattach resolves from history exactly once, and a Stop survives a reattach to the same run (both fail on the previous design).queue.specandabort.spec: the reply a regeneration replaces is rejected by epoch, and a settled abort releases Stop.queue.spec: outcome resolution covers completed, failed, unfinished, missing and sibling responses.npx jest --findRelatedTests <changed files> --maxWorkers=2: 567 suites, 8010 tests.npm run typecheck(client): 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.
Two narrower cases stay as today's behavior and are tracked separately: a row queued behind an off-screen successor run (berry-13#258), and a stale inactive status read racing the optimistic tail (berry-13#259).
Checklist