From 44b87c72dc0d1d0f236bbdeb1c68449ed6fd9dd4 Mon Sep 17 00:00:00 2001 From: Evy Kassirer Date: Wed, 9 Apr 2025 19:04:19 -0700 Subject: [PATCH] stream_edit: Fetch subscribers before showing subscriber tab. Work towards #34244. Now that we're supporting partial subscriber data, we might need to fetch the full list of subscribers when opening the subscribers tab of the edit channel modal. This commit handles a slow load with a loading spinner while we fetch the data, and also makes sure to ignore the data if it's received after it stops being relevant (in case the user has another stream's data open). --- web/src/peer_data.ts | 15 +++++++ web/src/stream_edit_subscribers.ts | 44 +++++++++++++++---- web/styles/subscriptions.css | 11 +++++ .../stream_settings/stream_members.hbs | 3 +- web/tests/peer_data.test.cjs | 13 ++++++ 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/web/src/peer_data.ts b/web/src/peer_data.ts index bf21e1117a..d1cb13486d 100644 --- a/web/src/peer_data.ts +++ b/web/src/peer_data.ts @@ -216,6 +216,21 @@ export function get_subscribers(stream_id: number): number[] { return [...subscribers.keys()]; } +export async function get_all_subscribers( + stream_id: number, + retry_on_failure = true, +): Promise { + // This function parallels `get_subscribers` but ensures we include all + // subscribers, possibly fetching that data from the server. + const subscribers = await get_full_subscriber_set(stream_id, retry_on_failure); + // This means the request failed, which can only happen if `retry_on_failure` + // is false. + if (subscribers === null) { + return null; + } + return [...subscribers.keys()]; +} + export function set_subscribers(stream_id: number, user_ids: number[], full_data = true): void { const subscribers = new LazySet(user_ids); stream_subscribers.set(stream_id, subscribers); diff --git a/web/src/stream_edit_subscribers.ts b/web/src/stream_edit_subscribers.ts index 476768d99d..7c66e0de29 100644 --- a/web/src/stream_edit_subscribers.ts +++ b/web/src/stream_edit_subscribers.ts @@ -15,6 +15,7 @@ import * as hash_parser from "./hash_parser.ts"; import {$t, $t_html} from "./i18n.ts"; import * as ListWidget from "./list_widget.ts"; import type {ListWidget as ListWidgetType} from "./list_widget.ts"; +import * as loading from "./loading.ts"; import * as peer_data from "./peer_data.ts"; import * as people from "./people.ts"; import type {User} from "./people.ts"; @@ -128,8 +129,30 @@ export function enable_subscriber_management({ $parent_container.find(".stream_subscription_request_result").empty(); }); - const user_ids = peer_data.get_subscribers(stream_id); const user_can_remove_subscribers = stream_data.can_unsubscribe_others(sub); + void render_subscriber_list_widget(sub, user_can_remove_subscribers, $parent_container); +} + +async function render_subscriber_list_widget( + sub: StreamSubscription, + user_can_remove_subscribers: boolean, + $parent_container: JQuery, +): Promise { + $(".subscriber_list_settings_container").toggleClass("no-display", true); + loading.make_indicator($(".subscriber-list-settings-loading"), { + text: $t({defaultMessage: "Loading…"}), + }); + + // Because we're using `retry_on_failure=true`, this will only return once it + // succeeds, so we can't get `null`. + const user_ids = await peer_data.get_all_subscribers(sub.stream_id, true); + assert(user_ids !== null); + + // Make sure we're still editing this stream after waiting for subscriber data. + if (!hash_parser.is_editing_stream(sub.stream_id)) { + blueslip.info("ignoring subscriber data for stream that is no longer being edited"); + return; + } // We track a single subscribers_list_widget for this module, since we // only ever have one list of subscribers visible at a time. @@ -139,6 +162,8 @@ export function enable_subscriber_management({ user_ids, user_can_remove_subscribers, }); + loading.destroy_indicator($(".subscriber-list-settings-loading")); + $(".subscriber_list_settings_container").toggleClass("no-display", false); } function make_list_widget({ @@ -424,16 +449,23 @@ export function update_subscribers_list(sub: StreamSubscription): void { // from an existing stream or a user group. const subscriber_ids = peer_data.get_subscribers(sub.stream_id); update_subscribers_list_widget(subscriber_ids); - $(".subscriber_list_settings_container").show(); } } function update_subscribers_list_widget(subscriber_ids: number[]): void { + // This can happen if we're still fetching user ids in + // `render_subscriber_list_widget`, but we'll render the widget with + // fetched ids after the fetch is complete, so we don't need to do + // anything here. + if (subscribers_list_widget === undefined) { + return; + } // This re-renders the subscribers_list_widget with a new // list of subscriber_ids. const users = people.get_users_from_ids(subscriber_ids); people.sort_but_pin_current_user_on_top(users); subscribers_list_widget.replace_list_data(users); + $(".subscriber_list_settings_container").show(); } export function rerender_subscribers_list(sub: sub_store.StreamSubscription): void { @@ -452,7 +484,6 @@ export function rerender_subscribers_list(sub: sub_store.StreamSubscription): vo return; } - const user_ids = peer_data.get_subscribers(sub.stream_id); const user_can_remove_subscribers = stream_data.can_unsubscribe_others(sub); const $parent_container = stream_settings_containers .get_edit_container(sub) @@ -463,12 +494,7 @@ export function rerender_subscribers_list(sub: sub_store.StreamSubscription): vo can_remove_subscribers: user_can_remove_subscribers, }), ); - subscribers_list_widget = make_list_widget({ - $parent_container, - name: "stream_subscribers", - user_ids, - user_can_remove_subscribers, - }); + void render_subscriber_list_widget(sub, user_can_remove_subscribers, $parent_container); } export function initialize(): void { diff --git a/web/styles/subscriptions.css b/web/styles/subscriptions.css index 65dc1f5ac2..0031ba5a08 100644 --- a/web/styles/subscriptions.css +++ b/web/styles/subscriptions.css @@ -136,6 +136,17 @@ h4.user_group_setting_subsection_title { line-height: 1.5; } +.subscriber_list_settings_container.no-display { + display: none; +} + +.subscriber-list-settings-loading { + display: flex; + align-items: center; + margin: auto; + margin-top: 100px; +} + .member-list-box, .subscriber-list-box { text-align: center; diff --git a/web/templates/stream_settings/stream_members.hbs b/web/templates/stream_settings/stream_members.hbs index 216fc15dc6..461c7286f0 100644 --- a/web/templates/stream_settings/stream_members.hbs +++ b/web/templates/stream_settings/stream_members.hbs @@ -1,5 +1,5 @@ {{#if render_subscribers}} -
+

{{t "Add subscribers" }}

@@ -20,4 +20,5 @@ {{> stream_members_table .}}
+
{{/if}} diff --git a/web/tests/peer_data.test.cjs b/web/tests/peer_data.test.cjs index a0e542a167..a3214de9bb 100644 --- a/web/tests/peer_data.test.cjs +++ b/web/tests/peer_data.test.cjs @@ -302,6 +302,19 @@ test("maybe_fetch_stream_subscribers", async () => { assert.equal(await peer_data.maybe_fetch_is_user_subscribed(india.stream_id, 2, false), true); assert.equal(await peer_data.maybe_fetch_is_user_subscribed(india.stream_id, 5, false), false); + peer_data.clear_for_testing(); + assert.deepEqual(await peer_data.get_subscribers(india.stream_id), []); + assert.deepEqual(await peer_data.get_all_subscribers(india.stream_id), [1, 2, 3, 4]); + + peer_data.clear_for_testing(); + channel.get = () => { + throw new Error("error"); + }; + blueslip.expect("error", "Failure fetching channel subscribers"); + // retry is false, so we get null because there was an error + assert.deepEqual(await peer_data.get_all_subscribers(india.stream_id, false), null); + blueslip.reset(); + channel.get = () => { throw new Error("error"); };