stream_create: Use ListWidget to render list of all users.

This improves the UX of creating a stream for atleast 1000+ users realm by showing the the stream creation form much faster than before. Search, user addition, scrolling worked smoothly on 15k+ users realm as tested on dev setup. Also, simplebar is used to replace the default scrollbar. Fixes #16805
2025-11-04 05:53:43 +00:00 · 2021-04-22 21:40:09 +00:00
parent 7a58446c82
commit 384156c307
9 changed files with 150 additions and 117 deletions
--- a/frontend_tests/node_tests/list_widget.js
+++ b/frontend_tests/node_tests/list_widget.js
@@ -219,6 +219,7 @@ run_test("filtering", () => {
    // is a glorified indexOf call.)
    search_input.val = () => "g";
    search_input.simulate_input_event();
+    assert.deepEqual(widget.get_current_list(), ["dog", "egg", "grape"]);
    expected_html = "<div>dog</div><div>egg</div><div>grape</div>";
    assert.deepEqual(container.appended_data.html(), expected_html);

--- a/frontend_tests/node_tests/people.js
+++ b/frontend_tests/node_tests/people.js
@@ -492,9 +492,27 @@ test_people("get_people_for_stream_create", () => {

    const others = people.get_people_for_stream_create();
    const expected = [
-        {email: "alice1@example.com", user_id: alice1.user_id, full_name: "Alice"},
-        {email: "alice2@example.com", user_id: alice2.user_id, full_name: "Alice"},
-        {email: "bob@example.com", user_id: bob.user_id, full_name: "Bob van Roberts"},
+        {
+            email: "alice1@example.com",
+            user_id: alice1.user_id,
+            full_name: "Alice",
+            checked: false,
+            disabled: false,
+        },
+        {
+            email: "alice2@example.com",
+            user_id: alice2.user_id,
+            full_name: "Alice",
+            checked: false,
+            disabled: false,
+        },
+        {
+            email: "bob@example.com",
+            user_id: bob.user_id,
+            full_name: "Bob van Roberts",
+            checked: false,
+            disabled: false,
+        },
    ];
    assert.deepEqual(others, expected);
 });
--- a/frontend_tests/puppeteer_tests/subscriptions.ts
+++ b/frontend_tests/puppeteer_tests/subscriptions.ts
@@ -6,11 +6,11 @@ import common from "../puppeteer_lib/common";

 async function user_checkbox(page: Page, name: string): Promise<string> {
    const user_id = await common.get_user_id_from_name(page, name);
-    return `#user-checkboxes [data-user-id="${CSS.escape(user_id.toString())}"]`;
+    return `#user_checkbox_${CSS.escape(user_id.toString())}`;
 }

 async function user_span(page: Page, name: string): Promise<string> {
-    return (await user_checkbox(page, name)) + " input ~ span";
+    return (await user_checkbox(page, name)) + " span";
 }

 async function stream_checkbox(page: Page, stream_name: string): Promise<string> {
@@ -26,7 +26,7 @@ async function wait_for_checked(page: Page, user_name: string, is_checked: boole
    const selector = await user_checkbox(page, user_name);
    await page.waitForFunction(
        (selector: string, is_checked: boolean) =>
-            $(selector).find("input")[0].checked === is_checked,
+            $(selector).find("input").prop("checked") === is_checked,
        {},
        selector,
        is_checked,
@@ -98,17 +98,11 @@ async function open_copy_from_stream_dropdown(
    await page.waitForSelector(rome_checkbox, {visible: true});
 }

-async function test_check_all_only_affects_visible_users(page: Page): Promise<void> {
-    await page.click(".subs_set_all_users");
+async function verify_check_all_only_affects_visible_users(page: Page): Promise<void> {
    await wait_for_checked(page, "cordelia", false);
    await wait_for_checked(page, "othello", true);
 }

-async function test_uncheck_all(page: Page): Promise<void> {
-    await page.click(".subs_unset_all_users");
-    await wait_for_checked(page, "othello", false);
-}
-
 async function clear_ot_filter_with_backspace(page: Page): Promise<void> {
    await page.click(".add-user-list-filter");
    await page.keyboard.press("Backspace");
@@ -132,6 +126,8 @@ async function test_user_filter_ui(
    rome_checkbox: string,
 ): Promise<void> {
    await page.waitForSelector("form#stream_creation_form", {visible: true});
+    // Desdemona should be checked by default
+    await wait_for_checked(page, "desdemona", true);

    await common.fill_form(page, "form#stream_creation_form", {user_list_filter: "ot"});
    await page.waitForSelector("#user-checkboxes", {visible: true});
@@ -142,11 +138,17 @@ async function test_user_filter_ui(
    await page.waitForSelector(scotland_checkbox, {visible: true});
    await page.waitForSelector(rome_checkbox, {visible: true});

-    await test_check_all_only_affects_visible_users(page);
-    await test_uncheck_all(page);
-
+    // Test check all
+    await page.click(".subs_set_all_users");
    await clear_ot_filter_with_backspace(page);
    await verify_filtered_users_are_visible_again(page, cordelia_checkbox, othello_checkbox);
+    await verify_check_all_only_affects_visible_users(page);
+
+    // Test unset all
+    await page.click(".subs_unset_all_users");
+    await verify_filtered_users_are_visible_again(page, cordelia_checkbox, othello_checkbox);
+    await wait_for_checked(page, "cordelia", false);
+    await wait_for_checked(page, "othello", false);
 }

 async function create_stream(page: Page): Promise<void> {
@@ -157,8 +159,10 @@ async function create_stream(page: Page): Promise<void> {
    });
    await page.click(await stream_span(page, "Scotland")); //  Subscribes all users from Scotland
    await page.click(await user_span(page, "cordelia")); // Add cordelia.
-    await wait_for_checked(page, "cordelia", true);
+    await page.click(await user_span(page, "desdemona")); // Add cordelia.
    await page.click(await user_span(page, "othello")); // Remove othello who was selected from Scotland.
+    await wait_for_checked(page, "cordelia", true);
+    await wait_for_checked(page, "desdemona", true); // Add desdemona back as we did unset all in last test.
    await wait_for_checked(page, "othello", false);
    await page.click("form#stream_creation_form button.button.sea-green");
    await page.waitForFunction(() => $(".stream-name").is(':contains("Puppeteer")'));
--- a/static/js/list_widget.js
+++ b/static/js/list_widget.js
@@ -166,6 +166,10 @@ export function create($container, list, opts) {

    const widget = {};

+    widget.get_current_list = function () {
+        return meta.filtered_list;
+    };
+
    widget.filter_and_sort = function () {
        meta.filtered_list = get_filtered_items(meta.filter_value, meta.list, opts);

--- a/static/js/people.js
+++ b/static/js/people.js
@@ -1061,6 +1061,8 @@ export function get_people_for_stream_create() {
                email: person.email,
                user_id: person.user_id,
                full_name: person.full_name,
+                checked: false,
+                disabled: false,
            });
        }
    }
--- a/static/js/stream_create.js
+++ b/static/js/stream_create.js
@@ -1,12 +1,13 @@
 import $ from "jquery";

 import render_announce_stream_docs from "../templates/announce_stream_docs.hbs";
+import render_new_stream_user from "../templates/new_stream_user.hbs";
 import render_new_stream_users from "../templates/new_stream_users.hbs";
 import render_subscription_invites_warning_modal from "../templates/subscription_invites_warning_modal.hbs";

-import * as blueslip from "./blueslip";
 import * as channel from "./channel";
 import {$t, $t_html} from "./i18n";
+import * as ListWidget from "./list_widget";
 import * as loading from "./loading";
 import {page_params} from "./page_params";
 import * as peer_data from "./peer_data";
@@ -17,6 +18,8 @@ import * as subs from "./subs";
 import * as ui_report from "./ui_report";

 let created_stream;
+let all_users;
+let all_users_list_widget;

 export function reset_created_stream() {
    created_stream = undefined;
@@ -142,10 +145,8 @@ function update_announce_stream_state() {
 }

 function get_principals() {
-    return Array.from($("#stream_creation_form input:checkbox[name=user]:checked"), (elem) => {
-        const label = $(elem).closest(".add-user-label");
-        return Number.parseInt(label.attr("data-user-id"), 10);
-    });
+    // Return list of user ids which were selected by user.
+    return all_users.filter((user) => user.checked === true).map((user) => user.user_id);
 }

 function create_stream() {
@@ -278,20 +279,41 @@ export function show_new_stream_modal() {
    $("#stream-creation").removeClass("hide");
    $(".right .settings").hide();

-    const html = blueslip.measure_time("render new stream users", () => {
-        const all_users = people.get_people_for_stream_create();
-        // Add current user on top of list
-        all_users.unshift(people.get_by_user_id(page_params.user_id));
-        return render_new_stream_users({
-            users: all_users,
+    const add_people_container = $("#people_to_add");
+    add_people_container.html(
+        render_new_stream_users({
            streams: stream_settings_data.get_streams_for_settings_page(),
-            is_admin: page_params.is_admin,
-        });
+        }),
+    );
+
+    all_users = people.get_people_for_stream_create();
+    // Add current user on top of list
+    const current_user = people.get_by_user_id(page_params.user_id);
+    all_users.unshift({
+        email: current_user.email,
+        user_id: current_user.user_id,
+        full_name: current_user.full_name,
+        checked: true,
+        disabled: !page_params.is_admin,
    });

-    const container = $("#people_to_add");
-    container.html(html);
-    create_handlers_for_users(container);
+    all_users_list_widget = ListWidget.create($("#user-checkboxes"), all_users, {
+        name: "new_stream_add_users",
+        parent_container: add_people_container,
+        modifier(item) {
+            return render_new_stream_user(item);
+        },
+        filter: {
+            element: $("#people_to_add .add-user-list-filter"),
+            predicate(user, search_term) {
+                return people.build_person_matcher(search_term)(user);
+            },
+        },
+        simplebar_container: $("#user-checkboxes-simplebar-wrapper"),
+        html_selector: (user) => $(`#${CSS.escape("user_checkbox_" + user.user_id)}`),
+    });
+
+    create_handlers_for_users(add_people_container);

    // Make the options default to the same each time:
    // public, "announce stream" on.
@@ -299,6 +321,7 @@ export function show_new_stream_modal() {
    $("#stream_creation_form .stream-message-retention-days-input").hide();
    $("#stream_creation_form select[name=stream_message_retention_setting]").val("realm_default");

+    update_announce_stream_state();
    if (stream_data.realm_has_notifications_stream()) {
        $("#announce-new-stream").show();
        $("#announce-new-stream input").prop("disabled", false);
@@ -307,99 +330,76 @@ export function show_new_stream_modal() {
        $("#announce-new-stream").hide();
    }
    clear_error_display();
-
-    $("#stream-checkboxes label.checkbox").on("change", function (e) {
-        const elem = $(this);
-        const stream_id = Number.parseInt(elem.attr("data-stream-id"), 10);
-        const checked = elem.find("input").prop("checked");
-        const subscriber_ids = new Set(peer_data.get_subscribers(stream_id));
-
-        $("#user-checkboxes label.checkbox").each(function () {
-            const user_elem = $(this);
-            const user_id = Number.parseInt(user_elem.attr("data-user-id"), 10);
-
-            if (subscriber_ids.has(user_id)) {
-                user_elem.find("input").prop("checked", checked);
-            }
-        });
-
-        e.preventDefault();
-    });
 }

 export function create_handlers_for_users(container) {
    // container should be $('#people_to_add')...see caller to verify
-    container.on("change", "#user-checkboxes input", update_announce_stream_state);
+    function update_checked_state_for_users(value, users) {
+        // Update the all_users backing data structure for
+        // which users will be submitted should the user click save,
+        // and also ensure that any visible checkboxes reflect
+        // the state of that data structure.

-    // 'Check all' and 'Uncheck all' visible users
-    container.on("click", ".subs_set_all_users", (e) => {
-        $("#user-checkboxes .checkbox").each((idx, li) => {
-            if (li.style.display !== "none") {
-                $(li.firstElementChild).prop("checked", true);
+        // If we have to rerender a very large number of users, it's
+        // eventually faster to just do a full redraw rather than
+        // many hundreds of single-item rerenders.
+        const full_redraw = !users || users.length > 250;
+        for (const user of all_users) {
+            // We don't want to uncheck the user creating the stream if it is not admin.
+            if (user.user_id === page_params.user_id && value === false && !page_params.is_admin) {
+                continue;
            }
-        });
-        e.preventDefault();
-        update_announce_stream_state();
+            // We update for all users if `users` parameter is empty.
+            if (users === undefined || users.includes(user.user_id)) {
+                user.checked = value;
+
+                if (!full_redraw) {
+                    all_users_list_widget.render_item(user);
+                }
+            }
+        }
+
+        if (full_redraw) {
+            all_users_list_widget.hard_redraw();
+        }
+    }
+
+    container.on("change", "#user-checkboxes input", (e) => {
+        const elem = $(e.target);
+        const user_id = Number.parseInt(elem.attr("data-user-id"), 10);
+        const checked = elem.prop("checked");
+        update_checked_state_for_users(checked, [user_id]);
    });

-    container.on("click", ".subs_unset_all_users", (e) => {
-        $("#user-checkboxes .checkbox").each((idx, li) => {
-            if (li.style.display !== "none") {
-                // The first checkbox is the one for ourself; this is the code path for:
-                // `stream_subscription_error.cant_create_stream_without_susbscribing`
-                if (idx === 0 && !page_params.is_admin) {
-                    return;
-                }
-                $(li.firstElementChild).prop("checked", false);
-            }
-        });
+    // 'Check all' and 'Uncheck all' visible users
+    container.on("click", ".subs_set_all_users, .subs_unset_all_users", (e) => {
        e.preventDefault();
-        update_announce_stream_state();
+        // Only `check / uncheck` users who are displayed.
+        const mark_checked = e.target.classList.contains("subs_set_all_users");
+        const users_displayed = all_users_list_widget.get_current_list();
+        if (all_users.length !== users_displayed.length) {
+            update_checked_state_for_users(
+                mark_checked,
+                users_displayed.map((user) => user.user_id),
+            );
+        } else {
+            update_checked_state_for_users(mark_checked);
+        }
    });

    container.on("click", "#copy-from-stream-expand-collapse", (e) => {
+        e.preventDefault();
        $("#stream-checkboxes").toggle();
        $("#copy-from-stream-expand-collapse .toggle").toggleClass("fa-caret-right fa-caret-down");
-        e.preventDefault();
    });

-    // Search people or streams
-    container.on("input", ".add-user-list-filter", (e) => {
-        const user_list = $(".add-user-list-filter");
-        if (user_list === 0) {
-            return;
-        }
-        const search_term = user_list.expectOne().val().trim();
-        const search_terms = search_term.toLowerCase().split(",");
-
-        (function filter_user_checkboxes() {
-            const user_labels = $("#user-checkboxes label.add-user-label");
-
-            if (search_term === "") {
-                user_labels.css({display: "block"});
-                return;
-            }
-
-            const users = people.get_people_for_stream_create();
-            const filtered_users = people.filter_people_by_search_terms(users, search_terms);
-
-            // Be careful about modifying the follow code.  A naive implementation
-            // will work very poorly with a large user population (~1000 users).
-            //
-            // I tested using: `./manage.py populate_db --extra-users 3500`
-            //
-            // This would break the previous implementation, whereas the new
-            // implementation is merely sluggish.
-            user_labels.each(function () {
-                const elem = $(this);
-                const user_id = Number.parseInt(elem.attr("data-user-id"), 10);
-                const user_checked = filtered_users.has(user_id);
-                const display = user_checked ? "block" : "none";
-                elem.css({display});
-            });
-        })();
-
+    $("#stream-checkboxes label.checkbox").on("change", (e) => {
        e.preventDefault();
+        const elem = $(e.target).closest("[data-stream-id]");
+        const stream_id = Number.parseInt(elem.attr("data-stream-id"), 10);
+        const checked = elem.find("input").prop("checked");
+        const subscriber_ids = peer_data.get_subscribers(stream_id);
+        update_checked_state_for_users(checked, subscriber_ids);
    });
 }

--- a/static/styles/zulip.css
+++ b/static/styles/zulip.css
@@ -2194,6 +2194,11 @@ div.floating_recipient {
    color: hsl(0, 0%, 100%);
 }

+#user-checkboxes-simplebar-wrapper {
+    max-height: 500px;
+    overflow-y: auto;
+}
+
 #user-checkboxes {
    margin-top: 10px;

--- a/static/templates/new_stream_user.hbs
+++ b/static/templates/new_stream_user.hbs
@@ -0,0 +1,5 @@
+<label class="checkbox add-user-label" id="user_checkbox_{{user_id}}">
+    <input type="checkbox" name="user" {{#if checked}}checked="checked"{{#if disabled}} disabled="disabled"{{/if}}{{/if}} data-user-id="{{user_id}}"/>
+    <span></span>
+    {{full_name}} ({{email}})
+</label>
--- a/static/templates/new_stream_users.hbs
+++ b/static/templates/new_stream_users.hbs
@@ -27,12 +27,6 @@
    <a href="#" draggable="false" class="subs_unset_all_users">{{t "Uncheck all" }}</a>
 </div>

-<div id="user-checkboxes">
-    {{#each users}}
-    <label class="checkbox add-user-label" data-user-id="{{this.user_id}}">
-        <input type="checkbox" name="user" {{#if @first}}checked="checked"{{#unless is_admin}} disabled="disabled"{{/unless}}{{/if}}/>
-        <span></span>
-        {{this.full_name}} ({{this.email}})
-    </label>
-    {{/each}}
+<div id="user-checkboxes-simplebar-wrapper" data-simplebar>
+    <div id="user-checkboxes"></div>
 </div>