buddy_list: Fetch full subscriber data before showing user counts.

Work towards #34244.

Note that this doesn't show the right counts when the counts appear, because
yet. We can double check functionality after that change is complete.

web/src/buddy_list.ts

@@ -75,6 +75,10 @@ function should_hide_headers(
 type BuddyListRenderData = {
     current_sub: StreamSubscription | undefined;
     pm_ids_set: Set<number>;
+    // Note: Until #34246 is complete, this might be incorrect when
+    // we are dealing with a partial subscriber matrix, leading to
+    // some counting bugs in the headers. This shouldn't show up in
+    // production.
     total_human_subscribers_count: number;
     other_users_count: number;
     hide_headers: boolean;
@@ -168,8 +172,8 @@ export class BuddyList extends BuddyListConf {
     current_filter: Filter | undefined | "unset" = "unset";
 
     initialize_tooltips(): void {
-        const non_participant_users_matching_view_count = (): number =>
+        const non_participant_users_matching_view_count = async (): Promise<number> =>
-            this.non_participant_users_matching_view_count();
+            await this.non_participant_users_matching_view_count();
         const total_human_subscribers_count = (): number =>
             this.render_data.total_human_subscribers_count;
 
@@ -203,52 +207,56 @@ export class BuddyList extends BuddyListConf {
                     placement,
                     showOnCreate: true,
                     onShow(instance) {
-                        let tooltip_text;
                         const participant_count = Number.parseInt(
                             $("#buddy-list-participants-section-heading").attr("data-user-count")!,
                             10,
                         );
                         const elem_id = $elem.attr("id");
                         if (elem_id === "buddy-list-participants-section-heading") {
-                            tooltip_text = $t(
+                            const tooltip_text = $t(
                                 {
                                     defaultMessage:
                                         "{N, plural, one {# participant} other {# participants}}",
                                 },
                                 {N: participant_count},
                             );
+                            instance.setContent(tooltip_text);
                         } else if (elem_id === "buddy-list-users-matching-view-section-heading") {
-                            const users_matching_view_count =
+                            void (async () => {
-                                non_participant_users_matching_view_count();
+                                let tooltip_text;
-                            if (narrow_state.stream_sub()) {
+                                const users_matching_view_count =
-                                tooltip_text = $t(
+                                    await non_participant_users_matching_view_count();
-                                    {
+                                if (narrow_state.stream_sub()) {
-                                        defaultMessage:
+                                    tooltip_text = $t(
-                                            "{N, plural, one {# other subscriber} other {# other subscribers}}",
+                                        {
-                                    },
+                                            defaultMessage:
-                                    {N: users_matching_view_count},
+                                                "{N, plural, one {# other subscriber} other {# other subscribers}}",
-                                );
+                                        },
-                            } else {
+                                        {N: users_matching_view_count},
-                                tooltip_text = $t(
+                                    );
-                                    {
+                                } else {
-                                        defaultMessage:
+                                    tooltip_text = $t(
-                                            "{N, plural, one {# participant} other {# participants}}",
+                                        {
-                                    },
+                                            defaultMessage:
-                                    {N: users_matching_view_count},
+                                                "{N, plural, one {# participant} other {# participants}}",
-                                );
+                                        },
-                            }
+                                        {N: users_matching_view_count},
+                                    );
+                                }
+                                instance.setContent(tooltip_text);
+                            })();
                         } else {
                             const other_users_count =
                                 people.get_active_human_count() - total_human_subscribers_count();
-                            tooltip_text = $t(
+                            const tooltip_text = $t(
                                 {
                                     defaultMessage:
                                         "{N, plural, one {# other user} other {# other users}}",
                                 },
                                 {N: other_users_count},
                             );
+                            instance.setContent(tooltip_text);
                         }
-                        instance.setContent(tooltip_text);
                     },
                     onHidden(instance) {
                         instance.destroy();
@@ -259,7 +267,7 @@ export class BuddyList extends BuddyListConf {
         );
     }
 
-    non_participant_users_matching_view_count(): number {
+    async non_participant_users_matching_view_count(): Promise<number> {
         const {current_sub, get_all_participant_ids} = this.render_data;
         // We don't show "participants" for DMs, we just show the
         // "in this narrow" section (i.e. everyone in the conversation).
@@ -268,11 +276,16 @@ export class BuddyList extends BuddyListConf {
             return this.render_data.total_human_subscribers_count;
         }
         const participant_ids_list = [...get_all_participant_ids()];
-        const subscribed_human_participant_ids = participant_ids_list.filter(
+        const subscribed_human_participant_ids = [];
-            (user_id) =>
+        for (const user_id of participant_ids_list) {
-                peer_data.is_user_subscribed(current_sub.stream_id, user_id) &&
+            const is_subscribed = await peer_data.maybe_fetch_is_user_subscribed(
-                !people.is_valid_bot_user(user_id),
+                current_sub.stream_id,
-        );
+                user_id,
+            );
+            if (is_subscribed && !people.is_valid_bot_user(user_id)) {
+                subscribed_human_participant_ids.push(user_id);
+            }
+        }
         return (
             this.render_data.total_human_subscribers_count - subscribed_human_participant_ids.length
         );
@@ -327,10 +340,10 @@ export class BuddyList extends BuddyListConf {
         $("#buddy-list-users-matching-view-container .view-all-subscribers-link").remove();
         $("#buddy-list-other-users-container .view-all-users-link").remove();
         if (!buddy_data.get_is_searching_users()) {
-            this.render_view_user_list_links();
+            void this.render_view_user_list_links();
         }
         this.display_or_hide_sections();
-        this.update_empty_list_placeholders();
+        void this.update_empty_list_placeholders();
 
         // `populate` always rerenders all user rows, so we need new load handlers.
         // This logic only does something is a user has enabled the setting to
@@ -349,7 +362,7 @@ export class BuddyList extends BuddyListConf {
     // Otherwise we hide sections with no users in them, except the "this
     // channel" section, since it could confuse users to show other sections
     // without that one.
-    update_empty_list_placeholders(): void {
+    async update_empty_list_placeholders(): Promise<void> {
         function add_or_update_empty_list_placeholder(
             list_selector: string,
             message: string,
@@ -378,9 +391,10 @@ export class BuddyList extends BuddyListConf {
         }
 
         const {get_all_participant_ids} = this.render_data;
-        if (this.non_participant_users_matching_view_count() > 0) {
+        $("#buddy-list-users-matching-view .empty-list-message").remove();
+        if ((await this.non_participant_users_matching_view_count()) > 0) {
             // There are more subscribers, so we don't need an empty list message.
-            $("#buddy-list-users-matching-view .empty-list-message").remove();
+            return;
         } else if (get_all_participant_ids().size > 0) {
             add_or_update_empty_list_placeholder(
                 "#buddy-list-users-matching-view",
@@ -396,13 +410,20 @@ export class BuddyList extends BuddyListConf {
         }
     }
 
-    update_section_header_counts(): void {
+    async update_section_header_counts(): Promise<void> {
-        const {other_users_count} = this.render_data;
+        const {other_users_count, current_sub} = this.render_data;
         const all_participant_ids = this.render_data.get_all_participant_ids();
 
+        // Hide the counts until we have the data
+        if (current_sub && !peer_data.has_full_subscriber_data(current_sub.stream_id)) {
+            $(".buddy-list-heading-user-count-with-parens").addClass("hide");
+        }
+
         const formatted_participants_count = get_formatted_user_count(all_participant_ids.size);
+        const non_participant_users_matching_view_count =
+            await this.non_participant_users_matching_view_count();
         const formatted_matching_users_count = get_formatted_user_count(
-            this.non_participant_users_matching_view_count(),
+            non_participant_users_matching_view_count,
         );
         const formatted_other_users_count = get_formatted_user_count(other_users_count);
 
@@ -415,6 +436,7 @@ export class BuddyList extends BuddyListConf {
         $("#buddy-list-other-users-container .buddy-list-heading-user-count").text(
             formatted_other_users_count,
         );
+        $(".buddy-list-heading-user-count-with-parens").removeClass("hide");
 
         $("#buddy-list-participants-section-heading").attr(
             "data-user-count",
@@ -422,7 +444,7 @@ export class BuddyList extends BuddyListConf {
         );
         $("#buddy-list-users-matching-view-section-heading").attr(
             "data-user-count",
-            this.non_participant_users_matching_view_count(),
+            non_participant_users_matching_view_count,
         );
         $("#buddy-list-users-other-users-section-heading").attr(
             "data-user-count",
@@ -430,20 +452,19 @@ export class BuddyList extends BuddyListConf {
         );
     }
 
-    render_section_headers(): void {
+    async render_section_headers(): Promise<void> {
         const {hide_headers} = this.render_data;
-        const all_participant_ids = this.render_data.get_all_participant_ids();
 
         // If we're not changing filters, this just means some users were added or
         // removed but otherwise everything is the same, so we don't need to do a full
         // rerender.
         if (this.current_filter === narrow_state.filter()) {
-            this.update_section_header_counts();
+            await this.update_section_header_counts();
             return;
         }
         this.current_filter = narrow_state.filter();
 
-        const {current_sub, other_users_count} = this.render_data;
+        const {current_sub} = this.render_data;
         $(".buddy-list-subsection-header").empty();
         $(".buddy-list-subsection-header").toggleClass("no-display", hide_headers);
         if (hide_headers) {
@@ -455,7 +476,6 @@ export class BuddyList extends BuddyListConf {
                 render_section_header({
                     id: "buddy-list-participants-section-heading",
                     header_text: $t({defaultMessage: "THIS CONVERSATION"}),
-                    user_count: get_formatted_user_count(all_participant_ids.size),
                     is_collapsed: this.participants_is_collapsed,
                 }),
             ),
@@ -468,9 +488,6 @@ export class BuddyList extends BuddyListConf {
                     header_text: current_sub
                         ? $t({defaultMessage: "THIS CHANNEL"})
                         : $t({defaultMessage: "THIS CONVERSATION"}),
-                    user_count: get_formatted_user_count(
-                        this.non_participant_users_matching_view_count(),
-                    ),
                     is_collapsed: this.users_matching_view_is_collapsed,
                 }),
             ),
@@ -481,11 +498,11 @@ export class BuddyList extends BuddyListConf {
                 render_section_header({
                     id: "buddy-list-other-users-section-heading",
                     header_text: $t({defaultMessage: "OTHERS"}),
-                    user_count: get_formatted_user_count(other_users_count),
                     is_collapsed: this.other_users_is_collapsed,
                 }),
             ),
         );
+        await this.update_section_header_counts();
     }
 
     set_section_collapse(container_selector: string, is_collapsed: boolean): void {
@@ -638,10 +655,11 @@ export class BuddyList extends BuddyListConf {
         $("#buddy-list-other-users-container").toggleClass("no-display", hide_other_users);
     }
 
-    render_view_user_list_links(): void {
+    async render_view_user_list_links(): Promise<void> {
         const {current_sub, other_users_count} = this.render_data;
         const has_inactive_users_matching_view =
-            this.non_participant_users_matching_view_count() > this.users_matching_view_ids.length;
+            (await this.non_participant_users_matching_view_count()) >
+            this.users_matching_view_ids.length;
         const has_inactive_other_users = other_users_count > this.other_user_ids.length;
 
         // For stream views, we show a link at the bottom of the list of subscribed users that
@@ -989,8 +1007,8 @@ export class BuddyList extends BuddyListConf {
         }
 
         this.display_or_hide_sections();
-        this.update_empty_list_placeholders();
+        void this.update_empty_list_placeholders();
-        this.render_section_headers();
+        void this.render_section_headers();
     }
 
     rerender_participants(): void {
@@ -1035,7 +1053,7 @@ export class BuddyList extends BuddyListConf {
                 chunk_size,
             });
         }
-        this.render_section_headers();
+        void this.render_section_headers();
     }
 
     start_scroll_handler(): void {

web/src/peer_data.ts

@@ -1,5 +1,10 @@
+import assert from "minimalistic-assert";
+import {z} from "zod";
+
 import * as blueslip from "./blueslip.ts";
+import * as channel from "./channel.ts";
 import {LazySet} from "./lazy_set.ts";
+import {page_params} from "./page_params.ts";
 import type {User} from "./people.ts";
 import * as people from "./people.ts";
 import * as sub_store from "./sub_store.ts";
@@ -7,9 +12,47 @@ import * as sub_store from "./sub_store.ts";
 // This maps a stream_id to a LazySet of user_ids who are subscribed.
 const stream_subscribers = new Map<number, LazySet>();
 const fetched_stream_ids = new Set<number>();
+export function has_full_subscriber_data(stream_id: number): boolean {
+    return fetched_stream_ids.has(stream_id);
+}
+const pending_subscriber_requests = new Map<number, Promise<LazySet>>();
 
 export function clear_for_testing(): void {
     stream_subscribers.clear();
+    fetched_stream_ids.clear();
+}
+
+const fetch_stream_subscribers_response_schema = z.object({
+    subscribers: z.array(z.number()),
+});
+
+export async function maybe_fetch_stream_subscribers(stream_id: number): Promise<LazySet> {
+    if (pending_subscriber_requests.has(stream_id)) {
+        return pending_subscriber_requests.get(stream_id)!;
+    }
+    const promise = (async () => {
+        let subscribers: number[];
+        try {
+            const xhr = await channel.get({
+                url: `/json/streams/${stream_id}/members`,
+            });
+            subscribers = fetch_stream_subscribers_response_schema.parse(xhr).subscribers;
+        } catch {
+            blueslip.error("Failure fetching channel subscribers", {
+                stream_id,
+            });
+            pending_subscriber_requests.delete(stream_id);
+            // Fall back to what we already have.
+            return get_loaded_subscriber_subset(stream_id);
+        }
+
+        set_subscribers(stream_id, subscribers);
+        pending_subscriber_requests.delete(stream_id);
+        return new LazySet(subscribers);
+    })();
+
+    pending_subscriber_requests.set(stream_id, promise);
+    return promise;
 }
 
 function get_loaded_subscriber_subset(stream_id: number): LazySet {
@@ -31,6 +74,17 @@ function get_loaded_subscriber_subset(stream_id: number): LazySet {
     return subscribers;
 }
 
+async function get_full_subscriber_set(stream_id: number): Promise<LazySet> {
+    assert(!page_params.is_spectator);
+    // This function parallels `get_loaded_subscriber_subset` but ensures we include all
+    // subscribers, possibly fetching that data from the server.
+    if (!fetched_stream_ids.has(stream_id) && sub_store.get(stream_id)) {
+        const fetched_subscribers = await maybe_fetch_stream_subscribers(stream_id);
+        stream_subscribers.set(stream_id, fetched_subscribers);
+    }
+    return get_loaded_subscriber_subset(stream_id);
+}
+
 export function is_subscriber_subset(stream_id1: number, stream_id2: number): boolean {
     const sub1_set = get_loaded_subscriber_subset(stream_id1);
     const sub2_set = get_loaded_subscriber_subset(stream_id2);
@@ -169,6 +223,17 @@ export function is_user_subscribed(stream_id: number, user_id: number): boolean
     return subscribers.has(user_id);
 }
 
+// TODO: If the server sends us a list of users for whom we have complete data,
+// we can use that to avoid waiting for the `get_full_subscriber_set` check. We'd
+// like to add that optimization in the future.
+export async function maybe_fetch_is_user_subscribed(
+    stream_id: number,
+    user_id: number,
+): Promise<boolean> {
+    const subscribers = await get_full_subscriber_set(stream_id);
+    return subscribers.has(user_id);
+}
+
 export function get_unique_subscriber_count_for_streams(stream_ids: number[]): number {
     const valid_subscribers = new LazySet([]);
 

web/templates/buddy_list/section_header.hbs

@@ -1,7 +1,8 @@
 <i class="buddy-list-section-toggle zulip-icon zulip-icon-heading-triangle-right {{#if is_collapsed}}rotate-icon-right{{else}}rotate-icon-down{{/if}}" aria-hidden="true"></i>
-<h5 id="{{id}}" data-user-count="{{user_count}}" class="buddy-list-heading no-style hidden-for-spectators">
+<h5 id="{{id}}" class="buddy-list-heading no-style hidden-for-spectators">
     <span class="buddy-list-heading-text">{{header_text}}</span>
-    <span class="buddy-list-heading-user-count-with-parens">
+    {{!-- Hide the count until we have fetched data to display the correct count --}}
-        (<span class="buddy-list-heading-user-count">{{user_count}}</span>)
+    <span class="buddy-list-heading-user-count-with-parens hide">
+        (<span class="buddy-list-heading-user-count"></span>)
     </span>
 </h5>

web/tests/peer_data.test.cjs

@@ -9,11 +9,13 @@
 const assert = require("node:assert/strict");
 
 const example_settings = require("./lib/example_settings.cjs");
-const {zrequire} = require("./lib/namespace.cjs");
+const {mock_esm, zrequire} = require("./lib/namespace.cjs");
 const {run_test} = require("./lib/test.cjs");
 const blueslip = require("./lib/zblueslip.cjs");
 const {page_params} = require("./lib/zpage_params.cjs");
 
+const channel = mock_esm("../src/channel");
+
 const peer_data = zrequire("peer_data");
 const people = zrequire("people");
 const {set_current_user, set_realm} = zrequire("state_data");
@@ -83,7 +85,7 @@ function test(label, f) {
         );
         override(realm, "realm_can_access_all_users_group", nobody_group.id);
 
-        f({override});
+        return f({override});
     });
 }
 
@@ -240,6 +242,47 @@ test("subscribers", () => {
     blueslip.reset();
 });
 
+test("maybe_fetch_stream_subscribers", async () => {
+    const india = {
+        stream_id: 102,
+        name: "India",
+        subscribed: true,
+    };
+    stream_data.add_sub(india);
+    let channel_get_calls = 0;
+    channel.get = (opts) => {
+        assert.equal(opts.url, `/json/streams/${india.stream_id}/members`);
+        channel_get_calls += 1;
+        return {
+            subscribers: [1, 2, 3, 4],
+        };
+    };
+
+    // Only one of these will do the fetch, and the other will wait
+    // for the first fetch to complete.
+    const promise1 = peer_data.maybe_fetch_stream_subscribers(india.stream_id);
+    const promise2 = peer_data.maybe_fetch_stream_subscribers(india.stream_id);
+    await promise1;
+    await promise2;
+    assert.equal(channel_get_calls, 1);
+
+    peer_data.clear_for_testing();
+    assert.equal(await peer_data.maybe_fetch_is_user_subscribed(india.stream_id, 2), true);
+    assert.equal(peer_data.has_full_subscriber_data(india.stream_id), true);
+
+    peer_data.clear_for_testing();
+    assert.equal(peer_data.has_full_subscriber_data(india.stream_id), false);
+    assert.equal(await peer_data.maybe_fetch_is_user_subscribed(india.stream_id, 2), true);
+    assert.equal(await peer_data.maybe_fetch_is_user_subscribed(india.stream_id, 5), false);
+
+    channel.get = () => {
+        throw new Error("error");
+    };
+    peer_data.clear_for_testing();
+    blueslip.expect("error", "Failure fetching channel subscribers");
+    peer_data.maybe_fetch_stream_subscribers(india.stream_id);
+});
+
 test("get_subscriber_count", () => {
     people.add_active_user(fred);
     people.add_active_user(gail);