From 37768958f36e23500142023c3628cae6b58ac446 Mon Sep 17 00:00:00 2001 From: Leonid <127580858+bashrusakh@users.noreply.github.com> Date: Sat, 11 Jul 2026 22:38:39 +1100 Subject: [PATCH] =?UTF-8?q?refactor(chat):=20simplify=20baseDisplayMessage?= =?UTF-8?q?s=20dedup=20=E2=80=94=20remove=20unnecessary=20reverse()=20(#20?= =?UTF-8?q?89)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(chat): preserve chronological message order during history pagination The baseDisplayMessages dedup loop iterated from tail to head (newest to oldest), keeping the newer occurrence of each message ID. During history pagination (prepend mode), the server returns older messages that may overlap with the current view at the boundary. The tail-first iteration discarded the older (prepended) duplicate in favor of the newer (existing) one, breaking chronological ordering. Change the loop to iterate head to tail (oldest to newest) so the first occurrence of each time-sortable message ID is preserved. Remove the now-unnecessary .reverse() call. Fixes #2088 * test(chat): add dedup logic coverage for baseDisplayMessages Covers message ID deduplication in baseDisplayMessages useMemo: - First-occurrence preservation during dedup - Input order maintenance - Empty input, single-element, all-same-ID edge cases - History pagination prepend scenario with overlapping IDs --------- Co-authored-by: bashrusakh --- .../ui/src/components/chat/MessageList.tsx | 14 +- .../baseDisplayMessagesDedup.test.ts | 164 ++++++++++++++++++ 2 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 packages/ui/src/components/chat/__tests__/baseDisplayMessagesDedup.test.ts diff --git a/packages/ui/src/components/chat/MessageList.tsx b/packages/ui/src/components/chat/MessageList.tsx index 58d2caae..3ffb37fc 100644 --- a/packages/ui/src/components/chat/MessageList.tsx +++ b/packages/ui/src/components/chat/MessageList.tsx @@ -1344,20 +1344,24 @@ const MessageList = React.forwardRef(({ const baseDisplayMessages = React.useMemo(() => streamPerfMeasure('ui.message_list.base_display_ms', () => { - const seenIdsFromTail = new Set(); + const seenIds = new Set(); const dedupedMessages: ChatMessageEntry[] = []; - for (let index = messages.length - 1; index >= 0; index -= 1) { + // Deduplicate from head to tail (oldest to newest) to preserve chronological + // order during history pagination (prepend). Older messages have smaller + // time-sortable IDs and appear first in the array. Keeping the first + // occurrence ensures prepended history isn't incorrectly discarded in favor + // of newer duplicates from the existing view. + for (let index = 0; index < messages.length; index += 1) { const message = messages[index]; const messageId = message.info?.id; if (typeof messageId === 'string') { - if (seenIdsFromTail.has(messageId)) { + if (seenIds.has(messageId)) { continue; } - seenIdsFromTail.add(messageId); + seenIds.add(messageId); } dedupedMessages.push(getNormalizedMessageForDisplay(message)); } - dedupedMessages.reverse(); const output: ChatMessageEntry[] = []; const compactionCommandIds = new Set(); diff --git a/packages/ui/src/components/chat/__tests__/baseDisplayMessagesDedup.test.ts b/packages/ui/src/components/chat/__tests__/baseDisplayMessagesDedup.test.ts new file mode 100644 index 00000000..b2c275f5 --- /dev/null +++ b/packages/ui/src/components/chat/__tests__/baseDisplayMessagesDedup.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, test } from 'bun:test'; +import type { ChatMessageEntry } from '../lib/turns/types'; + +/** + * Dedup logic extracted from MessageList.tsx baseDisplayMessages. + * Tests verify that deduplication preserves chronological order + * and keeps the first occurrence of each message ID. + */ +function deduplicateMessages(messages: ChatMessageEntry[]): ChatMessageEntry[] { + const seenIds = new Set(); + const dedupedMessages: ChatMessageEntry[] = []; + + for (let index = 0; index < messages.length; index += 1) { + const message = messages[index]; + const messageId = message.info?.id; + if (typeof messageId === 'string') { + if (seenIds.has(messageId)) { + continue; + } + seenIds.add(messageId); + } + dedupedMessages.push(message); + } + + return dedupedMessages; +} + +function createMessageEntry({ + id, + role, + parentID, + createdAt, +}: { + id: string; + role: 'user' | 'assistant' | 'system'; + parentID?: string; + createdAt: number; +}): ChatMessageEntry { + return { + info: { + id, + role, + ...(parentID ? { parentID } : {}), + time: { created: createdAt }, + } as ChatMessageEntry['info'], + parts: [], + }; +} + +describe('baseDisplayMessages dedup', () => { + test('removes duplicate message IDs, keeping the first occurrence', () => { + const msg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const msg2 = createMessageEntry({ id: 'msg-2', role: 'assistant', createdAt: 2 }); + const msg1Duplicate = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 3 }); + + const result = deduplicateMessages([msg1, msg2, msg1Duplicate]); + + expect(result).toHaveLength(2); + expect(result[0]?.info.id).toBe('msg-1'); + expect(result[1]?.info.id).toBe('msg-2'); + }); + + test('preserves input order when there are no duplicates', () => { + const msg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const msg2 = createMessageEntry({ id: 'msg-2', role: 'assistant', createdAt: 2 }); + const msg3 = createMessageEntry({ id: 'msg-3', role: 'user', createdAt: 3 }); + + const result = deduplicateMessages([msg1, msg2, msg3]); + + expect(result).toHaveLength(3); + expect(result[0]?.info.id).toBe('msg-1'); + expect(result[1]?.info.id).toBe('msg-2'); + expect(result[2]?.info.id).toBe('msg-3'); + }); + + test('handles empty input', () => { + const result = deduplicateMessages([]); + expect(result).toHaveLength(0); + }); + + test('handles messages without IDs (keeps all)', () => { + const msg1 = { info: { role: 'user' } as ChatMessageEntry['info'], parts: [] }; + const msg2 = { info: { role: 'assistant' } as ChatMessageEntry['info'], parts: [] }; + + const result = deduplicateMessages([msg1, msg2]); + + expect(result).toHaveLength(2); + }); + + test('handles empty string ID (treated as no ID, keeps all)', () => { + const msg1 = createMessageEntry({ id: '', role: 'user', createdAt: 1 }); + const msg2 = createMessageEntry({ id: '', role: 'assistant', createdAt: 2 }); + + const result = deduplicateMessages([msg1, msg2]); + + // Empty string passes typeof === 'string' check, so it IS deduplicated + expect(result).toHaveLength(1); + }); + + test('handles single-element input', () => { + const msg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + + const result = deduplicateMessages([msg1]); + + expect(result).toHaveLength(1); + expect(result[0]?.info.id).toBe('msg-1'); + }); + + test('all messages sharing same ID keeps only first', () => { + const msg1 = createMessageEntry({ id: 'same-id', role: 'user', createdAt: 1 }); + const msg2 = createMessageEntry({ id: 'same-id', role: 'assistant', createdAt: 2 }); + const msg3 = createMessageEntry({ id: 'same-id', role: 'user', createdAt: 3 }); + + const result = deduplicateMessages([msg1, msg2, msg3]); + + expect(result).toHaveLength(1); + expect(result[0]?.info.id).toBe('same-id'); + expect(result[0]?.info.role).toBe('user'); + }); + + test('deduplication scenario: prepend history with overlapping IDs', () => { + // Simulates history pagination where older messages are prepended + // and may overlap with existing messages in the view + const existingMsg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const existingMsg2 = createMessageEntry({ id: 'msg-2', role: 'assistant', createdAt: 2 }); + + // Prepended history (older) that overlaps with existing view + const prependedMsg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const prependedMsg2 = createMessageEntry({ id: 'msg-0', role: 'assistant', createdAt: 0 }); + + // After prepend, array is: [prepended older msgs, existing msgs] + const messages = [prependedMsg2, prependedMsg1, existingMsg1, existingMsg2]; + + const result = deduplicateMessages(messages); + + // Should keep all unique IDs, first occurrence wins + expect(result).toHaveLength(3); + expect(result[0]?.info.id).toBe('msg-0'); // prepended (oldest) + expect(result[1]?.info.id).toBe('msg-1'); // first occurrence from prepend + expect(result[2]?.info.id).toBe('msg-2'); // existing + }); + + test('handles multiple duplicates of the same ID', () => { + const msg1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const msg1Dup1 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 2 }); + const msg1Dup2 = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 3 }); + + const result = deduplicateMessages([msg1, msg1Dup1, msg1Dup2]); + + expect(result).toHaveLength(1); + expect(result[0]?.info.id).toBe('msg-1'); + }); + + test('preserves first occurrence when duplicates appear later', () => { + const msg1First = createMessageEntry({ id: 'msg-1', role: 'user', createdAt: 1 }); + const msg1Later = createMessageEntry({ id: 'msg-1', role: 'assistant', createdAt: 5 }); + + const result = deduplicateMessages([msg1First, msg1Later]); + + expect(result).toHaveLength(1); + // Should keep the first occurrence (createdAt: 1), not the later one + expect(result[0]?.info.role).toBe('user'); + }); +});