Fix: Chat messages deduplication (#12667)

* Prevent the duplicated message IDs by filtering them out

* fix: Prevent the duplicated message IDs by filtering them out

GitOrigin-RevId: 6d6c2821e6e29c9949fd323fa3a507016b51aff4

services/web/frontend/js/features/chat/context/chat-context.js

@@ -38,7 +38,11 @@ export function chatReducer(state, action) {
       return {
         ...state,
         status: 'idle',
-        messages: prependMessages(state.messages, action.messages),
+        ...prependMessages(
+          state.messages,
+          action.messages,
+          state.uniqueMessageIds
+        ),
         lastTimestamp: action.messages[0] ? action.messages[0].timestamp : null,
         atEnd: action.messages.length < PAGE_SIZE,
       }
@@ -46,7 +50,9 @@ export function chatReducer(state, action) {
     case 'SEND_MESSAGE':
       return {
         ...state,
-        messages: appendMessage(state.messages, {
+        ...appendMessage(
+          state.messages,
+          {
             // Messages are sent optimistically, so don't have an id (used for
             // React keys). The uuid is valid for this session, and ensures all
             // messages have an id. It will be overwritten by the actual ids on
@@ -55,13 +61,19 @@ export function chatReducer(state, action) {
             user: action.user,
             content: action.content,
             timestamp: Date.now(),
-        }),
+          },
+          state.uniqueMessageIds
+        ),
       }
 
     case 'RECEIVE_MESSAGE':
       return {
         ...state,
-        messages: appendMessage(state.messages, action.message),
+        ...appendMessage(
+          state.messages,
+          action.message,
+          state.uniqueMessageIds
+        ),
         unreadMessageCount: state.unreadMessageCount + 1,
       }
 
@@ -94,6 +106,7 @@ const initialState = {
   atEnd: false,
   unreadMessageCount: 0,
   error: null,
+  uniqueMessageIds: [],
 }
 
 export const ChatContext = createContext()

services/web/frontend/js/features/chat/utils/message-list-appender.js

@@ -1,6 +1,14 @@
 const TIMESTAMP_GROUP_SIZE = 5 * 60 * 1000 // 5 minutes
 
-export function appendMessage(messageList, message) {
+export function appendMessage(messageList, message, uniqueMessageIds) {
+  if (uniqueMessageIds.includes(message.id)) {
+    return { messages: messageList, uniqueMessageIds }
+  }
+
+  uniqueMessageIds = uniqueMessageIds.slice(0)
+
+  uniqueMessageIds.push(message.id)
+
   const lastMessage = messageList[messageList.length - 1]
 
   const shouldGroup =
@@ -12,7 +20,7 @@ export function appendMessage(messageList, message) {
     message.timestamp - lastMessage.timestamp < TIMESTAMP_GROUP_SIZE
 
   if (shouldGroup) {
-    return messageList.slice(0, messageList.length - 1).concat({
+    messageList = messageList.slice(0, messageList.length - 1).concat({
       ...lastMessage,
       // the `id` is updated to the latest received content when a new
       // message is appended or prepended
@@ -21,21 +29,30 @@ export function appendMessage(messageList, message) {
       contents: lastMessage.contents.concat(message.content),
     })
   } else {
-    return messageList.slice(0).concat({
+    messageList = messageList.slice(0).concat({
       id: message.id,
       user: message.user,
       timestamp: message.timestamp,
       contents: [message.content],
     })
   }
+
+  return { messages: messageList, uniqueMessageIds }
 }
 
-export function prependMessages(messageList, messages) {
+export function prependMessages(messageList, messages, uniqueMessageIds) {
   const listCopy = messageList.slice(0)
+
+  uniqueMessageIds = uniqueMessageIds.slice(0)
+
   messages
     .slice(0)
     .reverse()
     .forEach(message => {
+      if (uniqueMessageIds.includes(message.id)) {
+        return
+      }
+      uniqueMessageIds.push(message.id)
       const firstMessage = listCopy[0]
       const shouldGroup =
         firstMessage &&
@@ -57,5 +74,6 @@ export function prependMessages(messageList, messages) {
         })
       }
     })
-  return listCopy
+
+  return { messages: listCopy, uniqueMessageIds }
 }

services/web/test/frontend/features/chat/context/chat-context.test.js

@@ -50,6 +50,10 @@ describe('ChatContext', function () {
       fetchMock.post('express:/project/:projectId/messages', 200)
     })
 
+    afterEach(function () {
+      fetchMock.reset()
+    })
+
     it('subscribes when mounted', function () {
       const socket = new EventEmitter()
       renderChatContextHook({ socket })
@@ -99,6 +103,108 @@ describe('ChatContext', function () {
       expect(message.contents).to.deep.equal(['new message'])
     })
 
+    it('deduplicate messages from preloading', async function () {
+      // Mock socket: we only need to emit events, not mock actual connections
+      const socket = new EventEmitter()
+      const { result, waitForNextUpdate } = renderChatContextHook({
+        socket,
+      })
+
+      fetchMock.get(
+        'express:/project/:projectId/messages',
+        [
+          {
+            id: 'msg_1',
+            content: 'new message',
+            timestamp: Date.now(),
+            user: {
+              id: 'another_fake_user',
+              first_name: 'another_fake_user_first_name',
+              email: 'another_fake@example.com',
+            },
+          },
+        ],
+        { overwriteRoutes: true }
+      )
+
+      // Mock message being received from another user
+      socket.emit('new-chat-message', {
+        id: 'msg_1',
+        content: 'new message',
+        timestamp: Date.now(),
+        user: {
+          id: 'another_fake_user',
+          first_name: 'another_fake_user_first_name',
+          email: 'another_fake@example.com',
+        },
+      })
+
+      // Check if received the message ID
+      expect(result.current.messages).to.have.length(1)
+
+      // Wait until initial messages have loaded
+      result.current.loadInitialMessages()
+      await waitForNextUpdate()
+
+      // Check if there are no message duplication
+      expect(result.current.messages).to.have.length(1)
+
+      const message = result.current.messages[0]
+      expect(message.id).to.equal('msg_1')
+      expect(message.contents).to.deep.equal(['new message'])
+    })
+
+    it('deduplicate messages from websocket', async function () {
+      // Mock socket: we only need to emit events, not mock actual connections
+      const socket = new EventEmitter()
+      const { result, waitForNextUpdate } = renderChatContextHook({
+        socket,
+      })
+
+      fetchMock.get(
+        'express:/project/:projectId/messages',
+        [
+          {
+            id: 'msg_1',
+            content: 'new message',
+            timestamp: Date.now(),
+            user: {
+              id: 'another_fake_user',
+              first_name: 'another_fake_user_first_name',
+              email: 'another_fake@example.com',
+            },
+          },
+        ],
+        { overwriteRoutes: true }
+      )
+
+      // Wait until initial messages have loaded
+      result.current.loadInitialMessages()
+      await waitForNextUpdate()
+
+      // Check if received the message ID
+      expect(result.current.messages).to.have.length(1)
+
+      // Mock message being received from another user
+      socket.emit('new-chat-message', {
+        id: 'msg_1',
+        content: 'new message',
+        timestamp: Date.now(),
+        user: {
+          id: 'another_fake_user',
+          first_name: 'another_fake_user_first_name',
+          email: 'another_fake@example.com',
+        },
+      })
+
+      // Check if there are no message duplication
+      expect(result.current.messages).to.have.length(1)
+
+      const message = result.current.messages[0]
+      expect(message.id).to.equal('msg_1')
+      expect(message.contents).to.deep.equal(['new message'])
+    })
+
     it("doesn't add received messages from the current user if a message was just sent", async function () {
       const socket = new EventEmitter()
       const { result, waitForNextUpdate } = renderChatContextHook({

services/web/test/frontend/features/chat/util/message-list-appender.test.js

@@ -43,7 +43,11 @@ describe('prependMessages()', function () {
 
   it('to an empty list', function () {
     const messages = createTestMessages()
-    expect(prependMessages([], messages)).to.deep.equal([
+    const uniqueMessageIds = []
+
+    expect(
+      prependMessages([], messages, uniqueMessageIds).messages
+    ).to.deep.equal([
       {
         id: messages[0].id,
         timestamp: messages[0].timestamp,
@@ -54,16 +58,21 @@ describe('prependMessages()', function () {
   })
 
   describe('when the messages to prepend are from the same user', function () {
-    let list, messages
+    let list, messages, uniqueMessageIds
 
     beforeEach(function () {
       list = createTestMessageList()
       messages = createTestMessages()
       messages[0].user = testUser // makes all the messages have the same author
+      uniqueMessageIds = []
     })
 
     it('when the prepended messages are close in time, contents should be merged into the same message', function () {
-      const result = prependMessages(createTestMessageList(), messages)
+      const result = prependMessages(
+        createTestMessageList(),
+        messages,
+        uniqueMessageIds
+      ).messages
       expect(result.length).to.equal(list.length + 1)
       expect(result[0]).to.deep.equal({
         id: messages[0].id,
@@ -75,7 +84,11 @@ describe('prependMessages()', function () {
 
     it('when the prepended messages are separated in time, each message is prepended', function () {
       messages[0].timestamp = messages[1].timestamp - 6 * 60 * 1000 // 6 minutes before the next message
-      const result = prependMessages(createTestMessageList(), messages)
+      const result = prependMessages(
+        createTestMessageList(),
+        messages,
+        uniqueMessageIds
+      ).messages
       expect(result.length).to.equal(list.length + 2)
       expect(result[0]).to.deep.equal({
         id: messages[0].id,
@@ -93,16 +106,21 @@ describe('prependMessages()', function () {
   })
 
   describe('when the messages to prepend are from different users', function () {
-    let list, messages
+    let list, messages, uniqueMessageIds
 
     beforeEach(function () {
       list = createTestMessageList()
       messages = createTestMessages()
+      uniqueMessageIds = []
     })
 
     it('should prepend separate messages to the list', function () {
       messages[0].user = otherUser
-      const result = prependMessages(createTestMessageList(), messages)
+      const result = prependMessages(
+        createTestMessageList(),
+        messages,
+        uniqueMessageIds
+      ).messages
       expect(result.length).to.equal(list.length + 2)
       expect(result[0]).to.deep.equal({
         id: messages[0].id,
@@ -123,8 +141,13 @@ describe('prependMessages()', function () {
     const list = createTestMessageList()
     const messages = createTestMessages()
     messages[0].user = messages[1].user = list[0].user
+    const uniqueMessageIds = []
 
-    const result = prependMessages(createTestMessageList(), messages)
+    const result = prependMessages(
+      createTestMessageList(),
+      messages,
+      uniqueMessageIds
+    ).messages
     expect(result.length).to.equal(list.length)
     expect(result[0]).to.deep.equal({
       id: messages[0].id,
@@ -147,7 +170,11 @@ describe('appendMessage()', function () {
 
   it('to an empty list', function () {
     const testMessage = createTestMessage()
-    expect(appendMessage([], testMessage)).to.deep.equal([
+    const uniqueMessageIds = []
+
+    expect(
+      appendMessage([], testMessage, uniqueMessageIds).messages
+    ).to.deep.equal([
       {
         id: 'appended_message',
         timestamp: testMessage.timestamp,
@@ -158,17 +185,18 @@ describe('appendMessage()', function () {
   })
 
   describe('messages appended shortly after the last message on the list', function () {
-    let list, message
+    let list, message, uniqueMessageIds
 
     beforeEach(function () {
       list = createTestMessageList()
       message = createTestMessage()
       message.timestamp = list[1].timestamp + 6 * 1000 // 6 seconds after the last message in the list
+      uniqueMessageIds = []
     })
 
     describe('when the author is the same as the last message', function () {
       it('should append the content to the last message', function () {
-        const result = appendMessage(list, message)
+        const result = appendMessage(list, message, uniqueMessageIds).messages
         expect(result.length).to.equal(list.length)
         expect(result[1].contents).to.deep.equal(
           list[1].contents.concat(message.content)
@@ -176,7 +204,7 @@ describe('appendMessage()', function () {
       })
 
       it('should update the last message timestamp', function () {
-        const result = appendMessage(list, message)
+        const result = appendMessage(list, message, uniqueMessageIds).messages
         expect(result[1].timestamp).to.equal(message.timestamp)
       })
     })
@@ -187,7 +215,7 @@ describe('appendMessage()', function () {
       })
 
       it('should append the new message to the list', function () {
-        const result = appendMessage(list, message)
+        const result = appendMessage(list, message, uniqueMessageIds).messages
         expect(result.length).to.equal(list.length + 1)
         expect(result[2]).to.deep.equal({
           id: 'appended_message',
@@ -200,16 +228,17 @@ describe('appendMessage()', function () {
   })
 
   describe('messages appended later after the last message on the list', function () {
-    let list, message
+    let list, message, uniqueMessageIds
 
     beforeEach(function () {
       list = createTestMessageList()
       message = createTestMessage()
       message.timestamp = list[1].timestamp + 6 * 60 * 1000 // 6 minutes after the last message in the list
+      uniqueMessageIds = []
     })
 
     it('when the author is the same as the last message, should be appended as new message', function () {
-      const result = appendMessage(list, message)
+      const result = appendMessage(list, message, uniqueMessageIds).messages
       expect(result.length).to.equal(3)
       expect(result[2]).to.deep.equal({
         id: 'appended_message',
@@ -222,7 +251,7 @@ describe('appendMessage()', function () {
     it('when the author is the different than the last message, should be appended as new message', function () {
       message.user = otherUser
 
-      const result = appendMessage(list, message)
+      const result = appendMessage(list, message, uniqueMessageIds).messages
       expect(result.length).to.equal(3)
       expect(result[2]).to.deep.equal({
         id: 'appended_message',