[native] Introduce `useNativeSetThreadAvatar` and use throughout `native`
ClosedPublic
Actions

Authored by atul on Aug 8 2023, 1:36 AM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMMc97d3b71cef5: [native] Introduce `useNativeSetThreadAvatar` and use throughout `native`

Summary

Thread equivalent of D8341

setThreadAvatar previously had a call to displayFailureAlert, however, this function is only relevant on native since we don't display alerts on web.

As part of making the provider platform-agnostic, we "lift up" error handling/alert displaying to avatar-hooks.

We replace usages of setThreadAvatar on native with nativeSetUserAvatar.

Test Plan

The modified flows on native continue to work as expected.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D8757 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Aug 8 2023, 1:36 AM

Herald added a subscriber: tomek. · View Herald TranscriptAug 8 2023, 1:36 AM

atul added a child revision: D8758: [lib][native] Remove `displayFailureAlert` prop from `EditThreadAvatarProvider`.Aug 8 2023, 1:40 AM

atul added inline comments.Aug 8 2023, 1:42 AM

native/avatars/avatar-hooks.js
313 ↗	(On Diff #29610)	There should be a separate `UpdateThreadAvatarRequest` (which excludes ENS request). Will handle cleaning of `avatar-types` in a diff later in this stack.

Harbormaster completed remote builds in B21584: Diff 29610.Aug 8 2023, 1:54 AM

atul requested review of this revision.Aug 8 2023, 1:54 AM

I don't think you should be swallowing the exception. If you disagree, can you please explain and then re-request review?

native/avatars/avatar-hooks.js
327 ↗	(On Diff #29610)	In the old code, we would still throw an exception here. Is there a reason you're swalling it now instead?
native/chat/settings/emoji-thread-avatar-creation.react.js
44 ↗	(On Diff #29610)	I think you'll end up displaying this "Avatar updated!" toast on failure because you're not re-throwing the exception you catch

This revision is now accepted and ready to land.Aug 8 2023, 7:12 AM

atul added inline comments.Aug 8 2023, 5:14 PM

native/avatars/avatar-hooks.js
327 ↗	(On Diff #29610)	No, the exception should be definitely be re-thrown. Thanks for catching that, I've had multiple promise/exception handling issues in this stack... will try to be more careful going forward.
native/chat/settings/emoji-thread-avatar-creation.react.js
44 ↗	(On Diff #29610)	Yeah, the exception in `nativeSetThreadAvatar` should be re-thrown.