[lib] Only consider children of GENESIS when proposing existing chats
ClosedPublic
Actions

Authored by ashoat on Sep 25 2023, 5:12 PM.

Details

Reviewers

atul
ginsu
tomek

Commits

rCOMMe91430a951e1: [lib] Only consider children of GENESIS when proposing existing chats

Summary

This diff addresses ENG-5016.

The chat composer on both native and web allows the viewer to select a set of users they want to chat with. As new users are added to the list, the chat composer attempts to find an existing chat that includes that exact set of users. Note that if a match is made, the viewer is unable to create a new chat; the only option is to use the existing chat.

The problem is that the current code is returning channels inside specific communities. We consider it okay to have two channels in a community with the same membership since they might have different topics. On top of that, matching a channel within a specific community is confusing behavior when the chat composer is not meant to be community-specific.

This diff addresses the issue by making sure we only propose children of GENESIS. More context in the commentary on Linear.

Test Plan

I patched this onto my keyserver in production and confirmed that I was no longer seeing the commbot chat appear when I wanted to chat with Atul and Varun

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Sep 25 2023, 5:12 PM

Harbormaster completed remote builds in B22838: Diff 31410.Sep 25 2023, 5:39 PM

ashoat requested review of this revision.Sep 25 2023, 5:39 PM

This actually breaks threadInfoFromSourceMessageIDSelector... I'll need to make the change up the stack

Allow SIDEBARs

pendingToRealizedThreadIDsSelector is used for three things:

Mapping pending threads on the client to the "realized" threads once they come in
Determining if a given message has a sidebar created from it
In the chat composer, determining if a chat exists with the given set of users

In all these cases, we only need to consider kinds of threads that can be pending. We have only two such kinds of threads: sidebars, and top-level channels under GENESIS (which is what the chat composer creates).

(Sidebars have a completely different pending thread ID format, so there is no risk of a collision.)

By filtering to only valid kinds of pending threads, we make sure that we don't accidentally match channels that aren't top-level under GENESIS for case 3. This resolves the original bug.

I considered two other solutions:

Updating getPendingThreadID would probably be the most "idiomatic" solution, but it's complicated because we want this format to stay "stable" since it's used in MessageStore.
- We could write a redux-persist migration that updates the format, but this seems risky
- Would be easier to just leave the format as-is for top-level channels of GENESIS, but introduce a different format for other channels. Then those channels would no longer "collide" with top-level channels of GENESIS
- Additional benefit is that we could easily introduce pending thread support for other communities in the future
- However, I looked into this and found it would be relatively complicated. We would need to update every callsite of getPendingThreadID to make sure it knows the parentThreadID, and there is some ID schema migration code that would need to be updated as well
Creating a fork of pendingToRealizedThreadIDsSelector for useExistingThreadInfoFinder.
- This seemed "safer", but after thinking about it I realized it's no better than this solution.
- The fork could have a simpler condition (eg. my original diff), but there's no benefit to such a simpler condition... we might as well have both use cases use the more "strict" condition.
- Furthermore, this fork doesn't put us in a better position for a potential launch of pending threads in other communities... we would still need to update getPendingThreadID to avoid collisions.