From 731282c2ecbba7312a16be5ac035a9d6dbfc6db3 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Wed, 26 Apr 2023 16:50:38 +0200 Subject: [PATCH] FIX: attempts to make cooking less order dependent (#21253) It's very hard to repro but under specific circumstances I suspect it was possible for this sequence to happen: - set message TEXT - cooking starts - set message COOKED through another mean (like a message bus) - the cooking started sooner finished and erases the cooked set at the step before causing the message to have the incorrect cooked --- .../discourse/models/chat-message.js | 29 +++++++++++-------- ...chat-channel-pane-subscriptions-manager.js | 6 ---- ...annel-thread-pane-subscriptions-manager.js | 6 ---- .../chat-pane-base-subscriptions-manager.js | 13 +-------- .../spec/system/chat_message_onebox_spec.rb | 2 +- 5 files changed, 19 insertions(+), 37 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 880e7d03feb..5c4355dcf76 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -31,13 +31,11 @@ export default class ChatMessage { @tracked deletedAt; @tracked uploads; @tracked excerpt; - @tracked message; @tracked threadId; @tracked threadReplyCount; @tracked reactions; @tracked reviewableId; @tracked user; - @tracked cooked; @tracked inReplyTo; @tracked expanded; @tracked bookmark; @@ -51,6 +49,9 @@ export default class ChatMessage { @tracked newest = false; @tracked highlighted = false; @tracked firstOfResults = false; + @tracked message; + + @tracked _cooked; constructor(channel, args = {}) { this.channel = channel; @@ -77,14 +78,7 @@ export default class ChatMessage { : null); this.draft = args.draft; this.message = args.message || ""; - - if (args.cooked) { - this.cooked = args.cooked; - } else { - this.cooked = ""; - this.cook(); - } - + this._cooked = args.cooked || ""; this.reactions = this.#initChatMessageReactionModel( args.id, args.reactions @@ -94,6 +88,19 @@ export default class ChatMessage { this.bookmark = args.bookmark ? Bookmark.create(args.bookmark) : null; } + get cooked() { + return this._cooked; + } + + set cooked(newCooked) { + // some markdown is cooked differently on the server-side, e.g. + // quotes, avatar images etc. + if (newCooked !== this._cooked) { + this._cooked = newCooked; + this.incrementVersion(); + } + } + cook() { const site = getOwner(this).lookup("service:site"); @@ -110,7 +117,6 @@ export default class ChatMessage { if (ChatMessage.cookFunction) { this.cooked = ChatMessage.cookFunction(this.message); - this.incrementVersion(); } else { generateCookFunction(markdownOptions).then((cookFunction) => { ChatMessage.cookFunction = (raw) => { @@ -121,7 +127,6 @@ export default class ChatMessage { }; this.cooked = ChatMessage.cookFunction(this.message); - this.incrementVersion(); }); } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js index 93a218eb2af..50d73097e65 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js @@ -20,12 +20,6 @@ export default class ChatChannelPaneSubscriptionsManager extends ChatPaneBaseSub return; } - // TODO (martin) Move scrolling functionality to pane from ChatLivePane? - afterProcessedMessage() { - // this.scrollToLatestMessage(); - return; - } - handleThreadCreated(data) { const message = this.messagesManager.findMessage(data.chat_message.id); if (message) { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js index 8cd9521e6b7..cd636f31c75 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js @@ -39,10 +39,4 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB handleThreadOriginalMessageUpdate() { return; } - - // NOTE: noop for now, later we may want to do scrolling or something like - // we do in the channel pane. - afterProcessedMessage() { - return; - } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js index d02962207fb..12cff6a7556 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js @@ -28,11 +28,7 @@ export function handleStagedMessage(messagesManager, data) { inReplyToMsg.threadId = data.chat_message.thread_id; } - // some markdown is cooked differently on the server-side, e.g. - // quotes, avatar images etc. - if (data.chat_message?.cooked !== stagedMessage.cooked) { - stagedMessage.cooked = data.chat_message.cooked; - } + stagedMessage.cooked = data.chat_message.cooked; return stagedMessage; } @@ -142,15 +138,9 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { const message = this.messagesManager.findMessage(data.chat_message.id); if (message) { message.cooked = data.chat_message.cooked; - message.incrementVersion(); - this.afterProcessedMessage(message); } } - afterProcessedMessage() { - throw "not implemented"; - } - handleReactionMessage(data) { const message = this.messagesManager.findMessage(data.chat_message_id); if (message) { @@ -166,7 +156,6 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { message.excerpt = data.chat_message.excerpt; message.uploads = cloneJSON(data.chat_message.uploads || []); message.edited = true; - message.incrementVersion(); } } diff --git a/plugins/chat/spec/system/chat_message_onebox_spec.rb b/plugins/chat/spec/system/chat_message_onebox_spec.rb index d697ac9959a..1a6c6dace11 100644 --- a/plugins/chat/spec/system/chat_message_onebox_spec.rb +++ b/plugins/chat/spec/system/chat_message_onebox_spec.rb @@ -39,7 +39,7 @@ RSpec.describe "Chat message onebox", type: :system, js: true do chat_page.visit_channel(channel_1) channel_page.send_message("https://en.wikipedia.org/wiki/Hyperlink") - expect(page).to have_content("This is a test") + expect(page).to have_content("This is a test", wait: 20) end end end