Page MenuHomePhabricator

[lib] Introduce `useNativeUpdateThreadImageAvatar` to `AvatarHooks`
ClosedPublic

Authored by atul on Aug 5 2023, 8:14 PM.
Tags
None
Referenced Files
F2148716: D8740.id29564.diff
Sun, Jun 30, 6:06 AM
Unknown Object (File)
Sat, Jun 29, 12:18 PM
Unknown Object (File)
Thu, Jun 27, 8:56 PM
Unknown Object (File)
Mon, Jun 24, 2:32 PM
Unknown Object (File)
Sun, Jun 16, 3:35 AM
Unknown Object (File)
Fri, Jun 14, 1:24 AM
Unknown Object (File)
Thu, Jun 13, 6:35 PM
Unknown Object (File)
Thu, Jun 13, 8:13 AM
Subscribers

Details

Summary

The updateThreadImageAvatar function within EditThreadAvatarProvider previously contained a call to the displayFailureAlert(...) function, which was passed in via props. However, the displayFailureAlert function is only relevant on native as we surface errors differently on web.

As part of making EditThreadAvatarProvider platform-agnostic, we introduce the useNativeUpdateThreadImageAvatar() hook. The function it "creates" encapsulates the call to updateThreadImageAvatar in a try/catch block that handles errors in a native-specific way. As a result, updateImageThreadAvatar can throw a plain old exception that will be caught by the platform-specific "wrapper" functions.

In subsequent diffs we'll do the same thing for setThreadAvatar so we can completely removed the displayFailureAlert prop from EditThreadAvatarProvider. After that we'll work on removing the useUploadSelectedMedia prop so the provider is fully "platform-agnostic." At that point we'll be able to consolidate things into a single EditThreadAvatarProvider component.

NOTE: This is the "thread avatar version" of D8339.
Test Plan
  1. Modify thread update endpoint to throw ServerError
  2. Try to set thread avatar via "Camera" flow
  3. Ensure that alert still appears as expected (via displayFailureAlert)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Aug 5 2023, 8:19 PM
atul added inline comments.
lib/components/base-edit-thread-avatar-provider.react.js
107–114 ↗(On Diff #29564)

Basically we're "lifting" the try/catch (and platform-specific alert) to the platform-specific nativeUpdateThreadImageAvatar that's created via useNativeUpdateThreadImageAvatar.

Also defining IIFE as promise and awaiting for symmetry with EditUserAvatarProvider.

native/avatars/avatar-hooks.js
311–335 ↗(On Diff #29564)

This is intended to be a fairly "dumb wrapper" around the platform-agnostic updateImageThreadAvatar with platform-specific error handling (aka display an alert)

native/media/thread-avatar-camera-modal.react.js
34 ↗(On Diff #29564)

Note that in some places we have eg updateThreadImageAvatar and other places we have updateImageThreadAvatar. There are also some mixups with the user avatar symbol naming.

I'm aware of this and have found it annoying enough that I'll put up a diff later in this stack to standardize on:

[entity][type]Avatar

where entity is user or thread
where type is image or emoji or ENS

So names would be eg

EditThreadEmojiAvatar
EditUserEmojiAvatar
EditUserENSAvatar
etc..

ashoat added inline comments.
native/avatars/avatar-hooks.js
323 ↗(On Diff #29564)

No reason to return result of updateImageThreadAvatar?

native/media/thread-avatar-camera-modal.react.js
33 ↗(On Diff #29564)

No reason to return the promise? Seems more "flexible" to do that

This revision is now accepted and ready to land.Aug 6 2023, 6:54 AM
lib/components/base-edit-thread-avatar-provider.react.js
107–110 ↗(On Diff #29564)

There's no point to the construction of this extra promise.

You immediately invoke the async function you create here, which means updateThreadAvatarMediaUploadInProgress(false) is called immediately. You then have a promise which is awaiting changeThreadSettingsCall(updateThreadRequest)... so why not return that promise directly?

lib/components/base-edit-thread-avatar-provider.react.js
107–110 ↗(On Diff #29564)

Ah yeah that makes more sense

native/avatars/avatar-hooks.js
323 ↗(On Diff #29564)

I don't think so, we "get what we need" from the SUCCESS and FAILURE payloads

native/media/thread-avatar-camera-modal.react.js
33 ↗(On Diff #29564)

Guess it doesn't hurt

have sendPhoto return promise

This revision was landed with ongoing or failed builds.Aug 7 2023, 11:26 PM
This revision was automatically updated to reflect the committed changes.