Skip to content

fix(client): preserve resumable SSE response identity - #14788

Open
Falenos wants to merge 6 commits into
danny-avila:mainfrom
Falenos:fix/resumable-sse-response-identity
Open

fix(client): preserve resumable SSE response identity#14788
Falenos wants to merge 6 commits into
danny-avila:mainfrom
Falenos:fix/resumable-sse-response-identity

Conversation

@Falenos

@Falenos Falenos commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #14787.

Resumable SSE sync now identifies the active response using the generation-owned response ID already resolved by the resumed submission: the server-provided response ID, then the submission's initial response ID, then the synthetic fallback. It no longer guesses by parent ID, which is ambiguous when regeneration preserves older assistant siblings.

The existing empty-snapshot reconciliation remains intact for an exact owned response, while a missing owned row is inserted without mutating an older sibling.

Change Type

  • Bug fix (non-breaking change which fixes an issue)

Testing

  • useResumableSSE.spec.ts: 81 tests passed
  • ESLint on both changed files
  • Client TypeScript type-check passed

Test Configuration:

  • Node.js v24.16.0
  • macOS

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Local unit tests pass with my changes

Copilot AI lite review requested due to automatic review settings August 13, 2026 13:00
Fixes danny-avila#14787

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes resumable SSE sync applying aggregated content to the wrong assistant message when resumeState.responseMessageId is missing (especially after ID rehydration and during regeneration), by switching to a generation-owned response ID selection order and avoiding ambiguous parent-based matching.

Changes:

  • Update sync response selection to prefer: server-provided responseMessageIdsubmission.initialResponse.messageId → synthetic ${userMsgId}_.
  • Remove the prior fallback that matched by parentMessageId === userMsgId, which is ambiguous with regenerated sibling responses.
  • Add focused unit tests to cover ID rehydration and regeneration sibling scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
client/src/hooks/SSE/useResumableSSE.ts Adjusts sync response identification to use generation-owned response IDs and avoids ambiguous parent-based matching.
client/src/hooks/SSE/tests/useResumableSSE.spec.ts Adds tests validating correct sync targeting after user message ID rehydration and during regeneration with older response siblings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Falenos
Falenos force-pushed the fix/resumable-sse-response-identity branch from afc9284 to 8eaefe4 Compare August 13, 2026 13:09
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danny-avila

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

setMessages(mergeResumeMessages(messages, userMessage, newMessage));

P1 Badge Append the resumed response after its older siblings

When a regeneration sync supplies a new responseMessageId before that response exists locally, this branch passes it to mergeResumeMessages, which inserts a missing response immediately after the user message (lines 596–599) and therefore before every existing assistant sibling. buildTree preserves that sibling order, while the default reversed sibling index selects the last child as newest (client/src/utils/messages.ts lines 43–47), so the older answer remains visible and the resumed generation is hidden behind sibling navigation—the exact missing-row regeneration exercised by the new test. Append the generation-owned response after the existing siblings, as the normal regeneration handlers do.

ℹ️ 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 responseId = resumeSubmission.initialResponse.messageId;
const responseIdx = messages.findIndex((message) => message.messageId === responseId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve resumed response metadata when inserting its row

When this exact lookup misses, such as after reconnecting to a regeneration before its active response row has been persisted, the newly reached insertion branch constructs the response from scratch and copies only iconURL and model. Although buildResumeEventSubmission has already resolved the response's sender and other submission metadata, those fields are discarded; for ordinary non-agent endpoints useMessageActions consequently returns an undefined message label and the streaming response renders with a blank author header until the final event replaces it. Construct the inserted row from resumeSubmission.initialResponse before applying the snapshot fields.

Useful? React with 👍 / 👎.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Falenos

Falenos commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both findings in 87ddbe2fc: missing resumed responses are appended after existing siblings, and inserted rows inherit metadata from resumeSubmission.initialResponse. The regression test now covers ordering and metadata preservation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Resumable SSE sync can misidentify response after ID rehydration

3 participants