Add recent_private_conversations key to /register.

[ishammahajan]

WIP PR. Just for review/general help.

[gnprice]

Thanks @ishammahajan ! This generally looks like the right structure.

A few comments:

• Let's first do a version that just solves PMs screen doesn't show many conversations #3133 , without yet showing a snippet of the latest message.
  • Once that's done: to add snippets, I think we can get the server to start sending those as part of the recent_private_conversations data. That'll save us from going and individually fetching a bunch of messages, which I'm guessing is what the new getMessage and the hypothetical fetchPrivateMessagesv2 is intended for.
• On the RecentPrivateConversations type, let's have a bit of jsdoc about what it means:
  • Link to the comment added in zulip/zulip@4c3c669b4 that describes the format.
  • Have a summary line (like the other examples there) that briefly describes it in a sentence.
  • Mention on user_ids the detail that it's of other users -- that's something that isn't totally consistent across the system.
• I think the next pieces are to add logic to the new reducer to keep it up to date as EVENT_NEW_MESSAGE and other relevant events come in (see the events handled in the messages and narrows reducers)... and then to use this new data to fix PMs screen doesn't show many conversations #3133!

[gnprice]

(Also note e137e29 -- I think you had a version based on before that and rebased past it, and you'll want to adjust the rebase a bit.)

[ishammahajan]

Thanks for the early review!

• getMessage and the hypothetical fetchPrivateMessagesv2 is intended for.

The getMessage was my attempt at fetching only one message from the /messages endpoint, not get the raw message, but I can see where the confusion arose from. Likewise, the fetchPrivateMessagesv2 was just added to test functionality alongside the existing behaviour to confirm correct behaviour.

• let's have a bit of jsdoc about what it means

I'll add it, thanks!

• I think the next pieces are to add logic to the new reducer to keep it up to date as EVENT_NEW_MESSAGE and other relevant events come in

Hmm. That sounds about right, but my consideration for this was that we don't really have any message data stored in the messages which we can refer to though the max_message_id while displaying data using a selector -- I think my next move would be to fetch all these messages max_message_id refers to, using a loop. What do you think?

[gnprice]

• getMessage and the hypothetical fetchPrivateMessagesv2 is intended for.

The getMessage was my attempt at fetching only one message from the /messages endpoint, not get the raw message, but I can see where the confusion arose from. Likewise, the fetchPrivateMessagesv2 was just added to test functionality alongside the existing behaviour to confirm correct behaviour.

Ah OK, if it's just for your testing in development then never mind.

What I mean is just that if you were thinking these would be part of the PR to merge, I think we can actually do without them. (We don't need that data for solving #3133 , and for showing snippets we'll get it another way that's more efficient.)

Hmm. That sounds about right, but my consideration for this was that we don't really have any message data stored in the messages which we can refer to though the max_message_id while displaying data using a selector -- I think my next move would be to fetch all these messages max_message_id refers to, using a loop. What do you think?

Right, so: for #3133 we don't actually need any message data! So it's fine that the message IDs in recent_private_conversations may refer to messages we don't have the message data for. The IDs are needed just so we can sort the conversations by recency, and then the only other data we need on a given conversation is the user IDs. (And the user data, but that we maintain the complete set of.)

[ishammahajan]

(Also note e137e29 -- I think you had a version based on before that and rebased past it, and you'll want to adjust the rebase a bit.)

Ah, I thought there was something wrong about that! I was confused when the linter gave errors for the exact same lines. Turns out there was an 's' missing for each. 😄

[gnprice]

• I think the next pieces are to add logic to the new reducer to keep it up to date as EVENT_NEW_MESSAGE and other relevant events come in (see the events handled in the messages and narrows reducers)... and then to use this new data to fix PMs screen doesn't show many conversations #3133!

(I used terms a bit loosely here, so to clarify just in case because you haven't already spent a lot of time with this part of the system: EVENT_NEW_MESSAGE is a type value we use on certain Redux actions we create within the app. So anything that involves EVENT_NEW_MESSAGE is about a Redux action, not directly about an event. But, like all our action types with names EVENT_*, the actions of this type are created one-to-one to correspond to events we get from the Zulip server. In this case, consulting eventToAction.js, you'll see that an EVENT_NEW_MESSAGE action corresponds to a message event. Because one of these actions corresponds one-to-one to an event, we'll sometimes conflate the two casually, like I did above.)

[ishammahajan]

Right, so: for #3133 we don't actually need any message data! So it's fine that the message IDs in recent_private_conversations may refer to messages we don't have the message data for. The IDs are needed just so we can sort the conversations by recency, and then the only other data we need on a given conversation is the user IDs. (And the user data, but that we maintain the complete set of.)

From that issue:

The PMs screen shows the PM conversations we've been in recently, but "recently" may be as few as the last 100 PM messages. For a user that heavily uses PMs, this can leave out conversations even from a day or two ago.

It makes sense now! Would you confirm whether the code I have written in messageSelectors.js is correct? I'll get to adding the extra logic.

EDIT: Now that I think of it, if the user has had conversations in the WebApp but not on mobile, and they want to have a look at the conversation from the last 101th message, they wouldn't be able to. We should add the relevant code to fetch the messages as well, I think (though perhaps in a later commit).

[gnprice]

I'll get to adding the extra logic.

Sounds good!

Would you confirm whether the code I have written in messageSelectors.js is correct?

Well, so the first question might be: what is that function getPrivateMessages supposed to mean?

We don't seem to have any jsdoc on it, so this is a bit implicit. But you can search for where it's used:

$ rg -C4 getPrivateMessages
...
10-export const getRecentConversations: Selector<PmConversationData[]> = createSelector(
11-  getOwnEmail,
12:  getPrivateMessages,
...
159-export const getUnreadPrivateMessagesCount: Selector<number> = createSelector(
160:  getPrivateMessages,
161-  getReadFlags,
162-  (privateMessages, readFlags) => countUnread(privateMessages.map(msg => msg.id), readFlags),

It's used in two places. The first of those is probably used only for the PM conversations screen (I haven't looked to confirm that), and I'm guessing is the path you meant to affect.

The second one, though, seems to have a different job: it's counting how many of those known PMs are unread.

So for that job, it really wants a full list of private messages, or their IDs anyway -- not just the latest one message per conversation.

Which does pretty well fit the name getPrivateMessages.

So let's leave getPrivateMessages as it is, doing basically what the name says it does. I think you'll want to make some changes to getRecentConversations, though, to use the new data.

[gnprice]

One other thing I notice, looking a bit more at the existing code:

We do use in a few places an unread count for each conversation. We're computing that from looking back through the actual messages we have.

For now, for fixing #3133 , I think we can leave that aspect of the logic as it is: the unread count for a conversation will just count unreads in the messages we happen to have (often, just the latest 100 PMs overall).

As a further improvement, we might see about getting unread counts added to the recent_private_conversations data sent by the server. Then we can use those instead. But we're already today showing unread counts based only on those most recent messages; so it's OK to leave that as is while we start at least showing the older conversations at all, fixing #3133.

[gnprice]

Oh btw, as you're writing that reducer logic -- here's a remark I made over on #3133 that may be helpful:

After fetching, clients will need to correctly maintain these data structures as new messages are sent or deleted (Though for deleted, probably you can just not worry about it too much? Unclear).

Hmm -- yeah, in fact we can't in general maintain them exactly on deletion. If the message that was latest is deleted, we don't know what the next-latest would be.

I think it's OK for us to just say that the semantics of our data structure are slightly relaxed: max_message_id is actually an upper bound on the message IDs, one that's "close enough" that it's suitable for use in sorting the conversations by recency.

Basically there's a case where you may get stuck trying to see how to write the reducer logic... and I think the right answer there is to punt on it and just let the data be slightly wrong in a certain way.

[gnprice]

EDIT: Now that I think of it, if the user has had conversations in the WebApp but not on mobile, and they want to have a look at the conversation from the last 101th message, they wouldn't be able to. We should add the relevant code to fetch the messages as well, I think (though perhaps in a later commit).

Right, so if the user goes and actually navigates to the conversation, we'll need to go fetch the messages for it.

This is something we already do, though, and the existing mechanism should handle it just fine. If for example you go to the stream list, you can navigate to a stream where potentially the last message was a very long time ago, so we don't have any of those messages at all -- and we go and fetch them. Even in the PM conversations screen today, we might have only a handful of messages from any given conversation we show, so if the user navigates to it we promptly go fetch more.

See fetchMessagesInNarrow (and its freshly-added jsdoc) for one key layer of where that happens. See also its callers -- the most important is doNarrow. We invoke doNarrow from all the different places where the user chooses to navigate to some narrow; and it's the only place where we call navigateToChat, which does the actual navigation.

[gnprice]

I just went and wrote some more jsdoc too, expanding on the subject of that last comment. Take a look at 089adeb and 2f44825.

[ishammahajan]

Which does pretty well fit the name getPrivateMessages.

So let's leave getPrivateMessages as it is, doing basically what the name says it does. I think you'll want to make some changes to getRecentConversations, though, to use the new data.

That makes sense, I think. Thanks!

As a further improvement, we might see about getting unread counts added to the recent_private_conversations data sent by the server. Then we can use those instead.

Hmm. That's a good idea. Do we get something similar to show the unread counts in the streams?

Basically there's a case where you may get stuck trying to see how to write the reducer logic... and I think the right answer there is to punt on it and just let the data be slightly wrong in a certain way.

Yeah, I did 'punt' on it since it's difficult to get the last message of a conversation (specially if the messages of that conversation have not been retrieved.

Actually, the act of a user deleting a message should make it a more recently active conversation than others with no activity (similar for EVENT_ADD_REACTION and others). So I wonder if we should artificially increase max_message_id to reflect the recency, since that number has only one use -- to sort the conversations by recency.

Right, so if the user goes and actually navigates to the conversation, we'll need to go fetch the messages for it.

Understood, thanks! 🙂

I just went and wrote some more jsdoc too, expanding on the subject of that last comment. Take a look at 089adeb and 2f44825.

Thanks for that, helped a lot!

[timabbott]

Hmm. That's a good idea. Do we get something similar to show the unread counts in the streams?

So, we actually already have the unread_msgs data structure that has data on unread messages everywhere; so if we want to display unread messages data in this UI widget, we can just splice that together client-side (I don't think it makes sense to have the server send two copies of the unread PMs data).

[ishammahajan]

So, we actually already have the unread_msgs data structure that has data on unread messages everywhere;

the unread count for a conversation will just count unreads in the messages we happen to have (often, just the latest 100 PMs overall).

Hmm. Now that I check up on it, we do already use this in the app. Not sure what @gnprice was referring to there.

[gnprice]

Hmm. Now that I check up on it, we do already use this in the app. Not sure what @gnprice was referring to there.

I was looking, I think, at the getUnreadPrivateMessagesCount function which I quoted from above. As I described when quoting from it, it does in fact go and count through the messages we have.

But: searching in turn for that function, it looks like we don't actually ever invoke it! And we haven't since bd88566, nearly two years ago.

I'll go cut it out so it doesn't cause any further confusion.

[gnprice]

Thanks @ishammahajan for this update!

Reading the code:

• In this new data structure in the state, let's have an invariant that the list of conversations is sorted (by reverse max_message_id, i.e. the order we want to use them in.) The initial value comes to us that way, and that simplifies using it.

  So let's document that in jsdoc on the RecentPrivateConversationsState type, and then adjust the reducer logic to maintain that.

• In a couple of places there are conditions like if (action.ownEmail && and .filter(conversation => conversation !== undefined) that look defensive against data that should be disallowed by the types. Are those situations actually possible? If so, we should think through what they actually mean and how we'd want to handle them. If not, let's leave out the extra code.

• Does this bit actually work? I think the === will never be true:

  const conversationIndex = state.findIndex(
    conversation => conversation.user_ids === newConversation.user_ids,
  );

• Speaking of "does this actually work": please write unit tests. 😁 The reducer is an especially good target for unit tests: a well-specified, self-contained interface, and the logic inside can be tricky.

  Take a look at src/users/__tests__/userSelectors-test.js and src/webview/html/__tests__/messageHeaderAsHtml-test.js for examples of unit tests in a recommended style. A lot of our existing unit tests are in a style that is both more work to write, and gives lower-quality results.

  In particular, let's apply @flow strict-local to new tests, just like we do for all our non-test code. The exampleData library used in those two examples will help you do that.

• Speaking of existing unit tests in a style that is a lot of work and also not as helpful as it should be: sorry the pmConversationsSelectors tests were such a mess to update. Looking at the diff there, though, I notice two changes that look like real changes in the interface and potentially concerning.

  First: a bunch of timestamp: 0, which looks like dummy garbage data. If we don't need those, can we just remove them entirely? How about even making that a preparatory commit, before the main change?

  If there's a reason that wouldn't work, then that would suggest something more complicated is going on that'd be good to explain more.

• Second: the change in the convention of whether the current user's ID is in the list. The commit message says we "don't need" that to be there... but why is that, and in particular why would whether we need it change because we changed the source of the data?

  Either convention is fundamentally OK -- but which convention is used is part of the interface between different parts of the system. If this function switches from one convention to the other in what it returns, that's a change to its interface, and callers may break because they were assuming the previous convention.

  One fine solution would be to adjust this change so it doesn't change the results. Another would be to have a separate commit that changes from one convention to the other, adjusting callers as needed... and again adjust this change so it doesn't change the results. (Either before or after the commit that does change the results.)

• I think this sequence of filter, map, map would be a lot clearer as a single loop:

+    const recipients = recentPrivateConversations
+      .filter(conversation => conversation !== undefined)
+      .map(
+        conversation =>
+          conversation.user_ids.length !== 0
+            ? conversation
...

• Is this filter really producing appropriate behavior, in the cases where it does anything? I'm pretty sure that if it does anything the result will always be quite buggy:

+          conversation.user_ids
+            .map(id => ({ email: emails[id] }))
+            .filter(recipient => recipient.email !== undefined),

What would be appropriate behavior in that case?

(I think correctly implementing appropriate behavior in that case is one of the things that becomes easier if the enclosing filter, map, map sequence is turned into simply a loop -- in addition to just understanding the code as it is.)

• What's this bit of string-processing mean?

+        || unreadHuddles[currentUser.user_id.toString().concat(',', recipient.ids)],

In particular, does that put the user IDs in the right order? What order does unreadHuddles have them in? This is another thing where many conventions are perfectly reasonable, but "which one?" is part of the interface, and different pieces of code need to agree on it.

If logic like this is indeed needed, this line is too crowded a place for it -- this logic is too gnarly, and too interface-sensitive, to happen anonymously inside a property lookup like this. We'd want to make it its own small function with a name, and a line or two of jsdoc saying what it means.

• Commit style: let's put the EVENT_NEW_MESSAGE handler before the change to the selector. Before that handler, the data in the new state subtree is correct only just after the /register call, and starts becoming wrong as new messages come in; so it wouldn't be good to switch to using it.

[ishammahajan]

Thanks for the review, @gnprice!

• In this new data structure in the state, let's have an invariant that the list of conversations is sorted (by reverse max_message_id, i.e. the order we want to use them in.) The initial value comes to us that way, and that simplifies using it.

That makes total sense, I don't know why I didn't think of it that way. Changed the commit to reflect that. :)

• In a couple of places there are conditions like if (action.ownEmail && and .filter(conversation => conversation !== undefined) that look defensive against data that should be disallowed by the types. Are those situations actually possible?

I believe the first (action.ownEmail) was a goof by me, and the second was a piece which I did not remove after iterating through the reducer a bit. Sorry about that.

(next responses will be a bit out of order because of the nature of the comments.)

• First: a bunch of timestamp: 0, which looks like dummy garbage data.

I had added that as a placeholder for the data in PmConversationData since its type consisted of the timestamp as well. The reason was that I did not want to disturb the functioning of the other callers of the functions which used the same type -- which in hindsight was also foolish since setting timestamp to 0 makes timestamp attribute as useless as just making it optional in the type.

Anyway, now that I went and checked the usage of timestamp, it isn't actually used anywhere -- PmConversationDatas usage lies in UnreadCard and PmConversationList, both of which don't use the attribute timestamp anywhere, as far as I can understand from the code. So perhaps it will be best to remove them from the type?

...and yes! Removing the attribute from the type only results in a flow error in pmConversationsSelectors where I was using it to set timestamp: 0. After removing that, flow passed successfully.

[ishammahajan]

• the change in the convention of whether the current user's ID is in the list. The commit message says we "don't need" that to be there... but why is that, and in particular why would whether we need it change because we changed the source of the data?

I tried to keep the tests faithful to how we would get data from the server. The server would send it with no user ID of the current user. The selector though, converts an empty field into one that contains the current user's ID, while leaving the fields with multiple user ids untouched -- this was the pattern in the previous reducer as well (normalizeRecipientsSansMe demonstrated) to prevent the usage of the current user's name in the displayed list.

• Is this filter really producing appropriate behaviour, in the cases where it does anything? I'm pretty sure that if it does anything the result will always be quite buggy:

+          conversation.user_ids
+            .map(id => ({ email: emails[id] }))
+            .filter(recipient => recipient.email !== undefined),

The reason I put that is because for some reason, the server was giving me a conversation with a user with user id 3 (I don't remember the exact number correctly), but such a user does not exist in the users tree of our redux state. I initially thought that was Welcome Bot but that isn't the case since my conversation with it does get displayed even after adding this line. Perhaps a deleted user?

... this is so strange! Now that I try out the code without the line, for getting what the error exactly was, since I'm very positive that such an error existed, the error disappeared! I think this should be investigated more, but I don't know how I would do such a thing.

[gnprice]

(I've just rebased this branch onto master, and also fixed a couple of tests at commit 3/4.)

[gnprice]

OK, and pushed a version with a number of revisions:

• Addressed all but one of my comments above.
• Made a few small other changes in the first commit, mainly keeping things alphabetized where we have them that way.
• Made some bigger changes in the commit "pms selectors: Change selectors for getRecentConversations.", mostly as added commits (wouldn't want to merge it that way, but may keep things clearer for now.)

The remaining major thing I think it's essential to do before merging this is:
I'm not certain this correctly handles the various forms in which we identify a PM conversation. (Include self, or not; user IDs, emails; arrays, comma-joined strings.) This is an unfortunately unnecessarily-complex area of the Zulip API.

In particular there's my one remaining comment from above:

+    user_ids: action.message.display_recipient.map(recipient => recipient.id),

• Hmm, is this right? In particular, doesn't display_recipient contain the self user's ID?

The newly rewritten getRecentConversations selector has some logic to deal with this patchwork too, and I'm not yet confident that that's all correct.

[gnprice]

Just pushed 35be10b, which confirms that display_recipient always contains the self user's ID.

Also confirmed that, as jsdoc in this branch mentions, the user-ID lists in recent_private_conversations never contain the self user's ID. (And that they're always sorted:

    # Sort to prevent test flakes and client bugs.
    for rec in recipient_map.values():
        rec['user_ids'].sort()

though that isn't a documented guarantee.)

Then the third bit of API surface area involved here is the unreads data structure, which getRecentConversations queries in order to add the information of how many in each conversation are unread. Still to do is to pin that down, as (another) part of #4035.

[rk-for-zulip]

Picking this up, as #3133 is on my plate now:

Also confirmed that, as jsdoc in this branch mentions, the user-ID lists in recent_private_conversations [are] always sorted:

    # Sort to prevent test flakes and client bugs.
    for rec in recipient_map.values():
        rec['user_ids'].sort()

Alas, this is not quite so. The above code was only added in 2.2-dev-53-g405a529340 (zulip/zulip@405a529340), which was backported to the 2.1 branch as 2.1.1-50-gd452ad31e0 (zulip/zulip@d452ad31e0).

Zulip server versions 2.1.0 and 2.1.1 will therefore not sort user IDs.

[chrisbobbe]

I've just made a rebased version of this PR, but I haven't pushed it to this PR branch in case it might accidentally remove something important to the direction of the PR. You can see it at my recentprivateconvo-rebased branch.

[chrisbobbe]

Hmm, have we thought through storing -Infinity in the Redux state; in particular, what happens when it's saved and then loaded from storage? It looks like it doesn't round-trip through JSON:

JSON.parse(JSON.stringify({ a: -Infinity }))
// {a: null}

[chrisbobbe]

I've just made a rebased version of this PR, but I haven't pushed it to this PR branch in case it might accidentally remove something important to the direction of the PR. You can see it at my recentprivateconvo-rebased branch.

Ah, and I've just come across #4104, which I'd forgotten about—it's a more recent PR that may indeed replace this one, for fixing #3133, so it may not have been super helpful for me to rebase this one (which I did in my recentprivateconvo-rebased branch, as mentioned). Oops.

[gnprice]

I've just sent #4392 as a new approach to this issue.