From b09dfb782deb811c2062f6d241201ad6b9c0cb93 Mon Sep 17 00:00:00 2001 From: Sayam Samal Date: Wed, 2 Oct 2024 12:46:01 +0530 Subject: [PATCH] reactions: Optimize DOM cost of message reactions via dynamic rendering. Before, the message reactions section along with the add reaction button was being rendered for every message even when there were no reactions present - this led to additional DOM cost. This commit adds the message reactions section only when there is at least a single reaction on the message, and follows up with a cleanup of the message reactions section when there are no reactions. Fixes #31137. Co-authored-by: Anmol-dev45 --- web/src/reactions.ts | 31 +++++- web/styles/reactions.css | 9 -- web/templates/message_body.hbs | 6 +- web/templates/message_reactions.hbs | 16 ++-- web/tests/reactions.test.js | 144 +++++++++++++++++++++++++++- 5 files changed, 178 insertions(+), 28 deletions(-) diff --git a/web/src/reactions.ts b/web/src/reactions.ts index 19b0a21417..fa6df71a61 100644 --- a/web/src/reactions.ts +++ b/web/src/reactions.ts @@ -3,6 +3,7 @@ import assert from "minimalistic-assert"; import {z} from "zod"; import render_message_reaction from "../templates/message_reaction.hbs"; +import render_message_reactions from "../templates/message_reactions.hbs"; import * as blueslip from "./blueslip"; import * as channel from "./channel"; @@ -369,11 +370,23 @@ export function insert_new_reaction( class: reaction_class, }; - const $new_reaction = $(render_message_reaction(context)); - - // Now insert it before the add button. - const $reaction_button_element = get_add_reaction_button(message.id); - $new_reaction.insertBefore($reaction_button_element); + // If the given reaction is the first reaction in a message, then we add + // the whole message reactions section along with the new reaction. + // Else, we insert the new reaction before the add reaction button. + if (message.clean_reactions.size - 1 === 0) { + const $rows = message_lists.all_rendered_row_for_message_id(message.id); + const reaction_section_context = { + msg: { + message_reactions: [context], + }, + }; + const $msg_reaction_section = render_message_reactions(reaction_section_context); + $rows.find(".messagebox-content").append($msg_reaction_section); + } else { + const $new_reaction = $(render_message_reaction(context)); + const $reaction_button_element = get_add_reaction_button(message.id); + $new_reaction.insertBefore($reaction_button_element); + } update_vote_text_on_message(message); } @@ -424,6 +437,14 @@ export function remove_reaction_from_view( const $reaction = find_reaction(message.id, local_id); const reaction_count = clean_reaction_object.user_ids.length; + // Cleanup: If the reaction being removed is the last reaction on the + // message, we remove the whole message reaction section and exit. + if (message.clean_reactions.size === 0) { + const $msg_reaction_section = get_reaction_sections(message.id); + $msg_reaction_section.remove(); + return; + } + if (reaction_count === 0) { // If this user was the only one reacting for this emoji, we simply // remove the reaction and exit. diff --git a/web/styles/reactions.css b/web/styles/reactions.css index 4f4deed425..55b02cb072 100644 --- a/web/styles/reactions.css +++ b/web/styles/reactions.css @@ -121,15 +121,6 @@ color: var(--color-message-reaction-button-text-hover); } - /* Configure the reaction button to appear if and only if there's an - existing reaction to the message. We reference first-child - rather than only-child because tooltips may be appended to - the DOM after this element, whereas actual reactions are - appended before it. */ - &:first-child { - display: none; - } - &:hover { color: var(--color-message-reaction-button-text-hover); background-color: var( diff --git a/web/templates/message_body.hbs b/web/templates/message_body.hbs index 1d888bac0c..10d16b8ff8 100644 --- a/web/templates/message_body.hbs +++ b/web/templates/message_body.hbs @@ -69,6 +69,6 @@
-{{#unless is_hidden}} -
{{> message_reactions }}
-{{/unless}} +{{#if (and (not is_hidden) msg.message_reactions)}} + {{> message_reactions }} +{{/if}} diff --git a/web/templates/message_reactions.hbs b/web/templates/message_reactions.hbs index 878f0883db..7828520d98 100644 --- a/web/templates/message_reactions.hbs +++ b/web/templates/message_reactions.hbs @@ -1,9 +1,11 @@ -{{#each this/msg/message_reactions}} - {{> message_reaction}} -{{/each}} -
-
- -
+
+
+ {{#each this/msg/message_reactions}} + {{> message_reaction}} + {{/each}} +
+
+ +
+
+
diff --git a/web/tests/reactions.test.js b/web/tests/reactions.test.js index 226f6dc3bf..bd1eeefc46 100644 --- a/web/tests/reactions.test.js +++ b/web/tests/reactions.test.js @@ -837,6 +837,74 @@ test("add_reaction/remove_reaction", ({override, override_rewire}) => { }); }); +test("insert_new_reaction (first reaction)", ({mock_template, override_rewire}) => { + const clean_reaction_object = { + class: "message_reaction", + count: 1, + emoji_alt_code: false, + emoji_code: "1f3b1", + emoji_name: "8ball", + is_realm_emoji: false, + label: "translated: You (click to remove) reacted with :8ball:", + local_id: "unicode_emoji,1f3b1", + reaction_type: "unicode_emoji", + user_ids: [alice.user_id], + }; + const message_id = 501; + + mock_template("message_reactions.hbs", false, (data) => { + assert.deepEqual(data, { + msg: { + message_reactions: [ + { + count: 1, + emoji_alt_code: false, + emoji_name: "8ball", + emoji_code: "1f3b1", + local_id: "unicode_emoji,1f3b1", + class: "message_reaction reacted", + message_id, + label: "translated: You (click to remove) reacted with :8ball:", + reaction_type: clean_reaction_object.reaction_type, + is_realm_emoji: false, + vote_text: "", + }, + ], + }, + }); + return ""; + }); + + const $rows = $.create("rows-stub"); + message_lists.all_rendered_row_for_message_id = () => $rows; + + const $messagebox_content = $.create("messagebox-content-stub"); + $rows.set_find_results(".messagebox-content", $messagebox_content); + + let append_called = false; + $messagebox_content.append = (element) => { + assert.equal(element, ""); + append_called = true; + }; + + const message = { + id: message_id, + reactions: [ + { + emoji_name: "8ball", + user_id: alice.user_id, + reaction_type: "unicode_emoji", + emoji_code: "1f3b1", + }, + ], + }; + + override_rewire(reactions, "update_vote_text_on_message", noop); + convert_reactions_to_clean_reactions(message); + reactions.insert_new_reaction(clean_reaction_object, message, alice.user_id); + assert.ok(append_called); +}); + test("insert_new_reaction (me w/unicode emoji)", ({mock_template}) => { const clean_reaction_object = { class: "message_reaction", @@ -855,9 +923,10 @@ test("insert_new_reaction (me w/unicode emoji)", ({mock_template}) => { const $message_reactions = stub_reactions(message_id); const $reaction_button = $.create("reaction-button-stub"); $message_reactions.find = () => $reaction_button; + const $message_reactions_count = $.create("message-reaction-count-stub"); $reaction_button.find = (selector) => { assert.equal(selector, ".message_reaction_count"); - return $.create("message-reaction-count-stub"); + return $message_reactions_count; }; mock_template("message_reaction.hbs", false, (data) => { @@ -886,6 +955,12 @@ test("insert_new_reaction (me w/unicode emoji)", ({mock_template}) => { const message = { id: message_id, reactions: [ + { + emoji_name: "+1", + user_id: bob.user_id, + reaction_type: "unicode_emoji", + emoji_code: "1f44d", + }, { emoji_name: "8ball", user_id: alice.user_id, @@ -894,8 +969,8 @@ test("insert_new_reaction (me w/unicode emoji)", ({mock_template}) => { }, ], }; - convert_reactions_to_clean_reactions(message); + convert_reactions_to_clean_reactions(message); reactions.insert_new_reaction(clean_reaction_object, message, alice.user_id); assert.ok(insert_called); }); @@ -918,9 +993,10 @@ test("insert_new_reaction (them w/zulip emoji)", ({mock_template}) => { const $message_reactions = stub_reactions(message_id); const $reaction_button = $.create("reaction-button-stub"); $message_reactions.find = () => $reaction_button; + const $message_reactions_count = $.create("message-reaction-count-stub"); $reaction_button.find = (selector) => { assert.equal(selector, ".message_reaction_count"); - return $.create("message-reaction-count-stub"); + return $message_reactions_count; }; mock_template("message_reaction.hbs", false, (data) => { @@ -951,6 +1027,12 @@ test("insert_new_reaction (them w/zulip emoji)", ({mock_template}) => { const message = { id: message_id, reactions: [ + { + emoji_name: "+1", + user_id: bob.user_id, + reaction_type: "unicode_emoji", + emoji_code: "1f44d", + }, { emoji_name: "8ball", user_id: bob.user_id, @@ -1155,7 +1237,7 @@ test("remove_reaction_from_view (them)", () => { ); }); -test("remove_reaction_from_view (last person)", () => { +test("remove_reaction_from_view (last person to react)", ({override_rewire}) => { const clean_reaction_object = { class: "message_reaction", count: 1, @@ -1170,12 +1252,66 @@ test("remove_reaction_from_view (last person)", () => { const message_id = 507; const $our_reaction = stub_reaction(message_id, "unicode_emoji,1f3b1"); + override_rewire(reactions, "find_reaction", (message_id_param, local_id) => { + assert.equal(message_id_param, message_id); + assert.equal(local_id, "unicode_emoji,1f3b1"); + return $our_reaction; + }); let removed; $our_reaction.remove = () => { removed = true; }; + const message = { + id: message_id, + reactions: [ + { + emoji_code: "1f3b1", + emoji_name: "8ball", + reaction_type: "unicode_emoji", + user_id: bob.user_id, + }, + { + emoji_code: "1f44d", + emoji_name: "thumbs_up", + reaction_type: "unicode_emoji", + user_id: alice.user_id, + }, + ], + }; + + override_rewire(reactions, "update_vote_text_on_message", noop); + convert_reactions_to_clean_reactions(message); + reactions.remove_reaction_from_view(clean_reaction_object, message, bob.user_id); + assert.ok(removed); +}); + +test("remove_reaction_from_view (last reaction)", () => { + const clean_reaction_object = { + class: "message_reaction", + count: 1, + emoji_alt_code: false, + emoji_code: "1f3b1", + emoji_name: "8ball", + is_realm_emoji: false, + local_id: "unicode_emoji,1f3b1", + reaction_type: "unicode_emoji", + user_ids: [], + }; + const message_id = 507; + + const $message_reactions = stub_reactions(message_id); + + // Create a stub for the specific reaction + const $specific_reaction = $.create("specific-reaction-stub"); + $message_reactions.find = () => $specific_reaction; + + let removed = false; + $message_reactions.remove = () => { + removed = true; + }; + const message = {id: message_id, reactions: []}; convert_reactions_to_clean_reactions(message); reactions.remove_reaction_from_view(clean_reaction_object, message, bob.user_id);