Page MenuHomePhabricator
Differential D12287

[lib] Introduce `useThreadFrozenDueToViewersBlock`
ClosedPublic
Actions

Authored by atul on Jun 3 2024, 12:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 16, 1:10 AM
Unknown Object (File)
Mon, Nov 11, 6:43 PM
Unknown Object (File)
Mon, Nov 11, 12:33 PM
Unknown Object (File)
Mon, Nov 11, 6:39 AM
Unknown Object (File)
Mon, Nov 11, 3:41 AM
Unknown Object (File)
Sun, Nov 10, 11:21 PM
Unknown Object (File)
Sun, Nov 10, 2:35 PM
Unknown Object (File)
Fri, Nov 8, 7:14 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
Branch
master
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

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

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.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

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.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

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

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

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 ↗(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.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

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/
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...