Page MenuHomePhabricator

[native] update AutoJoinCommunityHandler to use useJoinCommunity hook
ClosedPublic

Authored by ginsu on Wed, Jul 10, 12:29 AM.
Tags
None
Referenced Files
F2321440: D12714.id42262.diff
Tue, Jul 23, 4:47 AM
F2319362: D12714.id42211.diff
Mon, Jul 22, 7:32 PM
F2318640: D12714.id42212.diff
Mon, Jul 22, 2:33 PM
Unknown Object (File)
Mon, Jul 22, 5:13 AM
Unknown Object (File)
Mon, Jul 22, 3:24 AM
Unknown Object (File)
Fri, Jul 19, 9:56 PM
Unknown Object (File)
Fri, Jul 19, 9:56 PM
Unknown Object (File)
Fri, Jul 19, 9:15 AM
Subscribers

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

ashoat requested changes to this revision.Wed, Jul 10, 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.Wed, Jul 10, 10:56 AM

Messed something up when stashing + rebasing

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)

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

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

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

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)

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.Thu, Jul 11, 5:57 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)

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.Fri, Jul 12, 7:10 AM

address comments + rebase before landing