Page MenuHomePhabricator
Differential D12287

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Authored by atul on Mon, Jun 3, 12:40 PM.
Tags
None
Referenced Files
F2121428: D12287.id41223.diff
Wed, Jun 26, 4:06 PM
Unknown Object (File)
Tue, Jun 25, 12:06 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
Unknown Object (File)
Fri, Jun 21, 5:51 AM
View All 30 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
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Mon, Jun 3, 12:40 PM
atul updated this revision to Diff 40896.Mon, Jun 3, 12:57 PM

ViewersBlock -> ViewerBlock

Harbormaster completed remote builds in B29394: Diff 40895.Mon, Jun 3, 12:57 PM
atul published this revision for review.Mon, Jun 3, 1:02 PM
atul added a child revision: D12288: [lib] Consume `useThreadFrozenDueToViewerBlock` in `web/ChatInputBar`.Mon, Jun 3, 1:03 PM
Harbormaster completed remote builds in B29395: Diff 40896.Mon, Jun 3, 1:13 PM
atul added inline comments.Mon, Jun 3, 2:33 PM
lib/shared/thread-utils.js
943–945

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.Tue, Jun 4, 1:23 AM
tomek added inline comments.
lib/shared/thread-utils.js
943–959

Should we memoize some of these values?

944

Does useCommunityRootMembersToRole memoize something? Is it ok to pass a new array to it on every render?

951–957

How skipMemberAdminRoleCheck works? What is its relationship to memberHasAdminRole?

This revision is now accepted and ready to land.Tue, Jun 4, 1:23 AM
ashoat requested changes to this revision.Tue, Jun 4, 10:39 AM

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

lib/shared/thread-utils.js
944

From looking at the definition, I doubt that this is okay. This array almost certainly needs to be memoized

946

Why is the first ?. necessary here? From looking at the definition of useCommunityRootMembersToRole, it never seems to return null or undefined

951–957

I think the idea is that the code above is replacing the member admin role check

This revision now requires changes to proceed.Tue, Jun 4, 10:39 AM
ashoat added inline comments.Tue, Jun 4, 10:41 AM
native/chat/chat-input-bar.react.js
705–708

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.Tue, Jun 4, 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.Tue, Jun 11, 1:39 PM

Add memoization to useThreadFrozenDueToViewerBlock

atul added inline comments.Tue, Jun 11, 1:43 PM
native/chat/chat-input-bar.react.js
705–708

Removed communityThreadInfo and userInfos. threadInfo and viewerID still need to be passed in.

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

remove now extraneous communityThreadInfo and userInfos props in native/ChatInputBar

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

fix useCommunityRootMembersToRole logic

Harbormaster completed remote builds in B29611: Diff 41223.Tue, Jun 11, 1:56 PM
atul added inline comments.Tue, Jun 11, 1:58 PM
lib/shared/thread-utils.js
946

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.Tue, Jun 11, 2:01 PM
Harbormaster completed remote builds in B29613: Diff 41225.Tue, Jun 11, 2:13 PM
ashoat accepted this revision.Wed, Jun 12, 8:56 AM
ashoat added inline comments.
lib/shared/thread-utils.js
974–977 ↗(On Diff #41225)

This doesn't need to be memoized

979–985 ↗(On Diff #41225)

This doesn't need to be memoized

946

I don't think it ever returned null/undefined?

This revision is now accepted and ready to land.Wed, Jun 12, 8:56 AM
atul updated this revision to Diff 41350.Sun, Jun 16, 2:05 PM

get rid of unneeded memoization

Harbormaster completed remote builds in B29696: Diff 41350.Sun, Jun 16, 2:22 PM
atul added inline comments.Sun, Jun 16, 2:59 PM
lib/shared/thread-utils.js
946

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.Sun, Jun 16, 4:19 PM

land

atul edited the test plan for this revision. (Show Details)Sun, Jun 16, 4:21 PM
This revision was landed with ongoing or failed builds.Sun, Jun 16, 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.Sun, Jun 16, 4:36 PM

Revision Contents
Changeset List

PathSize
lib/
shared/
26 lines
native/
chat/
18 lines

Diff 40896

View Options

lib/shared/thread-utils.js

Loading...
View Options

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

Loading...