Page MenuHomePhabricator
Differential D13744

[lib] use fc channel avatar url in useResolvedThreadAvatar
ClosedPublic
Actions

Authored by varun on Oct 17 2024, 5:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 8:17 AM
Unknown Object (File)
Wed, Nov 20, 8:16 AM
Unknown Object (File)
Wed, Nov 20, 8:16 AM
Unknown Object (File)
Tue, Nov 19, 4:30 PM
Unknown Object (File)
Mon, Nov 11, 12:53 PM
Unknown Object (File)
Sun, Nov 10, 9:55 PM
Unknown Object (File)
Sat, Nov 9, 11:17 AM
Unknown Object (File)
Thu, Nov 7, 2:07 AM
View All 44 Files
Subscribers
tomek

Details

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

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:

  1. Replace the second param here with a "big of params"
  2. 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 FCChannelInfos 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.
varun updated this revision to Diff 45321.Oct 22 2024, 12:11 PM

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, invariants 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.Tue, Oct 22, 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.

varun updated this revision to Diff 45323.Tue, Oct 22, 4:05 PM

address feedback

varun added inline comments.Tue, Oct 22, 4:18 PM
lib/shared/avatar-utils.js
325 ↗(On Diff #45323)

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 ↗(On Diff #45323)

this is replaced two diffs later with a nullable fcChannelID that we get from the communityStore

web/avatars/thread-avatar.react.js
60 ↗(On Diff #45323)

this is replaced in the next diff with a nullable fcChannelID that we get from the communityStore

varun planned changes to this revision.Tue, Oct 22, 4:21 PM
varun added inline comments.
lib/shared/avatar-utils.js
378–381 ↗(On Diff #45323)

forgot to make this change. fixing now

Harbormaster completed remote builds in B32334: Diff 45323.Tue, Oct 22, 4:25 PM
varun updated this revision to Diff 45327.Tue, Oct 22, 4:27 PM

replace argument to useResolvedUserAvatar and add a code comment

Harbormaster completed remote builds in B32338: Diff 45327.Tue, Oct 22, 4:52 PM
ashoat accepted this revision.Tue, Oct 22, 5:26 PM
This revision is now accepted and ready to land.Tue, Oct 22, 5:26 PM
Closed by commit rCOMM392dacb6c3e0: [lib] use fc channel avatar url in useResolvedThreadAvatar (authored by varun). · Explain WhyTue, Oct 22, 9:51 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM392dacb6c3e0: [lib] use fc channel avatar url in useResolvedThreadAvatar.

Revision Contents
Changeset List

PathSize
lib/
shared/
49 lines
native/
avatars/
5 lines
web/
avatars/
5 lines

Diff 45337

View Options

lib/shared/avatar-utils.js

Loading...
View Options

native/avatars/thread-avatar.react.js

Loading...
View Options

web/avatars/thread-avatar.react.js

Loading...