presence: Frontend implementation of the last_update_id API.

Note: This involves adding presence info of unknown users to the
presence data.
With some small tweaks, we can just add the info to the presence data
structures, just making sure the buddy list correctly skips those
entries and that we redraw the user in the case where the user creation
event arrives after the presence polling loop.

web/src/activity.ts

@@ -10,6 +10,13 @@ import * as watchdog from "./watchdog";
 const post_presence_response_schema = z.object({
     msg: z.string(),
     result: z.string(),
+    // A bunch of these fields below are .optional() due to the fact
+    // that we have two modes of querying the presence endpoint:
+    // ping_only mode and a mode where we also fetch presence data
+    // for the realm.
+    // For ping_only requests, these fields are not returned in the
+    // response. If we're fetching presence data however, they should
+    // all be present, and send_presence_to_server() will validate that.
     server_timestamp: z.number().optional(),
     zephyr_mirror_active: z.boolean().optional(),
     presences: z
@@ -21,6 +28,7 @@ const post_presence_response_schema = z.object({
             }),
         )
         .optional(),
+    presence_last_update_id: z.number().optional(),
 });
 
 /* Keep in sync with views.py:update_active_status_backend() */
@@ -119,6 +127,7 @@ export function send_presence_to_server(redraw?: () => void): void {
             ping_only: !redraw,
             new_user_input,
             slim_presence: true,
+            last_update_id: presence.presence_last_update_id,
         },
         success(response) {
             const data = post_presence_response_schema.parse(response);
@@ -141,7 +150,16 @@ export function send_presence_to_server(redraw?: () => void): void {
                     data.server_timestamp !== undefined,
                     "Server timestamp should be present if not a ping only presence request",
                 );
-                presence.set_info(data.presences, data.server_timestamp);
+                assert(
+                    data.presence_last_update_id !== undefined,
+                    "Presence last update id should be present if not a ping only presence request",
+                );
+
+                presence.set_info(
+                    data.presences,
+                    data.server_timestamp,
+                    data.presence_last_update_id,
+                );
                 redraw();
             }
         },

web/src/activity_ui.ts

@@ -78,6 +78,16 @@ export function redraw_user(user_id: number): void {
     });
 }
 
+export function check_should_redraw_new_user(user_id: number): boolean {
+    if (realm.realm_presence_disabled) {
+        return false;
+    }
+
+    const user_is_in_presence_info = presence.presence_info.has(user_id);
+    const user_not_yet_known = people.maybe_get_user_by_id(user_id, true) === undefined;
+    return user_is_in_presence_info && user_not_yet_known;
+}
+
 export function searching(): boolean {
     return user_filter?.searching() ?? false;
 }

web/src/buddy_data.ts

@@ -1,4 +1,3 @@
-import * as blueslip from "./blueslip";
 import * as hash_util from "./hash_util";
 import {$t} from "./i18n";
 import * as muted_users from "./muted_users";
@@ -312,10 +311,11 @@ function filter_user_ids(user_filter_text: string, user_ids: number[]): number[]
     // This first filter is for whether the user is eligible to be
     // displayed in the right sidebar at all.
     user_ids = user_ids.filter((user_id) => {
-        const person = people.maybe_get_user_by_id(user_id);
+        const person = people.maybe_get_user_by_id(user_id, true);
 
         if (!person) {
-            blueslip.warn("Got user_id in presence but not people: " + user_id);
+            // See the comments in presence.set_info for details, but this is an expected race.
+            // User IDs for whom we have presence but no user metadata should be skipped.
             return false;
         }
 

web/src/presence.ts

@@ -34,10 +34,16 @@ export type PresenceInfoFromEvent = {
 const raw_info = new Map<number, RawPresence>();
 export const presence_info = new Map<number, PresenceStatus>();
 
-// We use this internally and export it for testing convenience.
+// An integer that is updated whenever we get new presence data.
+// TODO: Improve this comment.
+export let presence_last_update_id = -1;
+
+// We keep and export this for testing convenience.
 export function clear_internal_data(): void {
     raw_info.clear();
     presence_info.clear();
+
+    presence_last_update_id = -1;
 }
 
 const BIG_REALM_COUNT = 250;
@@ -166,6 +172,7 @@ export function update_info_from_event(
 export function set_info(
     presences: Record<number, Omit<RawPresence, "server_timestamp">>,
     server_timestamp: number,
+    last_update_id: number,
 ): void {
     /*
         Example `presences` data:
@@ -177,7 +184,8 @@ export function set_info(
         }
     */
 
-    clear_internal_data();
+    presence_last_update_id = last_update_id;
+
     for (const [user_id_str, info] of Object.entries(presences)) {
         const user_id = Number.parseInt(user_id_str, 10);
 
@@ -190,19 +198,14 @@ export function set_info(
         // new users were created in the meantime, we may see user IDs
         // not yet present in people.js if server_events doesn't have
         // current data (or we've been offline, our event queue was
-        // GC'ed, and we're about to reload).  Such user_ids being
-        // present could, in turn, create spammy downstream exceptions
-        // when rendering the buddy list.  To address this, we check
-        // if the user ID is not yet present in people.js, and if it
-        // is, we skip storing that user (we'll see them again in the
-        // next presence request in 1 minute anyway).
-        //
-        // It's important to check both suspect_offline and
-        // reload_state.is_in_progress, because races where presence
-        // returns data on users not yet received via the server_events
-        // system are common in both situations.
+        // GC'ed, and we're about to reload).
+        // Despite that, we still add the presence data to our structures,
+        // and it is the job of the code using them to correctly
+        // ignore these until we receive the basic metadata on this user.
+        // We skip inaccessible users here, as we do in other places;
+        // presence info for them is not used.
         const person = people.maybe_get_user_by_id(user_id, true);
-        if (person === undefined || person.is_inaccessible_user) {
+        if (person?.is_inaccessible_user) {
             // There are a number of situations where it is expected
             // that we get presence data for a user ID that we do
             // not have in our user database, including when we're
@@ -212,8 +215,8 @@ export function set_info(
             // and whenever presence wins a race with the events system
             // for events regarding a newly created or visible user.
             //
-            // Either way, we deal by skipping this user and
-            // continuing with processing everyone else.
+            // Either way, we still record the information unless
+            // we're dealing with an inaccessible user.
             continue;
         }
 
@@ -281,6 +284,7 @@ export function last_active_date(user_id: number): Date | undefined {
 export function initialize(params: {
     presences: Record<number, Omit<RawPresence, "server_timestamp">>;
     server_timestamp: number;
+    presence_last_update_id: number;
 }): void {
-    set_info(params.presences, params.server_timestamp);
+    set_info(params.presences, params.server_timestamp, params.presence_last_update_id);
 }

web/src/server_events_dispatch.js

@@ -454,13 +454,24 @@ export function dispatch_normal_event(event) {
 
         case "realm_user":
             switch (event.op) {
-                case "add":
+                case "add": {
+                    // There may be presence data we already received from the server
+                    // before getting this event. Check if we need to redraw.
+                    const should_redraw = activity_ui.check_should_redraw_new_user(
+                        event.person.user_id,
+                    );
+
                     people.add_active_user(event.person);
                     settings_account.maybe_update_deactivate_account_button();
                     if (event.person.is_bot) {
                         settings_users.redraw_bots_list();
                     }
+
+                    if (should_redraw) {
+                        activity_ui.redraw_user(event.person.user_id);
+                    }
                     break;
+                }
                 case "update":
                     user_events.update_person(event.person);
                     settings_account.maybe_update_deactivate_account_button();

web/src/ui_init.js

@@ -480,7 +480,7 @@ export function initialize_everything(state_data) {
 
     const pm_conversations_params = pop_fields("recent_private_conversations");
 
-    const presence_params = pop_fields("presences", "server_timestamp");
+    const presence_params = pop_fields("presences", "server_timestamp", "presence_last_update_id");
 
     const starred_messages_params = pop_fields("starred_messages");
     const stream_data_params = pop_fields(

web/tests/activity.test.js

@@ -805,6 +805,7 @@ test("initialize", ({override, mock_template}) => {
         msg: "",
         result: "success",
         server_timestamp: 0,
+        presence_last_update_id: -1,
     });
     $(window).trigger("focus");
     clear();
@@ -829,6 +830,7 @@ test("initialize", ({override, mock_template}) => {
         msg: "",
         result: "success",
         server_timestamp: 0,
+        presence_last_update_id: -1,
     });
 
     assert.ok($("#zephyr-mirror-error").hasClass("show"));
@@ -881,3 +883,20 @@ test("test_send_or_receive_no_presence_for_spectator", () => {
     page_params.is_spectator = true;
     activity.send_presence_to_server();
 });
+
+test("check_should_redraw_new_user", () => {
+    presence.presence_info.set(9999, {status: "active"});
+
+    // A user that wasn't yet known, but has presence info should be redrawn
+    // when being added.
+    assert.equal(activity_ui.check_should_redraw_new_user(9999), true);
+
+    // We don't even build the user sidebar if realm_presence_disabled is true,
+    // so nothing to redraw.
+    realm.realm_presence_disabled = true;
+    assert.equal(activity_ui.check_should_redraw_new_user(9999), false);
+
+    realm.realm_presence_disabled = false;
+    // A new user that didn't have presence info should not be redrawn.
+    assert.equal(activity_ui.check_should_redraw_new_user(99999), false);
+});

web/tests/buddy_data.test.js

@@ -6,7 +6,6 @@ const _ = require("lodash");
 
 const {mock_esm, zrequire} = require("./lib/namespace");
 const {run_test} = require("./lib/test");
-const blueslip = require("./lib/zblueslip");
 const {current_user, realm, user_settings} = require("./lib/zpage_params");
 
 const timerender = mock_esm("../src/timerender");
@@ -500,9 +499,16 @@ test("get_items_for_users", () => {
     ]);
 });
 
-test("error handling", () => {
-    presence.presence_info.set(42, {status: "active"});
-    blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id");
-    blueslip.expect("warn", "Got user_id in presence but not people: 42");
-    buddy_data.get_filtered_and_sorted_user_ids();
+test("unknown user id in presence", () => {
+    // This test is to make sure that we don't generate errors
+    // when we have a user_id in presence that we don't have
+    // information about.
+    // Such scenarios can happen if we receive presence info involving
+    // a new user before the user creation event reaches us.
+    presence.presence_info.set(999, {status: "active"});
+    const user_ids = buddy_data.get_filtered_and_sorted_user_ids();
+
+    // This user id should not be present in the filtered list,
+    // it's meant to be ignored until we know about the user.
+    assert.equal(user_ids.includes(999), false);
 });

web/tests/dispatch.test.js

@@ -125,6 +125,7 @@ const alert_words = zrequire("alert_words");
 const emoji = zrequire("emoji");
 const message_store = zrequire("message_store");
 const people = zrequire("people");
+const presence = zrequire("presence");
 const user_status = zrequire("user_status");
 const onboarding_steps = zrequire("onboarding_steps");
 
@@ -728,8 +729,28 @@ run_test("realm_domains", ({override}) => {
     assert_same(realm.realm_domains, []);
 });
 
+run_test("add_realm_user_redraw_logic", ({override}) => {
+    presence.presence_info.set(999, {status: "active"});
+
+    override(settings_account, "maybe_update_deactivate_account_button", noop);
+
+    const check_should_redraw_new_user_stub = make_stub();
+    // make_stub().f returns true by default, so it's already doing what we want.
+    override(activity_ui, "check_should_redraw_new_user", check_should_redraw_new_user_stub.f);
+    const redraw_user_stub = make_stub();
+    override(activity_ui, "redraw_user", redraw_user_stub.f);
+
+    const event = event_fixtures.realm_user__add;
+    event.person.user_id = 999;
+    dispatch(event);
+
+    assert.equal(redraw_user_stub.num_calls, 1);
+    assert.equal(redraw_user_stub.get_args("user_id").user_id, 999);
+});
+
 run_test("realm_user", ({override}) => {
     override(settings_account, "maybe_update_deactivate_account_button", noop);
+    override(activity_ui, "check_should_redraw_new_user", noop);
     let event = event_fixtures.realm_user__add;
     dispatch({...event});
     const added_person = people.get_by_user_id(event.person.user_id);

web/tests/example4.test.js

@@ -86,6 +86,7 @@ run_test("add users with event", ({override}) => {
     // We need to override a stub here before dispatching the event.
     // Keep reading to see how overriding works!
     override(settings_users, "redraw_bots_list", noop);
+    override(activity_ui, "check_should_redraw_new_user", noop);
     // Let's simulate dispatching our event!
     server_events_dispatch.dispatch_normal_event(event);
 

web/tests/presence.test.js

@@ -68,6 +68,11 @@ people.add_active_user(zoe);
 people.add_active_user(bot);
 people.add_active_user(john);
 people.add_active_user(jane);
+
+const inaccessible_user_id = 9999;
+const inaccessible_user = people.add_inaccessible_user(inaccessible_user_id);
+inaccessible_user.is_inaccessible_user = true;
+
 people.initialize_current_user(me.user_id);
 
 function test(label, f) {
@@ -83,17 +88,6 @@ test("my user", () => {
     assert.equal(presence.get_status(me.user_id), "active");
 });
 
-test("unknown user", () => {
-    const unknown_user_id = 999;
-    const now = 888888;
-    const presences = {};
-    presences[unknown_user_id.toString()] = "does-not-matter";
-
-    // We just skip the unknown user.
-    presence.set_info(presences, now);
-    assert.equal(presence.presence_info.get(unknown_user_id), undefined);
-});
-
 test("status_from_raw", () => {
     const status_from_raw = presence.status_from_raw;
 
@@ -137,6 +131,8 @@ test("set_presence_info", () => {
     const recent = now + 1 - OFFLINE_THRESHOLD_SECS;
     const a_while_ago = now - OFFLINE_THRESHOLD_SECS * 2;
 
+    const unknown_user_id = 999;
+
     presences[alice.user_id.toString()] = {
         active_timestamp: recent,
     };
@@ -162,6 +158,15 @@ test("set_presence_info", () => {
         idle_timestamp: now,
     };
 
+    // Unknown user ids can also be in the presence data.
+    presences[unknown_user_id.toString()] = {
+        idle_timestamp: now,
+    };
+
+    presences[inaccessible_user_id.toString()] = {
+        idle_timestamp: now,
+    };
+
     const params = {};
     params.presences = presences;
     params.server_timestamp = now;
@@ -204,6 +209,14 @@ test("set_presence_info", () => {
 
     assert.deepEqual(presence.presence_info.get(jane.user_id), {status: "idle", last_active: now});
     assert.equal(presence.get_status(jane.user_id), "idle");
+
+    assert.deepEqual(presence.presence_info.get(unknown_user_id), {
+        status: "idle",
+        last_active: now,
+    });
+    assert.equal(presence.get_status(unknown_user_id), "idle");
+
+    assert.equal(presence.presence_info.get(inaccessible_user_id), undefined);
 });
 
 test("missing values", () => {