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

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

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

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

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

address comments

Messed something up when stashing + rebasing

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: After the changes when I tried to auto join 3 different communities:

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)

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

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

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

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

If useJoinCommunity don't always execute the add_keyserver step
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)	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? 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

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

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

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

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.

[native] update AutoJoinCommunityHandler to use useJoinCommunity hook
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42201

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

[native] update AutoJoinCommunityHandler to use useJoinCommunity hookClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42201

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

[native] update AutoJoinCommunityHandler to use useJoinCommunity hook
ClosedPublic
Actions

Revision Contents
Changeset List