From 3a75fed5678989ad4fb2b7d73d490c7df5b4d1a8 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Sun, 9 Feb 2025 17:36:49 +0100 Subject: [PATCH 1/4] fix(MessagesList): do not replace container when switching conversations - covered by hard update of list in messagesList watcher Signed-off-by: Maksim Sukharev --- src/components/MessagesList/MessagesList.vue | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/MessagesList/MessagesList.vue b/src/components/MessagesList/MessagesList.vue index 0282f9846a4..1aa36d3b032 100644 --- a/src/components/MessagesList/MessagesList.vue +++ b/src/components/MessagesList/MessagesList.vue @@ -7,7 +7,6 @@
Date: Fri, 21 Feb 2025 14:14:15 +0100 Subject: [PATCH 2/4] fix(MessagesList): check if chats are non-scrollable Signed-off-by: Maksim Sukharev --- src/components/MessagesList/MessagesList.vue | 33 ++++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/components/MessagesList/MessagesList.vue b/src/components/MessagesList/MessagesList.vue index 1aa36d3b032..975a9e71bae 100644 --- a/src/components/MessagesList/MessagesList.vue +++ b/src/components/MessagesList/MessagesList.vue @@ -300,11 +300,14 @@ export default { // scroll to bottom if needed this.scrollToBottom({ smooth: false }) - if (this.conversation?.type === CONVERSATION.TYPE.NOTE_TO_SELF) { - this.$nextTick(() => { + this.$nextTick(() => { + this.checkChatNotScrollable() + + if (this.conversation?.type === CONVERSATION.TYPE.NOTE_TO_SELF) { this.updateTasksCount() - }) - } + } + }) + }, }, @@ -320,8 +323,7 @@ export default { this.$nextTick(() => { this.checkSticky() // setting wheel event for non-scrollable chat - const isScrollable = this.$refs.scroller.scrollHeight > this.$refs.scroller.clientHeight - if (!this.isChatBeginningReached && !isScrollable) { + if (!this.isChatBeginningReached && this.checkChatNotScrollable()) { this.$refs.scroller.addEventListener('wheel', this.handleWheelEvent, { passive: true }) } }) @@ -385,6 +387,8 @@ export default { this.$refs.scroller.scrollTo({ top: this.$refs.scroller.scrollHeight, }) + } else { + this.checkChatNotScrollable() } }, @@ -1161,10 +1165,7 @@ export default { this.$refs.scroller.scrollTop += this.$refs.scroller.offsetHeight / 4 } - if (this.$refs.scroller && this.$refs.scroller.clientHeight === this.$refs.scroller.scrollHeight) { - // chat is not scrollable - this.setChatScrolledToBottom(true) - } + this.checkChatNotScrollable() if (highlightAnimation && scrollElement === element) { // element is visible, highlight it @@ -1286,6 +1287,18 @@ export default { this.chatExtrasStore.setTasksCounters({ tasksCount, tasksDoneCount }) }, + checkChatNotScrollable() { + const isNotScrollable = this.$refs.scroller + ? this.$refs.scroller.clientHeight === this.$refs.scroller.scrollHeight + : false + + if (isNotScrollable && !this.isChatScrolledToBottom) { + this.setChatScrolledToBottom(true) + } + + return isNotScrollable + }, + handleWheelEvent(event) { // If messages fit in the viewport and user scrolls up, we need to trigger the loading of older messages if (event.deltaY < 0) { From 78f0c2823765b360c856e438f2943fcdfa543ff3 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Fri, 21 Feb 2025 14:40:50 +0100 Subject: [PATCH 3/4] fix(MessagesList): show 'scroll to bottom' only if there is an actual offset from bottom Signed-off-by: Maksim Sukharev --- src/components/MessagesList/MessagesList.vue | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/components/MessagesList/MessagesList.vue b/src/components/MessagesList/MessagesList.vue index 975a9e71bae..50030669c03 100644 --- a/src/components/MessagesList/MessagesList.vue +++ b/src/components/MessagesList/MessagesList.vue @@ -898,17 +898,20 @@ export default { } const { scrollHeight, scrollTop, clientHeight } = this.$refs.scroller - const scrollOffset = scrollHeight - scrollTop + const scrollOffsetFromTop = scrollHeight - scrollTop + const scrollOffsetFromBottom = Math.abs(scrollOffsetFromTop - clientHeight) // For chats that are scrolled to bottom and not fitted in one screen - if (Math.abs(scrollOffset - clientHeight) < SCROLL_TOLERANCE && !this.hasMoreMessagesToLoad && scrollTop > 0) { + if (scrollOffsetFromBottom < SCROLL_TOLERANCE && !this.hasMoreMessagesToLoad && scrollTop > 0) { this.setChatScrolledToBottom(true) this.displayMessagesLoader = false this.debounceUpdateReadMarkerPosition() return } - this.setChatScrolledToBottom(false) + if (scrollOffsetFromBottom >= SCROLL_TOLERANCE) { + this.setChatScrolledToBottom(false) + } if ((scrollHeight > clientHeight && scrollTop < 800 && this.isScrolling === 'up') || skipHeightCheck) { @@ -1309,6 +1312,7 @@ export default { return } + this.isScrolling = 'up' this.debounceHandleScroll({ skipHeightCheck: true }) } }, From 0d3cb5c6c201974ce00c881931f83a5afa417ce0 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Thu, 27 Feb 2025 12:24:13 +0100 Subject: [PATCH 4/4] fix(messagesStore): move logic from postNewMessage to processMessage - that should allow to mark own posted file shares as read, same as text messages Signed-off-by: Maksim Sukharev --- src/store/messagesStore.js | 51 ++++++++++++++------------------- src/store/messagesStore.spec.js | 51 ++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 33 deletions(-) diff --git a/src/store/messagesStore.js b/src/store/messagesStore.js index f2b379971bd..2f06a1d5664 100644 --- a/src/store/messagesStore.js +++ b/src/store/messagesStore.js @@ -586,9 +586,27 @@ const actions = { if (message.referenceId) { const tempMessages = context.getters.getTemporaryReferences(token, message.referenceId) - tempMessages.forEach(tempMessage => { - context.commit('deleteMessage', { token, id: tempMessage.id }) - }) + if (tempMessages.length > 0) { + // Replacing temporary placeholder message with server response (text message / file share) + const conversation = context.getters.conversation(token) + const isOwnMessage = message.actorId === context.getters.getActorId() + && message.actorType === context.getters.getActorType() + + // update lastMessage and lastReadMessage (no await to make it async) + // do it conditionally because there could have been more messages appearing concurrently + if (conversation?.lastMessage && isOwnMessage && message.id > conversation.lastMessage.id) { + context.dispatch('updateConversationLastMessage', { token, lastMessage: message }) + } + + if (conversation?.lastReadMessage && isOwnMessage && message.id > conversation.lastReadMessage) { + context.dispatch('updateLastReadMessage', { token, id: message.id, updateVisually: true }) + } + + // If successful, deletes the temporary message from the store + tempMessages.forEach(tempMessage => { + context.dispatch('removeTemporaryMessageFromStore', { token, id: tempMessage.id }) + }) + } } if (message.systemMessage === 'poll_voted') { @@ -1263,32 +1281,7 @@ const actions = { }) } - // If successful, deletes the temporary message from the store - context.dispatch('removeTemporaryMessageFromStore', { token, id: temporaryMessage.id }) - - const message = response.data.ocs.data - // And adds the complete version of the message received - // by the server - context.dispatch('processMessage', { token, message }) - - const conversation = context.getters.conversation(token) - - // update lastMessage and lastReadMessage - // do it conditionally because there could have been more messages appearing concurrently - if (conversation?.lastMessage && message.id > conversation.lastMessage.id) { - context.dispatch('updateConversationLastMessage', { - token, - lastMessage: message, - }) - } - if (conversation && message.id > conversation.lastReadMessage) { - // no await to make it async - context.dispatch('updateLastReadMessage', { - token, - id: message.id, - updateVisually: true, - }) - } + context.dispatch('processMessage', { token, message: response.data.ocs.data }) return response } catch (error) { diff --git a/src/store/messagesStore.spec.js b/src/store/messagesStore.spec.js index cf5627bd718..91b579c5fb3 100644 --- a/src/store/messagesStore.spec.js +++ b/src/store/messagesStore.spec.js @@ -205,6 +205,37 @@ describe('messagesStore', () => { expect(store.getters.messagesList(TOKEN)).toStrictEqual([message1]) }) + test('updates last read message when replacing matching temporary message', () => { + conversationMock.mockReturnValueOnce({ + token: TOKEN, + lastReadMessage: 100, + lastMessage: { + id: 123, + }, + }) + const response = generateOCSResponse({ payload: conversation }) + updateLastReadMessage.mockResolvedValueOnce(response) + + const temporaryMessage = { + id: 'temp-1', + referenceId: 'reference-1', + token: TOKEN, + } + store.dispatch('addTemporaryMessage', { token: TOKEN, message: temporaryMessage }) + + const message1 = { + id: 123, + token: TOKEN, + actorId: 'actor-id-1', + actorType: ATTENDEE.ACTOR_TYPE.USERS, + referenceId: 'reference-1', + } + + store.dispatch('processMessage', { token: TOKEN, message: message1 }) + expect(store.getters.messagesList(TOKEN)).toStrictEqual([message1]) + expect(updateLastReadMessage).toHaveBeenCalledWith(TOKEN, message1.id) + }) + test('replaces existing message', () => { const message1 = { id: 1, @@ -1606,6 +1637,8 @@ describe('messagesStore', () => { let message1 let conversationMock let getUserIdMock + let getActorIdMock + let getActorTypeMock let updateLastCommonReadMessageAction let updateConversationLastMessageAction let cancelFunctionMocks @@ -1619,10 +1652,14 @@ describe('messagesStore', () => { conversationMock = jest.fn() getUserIdMock = jest.fn() + getActorIdMock = jest.fn().mockReturnValue(() => 'actor-id-1') + getActorTypeMock = jest.fn().mockReturnValue(() => ATTENDEE.ACTOR_TYPE.USERS) updateConversationLastMessageAction = jest.fn() updateLastCommonReadMessageAction = jest.fn() testStoreConfig.getters.conversation = jest.fn().mockReturnValue(conversationMock) testStoreConfig.getters.getUserId = jest.fn().mockReturnValue(getUserIdMock) + testStoreConfig.getters.getActorId = getActorIdMock + testStoreConfig.getters.getActorType = getActorTypeMock testStoreConfig.actions.updateConversationLastMessage = updateConversationLastMessageAction testStoreConfig.actions.updateLastCommonReadMessage = updateLastCommonReadMessageAction // mock this complex local action as we already tested it elsewhere @@ -1662,17 +1699,23 @@ describe('messagesStore', () => { }) getUserIdMock.mockReturnValue(() => 'current-user') - const temporaryMessage = { - id: 'temp-123', + const baseMessage = { + actorId: 'actor-id-1', + actorType: ATTENDEE.ACTOR_TYPE.USERS, message: 'blah', token: TOKEN, + referenceId: 'abc123', + } + + const temporaryMessage = { + ...baseMessage, + id: 'temp-123', sendingFailure: '', } const messageResponse = { + ...baseMessage, id: 200, - token: TOKEN, - message: 'blah', } const response = generateOCSResponse({