fix(site/src): fan chat message upserts out to every containing page - #27912
fix(site/src): fan chat message upserts out to every containing page#27912DanielleMaywood wants to merge 9 commits into
Conversation
Durable message batches previously probed membership against page 0 of the infinite messages cache only, so a message living in an older page was re-inserted into page 0 as a duplicate and the flattener rendered the stale older copy after scrollback. Upserts now replace an ID in place in every page that contains it, insert unknown IDs into page 0 in descending order, never touch pageParams, and return the previous InfiniteData reference when no value changed. History replacement collapses pages and pageParams together to length 1 and gains a no-op reference guard. chatMessagesEqualByValue moves from chatStore.ts into api/queries/chats.ts so the cache layer never imports page code.
…nd use lodash isEqual Replace the JSON.stringify-based comparator with lodash isEqual so message equality does not depend on wire key order or stringify quirks, and split the fan-out into named blocks for readability.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69076dde6c
ℹ️ 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".
| ], | ||
| }; | ||
| }); | ||
| upsertChatMessages(queryClient, chatID, messages); |
There was a problem hiding this comment.
Add Storybook coverage for paginated message fan-out
When an older paginated message receives a durable WebSocket update, this call now changes the user-visible conversation content, but the diff adds only Vitest/renderHook coverage. Add an AgentChatPage Storybook story whose play function exercises the WebSocket update and verifies that the refreshed content is rendered without a duplicate; FE1 explicitly requires Storybook interaction coverage for user-visible behavior changes.
AGENTS.md reference: site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5cea2a0. Added the DurableUpdateFansOutToOlderPage story: it seeds a two-page messages cache with the target message living only in page 1, delivers a durable type:"message" WebSocket event through the existing /chats/ harness, and asserts the refreshed content renders exactly once while the stale text is gone. Note the precedent from #27892 does not apply here: that decline was for the global watcher in AgentsPageLayout, while the durable consumer is the per-chat stream inside AgentChatPage, which the existing WebSocket harness already drives. The durable path also writes synchronously to the cache, so it is not affected by the setTimeout(0) timing issue noted for the message_part stories.
Reply generated by Coder Agents.
| const messagesToInsert = messages.filter( | ||
| (message) => !foundIDs.has(message.id), | ||
| ); |
There was a problem hiding this comment.
Deduplicate unseen messages before inserting them
When two revisions of the same previously unseen message are delivered in one WebSocket batch, incomingByID retains the latest revision, but this filter rebuilds the insertion list from the original array, so both copies are appended to page 0. The stream transport can batch consecutive revision events, making this a reachable cache-corruption path; build messagesToInsert from the deduplicated map instead so each message ID is stored once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5cea2a0. messagesToInsert is now built from [...incomingByID.values()] instead of the raw batch array, so a batch carrying two revisions of the same unseen ID stores one entry (the latest revision). The render was only masked by the flattener; the cache no longer carries permanent duplicate entries. A Vitest test pins the single-insertion plus latest-revision behavior.
Reply generated by Coder Agents.
…n a story Build the page-0 insertion list from the deduplicated batch map so a batch carrying two revisions of the same unseen ID stores one entry. Add a Storybook play story that delivers a durable WebSocket message event for a message living in an older cache page and asserts the refreshed content renders exactly once.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 hand-maintained field list silently skips any field added to ChatMessage in the future, which would make the comparator report equal messages and skip cache updates. Comparing the whole message with isEqual covers every field by construction.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83102e1dae
ℹ️ 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".
Co-authored-by: Danielle Maywood <danielle@themaywoods.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19f0f40c35
ℹ️ 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".
…m MockChatMessage Replace the handwritten ChatMessage literals in the fan-out story with named variants spreading the shared MockChatMessage fixture, and drop the narrating JSDoc that restated the play assertions.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: c8c7c2d5ea
ℹ️ 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".
| ): TypesGen.ChatMessage => ({ | ||
| ...MockChatMessage, | ||
| id, | ||
| content: [{ type: "text" as const, text }], |
There was a problem hiding this comment.
Remove the avoidable const assertion
Remove as const here: the function's explicit TypesGen.ChatMessage return type already contextually types the content entry and preserves the "text" discriminant. Keeping the redundant cast violates the frontend rule against avoidable assertions and weakens the intended generated-type checking.
AGENTS.md reference: site/AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29dec6a. The explicit TypesGen.ChatMessage return type contextually types the content entry, so the as const was avoidable under FE2. Confirmed with tsc clean and 172 tests passing after removal.
Reply generated by Coder Agents.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Problem
upsertCacheMessagesprobed membership againstpages[0]of the infinite messages cache only. A durable WS batch for a message living in an older page re-inserted it into page 0 as a duplicate, and the flattener's last-occurrence-wins dedup rendered the stale older-page copy after scrollback (FINDINGS 1.1).Fix
Message upserts now fan out across all containing pages, per the updater contract:
pageParamsare never changed during an upsert; a history replacement replacespagesandpageParamstogether with arrays of length 1.InfiniteDatareference is returned when no message value changed.The pure updaters and semantic ops (
upsertChatMessages,replaceChatMessagesHistory) live insite/src/api/queries/chats.ts, anduseChatStoredelegates to them.chatMessagesEqualByValuemoved fromchatStore.tsintochats.tsso the cache layer does not import page code.Tests
chats.test.ts: 12 new tests covering contract points 1-5: in-place replacement in older pages, duplicate-ID fan-out, found-but-equal reference guard, descending inserts, mixed batches,pageParamsstability, history collapse, no-op reference guards, and absent-cache no-ops.chatStore.test.tsx: multi-pagehistory_resetcollapse to one page plus one pageParam, and a renderHook-level pin for FINDINGS 1.1 (a WS durable message updates an older page in place without duplicating into page 0).Known remaining gaps
fetchNextPagesnapshots cache state at fetch start (upstream TanStack issue #3579, closed not_planned), sosetQueryDatawrites during an in-flight page fetch are discarded. A follow-up PR will address this; until then fan-out fixes the common non-paginating case.has_moretrue, inserting a not-found message whose ID is below the loaded minimum lowers the nextbefore_idcursor. Accepted per the updater contract; rare because the reconnect cursor makes durable events older than the loaded window uncommon.PR generated by Coder Agents.