fix(site/src): preserve durable message writes across pagination fetches - #27988
fix(site/src): preserve durable message writes across pagination fetches#27988DanielleMaywood wants to merge 6 commits into
Conversation
fetchNextPage snapshots state.data.pages at fetch start and settles with the snapshot plus the new page, discarding any cache write made in between. A durable WebSocket message batch for a loaded page, arriving while an older page is fetching, was therefore clobbered at settle and the store synced the stale text until the next refetch. Wrap fetchNextPage in a per-chat pagination epoch. While an epoch is open, upsertCacheMessages and replaceCacheMessages still write to the cache immediately for low stream latency and additionally buffer the cache-write call. On epoch close the buffered writes replay synchronously against the settled cache, before React can observe the stale snapshot, because the notifyManager schedules notifications on a setTimeout(0) macrotask. A replacement supersedes every buffered write before it. The epoch is refcounted so a cancelled re-entrant fetchNextPage never replays early, and its state outlives the component so an unmounted page's in-flight fetch still heals the departed chat's cache. The Storybook WebSocket harness gains play-controlled event delivery: event entries with controlled: true wait for deliverWebSocketEvents() instead of auto-delivering after mount, so the new DurableUpdateSurvivesPaginationFetch story lands the durable update deterministically while the deferred pagination fetch is held.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
|
/coder-agents-review model:kimi-k3 thinking:xhigh |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 31 findings (4 P2, 8 P3, 12 Nit, 7 Note), COMMENT. Review Finding inventoryFinding inventory: PR #27988Findings
Contested and acknowledgedCRF-6 (Note, paginationEpoch.ts:1) - CHATS_QUERY_ARCHITECTURE.md not in repo
CRF-12 (P3, useChatStore.ts:242) - Record at the patchChatMessages funnel
CRF-21 (Nit, chatStore.test.tsx:1) - Bodyless commit deletes the advertised story harness
CRF-23 (Nit, chatStore.test.tsx:1117) - Tenth verbatim QueryClient defaultOptions copy
Round logRound 1Panel round (16 reviewers + Netero). Netero first pass: 3 P3, 1 Nit, 2 Note. Panel: 3 P2, 9 P3 (incl. Netero), 11 Nit, 9 Note total after cross-check. Trust signals (all load-bearing library claims verified against installed @tanstack/query-core 5.82.0; red-green reproduced by two reviewers independently) folded into the review body. Reviewed against 9a57dfa..d404f52. Round 2BLOCKED. CRF-21 silent (no reply, no code change); all other 29 findings addressed (18), acknowledged (8), or contested (3: CRF-6, CRF-12, CRF-23). No review. Reviewed against 9a57dfa..82a9dd4. Round 3Panel (Bisky, Hisoka, Pariston, Komugi, Meruem, Knov wildcard). Head unchanged since R2. All three contested findings closed: CRF-12 author correct (Pariston withdrew), CRF-6 closed with a Note-level residual (PR body Fix section still cites the doc), CRF-23 closed (defense verified, deferral proportionate). R1 fix claims verified where encountered (CRF-3, CRF-11, CRF-14, CRF-20 by Pariston; assembly-test determinism and the settle-ordering chain by Komugi). 1 new Nit (CRF-31, convergent Meruem + Knov). Bisky and Hisoka: no findings. Reviewed against 9a57dfa..82a9dd4. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a carefully built fix for a real, verified bug. The diagnosis (fetchNextPage snapshot-and-settle discarding concurrent setQueryData, upstream TanStack/query#3579) was checked against the installed @tanstack/query-core 5.82.0 source by five reviewers independently and holds; the timing claims (commit-before-resolve, setTimeout(0) notify scheduler, cancel-and-restart refcount shape, unconsumed abort signal) all check out; the red-green claim was reproduced, not just asserted. Buffer-and-replay earns its place over the codebase's usual cancel-the-refetch answer because a pagination fetch cannot be cancelled without losing the page. In Pariston's words: "The diagnosis is correct and the fix is at the right causal level (cache, not store)."
That said, the panel converged hard on two exposures, each raised independently by eight reviewers: the shipped assembly (the fetchOlderMessages wrapper) is exercised by no test, and the epoch guards only the pagination trigger while reconnect/remount/invalidation refetches of the same query clobber through the identical mechanism, with the deferral pointing at a plan document that does not exist in the repository and no ticket. A deferral without a ticket is a drop: the residual gap needs a tracked issue or a human decision to accept it.
Severity count: 3 P2, 9 P3, 11 Nit, 9 Note (24 findings as inline comments, plus trust-signal notes folded into this body).
Hisoka, on the epoch manager itself: "The refcounted epoch itself held everything I threw at it: stale-generation close, double-close after cancel-and-restart, cross-chat isolation, a replace replay collapsing pageParams to one page exactly as it does at write time, cancel-with-revert clobbering mid-flight writes and the replay healing that too. A worthy little machine. Its weakness isn't inside; it's the doors it doesn't guard."
Process notes worth reading: an intermediate Storybook repro harness was built, judged wrong, and fully reverted, leaving a proportional 5-file diff (honest process, but the deletion commit d404f52 has no body explaining why, and the PR description's "Phase 2 item 9" reference resolves to nothing a reader can find). Comment bloat is a pattern in this PR: 6 of 12 added comments either restate adjacent code or repeat a rationale that already lives at its owning declaration; each "why" should live once.
site/src/pages/AgentsPage/components/ChatConversation/chatStore.test.tsx:1
Nit [CRF-21] Commit d404f52488 deletes the story and harness the fix commit's body advertises, with no body explaining why. (Leorio)
(Placement note: this finding lives in the commit metadata; the inline comment tool folded it here because the anchor line is outside the diff.)
"The chart contradicts itself. de5c219f4b's body spends a paragraph describing the play-controlled WebSocket harness and the DurableUpdateSurvivesPaginationFetch story. Two commits later, a bodyless subject removes all of it. The next doctor reading this history has no idea whether the story was flaky, redundant, or wrong, and whether resurrecting a story here is safe or a known trap. The frontend guideline (site/AGENTS.md) prefers stories for component testing; the renderHook-without-DOM carve-out makes this unit test legitimate, but nobody wrote that reasoning down. One body paragraph on d404f52488 saying why the story lost to the unit test would close the gap. (If this PR squash-merges, the intermediate history vanishes, but the reviewer reading the branch today hits the contradiction either way.)"
Orchestrator judgment: location is nominal; the issue lives in the commit metadata.
🤖
🤖 This review was automatically generated with Coder Agents.
| clearChatErrorReason, | ||
| aiGatewayDisabled, | ||
| }); | ||
| const fetchOlderMessages = () => { |
There was a problem hiding this comment.
P2 [CRF-7] The fetchOlderMessages wrapper, the only place the fix actually runs in production, has zero test coverage, and neither do the four private query-core 5.82.0 behaviors the fix's correctness rests on. (Bisky P2, Hisoka P2, Mafu-san P3, Mafuuu P3, Pariston P3, Meruem P3, Nami P3, Kite P3)
Bisky: "The unit test in chatStore.test.tsx re-implements the wiring by hand: the test itself calls chatPaginationEpochs.open, simulates the settle with a raw setQueryData, calls close, and calls replayChatCacheWrites. So it proves the pieces, never the assembly. If the wrapper shipped with close called with the wrong generation, the replay outside the finally, or no replay at all, every test in this PR stays green while the fixed bug quietly returns."
Hisoka: "The four, each verified in the installed source, none a public contract: (1) fetchNextPage snapshots state.data.pages at fetch start and settles with snapshot + page; (2) the cache commit happens before the fetch promise resolves (retryer.ts calls config.onSuccess at line 107 before thenable.resolve at 109), which is what makes the finally replay run after the clobber; (3) React notifications go through defaultScheduler = (cb) => setTimeout(cb, 0) (notifyManager.ts:13); (4) re-entrant fetchNextPage is cancel-and-restart with both promises settling. ... Upgrade @tanstack/query and change any one of the four and every test still passes while the stale-render race returns, or the replay starts landing after a render."
Orchestrator judgment: eight reviewers converged on this from two angles (revert vector, dependency-bump vector) with one shared fix, so the convergence itself is the signal. Severity held at P2 over the five P3 votes because the revert vector is not a transient flash: reverting line 2030 to chatMessagesQuery.fetchNextPage restores the exact user-visible bug permanently with a fully green suite. The fix all eight reviewers converged on: one integration test that drives a real useInfiniteQuery with a deferred queryFn through the real wrapper, delivers a durable write mid-flight, resolves, and asserts the settled cache. Bisky sketched it as an extracted helper with three cases (happy path, re-entrant cancel-and-restart, rejection) in roughly forty lines using #/testHelpers/deferred.
🤖
There was a problem hiding this comment.
Fixed in 82a9dd4. The protocol is now owned by a single exported helper, fetchChatMessagesPageWithReplay (useChatStore.ts), and two assembly tests in chatStore.test.tsx drive a real useInfiniteQuery with a deferred queryFn through that exact production path: "restores a durable write clobbered by a real pagination settle" and "re-applies a history replacement that a pagination settle clobbers". Verified the revert vector is caught: replacing the helper with a bare fetchNextPage() call makes both tests fail; with the fix they pass. On the dependency-bump vector: the four behaviors are private implementation details, so the assembly tests are the executable spec that a bump runs; the module header now pins the version and upstream issue so the bump reviewer knows what to re-verify.
Reply generated by Coder Agents.
| clearChatErrorReason, | ||
| aiGatewayDisabled, | ||
| }); | ||
| const fetchOlderMessages = () => { |
There was a problem hiding this comment.
P2 [CRF-8] Only user-initiated pagination opens an epoch; remount, reconnect, and invalidation-driven refetches of the same messages query clobber concurrent writes through the identical snapshot-and-settle mechanism, and the deferral has no ticket. (Hisoka P2, Komugi P3, Mafu-san P3, Mafuuu P3, Pariston P3, Meruem P3, Nami P3, Kite P3)
Hisoka: "record is a no-op without an open epoch, and nothing but fetchOlderMessages ever opens one. The messages query has no staleTime (chats.ts:1170) and the global client only disables refetchOnWindowFocus (App.tsx:19-26), so refetchOnReconnect stays at its default of true. When the network blips: the infinite query refetches every page from the oldPages snapshot, and the reconnecting WebSocket simultaneously replays the durable-message snapshot into upsertCacheMessages/replaceCacheMessages. The settle discards those writes, no epoch, no replay, and the store sync effect renders stale text. This is the PR's own bug, reproduced verbatim by the fetch initiator most likely to coincide with a write burst."
Kite adds the sharper interaction: "An invalidation refetch on an active query runs query.fetch with cancelRefetch: true, which ... if a pagination epoch is open, silently cancels the in-flight fetchNextPage, whose finally closes the epoch and replays while the unprotected refetch is still in flight. So deleting a queued message while messages stream can reproduce the stale-text symptom this PR fixes, through a trigger the fix does not cover."
Orchestrator judgment: kept at P2 on Hisoka's reconnect argument (the trigger most correlated with a write burst) plus Kite's epoch-cancellation interaction; the seven P3 votes argue the window is narrow and self-healing, which is true per-instance, but this is the same defect class the PR exists to kill and the PR's own Known Gaps defers it to "Phase 2 item 9" of CHATS_QUERY_ARCHITECTURE.md, a document that does not exist in this repository (see CRF-6), with no linked ticket. A deferral without a ticket is a drop: this needs a tracked issue, or a human decision that the residual window is accepted. Meruem also sketched the structural fix that eliminates the class: drive epochs from the query cache fetch lifecycle (subscribe to queryClient.getQueryCache() events for chatMessagesKey) instead of from callers, which covers every fetch type and removes the per-callsite protocol entirely.
🤖
There was a problem hiding this comment.
Acknowledged, and agreed this is the same defect class, not a new one. The human in the loop has filed a ticket covering reconnect, remount, and invalidation-driven refetches of the messages query; it cannot be linked from this PR. The structural fix (cache-lifecycle-driven epochs rather than caller-driven) belongs there, together with the Phase 2 item 9 cancellation work, not in this PR. Known gaps in the PR body now states this deferral explicitly instead of pointing at a document outside the repository.
Reply generated by Coder Agents.
| // above must also be buffered for replay. The prompt and | ||
| // search invalidations run once here and are not repeated | ||
| // on replay. | ||
| chatPaginationEpochs.record(chatID, { |
There was a problem hiding this comment.
P3 [CRF-1] The queued-messages cache writers are left clobber-prone during an open epoch with no ticket for the gap. (Netero)
"Only upsertCacheMessages (line 242) and replaceCacheMessages (line 262) record into the epoch buffer. writeQueuedMessagesToCache (defined line 72, called from the queue_update handler at line 638 and setCacheQueuedMessages at line 812) writes page-0 queued_messages via patchChatMessages and is not buffered, so a queue_update landing during a pagination fetch is silently reverted by the settle. The PR body acknowledges this ('Known gaps') and argues the impact is bounded (store authoritative while mounted, re-hydration heals on remount); I traced the hydrate guard at lines 377-382 and the bound holds while mounted, but the cache stays stale after unmount until the next refetch or queue_update. The deferral has no linked ticket. A deferral without a ticket is a drop; a human should decide whether the gap is acceptable, so it is recorded here as a finding."
Orchestrator judgment: verified against the record call sites. This one is at least named in the PR body's Known gaps, unlike CRF-2, but it still needs a ticket or an explicit human decision.
🤖
There was a problem hiding this comment.
Acknowledged. This gap is now named in the PR body's Known gaps section, and the human in the loop has filed a ticket covering the uncovered writers; it cannot be linked from this PR. The hydrate-guard bound you traced holds, and the ticket is where the decision to buffer queued-message writes (or not) will be made alongside the epoch-ownership work.
Reply generated by Coder Agents.
| // above must also be buffered for replay. The prompt and | ||
| // search invalidations run once here and are not repeated | ||
| // on replay. | ||
| chatPaginationEpochs.record(chatID, { |
There was a problem hiding this comment.
P3 [CRF-2] The editChatMessage mutation's cache patches are an unacknowledged sibling of the fixed bug class: they write chatMessagesKey mid-flight and are not buffered on the epoch. (Netero)
"editChatMessage in site/src/api/queries/chats.ts patches the messages cache in onMutate (optimistic truncation), onSuccess (server reconciliation), and onError (rollback), all via patchChatMessages. onMutate cancels in-flight messages fetches (cancelChatMessages), which protects the start of the mutation, but a pagination fetch started after the mutation began (user scrolls up while an edit is in flight) snapshots the cache, and the onSuccess/onError patch landing mid-fetch is clobbered by the settle with nothing to replay it. This is the exact defect class this PR fixes for WS durable writes. Consequence is bounded because the post-edit WS FullRefresh arrives through replaceCacheMessages, which is buffered, so the durable heal survives; hence P3 rather than P2. Not verified with a runtime reproduction; verified by reading the mutation callbacks and the epoch record sites. The PR's Known gaps section does not mention this writer."
Orchestrator judgment: distinct from CRF-1 (which is acknowledged in the PR body); this writer is unacknowledged. If CRF-12 (record at the funnel) is adopted, both findings close mechanically.
🤖
There was a problem hiding this comment.
Acknowledged and now documented. The PR body's Known gaps section names the editChatMessage patches explicitly, including the bounded-impact reasoning you gave: the post-edit durable history replacement arrives through the buffered replaceCacheMessages path, so the heal survives. Per the human in the loop this stays documented rather than ticketed. Recording raw updaters mechanically was evaluated and rejected because onError's full-snapshot restore (() => context.previousData) would drop the fetched page if replayed after a settle.
Reply generated by Coder Agents.
| writes: readonly ChatCacheWrite[], | ||
| ): void => { | ||
| for (const write of writes) { | ||
| if (write.kind === "replace") { |
There was a problem hiding this comment.
P3 [CRF-3] The replace branch of replayChatCacheWrites is never executed by any test. (Netero)
"Verified by reading both test files: chatStore.test.tsx:1210 calls replayChatCacheWrites with a buffer produced only through upsertCacheMessages (kind \"upsert\"), and paginationEpoch.test.ts exercises supersession on its own TestWrite objects without touching replayChatCacheWrites. The replace branch has the riskiest semantics in the change: replaceChatMessagesHistory collapses the cache to a single page with has_more: false (chats.ts, replaceMessagesHistory), which on replay discards the just-fetched older page. That behavior is intended supersedence per the design, but it ships untested. Fix: extend the chatStore race test (or add one) that records a replace during an open epoch and asserts the settled cache after replay."
Orchestrator judgment: Bisky independently confirmed the branch is unexecuted. If CRF-10 (Robin) is adopted and verbatim replay replaces supersession, this branch still needs the same test, so the findings do not cancel.
🤖
There was a problem hiding this comment.
Fixed in 82a9dd4. New test "re-applies a history replacement that a pagination settle clobbers" in chatStore.test.tsx drives a real useInfiniteQuery with a deferred queryFn, emits a history_reset batch mid-flight through the mocked socket so the replace write lands on the real replaceCacheMessages path, then resolves the fetch and asserts the settled cache collapses back to the replacement's single page with has_more: false. With the helper reverted to a bare fetchNextPage, the test fails.
Reply generated by Coder Agents.
| epochs.set(chatID, { refCount: 1, generation, buffer: [] }); | ||
| return generation; | ||
| }, | ||
| record: (chatID: string, write: TWrite): void => { |
There was a problem hiding this comment.
Nit [CRF-25] record silently drops the write when no epoch is open, and nothing at the declaration says so. (Gon)
"The call sites (useChatStore.ts:242, :262) read as unconditional buffering; the drop-when-closed behavior is only discoverable from epochs.get(chatID)?.buffer.push(...) or the test 'ignores records when no epoch is open'. This is the function's primary contract, since the common case is no epoch. Add one doc line, 'No-op when no epoch is open for the chat,' or rename to recordIfOpen."
🤖
There was a problem hiding this comment.
Fixed in 82a9dd4. record carries the doc line "No-op when no epoch is open for the chat." Chose the doc line over recordIfOpen since the no-op is the common case and the call sites read as intended with it.
Reply generated by Coder Agents.
| // unmount still settles the stale snapshot after the component is gone, | ||
| // and the fetch wrapper's finally replay is the only path that heals the | ||
| // departed chat's cache. | ||
| export const chatPaginationEpochs = |
There was a problem hiding this comment.
Note [CRF-27] chatPaginationEpochs is a module singleton mutated by tests with no reset API and no afterEach cleanup. (Hisoka)
"If an assertion in the chatStore replay test throws between open and close, the epoch leaks open into every later test in the file; buffered writes for that chatID accumulate inert. Consequence today is only a confusing failure cascade after a first failure, since each test uses a distinct chatID. Worth a reset() on the manager the day a second test file touches it."
🤖
There was a problem hiding this comment.
Noted. No change today, per the finding's own framing: tests use distinct chatIDs and exactly one test file touches the epoch module. A reset() goes in with the second consumer.
Reply generated by Coder Agents.
| } | ||
| const generation = ++generationCounter; | ||
| epochs.set(chatID, { refCount: 1, generation, buffer: [] }); | ||
| return generation; |
There was a problem hiding this comment.
Note [CRF-28] An epoch stays open for as long as its fetch is paused, buffering every durable write with no bound. (Komugi)
"With the default networkMode: 'online', a fetchNextPage started while offline (or interrupted by going offline) parks in paused and its promise stays pending until reconnect; the epoch stays open the whole time and every durable batch is appended to the buffer. The UI stays correct because writes still apply immediately, supersedence trims the buffer only at close, and the eventual settle replays and heals, so the cost is memory growth proportional to durable traffic during the outage plus a replay of the whole surviving buffer on reconnect. Bounded by session length; no user-visible failure. Worth knowing if long-lived offline sessions become a target."
🤖
There was a problem hiding this comment.
Noted. No change: writes apply immediately so the UI stays correct, and buffer growth is bounded by session length plus durable traffic during the outage. Worth revisiting if long-lived offline sessions become a target.
Reply generated by Coder Agents.
| @@ -0,0 +1,65 @@ | |||
| // fetchNextPage snapshots state.data.pages when the fetch starts and | |||
There was a problem hiding this comment.
Note [CRF-29] The codebase now has two mechanisms for the same bug class with nothing pointing between them. (Zoro)
"cancelChatListRefetches (api/queries/chats.ts:815-823) is the cancel-the-refetch answer for background refetches; this module is the buffer-and-replay answer for pagination fetches that must not be cancelled. The next engineer hitting a WS-write-vs-fetch race has to rediscover which mechanism applies. One sentence in this file's header naming the cancel convention and why it does not apply to pagination would close that gap; the PR's deferred Phase 2 item (cancelling non-pagination message refetches) is exactly the point where someone will need it."
🤖
There was a problem hiding this comment.
Fixed in 82a9dd4. The paginationEpoch.ts header names the cancel convention (cancelChatListRefetches) for background refetches and states why pagination is different: the fetch is user-requested, so it must settle rather than be cancelled. That is exactly where the Phase 2 refetch-cancellation reader will look.
Reply generated by Coder Agents.
| let generationCounter = 0; | ||
| return { | ||
| open: (chatID: string): number => { | ||
| const epoch = epochs.get(chatID); |
There was a problem hiding this comment.
Note [CRF-30] The get-or-create branch in open is the exact shape Map.prototype.getOrInsertComputed targets; not usable yet. (Ging-ts)
"TypeScript 6.0 ships the types in lib.esnext.collection (this project's lib: esnext picks them up), but the runtime method hasn't landed. Verified typeof Map.prototype.getOrInsert is undefined on the workspace's Node 22.19, so the type checker would accept a call that throws at runtime in both tests and browsers. Nothing to change now; worth knowing the compiler won't catch it if someone reaches for it early."
🤖
There was a problem hiding this comment.
Noted, no change. Agreed: the types land before the runtime method, so the current get-or-create branch stays.
Reply generated by Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 is blocked on one silent finding; no new review this round.
The churn check against 82a9dd4 shows strong progress: 18 of 30 round-1 findings are addressed in the new commit (the fetchChatMessagesPageWithReplay extraction, both assembly tests through a real useInfiniteQuery, verbatim replay with the fuzz-verified equivalence, the pinned 5.82.0/#3579 citation, the facade re-export, and the comment trims all check out against the diff), 8 are acknowledged with substantive responses, and 3 are contested with real technical defenses (CRF-6, CRF-12, CRF-23) that the panel will weigh next round.
The blocker: CRF-21 (commit d404f52488 deletes the story harness its parent commit's body advertises, with no body explaining why) received no reply and no code change. It was folded into the round-1 review body rather than posted as a threaded comment, which is likely why it was missed; that placement is on us. What is needed: one sentence here acknowledging the finding, and either a body on the commit explaining why the story lost to the unit test, or a statement that the squash-merge makes the intermediate history moot.
Further review is blocked until CRF-21 gets a response. Once it does, the next round proceeds directly to the panel: verification of the 18 claimed fixes, evaluation of the three contested findings, and a fresh pass over the new assembly tests.
🤖 This review was automatically generated with Coder Agents.
|
Re: CRF-21 from the round-2 review. Acknowledged, and no code change. Substantively: the story was deleted in d404f52 because its deterministic ordering requirement (the durable WS event must land while the deferred fetch is held) forced a shared-harness extension for a single consumer. The pagination race is pinned without it by the unit tests in chatStore.test.tsx, which drive the production helper (now fetchChatMessagesPageWithReplay) through a real useInfiniteQuery with a deferred queryFn, and the harness files were reverted byte-identical to main. The subject line of d404f52 says so; the body should have carried the why as well, and that is the fair part of the finding. On the history itself: coder/coder is squash-merge only (allow_squash_merge is the sole enabled method), so all four branch commits collapse into one PR-titled commit on merge. The intermediate commit bodies never reach main's history, which makes the inconsistency moot rather than worth a force-push rewrite, consistent with the CRF-22 decision on this branch. Reply generated by Coder Agents. |
|
/coder-agents-review |
|
@codex review |
![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: 82a9dd494c
ℹ️ 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 fetchOlderMessages = () => { | ||
| if (!agentId) { | ||
| return; | ||
| } | ||
| fetchChatMessagesPageWithReplay( |
There was a problem hiding this comment.
When loading an older page overlaps a WebSocket message or history reset, this callback changes the visible transcript by restoring the newer write, but the commit only exercises that behavior through renderHook tests. Add or update a .stories.tsx story whose play function triggers pagination and the concurrent update so the UI-level invariant is covered as FE1 requires.
AGENTS.md reference: site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Won't fix. Story-level coverage of this race was built (story DurableUpdateSurvivesPaginationFetch), reviewed, and deliberately removed because its deterministic ordering requirement (the durable event must land while the deferred pagination fetch is held) forced a shared Storybook WebSocket-harness extension for a single consumer. The race is pinned at unit level instead, through the production path: two assembly tests in chatStore.test.tsx drive a real useInfiniteQuery with a deferred queryFn through the exported fetchChatMessagesPageWithReplay helper, covering both the upsert and history-replacement branches. Red-green verified: replacing the helper with a bare fetchNextPage() call makes both tests fail; with the fix they pass. The UI render branch this callback feeds is already covered by the existing durable-path stories (e.g. DurableUpdateFansOutToOlderPage); what changed here is cache reconciliation timing, which is exactly what the unit tests pin.
Reply generated by Coder Agents.
| () => { | ||
| const messagesQuery = useInfiniteQuery({ | ||
| queryKey: chatMessagesKey(chatID), | ||
| initialPageParam: undefined as number | undefined, |
There was a problem hiding this comment.
Both new infinite-query test setups force undefined through as number | undefined, even though the value can be typed without an assertion. Replace these assertions with an explicit typed variable or appropriate query generic so the tests continue checking the actual type contract instead of bypassing it, as required by FE2.
AGENTS.md reference: site/AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 21aca50. Both undefined as number | undefined assertions are replaced by a single module-level const initialMessagePageParam: number | undefined = undefined with a two-line comment explaining that the constant exists so the setups exercise the real initialPageParam type contract rather than forcing it through an assertion. Both useInfiniteQuery setups now reference the typed constant.
Reply generated by Coder Agents.
| epochs.set(chatID, { refCount: 1, generation, buffer: [] }); | ||
| return generation; | ||
| }, | ||
| // No-op when no epoch is open for the chat. |
There was a problem hiding this comment.
This comment only paraphrases the immediately visible epochs.get(chatID)?.buffer.push(write) control flow and does not document a non-obvious invariant or constraint. Remove it to comply with FE4's prohibition on comments that restate identifiers or control flow.
AGENTS.md reference: site/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Won't fix. The comment was added in response to round-1 finding CRF-25, which flagged that record's silent drop when no epoch is open is the function's primary contract and was only discoverable by reading epochs.get(chatID)?.buffer.push(write). The line states that contract (the common case is no epoch and the call is a deliberate no-op), not the control flow; the call sites read as unconditional buffering without it. Removing it would re-open CRF-25.
Reply generated by Coder Agents.
There was a problem hiding this comment.
Round 3. Panel of six (Bisky, Hisoka, Pariston, Komugi, Meruem, Knov) against the unchanged head 82a9dd4.
All three contested findings from round 2 are now closed. On CRF-12, Pariston (who raised the funnel-recording proposal in round 1) evaluated your defense against the code and withdrew the finding: replaying editChatMessage.onError's whole-snapshot restore after a settle installs the pre-mutation snapshot captured before the fetched page existed, and with global retry: false no refetch is guaranteed to heal it. The funnel refactor is a worse bug, not just a worse shape; value recording at the two WS callsites stands as the deliberate design. CRF-23's defense was verified by inspection (16 copies with gcTime: Infinity vs the shared helper's gcTime: 0; you added 2, 14 pre-existed), and the deferral to a dedicated refactor is proportionate. CRF-6 is closed with one Note-level residual: the PR body's Fix section still cites CHATS_QUERY_ARCHITECTURE.md in its opening line even though Known gaps no longer does; striking "per CHATS_QUERY_ARCHITECTURE.md" from that header makes the body self-consistent. Squash-merge bounds the impact to review-time readers.
Round-1 fix claims were verified where the panel encountered the code: the replace-branch assembly test exercises the real replaceCacheMessages path end-to-end (CRF-3), the epoch protocol pairing now lives in one helper (CRF-11), the garbled callsite comment is gone (CRF-14), and the "only path... before the next refetch" qualifier now reads honestly (CRF-20). Komugi re-walked the settle ordering through the installed query-core 5.82.0 source (retryer commit before promise resolution, setTimeout(0) notify macrotask, .catch(noop) on the observer) and confirmed the two new assembly tests have no wall-clock waits, no unseeded randomness, and no shared state. Bisky and Hisoka found nothing new.
One new Nit below, raised independently by two reviewers. No P0-P3 this round.
The residual deferred class (CRF-1, CRF-2, CRF-8) remains as acknowledged in round 2: ticketed per the human in the loop, documented in Known gaps. Nothing further needed from this PR on those.
🤖 This review was automatically generated with Coder Agents.
| for (const write of writes) { | ||
| if (write.kind === "replace") { | ||
| replaceChatMessagesHistory(queryClient, chatID, write.messages); | ||
| } else { |
There was a problem hiding this comment.
Nit [CRF-31] replayChatCacheWrites discriminates ChatCacheWrite.kind with if (kind === "replace") ... else ..., so a future variant added to the union compiles clean and is silently routed through upsertChatMessages. (Meruem, Knov)
Meruem: "CRF-9's fix hardened the declaration by making kind: \"upsert\" | \"replace\", which prevents an unrecognized string from being written. It did not harden the dispatch: adding \"delete\" to the union typechecks against write.messages at both callsites and falls into the else branch here unchanged, calling upsertChatMessages(queryClient, chatID, write.messages) on messages the caller meant to remove. The rest of this directory (and the sibling AgentChatPageView.tsx) already uses the project convention for this class of dispatch: const _exhaustive: never = X on the fallthrough (ConversationTimeline.tsx:124, streamState.ts:219, messageParsing.ts:302, AgentChatPageView.tsx:309)."
Orchestrator judgment: verified the if/else at useChatStore.ts:48-53 and the _exhaustive: never convention at the cited sites. Consequence today is nil (two kinds, both handled); the finding is that the mechanical guard the surrounding module already applies to discriminated unions is absent here. Rewrite as switch (write.kind) with a const _exhaustive: never = write.kind; default so the misroute class is caught at compile time.
🤖
There was a problem hiding this comment.
Fixed in 21aca50. replayChatCacheWrites now dispatches with switch (write.kind) over explicit replace and upsert cases, and a default containing const _exhaustive: never = write.kind plus a throw, matching the convention at ConversationTimeline.tsx:124, streamState.ts:219, messageParsing.ts:302, and AgentChatPageView.tsx:309. A future union variant now fails at compile time instead of silently routing through upsertChatMessages.
Reply generated by Coder Agents.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
1 participant
Problem
fetchNextPageon the messages infinite query snapshotsstate.data.pagesat fetch start and settles with snapshot + new page, discarding anysetQueryDatamade during the fetch (verified in the installed @tanstack/query-core 5.82.0, upstream TanStack/query#3579). A durable WebSocket message batch for a message in a loaded page, arriving while an older page is fetching, was therefore clobbered when the fetch settled. The store sync effect then classified the missing ID as stale and rendered the stale text until a later refetch.Fix
Buffer-and-replay across pagination via a per-chat pagination epoch:
AgentChatPagecallsfetchChatMessagesPageWithReplay(exported fromuseChatStore.ts, re-exported via thechatStore.tsfacade) instead of the rawchatMessagesQuery.fetchNextPage. The helper opens a per-chat epoch, awaitsfetchNextPage, and replays buffered writes infinally, keeping the open/close/replay pairing invariant in one place. Re-entrantfetchNextPageis cancel-and-restart in v5.82.0, so the epoch is refcounted: a cancelled call'sfinallydecrements without replaying when the count does not reach zero.upsertCacheMessagesandreplaceCacheMessagesstill write to the cache immediately for low stream latency, and additionally buffer only the cache-write call (upsertChatMessages/replaceChatMessagesHistory). The prompt/search invalidation fan-out runs once at write time and is not repeated on replay.replaceChatMessagesHistoryrebuilds the cache purely from its input, so verbatim replay is naturally last-write-wins; no explicit supersedence rule is needed (validated by a 200k-iteration fuzz equivalence check). Replay is synchronous within the settle continuation. The settle commit happens before thefetchNextPagepromise resolves, and React notifications go through the notifyManagersetTimeout(0)scheduler, so the replay completes before any render can observe the stale snapshot.KeyedAgentChatPageremounts on agentId change so chatID never changes within a mount. The epoch state is module-level, so unmount with a fetch in flight is covered: the queryFn never consumes the abort signal, the background fetch still settles the stale snapshot, and thefinallyreplay (closure-captured chatID and buffer) heals the departed chat's cache.Tests
site/src/pages/AgentsPage/components/ChatConversation/paginationEpoch.test.ts: unit tests for the replay/supersede helper and the epoch manager (refcount, generation guard, per-chat isolation).site/src/pages/AgentsPage/components/ChatConversation/chatStore.test.tsx: "replays a buffered durable write after a pagination settle clobbers it" drives the race deterministically at unit level. It opens an epoch, delivers a durable upsert through the hook's real buffering path, simulates the settle clobber, closes the epoch, replays, and asserts the update is restored on the settled cache while the fetched page survives.chatStore.test.tsxalso has two assembly tests that drive a realuseInfiniteQuerywith a deferredqueryFnthrough the production helper: "restores a durable write clobbered by a real pagination settle" (upsert branch) and "re-applies a history replacement that a pagination settle clobbers" (replace branch). These catch the revert vector: replacing the helper with rawfetchNextPagemakes both fail.fetchNextPagecall, both assembly tests fail; with the fix, they pass.Known gaps
writeQueuedMessagesToCache,setCacheQueuedMessages) are also clobber-prone during an open epoch, but the impact is bounded: the store is authoritative while mounted and the hydrate guard blocks stale re-hydration until remount, where the nextqueue_updateheals it.editChatMessagemutation cache patches (optimistic truncation, server reconciliation, rollback) are not buffered on the epoch. Impact is bounded because the post-edit durable history replacement arrives through the bufferedreplaceCacheMessagespath, but a pagination fetch started after the mutation began can clobber the optimistic patch until the durable heal.PR generated by Coder Agents.