Page MenuHomePhabricator

[lib/native/keyserver] included RawReactionMessageInfo to RawMessageInfo and ReactionMessageInfo to MessageInfo
ClosedPublic

Authored by ginsu on Dec 2 2022, 12:24 PM.
Tags
None
Referenced Files
F3552397: D5802.diff
Thu, Dec 26, 8:07 PM
Unknown Object (File)
Sun, Dec 15, 6:22 AM
Unknown Object (File)
Sun, Dec 15, 6:22 AM
Unknown Object (File)
Sun, Dec 15, 6:22 AM
Unknown Object (File)
Sun, Dec 15, 6:22 AM
Unknown Object (File)
Sun, Dec 15, 6:19 AM
Unknown Object (File)
Thu, Nov 28, 11:52 PM
Unknown Object (File)
Thu, Nov 28, 11:52 PM
Subscribers

Details

Summary

included RawReactionMessageInfo to RawMessageInfo and ReactionMessageInfo to MessageInfo. We need to do this because unshimMessageInfo uses RawMessageInfo for its type and MessageInfo type is used extensively in chat selectors.


Linear Task: ENG-2244

Test Plan

ran flow in lib, native, and keyserver directories and nothing crashes locally on native/web

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/selectors/chat-selectors.js
312–314 ↗(On Diff #19112)

A subsequent diff will handle what happens if a message is a reaction type and do all the merging magic, but for now, we just want to filter out all the reaction messages from this function

ginsu added reviewers: atul, tomek, rohan.
ginsu edited the test plan for this revision. (Show Details)
ginsu requested review of this revision.Dec 2 2022, 12:36 PM
tomek requested changes to this revision.Dec 6 2022, 5:53 AM
tomek added inline comments.
keyserver/src/creators/thread-creator.js
195–198 ↗(On Diff #19112)

I don't think we should use invariant. We should stay consistent with other places in the code and throw ServerError when validation fails.

keyserver/src/fetchers/message-fetchers.js
683–690 ↗(On Diff #19112)

It might make sense to merge these invariants. Moreover it might be a good idea to define a set of message types that can't be a sidebar source and use it in all the places.

This revision now requires changes to proceed.Dec 6 2022, 5:53 AM

addressed tomek's feedback

keyserver/src/fetchers/message-fetchers.js
683–690 ↗(On Diff #19112)

Attempted to do this but was getting some weird flow issues. I merged these two invariant statements for now and will circle back to this later. Created a linear task to track

tomek added inline comments.
keyserver/src/creators/thread-creator.js
195 ↗(On Diff #19195)

It probably can be simplified

lib/shared/messages/sidebar-source-message-spec.js
110–117 ↗(On Diff #19195)

When addressing a comment please check if there are other instances where the change could be also applied. This is really similar to the other place where invariants got merged.

keyserver/src/creators/thread-creator.js
195 ↗(On Diff #19195)

When I tried this, flow was giving me some errors about how there were missing fields in the sourceMessage

Screenshot 2022-12-07 at 1.32.49 PM.png (1×3 px, 2 MB)

This revision is now accepted and ready to land.Dec 7 2022, 11:45 AM