fix(site): write WebSocket messages to React Query cache by DanielleMaywood · Pull Request #23618 · coder/coder

DanielleMaywood · 2026-03-25T17:24:29Z

🤖 This PR was written by Coder Agent on behalf of Danielle Maywood 🤖

Problem

When navigating away from an agent chat and returning, new messages don't appear immediately.

WebSocket-delivered durable messages update the in-memory chat store but are never written back into the React Query infinite query cache. On re-mount, the stale cache is served and — if the background refetch returns structurally equal data (no new durable messages committed to DB yet) — the sync effect's element-level reference check sees no change and skips the store update.

Solution

Add upsertCacheMessages(), mirroring the existing updateChatQueuedMessages() pattern, to write WebSocket-delivered durable messages into the infinite query cache alongside the store upsert.

How it works

Targets pages[0] (the newest page) using queryClient.setQueryData
Uses chatMessagesEqualByValue for dedup (same function the store uses)
Returns the current cache reference unchanged when nothing changed (reference stability)
Sorts DESC to match the API's page order
Called right after store.upsertDurableMessages(pendingMessages) in the WebSocket message handler

What this fixes

The React Query cache now contains ALL messages delivered via WebSocket, not just the original REST fetch
Navigating back serves up-to-date cached data, so the sync effect detects real changes
The WebSocket initial snapshot also benefits — snapshot messages flow through the same handleMessage callback

What this doesn't change

The updateChatQueuedMessages helper is untouched (already works)
The store remains the primary source of truth for rendering
No server-side changes

mafredri

The design is sound. The dual-write approach (store + cache) closes a real gap where WebSocket messages were invisible to the React Query cache, causing stale data on navigate-back. The implementation mirrors updateChatQueuedMessages cleanly: same setQueryData shape, same reference-stability guards, correct DESC sort to match the API page contract. No correctness bugs in the implementation.

1 P2, 1 P3, 1 note. No blockers.

The dominant finding: the entire behavioral fix has no test. All six reviewers converged on this independently. The test template already exists at chatStore.test.tsx:891 for the sibling queue_update path. One additional process observation from Mafu-san: the PR's bug diagnosis is detailed but unverified, and the missing test is the proof that the diagnosis is correct.

"The fix is defended only by 'it didn't break existing tests,' which is exactly what the bug it fixes also passed." -- Bisky

🤖 This review was automatically generated with Coder Agents.

mafredri · 2026-03-30T11:56:00Z

+		// last REST fetch, and structural sharing can suppress the
+		// refetch-driven store update when no new durable messages
+		// have been committed to the DB yet.
+		const upsertCacheMessages = (messages: readonly TypesGen.ChatMessage[]) => {


P2 No test for the core behavioral change. (Bisky P2, Nami P2, Mafuuu P2, Hisoka P2, Meruem P2, Mafu-san P2)

All six reviewers independently flagged this. The existing test at chatStore.test.tsx:891 proves the identical pattern for queue_update events. No equivalent exists for message events flowing through upsertCacheMessages.

"The fix is defended only by 'it didn't break existing tests,' which is exactly what the bug it fixes also passed." -- Bisky

"'Mirrors the existing pattern' means mirror the test too." -- Mafu-san

Test sketch (following the line 891 template):

Seed queryClient with infinite query data matching chatMessagesKey(chatID).

Emit a WS message event with a new message.

Assert queryClient.getQueryData(chatMessagesKey(chatID)).pages[0].messages contains the new message sorted DESC by ID.

Emit the same message again, assert reference stability (same object returned).

Without this, a regression in the dedup or sort silently reintroduces the original bug.

🤖

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Added test "writes WebSocket message events into the chat query cache" following the line 891 template — seeds cache, emits a WS message event, asserts pages[0].messages contains the new message sorted DESC, and verifies reference stability on duplicate emit.

mafredri · 2026-03-30T11:56:00Z

+				};
+			});
+		};
 		cancelScheduledStreamReset();


P3 Missing blank line between upsertCacheMessages closing brace and cancelScheduledStreamReset(). (Bisky)

"Every other local function in this effect is separated by blank lines. This breaks the visual grouping that separates function definitions from effect body." -- Bisky

Compare lines 257-258, 289-290 (blank-line separated) vs 337-338 (not separated). Trivial fix.

🤖

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Fixed.

mafredri · 2026-03-30T11:56:00Z

 				// pass: one Map copy + one sort instead of N each.
 				if (pendingMessages.length > 0) {
 					store.upsertDurableMessages(pendingMessages);
+					upsertCacheMessages(pendingMessages);


Note Cache write triggers an extra sync-effect cycle per WS message batch. (Nami, Mafuuu, Hisoka)

Three reviewers independently traced this: after upsertCacheMessages updates the cache, the sync effect re-fires, detects contentChanged, and calls store.upsertDurableMessages as a no-op (store already has those messages via the line above). The extra work is O(messages), hits reference-equality fast paths, and produces no state change or re-render cascade. Not actionable, just worth knowing when profiling render frequency.

🤖

mafredri

Round 1 findings addressed: the P2 (missing test) is resolved with a genuine test that covers insertion, DESC sort, and reference stability. The P3 (missing blank line in useChatStore.ts) is fixed. Good turnaround.

1 P2, 1 P3 remain. No blockers.

The new test covers the happy path well but misses one branch: same message ID with different content. The content-update comparison (chatMessagesEqualByValue) in the cache updater is dead code in all tests. One formatting nit in the test file.

"Same class of issue as round 1 finding #2, introduced by the same diff that fixed it for useChatStore.ts." -- Mafuuu, on the missing blank line

🤖 This review was automatically generated with Coder Agents.

mafredri · 2026-03-30T13:10:57Z

 		expect(cachedData?.pages[0]?.queued_messages).toEqual([]);
 	});

+	it("writes WebSocket message events into the chat query cache", async () => {


P2 The content-update cache path (same ID, different content) is dead code in all tests. (Bisky P2)

"It never sends a message with an existing ID but different content. The !chatMessagesEqualByValue(existing, msg) branch in upsertCacheMessages (useChatStore.ts:314) is the one that handles content updates in the cache, and nothing exercises it." -- Bisky

The test sends a new message (id=2), then re-sends the identical message for reference stability. It never sends id=2 with different content. The existing test at line 410 ("clears stream state when a duplicate message id arrives with new content") does send updated content, but it uses createTestQueryClient() with gcTime: 0 and no seeded cache, so upsertCacheMessages bails at the !currentData?.pages?.length guard.

Consequence: if someone breaks the value comparison for the cache updater, the store shows correct content but the cache serves stale content after remount. Same class of bug this PR fixes.

Fix: add a third phase to this test: emit makeMessage(chatID, 2, "assistant", "revised"), assert cachedMessages[0].content[0].text === "revised".

🤖

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Added a third phase to the test: emits makeMessage(chatID, 2, "assistant", "revised") and asserts the cache entry is updated. This exercises the chatMessagesEqualByValue content-update branch in upsertCacheMessages.

mafredri · 2026-03-30T13:10:57Z

+		});
+		const refAfter = queryClient.getQueryData(chatMessagesKey(chatID));
+		expect(refAfter).toBe(refBefore);
+	});


P3 Missing blank line between test blocks. (Bisky P3, Nami P3, Mafuuu P3)

"Same class of issue as round 1 finding #2, introduced by the same diff that fixed it for useChatStore.ts." -- Mafuuu

Line 1055 }); runs directly into line 1056 it("closes old WebSocket... with no separator. Every other test boundary in this file has a blank line (e.g., lines 964-966). Add a blank line after });.

🤖

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Fixed.

mafredri

Round 2 findings addressed: the P2 (content-update path) is resolved with a third test phase covering same-ID-different-content. The P3 (blank line) is fixed. Mafuuu gave a clean review with a thorough contract trace, lifecycle walk, and adversarial scenarios. No contract breaks, no lifecycle gaps.

1 P2. No blockers.

One assertion gap remains in the test: the cache merge is verified by toContain(2) which also passes if the cache is replaced rather than merged. The existing message (id=1) surviving the upsert is the core promise and it's not asserted.

"A buggy implementation that replaces the cache with only the new message passes every assertion in this test." -- Bisky

🤖 This review was automatically generated with Coder Agents.

mafredri · 2026-03-30T14:02:35Z

+			pageParams: unknown[];
+		}>(chatMessagesKey(chatID));
+		const cachedMessages = cachedData?.pages[0]?.messages ?? [];
+		expect(cachedMessages.map((m) => m.id)).toContain(2);


P2 Assertions don't verify existing messages survive the upsert. (Bisky P2)

"A buggy implementation that replaces the cache with only the new message passes every assertion in this test." -- Bisky

toContain(2) and cachedMessages[0].id === 2 both pass for [2] (buggy replace) and [2, 1] (correct merge). The merge behavior is the core promise of upsertCacheMessages: it inserts new messages into existing cache data, not over it. If someone regressed this, every navigate-back would show only the last WS batch.

Fix: replace lines 1039-1041 with:

expect(cachedMessages.map((m) => m.id)).toEqual([2, 1]);

This verifies insertion, preservation, and DESC order in one assertion.

🤖

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Replaced the two assertions with a single toEqual([2, 1]) — verifies insertion, preservation, and DESC order in one shot.

…ache When navigating away from an agent chat and returning, new messages don't appear until a full page refresh. The root cause is that WebSocket-delivered durable messages update the in-memory chat store but are never written back into the React Query infinite query cache. On re-mount, the stale cache is served and — if the background refetch returns structurally equal data — the sync effect's element-level reference check sees no change and skips the store update. Add upsertCacheMessages(), mirroring the existing updateChatQueuedMessages() pattern, to write WebSocket-delivered durable messages into the infinite query cache alongside the store upsert. This ensures navigating back serves up-to-date cached data.

github-actions Bot added the community Pull Requests and issues created by the community. label Mar 25, 2026

github-actions Bot assigned DanielleMaywood Mar 25, 2026

matifali removed the community Pull Requests and issues created by the community. label Mar 26, 2026

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch 2 times, most recently from 3f2a480 to 02b9793 Compare March 30, 2026 11:09

DanielleMaywood changed the title ~~fix(site/pages/AgentsPage): write WebSocket messages to React Query cache~~ fix(site): write WebSocket messages to React Query cache Mar 30, 2026

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch 2 times, most recently from 50fcfdb to 4e45131 Compare March 30, 2026 11:27

DanielleMaywood marked this pull request as ready for review March 30, 2026 11:38

DanielleMaywood requested a review from mafredri March 30, 2026 11:38

mafredri reviewed Mar 30, 2026

View reviewed changes

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch from 4e45131 to b64b781 Compare March 30, 2026 12:13

mafredri reviewed Mar 30, 2026

View reviewed changes

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch from b64b781 to 249a624 Compare March 30, 2026 13:17

DanielleMaywood requested a review from mafredri March 30, 2026 13:52

mafredri reviewed Mar 30, 2026

View reviewed changes

mafredri approved these changes Mar 30, 2026

View reviewed changes

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch from 249a624 to e7afa2b Compare March 30, 2026 14:37

DanielleMaywood force-pushed the fix/chat-navigation-stale-messages branch from e7afa2b to 95f1491 Compare March 30, 2026 14:47

DanielleMaywood merged commit 3f8e300 into main Mar 30, 2026
27 checks passed

DanielleMaywood deleted the fix/chat-navigation-stale-messages branch March 30, 2026 14:56

github-actions Bot locked and limited conversation to collaborators Mar 30, 2026

Conversation

DanielleMaywood commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

How it works

What this fixes

What this doesn't change

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanielleMaywood commented Mar 25, 2026 •

edited

Loading