Page MenuHomePhabricator
Differential D5749

[keyserver] introduced reactionMessageCreationResponder function to message responders
ClosedPublic
Actions

Authored by ginsu on Nov 28 2022, 4:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
Unknown Object (File)
Mon, Mar 17, 6:26 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
rohan
tomek
ashoat
Commits
rCOMM7a552df95c20: [keyserver] introduced reactionMessageCreationResponder function to message…
Summary

introduced reactionMessageCreationResponder function to message responders. reactionMessageCreationResponder validates the input from the client, does the permission checks, and if everything is copacetic creates the reaction message data object, and sends it to createMessages where it gets pushed into the db

Depends on D5747 and D5752

Linear Task: ENG-2250

Test Plan

This will be tested further in subsequent diffs. But ran this locally on web/mobile and nothing crashed. Also helped me get to the point where I could push a reaction message into the DB (see screenshot below)

Screenshot 2022-11-28 at 7.16.55 PM.png (2×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Nov 28 2022, 4:31 PM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptNov 28 2022, 4:31 PM
ginsu edited the summary of this revision. (Show Details)Nov 28 2022, 4:38 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, rohan, tomek.
ginsu added parent revisions: D5748: [keyserver] introduce sendReactionMessageRequestInputValidator to message responders, D5747: [keyserver] modified createMessages function to handle inserting targetMessageID into the DB.
ginsu attached a referenced file: F263988: Screenshot 2022-11-28 at 7.16.55 PM.png. (Show Details)
Harbormaster completed remote builds in B13812: Diff 18904.Nov 28 2022, 4:45 PM
ginsu requested review of this revision.Nov 28 2022, 4:45 PM
ashoat requested changes to this revision.Nov 28 2022, 7:02 PM
ashoat added inline comments.
keyserver/src/responders/message-responders.js
12 ↗(On Diff #18904)

Can't find where this type is in master or any of the diffs in the stack

202 ↗(On Diff #18904)

Great work checking this

This revision now requires changes to proceed.Nov 28 2022, 7:02 PM
ginsu requested review of this revision.Nov 29 2022, 7:01 AM
ginsu added inline comments.
keyserver/src/responders/message-responders.js
12 ↗(On Diff #18904)

My bad thought I included the diff where I introduced SendReactionMessageRequest for review yesterday but here it is:

D5752

ginsu updated this revision to Diff 18962.Nov 29 2022, 7:52 AM

added sendReactionMessageRequestInputValidator to this diff

ginsu added inline comments.Nov 29 2022, 7:53 AM
keyserver/src/responders/message-responders.js
12 ↗(On Diff #18962)

D5752

ginsu edited the summary of this revision. (Show Details)Nov 29 2022, 7:53 AM
ginsu mentioned this in D5748: [keyserver] introduce sendReactionMessageRequestInputValidator to message responders.
Harbormaster completed remote builds in B13844: Diff 18962.Nov 29 2022, 8:06 AM
ginsu edited the summary of this revision. (Show Details)Nov 29 2022, 11:47 AM
ginsu added a parent revision: D5752: [lib] introduce SendReactionMessageRequest type to message types.
ashoat accepted this revision.Nov 30 2022, 7:00 AM
tomek accepted this revision.Nov 30 2022, 10:14 AM
tomek added inline comments.
keyserver/src/responders/message-responders.js
187 ↗(On Diff #18962)

Should we check if this is actually the only supported reaction?

187–198 ↗(On Diff #18962)

Can we make reaction mandatory in sendReactionMessageRequestInputValidator instead of checking if it's undefined later?

217 ↗(On Diff #18962)

Are there other constraints on reaction that can be verified?

atul accepted this revision.Nov 30 2022, 2:21 PM

Looks good!

(make sure to respond to @tomek's questions before landing)

This revision is now accepted and ready to land.Nov 30 2022, 2:21 PM
ginsu added a child revision: D5750: [lib/keyserver] included reactionMessageCreationResponder to the JSON endpoints.Nov 30 2022, 3:50 PM
ginsu updated this revision to Diff 19117.Dec 2 2022, 2:33 PM

rebase with unsettingReaction field

ginsu updated this revision to Diff 19118.Dec 2 2022, 2:38 PM

updated parameter check

ginsu added inline comments.Dec 2 2022, 2:43 PM
keyserver/src/responders/message-responders.js
187 ↗(On Diff #18962)

@atul and I talked about making a set of supported reactions and using that set as an extra check. I was thinking of doing that in a subsequent diff, but if you prefer me to do that in this diff let me know!

187–198 ↗(On Diff #18962)

The new unsettingReaction field should address this, but please let me know if I missed something!

217 ↗(On Diff #18962)

@atul and I talked about making a set of supported reactions and using that set as an extra check. I was thinking of doing that in a subsequent diff, but if you prefer me to do that in this diff let me know!

Harbormaster completed remote builds in B13951: Diff 19117.Dec 2 2022, 2:44 PM
Harbormaster completed remote builds in B13952: Diff 19118.Dec 2 2022, 2:50 PM
ginsu updated this revision to Diff 19123.Dec 2 2022, 4:12 PM

fix small bug

Harbormaster completed remote builds in B13957: Diff 19123.Dec 2 2022, 4:25 PM
ginsu updated this revision to Diff 19144.Dec 5 2022, 1:27 PM

rebase with action field

Harbormaster completed remote builds in B13971: Diff 19144.Dec 5 2022, 1:39 PM
ginsu updated this revision to Diff 19749.Dec 19 2022, 5:01 PM

fixed inconsitency in privacy/blocking logic

ginsu requested review of this revision.Dec 19 2022, 5:03 PM
ginsu mentioned this in D5853: [native] introduced useCanCreateReactionFromMessage function to reaction message utils.

Rerequesting review because made some major changes with the creatorRelationshipHasBlock check in this responder

Harbormaster completed remote builds in B14493: Diff 19749.Dec 19 2022, 5:12 PM
ashoat accepted this revision.Dec 20 2022, 5:43 AM
ashoat added 1 blocking reviewer(s): atul.
ashoat changed 2 blocking reviewer(s), added 1: tomek; removed 1: atul.
tomek accepted this revision.Dec 20 2022, 9:20 AM

Looks good! A couple of small issues inline - feel free to request a review if some of them require significant changes.

keyserver/src/responders/message-responders.js
190 ↗(On Diff #19749)
202–204 ↗(On Diff #19749)

Wouldn't the validator catch !action? We're setting t.enums.of(['add_reaction', 'remove_reaction']) so it shouldn't be empty here.

212–216 ↗(On Diff #19749)

It is confusing to see a result of fetchKnownUserInfos being assigned to a singular variable and then accessed as an object.

222–226 ↗(On Diff #19749)

It seems like this and fetchKnownUserInfos could be awaited at the same time.

187 ↗(On Diff #18962)

I think at this point it is a matter of only a couple of chars changed, so it's worth doing it now.

This revision is now accepted and ready to land.Dec 20 2022, 9:20 AM
ginsu updated this revision to Diff 19792.Dec 20 2022, 10:34 AM

address tomek's feedback

Harbormaster completed remote builds in B14535: Diff 19792.Dec 20 2022, 10:45 AM
ginsu updated this revision to Diff 19896.Dec 20 2022, 3:24 PM

rebase before landing

Harbormaster completed remote builds in B14590: Diff 19896.Dec 20 2022, 3:37 PM
Closed by commit rCOMM7a552df95c20: [keyserver] introduced reactionMessageCreationResponder function to message… (authored by ginsu). · Explain WhyDec 20 2022, 4:02 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM7a552df95c20: [keyserver] introduced reactionMessageCreationResponder function to message….

Revision Contents
Changeset List

PathSize
keyserver/
src/
responders/
46 lines

Diff 19144

View Options

keyserver/src/responders/message-responders.js

// @flow // @flow
import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native'; import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { View } from 'react-native'; import { View, Text } from 'react-native';
import genesis from 'lib/facts/genesis.js'; import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js'; import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import { import {
userInfoSelectorForPotentialMembers, userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers, userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js'; } from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js'; import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights; this.pendingListDataWithHeights = listDataWithHeights;
} else { } else {
this.setState({ listDataWithHeights }); this.setState({ listDataWithHeights });
} }
}; };
} }
const unboundStyles = { const unboundStyles = {
pinnedCountBanner: {
backgroundColor: 'panelForeground',
height: 30,
flexDirection: 'row',
textAlign: 'center',
justifyContent: 'center',
alignItems: 'center',
},
pinnedCountText: {
color: 'panelBackgroundLabel',
marginRight: 5,
},
container: { container: {
backgroundColor: 'listBackground', backgroundColor: 'listBackground',
flex: 1, flex: 1,
}, },
threadContent: { threadContent: {
flex: 1, flex: 1,
}, },
hiddenThreadContent: { hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines ) {
const styles = useStyles(unboundStyles); const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext); const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer(); const measureMessages = useHeightMeasurer();
const genesisThreadInfo = useSelector( const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id], state => threadInfoSelector(state)[genesis.id],
); );
const bannerText = React.useMemo(() => {
if (!threadInfo.pinnedCount || threadInfo.pinnedCount === 0) {
return '';
}
const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
}, [threadInfo.pinnedCount]);
atulUnsubmitted
Not Done
Inline Actions

You don't need to use React.useMemo(...) here.

The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).

On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.

See below:

875403.png (620×810 px, 104 KB)

(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)

atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…
const pinnedCountBanner = React.useMemo(() => {
if (!bannerText) {
return null;
}
return (
<View style={styles.pinnedCountBanner}>
<Text style={styles.pinnedCountText}>{bannerText}</Text>
<Icon
name="chevron-right"
size={12}
color={colors.panelBackgroundLabel}
/>
</View>
);
}, [
bannerText,
styles.pinnedCountBanner,
styles.pinnedCountText,
colors.panelBackgroundLabel,
]);
return ( return (
<MessageListContextProvider threadInfo={threadInfo}> <MessageListContextProvider threadInfo={threadInfo}>
{pinnedCountBanner}
<MessageListContainer <MessageListContainer
{...props} {...props}
usernameInputText={usernameInputText} usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput} updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray} userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput} updateTagInput={updateTagInput}
resolveToUser={resolveToUser} resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos} otherUserInfos={otherUserInfos}
Show All 14 Lines