Page MenuHomePhabricator

[native] Match changes made to `EditUserAvatar` for `EditThreadAvatar`
ClosedPublic

Authored by atul on Apr 19 2023, 4:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 5:21 PM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:13 AM
Unknown Object (File)
Fri, Nov 1, 9:12 AM
Unknown Object (File)
Fri, Nov 1, 9:09 AM
Unknown Object (File)
Tue, Oct 29, 11:01 AM
Unknown Object (File)
Oct 2 2024, 6:34 PM
Subscribers

Details

Summary

As stated in D7527:

For convenience, I'm going to have a multiple diffs where I update the EditUserAvatar flow, and then a single diff where I update the EditThreadAvatar flow to match. There's a lot of repetition, but I have ideas on how to consolidate things once we have a shippable experience.

This is that diff that brings thread avatars "up to speed" with user avatars. In hindsight it may have been good to make the changes simultaneously so this diff isn't so difficult to review.

But effectively this diff handles:

D7527: Updating useSelectFromGalleryAndUpdateThreadAvatar avatar with new processingOrUploadInProgress state and combining that with info from updateThreadAvatarLoadingStatusSelector to determine whether avatar update is "in progress" and to set inProgress accordingly. EditThreadAvatar pulls the inProgress boolean from useSelectFromGalleryAndUpdateThreadAvatar to (partially) determine whether to display a spinner. The changes here that correspond to D7527 are in useSelectFromGalleryAndUpdateThreadAvatar (we also change how we call the hook in EditThreadAvatar, but that's just wrapping in brackets). Almost all the changes here were copy/pasted from useSelectFromGalleryAndUpdateUserAvatar with slight differences for the endpoint and loading selector.

D7528: Updating useRemoveThreadAvatar hook to return isLoading. This is similar to the changes made to match D7527 described above, but effectively getting the avatar update status from updateThreadAvatarLoadingStatusSelector and exposing whether or not it's "loading" by returning inProgress. The changes here are in useRemoveThreadAvatar. The changes, again, were effectively copy/pasted from useRemoveUserAvatar.

D7529: Updating EditThreadAvatar to display a spinner based on the isLoading status from useSelectFromGalleryAndUpdateThreadAvatar and useRemoveThreadAvatar we get from the previous two changes. The code here is identical to the code in EditUserAvatar. Definitely some room for refactoring after this is shippable... I have a list of changes I want to make.

D7530: Basically replace the console.logs with Alert.alerts. The changes here are entirely in useSelectFromGalleryAndUpdateThreadAvatar and are identical to those in useSelectFromGalleryAndUpdateUserAvatar.

Apologies in advance to the reviewer, I probably should've just included the changes to threads along with the changes to users to make things easier to review. For what it's worth the changes are almost identical and effectively copy/paste. The only differences are the loading status selectors and the endpoints being called by dispatchActionPromise.

Test Plan

Tested these changes thoroughly.

  1. Set and unset avatar to images/emoji/etc and observed that spinner appeared as expected.
  2. Set and unset other thread settings and observed that spinner did not appear... again as expected.
  3. Logged the value of isLoading from both useSelectFromGalleryAndUpdateThreadAvatar and useRemoveThreadAvatar and ensured that they were exactly as expected.
  4. Toggled various booleans and ensures that the Alert.alerts appeared as expected.
  5. "Broke" various endpoints on keyserver (throw new ServerError() at entrypoint) and ensured that failures were displayed on client as expected (broke upload and threadUpdateResponder and made sure correct Alerts were displayed)
  6. Made a bunch of simultaneous thread avatar change requests from different users and ensured that things remained in a consistent state (not really in the scope of this diff though)
  7. Went carefully line-by-line and compared to EditUserAvatar (and accompanying user avatar hooks) and made sure that the two were identical except for loading status selectors and endpoints.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Apr 19 2023, 4:46 PM
ashoat added inline comments.
native/avatars/avatar-hooks.js
349 ↗(On Diff #25432)

Is there a period where this flips to false before updateThreadAvatarLoadingStatus === 'loading' flips to true? One potential solution would be to move this line to line 341

404 ↗(On Diff #25432)

We can remove the async here

(Worth checking if user avatars has the same thing)

This revision is now accepted and ready to land.Apr 19 2023, 6:12 PM
native/avatars/avatar-hooks.js
349 ↗(On Diff #25432)

Yes! There was no good way to handle this before when we were just dispatching, but now that we have the IIFE that would be the correct place to place it.

Thanks for pointing that out

404 ↗(On Diff #25432)

Will do.

There's also a hook or two in this file that no longer need to be hooks and can be normal JS functions... I have that on my list of cleanup tasks.

address feedback before landing

This revision was landed with ongoing or failed builds.Apr 19 2023, 8:29 PM
This revision was automatically updated to reflect the committed changes.