Page MenuHomePhabricator

[lib] Pull `useCommunityRootMembersToRole` out of `useThreadSearchIndex`
ClosedPublic

Authored by atul on May 21 2024, 7:22 AM.
Tags
None
Referenced Files
F3376962: D12145.diff
Wed, Nov 27, 3:05 AM
Unknown Object (File)
Sat, Nov 23, 12:49 PM
Unknown Object (File)
Sat, Nov 9, 5:12 AM
Unknown Object (File)
Fri, Nov 8, 5:52 AM
Unknown Object (File)
Tue, Nov 5, 6:58 AM
Unknown Object (File)
Tue, Nov 5, 6:55 AM
Unknown Object (File)
Tue, Nov 5, 4:29 AM
Unknown Object (File)
Oct 11 2024, 4:36 PM
Subscribers
None

Details

Summary

We want to use this logic in useThreadsWithPermission as well so it makes sense to pull it out as a hook instead of copy/pasting code. Especially since the code for creating nested dictionary isn't the easiest to read.

Test Plan

Will add log statements to useThreadSearchIndex to make sure communityRootMemebersToRole gets computed as before.

Will also set breakpoints to check structure of communityRootMembersToRole to make sure it's as expected.


BEFORE (checking constructed communityRootMembersToRole within useSearchIndex with breakpoint):

33f8e2.png (846×804 px, 144 KB)

AFTER (checking return value of useCommunityRootMembersToRole):

d0577f.png (604×726 px, 60 KB)

Note that I wasn't able to get breakpoints to hit in AFTER case for some reason, but I console.logged and made sure that the structure of object was the same.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.May 21 2024, 7:24 AM
atul added inline comments.
lib/shared/thread-utils.js
158–162 ↗(On Diff #40469)

Would appreciate advice on if this is the right way to type this.

Before creating this type, flow said the return value of useCommunityRootMembersToRole was

lodash.Dictionary<lodash.Dictionary<RoleInfo | null>>

Did RoleInfo | null since it seemed narrower than ?RoleInfo since undefined isn't possible? Not sure if that was the right call, so open to reverting.

tomek added inline comments.
lib/shared/thread-utils.js
158–162 ↗(On Diff #40469)

I don't think it matters too much, and we're using ?Type in most of the other places, so it's probably less confusing to keep this approach.

159–161 ↗(On Diff #40469)
This revision is now accepted and ready to land.May 22 2024, 6:27 AM

address feedback before updating test plan and before landing