Page MenuHomePhabricator

[native] Avoid updating relationships before the previous call finished
ClosedPublic

Authored by tomek on Oct 8 2024, 4:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 11:09 AM
Unknown Object (File)
Sun, Nov 10, 8:44 AM
Unknown Object (File)
Sun, Nov 10, 5:56 AM
Unknown Object (File)
Sun, Nov 10, 3:06 AM
Unknown Object (File)
Fri, Nov 1, 3:43 PM
Unknown Object (File)
Fri, Nov 1, 4:30 AM
Unknown Object (File)
Thu, Oct 24, 5:48 AM
Unknown Object (File)
Wed, Oct 23, 12:35 AM
Subscribers

Details

Summary

Make sure we don't update the relationship before the previous call ended. Currently, it is possible to click a button multiple times in a row which results in multiple thick threads being created.

https://linear.app/comm/issue/ENG-9508/adding-new-friends-can-result-in-creating-multiple-thick-threads

Test Plan

Click the save button multiple times and check if only one thread was created.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/profile/relationship-list.react.js
205–230 ↗(On Diff #44962)

Ideally, we could use loadingStatus here. The problem with this is that we're updating the button using setOptions from navigation, which is slow and still allows multiple interactions before the correct state is rendered.

tomek requested review of this revision.Oct 8 2024, 5:07 AM
native/profile/relationship-list.react.js
205–230 ↗(On Diff #44962)

Either way you're updating the button via setOptions, right? And either way you'll need a check in updateRelationshipsOnServer to deal with that.

I imagine it might be slower to check loadingStatus inside updateRelationshipsOnServer instead of this ref because loadingStatus only updates later, after the first action is reduced. I'm guessing you could get multiple clicks in before this gets processed.

This revision is now accepted and ready to land.Oct 10 2024, 4:51 AM
native/profile/relationship-list.react.js
205–230 ↗(On Diff #44962)

Either way you're updating the button via setOptions, right?

Yes

And either way you'll need a check in updateRelationshipsOnServer to deal with that.

Not necessarily, because updateRelationshipsOnServer callback would depend on the loadingStatus so if the update to the button was immediate (in the next render) then it would be relatively safe.

I imagine it might be slower to check loadingStatus inside updateRelationshipsOnServer instead of this ref because loadingStatus only updates later, after the first action is reduced. I'm guessing you could get multiple clicks in before this gets processed.

My use of the word slow was unfortunate. What happens here is that updateRelationshipsOnServer is a callback that would depend on the loadingStatus. After loadingStatus changes, an effect would be run and call setOptions with a new onPressAdd that depends on updateRelationshipsOnServer. It takes some time for the setOptions call to take effect, which means that for a couple of renders the button would still have a callback with an outdated loadingStatus bound. So overall, checking the loadingStatus isn't slow, but reflecting its change in the UI is.