Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: notified on canceled invited #366

Merged
merged 6 commits into from
May 27, 2024
Merged

feat: notified on canceled invited #366

merged 6 commits into from
May 27, 2024

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented May 19, 2024

Sorry for the long explanation, but I think it is necessary to understand the architectural decisions that were made

Creates notification when invite has been cancelled.

I had to refactor the invite listeners to account for some of the peculiarities of this flow.

When an invitor cancels an invite, it is only relevant to the invitee if the invitee has been notified of the invite. For example, if the invitee receives an invite, they will be notified of that invite and a bottom sheet will pop up asking them if they would like to accept an invite. Before they have chosen to accept or decline this invite, if they receive another invite, this second invite is queued, and the second invite is only shown if they decline the first invite. But if the invitor of the second invite cancels the invite, and the invitee has not even seen this invite yet, we don't want to notify the invitee it has been cancelled. Otherwise they might think the first invite has been cancelled. So we want to make sure that they only received the notification that it is being cancelled when the user is actively interacting with the invite.

We know a user is actively interacting with a cancelled invite when:

  1. the project invite bottoms sheet is open
  2. the cancelled invite is the oldest invite (because it is designed that invites are dealt with chronologically)

Previous implementation (and why it didn't work for canceling)

Invite listener was set up at the root of the app. This listener listened for invites being added and removed. When an invite was added or removed, we invalidated the cache which retrieved all invites. This meant the client state was always updated according to the server state.

If we kept this architecture, when an invite was cancelled, the list of invites would be updated, and would not contain the cancelled invite. Due to the declarative nature of react, this would mean the invite would disappear and the invite bottom sheet would be closed (or the next queued invite would be shown). Obviously we want the user to be notified that their invite was cancelled, so this wouldn't work.

New architectural decisions

On first glance it would seem that the easy way would be to intercept all cancelled invites and notify the user. But again we do not want to notify the user of all cancelled invites, only the ones that the user is already interacting with. In order to do that we need to
1.intercept all removed invites
2a.If reason !== 'cancelled' invalidate the cache as normal
2b.if reason === 'cancelled' determine if the user is actively interacting with the invite that was cancelled (based on the 2 criteria above)
3a.If the user is not interacting with the cancelled invite, invalidate cache as normal
3b.If the IS interacting with the cancelled invite, do not invalidate the cache (again, the bottom sheet is declarative so if the invite list gets updated the bottom sheet will close or show the next invite).
4.Notify the user (by updating the UI of the bottom sheet)
5.User acknowledges that the invite has been cancelled, the cache is imperatively invalidated, and the client side list of invites now matches the server side list of invites-where the cancelled invite has been removed.

Screen.Recording.2024-05-19.at.11.10.57.AM.mov

@ErikSin ErikSin requested a review from achou11 May 19, 2024 19:04
Comment on lines +29 to +30
inviteId: invite?.inviteId,
bottomSheetIsOpen: isOpen,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the conditions used to determine if a canceled invite is the invite the user is interacting with.

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading through this implementation is giving me a clearer idea of some refactoring that would make all of this a lot more straightforward. i have most of the changes done locally as I was experimenting, but probably not worth trying to introduce to this PR and instead do as a follow-up (or writing a detailed issue first, then PR).

import {MapBuffers} from '@mapeo/core/dist/types';
import {InviteInternal, InviteRemovalReason} from '@mapeo/core/dist/invite-api';

export const useProjectInvitesListener = ({
Copy link
Member

@achou11 achou11 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this hook is doing too much and the bottom sheet-specific pieces can be extracted to a separate hook.

for example, i would basically restore initializeInviteListener(), but update the invite-removed listener to ignore cancelled invites. then define a separate hook for the invite-removed event that handles those cancelled cases:

function useCancelledInvite(
  inviteId: string,
  bottomSheetIsOpen: boolean,
) {
  const mapeoApi = useApi();
  const queryClient = useQueryClient();

  const [cancelled, setCancelled] = useState(false);

  const dismissCancelledInvite = useCallback(() => {
    setCancelled(false);
    queryClient.invalidateQueries({queryKey: [INVITE_KEY]});
  }, [queryClient, setCancelled]);

  useEffect(() => {
    function shouldInterceptCancel(
      val: MapBuffers<InviteInternal>,
      reason: InviteRemovalReason,
    ) {
      if (
        reason === 'canceled' &&
        inviteId === val.inviteId &&
        bottomSheetIsOpen
      ) {
        setCancelled(true);
      }
    }

    mapeoApi.invite.addListener('invite-removed', shouldInterceptCancel);

    return () => {
      mapeoApi.invite.removeListener('invite-removed', shouldInterceptCancel);
    };
  }, [mapeoApi, inviteId, bottomSheetIsOpen, setCancelled]);

  return [cancelled, dismissCancelled] as const;
}

useProjectInvitesListener() could then be used anywhere near the top level of the app, similar to what initializeInviteListener() was doing. useCancelledInvite() would then be used in ProjectInviteBottomSheet

Copy link
Member

@achou11 achou11 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about this more, and maybe the separation is trickier than i thought. i think that's a symptom of how the invites are set up and i think the potential refactor i'm thinking of would fix it.

if it turns out that this separation that i'm proposing isn't feasible or reasonable, can potentially ignore it for now.

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will create a separate issue with some thoughts about refactoring how invites are represented in the app to account for UX needs that this PR highlights :)

@ErikSin ErikSin merged commit a580641 into main May 27, 2024
3 checks passed
@ErikSin ErikSin deleted the es/cancelled-invite branch May 27, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants