From fb306748069015130b5d49aa36c921b84fb86787 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:43:11 +0200 Subject: [PATCH] fix(site/src/pages/AgentsPage): order chat transcript by message id (#27620) ## Context Follow-up to #27495 (append-order guarantee for `chat_messages.id`) and #27619 (prompt query ordering), both merged. This PR applies the same id ordering to the transcript the user actually sees. ## Why? `buildOrderedMessageIDs` in `chatStore.ts` sorted by `created_at`, which is `now()` and therefore shared by every row in an insert batch. It builds `orderedMessageIDs`, which is what the transcript renders, so it re-imposed the ordering the backend PRs remove. The failure needs the merge path, not a plain fetch. Initial REST hydration already sorts by numeric id before reaching the store, and `Array.prototype.sort` is stable, so a single correctly ordered response rendered correctly. But `upsertDurableMessages` copies the existing message `Map`, appends new ids, and re-sorts. `Map` iteration is insertion ordered, so when a refetch or reconnect merges earlier ids into a map that already holds later ones, the stable timestamp sort faithfully preserves the wrong order. This also makes the store consistent with `useChatStore.ts` and `api/queries/chatMessageEdits.ts`, which already sort by `id`. ## Changes `buildOrderedMessageIDs` now calls `toSorted` with an `id` comparator inlined at its single call site, and the `byMessageCreatedAt` helper is gone. `ChatMessage.id` is a `number` in `typesGenerated.ts`, backed by a Go `int64`, so numeric subtraction is correct. ## Testing Two vitest cases, both verified red by restoring the timestamp comparator: - `sorts messages by id when created_at disagrees with append order` returned `[2,1]`. - `orders merged messages by id rather than by arrival` returned `[3,4,1,2]`, the exact inversion the merge path produces. `MergedMessagesRenderInIDOrder` in `ChatPageContent.stories.tsx` covers the same merge path through the rendered timeline. All 334 `ChatConversation` unit tests and the 4 `ChatPageContent` storybook interaction tests pass, and `tsc -p .` plus biome are clean. > Opened by Mux on behalf of Mike. --- .../chatStore.createStore.test.ts | 41 +++++++++++++++---- .../components/ChatConversation/chatStore.ts | 13 +----- .../components/ChatPageContent.stories.tsx | 33 +++++++++++++++ 3 files changed, 67 insertions(+), 20 deletions(-) diff --git a/site/src/pages/AgentsPage/components/ChatConversation/chatStore.createStore.test.ts b/site/src/pages/AgentsPage/components/ChatConversation/chatStore.createStore.test.ts index 801524047e..2087397b8e 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/chatStore.createStore.test.ts +++ b/site/src/pages/AgentsPage/components/ChatConversation/chatStore.createStore.test.ts @@ -6,8 +6,6 @@ import { createChatStore, selectIsAwaitingFirstStreamChunk } from "./chatStore"; // Helpers // --------------------------------------------------------------------------- -/** Minimal ChatMessage factory. `created_at` is derived from `id` to make - * ordering deterministic in tests that care about sort order. */ const makeMessage = ( id: number, role: string, @@ -53,19 +51,19 @@ describe("replaceMessages", () => { expect(state.orderedMessageIDs).toEqual([1, 2]); }); - it("sorts messages by created_at", () => { + it("sorts messages by id when created_at disagrees with append order", () => { const store = createChatStore(); - const older = { + const first = { ...makeMessage(1, "user", "first"), - created_at: "2025-01-01T00:00:01.000Z", - } as TypesGen.ChatMessage; - const newer = { - ...makeMessage(2, "assistant", "second"), created_at: "2025-01-01T00:00:05.000Z", } as TypesGen.ChatMessage; + const second = { + ...makeMessage(2, "assistant", "second"), + created_at: "2025-01-01T00:00:01.000Z", + } as TypesGen.ChatMessage; // Insert in reverse order. - store.replaceMessages([newer, older]); + store.replaceMessages([second, first]); expect(store.getSnapshot().orderedMessageIDs).toEqual([1, 2]); }); @@ -98,6 +96,31 @@ describe("replaceMessages", () => { }); }); +describe("upsertDurableMessages", () => { + it("orders merged messages by id rather than by arrival", () => { + const store = createChatStore(); + const sharedCreatedAt = "2025-01-01T00:00:00.000Z"; + const withSharedCreatedAt = ( + id: number, + role: string, + ): TypesGen.ChatMessage => ({ + ...makeMessage(id, role, `message-${id}`), + created_at: sharedCreatedAt, + }); + + store.replaceMessages([ + withSharedCreatedAt(3, "assistant"), + withSharedCreatedAt(4, "tool"), + ]); + store.upsertDurableMessages([ + withSharedCreatedAt(1, "user"), + withSharedCreatedAt(2, "assistant"), + ]); + + expect(store.getSnapshot().orderedMessageIDs).toEqual([1, 2, 3, 4]); + }); +}); + // --------------------------------------------------------------------------- // upsertDurableMessage // --------------------------------------------------------------------------- diff --git a/site/src/pages/AgentsPage/components/ChatConversation/chatStore.ts b/site/src/pages/AgentsPage/components/ChatConversation/chatStore.ts index 428e93cc08..886524feed 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/chatStore.ts +++ b/site/src/pages/AgentsPage/components/ChatConversation/chatStore.ts @@ -7,15 +7,6 @@ import { import { applyMessagePartToStreamState } from "./streamState"; import type { ReconnectState, RetryState, StreamState } from "./types"; -const byMessageCreatedAt = ( - left: TypesGen.ChatMessage, - right: TypesGen.ChatMessage, -): number => { - return ( - new Date(left.created_at).getTime() - new Date(right.created_at).getTime() - ); -}; - const buildMessageMap = ( messages: readonly TypesGen.ChatMessage[], ): Map => @@ -24,8 +15,8 @@ const buildMessageMap = ( const buildOrderedMessageIDs = ( messages: readonly TypesGen.ChatMessage[], ): readonly number[] => { - const sorted = [...messages]; - sorted.sort(byMessageCreatedAt); + // created_at is shared across an insert batch, so only id tracks append order. + const sorted = messages.toSorted((left, right) => left.id - right.id); // Deduplicate by ID. The input can contain duplicate IDs when // cross-page duplication occurs in the React Query cache (e.g. // upsertCacheMessages writes to page 0 while the same message diff --git a/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx b/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx index 17e3f18340..0ffe7e873c 100644 --- a/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatPageContent.stories.tsx @@ -113,3 +113,36 @@ export const HiddenAssistantPlaceholderDoesNotRender: Story = { expect(rows[1]).toHaveTextContent("Done."); }, }; + +export const MergedMessagesRenderInIDOrder: Story = { + render: () => { + const store = createChatStore(); + // One created_at for all four, so id is the only ordering signal. + const batchCreatedAt = new Date(FIXTURE_NOW).toISOString(); + const batched = ( + id: number, + role: TypesGen.ChatMessageRole, + text: string, + ): TypesGen.ChatMessage => ({ + ...buildMessage(id, role, [{ type: "text", text }]), + created_at: batchCreatedAt, + }); + + store.replaceMessages([ + batched(3, "user", "charlie"), + batched(4, "assistant", "delta"), + ]); + store.upsertDurableMessages([ + batched(1, "user", "alpha"), + batched(2, "assistant", "bravo"), + ]); + + return ; + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByTestId("conversation-timeline")).toHaveTextContent( + /alpha[\s\S]*bravo[\s\S]*charlie[\s\S]*delta/, + ); + }, +};