[lib] Fix missing thread bug in updateRelationshipsAndSendRobotext()
ClosedPublic
Actions

Authored by angelika on Wed, Jan 8, 5:35 PM.

Details

Reviewers

tomek
kamil

Commits

rCOMM0af6f0b441ba: [lib] Fix missing thread bug in updateRelationshipsAndSendRobotext()

Summary

Fixes https://linear.app/comm/issue/ENG-9946/cannot-read-properties-of-undefined-reading-members

In createThickThreadAndSendRobotextPromise there are two called two functions: createNewThickThread and sendRobotextToThickThread. After the thread is created, sendRobotextToThickThread is called but it’s an outdated reference that was captured before createNewThickThread is called and it uses old threads storage without the new thread.
The fix is inspired by "step-based approach" in the codebase but modified so that it allows for waiting for multiple threads at once.

Depends on D14187 but just for testing

Test Plan

Go to Profile -> Friend list
Select 1 or more users and add them as friends
Click Save
Verify that the requests are sent and there is no longer an error in the logs about missing thread

Diff Detail

Repository

rCOMM Comm

Branch

graszka22/ENG-9946

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Wed, Jan 8, 5:35 PM

Herald added a subscriber: ashoat. · View Herald TranscriptWed, Jan 8, 5:35 PM

angelika edited the summary of this revision. (Show Details)Wed, Jan 8, 5:36 PM

angelika added a parent revision: D14187: [native] Fix adding several friends at once.

Harbormaster completed remote builds in B33205: Diff 46618.Wed, Jan 8, 5:53 PM

angelika requested review of this revision.Wed, Jan 8, 5:53 PM

tomek requested changes to this revision.Thu, Jan 9, 1:59 AM

tomek added inline comments.

lib/hooks/relationship-hooks.js
43–48 ↗	(On Diff #46618)	We're using it more like a queue or even a pool of operations - step indicates that we have to consume them one-by-one and we don't do that. To reduce confusion, we can rename it.
118–121 ↗	(On Diff #46618)	This looks really dangerous because it allows growing the `steps` indefinitely. We should only add a new step if it isn't already processed (so move this `if` after the next one).
125 ↗	(On Diff #46618)	We never set the value in a map to false, so maybe we can simplify it by replacing it with a `Set`.
260 ↗	(On Diff #46618)	We prefer using spread over using the `concat`.

This revision now requires changes to proceed.Thu, Jan 9, 1:59 AM

angelika added inline comments.Thu, Jan 9, 7:31 AM

lib/hooks/relationship-hooks.js
118–121 ↗	(On Diff #46618)	We're not adding any new steps to the state in this useEffect. The steps added to newSteps array are the steps that are already in steps array and are still waiting for the thread so the length of newSteps array is always <= the length of steps array.

Review changes

Harbormaster completed remote builds in B33215: Diff 46634.Thu, Jan 9, 7:56 AM

The changes look good, but I have some more questions, that probably should've been asked in the first iteration - sorry for that!

lib/hooks/relationship-hooks.js
120	What if there are two operations for a single thread ID? Would we skip both of them when only one was in progress? Maybe we can keep a flag inside `Operation` telling if it's in progress. Or maybe we can use something unique as a key in the map?
130–132	Is it possible that in one iteration, we add one operation and remove another? If that's the case, we should introduce a flag telling if the operations changed instead of relying on the length.

This revision now requires changes to proceed.Fri, Jan 10, 1:53 AM

angelika added inline comments.Fri, Jan 10, 5:31 AM

lib/hooks/relationship-hooks.js
120	I think it's not possible for two operations to have the same thread ID. Notice that operations are added just in one place: when we just created a new thread and we need to wait for this thread to be in the store to send a message to it.
130–132	As mentioned in my other comment: we're not adding any new operations to the state in this useEffect. For each operation in the operations array we're either: processing the operation and not adding it to newOperations, so removing operation from further processing adding the operation to newOperation, so just retaining the operation that was in operations array before so the length of newOperations array is always <= the length of operations array. And if there are less operations in newOperations array than in operations array it means that we removed some operations. Maybe the name `newOperations` is misleading? Maybe something like `unprocessedOperations` or `retainedOperations` would be better?