Details

Reviewers

Commits

rCOMM392dacb6c3e0: [lib] use fc channel avatar url in useResolvedThreadAvatar

Summary

Depends on D13743

useResolvedThreadAvatar can take an object containing a username and FID or an object containing a farcaster channel ID.

the former is when we want to display a user avatar as the thread avatar

we want these types to be mutually exclusive to avoid confusion when the avatar type is 'farcaster' and the function caller provides a farcaster channel ID and a user FID

Test Plan

tested in next diffs by successfully rendering farcaster channel images as thread avatars

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Oct 17 2024, 5:50 AM

varun held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptOct 17 2024, 5:50 AM

varun added a child revision: D13745: [web] update ThreadAvatar component to pass farcaster channel ID to useResolvedThreadAvatar.Oct 17 2024, 5:53 AM

varun added inline comments.Oct 17 2024, 5:57 AM

lib/shared/avatar-utils.js
347–354 ↗	(On Diff #45252)	this is an unrelated change but felt it was small enough to include here

Harbormaster completed remote builds in B32290: Diff 45252.Oct 17 2024, 6:11 AM

varun published this revision for review.Oct 17 2024, 8:57 PM

varun added inline comments.

lib/shared/avatar-utils.js
369 ↗	(On Diff #45252)	we want these types to be mutually exclusive to avoid confusion when the avatar type is 'farcaster' and the function caller provides a farcaster channel ID and a user FID

ashoat requested changes to this revision.Oct 20 2024, 11:33 AM

ashoat added inline comments.

lib/shared/avatar-utils.js
367 ↗	(On Diff #45252)	I'm worried that we're making `useResolvedThreadAvatar`'s API kinda complex here. Previously the second param here was sort of a "extra info if you need it" situation, and the decision for what to render was fully up to `useResolvedThreadAvatar`. But now the responsibility for that decision is split a little bit, because the caller needs to know what extra info to send, and the result of the render depends on what extra info is included. You call this out here. But I think it would be better to keep all the decision-making in `useResolvedThreadAvatar`. Here's what I'm thinking of: Replace the second param here with a "big of params" Include both the username and channel info when calling? What do you think?
369 ↗	(On Diff #45252)	I don't think they're mutually exclusive... aren't all `FCChannelInfo`s also a `UsernameAndFID`?
377–378 ↗	(On Diff #45252)	These checks are only necessary here because the types are not mutually exclusive

This revision now requires changes to proceed.Oct 20 2024, 11:33 AM

ashoat mentioned this in D13745: [web] update ThreadAvatar component to pass farcaster channel ID to useResolvedThreadAvatar.Oct 20 2024, 11:36 AM

ashoat mentioned this in D13746: [native] update ThreadAvatar component to pass farcaster channel ID to useResolvedThreadAvatar.

address feedback

Harbormaster completed remote builds in B32332: Diff 45321.Oct 22 2024, 12:32 PM

varun added inline comments.Oct 22 2024, 12:57 PM

lib/shared/avatar-utils.js
380–383 ↗	(On Diff #45321)	will change this before landing

ashoat requested changes to this revision.Oct 22 2024, 1:38 PM

ashoat added inline comments.

lib/shared/avatar-utils.js
366–370 ↗	(On Diff #45321)	Your suggested change won't work below because you're losing the read-only-ness of `UsernameAndFID` when spreading. You need to do this
374 ↗	(On Diff #45321)	Rather than this, I was thinking of something like this: params: ?{ +userProfileInfo?: ?UsernameAndFID, +channelInfo?: ?FCChannelInfo, }, Flattening the two sets of params here seems unnecessary and complicated
406–410 ↗	(On Diff #45321)	What is this change about? Why are we hardcoding this?

This revision now requires changes to proceed.Oct 22 2024, 1:38 PM

varun added inline comments.Oct 22 2024, 2:26 PM

lib/shared/avatar-utils.js
406–410 ↗	(On Diff #45321)	we need a default resolved thread avatar if `useFarcasterChannelAvatarURL` returns null (this could happen if a Farcaster channel is deleted, for example) or the caller doesn't supply any params (maybe we could use an invariant to handle this case since we don't expect it to happen?)

varun added inline comments.Oct 22 2024, 2:29 PM

lib/shared/avatar-utils.js
406–410 ↗	(On Diff #45321)	we could use the farcaster logo instead of an emoji if the caller supplies an `fcChannelID` but `useFarcasterChannelAvatarURL` returns null

ashoat added inline comments.Oct 22 2024, 2:37 PM

lib/shared/avatar-utils.js
406–410 ↗	(On Diff #45321)	maybe we could use an invariant to handle this case since we don't expect it to happen? Ideally, `invariant`s are for things that you can prove are impossible, but the typechecker isn't smart enough to realize. They're definitely inappropriate for something that can happen if a fetch fails, and especially so given this seems to occur while the fetch is ongoing. Really they should never be used for something that can happen.

ashoat added inline comments.Oct 22 2024, 2:54 PM

lib/shared/avatar-utils.js
406–410 ↗	(On Diff #45321)	Spent some more time on this and I think this change makes sense, but with a different emoji. Not sure if you intentionally diverged from `defaultAnonymousUserEmojiAvatar`, but if so I'm confused by why you thought a smiley (🙂) was an improvement over the anonymous silhouette (👤) for representing a Farcaster-linked community. How about a generic business building? Feels kinda anonymous but with more people: 🏢 Could also use this one which is a group of 2 people: 👥 Separately: you would have saved me 25min if you had included a note like below. Would appreciate you going deeper next time. This case can occur when fetching a Farcaster channel avatar URL fails or is in-progress. In my prior revision, I was falling back to `useResolvedUserAvatar`'s default, which is `defaultAnonymousUserEmojiAvatar`. After thinking about it more, I think it would be better to display a different default in this case.

address feedback

varun added inline comments.Oct 22 2024, 4:18 PM

lib/shared/avatar-utils.js
325		threw this change in. we can use a slightly stricter type here
374 ↗	(On Diff #45321)	I've made `params` non-nullable, `userProfileInfo` required, and `channelInfo` required and non-nullable. seemed better to make these types as strict as possible
native/avatars/thread-avatar.react.js
60	this is replaced two diffs later with a nullable `fcChannelID` that we get from the `communityStore`
web/avatars/thread-avatar.react.js
60	this is replaced in the next diff with a nullable `fcChannelID` that we get from the `communityStore`