[lib] Fix leaving thread logic
ClosedPublic
Actions

Authored by tomek on Oct 4 2024, 9:12 AM.

Details

Reviewers

kamil
inka

Commits

rCOMM56bb0af60171: [lib] Fix leaving thread logic

Summary

The logic had a couple of issues:

We weren't checking the timestamps when the viewer was leaving a top-level thread
We were checking top-level thread membership of the viewer, which doesn't make sense because it is guaranteed
We weren't removing non-viewer users from sidebars while leaving their parent thread

https://linear.app/comm/issue/ENG-9452/remove-user-from-sidebars-when-they-leave-a-parent-thread

Test Plan

Create a thick thread and a sidebar with two users. Leave the parent thread as one user and verify that the user was removed from both threads.
Create a thick thread and a sidebar with two users. Log in as the same user on two devices. Open the sidebar on one device and leave the parent thread on the other. An alert about chat invalidation should appear and both threads shouldn't be visible in the UI.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Oct 4 2024, 9:12 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 4 2024, 9:12 AM

Harbormaster completed remote builds in B32076: Diff 44908.Oct 4 2024, 9:31 AM

tomek requested review of this revision.Oct 4 2024, 9:31 AM

Nit: no reason to return $ReadOnlyArray from createLeaveSubthreadsUpdates

When a function is constructing a new collection and returning it, I think it's usually best to return that as a mutable collection, so that the caller can do whatever they wish with it

lib/shared/dm-ops/leave-thread-spec.js
65 ↗	(On Diff #44908)
225–227 ↗	(On Diff #44908)

inka added inline comments.Oct 8 2024, 1:44 AM

lib/shared/dm-ops/leave-thread-spec.js
68 ↗	(On Diff #44908)	Is this needed for flow?
144–151 ↗	(On Diff #44908)	Can we move this check higher, before calculating `memberTimestamps`? Why don't we have to leave the sidebars when `viewerID === editorID`? Doesn't the comment from line 229 apply here as well?

tomek added inline comments.Oct 8 2024, 2:39 AM

lib/shared/dm-ops/leave-thread-spec.js
68 ↗	(On Diff #44908)	It is needed in general because only thick threads have the `timestamps` property and `threadInfos` contain both thin and thick threads.
144–151 ↗	(On Diff #44908)	Can we move this check higher, before calculating memberTimestamps? The logic is quite different. When `viewerID === editorID` and the `isMember` is newer than `time` we don't want to delete / leave the sidebars, because it isn't guaranteed we can do this safely. When `viewerID === editorID` we delete both a parent thread and sidebars which results in forgetting about the timestamps. When `viewerID !== editorID` we're removing members and keep their timestamps. Why don't we have to leave the sidebars when viewerID === editorID? Doesn't the comment from line 229 apply here as well? When `viewerID === editorID` we delete the sidebars. This means that we can't rely on the timestamps and can't handle e.g. a case when someone rejoined a sidebar. It is safer to keep the current approach.

tomek added inline comments.Oct 8 2024, 3:45 AM

lib/shared/dm-ops/leave-thread-spec.js
144–151 ↗	(On Diff #44908)	Discussed this with @inka and we figured out the proper solution to this problem. When `viewerID === editorID` and the `isMember` is newer than `time` we should delete all the messages older than `time` and all the sidebars created from them. Handling this requires some effort - created a follow-up task https://linear.app/comm/issue/ENG-9554/remove-a-subset-of-messages-when-leaving-a-thread to track. In the current approach, we keep the messages, which means that we also shouldn't delete the sidebars created from them. It would be weird for the sidebars to disappear for messages that are still present in the UI. An approach where we leave the sidebars (after a viewer leaving a parent thread operation), instead of deleting them is also far from ideal. If all the operations were received in the correct order, we could split all the sidebars into two sets: sidebars that aren't affected (their source messages were created after the leave operation was created) and sidebars that are deleted from the store (their source messages were created before the leave operation was created). There shouldn't be any sidebar where the viewer becomes a non-member. Our timestamps are a solution to the out-of-sequence operations problem, and try to better replicate the eventually correct state and thus shouldn't result in a state that was simply impossible if all the operations were received in the correct order. The conclusion is that we should keep the current solution and improve it as a part of the follow-up task.