From 08eb87a9823bef003318d28ea37f5919e5a2fe6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Mon, 22 May 2023 16:25:51 +0000 Subject: [PATCH] [FIX] mail: fix uncessary render in message (potential initie loop) Overwriting array with new array triggers reactive. This leads to uncessary renders, and potential infitine loop if both read and insert methods are used inside rendering cycle. Part-of: odoo/odoo#121418 --- .../src/attachments/attachment_service.js | 16 ++-- addons/mail/static/src/core/message_model.js | 2 +- .../mail/static/src/core/message_service.js | 88 ++++++++++++------- .../static/src/core/notification_model.js | 1 - addons/mail/static/src/core/thread_service.js | 10 +-- addons/mail/static/src/utils/arrays.js | 23 ++++- 6 files changed, 88 insertions(+), 52 deletions(-) diff --git a/addons/mail/static/src/attachments/attachment_service.js b/addons/mail/static/src/attachments/attachment_service.js index dacb7531488..7b7f3b91095 100644 --- a/addons/mail/static/src/attachments/attachment_service.js +++ b/addons/mail/static/src/attachments/attachment_service.js @@ -17,13 +17,12 @@ export class AttachmentService { if (!("id" in data)) { throw new Error("Cannot insert attachment: id is missing in data"); } - if (data.id in this.store.attachments) { - const attachment = this.store.attachments[data.id]; - this.update(attachment, data); - return attachment; + let attachment = this.store.attachments[data.id]; + if (!attachment) { + this.store.attachments[data.id] = new Attachment(); + attachment = this.store.attachments[data.id]; + Object.assign(attachment, { _store: this.store, id: data.id }); } - const attachment = (this.store.attachments[data.id] = new Attachment()); - Object.assign(attachment, { _store: this.store, id: data.id }); this.update(attachment, data); return attachment; } @@ -55,9 +54,8 @@ export class AttachmentService { id: threadData.id, }); attachment.originThreadLocalId = createLocalId(threadData.model, threadData.id); - const originThread = this.store.threads[attachment.originThreadLocalId]; - if (!originThread.attachments.some((a) => a.id === attachment.id)) { - originThread.attachments.push(attachment); + if (!attachment.originThread.attachments.includes(attachment)) { + attachment.originThread.attachments.push(attachment); } } } diff --git a/addons/mail/static/src/core/message_model.js b/addons/mail/static/src/core/message_model.js index 8183f873e20..ffe2e92cb2d 100644 --- a/addons/mail/static/src/core/message_model.js +++ b/addons/mail/static/src/core/message_model.js @@ -41,7 +41,7 @@ export class Message { parentMessage; /** @type {MessageReactions[]} */ reactions = []; - /** @type {Notification[]} */ + /** @type {import("@mail/core/notification_model").Notification[]} */ notifications = []; /** @type {import("@mail/core/persona_model").Persona[]} */ recipients = []; diff --git a/addons/mail/static/src/core/message_service.js b/addons/mail/static/src/core/message_service.js index 9ae2f78e571..640b6a70f79 100644 --- a/addons/mail/static/src/core/message_service.js +++ b/addons/mail/static/src/core/message_service.js @@ -1,7 +1,7 @@ /** @odoo-module */ import { Message } from "./message_model"; -import { removeFromArrayWithPredicate } from "../utils/arrays"; +import { removeFromArrayWithPredicate, replaceArrayWithCompare } from "../utils/arrays"; import { convertBrToLineBreak, prettifyMessageContent } from "../utils/format"; import { _t } from "@web/core/l10n/translation"; import { registry } from "@web/core/registry"; @@ -150,7 +150,9 @@ export class MessageService { const thread = message.originThread; await this.env.services["mail.thread"].removeFollower(thread.followerOfSelf); this.env.services.notification.add( - sprintf(_t('You are no longer following "%(thread_name)s".'), { thread_name: thread.name }), + sprintf(_t('You are no longer following "%(thread_name)s".'), { + thread_name: thread.name, + }), { type: "success" } ); } @@ -221,7 +223,6 @@ export class MessageService { message = this.store.messages[data.id] = message; } this.update(message, data); - this.updateNotifications(message); // return reactive version return message; } @@ -246,15 +247,14 @@ export class MessageService { linkPreviews = message.linkPreviews, message_type: type = message.type, model: resModel = message.resModel, + notifications = message.notifications, + recipients = message.recipients, res_id: resId = message.resId, subtype_description: subtypeDescription = message.subtypeDescription, ...remainingData } = data; assignDefined(message, remainingData); assignDefined(message, { - attachments: attachments.map((attachment) => - this.attachmentService.insert({ message, ...attachment }) - ), defaultSubject, isDiscussion, isNote, @@ -262,13 +262,25 @@ export class MessageService { ? message.starred_partner_ids.includes(this.store.user.id) : false, isTransient, - linkPreviews: linkPreviews.map((data) => new LinkPreview(data)), parentMessage: message.parentMessage ? this.insert(message.parentMessage) : undefined, resId, resModel, subtypeDescription, type, }); + // origin thread before other information (in particular notification insert uses it) + if (data.record_name) { + message.originThread.name = data.record_name; + } + if (data.res_model_name) { + message.originThread.modelName = data.res_model_name; + } + replaceArrayWithCompare( + message.attachments, + attachments.map((attachment) => + this.attachmentService.insert({ message, ...attachment }) + ) + ); if ( Array.isArray(message.author) && message.author.some((command) => command.includes("clear")) @@ -288,17 +300,22 @@ export class MessageService { channelId: message.originThread.id, }); } - if (data.recipients) { - message.recipients = data.recipients.map((recipient) => + replaceArrayWithCompare( + message.linkPreviews, + linkPreviews.map((data) => this.insertLinkPreview({ ...data, message })) + ); + replaceArrayWithCompare( + message.notifications, + notifications.map((notification) => + this.insertNotification({ ...notification, messageId: message.id }) + ) + ); + replaceArrayWithCompare( + message.recipients, + recipients.map((recipient) => this.personaService.insert({ ...recipient, type: "partner" }) - ); - } - if (data.record_name) { - message.originThread.name = data.record_name; - } - if (data.res_model_name) { - message.originThread.modelName = data.res_model_name; - } + ) + ); if ("user_follower_id" in data && data.user_follower_id && this.store.self) { this.env.services["mail.thread"].insertFollower({ followedThread: message.originThread, @@ -307,7 +324,9 @@ export class MessageService { partner: this.store.self, }); } - this._updateReactions(message, data.messageReactionGroups); + if (data.messageReactionGroups) { + this._updateReactions(message, data.messageReactionGroups); + } if (message.isNotification && !message.notificationType) { const parser = new DOMParser(); const htmlBody = parser.parseFromString(message.body, "text/html"); @@ -316,13 +335,7 @@ export class MessageService { } } - updateNotifications(message) { - message.notifications = message.notifications.map((notification) => - this.insertNotification({ ...notification, messageId: message.id }) - ); - } - - _updateReactions(message, reactionGroups = []) { + _updateReactions(message, reactionGroups) { const reactionContentToUnlink = new Set(); const reactionsToInsert = []; for (const rawReaction of reactionGroups) { @@ -349,6 +362,20 @@ export class MessageService { }); } + /** + * @param {Object} data + * @returns {LinkPreview} + */ + insertLinkPreview(data) { + const linkPreview = data.message.linkPreviews.find( + (linkPreview) => linkPreview.id === data.id + ); + if (linkPreview) { + return Object.assign(linkPreview, data); + } + return new LinkPreview(data); + } + /** * @param {Object} data * @returns {MessageReactions} @@ -399,16 +426,13 @@ export class MessageService { * @returns {Notification} */ insertNotification(data) { - let notification; - if (data.id in this.store.notifications) { + let notification = this.store.notifications[data.id]; + if (!notification) { + this.store.notifications[data.id] = new Notification(this.store, data); notification = this.store.notifications[data.id]; - this.updateNotification(notification, data); - return notification; } - notification = new Notification(this.store, data); this.updateNotification(notification, data); - // return reactive version - return this.store.notifications[data.id]; + return notification; } updateNotification(notification, data) { diff --git a/addons/mail/static/src/core/notification_model.js b/addons/mail/static/src/core/notification_model.js index 8960ba725cb..cb0d03c6892 100644 --- a/addons/mail/static/src/core/notification_model.js +++ b/addons/mail/static/src/core/notification_model.js @@ -23,7 +23,6 @@ export class Notification { id: data.id, _store: store, }); - store.notifications[this.id] = this; } get message() { diff --git a/addons/mail/static/src/core/thread_service.js b/addons/mail/static/src/core/thread_service.js index 0032c089bfe..4f9c6e8c5cb 100644 --- a/addons/mail/static/src/core/thread_service.js +++ b/addons/mail/static/src/core/thread_service.js @@ -672,14 +672,14 @@ export class ThreadService { * @param {Object} data */ update(thread, data) { - const { id, name, attachments, description, ...serverData } = data; + const { id, name, attachments: attachmentsData, description, ...serverData } = data; assignDefined(thread, { id, name, description }); - if (attachments) { - // smart process to avoid triggering reactives when there is no change between the 2 arrays + if (attachmentsData) { replaceArrayWithCompare( thread.attachments, - attachments.map((attachment) => this.attachmentsService.insert(attachment)), - (a1, a2) => a1.id === a2.id + attachmentsData.map((attachmentData) => + this.attachmentsService.insert(attachmentData) + ) ); } if (serverData) { diff --git a/addons/mail/static/src/utils/arrays.js b/addons/mail/static/src/utils/arrays.js index d144012f2f6..8632b98c7dc 100644 --- a/addons/mail/static/src/utils/arrays.js +++ b/addons/mail/static/src/utils/arrays.js @@ -1,5 +1,7 @@ /* @odoo-module */ +import { toRaw } from "@odoo/owl"; + export function removeFromArray(array, elem) { const index = array.indexOf(elem); if (index >= 0) { @@ -14,14 +16,27 @@ export function removeFromArrayWithPredicate(array, predicate) { } } -export function replaceArrayWithCompare(array1, array2, compareFn) { +/** + * Replaces the content of array1 with the content of array2. Order of elements + * is not guaranteed: new elements are inserted last. + * + * Smart process to avoid triggering reactives when there is no change between + * the 2 arrays. + */ +export function replaceArrayWithCompare(array1, array2) { + array1 = toRaw(array1); + array2 = toRaw(array2); + const elementsToRemove = new Set(); for (const el1 of array1) { - if (!array2.some((el2) => compareFn(el1, el2))) { - removeFromArrayWithPredicate(array1, (el) => compareFn(el, el1)); + if (!array2.includes(el1)) { + elementsToRemove.add(el1); } } + for (const el of elementsToRemove) { + removeFromArray(array1, el); + } for (const el2 of array2) { - if (!array1.some((el1) => compareFn(el1, el2))) { + if (!array1.includes(el2)) { array1.push(el2); } }