[lib] Handle a case where viewer is added to a thread while being its member
ClosedPublic
Actions

Authored by tomek on Sep 26 2024, 4:16 AM.

Details

Reviewers

kamil
will
ashoat

Commits

rCOMMa432a967f452: [lib] Handle a case where viewer is added to a thread while being its member

Summary

It is possible that we receive an operation about viewer being added to a thread while the thread is already present in the store and viewer is its member. We need to make sure that we don't override the whole thread and instead update it as little as possible.

https://linear.app/comm/issue/ENG-8720/add-protection-against-thick-thread-creation-overwriting-existing

Depends on D13483

Test Plan

Create a sidebar, add, remove, and add a user and check on that user's device if the thread is displayed and updated correctly.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Sep 26 2024, 4:16 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 26 2024, 4:16 AM

tomek added inline comments.Sep 26 2024, 4:18 AM

lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
183–189 ↗	(On Diff #44599)	The message is already included in `JOIN_THREAD`

Harbormaster completed remote builds in B31881: Diff 44599.Sep 26 2024, 4:33 AM

tomek requested review of this revision.Sep 26 2024, 4:33 AM

ashoat accepted this revision.Sep 26 2024, 8:13 AM

ashoat added inline comments.

lib/shared/dm-ops/add-members-spec.js
58 ↗	(On Diff #44599)	Nit: shorthand
lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
117 ↗	(On Diff #44599)	Unrelated to this diff, but I did some investigation as to why we still have `permissions` for `members` here. The context is that in D12789, we removed member permissions from the SQLite DB on `native` clients. I know Atul never finished it, but I was under the impression that the only thing left was to implement an equivalent migration on `web`. However, looking at the types it seems like the changes were made for thin threads but NOT for thick threads. I'm guessing Atul's work intersected with the thick threads work, and he ended up making the changes only for thin threads. I'm not 100% sure, but I suspect we should probably do the same for thick threads as thin threads (calculate the member permissions "on the fly") instead of storing them in the DB. I created ENG-9404 to track this.