mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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.
This commit is contained in:
+32
-9
@@ -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
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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<number, TypesGen.ChatMessage> =>
|
||||
@@ -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
|
||||
|
||||
@@ -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 <ChatPageTimeline store={store} persistedError={undefined} />;
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
expect(canvas.getByTestId("conversation-timeline")).toHaveTextContent(
|
||||
/alpha[\s\S]*bravo[\s\S]*charlie[\s\S]*delta/,
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user