Page MenuHomePhabricator
Differential D12287

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Authored by atul on Mon, Jun 3, 12:40 PM.
Tags
None
Referenced Files
F2054377: D12287.diff
Thu, Jun 20, 4:44 AM
F2053958: D12287.id40896.diff
Thu, Jun 20, 4:02 AM
Unknown Object (File)
Tue, Jun 18, 7:03 PM
Unknown Object (File)
Mon, Jun 17, 2:25 PM
Unknown Object (File)
Sat, Jun 15, 6:30 PM
Unknown Object (File)
Sat, Jun 15, 6:11 PM
Unknown Object (File)
Sat, Jun 15, 3:22 PM
Unknown Object (File)
Sat, Jun 15, 1:21 PM
View All 22 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
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(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.Tue, Jun 4, 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.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 ↗(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.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 ↗(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.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 ↗(On Diff #40896)

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 ↗(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.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 ↗(On Diff #40896)

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 ↗(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.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/
44 lines
native/
chat/
23 lines

Diff 41352

View Options

lib/shared/thread-utils.js

Loading...
View Options

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

Loading...