Page MenuHomePhabricator

[native] have upload thread avatar processing/uploading loader only appear for thread being updated
ClosedPublic

Authored by ginsu on Apr 26 2023, 9:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:40 PM
Unknown Object (File)
Wed, Nov 6, 3:40 PM
Unknown Object (File)
Tue, Nov 5, 8:59 AM
Unknown Object (File)
Tue, Oct 29, 5:14 PM
Unknown Object (File)
Tue, Oct 29, 7:58 AM
Unknown Object (File)
Sun, Oct 27, 12:55 AM
Unknown Object (File)
Sun, Oct 27, 12:55 AM
Unknown Object (File)
Sun, Oct 27, 12:55 AM
Subscribers

Details

Summary

In selectFromGalleryAndUpdateThreadAvatar we had a piece of state called processingOrUploadInProgress that would keep track of when we were uploading or processing the image the user selected in the setting image avatar process. We previously kept track of this state using a boolean; however, with this state now being used in this provider we need a more robust way to keep track of this state for a thread, and make sure this state is not shared across different threads.

To fix this, I changed the type of this state to be a Set<string> from boolean. This set will keep track of all the thread ids that are actively uploading or processing an image for a new thread avatar. We then have threadAvatarSaveInProgress check if this set has the active threadID currently in this set will update the loader accordingly

Depends on D7684

Test Plan

I tested setting an image thread avatar with a delayed upload/processing and the image thread avatar loaders are only shown in the correct thread

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu added reviewers: ashoat, atul.
ginsu edited the summary of this revision. (Show Details)

Will split this diff up even more too since I'm moving and adding code

ashoat requested changes to this revision.Apr 27 2023, 10:24 AM

I can't tell for sure, but it looks like you're moving and updating code in the same diff

https://www.notion.so/commapp/Moving-code-around-bbb551c4350b4d5cb553c6751be44314

Please separate into two diffs if that's the case, or otherwise please re-request review and clarify that this is a pure move (in which case I will accept it blindly)

split diff into two (this is part 2 and part 1 is handeled by D7684)

ginsu retitled this revision from [native] move selecting from gallery logic from avatar hooks to edit thread avatar to [native] have upload thread avatar processing/uploading loader only appear for thread being updated.Apr 28 2023, 1:05 PM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Apr 28 2023, 2:08 PM
ashoat added inline comments.
native/avatars/edit-user-avatar-provider.react.js
60–63 ↗(On Diff #25926)

What does this achieve?

This revision now requires changes to proceed.Apr 28 2023, 2:08 PM
native/avatars/edit-user-avatar-provider.react.js
60–63 ↗(On Diff #25926)

I initially thought it would be okay to have this callback to have a matching structure between EditUserAvatarProvider and EditThreadAvatarProvider but I see that it is unnecessary and I shouldn't have put this in initially so let's just get rid of this and pass setUserAvatarMediaUploadInProgress directly

This pattern of passing functions directly in instead of creating aliases is always easier if you type functions that take other functions as input (like useUploadSelectedMedia here) in a more permissive way, but letting them take any function that returns anything (mixed) instead of insisting that those functions return void specifically. In this case it didn’t matter because setState functions return void (see here) but it’s still a good pattern to follow.

This revision is now accepted and ready to land.Apr 30 2023, 1:38 PM
native/avatars/avatar-hooks.js
116 ↗(On Diff #25950)

Let me be more explicit: @ginsu, please fix this type

native/avatars/avatar-hooks.js
116 ↗(On Diff #25950)

I'm sorry did not mean to land a diff w/o addressing any feedback. I thought because of setState returning void it was okay to land. D7692 should address this concern.