Page MenuHomePhabricator

[keyserver/lib] Dedup isMessageSidebarSourceReactionEditOrPin into isInvalidSidebarSource
ClosedPublic

Authored by rohan on Sep 28 2023, 12:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 4:38 PM
Unknown Object (File)
Wed, Nov 20, 4:38 PM
Unknown Object (File)
Wed, Nov 20, 4:38 PM
Unknown Object (File)
Wed, Nov 20, 4:36 PM
Unknown Object (File)
Wed, Nov 20, 4:18 PM
Unknown Object (File)
Sat, Nov 16, 7:26 AM
Unknown Object (File)
Thu, Nov 14, 8:09 PM
Unknown Object (File)
Thu, Nov 14, 5:08 PM
Subscribers

Details

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 edited the test plan for this revision. (Show Details)
rohan added inline comments.
lib/shared/message-utils.test.js
1 ↗(On Diff #31534)

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

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!
rohan requested review of this revision.Sep 28 2023, 1:34 PM

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

Use messageTypes constant

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

lib/shared/message-utils.js
660

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

This revision is now accepted and ready to land.Sep 29 2023, 12:05 PM
rohan edited the summary of this revision. (Show Details)
lib/shared/message-utils.js
660

Missed this comment, but I had to keep it as a predicate to deal with Flow (same reason I mention in D9330)