Details

Reviewers

ashoat
ginsu
tomek
varun

Commits

rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`

Summary

We're no longer using member permissions anywhere on client (or anywhere ThreadInfo is used which includes eg send.js), so it can be removed.

NOTE: Marked this as DRAFT because I think we can do better than the invariant in memberHasAdminRole, but I think it would require some flow refactoring which I want to defer for now until I get rest of migration stack up.

Depends on D12259

Test Plan

flow

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Jun 1 2024, 9:21 PM

atul added inline comments.Jun 1 2024, 9:32 PM

lib/types/minimally-encoded-thread-permissions-types.js
134–141 ↗	(On Diff #40822)	Rather than deriving this from `MemberInfo` which is then derived from `LegacyMemberInfo`, I think it requires less jumping between files to just have it all inline like this. Open to different approach if it would be preferred.

Harbormaster completed remote builds in B29342: Diff 40822.Jun 1 2024, 9:38 PM

atul requested review of this revision.Jun 1 2024, 9:38 PM

atul added a child revision: D12266: [lib] Narrow `threadInfo` argument of `threadFrozenDueToViewerBlock`.Jun 1 2024, 10:00 PM

atul planned changes to this revision.Jun 1 2024, 10:07 PM

Agree we can do better than the invariant. It seems like memberHasAdminPowers should not be called with RelativeMemberInfo anymore, since permissions is missing there. Are there current callsites that pass RelativeMemberInfo into memberHasAdminPowers?

lib/types/minimally-encoded-thread-permissions-types.js
134–141 ↗	(On Diff #40822)	I don't have a strong opinion

atul added a child revision: D12287: [lib] Introduce `useThreadFrozenDueToViewersBlock`.Jun 3 2024, 12:40 PM

atul mentioned this in D12292: [lib] Narrow types in `memberHasAdminPowers` call stack to remove `invariant`.Jun 3 2024, 1:43 PM

atul added inline comments.Jun 3 2024, 2:24 PM

lib/permissions/minimally-encoded-thread-permissions-validators.js
17–26 ↗	(On Diff #40822)	NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

Agree we can do better than the invariant.

invariant gets removed in D12292

Please make sure to consider your "note to self" above before landing:

NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

lib/types/minimally-encoded-thread-permissions-types.js
134 ↗	(On Diff #40822)	Now that we no longer have a type spread, we can remove the `$ReadOnly` wrapper

This revision is now accepted and ready to land.Jun 12 2024, 8:49 AM

rebase BEFORE addressing feedback and landing

Harbormaster completed remote builds in B29635: Diff 41258.Jun 12 2024, 3:42 PM

address feedback (remove $ReadOnly wrapper)

Harbormaster completed remote builds in B29694: Diff 41347.Jun 15 2024, 2:43 PM

remove relativeMemberInfoValidator export

Harbormaster completed remote builds in B29695: Diff 41348.Jun 15 2024, 3:30 PM

NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

There is one use of relativeMemberInfoValidator and it's in threadInfoValidator.

threadInfoValidator is used in

thread-utils.test.js: threadInfoFromRawThreadInfo should return correctly formed ThreadInfo from RawThreadInfo test where things remain working as expected.
webNavInfoValidator

webNavInfoValidator is only consumed by initialReduxStateValidator which validates ServerWebInitialReduxStateResponse.

webNavInfoValidator is of type TInterface<WebNavInfo> and threadInfoValidator is consumed within to validate pendingThread:

...
pendingThread: t.maybe(threadInfoValidator),
...

My understanding is the WebNavInfo.pendingThread is a client concept and can only exist for the client representation of WebNavInfo and can't be included in response from keyserver since they aren't persisted or stored anywhere.

To double check this, looked at getInitialReduxStateResponder which calls navInfoFromURL which takes in urlInfo and backupInfo and returns WebNavInfo. Nowhere in navInfoFromURL is a pendingThread field included:

As a result, I think this diff is safe to land. Please let me know if there's something I'm missing/misunderstanding and I will revert.

Closed by commit rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions` (authored by atul). · Explain WhyJun 15 2024, 6:17 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`.

atul mentioned this in rCOMM509ef589a60d: [lib] Narrow types in `memberHasAdminPowers` call stack to remove `invariant`.Jun 16 2024, 5:50 PM

[lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41349

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

lib/selectors/user-selectors.js

lib/shared/thread-utils.js

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

[lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41349

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

lib/selectors/user-selectors.js

lib/shared/thread-utils.js

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

[lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`
ClosedPublic
Actions

Revision Contents
Changeset List