good point, we should expect all communities passed to this component to have a farcasterChannelID. I need to tweak the logic in the parent components

thanks for catching this

it can be, but don't we typically prefer to make the constraints as tight as possible? we expect all the items in this list to have a farcaster channel ID

I was thinking initially that the check could simply be removed if we're not sure if we have a farcasterChannelID. I don't think we need that as a hard requirement. But if it's built-in as a requirement already, I suppose you can tweak the types instead

I was asking in the context of my earlier comment about removing the !farcasterChannelID check

rethinking this. it's not so straightforward to tweak the types since the keyserver API we're using returns the communities a user is already in (regardless of whether or not there's an associated farcaster channel ID).

i think we should keep this check, because we only want to show public (i.e., farcaster channel-associated) communities in our directory

Can't we decide that on the keyserver side? I'd prefer to keep the client logic as flexible as possible

Want to make sure we're on the same page. The API currently returns:

export type ServerFetchNativeDrawerAndDirectoryInfosResponse = {
  +allCommunityInfosWithNames: $ReadOnlyArray<ServerCommunityInfoWithCommunityName>,
};

export type ServerCommunityInfoWithCommunityName = $ReadOnly<{
  ...ServerCommunityInfo,
  +communityName: string,
  +threadInfo: LegacyThinRawThreadInfo | ThinRawThreadInfo | null,
}>;

export type ServerCommunityInfo = {
  +id: string,
  +farcasterChannelID: ?string,
};

You're suggesting modifying the response type to look like:

export type ServerFetchNativeDrawerAndDirectoryInfosResponse = {
  +memberCommunities: $ReadOnlyArray<ServerCommunityInfoWithCommunityName>,
  +nonMemberCommunities: $ReadOnlyArray<PublicCommunityAndThreadInfo>
};

export type PublicCommunityAndThreadInfo = $ReadOnly<{
  +id: string,
  +farcasterChannelID: string,
  +communityName: string,
  +threadInfo: LegacyThinRawThreadInfo | ThinRawThreadInfo | null,
}>;

Or am I misunderstanding?

I'm confused. I'm suggesting no changes to the server APIs. I'm suggesting what I initially suggested in my first review:

1. Remove the !farcasterChannelID check here
2. Update ThreadInfoAndFarcasterChannelID to have farcasterChannelID be optional

Oh I missed what you were saying above... so the keyserver API (is this fetchAllCommunityInfosWithNames?) returns a union of all Farcaster-tagged communities, and all the communities the user is in? Vaguely recalling this now... the context in my head gets lost as it's taking a while between iterations

Are you available for a quick Zoom call to try and figure this out? I'm checking the codebase now, and confused why we decided to do this, as the endpoint appears to only be used here. Was the intention to use it for refreshing the communities displayed in the sidebar as well, or am I missing something?

We discussed this on the call and we're going to move the filter upstream:

1. In D14053, we'll add a filter to DisplayCommunityDirectoryPromptHandlerInner before calling navigate()
2. In this diff, we'll add a filter to CommunityDrawerContent before calling navigate()

And we'll apply the suggestions I made earlier:

1. Remove the !farcasterChannelID check here
2. Update ThreadInfoAndFarcasterChannelID to have farcasterChannelID be optional

We agreed that @varun's proposed API change above would be good, but we don't have the cycles to prioritize right now.

in order to remove the !farcasterChannelID check here, farcasterChannelID has to be optional and nullable downstream

made it optional and nullable

as discussed last week, we will filter out all the communities that the user is already a member of