Page MenuHomePhabricator

[lib] DMOperationSpec for add members operation
ClosedPublic

Authored by tomek on Jul 22 2024, 6:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 23, 8:27 PM
Unknown Object (File)
Tue, Oct 22, 1:17 PM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Unknown Object (File)
Tue, Oct 22, 8:34 AM
Subscribers

Details

Summary
Test Plan

Just the Flow. Further testing will be performed after it is connected with the rest of the app.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/dm-ops/dm-op-spec.js
10 ↗(On Diff #42608)

Why is this not read only?

tomek requested review of this revision.Jul 22 2024, 7:07 AM
lib/shared/dm-ops/dm-op-spec.js
10 ↗(On Diff #42608)

I think after rebasing, this diff will introduce a Flow error until fetchThread is specified

Separately, I don't know if this needs to return a Promise... with the current design, all threads are in Redux, so we should be able to return it immediately

lib/shared/dm-ops/dm-op-spec.js
10 ↗(On Diff #42608)

Why is this not read only?

Should be readonly.

I think after rebasing, this diff will introduce a Flow error until fetchThread is specified

Yes, the plan was to wait for the rebase with introducing this change.

Separately, I don't know if this needs to return a Promise... with the current design, all threads are in Redux, so we should be able to return it immediately

Yes, updated the type to be synchronous.

lib/shared/dm-ops/process-dm-ops.js
26 ↗(On Diff #42701)

We can also consider passing all the thread infos directly

Still have to figure out a distinction between this and JOIN_THREAD

lib/shared/dm-ops/add-members-spec.js
44 ↗(On Diff #42740)

It may happen that some of these users are already members of the chat from the viewer's point of view. We could filter them out here, but that would mean that different clients can have different message infos. I think we should keep the infos consistent between clients, so keeping the unfiltered list here.

71 ↗(On Diff #42740)
lib/types/dm-ops.js
149–151 ↗(On Diff #42740)

In our current approach, we're sending the same message to all the peers. We can consider sending different payloads to different peers - e.g. the user that is added to a thread needs more data than a member of that thread.

lib/shared/dm-ops/add-members-spec.js
81 ↗(On Diff #42740)

Are we sure we want to crash users app if another client sends them incorrect data?

109 ↗(On Diff #42740)

should we also update the unread status?

lib/shared/dm-ops/add-members-spec.js
81 ↗(On Diff #42740)

Actually this is calculated based on current users store, not on what the other client has sent us.
On the other hand this is something someone has sent to the current user before. This makes me wander if we should be doing this validation as we first get the thread info. And if there is any other validation we should do

lib/shared/dm-ops/add-members-spec.js
81 ↗(On Diff #42740)

In the most recent approach, we're creating the thread info ourselves when we the thread for the first time. So this code becomes safer.

Include new messages in thread creation join update

inka added inline comments.
lib/shared/dm-ops/add-members-spec.js
81–82

In this branch we ignore other existingThreadDetails. What if they don't match what we have? What would that mean and should we do anything about it?

This revision is now accepted and ready to land.Jul 31 2024, 1:45 AM
lib/shared/dm-ops/add-members-spec.js
81–82

We're intentionally ignoring them. They represent a state of a thread from the sender's point of view. And that could be different from what a receiver has. We use this value when creating a new thread. But if the receiver already has the thread, their state could be older or younger than the existingThreadDetails they received. What matters is the intention of adding new members. Eventually, the state should become the same, after all the clients receive all the missing updates.

lib/shared/dm-ops/add-members-spec.js
81–82

Makes sense, thank you!

I wonder if the AddMembers spec should be split into two: one for adding the viewer, and one for adding somebody else

Then when somebody adds a member, they'll send the first type to all of the added users' devices, and the second type to all of the existing users' devices

What do you think?

I wonder if the AddMembers spec should be split into two: one for adding the viewer, and one for adding somebody else

Then when somebody adds a member, they'll send the first type to all of the added users' devices, and the second type to all of the existing users' devices

What do you think?

I think it is a good idea, but it will require changes in multiple places. Created a task where we can track it https://linear.app/comm/issue/ENG-8929/split-add-members-operation.