From b2526569840c0114a5c768078be0d4a2db7364ce Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Tue, 10 Feb 2026 15:23:14 +0530 Subject: [PATCH] fix: Prevent race condition in conversation dataFetched flag (#13492) Co-authored-by: Shivam Mishra --- .../store/modules/conversations/actions.js | 6 +- .../store/modules/conversations/index.js | 23 +++- .../specs/conversations/actions.spec.js | 58 ++++++++++ .../specs/conversations/mutations.spec.js | 103 ++++++++++++++++-- .../dashboard/store/mutation-types.js | 1 + 5 files changed, 174 insertions(+), 17 deletions(-) diff --git a/app/javascript/dashboard/store/modules/conversations/actions.js b/app/javascript/dashboard/store/modules/conversations/actions.js index 559eaaad9..cc3e9d428 100644 --- a/app/javascript/dashboard/store/modules/conversations/actions.js +++ b/app/javascript/dashboard/store/modules/conversations/actions.js @@ -96,7 +96,7 @@ const actions = { data: payload, }); if (!payload.length) { - commit(types.SET_ALL_MESSAGES_LOADED); + commit(types.SET_ALL_MESSAGES_LOADED, data.conversationId); } } catch (error) { // Handle error @@ -191,7 +191,7 @@ const actions = { async setActiveChat({ commit, dispatch }, { data, after }) { commit(types.SET_CURRENT_CHAT_WINDOW, data); - commit(types.CLEAR_ALL_MESSAGES_LOADED); + commit(types.CLEAR_ALL_MESSAGES_LOADED, data.id); if (data.dataFetched === undefined) { try { await dispatch('fetchPreviousMessages', { @@ -199,7 +199,7 @@ const actions = { before: data.messages[0].id, conversationId: data.id, }); - data.dataFetched = true; + commit(types.SET_CHAT_DATA_FETCHED, data.id); } catch (error) { // Ignore error } diff --git a/app/javascript/dashboard/store/modules/conversations/index.js b/app/javascript/dashboard/store/modules/conversations/index.js index 8d3ef9a9a..3c4003af4 100644 --- a/app/javascript/dashboard/store/modules/conversations/index.js +++ b/app/javascript/dashboard/store/modules/conversations/index.js @@ -63,14 +63,18 @@ export const mutations = { _state.allConversations = []; _state.selectedChatId = null; }, - [types.SET_ALL_MESSAGES_LOADED](_state) { - const [chat] = getSelectedChatConversation(_state); - chat.allMessagesLoaded = true; + [types.SET_ALL_MESSAGES_LOADED](_state, conversationId) { + const chat = getConversationById(_state)(conversationId); + if (chat) { + chat.allMessagesLoaded = true; + } }, - [types.CLEAR_ALL_MESSAGES_LOADED](_state) { - const [chat] = getSelectedChatConversation(_state); - chat.allMessagesLoaded = false; + [types.CLEAR_ALL_MESSAGES_LOADED](_state, conversationId) { + const chat = getConversationById(_state)(conversationId); + if (chat) { + chat.allMessagesLoaded = false; + } }, [types.CLEAR_CURRENT_CHAT_WINDOW](_state) { _state.selectedChatId = null; @@ -91,6 +95,13 @@ export const mutations = { chat.messages = data; }, + [types.SET_CHAT_DATA_FETCHED](_state, conversationId) { + const chat = getConversationById(_state)(conversationId); + if (chat) { + chat.dataFetched = true; + } + }, + [types.SET_CURRENT_CHAT_WINDOW](_state, activeChat) { if (activeChat) { _state.selectedChatId = activeChat.id; diff --git a/app/javascript/dashboard/store/modules/specs/conversations/actions.spec.js b/app/javascript/dashboard/store/modules/specs/conversations/actions.spec.js index 502c072f6..87ec5ced7 100644 --- a/app/javascript/dashboard/store/modules/specs/conversations/actions.spec.js +++ b/app/javascript/dashboard/store/modules/specs/conversations/actions.spec.js @@ -716,6 +716,64 @@ describe('#addMentions', () => { }); }); + describe('#setActiveChat', () => { + it('should commit SET_CHAT_DATA_FETCHED with conversation ID after fetch', async () => { + const localCommit = vi.fn(); + const localDispatch = vi.fn().mockResolvedValue(); + const data = { id: 42, messages: [{ id: 100 }] }; + + await actions.setActiveChat( + { commit: localCommit, dispatch: localDispatch }, + { data, after: 99 } + ); + + expect(localCommit.mock.calls).toEqual([ + [types.SET_CURRENT_CHAT_WINDOW, data], + [types.CLEAR_ALL_MESSAGES_LOADED, 42], + [types.SET_CHAT_DATA_FETCHED, 42], + ]); + expect(localDispatch).toHaveBeenCalledWith('fetchPreviousMessages', { + after: 99, + before: 100, + conversationId: 42, + }); + }); + + it('should not dispatch fetchPreviousMessages if dataFetched is already set', async () => { + const localCommit = vi.fn(); + const localDispatch = vi.fn(); + const data = { id: 42, messages: [{ id: 100 }], dataFetched: true }; + + await actions.setActiveChat( + { commit: localCommit, dispatch: localDispatch }, + { data } + ); + + expect(localCommit.mock.calls).toEqual([ + [types.SET_CURRENT_CHAT_WINDOW, data], + [types.CLEAR_ALL_MESSAGES_LOADED, 42], + ]); + expect(localDispatch).not.toHaveBeenCalled(); + }); + + it('should commit SET_CHAT_DATA_FETCHED by ID, not mutate the data object directly (race condition fix)', async () => { + const localCommit = vi.fn(); + const localDispatch = vi.fn().mockResolvedValue(); + const data = { id: 42, messages: [{ id: 100 }] }; + + await actions.setActiveChat( + { commit: localCommit, dispatch: localDispatch }, + { data } + ); + + // The action must NOT set dataFetched on the data object directly + expect(data.dataFetched).toBeUndefined(); + + // Instead it commits a mutation that finds the conversation by ID in the store + expect(localCommit).toHaveBeenCalledWith(types.SET_CHAT_DATA_FETCHED, 42); + }); + }); + describe('#getInboxCaptainAssistantById', () => { it('fetches inbox assistant by id', async () => { axios.get.mockResolvedValue({ diff --git a/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js b/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js index e9e78a25c..0c04d2f61 100644 --- a/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js +++ b/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js @@ -570,25 +570,84 @@ describe('#mutations', () => { }); }); - describe('#SET_ALL_MESSAGES_LOADED', () => { - it('should set allMessagesLoaded to true on selected chat', () => { + describe('#SET_CHAT_DATA_FETCHED', () => { + it('should set dataFetched to true on the conversation by ID', () => { const state = { - allConversations: [{ id: 1, allMessagesLoaded: false }], + allConversations: [{ id: 1 }, { id: 2 }], + }; + mutations[types.SET_CHAT_DATA_FETCHED](state, 1); + expect(state.allConversations[0].dataFetched).toBe(true); + expect(state.allConversations[1].dataFetched).toBeUndefined(); + }); + + it('should do nothing if conversation is not found', () => { + const state = { allConversations: [{ id: 1 }] }; + mutations[types.SET_CHAT_DATA_FETCHED](state, 999); + expect(state.allConversations[0].dataFetched).toBeUndefined(); + }); + + it('should survive the race: SET_ALL_CONVERSATION replaces the object, then SET_CHAT_DATA_FETCHED still works', () => { + // 1. Initial state: conversation exists with dataFetched undefined + const state = { + allConversations: [{ id: 1, messages: [{ id: 'm1' }] }], selectedChatId: 1, }; - mutations[types.SET_ALL_MESSAGES_LOADED](state); + const originalRef = state.allConversations[0]; + + // 2. Simulate SET_ALL_CONVERSATION replacing the object (WebSocket/polling) + // This copies dataFetched from the old object (still undefined) + mutations[types.SET_ALL_CONVERSATION](state, [ + { id: 1, name: 'refreshed', messages: [{ id: 'm2' }] }, + ]); + + // The store now holds a NEW object, old reference is detached + const newRef = state.allConversations[0]; + expect(newRef).not.toBe(originalRef); + expect(newRef.dataFetched).toBeUndefined(); + + // 3. SET_CHAT_DATA_FETCHED finds by ID — works on the current store object + mutations[types.SET_CHAT_DATA_FETCHED](state, 1); + expect(state.allConversations[0].dataFetched).toBe(true); + + // Old detached reference is unaffected + expect(originalRef.dataFetched).toBeUndefined(); + }); + }); + + describe('#SET_ALL_MESSAGES_LOADED', () => { + it('should set allMessagesLoaded to true on the conversation by ID', () => { + const state = { + allConversations: [{ id: 1, allMessagesLoaded: false }, { id: 2 }], + }; + mutations[types.SET_ALL_MESSAGES_LOADED](state, 1); expect(state.allConversations[0].allMessagesLoaded).toBe(true); + expect(state.allConversations[1].allMessagesLoaded).toBeUndefined(); + }); + + it('should do nothing if conversation is not found', () => { + const state = { allConversations: [{ id: 1 }] }; + mutations[types.SET_ALL_MESSAGES_LOADED](state, 999); + expect(state.allConversations[0].allMessagesLoaded).toBeUndefined(); }); }); describe('#CLEAR_ALL_MESSAGES_LOADED', () => { - it('should set allMessagesLoaded to false on selected chat', () => { + it('should set allMessagesLoaded to false on the conversation by ID', () => { const state = { - allConversations: [{ id: 1, allMessagesLoaded: true }], - selectedChatId: 1, + allConversations: [ + { id: 1, allMessagesLoaded: true }, + { id: 2, allMessagesLoaded: true }, + ], }; - mutations[types.CLEAR_ALL_MESSAGES_LOADED](state); + mutations[types.CLEAR_ALL_MESSAGES_LOADED](state, 1); expect(state.allConversations[0].allMessagesLoaded).toBe(false); + expect(state.allConversations[1].allMessagesLoaded).toBe(true); + }); + + it('should do nothing if conversation is not found', () => { + const state = { allConversations: [{ id: 1, allMessagesLoaded: true }] }; + mutations[types.CLEAR_ALL_MESSAGES_LOADED](state, 999); + expect(state.allConversations[0].allMessagesLoaded).toBe(true); }); }); @@ -797,6 +856,34 @@ describe('#mutations', () => { mutations[types.UPDATE_CONVERSATION](state, conversation); expect(state.allConversations[0].status).toEqual('resolved'); }); + + it('should preserve dataFetched and allMessagesLoaded during update', () => { + const state = { + allConversations: [ + { + id: 1, + status: 'open', + updated_at: 100, + messages: [{ id: 'msg1' }], + dataFetched: true, + allMessagesLoaded: true, + }, + ], + }; + + const conversation = { + id: 1, + status: 'resolved', + updated_at: 200, + messages: [{ id: 'msg2' }], + }; + + mutations[types.UPDATE_CONVERSATION](state, conversation); + expect(state.allConversations[0].status).toEqual('resolved'); + expect(state.allConversations[0].dataFetched).toBe(true); + expect(state.allConversations[0].allMessagesLoaded).toBe(true); + expect(state.allConversations[0].messages).toEqual([{ id: 'msg1' }]); + }); }); describe('#UPDATE_CONVERSATION_CONTACT', () => { diff --git a/app/javascript/dashboard/store/mutation-types.js b/app/javascript/dashboard/store/mutation-types.js index 1ecafc493..989e08916 100644 --- a/app/javascript/dashboard/store/mutation-types.js +++ b/app/javascript/dashboard/store/mutation-types.js @@ -64,6 +64,7 @@ export default { SET_CONTEXT_MENU_CHAT_ID: 'SET_CONTEXT_MENU_CHAT_ID', + SET_CHAT_DATA_FETCHED: 'SET_CHAT_DATA_FETCHED', SET_CHAT_LIST_FILTERS: 'SET_CHAT_LIST_FILTERS', UPDATE_CHAT_LIST_FILTERS: 'UPDATE_CHAT_LIST_FILTERS',