Details

Reviewers

ashoat
ginsu
tomek
varun

Commits

rCOMMd6fa6b7fc944: [lib] Introduce `useThreadFrozenDueToViewersBlock`

Summary

We introduce a "hook version" of threadFrozenDueToViewersBlock which will allow us to set skipMemberAdminRoleCheck to true in the call to threadIsWithBlockedUserOnly in [web/native]/ChatInputBar and use the "updated" community role logic instead.

Depends on D12265

Test Plan

So far just flow and some log statements to ensure that value is as expected + same as before.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Jun 3 2024, 12:40 PM

ViewersBlock -> ViewerBlock

Harbormaster completed remote builds in B29394: Diff 40895.Jun 3 2024, 12:57 PM

atul published this revision for review.Jun 3 2024, 1:02 PM

atul added a child revision: D12288: [lib] Consume `useThreadFrozenDueToViewerBlock` in `web/ChatInputBar`.Jun 3 2024, 1:03 PM

Harbormaster completed remote builds in B29395: Diff 40896.Jun 3 2024, 1:13 PM

atul added inline comments.Jun 3 2024, 2:33 PM

lib/shared/thread-utils.js
943–945 ↗	(On Diff #40896)	NOTE TO SELF: Make sure all the `lodash` stuff in `useCommunityRootMembersToRole` doesn't break when empty array is passed into `useCommunityRootMembersToRole`.

tomek accepted this revision.Jun 4 2024, 1:23 AM

tomek added inline comments.

lib/shared/thread-utils.js
943–959 ↗	(On Diff #40896)	Should we memoize some of these values?
944 ↗	(On Diff #40896)	Does `useCommunityRootMembersToRole` memoize something? Is it ok to pass a new array to it on every render?
951–957 ↗	(On Diff #40896)	How `skipMemberAdminRoleCheck` works? What is its relationship to `memberHasAdminRole`?

This revision is now accepted and ready to land.Jun 4 2024, 1:23 AM

I'm concerned that the input to useCommunityRootMembersToRole is not being memoized

lib/shared/thread-utils.js
944 ↗	(On Diff #40896)	From looking at the definition, I doubt that this is okay. This array almost certainly needs to be memoized
946 ↗	(On Diff #40896)	Why is the first `?.` necessary here? From looking at the definition of `useCommunityRootMembersToRole`, it never seems to return `null` or `undefined`
951–957 ↗	(On Diff #40896)	I think the idea is that the code above is replacing the member admin role check

This revision now requires changes to proceed.Jun 4 2024, 10:39 AM

ashoat added inline comments.Jun 4 2024, 10:41 AM

native/chat/chat-input-bar.react.js
705–708 ↗	(On Diff #40896)	Can you reevaluate whether each of these props still need to be passed to the class component? It looks like at least `communityThreadInfo` can be removed, for instance

Local commit includes memoization/pruning of props. Will do a rebase and update this diff.

Add memoization to useThreadFrozenDueToViewerBlock

atul added inline comments.Jun 11 2024, 1:43 PM

native/chat/chat-input-bar.react.js
705–708 ↗	(On Diff #40896)	Removed `communityThreadInfo` and `userInfos`. `threadInfo` and `viewerID` still need to be passed in.

remove now extraneous communityThreadInfo and userInfos props in native/ChatInputBar

fix useCommunityRootMembersToRole logic

Harbormaster completed remote builds in B29611: Diff 41223.Jun 11 2024, 1:56 PM

atul added inline comments.Jun 11 2024, 1:58 PM

lib/shared/thread-utils.js
946 ↗	(On Diff #40896)	Fixed this by having `useCommunityRootMembersToRole` return empty object instead of `null`/`undefined` if there are no community roots.

Harbormaster completed remote builds in B29612: Diff 41224.Jun 11 2024, 2:01 PM

Harbormaster completed remote builds in B29613: Diff 41225.Jun 11 2024, 2:13 PM

ashoat accepted this revision.Jun 12 2024, 8:56 AM

ashoat added inline comments.

lib/shared/thread-utils.js
974–977		This doesn't need to be memoized
979–985		This doesn't need to be memoized
946 ↗	(On Diff #40896)	I don't think it ever returned `null`/`undefined`?

This revision is now accepted and ready to land.Jun 12 2024, 8:56 AM

get rid of unneeded memoization

Harbormaster completed remote builds in B29696: Diff 41350.Jun 16 2024, 2:22 PM

atul added inline comments.Jun 16 2024, 2:59 PM

lib/shared/thread-utils.js
946 ↗	(On Diff #40896)	Yeah it didn't, only the inner `communityThreadInfo?.id` needs to be accessed with `?.`. Left early return in `useCommunityRootMembersToRole` since it doesn't hurt, but can remove if that's preferred.

land

atul edited the test plan for this revision. (Show Details)Jun 16 2024, 4:21 PM

This revision was landed with ongoing or failed builds.Jun 16 2024, 4:30 PM

Closed by commit rCOMMd6fa6b7fc944: [lib] Introduce `useThreadFrozenDueToViewersBlock` (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMd6fa6b7fc944: [lib] Introduce `useThreadFrozenDueToViewersBlock`.

Harbormaster completed remote builds in B29697: Diff 41351.Jun 16 2024, 4:36 PM

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41225

lib/shared/thread-utils.js

native/chat/chat-input-bar.react.js

[lib] Introduce `useThreadFrozenDueToViewersBlock`ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41225

lib/shared/thread-utils.js

native/chat/chat-input-bar.react.js

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Revision Contents
Changeset List