[keyserver/lib] Dedup isMessageSidebarSourceReactionEditOrPin into isInvalidSidebarSource
ClosedPublic
Actions

Authored by rohan on Sep 28 2023, 12:37 PM.

Details

Reviewers

atul
ginsu

Commits

rCOMM5db09ba53c31: [keyserver/lib] Dedup isMessageSidebarSourceReactionEditOrPin into…

Summary

As a first step of separating logic into message-specs to determine whether messages can be sidebar sources / can be pinned / etc, we should first dedup isMessageSidebarSourceReactionEditOrPin into isInvalidSidebarSource since the checks are the same.

Resolves https://linear.app/comm/issue/ENG-5206/dedup-ismessagesidebarsourcereactioneditorpin-into

Test Plan

Ran flow, confirmed using isInvalidSidebarSource in place of isMessageSidebarSourceReactionEditorPin didn't cause any regressions, and just general testing to make sure sidebars (especially with robotext) still works. Also rewrote the unit tests to be more resistant to unintentional changes to message types and ran yarn workspace lib test

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Sep 28 2023, 12:37 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 28 2023, 12:37 PM

rohan edited the summary of this revision. (Show Details)Sep 28 2023, 12:38 PM

rohan edited the test plan for this revision. (Show Details)

rohan added inline comments.

lib/shared/message-utils.test.js
1	At some point we could probably define a shared set of test messages (since @atul did a similar thing in `message-ops-utils.test.js`), but I didn't want to spend a ton more time on this since the rest of the stack is a priority right now

rohan added a child revision: D9323: [keyserver/lib] Move isInvalidSidebarSource from message-utils to message-types.Sep 28 2023, 12:42 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 28 2023, 12:44 PM

Harbormaster failed remote builds in B22903: Diff 31534!

I'll rerun CI when it's back up, requesting review in the meantime

Harbormaster completed remote builds in B22903: Diff 31534.Sep 28 2023, 5:51 PM

Use messageTypes constant

Harbormaster completed remote builds in B22916: Diff 31550.Sep 29 2023, 11:21 AM

looks good, skimmed through the tests file (trusting your test plan here), but please see one inline comment

lib/shared/message-utils.js
660 ↗	(On Diff #31550)	Since this check generally results in something not happening (returning false/triggering an invariant) it seems like having this be a predicate function might not be super necessary, and we should instead opt to have something easier to read/understand