msglist: Offer bulk-mark-as-read button for PM conversations

[chrisbobbe]

Fixes: #5156

[chrisbobbe]

Also posted a thing I noticed while working on this.

[gnprice]

Thanks @chrisbobbe! Comments below.

[gnprice]

Yikes, this is kind of messy, isn't it. Particularly that gotcha commented on in the first commit.

I'd like to have this kind of code, dealing with the messy structure inside a data model, be located inside that model's code. So e.g. a getUnreadIdsForPmNarrow, located among the selectors at the top of unreadModel.js.

There's an existing spot where we do something similar, in getUnreadCountForNarrow in unreadSelectors.js:

      pm: ids => {
        if (ids.length > 1) {
          const unreadsKey = pmUnreadsKeyFromPmKeyIds(ids, ownUserId);
          const unreadItem = unread.huddles.find(x => x.user_ids_string === unreadsKey);
          return unreadItem?.unread_message_ids.length ?? 0;
        } else {
          const senderId = ids[0];
          const unreadItem = unread.pms.find(x => x.sender_id === senderId);
          return unreadItem?.unread_message_ids.length ?? 0;
        }
      },

So a good structure would be to factor that logic out of there, and then that case of getUnreadCountForNarrow just looks like ids => getUnreadIdsForPmNarrow(ids).length.

Note that that also has a variation of how to deal with the string/number thing, that's perhaps a bit cleaner.

[chrisbobbe]

Note that that also has a variation of how to deal with the string/number thing, that's perhaps a bit cleaner.

Yeah, maybe. It's not a biggie, but that one doesn't use pmUnreadsKeyFromPmKeyIds at all in the 1:1 pm case, even though it's written to be able to handle it.

[gnprice]

Should probably skip this if messageIds is empty.

[gnprice]

No need for that, I think; the job of that queue is when you're scrolling and have lots of frequent little batches to mark as read.

[chrisbobbe]

Thanks! Revision pushed.

[gnprice]

Thanks for the revision! Looks good. One nit:

-  invariant(isPmNarrow(narrow));

This gets added with the surrounding code, then removed in the subsequent commit. Did you intend to take it out?

(I think it's fine to either have it or not have it; the next line will throw an error if it's not a PM narrow anyway.)

[chrisbobbe]

Thanks for the revision! Looks good. One nit:

-  invariant(isPmNarrow(narrow));

This gets added with the surrounding code, then removed in the subsequent commit. Did you intend to take it out?

(I think it's fine to either have it or not have it; the next line will throw an error if it's not a PM narrow anyway.)

Eep, thanks for catching this! Yeah, I think I was doing a complicated reordering of commits, and this fell through. Agreed that we don't need the invariant here.

I think it's fine to either have it or not have it […]

Small correction: at the commit that includes that invariant line, the function (getUnreadIdsForPmNarrow) would actually error unconditionally. I've sent zertosh/invariant#48 for that. 😝 I'm not sure invariant is actively maintained; if you'd like, I'll make a little libdef for it with that fix.

[chrisbobbe]

Merged, with the invariant line taken out.

[gnprice]

the function (getUnreadIdsForPmNarrow) would actually error unconditionally. I've sent zertosh/invariant#48 for that.

Ha, I see.

I'm not sure invariant is actively maintained; if you'd like, I'll make a little libdef for it with that fix.

Could try that. Though I'm not sure a libdef (or that .js.flow file) will actually have any effect… because Flow actually special-cases calls to invariant, by name:
https://github.com/facebook/flow/blob/d247c5eef542a1745d831edeab4e11e305effaf4/src/typing/statement.ml#L221-L223

So this is likely a bug in that logic in Flow… that or a divergence between the invariant NPM package and the Facebook-internal invariant. The way to find out which probably runs through filing an issue in Flow.

Related Flow issue, with background on why it's special: https://github.com/facebook/flow/issues/ 112

From back-links in that thread, a possible form of a solution if the NPM package and what's inside Facebook have diverged (other than the solution of making the package align with the Facebook version):
facebook/flow@3e35bd4

I haven't really been bothered by the fact that invariant is special-cased, because the name is specific enough that it doesn't feel likely to be accidentally used for something unrelated. But if the special-cased behavior is wrong, it'd certainly be good to fix it.