narrow: Add `getNarrowsForMessage`. #4317

chrisbobbe · 2020-11-25T22:28:23Z

narrow: Add getNarrowsForMessage.

We've never had a function for getting all the narrows that a
message appears in. Such a function will be important
algorithmically for avoiding a performance regression with the
EVENT_NEW_MESSAGE action if/when we start storing state.narrows as
an Immutable.Map; see discussion of this strategy at
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1068912.

Might as well put it in narrow.js and make it reusable, too.

gnprice

Thanks @chrisbobbe ! Comments below. As you'll see, some of these refer to #4329, which I hadn't written yet when you originally wrote this PR 🙂

gnprice · 2020-12-08T02:58:36Z

src/utils/narrow.js

+    if (message.display_recipient.length > 1) {
+      return groupNarrow(
+        message.display_recipient.filter(x => x.email !== ownEmail).map(x => x.email),
+      );
+    } else if (message.display_recipient.length === 1) {
+      return privateNarrow(message.display_recipient[0].email);


Passing a single email to `groupNarrow` happens to give the same output as you get when passing that email to `privateNarrow`. But that's not really a reason to call `groupNarrow` when we clearly mean to make a private narrow (i.e., a conversation between the self and one user).

There's definitely an awkward mismatch here. The way I'd actually like to fix this is in the opposite direction, though: the model in the Zulip API is generally that there's no conceptual difference between a group PM and a 1:1 PM, and I'd like to make more of our code handle them that way.

Or put another way: the thing that's wrong here with groupNarrow and privateNarrow isn't that they accidentally/confusingly happen to give the same output, but rather that they have different names in the first place. (Or possibly there's a role for two of them, but where one is explicitly just a convenience helper for calling the other.)

I have a change that unifies the two of them in one of my draft branches for refactoring Narrow, but it's atop a bunch of other bigger changes. I noticed this same confusion in this bit of code the other day when preparing #4329, and I think I've seen it somewhere else recently too. So it might be past time for me to try making just that unification on its own -- it should simplify any further Narrow-refactoring anyway.

Pending that, in places where we have a call to groupNarrow that's relying on the fact that giving it a singleton array is the same as calling privateNarrow, let's just let that be.

I have a change that unifies the two of them in one of my draft branches for refactoring Narrow, but it's atop a bunch of other bigger changes. I noticed this same confusion in this bit of code the other day when preparing #4329, and I think I've seen it somewhere else recently too. So it might be past time for me to try making just that unification on its own -- it should simplify any further Narrow-refactoring anyway.

OK, and sent this as #4330!

Turned out the privateNarrow / groupNarrow unification was actually at the very start of that branch. 🙂 But I went and included some other pieces as well.

Great! I just reviewed and merged #4330; it LGTM.

gnprice · 2020-12-08T03:01:50Z

src/utils/narrow.js

+ * topic. (FIXME: What does that mean; don't all messages have
+ * topics?)
+ */
+export const getNarrowForReply = (message: Message | Outbox, ownEmail: string) => {


Great -- this is a much better name 🙂

gnprice · 2020-12-08T03:07:48Z

src/utils/narrow.js

+  const sendableNarrows = getNarrowsForMessage(message, ownEmail, fakeFlags).filter(
+    canSendToNarrow,
+  );

-  return topicNarrow(message.display_recipient, message.subject);
+  const [sendableStreamNarrow, sendableTopicNarrow] = [
+    sendableNarrows.find(n => isStreamNarrow(n)),
+    sendableNarrows.find(n => isTopicNarrow(n)),
+  ];
+  if (sendableStreamNarrow && sendableTopicNarrow) {
+    return sendableTopicNarrow;
+  } else if (sendableNarrows[0]) {
+    return sendableNarrows[0];


This feels pretty fiddly to me. I think a cleaner way to avoid the duplication here is to factor just the duplicated code out: effectively this version is calling getNarrowsForMessage but then undoing some of what that function did, and we can get a cleaner result by instead having both functions call a common simpler helper that just does the part that's duplicated.

Following #4239, pmKeyRecipientsFromMessage is that helper and that's what getNarrowFromMessage now uses.

Following #4239, pmKeyRecipientsFromMessage is that helper and that's what getNarrowFromMessage now uses.

(#4329, I think. 🙂)

I think a cleaner way to avoid the duplication here is to factor just the duplicated code out: effectively this version is calling getNarrowsForMessage but then undoing some of what that function did, and we can get a cleaner result by instead having both functions call a common simpler helper that just does the part that's duplicated.

Ooh, nice! Thanks.

src/utils/narrow.js

gnprice · 2020-12-08T03:14:18Z

src/utils/narrow.js

+  if (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) {
+    result.push(MENTIONED_NARROW);
+  }
+
+  if (flags.includes('starred')) {
+    result.push(STARRED_NARROW);
+  }
+


Let's add a comment mentioning search narrows, saying they're excluded.

In fact, that should probably be in the jsdoc -- just like isMessageInNarrow.

chrisbobbe · 2020-12-08T19:41:09Z

Thanks for the review! 🙂 I've just pushed a new revision.

A message can easily exist in multiple narrows (e.g., a stream narrow, a topic narrow, and the starred narrow), so `getNarrowFromMessage` was a misleading name. Looking at its implementation and its one call site, this function does some important work to answer the reasonable question, "Which narrow should my reply to this message go in?". So, name it that way.

There's no reason we would sometimes want a stream narrow here. This is just how it was when this function was first written (as `getNarrowFromMessage`), for narrowing to a message's conversation, in 34ab1f9 (zulip#333) in 2017. This case also should be impossible anyway, because message topics should always be nonempty -- we convert empty to "(no topic)". Co-authored-by: Chris Bobbe <cbobbe@zulip.com>

Added in jest-community/jest-extended#122.

We've never had a function for getting all the narrows that a message appears in. Such a function will be important algorithmically for avoiding a performance regression with the EVENT_NEW_MESSAGE action if/when we start storing `state.narrows` as an `Immutable.Map`; see discussion of this strategy at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1068912 Might as well put it in `narrow.js` and make it reusable, too.

… converse. Whenever we add a new type of narrow, this function will probably need to gain a bit of logic to include that type of narrow where appropriate. There's no obvious way to get the type-checker to remind us to do that, because here we're conditionally building up a Narrow value rather than breaking one down. But the closely-related function isMessageInNarrow does have an actual Narrow object to break down, so it enjoys the benefit of Flow confirming it's covered all the cases. Add a comment there pointing here, in hopes of riding on that. Also add a comment about SEARCH_NARROW so that this function's implementation explicitly mentions every type of narrow; and add cross-references to the jsdoc.

gnprice · 2020-12-09T21:45:14Z

Thanks! Merged, with small tweaks.

chrisbobbe · 2020-12-09T21:49:18Z

Thanks! I saw the tweaks; thanks for making them. 👍

chrisbobbe requested a review from gnprice November 25, 2020 22:28

chrisbobbe mentioned this pull request Dec 5, 2020

Introduce invariant; use it to handle display_recipient more cleanly. #4329

Merged

chrisbobbe force-pushed the pr-get-narrows-for-message branch from 58f6fd8 to 8f49d2e Compare December 5, 2020 20:30

gnprice reviewed Dec 8, 2020

View reviewed changes

gnprice mentioned this pull request Dec 8, 2020

narrow: Treat group PMs and 1:1 PMs more symmetrically. #4330

Merged

chrisbobbe force-pushed the pr-get-narrows-for-message branch from 8f49d2e to 947b71a Compare December 8, 2020 19:40

chrisbobbe and others added 5 commits December 9, 2020 13:42

jest libdef: Add .toIncludeSameMembers.

e1c6c7b

Added in jest-community/jest-extended#122.

gnprice force-pushed the pr-get-narrows-for-message branch from 947b71a to c2eafc1 Compare December 9, 2020 21:44

gnprice merged commit c2eafc1 into zulip:master Dec 9, 2020

gnprice mentioned this pull request Dec 10, 2020

narrow/recipient: Fix more bugs, and prep for bigger refactors #4332

Merged

chrisbobbe deleted the pr-get-narrows-for-message branch November 5, 2021 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

narrow: Add `getNarrowsForMessage`. #4317

narrow: Add `getNarrowsForMessage`. #4317

chrisbobbe commented Nov 25, 2020

gnprice left a comment

gnprice Dec 8, 2020

gnprice Dec 8, 2020

chrisbobbe Dec 8, 2020

gnprice Dec 8, 2020

gnprice Dec 8, 2020

chrisbobbe Dec 8, 2020

chrisbobbe Dec 8, 2020

gnprice Dec 8, 2020

chrisbobbe commented Dec 8, 2020

gnprice commented Dec 9, 2020

chrisbobbe commented Dec 9, 2020

narrow: Add getNarrowsForMessage. #4317

narrow: Add getNarrowsForMessage. #4317

Conversation

chrisbobbe commented Nov 25, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Dec 8, 2020

gnprice commented Dec 9, 2020

chrisbobbe commented Dec 9, 2020

narrow: Add `getNarrowsForMessage`. #4317

narrow: Add `getNarrowsForMessage`. #4317