Page MenuHomePhabricator
Differential D11707

[keyserver] Send farcaster_mutual UPDATE_RELATIONSHIP message when establishing Farcaster relationship
ClosedPublic
Actions

Authored by ashoat on Apr 21 2024, 4:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 31, 4:22 PM
Unknown Object (File)
Mon, Mar 31, 2:27 PM
Unknown Object (File)
Mon, Mar 31, 4:43 AM
Unknown Object (File)
Sun, Mar 30, 11:35 PM
Unknown Object (File)
Sun, Mar 30, 12:33 AM
Unknown Object (File)
Sat, Mar 29, 9:45 PM
Unknown Object (File)
Fri, Mar 21, 11:45 AM
Unknown Object (File)
Mon, Mar 10, 4:25 PM
View All 81 Files
Subscribers
atul
tomek

Details

Reviewers
atul
ginsu
Commits
rCOMM1548889e36d8: [keyserver] Send farcaster_mutual UPDATE_RELATIONSHIP message when establishing…
Summary

This is developed from my Hackathon commit 7024301fcda748efefa900bd952cd8e127aac87d, but updated to reflect changes @atul has made to allow us to include FID:

  1. Updates to the structure of the UPDATE_RELATIONSHIP message
  2. Updates to the FARCASTER_MUTUAL API

Note that I also updated the code to avoid creating KNOW_OF relationships between the viewer and themselves.

Test Plan

I tested this during the Hackathon, but haven't tested the updates since then. Will try to test this in the office this week

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Apr 21 2024, 4:57 PM
Herald added a subscriber: tomek. · View Herald TranscriptApr 21 2024, 4:57 PM
Harbormaster completed remote builds in B28347: Diff 39329.Apr 21 2024, 5:14 PM
ashoat requested review of this revision.Apr 21 2024, 5:14 PM
atul mentioned this in D11712: [lib] Shim unsupported messages for `web`/`desktop`.Apr 22 2024, 10:13 AM
atul accepted this revision.Apr 22 2024, 10:16 AM

Change looks good, but we should probably land something like D11712 first so web/desktop clients receive shimmed UNSUPPORTED messages?

This revision is now accepted and ready to land.Apr 22 2024, 10:16 AM
ashoat added a comment.Apr 22 2024, 10:23 AM

Makes sense – I'll defer landing this

ashoat added a parent revision: D11712: [lib] Shim unsupported messages for `web`/`desktop`.Apr 26 2024, 5:41 PM
atul mentioned this in rCOMM4c5a86dbda62: [lib] Shim unsupported messages for `web`/`desktop`.May 1 2024, 4:54 PM
ashoat added a comment.May 2 2024, 10:16 PM

I haven't landed this yet because I'm having trouble testing it while tethering and don't have access to Wi-Fi right now. Will hopefully find some Wi-Fi tomorrow

ashoat updated this revision to Diff 39769.May 3 2024, 1:38 PM

Finally got to testing this and found some issues:

  1. The list of userIDs passed to createPersonalThreads is not filtered through fetchUserInfos, which can lead to validateCandidateMembers throwing when createThread is called. This issue was present with relationshipActions.FRIEND as well. I addressed it by adding a third parameter to createPersonalThreads that passes in the filtered list of userIDs.
  2. We weren't filtering the viewer out before calling createPersonalThreads. (This one wasn't affecting relationshipActions.FRIEND; it's specific to relationshipActions.FARCASTER_MUTUAL.)
  3. We aren't specifying dontPrefixCreator when shimming UPDATE_RELATIONSHIP messages. Will address in a separate diff.
This revision was landed with ongoing or failed builds.May 3 2024, 1:41 PM
Closed by commit rCOMM1548889e36d8: [keyserver] Send farcaster_mutual UPDATE_RELATIONSHIP message when establishing… (authored by ashoat). · Explain Why
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM1548889e36d8: [keyserver] Send farcaster_mutual UPDATE_RELATIONSHIP message when establishing….
ashoat added a comment.May 3 2024, 1:46 PM
  1. We aren't specifying dontPrefixCreator when shimming UPDATE_RELATIONSHIP messages. Will address in a separate diff.

Addressed in D11868

Harbormaster completed remote builds in B28647: Diff 39769.May 3 2024, 1:56 PM

Revision Contents
Changeset List

PathSize
keyserver/
src/
updaters/
52 lines

Diff 39769

View Options

keyserver/src/updaters/relationship-updaters.js

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;