[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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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.