Details
- Reviewers
kamil inka - Commits
- rCOMMb38f4098c4b9: [lib] DMOperationSpec for add members operation
Just the Flow. Further testing will be performed after it is connected with the rest of the app.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- dmops
- 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? |
| 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) |
Should be readonly.
Yes, the plan was to wait for the rebase with introducing this change.
Yes, updated the type to be synchronous. |
| lib/shared/dm-ops/process-dm-ops.js | ||
|---|---|---|
| 26 | We can also consider passing all the thread infos directly | |
| 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) | This will be handled in https://linear.app/comm/issue/ENG-8763/queue-updates-that-cant-be-currently-applied |
| 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) | Actually this is calculated based on current users store, not on what the other client has sent us. |
| 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. |
| lib/shared/dm-ops/add-members-spec.js | ||
|---|---|---|
| 81–82 ↗ | (On Diff #42908) | 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? |
| lib/shared/dm-ops/add-members-spec.js | ||
|---|---|---|
| 81–82 ↗ | (On Diff #42908) | 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 ↗ | (On Diff #42908) | 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 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.