Page MenuHomePhabricator
Differential D12287

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Authored by atul on Jun 3 2024, 12:40 PM.
Tags
None
Referenced Files
F3149463: D12287.id41224.diff
Mon, Nov 4, 2:08 PM
F3148179: D12287.id40895.diff
Mon, Nov 4, 9:05 AM
Unknown Object (File)
Mon, Nov 4, 4:19 AM
Unknown Object (File)
Mon, Nov 4, 3:33 AM
Unknown Object (File)
Sun, Nov 3, 10:38 PM
Unknown Object (File)
Sun, Nov 3, 5:40 PM
Unknown Object (File)
Mon, Oct 28, 11:38 AM
Unknown Object (File)
Tue, Oct 22, 9:58 PM
View All Files
Subscribers
None

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
atul updated this revision to Diff 40896.Jun 3 2024, 12:57 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
ashoat requested changes to this revision.Jun 4 2024, 10:39 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

atul planned changes to this revision.Jun 4 2024, 1:03 PM

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

atul updated this revision to Diff 41223.Jun 11 2024, 1:39 PM

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.

atul updated this revision to Diff 41224.Jun 11 2024, 1:44 PM

remove now extraneous communityThreadInfo and userInfos props in native/ChatInputBar

atul updated this revision to Diff 41225.Jun 11 2024, 1:56 PM

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
atul updated this revision to Diff 41350.Jun 16 2024, 2:05 PM

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.

atul updated this revision to Diff 41351.Jun 16 2024, 4:19 PM

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

Revision Contents
Changeset List

PathSize
lib/
shared/
51 lines
native/
chat/
23 lines

Diff 41225

View Options

lib/shared/thread-utils.js

Loading...
View Options

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

Loading...