Page MenuHomePhabricator
Differential D12714

[native] update AutoJoinCommunityHandler to use useJoinCommunity hook
ClosedPublic
Actions

Authored by ginsu on Jul 10 2024, 12:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 9, 10:07 AM
Unknown Object (File)
Wed, Apr 9, 7:00 AM
Unknown Object (File)
Wed, Apr 9, 6:21 AM
Unknown Object (File)
Wed, Apr 9, 5:48 AM
Unknown Object (File)
Wed, Apr 9, 3:06 AM
Unknown Object (File)
Wed, Apr 9, 1:13 AM
Unknown Object (File)
Tue, Apr 8, 10:27 PM
Unknown Object (File)
Tue, Apr 8, 10:16 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
ashoat
inka
Commits
rCOMM41570ee3f402: [native] update AutoJoinCommunityHandler to use useJoinCommunity hook
Summary

This diff updates AutoJoinCommunityHandler to use the useJoinCommunity hook. Since the logic for useJoinCommunity can only handle one community at a time, we needed to create a secondary handler component (JoinHandler) which handles calling useJoinCommunity and then awaiting the promise that gets returned by the hook.

Depends on D12713

Test Plan

Confirmed that I was still able to auto join any community that was tagged to a farcaster channel that I am following on farcaster

Diff Detail

Repository
rCOMM Comm
Branch
eng-7462 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Jul 10 2024, 12:29 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 10 2024, 12:29 AM
Harbormaster completed remote builds in B30257: Diff 42197.Jul 10 2024, 12:46 AM
ginsu requested review of this revision.Jul 10 2024, 12:46 AM
ginsu updated this revision to Diff 42201.Jul 10 2024, 1:55 AM

update

ginsu edited the summary of this revision. (Show Details)Jul 10 2024, 2:10 AM
ginsu added reviewers: ashoat, inka.
ginsu added a parent revision: D12713: [lib] make useJoinCommunity compatible with the auto join community handler.
Harbormaster completed remote builds in B30260: Diff 42201.Jul 10 2024, 2:17 AM
ginsu updated this revision to Diff 42202.Jul 10 2024, 2:21 AM

update

ginsu added inline comments.Jul 10 2024, 2:25 AM
native/components/auto-join-community-handler.react.js
45–50 ↗(On Diff #42202)

This got moved to JoinHandler

52–72 ↗(On Diff #42202)

This is now handled in useJoinCommunity

92 ↗(On Diff #42202)

Flow was not happy when I tried to initially pass in an array of promises to Promise.all() where the promises could either return null or CommunityToAutoJoin

Screenshot 2024-07-10 at 5.04.37 AM.png (732×1 px, 152 KB)

Creating an obj of promises then using promiseAll seemed to do the trick

ginsu attached a referenced file: F2225800: Screenshot 2024-07-10 at 5.04.37 AM.png. (Show Details)Jul 10 2024, 2:26 AM
ginsu added a child revision: D12716: [lib/native] lift auto join community handler to lib.Jul 10 2024, 2:56 AM
Harbormaster completed remote builds in B30261: Diff 42202.Jul 10 2024, 3:05 AM
ashoat requested changes to this revision.Jul 10 2024, 10:56 AM

Your general approach with rendering a separate JoinHandler for each community makes sense to me. Helps avoid the complexity of having to convert useJoinCommunity to useJoinCommunities

Requesting changes mostly because I'm concerned that React will re-run this stuff and end up triggering a lot of wasted network activity

native/components/auto-join-community-handler.react.js
60 ↗(On Diff #42202)

Typo

63 ↗(On Diff #42202)

What do you think about making sure this effect is only run once per app startup?

I'm a bit concerned that it's going to run every time the keyserver connection status flips

156 ↗(On Diff #42202)

Using index here as the key is dangerous. You're relying on the array index inside communitiesToAutoJoin to not change, but it can totally change if eg. the user follows a new channel on Farcaster.

Instead, can we use the communityID as the key? That will make sure that if communitiesToAutoJoin changes, we don't change the communityID we're passing to any individual JoinHandler component.

188 ↗(On Diff #42202)

When does this get regenerated? I'm worried about a risk of joinCommunity being called multiple times for a single community. Note that this will continue to be a risk even if you make sure setCommunitiesToAutoJoin is only ever called once in AutoJoinCommunityHandler

200–202 ↗(On Diff #42202)

There is no reason for this indirection

This revision now requires changes to proceed.Jul 10 2024, 10:56 AM
ginsu updated this revision to Diff 42211.Jul 10 2024, 1:07 PM

address comments

ginsu planned changes to this revision.Jul 10 2024, 1:10 PM

Messed something up when stashing + rebasing

ginsu updated this revision to Diff 42212.Jul 10 2024, 1:23 PM

update

Harbormaster completed remote builds in B30270: Diff 42211.Jul 10 2024, 1:24 PM
Harbormaster completed remote builds in B30271: Diff 42212.Jul 10 2024, 1:40 PM
ginsu added inline comments.Jul 10 2024, 1:42 PM
native/components/auto-join-community-handler.react.js
188 ↗(On Diff #42202)

Logged out when we were regenerating joinCommunity and it definitely was getting regenerated many more times than needed.

I added the check to make sure the effect in AutoJoinCommunityHandler is only ever called once and additionally also called setCommunitiesToAutoJoin(null); if we try to run that effect again. With these two changes we significantly cut down the amount of times joinCommunity gets regenerated. Will keep investigating if there are better ways to do this, but wanted to check in with what I found

Before the changes when I try to auto join 3 different communities:

Screenshot 2024-07-10 at 4.36.18 PM.png (1×768 px, 378 KB)

After the changes when I tried to auto join 3 different communities:

Screenshot 2024-07-10 at 4.36.44 PM.png (360×666 px, 73 KB)

ginsu attached a referenced file: F2231502: Screenshot 2024-07-10 at 4.36.44 PM.png. (Show Details)Jul 10 2024, 1:42 PM
ginsu attached a referenced file: F2231501: Screenshot 2024-07-10 at 4.36.18 PM.png. (Show Details)
ashoat added a comment.Jul 10 2024, 1:57 PM

Hmm... what did you think of my suggestion earlier?

What do you think about making sure this effect is only run once per app startup?

native/components/auto-join-community-handler.react.js
68 ↗(On Diff #42212)

This feels like a weird place to reset this. It's not guaranteed to be run

ginsu added a comment.Jul 10 2024, 2:43 PM

Hmm... what did you think of my suggestion earlier?

What do you think about making sure this effect is only run once per app startup?

I tried this too using prevCanQueryRef and just setting it to false after we ran the effect once on app startup (also tried to removing all the deps from the effect too as a sanity check) but was still seeing a lot of calls to useJoinCommunity with this suggestion.

Another idea I had is what if we created a second effect that watches threadInfos and setCommunitiesToAutoJoin to be null from there. This effect will get triggered once we have auto joined the communities since the value of threadInfos will change with all the new communities that were auto joined

Will continue to think of more elegant solutions

ashoat added a comment.Jul 10 2024, 3:00 PM

Can't we factor out some shared state and have JoinHandler set the state after it tries once? We would check the state in both components

ginsu updated this revision to Diff 42221.Jul 11 2024, 12:54 AM

update

Harbormaster failed remote builds in B30279: Diff 42221!Jul 11 2024, 1:07 AM
ginsu added inline comments.Jul 11 2024, 1:18 AM
native/components/auto-join-community-handler.react.js
41 ↗(On Diff #42221)

Introduced joinStatus and updated JoinHandler to set the state of the status based on whether the join community mechanism is currently in progress or has already been attempted once.

248 ↗(On Diff #42221)

Had a console.log() here, and was able to confirm that with the three communities the user is trying to auto join, we call joinCommunity three times

Screenshot 2024-07-11 at 3.53.12 AM.png (91×275 px, 11 KB)

ginsu attached a referenced file: F2235986: Screenshot 2024-07-11 at 3.53.12 AM.png. (Show Details)Jul 11 2024, 1:18 AM
Harbormaster completed remote builds in B30279: Diff 42221.Jul 11 2024, 1:54 AM
ashoat accepted this revision.Jul 11 2024, 5:57 PM

Please re-request review if either of the following is true:

  1. If useJoinCommunity don't always execute the add_keyserver step
  2. If my suggestion to simplify lines 157 through 165 into Object.values(communitiesObj).filter(Boolean) doesn't work or doesn't make sense
native/components/auto-join-community-handler.react.js
45 ↗(On Diff #42221)

Let's make this one read-only

161 ↗(On Diff #42221)

And then you can make this change to strip $ReadOnly so this part still works

163–165 ↗(On Diff #42221)
  1. It's not clear why you turned this into an object. You don't seem to be using the keys below in joinHandlers, and that seems to be the only place you communitiesToAutoJoin. Why did you update this to an object?
  2. If actually starts as an object as filteredCommunitiesObj, but then you turn it into an array and then rekey it. Is all of that work necessary?

I think you can probably replace all of lines 157 through 165 with just:

Object.values(communitiesObj).filter(Boolean)

And then convert communitiesToAutoJoin back to $ReadOnlyArray<CommunityToAutoJoin>

258 ↗(On Diff #42221)

Are you sure we always execute the add_keyserver step? Do we skip it if the keyserver is already in the store?

This revision is now accepted and ready to land.Jul 11 2024, 5:57 PM
ginsu requested review of this revision.Jul 11 2024, 11:38 PM

It's not clear why you turned this into an object. You don't seem to be using the keys below in joinHandlers, and that seems to be the only place you communitiesToAutoJoin. Why did you update this to an object?

I updated communitiesToAutoJoin into an object so that when we call setCommunitiesToAutoJoin inside of the JoinHandler component, we would not need to use the .find() method to locate the correct community to update in communitiesToAutoJoin. I thought this would be a bit more performant

native/components/auto-join-community-handler.react.js
45 ↗(On Diff #42221)

Will update this if we decide that communitiesToAutoJoin should remain an object

161 ↗(On Diff #42221)

Will update this if we decide that communitiesToAutoJoin should remain an object

163–165 ↗(On Diff #42221)

If actually starts as an object as filteredCommunitiesObj, but then you turn it into an array and then rekey it. Is all of that work necessary?

Yea looking at this again, I agree some of this work is a bit redundant/unnecessary. Will fix this when I update the diff

258 ↗(On Diff #42221)

Confirmed in the useJoinCommunity hook that we always execute the add_keyserver step

At the end of the createJoinPromise callback we call setStep('add_keyserver');:
https://github.com/CommE2E/comm/blob/master/lib/shared/community-utils.js#L272

One of the first things we check in the add_keyserver step is if the keyserver is known. If it is, we then move to the next step which is auth_to_keyserver and from there there's another check that will move us to the join community step

https://github.com/CommE2E/comm/blob/master/lib/shared/community-utils.js#L288-L291
https://github.com/CommE2E/comm/blob/master/lib/shared/community-utils.js#L321-L325

Also ran the auto join community handler and added a log just right above setCommunitiesToAutoJoin as a sanity check and confirmed that we are always executing the add_keyserver step

Screenshot 2024-07-12 at 12.23.44 AM.png (114×844 px, 30 KB)

ginsu attached a referenced file: F2247320: Screenshot 2024-07-12 at 12.23.44 AM.png. (Show Details)Jul 11 2024, 11:38 PM
ashoat accepted this revision.Jul 12 2024, 7:10 AM

I updated communitiesToAutoJoin into an object so that when we call setCommunitiesToAutoJoin inside of the JoinHandler component, we would not need to use the .find() method to locate the correct community to update in communitiesToAutoJoin. I thought this would be a bit more performant

Thanks, I totally missed the setCommunitiesToAutoJoin call – my bad! Let's keep it as an object.

I can also see that keeping the old key structure (keying on Farcaster channel) won't be helpful, so it makes sense that you want to rekey it.

Yea looking at this again, I agree some of this work is a bit redundant/unnecessary. Will fix this when I update the diff

Thanks – looks like you'll want to keep the rekeying logic, but it can probably be simplified a tiny bit (skipping the conversion to an array, and just directly rekeying from one object to another).

native/components/auto-join-community-handler.react.js
258 ↗(On Diff #42221)

Thanks for sharing this detailed analysis!!

This revision is now accepted and ready to land.Jul 12 2024, 7:10 AM
ginsu updated this revision to Diff 42262.Jul 12 2024, 4:59 PM

address comments + rebase before landing

Harbormaster completed remote builds in B30305: Diff 42262.Jul 12 2024, 5:16 PM
Closed by commit rCOMM41570ee3f402: [native] update AutoJoinCommunityHandler to use useJoinCommunity hook (authored by ginsu). · Explain WhyJul 12 2024, 5:48 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM41570ee3f402: [native] update AutoJoinCommunityHandler to use useJoinCommunity hook.

Revision Contents
Changeset List

PathSize
native/
components/
138 lines

Diff 42197

View Options

native/components/auto-join-community-handler.react.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines