Skip to content

Commit 9113255

Browse files
fix(app): order stored messages by creation time (#41001)
Co-authored-by: Dax <mail@thdxr.com>
1 parent 23cc677 commit 9113255

7 files changed

Lines changed: 89 additions & 65 deletions

File tree

packages/app/src/context/global-sync/event-reducer.test.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@ const rootSession = (input: { id: string; parentID?: string; archived?: number }
1515
},
1616
}) as Session
1717

18-
const userMessage = (id: string, sessionID: string) =>
18+
const userMessage = (id: string, sessionID: string, created = 1) =>
1919
({
2020
id,
2121
sessionID,
2222
role: "user",
23-
time: { created: 1 },
23+
time: { created },
2424
agent: "assistant",
2525
model: { providerID: "openai", modelID: "gpt" },
2626
}) as Message
@@ -370,28 +370,28 @@ describe("applyDirectoryEvent", () => {
370370
const sessionID = "ses_1"
371371
const [store, setStore] = createStore(
372372
baseState({
373-
message: { [sessionID]: [userMessage("msg_1", sessionID), userMessage("msg_3", sessionID)] },
374-
part: { msg_2: [textPart("prt_1", sessionID, "msg_2")] },
373+
message: { [sessionID]: [userMessage("msg_z", sessionID, 1), userMessage("msg_b", sessionID, 3)] },
374+
part: { msg_a: [textPart("prt_1", sessionID, "msg_a")] },
375375
}),
376376
)
377377

378378
applyDirectoryEvent({
379-
event: { type: "message.updated", properties: { info: userMessage("msg_2", sessionID) } },
379+
event: { type: "message.updated", properties: { info: userMessage("msg_a", sessionID, 2) } },
380380
store,
381381
setStore,
382382
push() {},
383383
directory: "/tmp",
384384
loadLsp() {},
385385
})
386386

387-
expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_1", "msg_2", "msg_3"])
387+
expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_z", "msg_a", "msg_b"])
388388

389389
applyDirectoryEvent({
390390
event: {
391391
type: "message.updated",
392392
properties: {
393393
info: {
394-
...userMessage("msg_2", sessionID),
394+
...userMessage("msg_a", sessionID, 2),
395395
role: "assistant",
396396
} as Message,
397397
},
@@ -403,19 +403,19 @@ describe("applyDirectoryEvent", () => {
403403
loadLsp() {},
404404
})
405405

406-
expect(store.message[sessionID]?.find((x) => x.id === "msg_2")?.role).toBe("assistant")
406+
expect(store.message[sessionID]?.find((x) => x.id === "msg_a")?.role).toBe("assistant")
407407

408408
applyDirectoryEvent({
409-
event: { type: "message.removed", properties: { sessionID, messageID: "msg_2" } },
409+
event: { type: "message.removed", properties: { sessionID, messageID: "msg_a" } },
410410
store,
411411
setStore,
412412
push() {},
413413
directory: "/tmp",
414414
loadLsp() {},
415415
})
416416

417-
expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_1", "msg_3"])
418-
expect(store.part.msg_2).toBeUndefined()
417+
expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_z", "msg_b"])
418+
expect(store.part.msg_a).toBeUndefined()
419419
})
420420

421421
test("upserts and prunes message parts", () => {

packages/app/src/context/global-sync/event-reducer.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type { State, VcsCache } from "./types"
1515
import { trimSessions } from "./session-trim"
1616
import { dropSessionCaches } from "./session-cache"
1717
import { diffs as list, message as clean } from "@/utils/diffs"
18+
import { messageKey } from "@/utils/session-message"
1819

1920
const SKIP_PARTS = new Set(["patch", "step-start", "step-finish"])
2021
const SESSION_CONTENT_EVENTS = new Set([
@@ -275,7 +276,7 @@ export function applyDirectoryEvent(input: {
275276
input.setStore("message", info.sessionID, [info])
276277
break
277278
}
278-
const result = Binary.search(messages, info.id, (m) => m.id)
279+
const result = Binary.search(messages, messageKey(info), messageKey)
279280
if (result.found) {
280281
input.setStore("message", info.sessionID, result.index, reconcile(info))
281282
break
@@ -295,8 +296,8 @@ export function applyDirectoryEvent(input: {
295296
produce((draft) => {
296297
const messages = draft.message[props.sessionID]
297298
if (messages) {
298-
const result = Binary.search(messages, props.messageID, (m) => m.id)
299-
if (result.found) messages.splice(result.index, 1)
299+
const index = messages.findIndex((message) => message.id === props.messageID)
300+
if (index >= 0) messages.splice(index, 1)
300301
}
301302
const parts = draft.part[props.messageID]
302303
if (parts) {
@@ -322,7 +323,7 @@ export function applyDirectoryEvent(input: {
322323
input.setStore("part", part.messageID, [part])
323324
break
324325
}
325-
const result = Binary.search(parts, part.id, (p) => p.id)
326+
const result = Binary.search(parts, part.id, (item) => item.id)
326327
if (result.found) {
327328
input.setStore("part", part.messageID, result.index, reconcile(part))
328329
break
@@ -345,13 +346,13 @@ export function applyDirectoryEvent(input: {
345346
)
346347
const parts = input.store.part[props.messageID]
347348
if (!parts) break
348-
const result = Binary.search(parts, props.partID, (p) => p.id)
349+
const result = Binary.search(parts, props.partID, (part) => part.id)
349350
if (result.found) {
350351
input.setStore(
351352
produce((draft) => {
352353
const list = draft.part[props.messageID]
353354
if (!list) return
354-
const next = Binary.search(list, props.partID, (p) => p.id)
355+
const next = Binary.search(list, props.partID, (part) => part.id)
355356
if (!next.found) return
356357
list.splice(next.index, 1)
357358
if (list.length === 0) delete draft.part[props.messageID]
@@ -364,7 +365,7 @@ export function applyDirectoryEvent(input: {
364365
const props = event.properties as { messageID: string; partID: string; field: string; delta: string }
365366
const parts = input.store.part[props.messageID]
366367
if (!parts) break
367-
const result = Binary.search(parts, props.partID, (p) => p.id)
368+
const result = Binary.search(parts, props.partID, (part) => part.id)
368369
if (!result.found) break
369370
const field = props.field as keyof (typeof parts)[number]
370371
const current = parts[result.index]?.[field]

packages/app/src/context/server-session.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ describe("server session", () => {
264264

265265
expect(requests).toEqual([{ sessionID: "root", limit: 20, order: "desc" }])
266266
expect(store.data.session_message.root.map((message) => message.id)).toEqual([user.id, assistant.id])
267+
expect(store.data.message.root.map((message) => message.id)).toEqual([user.id, assistant.id])
267268
})
268269

269270
test("extends a current page to include the user for split assistant turns", async () => {
@@ -1497,7 +1498,7 @@ describe("server session", () => {
14971498

14981499
await store.sync("child", { force: true })
14991500

1500-
expect(store.data.message.child).toEqual([boundary, older])
1501+
expect(store.data.message.child).toEqual([older, boundary])
15011502
})
15021503

15031504
test("preserves a part update for a message being loaded from history", async () => {

packages/app/src/context/server-session.ts

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,14 @@ import { message as cleanMessage } from "@/utils/diffs"
1818
import { sessionNotFoundError } from "@/utils/server-errors"
1919
import { rootSession } from "@/utils/session-route"
2020
import { normalizeSessionInfo } from "@/utils/session"
21-
import { normalizeSessionMessages } from "@/utils/session-message"
21+
import { compareMessages, messageKey, normalizeSessionMessages } from "@/utils/session-message"
2222
import { dropSessionCaches, pickSessionCacheEvictions, SESSION_CACHE_LIMIT } from "./global-sync/session-cache"
2323
import { createV2SessionReducer, type V2SessionReduction } from "./server-session-v2-reducer"
2424
import type { ServerApi } from "@/utils/server"
2525

2626
type MessageApi = ServerApi["message"]
2727

2828
const cmp = (a: string, b: string) => (a < b ? -1 : a > b ? 1 : 0)
29-
const cmpMessage = (a: Message, b: Message) => a.time.created - b.time.created || cmp(a.id, b.id)
3029
const SKIP_PARTS = new Set(["patch", "step-start", "step-finish"])
3130
const initialMessagePageSize = 20
3231
const historyMessagePageSize = 200
@@ -64,7 +63,7 @@ type MessagePage = {
6463
function legacyMessageSource(items: { info: Message; parts: Part[] }[]): SessionMessageInfo[] {
6564
return items
6665
.slice()
67-
.sort((a, b) => cmp(a.info.id, b.info.id))
66+
.sort((a, b) => compareMessages(a.info, b.info))
6867
.map((item) => {
6968
if (item.info.role === "user") {
7069
return {
@@ -111,17 +110,16 @@ function mergeOptimisticPage(page: MessagePage, items: OptimisticItem[]) {
111110
const part = new Map(page.part.map((item) => [item.id, item.part]))
112111
const observed: { messageID: string; parts: Part[] }[] = []
113112
for (const item of items) {
114-
const result = Binary.search(session, item.message.id, (message) => message.id)
115-
if (!result.found) session.splice(result.index, 0, item.message)
113+
const result = Binary.search(session, messageKey(item.message), messageKey)
114+
const found = result.found
115+
if (!found) session.splice(result.index, 0, item.message)
116116
const current = part.get(item.message.id)
117-
const confirmed = result.found
118-
? item.parts.filter((part) => Binary.search(current ?? [], part.id, (value) => value.id).found)
119-
: []
120-
if (result.found) observed.push({ messageID: item.message.id, parts: confirmed })
117+
const confirmed = found ? item.parts.filter((part) => current?.some((value) => value.id === part.id)) : []
118+
if (found) observed.push({ messageID: item.message.id, parts: confirmed })
121119
part.set(
122120
item.message.id,
123121
merge(
124-
result.found ? (current ?? []) : merge(item.confirmedParts ?? [], current ?? []),
122+
found ? (current ?? []) : merge(item.confirmedParts ?? [], current ?? []),
125123
item.parts.filter((part) => !confirmed.includes(part)),
126124
),
127125
)
@@ -158,6 +156,7 @@ function reconcileFetched<T extends { id: string }>(
158156
retained?: ReadonlySet<string>
159157
removed?: ReadonlySet<string>
160158
preserveUnfetched?: boolean | ((item: T) => boolean)
159+
compare?: (a: T, b: T) => number
161160
} = {},
162161
) {
163162
const result = new Map(fetched.map((item) => [item.id, item]))
@@ -180,7 +179,8 @@ function reconcileFetched<T extends { id: string }>(
180179
if (!item) result.delete(id)
181180
}
182181
for (const id of options.removed ?? emptyIDs) result.delete(id)
183-
return [...result.values()].sort((a, b) => cmp(a.id, b.id))
182+
const items = [...result.values()]
183+
return options.compare ? items.sort(options.compare) : items
184184
}
185185

186186
type ServerSessionOptions = { retry?: typeof retry; protocol?: Promise<"v1" | "v2"> }
@@ -413,8 +413,7 @@ export function createServerSession(
413413
if (!load) return
414414
// A part event keeps an existing parent when the fetched page omits it without overriding fetched metadata.
415415
const messages = data.message[sessionID]
416-
if (messages && Binary.search(messages, messageID, (message) => message.id).found)
417-
load.retainedMessages.add(messageID)
416+
if (messages?.some((message) => message.id === messageID)) load.retainedMessages.add(messageID)
418417
const parts = load.touchedParts.get(messageID)
419418
if (parts) {
420419
parts.add(partID)
@@ -437,16 +436,14 @@ export function createServerSession(
437436
load.touchedParts.set(messageID, new Set(parts))
438437
load.carriedDeltaParts.set(messageID, new Set(parts))
439438
const messages = data.message[sessionID]
440-
if (messages && Binary.search(messages, messageID, (message) => message.id).found)
441-
load.retainedMessages.add(messageID)
439+
if (messages?.some((message) => message.id === messageID)) load.retainedMessages.add(messageID)
442440
}
443441
for (const [messageID, parts] of load.removedParts) {
444442
const touched = load.touchedParts.get(messageID) ?? new Set<string>()
445443
parts.forEach((partID) => touched.add(partID))
446444
load.touchedParts.set(messageID, touched)
447445
const messages = data.message[sessionID]
448-
if (messages && Binary.search(messages, messageID, (message) => message.id).found)
449-
load.retainedMessages.add(messageID)
446+
if (messages?.some((message) => message.id === messageID)) load.retainedMessages.add(messageID)
450447
}
451448
for (const [messageID, parts] of load.optimisticParts) {
452449
load.removedMessages.delete(messageID)
@@ -555,7 +552,7 @@ export function createServerSession(
555552
const source = pages.flatMap((page) => page.data).toReversed()
556553
const normalized = normalizeSessionMessages(sessionID, source)
557554
return {
558-
session: normalized.messages.sort((a, b) => cmp(a.id, b.id)),
555+
session: normalized.messages.sort(compareMessages),
559556
part: [...normalized.parts.entries()]
560557
.map(([id, part]) => ({ id, part: part.sort((a, b) => cmp(a.id, b.id)) }))
561558
.sort((a, b) => cmp(a.id, b.id)),
@@ -572,7 +569,7 @@ export function createServerSession(
572569
})
573570
const items = (response.data ?? []).filter((item) => !!item?.info?.id)
574571
return {
575-
session: items.map((item) => cleanMessage(item.info)).sort((a, b) => cmp(a.id, b.id)),
572+
session: items.map((item) => cleanMessage(item.info)).sort(compareMessages),
576573
part: items.map((item) => ({
577574
id: item.info.id,
578575
part: item.parts.filter((part) => !!part?.id).sort((a, b) => cmp(a.id, b.id)),
@@ -696,7 +693,7 @@ export function createServerSession(
696693
const normalized = normalizeSessionMessages(sessionID, source)
697694
return {
698695
...page,
699-
session: normalized.messages.sort((a, b) => cmp(a.id, b.id)),
696+
session: normalized.messages.sort(compareMessages),
700697
part: [...normalized.parts.entries()]
701698
.map(([id, part]) => ({ id, part: part.sort((a, b) => cmp(a.id, b.id)) }))
702699
.sort((a, b) => cmp(a.id, b.id)),
@@ -713,6 +710,7 @@ export function createServerSession(
713710
retained: load?.retainedMessages,
714711
removed: load?.removedMessages,
715712
preserveUnfetched,
713+
compare: compareMessages,
716714
})
717715
batch(() => {
718716
if (source) setData("session_message", sessionID, reconcile(source))
@@ -754,7 +752,7 @@ export function createServerSession(
754752
try {
755753
const page = await fetchMessages(sessionID, limit, before, () => resetMessageLoad(sessionID, load))
756754
const first = page.session.reduce<Message | undefined>(
757-
(oldest, message) => (!oldest || cmpMessage(message, oldest) < 0 ? message : oldest),
755+
(oldest, message) => (!oldest || compareMessages(message, oldest) < 0 ? message : oldest),
758756
undefined,
759757
)
760758
if (generations.get(sessionID) !== active) return
@@ -804,14 +802,15 @@ export function createServerSession(
804802
session: merge(
805803
page.session,
806804
parents.map((parent) => parent.message),
807-
),
805+
).sort(compareMessages),
808806
part: merge(
809807
page.part,
810808
parents.map((parent) => ({ id: parent.message.id, part: parent.parts })),
811809
),
812810
}
813811
const preserveUnfetched =
814-
mode === "prepend" || (!result.complete && (!first || ((message: Message) => cmpMessage(message, first) < 0)))
812+
mode === "prepend" ||
813+
(!result.complete && (!first || ((message: Message) => compareMessages(message, first) < 0)))
815814
applyMessagePage(
816815
sessionID,
817816
result,
@@ -928,7 +927,7 @@ export function createServerSession(
928927
.message({ sessionID, messageID })
929928
.then((message) => {
930929
const current = data.session_message[sessionID] ?? []
931-
const messages = [...current.filter((item) => item.id !== message.id), message].sort((a, b) => cmp(a.id, b.id))
930+
const messages = [...current.filter((item) => item.id !== message.id), message].sort(compareMessages)
932931
projectV2({ sessionID, messages, touched: [message.id] })
933932
})
934933
.catch(() => {})
@@ -1051,7 +1050,7 @@ export function createServerSession(
10511050
setData("message", info.sessionID, [info])
10521051
return
10531052
}
1054-
const result = Binary.search(messages, info.id, (message) => message.id)
1053+
const result = Binary.search(messages, messageKey(info), messageKey)
10551054
if (result.found) setData("message", info.sessionID, result.index, reconcile(info))
10561055
if (!result.found)
10571056
setData("message", info.sessionID, (value = []) => {
@@ -1084,8 +1083,8 @@ export function createServerSession(
10841083
produce((draft) => {
10851084
const messages = draft.message[props.sessionID]
10861085
if (messages) {
1087-
const result = Binary.search(messages, props.messageID, (message) => message.id)
1088-
if (result.found) messages.splice(result.index, 1)
1086+
const index = messages.findIndex((message) => message.id === props.messageID)
1087+
if (index >= 0) messages.splice(index, 1)
10891088
}
10901089
deleteMessageParts(draft, props.messageID)
10911090
}),
@@ -1097,7 +1096,7 @@ export function createServerSession(
10971096
if (SKIP_PARTS.has(part.type)) return
10981097
const messages = data.message[part.sessionID]
10991098
const load = messageLoads.get(part.sessionID)
1100-
const missing = !messages || !Binary.search(messages, part.messageID, (message) => message.id).found
1099+
const missing = !messages?.some((message) => message.id === part.messageID)
11011100
// Outside a page load, accepting a part without its ordered parent event would create an unbounded orphan.
11021101
if (
11031102
missing &&
@@ -1341,7 +1340,7 @@ export function createServerSession(
13411340
if (items) items.set(input.message.id, { ...input, parts, confirmedParts: [] })
13421341
if (!items)
13431342
optimistic.set(input.sessionID, new Map([[input.message.id, { ...input, parts, confirmedParts: [] }]]))
1344-
setData("message", input.sessionID, (messages = []) => merge(messages, [input.message]))
1343+
setData("message", input.sessionID, (messages = []) => merge(messages, [input.message]).sort(compareMessages))
13451344
setData(
13461345
"part_text_accum_delta",
13471346
produce((draft) => {

0 commit comments

Comments
 (0)