[lib] Simplify thread settings types
ClosedPublic
Actions

Authored by tomek on Apr 28 2025, 2:09 PM.

Details

Reviewers

kamil
angelika

Commits

rCOMMb8bbd3b9479e: [lib] Simplify thread settings types

Summary

In D14599 the types were modified so that they are consistent, but we couldn't simplify it further. This diff addresses that, but with a tradeoff: now the DM type can contain newMemberIDs that isn't supported and is checked in the runtime. This isn't ideal, but overall doesn't break the protocol contract - it is expected that some protocols won't support all the possible features. We can consider making this more formal, by e.g. introducing a flag, but I'm not sure if we need to.

Depends on D14609

Test Plan

Check if changing name and description of thin and thick thread works.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Apr 28 2025, 2:09 PM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 28 2025, 2:10 PM

Harbormaster completed remote builds in B34035: Diff 47882.Apr 28 2025, 2:42 PM

tomek requested review of this revision.Apr 28 2025, 2:42 PM

tomek added a child revision: D14612: [lib] Extract common logic from reaction hooks.Apr 28 2025, 3:20 PM

angelika accepted this revision.Apr 29 2025, 11:16 AM

angelika added inline comments.

lib/shared/messages/text-message-spec.js
328–329 ↗	(On Diff #47882)	is `threadID` necessary if we pass `threadInfo`? maybe we can simplify it more?

This revision is now accepted and ready to land.Apr 29 2025, 11:16 AM

tomek added inline comments.May 7 2025, 4:54 PM

lib/shared/messages/text-message-spec.js
328–329 ↗	(On Diff #47882)	I was considering that, and it would simplify things, but not that much, and the solution would be less safe. In the current solution, we define a type `UseChangeThreadSettingsInput` that spreads `UpdateThreadRequest` which includes `threadID` and adds `threadInfo` to it. `UpdateThreadRequest` can't be changed easily, because it describes a type that a keyserver requires - definitely isn't worth modifying. We could modify `UseChangeThreadSettingsInput` to stop spreading `UpdateThreadRequest` and instead define all the properties explicitly. It makes some sense, but I think it would be less safe because someone could add a property to `UpdateThreadRequest` without adding it to `UseChangeThreadSettingsInput` - it shouldn't cause big issues though... but overall, I'm not sure if this change would be that beneficial.