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.

Sign up for GitHub

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

Ml/fix deeplink to groupchat #1395

Merged
merged 11 commits into from
Dec 19, 2024
Merged

Ml/fix deeplink to groupchat #1395

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
24975c4
bugfix: fix notification deeplink to group chat
technoplato Dec 19, 2024
8f6ba7c
attempt to wait for db to reconnect
technoplato Dec 19, 2024
cfa6299
wait for xmtp client hydration; cleanup nav
technoplato Dec 19, 2024
0a0fa5b
Update onInteractWithNotification.ts
technoplato Dec 19, 2024
956242a
Merge branch 'release/3.0.0' into ml/fix-deeplink-to-groupchat
technoplato Dec 19, 2024
b02e0fb
typescript
technoplato Dec 19, 2024
c9c012b
Merge branch 'release/3.0.0' into ml/fix-deeplink-to-groupchat
technoplato Dec 19, 2024
e6dc449
better waiting logic based on subscribe instead of polling
technoplato Dec 19, 2024
2ebf4f0
use store subscription rather than poll; add timeout (defaults to 1 s…
technoplato Dec 19, 2024
807aff5
refacgtor waitForXmtpClientHydration to use subscription to store; wa…
technoplato Dec 19, 2024
6d4539e
Merge branch 'release/3.0.0' into ml/fix-deeplink-to-groupchat
technoplato Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions data/store/appStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { create } from "zustand";
import { persist, createJSONStorage } from "zustand/middleware";

import { zustandMMKVStorage } from "../../utils/mmkv";
import logger from "@/utils/logger";

// A app-wide store to store settings that don't depend on
// an account like if notifications are accepted
Expand Down Expand Up @@ -85,3 +86,29 @@ export const useAppStore = create<AppStoreType>()(
}
)
);

/**
* Utility function to wait for the app store to be hydrated.
* This can be used, for example, to ensure that the xmtp clients have been instantiated and the
* conversation list is fetched before navigating to a conversation.
*
* As of December 19, 2024, when we say Hydration, we mean that the following are true:
* 1) XMTP client for all accounts added to device have been instantiated and cached
* 2) Conversation list for all accounts added to device have been fetched from the network and cached
* 3) Inbox ID for all accounts added to device have been fetched from the network and cached
*
* You can observe that logic in the HydrationStateHandler, and that will likely be moved once
* we refactor accounts to be InboxID based in upcoming refactors.
*/
export const waitForXmtpClientHydration = async () => {
logger.debug(
`[waitForAppStoreHydration] Waiting for app store hydration to complete`
);
while (!useAppStore.getState().hydrationDone) {
logger.debug(
`[waitForAppStoreHydration] App store hydration not complete, waiting 100ms...`
);
await new Promise((resolve) => setTimeout(resolve, 100));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing this vs creating a subscription to the store instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I thought about that too. But It would look something like that no? Which isn't cleaner IMO or even much more performant. Maybe we keep what Michael have for now which is good enough IMO.

And we'll probably have to refactor stuff anyway later

const waitForReady = (): Promise<void> => {
  return new Promise((resolve) => {
    const { isReady } = useAccountsStore.getState();
    if (isReady) {
      resolve();
      return;
    }
    const unsubscribe = useAccountsStore.subscribe(
      (state) => state.isReady,
      (ready) => {
        if (ready) {
          resolve();
          unsubscribe(); // Stop listening after `isReady` becomes true
        }
      }
    );
  });
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you guys are right I like this better. It would also resolve sooner (rather than waiting for the 100ms window to elapse)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here's a loom with this change: https://www.loom.com/share/d1967df857fb4d1984cbdce195ff7739 (explaining the reason for the wait(1)

}
logger.debug(`[waitForAppStoreHydration] App store hydration complete`);
};
44 changes: 7 additions & 37 deletions features/notifications/utils/onInteractWithNotification.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
import * as Notifications from "expo-notifications";
import {
navigate,
navigateToTopic,
setTopicToNavigateTo,
} from "@utils/navigation";
import { navigate, navigateToTopic } from "@utils/navigation";
import { getTopicFromV3Id } from "@utils/groupUtils/groupId";
import { useAccountsStore } from "@data/store/accountsStore";
import type { ConversationId, ConversationTopic } from "@xmtp/react-native-sdk";
import { fetchPersistedConversationListQuery } from "@/queries/useConversationListQuery";
import { resetNotifications } from "./resetNotifications";
import logger from "@/utils/logger";
import { getXmtpClient } from "@/utils/xmtpRN/sync";
import { ConverseXmtpClientType } from "@/utils/xmtpRN/client";
import { waitForXmtpClientHydration } from "@/data/store/appStore";
import logger from "@utils/logger";

export const onInteractWithNotification = async (
event: Notifications.NotificationResponse
) => {
logger.debug("[onInteractWithNotification]");
// todo(lustig): zod verification of external payloads such as those from
// notifications, deep links, etc
let notificationData = event.notification.request.content.data;
// Android returns the data in the body as a string
if (
Expand Down Expand Up @@ -58,40 +55,13 @@ export const onInteractWithNotification = async (
| undefined;

if (conversationTopic) {
// todo(lustig): zod verification of external payloads such as those from
// notifications, deep links, etc
await waitForXmtpClientHydration();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out all the crap I was doing before in the previous diff uh was done in client hydration so it's not needed here

const account: string =
notificationData["account"] || useAccountsStore.getState().currentAccount;

// Fetch the conversation list to ensure we have the latest conversation list
// before navigating to the conversation
try {
await fetchPersistedConversationListQuery({ account });
} catch (e) {
logger.error(
`[onInteractWithNotification] Error fetching conversation list from network`
);
if (
`${e}`.includes("storage error: Pool needs to reconnect before use")
) {
logger.info(
`[onInteractWithNotification] Reconnecting XMTP client for account ${account}`
);
const client = (await getXmtpClient(account)) as ConverseXmtpClientType;
await client.reconnectLocalDatabase();
logger.info(
`[onInteractWithNotification] XMTP client reconnected for account ${account}`
);
logger.info(
`[onInteractWithNotification] Fetching conversation list from network for account ${account}`
);
await fetchPersistedConversationListQuery({ account });
}
}
useAccountsStore.getState().setCurrentAccount(account, false);

navigateToTopic(conversationTopic as ConversationTopic);
setTopicToNavigateTo(undefined);
resetNotifications();
}
};
18 changes: 11 additions & 7 deletions screens/Main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
import { useAddressBookStateHandler } from "../utils/addressBook";
import { useAutoConnectExternalWallet } from "../utils/evm/external";
import { usePrivyAccessToken } from "../utils/evm/privy";
import { converseNavigations } from "../utils/navigation";
import {
converseNavigations,

Check failure on line 25 in screens/Main.tsx

View workflow job for this annotation

GitHub Actions / tsc 

Module '"../utils/navigation"' has no exported member 'converseNavigations'.
converseNavigatorRef,

Check failure on line 26 in screens/Main.tsx

View workflow job for this annotation

GitHub Actions / tsc 

'"../utils/navigation"' has no exported member named 'converseNavigatorRef'. Did you mean 'setConverseNavigatorRef'?
setConverseNavigatorRef,
} from "../utils/navigation";
import { ConversationScreenConfig } from "../features/conversation/conversation.nav";
import { GroupScreenConfig } from "./Navigation/GroupNav";
import {
Expand All @@ -39,6 +43,7 @@
getConverseStateFromPath,
} from "./Navigation/navHelpers";
import { JoinGroupScreenConfig } from "@/features/GroupInvites/joinGroup/JoinGroupNavigation";
import logger from "@/utils/logger";

const prefix = Linking.createURL("/");

Expand Down Expand Up @@ -84,9 +89,10 @@
theme={navigationTheme}
linking={linking}
ref={(r) => {
if (r) {
converseNavigations["main"] = r;
}
logger.info(
`[Main] Setting navigation ref to ${r ? "not null" : "null"}`
);
setConverseNavigatorRef(r);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only have one navigation ref now that we are no longer supporting different layouts and stuff so I did some refactoring to reflect that

}}
onUnhandledAction={() => {
// Since we're handling multiple navigators,
Expand All @@ -105,9 +111,7 @@
const NavigationContent = () => {
const authStatus = useAuthStatus();

const { splashScreenHidden } = useAppStore(
useSelect(["notificationsPermissionStatus", "splashScreenHidden"])
);
const { splashScreenHidden } = useAppStore(useSelect(["splashScreenHidden"]));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unused


// Uncomment to test design system components
// return (
Expand Down
44 changes: 32 additions & 12 deletions utils/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,52 @@ import { Linking as RNLinking } from "react-native";

import logger from "./logger";
import config from "../config";
import { currentAccount, getChatStore } from "../data/store/accountsStore";
import { NavigationParamList } from "../screens/Navigation/Navigation";
import type { ConversationWithCodecsType } from "./xmtpRN/client";
import type { ConversationTopic } from "@xmtp/react-native-sdk";
import { NavigationContainerRef } from "@react-navigation/native";

export const converseNavigations: { [navigationName: string]: any } = {};
let converseNavigatorRef: NavigationContainerRef<NavigationParamList> | null =
null;

export const setConverseNavigatorRef = (
ref: NavigationContainerRef<NavigationParamList> | null
) => {
if (converseNavigatorRef) {
logger.error("[Navigation] Conversation navigator ref already set");
return;
}
if (!ref) {
logger.error("[Navigation] Conversation navigator ref is null");
return;
}
converseNavigatorRef = ref;
};

export const navigate = async <T extends keyof NavigationParamList>(
screen: T,
params?: NavigationParamList[T]
) => {
if (!converseNavigatorRef) {
logger.error("[Navigation] Conversation navigator not found");
return;
}

if (!converseNavigatorRef.isReady()) {
logger.error(
"[Navigation] Conversation navigator is not ready (wait for appStore#hydrated to be true using waitForAppStoreHydration)"
);
return;
}

logger.debug(
`[Navigation] Navigating to ${screen} ${
params ? JSON.stringify(params) : ""
}`
);
// Navigate to a screen in all navigators
// like Linking.openURL but without redirect on web
Object.values(converseNavigations).forEach((navigation) => {
navigation.navigate(screen, params);
});
};

export let topicToNavigateTo: string | undefined = undefined;
export const setTopicToNavigateTo = (topic: string | undefined) => {
topicToNavigateTo = topic;
// todo(any): figure out proper typing here
// @ts-ignore
converseNavigatorRef.navigate(screen, params);
};

export const navigateToTopic = async (topic: ConversationTopic) => {
Expand Down
Loading