[lib] Extract the unsupported sidebar source message types to one single source of truth
ClosedPublic
Actions

Authored by rohan on Sep 19 2023, 11:01 PM.

Details

Reviewers

ashoat
atul
ginsu

Commits

rCOMM9e8f0491295e: [lib] Extract the unsupported sidebar source message types to one single source…

Summary

As mentioned in the previous diff, we want a single source of truth for a list of message types that should disallow sidebar creation. This diff moves the check into a helper function in
message-utils.js, and adds a quick unit test alongside this to check the integrity of the method.

Depends on D9237

Addresses part of ENG-4849

Test Plan

Ran flow, confirmed that the test plan in D9237 still works, and ran yarn workspace lib test to see the unit tests passing

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Sep 19 2023, 11:01 PM

Herald added a subscriber: tomek. · View Herald TranscriptSep 19 2023, 11:01 PM

rohan added a child revision: D9239: [keyserver] Prevent non-composable message types from being pinned on the keyserver.Sep 19 2023, 11:04 PM

rohan added inline comments.Sep 19 2023, 11:05 PM

keyserver/src/creators/thread-creator.js
422–429 ↗	(On Diff #31293)	I'm actually not certain on if this is the right invariant, it seems to be doing exactly what the helper method does and may be redundant. I'm going to take another look at this tomorrow

Harbormaster completed remote builds in B22750: Diff 31293.Sep 19 2023, 11:18 PM

rohan requested review of this revision.Sep 19 2023, 11:18 PM

Accepting to unblock; we can dedup everything and figure out the types later. This Linear comment will hopefully be helpful

keyserver/src/creators/thread-creator.js
422–429 ↗	(On Diff #31293)	I think you only need to `invariant` on `messageTypes.SIDEBAR_SOURCE` right now, but that's because this type only excludes `SIDEBAR_SOURCE`, when arguably it should exclude all of them Making sure that the lists match up can be handled later. Leaving this `invariant` here is probably fine, but it would be good to update the types as well
lib/shared/message-utils.js
645 ↗	(On Diff #31293)	Same comment as here... feels like we can use `isMessageSidebarSourceReactionEditOrPin`
lib/shared/message-utils.test.js
7 ↗	(On Diff #31293)	Love the tests!