From df300b5f24c07ead03f4327b7a218ff9e07ca39c Mon Sep 17 00:00:00 2001 From: Rebeka Dekany <50901361+kakapofelix@users.noreply.github.com> Date: Wed, 19 Apr 2023 09:55:25 +0200 Subject: [PATCH] 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 --- .../js/features/chat/context/chat-context.js | 37 ++++-- .../chat/utils/message-list-appender.js | 28 ++++- .../chat/context/chat-context.test.js | 106 ++++++++++++++++++ .../chat/util/message-list-appender.test.js | 59 +++++++--- 4 files changed, 198 insertions(+), 32 deletions(-) diff --git a/services/web/frontend/js/features/chat/context/chat-context.js b/services/web/frontend/js/features/chat/context/chat-context.js index bed03fe641..99429afbf2 100644 --- a/services/web/frontend/js/features/chat/context/chat-context.js +++ b/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,22 +50,30 @@ export function chatReducer(state, action) { case 'SEND_MESSAGE': return { ...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 - // refresh - id: uuid(), - user: action.user, - content: action.content, - timestamp: Date.now(), - }), + ...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 + // refresh + id: uuid(), + 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() diff --git a/services/web/frontend/js/features/chat/utils/message-list-appender.js b/services/web/frontend/js/features/chat/utils/message-list-appender.js index 97717c1d86..3362bf6fa0 100644 --- a/services/web/frontend/js/features/chat/utils/message-list-appender.js +++ b/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 } } diff --git a/services/web/test/frontend/features/chat/context/chat-context.test.js b/services/web/test/frontend/features/chat/context/chat-context.test.js index a96a7f6e2b..12a0abc40b 100644 --- a/services/web/test/frontend/features/chat/context/chat-context.test.js +++ b/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({ diff --git a/services/web/test/frontend/features/chat/util/message-list-appender.test.js b/services/web/test/frontend/features/chat/util/message-list-appender.test.js index a7eaa20ea5..0338f1a12a 100644 --- a/services/web/test/frontend/features/chat/util/message-list-appender.test.js +++ b/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',