Merge pull request #8291 from nextcloud/bugfix/8289/fix-chat-loading-and-performance-with-many-reactions-or-votes

Fix chat "not loading" and performance issue with many reactions or v…

src/components/MessagesList/MessagesList.vue

@@ -75,7 +75,10 @@ import MessagesGroup from './MessagesGroup/MessagesGroup.vue'
 import Axios from '@nextcloud/axios'
 import { subscribe, unsubscribe } from '@nextcloud/event-bus'
 import isInLobby from '../../mixins/isInLobby.js'
-import { ATTENDEE } from '../../constants.js'
+import {
+	ATTENDEE,
+	CHAT,
+} from '../../constants.js'
 import debounce from 'debounce'
 import { EventBus } from '../../services/EventBus.js'
 import LoadingPlaceholder from '../LoadingPlaceholder.vue'
@@ -164,18 +167,7 @@ export default {
 			return this.$store.getters.messagesList(this.token)
 		},
 		/**
-		 * Gets the messages object, which is structured so that the key of each message element
-		 * corresponds to the id of the message, and makes it easy and efficient to access the
-		 * individual message object.
-		 *
-		 * @return {object}
-		 */
-		messages() {
-			// FIXME: remove if unused ?
-			return this.$store.getters.messages(this.token)
-		},
-		/**
-		 * Creates an array of messages grouped in nested arrays by same autor.
+		 * Creates an array of messages grouped in nested arrays by same author.
 		 *
 		 * @return {Array}
 		 */
@@ -558,6 +550,7 @@ export default {
 					token: this.token,
 					lastKnownMessageId: this.$store.getters.getFirstKnownMessageId(this.token),
 					includeLastKnown,
+					minimumVisible: CHAT.MINIMUM_VISIBLE,
 				})
 
 				this.loadingOldMessages = false
@@ -599,7 +592,7 @@ export default {
 					return
 				}
 
-				if (exception.response && exception.response.status === 304) {
+				if (exception?.response?.status === 304) {
 					// 304 - Not modified
 					// This is not an error, so reset error timeout and poll again
 					this.pollingErrorTimeout = 1

src/constants.js

@@ -24,6 +24,10 @@ export const SIGNALING = {
 		CLUSTER_CONVERSATION: 'conversation_cluster',
 	},
 }
+export const CHAT = {
+	FETCH_LIMIT: 100,
+	MINIMUM_VISIBLE: 5,
+}
 export const CONVERSATION = {
 	START_CALL: {
 		EVERYONE: 0,

src/services/messagesService.js

@@ -33,14 +33,16 @@ import Hex from 'crypto-js/enc-hex.js'
  * @param {string} data.token the conversation token;
  * @param {string} data.lastKnownMessageId last known message id;
  * @param {boolean} data.includeLastKnown whether to include the last known message in the response;
+ * @param {number} data.limit Number of messages to load (default: 100)
  * @param {object} options options;
  */
-const fetchMessages = async function({ token, lastKnownMessageId, includeLastKnown }, options) {
+const fetchMessages = async function({ token, lastKnownMessageId, includeLastKnown, limit }, options) {
 	return axios.get(generateOcsUrl('apps/spreed/api/v1/chat/{token}', { token }), Object.assign(options, {
 		params: {
 			setReadMarker: 0,
 			lookIntoFuture: 0,
 			lastKnownMessageId,
+			limit: limit || 100,
 			includeLastKnown: includeLastKnown ? 1 : 0,
 		},
 	}))

src/services/messagesService.spec.js

@@ -32,6 +32,7 @@ describe('messagesService', () => {
 					setReadMarker: 0,
 					lookIntoFuture: 0,
 					lastKnownMessageId: 1234,
+					limit: 100,
 					includeLastKnown: 0,
 				},
 			}
@@ -55,6 +56,7 @@ describe('messagesService', () => {
 					setReadMarker: 0,
 					lookIntoFuture: 0,
 					lastKnownMessageId: 1234,
+					limit: 100,
 					includeLastKnown: 1,
 				},
 			}

src/store/messagesStore.js

@@ -37,6 +37,7 @@ import CancelableRequest from '../utils/cancelableRequest.js'
 import { showError } from '@nextcloud/dialogs'
 import {
 	ATTENDEE,
+	CHAT,
 } from '../constants.js'
 
 /**
@@ -153,27 +154,10 @@ const getters = {
 	 */
 	messagesList: (state) => (token) => {
 		if (state.messages[token]) {
-			return Object.values(state.messages[token]).filter(message => {
-				// Filter out some system messages
-				if (message.systemMessage === 'reaction'
-					|| message.systemMessage === 'reaction_deleted'
-					|| message.systemMessage === 'reaction_revoked'
-					|| message.systemMessage === 'poll_voted'
-				) {
-					return false
-				} else {
-					return true
-				}
-			})
+			return Object.values(state.messages[token])
 		}
 		return []
 	},
-	messages: (state) => (token) => {
-		if (state.messages[token]) {
-			return state.messages[token]
-		}
-		return {}
-	},
 	message: (state) => (token, id) => {
 		if (state.messages[token][id]) {
 			return state.messages[token][id]
@@ -479,7 +463,9 @@ const actions = {
 			})
 		}
 
-		if (message.systemMessage === 'reaction' || message.systemMessage === 'reaction_revoked') {
+		if (message.systemMessage === 'reaction'
+			|| message.systemMessage === 'reaction_deleted'
+			|| message.systemMessage === 'reaction_revoked') {
 			context.commit('resetReactions', {
 				token: message.token,
 				messageId: message.parent,
@@ -500,7 +486,14 @@ const actions = {
 			})
 		}
 
-		context.commit('addMessage', message)
+		// Filter out some system messages
+		if (message.systemMessage !== 'reaction'
+			&& message.systemMessage !== 'reaction_deleted'
+			&& message.systemMessage !== 'reaction_revoked'
+			&& message.systemMessage !== 'poll_voted'
+		) {
+			context.commit('addMessage', message)
+		}
 
 		 if ((message.messageType === 'comment' && message.message === '{file}' && message.messageParameters?.file)
 			|| (message.messageType === 'comment' && message.message === '{object}' && message.messageParameters?.object)) {
@@ -740,9 +733,12 @@ const actions = {
 	 * @param {string} data.token the conversation token;
 	 * @param {object} data.requestOptions request options;
 	 * @param {string} data.lastKnownMessageId last known message id;
+	 * @param {number} data.minimumVisible Minimum number of chat messages we want to load
 	 * @param {boolean} data.includeLastKnown whether to include the last known message in the response;
 	 */
-	async fetchMessages(context, { token, lastKnownMessageId, includeLastKnown, requestOptions }) {
+	async fetchMessages(context, { token, lastKnownMessageId, includeLastKnown, requestOptions, minimumVisible }) {
+		minimumVisible = (typeof minimumVisible === 'undefined') ? CHAT.MINIMUM_VISIBLE : minimumVisible
+
 		context.dispatch('cancelFetchMessages')
 
 		// Get a new cancelable request function and cancel function pair
@@ -754,6 +750,7 @@ const actions = {
 			token,
 			lastKnownMessageId,
 			includeLastKnown,
+			limit: CHAT.FETCH_LIMIT,
 		}, requestOptions)
 
 		let newestKnownMessageId = 0
@@ -774,6 +771,14 @@ const actions = {
 			}
 			context.dispatch('processMessage', message)
 			newestKnownMessageId = Math.max(newestKnownMessageId, message.id)
+
+			if (message.systemMessage !== 'reaction'
+				&& message.systemMessage !== 'reaction_deleted'
+				&& message.systemMessage !== 'reaction_revoked'
+				&& message.systemMessage !== 'poll_voted'
+			) {
+				minimumVisible--
+			}
 		})
 
 		if (response.headers['x-chat-last-given']) {
@@ -796,6 +801,17 @@ const actions = {
 
 		context.commit('loadedMessagesOfConversation', { token })
 
+		if (minimumVisible > 0) {
+			// There are not yet enough visible messages loaded, so fetch another chunk.
+			// This can happen when a lot of reactions or poll votings happen
+			return await context.dispatch('fetchMessages', {
+				token,
+				lastKnownMessageId: context.getters.getFirstKnownMessageId(token),
+				includeLastKnown,
+				minimumVisible,
+			})
+		}
+
 		return response
 	},
 

src/store/messagesStore.spec.js

@@ -137,19 +137,14 @@ describe('messagesStore', () => {
 		expect(store.getters.messagesList(TOKEN)[1]).toStrictEqual(message3)
 		expect(store.getters.messagesList('token-2')[0]).toStrictEqual(message2)
 
-		// by id
-		expect(store.getters.messages(TOKEN)[1]).toStrictEqual(message1)
-		expect(store.getters.messages(TOKEN)[3]).toStrictEqual(message3)
-		expect(store.getters.messages('token-2')[2]).toStrictEqual(message2)
-
 		// with messages getter
-		expect(store.getters.messages(TOKEN)).toStrictEqual({
-			1: message1,
-			3: message3,
-		})
-		expect(store.getters.messages('token-2')).toStrictEqual({
-			2: message2,
-		})
+		expect(store.getters.messagesList(TOKEN)).toStrictEqual([
+			message1,
+			message3,
+		])
+		expect(store.getters.messagesList('token-2')).toStrictEqual([
+			message2,
+		])
 	})
 
 	describe('delete message', () => {
@@ -767,12 +762,14 @@ describe('messagesStore', () => {
 				requestOptions: {
 					dummyOption: true,
 				},
+				minimumVisible: 0,
 			})
 
 			expect(fetchMessages).toHaveBeenCalledWith({
 				token: TOKEN,
 				lastKnownMessageId: 100,
 				includeLastKnown: true,
+				limit: 100,
 			}, {
 				dummyOption: true,
 			})
@@ -818,12 +815,14 @@ describe('messagesStore', () => {
 				requestOptions: {
 					dummyOption: true,
 				},
+				minimumVisible: 0,
 			})
 
 			expect(fetchMessages).toHaveBeenCalledWith({
 				token: TOKEN,
 				lastKnownMessageId: 100,
 				includeLastKnown: false,
+				limit: 100,
 			}, {
 				dummyOption: true,
 			})