[lib] Fix useThreadsWithPermission check
ClosedPublic
Actions

Authored by ashoat on Tue, Nov 5, 7:19 PM.

Details

Reviewers

tomek
angelika

Commits

rCOMMcaf1601b898e: [lib] Fix useThreadsWithPermission check

Summary

This check is currently equivalent to a threadTypeIsCommunityRoot check. I initially gleaned this by reading the code, but I verified it by comparing the two checks for each thread in my ThreadStore on production. The reason is that communityRootMembersToRole only contains entries for community roots, and all community roots have at least one admin.

This is probably the wrong check for us to making here. We should probably treat the channels inside the community root the same as we treat the community. Additionally, we should probably carve out an exception for channels inside GENESIS, which we generally treat the same as thick threads (for the purposes of determining whether to freeze the chat due to there being only one other member, and that member has blocked you).

Depends on D13874

Test Plan

I deployed a custom environment to comm.software where I could run React profiling on the production dataset as described here. I confirmed this change reduced rerender time from 3.0s to 0.5s

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Tue, Nov 5, 7:19 PM

Harbormaster completed remote builds in B32556: Diff 45633.Tue, Nov 5, 7:39 PM

ashoat requested review of this revision.Tue, Nov 5, 7:39 PM

tomek accepted this revision.Thu, Nov 7, 4:34 AM

tomek added inline comments.

lib/shared/thread-utils.js
179 ↗	(On Diff #45633)	Should we include the sidebars?
182 ↗	(On Diff #45633)	Are we sure we want to make this exception for all the GENESIS descendants, including e.g. the sidebars?
185 ↗	(On Diff #45633)	So we're saying that GENESIS itself isn't `InsideGenesis`, which doesn't sound right but is probably useful in this case.

This revision is now accepted and ready to land.Thu, Nov 7, 4:34 AM

ashoat added inline comments.Thu, Nov 7, 4:45 AM

lib/shared/thread-utils.js
179 ↗	(On Diff #45633)	Yes, I think so. What we're doing here is "freezing" a sidebar when the only other member is somebody who you have blocked or has blocked you. We could argue for something more complicated, where the sidebar is only frozen is the parent thread is. I could see an argument for that being a better product experience, but I'm not sure it's common enough that it matters. I'll create a follow-up task before landing.
182 ↗	(On Diff #45633)	Similar answer to above. It might be better to handle this differently for grandchildren of GENESIS (basically checking the immediate child of GENESIS), but it would be more complicated. We can track it in the follow-up task.
185 ↗	(On Diff #45633)	Yes, that's right. I'll add a code comment to clarify.