Implement hook for getting pending invites

[achou11]

Rough sketch of what we initially thought about:

export function usePendingInvites() {
	const queryClient = useQueryClient()
	const clientApi = useClientApi()

	const { data, isFetching } = useQuery(
		pendingInvitesQueryOptions({ clientApi }),
	)

	useEffect(() => {
		function onInviteEvent() {
			queryClient.invalidateQueries({ queryKey: getInvitesQueryKey() })
		}

		clientApi.invite.addListener('invite-received', onInviteEvent)
		clientApi.invite.addListener('invite-removed', onInviteEvent)

		return () => {
			clientApi.invite.removeListener('invite-received', onInviteEvent)
			clientApi.invite.removeListener('invite-removed', onInviteEvent)
		}
	}, [queryClient, clientApi.invite])


	// Not sure what this return value should be...
	return { data: dedupeInvites(data || []), isFetching }
}

Questions:

• We had doubts about whether we wanted this hook to be suspense-based. Given that, not entirely sure what we want the return value to look like. Are we okay with this being one of those weird exceptions to the rest of the read hooks?

• We had mentioned needing to dedupe the invites payload. Not exactly sure why or what that looks like right now.

• Should we extract the effect to its own hook and expose that separately?

[achou11]

Based on discussion, an alternative approach to consider introducing a hook that purely deals with new invites as an effect e.g.

function App() {
    useInviteEffect((invite) => {
        navigation.navigate(InviteModal, { invite })
    })
}

As it currently stands, our applications don't really need access to a list of invites (pending or not) so it probably doesn't make it immediately necessary to expose something like the outlined usePendingInvites() hook above just yet.

With this alternative approach, we need to further investigate how we handle invites statuses changing (e.g. cancelled). Something that was brought up was another hook like useInviteCancelledEffect(), although I had some initial doubts about it.

[gmaclennan]

I was doing some more thinking about this today, given the issues with invites in QA. I think invites need to rely on two event listeners: a navigation event (e.g. changing screen) and an invite received event, because sometimes the invite will be received when on a screen where we cannot show an invite, so we need to check if there is a pending invite when we switch screens:

flowchart TD
    A("`Invite received event`") --> |Check current screen| B{"`Can show 
    invite?`"}
    B --> |Yes| C
    B --> |No| D[Do Nothing]
    C[Show Invite Screen]
    M(Navigation event) --> |Check for invite| N{"`Pending
    Invite?`"}
    N --> |Yes| C
    N --> |No| D

Loading

We can listen to the state event on the rootNavigationRef, and use our existing shouldEnableInviteSheet except rather than enabling the invite sheet, check if there is an invite (const invite: Invite | null = usePendingInvite()) and if there is, then rootNavigationRef.navigate('InviteReceivedScreen', { invite }).

We can then also have a hook useInviteReceivedEvent((invite: Invite) => void): void where we similarly check shouldEnableInviteSheet() and similarly navigate based on the result.

For implementing useInviteReceivedEvent() and simplifying the state listener on rootNavigationRef I recommend Expo's useEventListener() which has a clever solution to avoid the need for dependencies.

I recommend passing the invite around as a param because it guarantees it will be stable. If we try to read it from hooks in each screen, it becomes difficult to guarantee that the invite from the hook will remain the same as the one showing on the screen, and we will end up with strange bugs.

The other "side effect" that we need to handle is an invite being removed. We could handle this side effect in an invite screen (e.g. the screen that shows when we receive an invite) but there is a risk of a race condition where the invite-removed event is fired before the listener is attached in the invite screen. For this reason I think it's better to also attach the invite-removed event to rootNavigationRef in the same place as invite-received, and then use currentRoute?.params?.invite?.inviteId to check if the removed invite is the same as the one currently being shown, and then push another screen onto the stack based if InviteRemovalReason is cancelled. Currently, all other invite removal events will also throw an error in accept() or reject(), however this is fragile and I'm not sure how to avoid this, e.g. what if we listen for a peer disconnecting and remove the invite based on the disconnection event, with a new reason peer disconnected - we would need to also handle that event in the root here. I think we could probably improve the Core API here to make this simpler. I think we should maybe implement the hook here useInviteRemovedEvent to only listen to the events that happen as side effects (e.g. outside user action) rather than as consequences of the user action accept() or reject().

The flow for this is:

flowchart TD
    A("`Invite removed event`") --> |Check current route params| B{"`Current route has
    invite in params?`"}
    B --> |Yes| C{"`Does params.inviteId match removedInvite.id?`"}
    B --> |No| D[Do Nothing]
    C --> |No| D
    C --> |Yes| E{"`Check removal
    reason`"}
    E --> |Other reason| D
    E --> |Canceled| G[Navigate to invite cancelled screen]

Loading

The effect for the user is that they will be on one of the screens in the step for receiving an invite, and this effect will push the invite cancelled (by the peer who sent it) on top of their current screen. The invite cancelled screen should then dismiss the entire stack.

All of this requires quite a lot of logic to remain in the app, rather than in comapeo-core-react, but I think this is unavoidable because desktop and mobile use different navigators, and the logic for when an invite should be shown might be different on mobile and desktop, so the main thing to implement in core-react is:

• useInviteReceivedEvent((invite: Invite) => void): void
• useInviteCancelledEvent((invite: Invite, reason: 'invitor cancelled' | 'connection error') => void): void

For checking whether there is a pending invite when navigation changes, I'm not sure the best approach to avoid strange bugs. If we use react query, then we don't have guarantee that the event useInviteReceivedEvent and the state in react query are the same. I'd welcome ideas about how we can approach this, and I can do some more thinking about it. Invalidating query caches based on the invite-received and invite-removed events seems like a race condition bug waiting to happen.

[gmaclennan]

The core issue is that if we are listening to events on the api.project.invites while reading invites via react query from api.project.invites.getPending(), we have no guarantee that the events and the pending invites are in sync.

We probably need to handle mirrored state for this ourselves. I can help work on this if needed.

What we need is a guarantee that after an invite-removed event, then getPending() will not include it, and after invite-received, then getPending() will include it, and it will not be included/excluded before either of these events.

[gmaclennan]

@achou11 an initial proposed implementation in #47, for feedback on the approach. The implementation is a bit of a pain, and the usage isn't super simple, but hopefully simpler than what we have and less prone to bugs.

If you think this is a good approach, then I can try and write some tests for this.

[gmaclennan]

Thinking through an alternative approach which is possibly more intuitive and/or reactive: We provide two hooks:

• usePendingInvite(): Invite | null
• useInvite(inviteId: string): Invite | null

Then do everything in useEffects, e.g. in the root component:

const RootComponent = () => {
  const navigation = useNavigation()
  const currentRoute = navigation.getCurrentRoute()
  const pendingInvite = usePendingInvite()
  
  useEffect(() => {
    if (pendingInvite && shouldShowInvite(currentRoute)) {
      navigation.navigate(INVITE_SCREEN, { params { inviteId: pendingInviteId } })
    }
  }, [currentRoute, pendingInvite])

Then in an invite screen:

const InviteScreen = ({ params: { inviteId } }) => {
    const invite = useInvite(inviteId)

    useEffect(() => {
      if (invite.state === 'canceled') navigation.navigate(INVITE_CANCELED)
    }, [invite])

    if (!invite) return navigation.navigate(HOME) // Or render error in screen, or navigate to error screen.
})

We would need that code in each screen of the invite flow.

We could mix different ideas from the two approaches, depending on what feels the easiest to work with. Note that this will require a new InviteStore that caches invites and rather than removing them when receiving invite-removed, instead adds a state to the invite. Then we can use useSyncExternalStore to hook up that store for the useInvite(inviteId) hook.

I'm not sure which is going to be more intuitive to use. Since you and @ErikSin will be using this code, maybe you both could weight in on which API makes most sense.

[achou11]

After briefly discussing elsewhere about the two proposed ideas, I'm preferring the second idea. One notable thing that has been tricky to get right in the app is presenting the invite when it's not in a pending state (e.g. rejected or cancelled). Think this is due to the awkwardness between the existing invites API and UX requirements. To me, the second idea will make this much easier to get right, and will prevent the app from having code that makes sure a received invite is updated to have the correct state (becomes an internal detail of the hook that is exposed here).

[gmaclennan]

Yes, the more I think about it, I'm also leaning towards the second. We do need some way of limiting the number of invites we cache, however I think we need that in the existing implementation (pending invites can currently grow unbounded I think), so I think it's ok to do an initial implementation that just leaves all invites in the cache, and assumes this will remain <100 during the apps lifecycle. We can then address bounding the number of stored invites - it will require a little bit of work because we want something slightly different than a LRU cache - for pending invites we want to start rejecting them if we get too many, and for invites that are cancelled, rejected or accepted we want to keep the most recently changed, and ensure that we always keep the latest one.

[ErikSin]

The second approach makes the most sense in terms of the front end. The complication before was maintaining the list of invites that matched what was in the server, while also NOT deleting an invite if a user was interacting with it (aka if it got cancelled while they were trying to accept an invite). In order to do that, we were intercepting the removed invites, and then doing a check before removing them. But we were only intercepting when the user was interacting with the invite (aka the invite modal was open). It was alot of passing of states that made the code quite complicated

If we do what you are suggesting, maintaining our own internal list of invites using useSyncExternalStore, where deleted invites are not removed from the list, but rather has a state that indicated it is removed, then the front end does not need to worry about intercepting the removed invites. And then we can simply use the 2 hooks in isolation which will greatly simplify the code.