🔑 feat: Expose the Messages Key on the Chat Contract - #16395
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: 1d23969433
ℹ️ 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 [submission, setSubmission] = useRecoilState(store.submissionByIndex(index)); | ||
| const initialResponse = submission?.initialResponse; |
There was a problem hiding this comment.
Reading submissionByIndex here adds a new Recoil consumer for feature-owned submission state. The repository requires an existing atom and all of its readers/writers to be converted together when that state is touched; exposing another reader through the public chat contract instead expands the migration boundary. Convert this atom and its consumers to Jotai before exposing the value.
AGENTS.md reference: AGENTS.md:L104-L107
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4288051 (head b503216). The host no longer reads submissionByIndex: it keeps the response ask submits in its own state, beside the setter it already hands useChatFunctions, so the Recoil atom gains no reader and needs no migration here. useChatFunctions now takes that setter as a plain value setter, which is the only way it calls it.
|
|
||
| const setSubmission = useSetRecoilState(store.submissionByIndex(index)); | ||
| const [submission, setSubmission] = useRecoilState(store.submissionByIndex(index)); | ||
| const initialResponse = submission?.initialResponse; |
There was a problem hiding this comment.
When an active generation is restored after reload or cross-device navigation, useResumeOnLoad stores a reconstructed submission whose initialResponse.content is already-populated resumeState.aggregatedContent (useResumeOnLoad.ts, lines 217-257 and 1052-1060). Returning that value here violates the new contract's pre-stream/object-identity guarantee, so an adapter cannot reliably distinguish retained seed parts from output that streamed before reconnection. Preserve a separate original seed, or explicitly represent that no pre-stream response is available for restored runs.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4288051. A restored run installs its submission through useResumeOnLoad, not ask, so the host records no pre-stream response for it and initialResponse is undefined, as the contract now states. Regression: 'has no submitted response for a run restored outside ask' in contract.spec.tsx.
| * The response the in-flight turn was submitted with, before any streamed output. Parts it was | ||
| * seeded with (a retained edit prefix, lane placeholders) stay these same objects in the cache | ||
| * until the stream replaces them, so a part that is not one of them has streamed. `undefined` | ||
| * when no turn has been submitted from this pane. |
There was a problem hiding this comment.
On the normal FINAL path, finalHandler sets isSubmitting to false but does not reset submissionByIndex; useResumeOnLoad.ts lines 485-487 explicitly note that a completed submission remains installed. Consequently this property continues returning the previous turn's response throughout the ready/error state, despite being specified as the response for the in-flight turn, so consumers can retain stale seed identities until navigation or the next send. Gate the exposed value on a live-turn signal or clear/extract the snapshot during terminal teardown.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4288051: initialResponse is exposed only while isSubmitting, so it reads undefined once the turn settles even though the submission stays installed. Regression: 'drops the submitted response once the turn settles' (submit, then settle).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b503216cb5
ℹ️ 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".
| }, | ||
| [setStoredSubmission], | ||
| ); | ||
| const initialResponse = isSubmitting ? submittedResponse : undefined; |
There was a problem hiding this comment.
When this ChatView instance navigates between routes, submittedResponse survives because the hook remains mounted. If conversation A has populated it and conversation B is then restored with an active generation, useResumeOnLoad installs B's submission outside this callback and sets isSubmitting true, causing the contract to expose A's response for B instead of undefined. The replacement local-state implementation is fresh evidence beyond the earlier restored-run finding; reset or key this snapshot by queryParam/conversation identity.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| * seeded with (a retained edit prefix, lane placeholders) stay these same objects in the cache | ||
| * until the stream replaces them, so a part that is not one of them has streamed. `undefined` | ||
| * while no turn is in flight, and for a run restored after a reload, whose response already |
There was a problem hiding this comment.
The stated object-identity guarantee does not hold for edited responses. ask clones the retained prefix and then writes it through React Query's setQueryData; with default structural sharing, unchanged nested parts can be replaced by the equal objects already in the cached response. A consumer comparing cached parts with initialResponse.content will therefore classify unchanged retained parts as streamed. Either force the optimistic cache entry to retain the submitted objects or expose an explicit prefix/seed marker instead of promising identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, the identity promise does not survive setQueryData's structural sharing. Fixed in 4c732a3 by changing what the contract promises: seeded parts hold their indices, the stream appends after them or fills an empty one and never rewrites a seeded part with content, so consumers compare by position. The facade in #16375 now reads it that way.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9db09300e7
ℹ️ 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 initialResponse = | ||
| isSubmitting && submitted?.key === queryParam ? submitted.response : undefined; |
There was a problem hiding this comment.
When a locally submitted turn in conversation A settles, submitted remains cached; if a later run for A is started on another client and useResumeOnLoad restores it, that hook writes directly to submissionByIndex and sets the pane submitting again without replacing this snapshot. Because the key still equals A, this condition exposes the old turn's response instead of undefined for the restored run. Fresh evidence beyond the earlier navigation fix is that re-entering the same conversation reuses the key while useResumeOnLoad.ts:1052-1060 installs a different generation outside this callback, so the snapshot needs a run identity or terminal clearing rather than only a conversation key.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1b364a0: the host drops the submitted response when the turn settles (render-time transition check on isSubmitting), so a later run restored into the same chat outside ask reports none. Regression: 'forgets a settled turn when a later run in the same chat is restored'.
| * holds the parts the response was seeded with (a retained edit prefix, empty lane placeholders) | ||
| * at their indices: the stream appends after them or fills an empty one, and never rewrites a | ||
| * seeded part that has content. Compare by position, not identity, since the cache may hold |
There was a problem hiding this comment.
For an edited response whose retained prefix ends in an unphased TEXT or matching THINK part, the stream does rewrite a populated seed position: calculateContentIndex maps server index 0 to editPrefixOffset - 1, and updateContent concatenates the incoming delta there. The existing steps.spec.ts case explicitly turns "kept tail" into "kept tail!". Consequently an adapter following this contract can classify newly streamed output as seed content; expose the folding boundary/length or document that the last seeded part may contain appended output. This is fresh evidence beyond the earlier object-identity discussion because the contradiction is in the stream's index-folding behavior itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, calculateContentIndex folds server index 0 into the last prefix part. Fixed in 1b364a0: the contract now says the stream may continue the last seeded part when it is text or reasoning of the same kind, and to compare by position and content. The facade in #16375 treats that part as streamed once its text differs from the seed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b364a0bb6
ℹ️ 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 setSubmission = useSetRecoilState(store.submissionByIndex(index)); | ||
| const setStoredSubmission = useSetRecoilState(store.submissionByIndex(index)); | ||
| const [submitted, setSubmitted] = useState<{ key: string; response?: TMessage }>(); |
There was a problem hiding this comment.
The new submitted snapshot—and the accompanying wasSubmitting transition state—introduces feature-owned client state through React useState. Repository policy requires all new client state to use Jotai and to keep feature-owned atoms within the feature, so these snapshots should be represented by feature-local Jotai state rather than adding another state mechanism that must be migrated later.
AGENTS.md reference: AGENTS.md:L104-L109
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not applying this one. The client-state rule is about which store holds shared state while the app moves off Recoil ("the unit of conversion is one atom plus every file that reads or writes it"). This value is local to the hook instance that calls ask, exactly like files, filesLoading and showPopover, which useChatHelpers already keeps in useState. An atom would make per-pane local state globally readable store state, which is the opposite of what the rule protects.
1b364a0 to
4e4e065
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e4e0656f4
ℹ️ 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 (wasSubmitting !== isSubmitting) { | ||
| setWasSubmitting(isSubmitting); | ||
| if (!isSubmitting) { | ||
| setSubmitted(undefined); | ||
| } |
There was a problem hiding this comment.
When the server reports that another generation has replaced the current one in the same conversation, useResumableSSE.ts lines 2709-2711 and 4416-4418 deliberately keep isSubmitting true while clearing the submission atom directly. This false-only cleanup therefore never runs, and because queryParam is unchanged, the contract exposes the superseded run's initialResponse for the replacement generation instead of undefined. Fresh evidence beyond the earlier same-chat restored-run comment is that the replacement-handoff paths explicitly preserve the true submitting state, so key or clear this snapshot using the generation identity rather than only a true-to-false render.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by removing the member in 5be1d4a. Replacement, restore and navigation all install or tear down a run outside ask, so a snapshot the host keeps beside ask cannot know when its run ended; the only true run identity is the submission atom, and reading it adds a Recoil reader. The contract now carries messagesKey only, and the facade in #16375 reads streaming status from content alone.
berry-13
changed the title
…contract Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
…the submission atom The contract's initialResponse is now the response ask submitted, held beside the host's own state and exposed only while the turn is in flight, so a settled turn and a run restored after a reload report none, and the submission atom gains no reader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
… messages key Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
…tions, not identity Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
…m can continue the last seeded part Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
A run can be restored, replaced or torn down outside ask, so a copy of the submitted response kept in the host describes the wrong run on those paths. The contract keeps messagesKey only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
…ness Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQdLgSaTheiBFHYnUDgobw
5be1d4a to
d39751d
Compare
1 participant
Summary
The chat contract hands consumers
getMessages()andconversation, but not which conversation those messages come from.getMessages()reads the route's id, which runs ahead of theconversationatom while navigation settles, so a consumer that reportsconversation.conversationIdbeside those messages names the previous chat for that window.This adds
messagesKey, the keygetMessages()reads when called without one: the route id, else the conversation id, else''before the pane has either.useChatHelpersserves it from thequeryParamit already computes; no consumer changes.Targets canary, where #16374 landed the parts mapping. #16375, the AI SDK-shaped
useChatfacade, is stacked on this and readsmessagesKeyas its chat id and its message query key.Type of change
Testing
Tested environments/configuration:
Unit level; no component reads the new member yet.
Automated tests:
client/src/hooks/Chat/__tests__/contract.spec.tsx: the key follows the route id before the conversation catches up, falls back to the conversation id without one, and is empty before either existscd client && npx jest --findRelatedTests src/hooks/Chat/useChatHelpers.ts src/hooks/Chat/contract.ts: 5624 passedcd client && npx tsc --noEmit: clean, includingcontract.check.tsreviewctl precheckagainst canary: static checks and related jest passScreenshots / recordings
No user-facing change.
Risk / compatibility
None. The member is read-only and derived from a value the host already computed.
Checklist