Details

Reviewers

kamil
ashoat
bartek

Commits

rCOMM7794e4bb2287: [native] introduce AutoJoinCommunityHandler

Summary

AutoJoinCommunityHandler handles the logic for getting the user to automatically join communites that are tagged to farcaster channels that the user follows.

In a subsequent diff we will update this logic so we background these communites/threads by default

Linear task: https://linear.app/comm/issue/ENG-7959/query-the-users-channels-that-they-follow-on-app-start + https://linear.app/comm/issue/ENG-7960/download-all-the-relevant-blobs-based-on-the-viewers-farcaster

Depends on D12064

Test Plan

Created a new community called Run club
Tagged /sovereignty farcaster channel to Run club
Logged out of my account
Created a new account with my fid hardcoded with my fid hardcoded in useCurrentUserFID
Confirmed that the new user auto joined run club since /sovereignty is a farcaster channel that my fid follows

Confirmed that when the authorization header is present, we were able to fetch the blobs, and when the authorization header were not present, the fetch calls failed

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.May 15 2024, 8:33 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 15 2024, 8:33 PM

ginsu edited the summary of this revision. (Show Details)May 15 2024, 8:36 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: tomek, kamil.

ginsu added a parent revision: D12064: [keyserver/lib] update joinThread to handle auto joining from farcaster channels.

Harbormaster completed remote builds in B28972: Diff 40275.May 15 2024, 8:50 PM

ginsu requested review of this revision.May 15 2024, 8:50 PM

This needs to be updated based on feedback on previous diffs

update

Switching @tomek with @ashoat for my reviewer since @tomek is on vacation

Harbormaster completed remote builds in B29690: Diff 41343.Fri, Jun 14, 1:56 PM

Accepting, but making @bartek blocking for one specific question about blob service auth

Please merge all of the async IIFEs before landing

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

85–87 ↗

(On Diff #41343)

@bartek is introducing auth for blob service. Does this need to be revised?

85–128 ↗

(On Diff #41343)

You can make the code more readable by reducing the number of steps:

const promises = followedFarcasterChannelIDs.map(channelID => {
  const blobHash = farcasterChannelTagBlobHash(channelID);
  const blobURL = getBlobFetchableURL(blobHash);

  const blobResult = await fetch(blobURL, {
    method: blobService.httpEndpoints.GET_BLOB.method,
  });

  if (blobResult.status !== 200) {
    return;
  }

  const { commCommunityID } = await blobResult.json();

  const keyserverID = commCommunityID.split('|')[0];
  if (keyserverID !== authoritativeKeyserverID()) {
    return;
  }

  // The user is already in the community
  if (threadInfos[commCommunityID]) {
    return;
  }

  void dispatchActionPromise(
    joinThreadActionTypes,
    joinThreadActionPromise(commCommunityID),
  );
})();
await Promise.all(promises);

The core thing that async IIFEs enable is composing async code. It's easier to reason about the steps here when they're working on a single community at a time

ginsu added a child revision: D12444: [keyserver] introduce setSubcriptionToDefault option in changeRole.Fri, Jun 14, 3:09 PM

Could you apply the same changes as I did in D12437?

native/components/auto-join-community-handler.react.js
85–87 ↗	(On Diff #41343)	Yes, thanks for the ping!

This revision now requires changes to proceed.Sun, Jun 16, 10:34 PM

address comments

ginsu edited the test plan for this revision. (Show Details)Mon, Jun 17, 11:08 PM

ginsu added inline comments.Mon, Jun 17, 11:14 PM

native/components/auto-join-community-handler.react.js
85–128 ↗	(On Diff #41343)	Makes sense, my initial thought was that it would be more performant not to `await` inside a loop, but this is definitely more readable

Harbormaster completed remote builds in B29734: Diff 41429.Mon, Jun 17, 11:23 PM

bartek accepted this revision.Mon, Jun 17, 11:46 PM

bartek added inline comments.

native/components/auto-join-community-handler.react.js
100–102 ↗	(On Diff #41429)	technically you can move it above the `.map()` loop to avoid recreating this each time. Headers are the same for each `fetch` call

This revision is now accepted and ready to land.Mon, Jun 17, 11:46 PM

A couple more async improvements. I should be able to give this a final review either today or tomorrow, but if not feel free to remove me from the review so you can land it while I'm out.

Makes sense, my initial thought was that it would be more performant not to await inside a loop, but this is definitely more readable

This seems like a core misunderstanding of how async/await work. There is no performance cost here, and in fact you are not awaiting inside a loop at all.

native/components/auto-join-community-handler.react.js
84–90 ↗	(On Diff #41429)	Why are we blocking `neynarClient.fetchFollowedFarcasterChannels` on `getAuthMetadata`?
100–102 ↗	(On Diff #41429)	Yes, let's avoid doing this multiple times

This revision now requires changes to proceed.Tue, Jun 18, 9:50 AM

address comments

ginsu edited the test plan for this revision. (Show Details)Tue, Jun 18, 11:17 AM

ashoat accepted this revision.Tue, Jun 18, 11:18 AM

This revision is now accepted and ready to land.Tue, Jun 18, 11:18 AM

Harbormaster completed remote builds in B29756: Diff 41463.Tue, Jun 18, 11:29 AM

ashoat added inline comments.Tue, Jun 18, 12:02 PM

native/components/auto-join-community-handler.react.js
123–126 ↗	(On Diff #41463)	Talked about this with @ginsu. He appears to be limiting Farcaster channel autojoin to the authoritative keyserver, but this doesn't make much sense as Farcaster channel association can be done regardless of keyserver. We talked it through and it seems like he was mostly worried about the user attempting to join a community on a keyserver that the user is not part of. It looks like @tomek recently introduced some code in `useAcceptInviteLink` to handle this by having the client join the linked keyserver. It would be good to adopt a similar approach here, but it's probably outside the scope of this diff. For now, I think we should replace this check with another that checks if the user has this keyserver in their `KeyserverStore` (`isKeyserverKnown` in `useAcceptInviteLink`).