From 652fea9bdfc32a69aed0a79b6a520648b6970f40 Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulip.com>
Date: Wed, 31 Jan 2024 10:39:14 -0800
Subject: [PATCH] narrow: Clarify some confusing details.

The update_selection function name was rather misleading, since that
function call is in fact what renders the message list object for the
view.

Also add comments about a few subtle/confusing details that I noticed
while debugging this code path today.
---
 web/src/message_list.js |  9 +++++++++
 web/src/narrow.js       | 14 +++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/web/src/message_list.js b/web/src/message_list.js
index 1360b24bd8..cdfee6684d 100644
--- a/web/src/message_list.js
+++ b/web/src/message_list.js
@@ -286,6 +286,15 @@ export class MessageList {
         this.data.set_selected_id(id);
 
         if (opts.force_rerender) {
+            // TODO: Because rerender() itself will call
+            // reselect_selected_id after doing the rendering, we
+            // actually end up with this function being called
+            // recursively in this case. The ordering will end up
+            // being that the message_selected.zulip event for that
+            // rerender is processed before execution returns here.
+            //
+            // The recursive call is unnecessary, so we should figure
+            // out how to avoid that, both here and in the next block.
             this.rerender();
         } else if (!opts.from_rendering) {
             this.view.maybe_rerender();
diff --git a/web/src/narrow.js b/web/src/narrow.js
index 3a8b574e4a..0f162ae117 100644
--- a/web/src/narrow.js
+++ b/web/src/narrow.js
@@ -497,7 +497,7 @@ export function activate(raw_terms, opts) {
                 anchor,
                 cont() {
                     if (!select_immediately) {
-                        update_selection({
+                        render_message_list_with_selected_message({
                             id_info,
                             select_offset: then_select_offset,
                             msg_list: message_lists.current,
@@ -509,14 +509,14 @@ export function activate(raw_terms, opts) {
         }
 
         // Important: We need to consider opening the compose box
-        // before calling update_selection, so that the logic in
+        // before calling render_message_list_with_selected_message, so that the logic in
         // recenter_view for positioning the currently selected
         // message can take into account the space consumed by the
         // open compose box.
         compose_actions.on_narrow(opts);
 
         if (select_immediately) {
-            update_selection({
+            render_message_list_with_selected_message({
                 id_info,
                 select_offset: then_select_offset,
                 msg_list: message_lists.current,
@@ -532,7 +532,6 @@ export function activate(raw_terms, opts) {
 
         handle_post_view_change(msg_list);
 
-
         unread_ui.update_unread_banner();
 
         // It is important to call this after other important updates
@@ -740,7 +739,7 @@ export function maybe_add_local_messages(opts) {
     return;
 }
 
-export function update_selection(opts) {
+export function render_message_list_with_selected_message(opts) {
     if (message_lists.current !== opts.msg_list) {
         // If we navigated away from a view while we were fetching
         // messages for it, don't attempt to move the currently
@@ -768,6 +767,11 @@ export function update_selection(opts) {
 
     const then_scroll = !preserve_pre_narrowing_screen_position;
 
+    // Here we render the actual message list to the DOM with the
+    // target selected message, using the force_rerender parameter.
+    //
+    // TODO: Probably this should accept the offset parameter rather
+    // than calling `set_message_offset` just after.
     message_lists.current.select_id(msg_id, {
         then_scroll,
         use_closest: true,