Es/invite flow #79

ErikSin · 2023-10-18T22:23:16Z

Allows user to list devices on the same network, and invite them to the project!

to note:

There seems to be a lot of files that were edited, but a large number of them are just SVGs (aka, this isn't a huge PR)

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

ErikSin · 2024-01-30T02:05:47Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectDevice.tsx

+          // this is not exposed yet
+          const deviceType = 'mobile';


Do we expose the deviceType (whether it is mobile or desktop)?

Good point, no we do not. I've opened an issue to initially hardcode this digidem/comapeo-core#451, and we can follow-up with sharing this (digidem/comapeo-core#452)

Can you open an issue to fix this once we update mapeo core with this change?

ErikSin · 2024-01-30T02:09:32Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/WaitingForInviteAccept.tsx

+  React.useEffect(() => {
+    project.$member
+      .invite(route.params.deviceId, {roleId: route.params.role})
+      .then(() => {
+        queryClient.invalidateQueries({queryKey: ['projectMembers']}),
+          navigation.navigate('InviteAccepted', route.params);
+      })
+      .catch(err => {
+        openSheet();
+      });
+  }, []);


The architecture is a little odd here. But essentially, when the coordinator clicks to invite someone, they are navigated to this page to wait for the invite. So im deferring the actual invite till after the navigation has happened so we have access to the promise. Otherwise, the user clicks invite, the invite promise is created, and then the user is navigated to this page. Since react-nav doesnt allow for non-serializable things to be passed between pages, it would not have access to the promise.

I understand the reasons for doing this within the limitations of react-nav, but unfortunately this approach will lead to subtle and hard-to-track-down bugs (e.g. see https://react.dev/learn/you-might-not-need-an-effect#sending-a-post-request). Currently:

An invite will be sent twice in development mode (because useEffects run twice in development mode)

If the user closes the app and re-opens it, the invite could be re-sent if the screen re-renders

If queryClient or openSheet change between renders, then this will fail because they are not included in the useEffect dependencies. This is currently ok because they are stable across renders, but a change to the useBottomSheetModal that made openSheet change between renders would cause bugs.

Some of this could be avoided by adding a ref that tracks whether this has run once, but some issues above would remain. I think to solve this we need to either:

Track invite state in context (lots of additional code)

Move all these invite states into a single screen (preferable I think), e.g. [inviteState, setInviteState] = useState<'pending' | 'waiting' | 'accepted' | 'rejected'>('pending') (then swap navigate() with setInviteState())

I updated this.

I dont think we need to worry about all 4 states in one screen. One screen just needs to handle sending the invite, and tracking the promise that comes along with it. As soon as the promise is resolved or rejected, we can just navigate to a new page. So I updated the page to do that.

Unfortunately we don't get the transition animation between sending and invite and waiting for an invite that react navigation gave us, but for now I think that is ok.

gmaclennan

Good work on this, it's looking good, however I think the approach of sending an invite as a side-effect of rendering the WaitingForInviteAccept screen is prone to bugs (especially as we don't have a meaningful way to cancel invites yet).

There are a couple of issues this throws up around the limitations of the API, and I've created some issues for those. I think the role names and IDs also needs cleaned up - it's created to be forwards compatible with custom role definitions, but I think it's not clear how to use it right now and maybe we are not exposing enough from the backend. It would help to discuss this together and figure out how to adapt the API, and the best workaround for the meantime.

src/frontend/screens/Settings/ProjectSettings/YourTeam/ReviewInvitation.tsx

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectInviteeRole.tsx

gmaclennan · 2024-01-30T12:58:34Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/WaitingForInviteAccept.tsx

+  React.useEffect(() => {
+    project.$member
+      .invite(route.params.deviceId, {roleId: route.params.role})
+      .then(() => {
+        queryClient.invalidateQueries({queryKey: ['projectMembers']}),
+          navigation.navigate('InviteAccepted', route.params);
+      })
+      .catch(err => {
+        openSheet();
+      });
+  }, []);


I understand the reasons for doing this within the limitations of react-nav, but unfortunately this approach will lead to subtle and hard-to-track-down bugs (e.g. see https://react.dev/learn/you-might-not-need-an-effect#sending-a-post-request). Currently:

An invite will be sent twice in development mode (because useEffects run twice in development mode)

If the user closes the app and re-opens it, the invite could be re-sent if the screen re-renders

If queryClient or openSheet change between renders, then this will fail because they are not included in the useEffect dependencies. This is currently ok because they are stable across renders, but a change to the useBottomSheetModal that made openSheet change between renders would cause bugs.

Some of this could be avoided by adding a ref that tracks whether this has run once, but some issues above would remain. I think to solve this we need to either:

Track invite state in context (lots of additional code)

Move all these invite states into a single screen (preferable I think), e.g. [inviteState, setInviteState] = useState<'pending' | 'waiting' | 'accepted' | 'rejected'>('pending') (then swap navigate() with setInviteState())

gmaclennan · 2024-01-30T13:21:38Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

+            name={coordinator.name || ''}
+            deviceId={coordinator.deviceId}
+            deviceType="mobile"
+            // This is a weak check. We should be using deviceIds, but those are not exposed


Good point, fix coming in digidem/comapeo-core#454

gmaclennan · 2024-01-30T13:31:55Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectDevice.tsx

+  const ssid = useLocalDiscoveryState(state => state.ssid);
+  const {formatMessage: t} = useIntl();
+
+  const devices = useLocalPeers();


Note, this will include devices that are already members of the project until #167 is fixed

src/frontend/screens/Settings/ProjectSettings/YourTeam/ReviewInvitation.tsx

gmaclennan · 2024-01-30T13:39:14Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectInviteeRole.tsx

+        onPress={() =>
+          navigation.navigate('ReviewInvitation', {
+            ...route.params,
+            role: 'coordinator',


This is used in the WaitingForInviteAccept component as the roleId, which is a unique ID. This should be renamed maybe, and use the roleId, however I'm not sure if we correctly expose this in the API. I think the API is confusing here, and maybe we can quickly jump on a call to discuss how to improve it.

gmaclennan

Good work on this, it's looking good, however I think the approach of sending an invite as a side-effect of rendering the WaitingForInviteAccept screen is prone to bugs (especially as we don't have a meaningful way to cancel invites yet).

There are a couple of issues this throws up around the limitations of the API, and I've created some issues for those. I think the role names and IDs also needs cleaned up - it's created to be forwards compatible with custom role definitions, but I think it's not clear how to use it right now and maybe we are not exposing enough from the backend. It would help to discuss this together and figure out how to adapt the API, and the best workaround for the meantime.

src/frontend/hooks/server/projects.ts

ErikSin · 2024-01-30T23:00:57Z

I tested this with 2 devices today. Unfortunately I do not think peer discover is running. I had both devices running the dev environment (I learned today that if you plug in 2 devices, metro will simultaneouly run on both devices). api.on('local-peers', ...) is not being called when there is another device around.

@gmaclennan

This change makes several improvements to the concept formerly known as "capabilities": - `Capability` is now called `Role` across the project, resulting in many simple renames. - `Role`s have an attached `roleId` property, which they didn't before. - The creator role and "no role" role now have role IDs.[^1] - The `RoleId` type now includes all possible role types, now including the creator role and the "no role" role. I created narrower types such as `RoleIdAssignableToOthers` and `RoleIdAssignableToAnyone`, which don't include those. This should help the front-end know what role someone is (for example, <digidem/comapeo-mobile#79 (comment)>). [^1]: I generated these with `crypto.randomBytes(8).toString('hex')`.

ErikSin · 2024-02-01T00:21:18Z

src/backend/patches/@mapeo+core+9.0.0-alpha.4+001+fix-quickbit-dynamic-require.patch

+       return
+     }
+
+    const { address } = referer


Evan and I discovered that address was showing up in referer, but not in service. @gmaclennan

Hmm I have a feeling that the changes in this patch will break things in other network environments, but if it’s enough to get it working for initial testing then that’s ok for now.

This doesn't change how the front end is being implemented, so if we need to revert it and fix it another way, it shouldn't make a difference

ErikSin · 2024-02-01T00:23:27Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/ReviewInvitation.tsx

+    setInviteSent(true);
+    project.$member
+      .invite(rest.deviceId, {roleId: role})
+      .then(val => navigation.navigate('InviteAccepted', {...route.params}))


This is timing out after 5 seconds:

Error: Server timed out after 5000ms. The server could be closed or the transport is down.

Otherwise, the other device is recieving the invite and is able to accept it and join the project @gmaclennan

achou11

not a full review but overall changes look good. will try to get to the rest of it tomorrow!

achou11 · 2024-02-06T21:09:59Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectInviteeRole.tsx

+    <TouchableOpacity
+      style={[styles.flexRow, styles.cardContainer, style]}
+      onPress={onPress}>
+      <MaterialIcon name="radio-button-off" size={25} color={DARK_GREY} />


nit: is this needed given that the screen immediately navigates after pressing?

If we are going to show it, we should set it to on when pressed, because navigation is never immediate in React Native.

achou11 · 2024-02-06T21:23:09Z

package.json

@@ -25,7 +25,7 @@
    "@formatjs/intl-pluralrules": "^5.2.4",
    "@formatjs/intl-relativetimeformat": "^11.2.4",
    "@gorhom/bottom-sheet": "^4.5.1",
-    "@mapeo/ipc": "^0.1.3",
+    "@mapeo/ipc": "^0.1.5",


nit: can update the backend package.json too to keep in sync (not really necessary though)

src/frontend/hooks/server/projects.ts

achou11 · 2024-02-06T21:34:09Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/WaitingForInviteAccept.tsx

+      <ErrorModal
+        sheetRef={sheetRef}
+        closeSheet={closeSheet}
+        isOpen={isOpen}
+        clearError={() => navigation.navigate('YourTeam')}
+      />


Suggestion: move this to ReviewInvitation. simplifies implementation of WaitingForInviteAccept and co-locates the usage of the bottom sheet modal with where the hook is used and what triggers the modal (which is generally easier to follow). would probably result in a little bit of refactoring in ReviewInvitation though. something along the lines of this is what i'm thinking:

export const ReviewInvitation: NativeNavigationComponent< 'ReviewInvitation' > = ({route, navigation}) => { const {role, name, deviceId, deviceType} = route.params; const [inviteStatus, setInviteStatus] = React.useState< 'reviewing' | 'waiting' >('reviewing'); const {openSheet, ...restBottomSheet} = useBottomSheetModal({ openOnMount: false, }); const project = useProject(); const queryClient = useQueryClient(); function sendInvite() { setInviteStatus('waiting'); project.$member .invite(deviceId, {roleId: role}) .then(val => { if (val == 'ACCEPT') queryClient.invalidateQueries({queryKey: ['projectMembers']}); navigation.navigate('InviteAccepted', {...route.params}); }) .catch(err => { console.log(err); openSheet(); }); } return ( <React.Fragment> {inviteStatus === 'reviewing' ? ( // Name can be whatever but just wanted to show a purely view-based component that you could create to display in the reviewing state <InvitationReview sendInvite={sendInvite} role={role} name={name} deviceId={deviceId} deviceType={deviceType} /> ) : ( <WaitingForInviteAccept /> )} <ErrorModal {...restBottomSheet} clearError={() => navigation.navigate('YourTeam')} /> </React.Fragment> ); };

This change makes several improvements to the concept formerly known as "capabilities": - `Capability` is now called `Role` across the project, resulting in many simple renames. - `Role`s have an attached `roleId` property, which they didn't before. - The creator role and "no role" role now have role IDs.[^1] - The `RoleId` type now includes all possible role types, now including the creator role and the "no role" role. I created narrower types such as `RoleIdAssignableToOthers` and `RoleIdAssignableToAnyone`, which don't include those. This should help the front-end know what role someone is (for example, <digidem/comapeo-mobile#79 (comment)>). [^1]: I generated these with `crypto.randomBytes(8).toString('hex')`. Co-Authored-By: Gregor MacLennan <[email protected]>

gmaclennan

Thanks Erik, I mainly focussed on reviewing the logic - I have not tested this in the emulator or on a device. There are a few small things, but nothing blocking this I don't think - we can fix things up in a follow-up as long as you open the issues. Two questions here about text size accessibility, and possible error when cancelling an invite then the invite errors. I think cancellable invites is an important missing feature from the backend that we need to address.

gmaclennan · 2024-02-07T17:27:56Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

+  const {formatMessage: t} = useIntl();
+  const membersQuery = useProjectMembers();
+  const deviceInfo = useDeviceInfo();
+  const coordinators = React.useMemo(


I wouldn't bother using useMemo on these, the overhead of memoization is likely greater than the cost of filtering. Array filtering operations like this are very fast unless you are dealing with millions of entries.
Fine to leave in for now.

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

gmaclennan · 2024-02-07T17:32:38Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

+            name={participant.name || ''}
+            deviceId={participant.deviceId}
+            deviceType="mobile"
+            // This is a weak check. We should be using deviceIds, but those are not exposed


Can you make sure an issue is open to fix this once we update to latest mapeo core?

gmaclennan · 2024-02-07T17:32:58Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx

+            deviceType="mobile"
+            // This is a weak check. We should be using deviceIds, but those are not exposed
+            thisDevice={
+              deviceInfo.data && deviceInfo.data.name === coordinator.name


can you make sure an issue is open to fix this once we update to latest mapeo core?

gmaclennan · 2024-02-07T17:36:02Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/SelectDevice.tsx

+          // this is not exposed yet
+          const deviceType = 'mobile';


Can you open an issue to fix this once we update mapeo core with this change?

gmaclennan · 2024-02-07T17:53:18Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/WaitingForInviteAccept.tsx

+  },
+  timerMessage: {
+    id: 'screens.Setting.ProjectSettings.YourTeam.WaitingForInviteAccept.timerMessage',
+    defaultMessage: 'Invite sent {seconds}s ago',


Can you open an issue to internationalize this? Will need to handle plurals for different languages.

gmaclennan · 2024-02-07T17:55:42Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/ReviewInvitation.tsx

+      })
+      .catch(err => {
+        console.log(err);
+        openSheet();


This screen could be unmounted at this point (if the user had clicked cancel invite on the waiting screen), so I think openSheet() would fail?

gmaclennan · 2024-02-07T17:59:14Z

src/frontend/sharedComponents/DeviceNameWithIcon.tsx

+        <Text style={{fontWeight: 'bold'}}>{name}</Text>
+        {deviceId && (
+          <Text style={{color: MEDIUM_GREY}} numberOfLines={1}>
+            {`${deviceId.slice(0, 12)}...`}


Can you open an issue to decide how we display deviceIds to the user. There is a previous discussion here.

gmaclennan · 2024-02-07T17:59:39Z

src/frontend/sharedComponents/DeviceNameWithIcon.tsx

+  style,
+}: DeviceNameWithIconProps) => {
+  const {formatMessage} = useIntl();
+  console.log({name});


remove before merge

src/frontend/sharedComponents/Text.tsx

gmaclennan · 2024-02-07T18:05:33Z

src/frontend/screens/Settings/ProjectSettings/YourTeam/ReviewInvitation.tsx

+      .then(val => {
+        if (val == 'ACCEPT')
+          queryClient.invalidateQueries({queryKey: ['projectMembers']});
+        navigation.navigate('InviteAccepted', {...route.params});


Just realized there is no code path for when an invite is rejected, nor do we have a screen for that. Maybe a follow-up?

Yeah, I discussed this with Sabella. We are just going to show the error screen with a message. Ill create a ticket and we can do it as follow up

gmaclennan

Sorry I missed something in my first review. An invite response can be ACCEPT, REJECT, ALREADY (device being invited is already part of the project) or UNRECOGNIZED (this is an unexpected/unknown response, I think actually the method should throw instead of resolving with this response). Currently all responses result in navigation to the InviteAccepted screen. I think maybe we need to show the different responses on that screen.

ErikSin · 2024-02-08T00:11:30Z

Sorry I missed something in my first review. An invite response can be ACCEPT, REJECT, ALREADY (device being invited is already part of the project) or UNRECOGNIZED (this is an unexpected/unknown response, I think actually the method should throw instead of resolving with this response). Currently all responses result in navigation to the InviteAccepted screen. I think maybe we need to show the different responses on that screen.

I updated so it only navigates on accept. Ill create a ticket for handling the other states (Im just going to throw them and pass a message to the error screen)

… resolved" This reverts commit 7fc1ed2.

ErikSin force-pushed the es/invite-flow branch from 7026eb7 to 6fb33f5 Compare December 16, 2023 18:25

ErikSin force-pushed the es/invite-flow branch 3 times, most recently from 5287e17 to 8657e84 Compare January 29, 2024 22:42

ErikSin commented Jan 30, 2024

View reviewed changes

src/frontend/screens/Settings/ProjectSettings/YourTeam/index.tsx Outdated Show resolved Hide resolved

ErikSin commented Jan 30, 2024

View reviewed changes

ErikSin requested a review from gmaclennan January 30, 2024 02:11

gmaclennan requested changes Jan 30, 2024

View reviewed changes

gmaclennan reviewed Jan 30, 2024

View reviewed changes

src/frontend/hooks/server/projects.ts Show resolved Hide resolved

EvanHahn mentioned this pull request Jan 31, 2024

feat!: rename "Capability" to "Role", attach ID digidem/comapeo-core#462

Merged

ErikSin commented Feb 1, 2024

View reviewed changes

ErikSin mentioned this pull request Feb 6, 2024

Es/listen for invites hook #172

Closed

achou11 reviewed Feb 6, 2024

View reviewed changes

gmaclennan approved these changes Feb 7, 2024

View reviewed changes

gmaclennan reviewed Feb 7, 2024

View reviewed changes

gmaclennan requested changes Feb 7, 2024

View reviewed changes

ErikSin added 7 commits February 8, 2024 15:07

created initial screens

fc14c0d

more invite screen

d0144bf

translations

ad8e1fa

finished screens

04f8850

chore: integrate api, and discovery hook

be4105c

chore: integrated api for invite

0b5f3f0

chore: ui changes

eb237a5

ErikSin added 13 commits February 8, 2024 15:08

chore: updated invite state management

d84fba5

chore: patch-package for mapeo/core discovery

3ffc1f5

chore: styling

11dcd8b

chore: update styling

c197d53

fix: ipc timeout removed

0286e4e

chore: updated component to encapsulate 2 screens

a55bbba

chore:translations

148bce2

chore:update @mapeo/ipc to @latest

ecfcbab

chore:remove memoization

f0718cf

chore: remove unnecessary undefined check

c693764

chore: remove unnecessary console logs

13ef6ec

chore:track mounting of screen so promise does not always get resolved

cb82b67

Revert "chore:track mounting of screen so promise does not always get…

1a1d0ab

… resolved" This reverts commit 7fc1ed2.

ErikSin force-pushed the es/invite-flow branch from 2fbdd5e to 1a1d0ab Compare February 8, 2024 23:13

chore:translations

9cd0eaa

ErikSin merged commit 8eec84a into main Feb 8, 2024

Es/invite flow #79

Es/invite flow #79

Conversation

ErikSin commented Oct 18, 2023 • edited Loading

to note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSin Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

gmaclennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaclennan left a comment

Choose a reason for hiding this comment

ErikSin commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaclennan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaclennan left a comment • edited Loading

Choose a reason for hiding this comment

ErikSin commented Feb 8, 2024

ErikSin commented Oct 18, 2023 •

edited

Loading

ErikSin Jan 30, 2024 •

edited

Loading

gmaclennan left a comment •

edited

Loading

gmaclennan left a comment •

edited

Loading