Details

Reviewers

tomek
marcin
inka

Commits

rCOMM71c956344d9f: [lib] DMOperationSpec for CREATE_THREAD operation

Summary

Handles creating all ThickThreadTypes, including sidebars.

The RawThreadInfo construction was loosely inspired by createPendingThread, but it's different because pending threads are intentionally constructed to have limited permissions, so eg. the user can't go the settings of a pending thread.

Depends on D12657

Test Plan

I haven't tested this stack outside of Flow. I'd like to propose that we land this so the team can iterate. I'll make sure to test all of the functionality once it's integrated into the rest of the codebase.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Jul 3 2024, 7:58 PM

ashoat added inline comments.Jul 3 2024, 8:09 PM

lib/shared/dm-ops/create-thread-spec.js
51–64 ↗	(On Diff #41979)	This part modelled after `createPendingThread`
66 ↗	(On Diff #41979)	Cast as `{ ...ThickRawThreadInfo }` to get rid of read-only
109 ↗	(On Diff #41979)	The only `await` necessary. I think we will need to query SQLite here
110–114 ↗	(On Diff #41979)	The caller will have a `try-catch` around calls around `processDMOperation`. If we fail to process a `DMOperation`, I think we should probably just print an error to the console and otherwise ignore it
119–158 ↗	(On Diff #41979)	This part modelled after `thread-creator.js` on the keyserver

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2024, 8:11 PM

Harbormaster failed remote builds in B30108: Diff 41979!

Rebase

ashoat added inline comments.Jul 3 2024, 8:13 PM

lib/shared/dm-ops/create-thread-spec.js
51–64 ↗	(On Diff #41985)	This part modelled after `createPendingThread`
66 ↗	(On Diff #41985)	Cast as `{ ...ThickRawThreadInfo }` to get rid of read-only
109 ↗	(On Diff #41985)	The only `await` necessary. I think we will need to query SQLite here
110–114 ↗	(On Diff #41985)	The caller will have a `try-catch` around calls around `processDMOperation`. If we fail to process a `DMOperation`, I think we should probably just print an error to the console and otherwise ignore it
119–158 ↗	(On Diff #41985)	This part modelled after `thread-creator.js` on the keyserver

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2024, 8:32 PM

Harbormaster failed remote builds in B30114: Diff 41985!

Rebase

Harbormaster completed remote builds in B30118: Diff 41989.Jul 3 2024, 9:06 PM

ashoat requested review of this revision.Jul 3 2024, 9:06 PM

ashoat added inline comments.Jul 4 2024, 5:45 AM

lib/shared/dm-ops/create-thread-spec.js
51–64	This part modelled after `createPendingThread`
66	Cast as `{ ...ThickRawThreadInfo }` to get rid of read-only
109	The only `await` necessary. I think we will need to query SQLite here
110–114	The caller will have a `try-catch` around calls around `processDMOperation`. If we fail to process a `DMOperation`, I think we should probably just print an error to the console and otherwise ignore it
119–158	This part modelled after `thread-creator.js` on the keyserver

ashoat added inline comments.Jul 4 2024, 6:46 AM

lib/shared/dm-ops/create-thread-spec.js
133	I should actually update this. I initially was thinking that it's not important because only receiving clients will process this, and it looks like `sourceMessageAuthorID` is only used for notifs. But now I'm thining the sending client will probably call `processDMOperation` to generate the `RawMessageInfo`s that are stored locally, and will use the result to generate notifs. We have access to the `sourceMessage` above, so it should be simple to set this correctly
162	These `id`s are only used for `UpdateInfo`s on the keyserver side... they don't matter at all on the client side. At some point we probably should have changed the types to remove them, and I suppose we should still do that now... but it's tough to prioritize it given our launch schedule Created ENG-8713 as a follow-up

Actually pass sourceMessageAuthorID in based on sourceMessage
Decided to use a UUID instead of 'ignored'. I'm pretty sure it's not used on the client anywhere, but figured it's better to avoid any risks in case it affects anything, given our tight timeline for launch

ashoat added inline comments.Jul 4 2024, 11:12 AM

lib/shared/dm-ops/create-thread-spec.js
52–65 ↗	(On Diff #42057)	This part modelled after `createPendingThread`
67 ↗	(On Diff #42057)	Cast as `{ ...ThickRawThreadInfo }` to get rid of read-only
110 ↗	(On Diff #42057)	The only `await` necessary. I think we will need to query SQLite here
111–115 ↗	(On Diff #42057)	The caller will have a `try-catch` around calls around `processDMOperation`. If we fail to process a `DMOperation`, I think we should probably just print an error to the console and otherwise ignore it
120–159 ↗	(On Diff #42057)	This part modelled after `thread-creator.js` on the keyserver

Harbormaster completed remote builds in B30165: Diff 42057.Jul 4 2024, 11:56 AM

tomek accepted this revision.Jul 5 2024, 5:09 AM

tomek added inline comments.

lib/shared/dm-ops/create-thread-spec.js
80 ↗	(On Diff #42057)	`allMemberIDs` contains `creatorID` for whom we should probably set `isSender` to true.
91 ↗	(On Diff #42057)	Is it possible that this condition is true?
95–109 ↗	(On Diff #42057)	Shouldn't we merge these two `if`s?
122 ↗	(On Diff #42057)	Are we relying somewhere on this ID schema? I think that using a deterministic ID could cause some issues, e.g. when we create a pending DM thread, add a new user, and then create a new DM thread with the original set of users. I'm not sure if `threadID` could be a "pending thread ID" - but if yes, then the described scenario will cause a conflict between message IDs. Can we use UUIDs instead for the role and all the created messages?

This revision is now accepted and ready to land.Jul 5 2024, 5:09 AM

ashoat added inline comments.Jul 5 2024, 8:31 AM

lib/shared/dm-ops/create-thread-spec.js
80 ↗	(On Diff #42057)	Good catch
91 ↗	(On Diff #42057)	Yes, I think that we'll call `processDMOperation` on all recipients – both the devices of the user, and the devices of their peers
95–109 ↗	(On Diff #42057)	Will merge
122 ↗	(On Diff #42057)	Are we relying somewhere on this ID schema? I think that using a deterministic ID could cause some issues, e.g. when we create a pending DM thread, add a new user, and then create a new DM thread with the original set of users. I'm not sure if `threadID` could be a "pending thread ID" - but if yes, then the described scenario will cause a conflict between message IDs. In this case, `threadID` should be a non-pending `threadID`. So I don't think that hypothetical could occur. Can we use UUIDs instead for the role and all the created messages? I suppose this would be safer, but it would require extending `DMCreateThreadOperation` to include UUIDs for all of these, which might be a little messy.