Details

Reviewers

Commits

rCOMM7f594b7fdd62: [Flow262][skip-ci] Fix thread infos validators

Summary

https://linear.app/comm/issue/ENG-10205/update-flow-to-02691

Deleted ThinRawThreadInfoWithoutSpecialRole because it seems to be no longer needed and it fixes a lot of issues with thread validators. It was introduced in this diff: https://phab.comm.dev/D11496 and from what I've read it was introduced because there was a change in RolesInfo. Old clients expected RoleInfo with isDefault and new clients expected RoleInfo with specialRole. That's why the keyserver sends role infos with isDefault for old clients: https://github.com/CommE2E/comm/blob/master/lib/shared/thread-utils.js#L822. shouldIncludeSpecialRoleFieldInRoles is true from version:

native: 336,
web: 79,

and we're no longer supporting those versions. Also we don't use those types and validators anywhere anymore, so it should be ok to delete them.

As for rawThreadInfos in StateChanges I changed validator to conform to types and it's broader now so there shouldn't be any issues with types.

Depends on D15065

Test Plan

Flow

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

angelika created this revision.Jul 23 2025, 9:13 AM

angelika held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 23 2025, 9:13 AM

angelika added a child revision: D15067: [Flow262][skip-ci] Fix policy validators.Jul 23 2025, 9:15 AM

Harbormaster completed remote builds in B34722: Diff 48926.Jul 23 2025, 9:17 AM

Rebase

Harbormaster completed remote builds in B34911: Diff 49118.Jul 24 2025, 4:27 PM

angelika published this revision for review.Jul 25 2025, 9:46 AM

angelika added a reviewer: tomek.

This is tricky, because indeed, keyservers can only accept and send thin thread infos. So the current behavior is correct - the validators shouldn't accept thick threads from keyservers. As a workaround, it is ok... but it would be better to find a solution where thick threads aren't accepted.

This revision now requires changes to proceed.Jul 31 2025, 2:18 PM

In D15066#416241, @tomek wrote:

This is tricky, because indeed, keyservers can only accept and send thin thread infos. So the current behavior is correct - the validators shouldn't accept thick threads from keyservers. As a workaround, it is ok... but it would be better to find a solution where thick threads aren't accepted.

I can see only two ways to solve this:

make validators accept thick threads (this diff)
keep validators as they are and change flow types

I tried changing flow types and that's not easy. Let's see ThreadUpdateInfo:

flow type of threadInfo is: LegacyRawThreadInfo | RawThreadInfo
according to validator threadInfo should be:

| LegacyThinRawThreadInfo
| ThinRawThreadInfo
| ThinRawThreadInfoWithoutSpecialRole

after changing the type I've got a bunch of flow errors
for example see createLeaveSubthreadsUpdates function https://github.com/CommE2E/comm/blob/master/lib/shared/dm-ops/leave-thread-spec.js#L60-L108
threadInfos in function parameters is ThickRawThreadInfos, so thread variable is ThickRawThreadInfo
updatedThread is created from thread so it has properties from ThickRawThreadInfo and then it's pushed into updates array which contains ClientUpdateInfo which is an union of ThreadUpdateInfo where we changed threadInfo to contain only thin threads

so it turns out those types like ThreadUpdateInfo can also contain thick threads infos...

Ok, makes sense - both options don't seem nice. But can we check a third option - from my inline comment?

lib/permissions/minimally-encoded-raw-thread-info-validators.js
143 ↗	(On Diff #49118)	Maybe rename it to `mixedThreadInfoValidator`?
lib/types/socket-types.js
227 ↗	(On Diff #49118)	Wondering if it is possible to have a validator that uses thin, thick, and legacy threads as its type parameter, but accepts only the thin threads. It should be, and if we use it, it will satisfy the type system and won't compromise our validation. Could you try this approach?

Rebase & review changes

Harbormaster completed remote builds in B35177: Diff 49428.Aug 9 2025, 12:29 PM

angelika added inline comments.Aug 9 2025, 1:07 PM

lib/permissions/minimally-encoded-raw-thread-info-validators.js
143 ↗	(On Diff #49118)	Renamed
lib/types/socket-types.js
227 ↗	(On Diff #49118)	Correct me if I'm wrong but do you want me to do something like this? const mixedThreadInfoValidator: TUnion< UnionOfThinThickLegacyThreads, > = t.union([ unionOfOnlyThinThreads ]); ? Because you can't do this, flow checks if validators match whatever is in the type parameters.

tomek requested changes to this revision.Aug 11 2025, 2:28 PM

tomek added inline comments.

lib/types/socket-types.js
227 ↗	(On Diff #49118)	Yeah, I know it does it to some extent, but maybe it's worth figuring something out. Maybe if we don't type it as a union, it could work? Could you experiment a bit with it? High level, it sounds reasonable that a validator for type `T` accepts only a subset of possible `T` values - that's what validators do. So if we had a validator checking `T \| U` and decide to accept all the `T`, it looks reasonable, at least from the principle.

This revision now requires changes to proceed.Aug 11 2025, 2:28 PM

Remove ThinRawThreadInfoWithoutSpecialRole

Harbormaster completed remote builds in B35299: Diff 49572.Aug 18 2025, 1:14 PM

Deleted ThinRawThreadInfoWithoutSpecialRole because it seems to be no longer needed and it fixes a lot of issues with thread validators. It was introduced in this diff: https://phab.comm.dev/D11496 and from what I've read it was introduced because there was a change in RolesInfo. Old clients expected RoleInfo with isDefault and new clients expected RoleInfo with specialRole. That's why the keyserver sends role infos with isDefault for old clients: https://github.com/CommE2E/comm/blob/master/lib/shared/thread-utils.js#L822. shouldIncludeSpecialRoleFieldInRoles is true from version:

native: 336,
web: 79,

and we're no longer supporting those versions. Also we don't use those types and validators anywhere anymore, so it should be ok to delete them.

angelika edited the summary of this revision. (Show Details)Aug 18 2025, 1:18 PM

angelika edited the summary of this revision. (Show Details)Aug 18 2025, 1:38 PM

I think it also shouldn't affect older clients migrating to a new version - the migration code shouldn't use the validators.

This revision is now accepted and ready to land.Aug 18 2025, 2:02 PM

Rebase & review changes

Harbormaster completed remote builds in B35443: Diff 49732.Aug 21 2025, 12:51 PM

Closed by commit rCOMM7f594b7fdd62: [Flow262][skip-ci] Fix thread infos validators. · Explain WhyAug 21 2025, 7:33 PM

This revision was automatically updated to reflect the committed changes.

angelika added a commit: rCOMM7f594b7fdd62: [Flow262][skip-ci] Fix thread infos validators.

[Flow262][skip-ci] Fix thread infos validators
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49910

lib/permissions/minimally-encoded-raw-thread-info-validators.js

lib/types/minimally-encoded-thread-permissions-types.js

lib/types/request-types.js

lib/types/thread-types.js

lib/types/validators/redux-state-validators.js

[Flow262][skip-ci] Fix thread infos validatorsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49910

lib/permissions/minimally-encoded-raw-thread-info-validators.js

lib/types/minimally-encoded-thread-permissions-types.js

lib/types/request-types.js

lib/types/thread-types.js

lib/types/validators/redux-state-validators.js

[Flow262][skip-ci] Fix thread infos validators
ClosedPublic
Actions

Revision Contents
Changeset List