recent_view: Refactor conversation data to prevent fetching messages. #30137

evykassirer · 2024-05-19T19:45:34Z

The motivation is that when we're able to provide a dedicated efficient API for the recent view, it's probably not going to preserve the invariant that any topic in this view has a message in message_store.

Further discussion here

web/src/hash_util.ts

web/src/recent_view_data.ts

evykassirer · 2024-05-19T19:57:29Z

web/tests/recent_view.test.js

 const sample_messages = [];
 sample_messages[0] = {
    stream_id: stream1,
-    id: (id += 1),
+    id: 1,


I changed these to numbers so it would be easier to find the messages by their ids when setting up tests, but I can change it back if that's preferred.

evykassirer · 2024-05-19T19:58:55Z

web/src/recent_view_data.ts

+
+        if (msg.type === "private") {
+            // Display recipient contains user information for DMs.
+            assert(typeof msg.display_recipient !== "string");


This is just preserving old logic, but ideally we'd refactor the message type so that we don't have to make this assertion.

So, we need to also think about how we are going to update the properties here which can change.

Like we used to update unread_count by simply calling inplace_rerender since that would get the latest unread count from last_msg_id and update it, but that won't work anymore since we are using a cached value.

We need to hook server_events for this. We could use the process_message as an opportunity to update them but that won't be reliabl, so probably a good idea to skip updating them here.

Agreed with @amanagr that we're going to need to be very careful to make sure things that could change any of the data that you're caching here, which is quite a lot (name changes go into display_recipient, etc.).

Realistically that means this is the wrong design; we should be only caching the metadata that we think is stable, not computed properties like display_recipient, which can just as well be processed later.

Just to lay out a bit more how I'm thinking this should work: A server API endpoint that sent data on what recent conversations exist would not include the following:

unread data -- clients should combine its unread data set with the recent view data to display a complete view.

Metadata on users, like names/etc. that go into display recipients -- I'd expect it to just have stream/user IDs.

Instead, I'd expect it to just have (1) list of participated user IDs, (2) last_message_id, (3) last_message_timestamp, (4) type and location of the message.

So I think it's correct for the data structure we're defining here to have those properties as well, and rely on combining that reasonably normalized data set with the unread data set and people/stream_data data sets, just like it will with which topics are followed in order to display that detail.

I just pushed to this branch with an extra commit to try addressing this -- let me know what you think! I wasn't totally sure which data we can rely on to be stable.

amanagr · 2024-05-20T04:18:53Z

web/src/recent_view_ui.ts

-    participated: boolean;
-    type: "private" | "stream";
-};
+type Row = ConversationData;


Do we need to define this type if they are the same?

Could be an additional commit to clean that up if that's convenient.

I wasn't sure here -- they were separate types before despite being the same. Was there a reason for that? I thought maybe there was a semantic reason for it or something. Would you prefer they both be called ConversationData?

evykassirer · 2024-05-28T22:47:20Z

FYI @timabbott Aman said on CZO:

You should wait for a review from Tim Abbott before making any changes here though.

timabbott · 2024-05-29T23:53:40Z

web/src/recent_view_util.ts

+              stream_id: number;
+              topic: string;
+          },
+): string {


Should this instead just take a ConversationData? Not sure why you're defining this other abbreviated type here.

It's because we call it in process_message with a message:

const key = get_key_from_conversation_data(msg);

and ConversationData has attributes that aren't on Message which also aren't needed here, but these attributes I listed are on both types and are the only attributes we need here.

web/src/recent_view_data.ts

evykassirer · 2024-06-04T00:14:59Z

web/src/message_store.ts

@@ -19,7 +19,7 @@ export type DisplayRecipientUser = {
    email: string;
    full_name: string;
    id: number;
-    is_mirror_dummy: boolean;
+    is_mirror_dummy?: boolean;


It doesn't seem like this is used at all in the frontend, so maybe we want to remove it completely here?

Yeah, looks like we can avoid sending it to the client to reduce response data. @timabbott FYI

@evykassirer I think you should start a czo discussion to fast track that.

https://chat.zulip.org/#narrow/stream/6-frontend/topic/remove.20is_mirror_dummy

evykassirer · 2024-06-04T00:15:39Z

web/src/recent_view_ui.ts

+            // See `echo.build_display_recipient` for more on this.
+            return {
+                // TODO(evy): What to do here for typing purposes? Or can we assume all users are defined here?
+                // I don't know much aobut zephyr and when it's relevant.


question here!

For recent view, I think it makes sense to treat them as normal users unless we have problems generating their avatar and display user profile popover. I don't have much context here either.

I'll let @timabbott comment before removing it then

evykassirer · 2024-06-04T00:16:11Z

web/src/recent_view_ui.ts

+}
+
+// This mirrors the structure of `echo.build_display_recipient`, using data
+// found in `ConversationData`.


It felt like it would be pretty hard to generalize echo.build_display_recipient to work for this case too, so I just built a variation on it for here.

evykassirer · 2024-06-10T00:23:15Z

@timabbott this is ready for your review again. I've responded to the open comments, some I wanted to check in with you about what to do or if what I did seems right. Thanks!

zulipbot added the size: XL label May 19, 2024