feat: Notification Subscriptions forV3 #1258
Conversation
Added notification subscribing for V3 only
WalkthroughThe pull request introduces significant changes across several files, primarily focusing on the restructuring of notification handling and conversation management within the chat and settings stores. Key alterations include the removal of certain subscription functions, modifications to how conversations are tracked, and enhancements to the notification subscription logic. Additionally, updates to method signatures and type definitions improve clarity and maintainability. The changes reflect a comprehensive refactoring aimed at streamlining interactions and improving error handling in the notification and synchronization processes. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| return { | ||
| "xmtp-api-signature": "123", | ||
| "xmtp-api-address": account, | ||
| "xmtp-api-address": evmHelpers.toChecksumAddress(account), | ||
| }; |
There was a problem hiding this comment.
The hardcoded API signature "123" should be removed to allow the proper signature generation logic below to execute. This appears to be debug/test code that was accidentally committed. The checksummed address change looks correct, but bypassing authentication is a security risk in production.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
utils/xmtpRN/api.ts (1)
Critical: Fix unreachable code and incorrect signature handling
The verification confirms this is a critical issue that needs immediate attention. The
getXmtpApiHeadersfunction is used extensively across the codebase for API authentication in critical operations like:
- User authentication and existence checks
- Peer status and consent management
- Topic management and pinning
- USDC transfer authorization
- Group management and invites
The current implementation with the early return and hardcoded "123" signature will break all these operations. Additionally, the codebase shows both mobile platforms (iOS and Android) expect proper XMTP API signatures for authentication.
Fix needed as suggested in the review:
export const getXmtpApiHeaders = async (account: string) => { - return { - "xmtp-api-signature": "123", - "xmtp-api-address": evmHelpers.toChecksumAddress(account), - }; if (account in xmtpSignatureByAccount) return { "xmtp-api-signature": xmtpSignatureByAccount[account], - "xmtp-api-address": account, + "xmtp-api-address": evmHelpers.toChecksumAddress(account), }; const xmtpApiSignature = await getXmtpApiSignature(account, "XMTP_IDENTITY"); xmtpSignatureByAccount[account] = xmtpApiSignature; return { "xmtp-api-signature": xmtpApiSignature, - "xmtp-api-address": account, + "xmtp-api-address": evmHelpers.toChecksumAddress(account), }; };🔗 Analysis chain
Line range hint
46-60: Critical: Fix unreachable code and incorrect signature handlingThere's a severe control flow issue in the
getXmtpApiHeadersfunction:
- The early return with hard-coded "123" signature makes all subsequent code unreachable
- The XMTP signature generation and caching logic is completely bypassed
- This will break authentication as the signature is invalid
Apply this fix to restore the proper authentication flow:
export const getXmtpApiHeaders = async (account: string) => { - return { - "xmtp-api-signature": "123", - "xmtp-api-address": evmHelpers.toChecksumAddress(account), - }; if (account in xmtpSignatureByAccount) return { "xmtp-api-signature": xmtpSignatureByAccount[account], - "xmtp-api-address": account, + "xmtp-api-address": evmHelpers.toChecksumAddress(account), }; const xmtpApiSignature = await getXmtpApiSignature(account, "XMTP_IDENTITY"); xmtpSignatureByAccount[account] = xmtpApiSignature; return { "xmtp-api-signature": xmtpApiSignature, - "xmtp-api-address": account, + "xmtp-api-address": evmHelpers.toChecksumAddress(account), }; };Let's verify the usage of this authentication header across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for places where getXmtpApiHeaders is used to ensure they handle the async nature rg -l "getXmtpApiHeaders" | xargs rg -A 5 "getXmtpApiHeaders" # Search for any direct XMTP API calls to ensure they use proper authentication rg -A 5 "xmtp-api-(signature|address)"Length of output: 8586
utils/xmtpRN/sync.ts (1)
Line range hint
177-184: Add notification subscription cleanup.The
deleteXmtpClientfunction should clean up notification subscriptions when removing a client.Consider adding cleanup for notification subscriptions:
export const deleteXmtpClient = async (account: string) => { if (account in xmtpClientByAccount) { stopStreamingAllMessage(account); stopStreamingConversations(account); + try { + await subscribeToNotifications({ conversations: [], account }); + } catch (e) { + logger.error(e, { + context: `Failed to cleanup notifications for ${account}`, + }); + } } delete xmtpClientByAccount[account]; delete xmtpSignatureByAccount[account];utils/notifications.ts (2)
67-71: Avoid unnecessary type assertionCasting the result of
getXmtpClient(account)toConverseXmtpClientTypeusingasmight mask potential issues. Consider refininggetXmtpClientto return the correct type or properly handle the possibility of an undefined client.If
getXmtpClientcan returnnullorundefined, adjust its return type accordingly and handle it without type assertion.-const client = (await getXmtpClient(account)) as ConverseXmtpClientType; +const client = await getXmtpClient(account); if (!client) { logger.error("[subscribeToNotifications] no client"); return; } // Now `client` is guaranteed to be of type `ConverseXmtpClientType`
99-108: Ensureconversation.topicandconversation.stateare definedAdd checks to handle cases where
conversation.topicorconversation.statemight be undefined to prevent potential runtime errors.Modify the loop to include validation:
for (const conversation of conversations) { + if (!conversation.topic || !conversation.state) { + logger.warn("[subscribeToNotifications] Conversation missing topic or state", conversation); + continue; + } const topic = conversation.topic; const conversationState = conversation.state; if (conversationState === "allowed") { dataToHash.push.push(topic); topicsToUpdateForPeriod[topic] = { status: "PUSH", }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
data/store/chatStore.ts(0 hunks)data/store/settingsStore.ts(0 hunks)utils/notifications.ts(3 hunks)utils/xmtpRN/api.ts(2 hunks)utils/xmtpRN/sync.ts(2 hunks)
💤 Files with no reviewable changes (2)
- data/store/chatStore.ts
- data/store/settingsStore.ts
🔇 Additional comments (7)
utils/xmtpRN/api.ts (2)
6-6: LGTM: Import statement is correctly placed
The new import for evmHelpers is properly structured and follows the existing import organization.
47-47: LGTM: Improved address handling with checksum
Using evmHelpers.toChecksumAddress() is a good practice as it ensures consistent address formatting and helps prevent errors in address handling.
utils/xmtpRN/sync.ts (1)
21-24: Verify performance impact of switching from prefetch to fetch.
The change from prefetchConversationListQuery to fetchConversationListQuery might affect the application's performance characteristics, particularly regarding data loading timing.
Let's analyze the implementation differences:
✅ Verification successful
No performance concerns with fetch vs prefetch change
The change from prefetchConversationListQuery to fetchConversationListQuery is an intentional refactoring that doesn't impact performance. Both functions use the same underlying query function v3ConversationListQueryFn with different caller identifiers. The key differences are:
prefetchConversationListQueryusesqueryClient.prefetchQueryfetchConversationListQueryusesqueryClient.fetchQuery
Both methods are valid React Query patterns, and the change aligns with the immediate data needs in sync.ts where the conversation list is required synchronously for notification subscription.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining prefetch implementations and compare with new fetch pattern
rg "prefetchConversationList" -A 5
rg "fetchConversationList" -A 5
Length of output: 4029
utils/notifications.ts (4)
7-7: Import statement is necessary and correctly added
The import saveNotificationsSubscribe from ./api is appropriately added and used in the code.
11-16: New imports are correctly utilized
The imports from xmtpRN modules (getXmtpClient, ConversationWithCodecsType, ConverseXmtpClientType) and logger are correctly added and used within the code.
49-51: Type definition SubscribeToNotificationsParams enhances clarity
Defining SubscribeToNotificationsParams improves type safety and readability of the subscribeToNotifications function parameters.
29-30: 🛠️ Refactor suggestion
Validate installationId before constructing the topic
If installationId is an empty string, the constructed topic may be invalid (e.g., /xmtp/mls/1/w-/proto). Consider adding validation to ensure installationId is not empty before using it.
You could modify the code to check for a valid installationId:
const buildUserV3InviteTopic = (installationId: string): string => {
+ if (!installationId) {
+ logger.error("Installation ID is missing");
+ throw new Error("Installation ID is required");
+ }
return `/xmtp/mls/1/w-${installationId}/proto`;
};And ensure that client.installationId is defined before calling:
const client = await getXmtpClient(account);
if (!client) {
logger.error("[subscribeToNotifications] no client");
return;
}
+if (!client.installationId) {
+ logger.error("[subscribeToNotifications] client.installationId is missing");
+ return;
+}
const userGroupInviteTopic = buildUserV3InviteTopic(client.installationId);| const conversationList = await fetchConversationListQuery(account); | ||
| subscribeToNotifications({ conversations: conversationList, account }); |
There was a problem hiding this comment.
Add error handling and consider race conditions.
The current implementation has two potential issues:
- No error handling for
subscribeToNotifications - Possible race condition between fetching conversations and subscribing to notifications
Consider wrapping the subscription in a try-catch and implementing a more atomic operation:
- const conversationList = await fetchConversationListQuery(account);
- subscribeToNotifications({ conversations: conversationList, account });
+ try {
+ const conversationList = await fetchConversationListQuery(account);
+ await subscribeToNotifications({ conversations: conversationList, account });
+ } catch (e) {
+ logger.error(e, {
+ context: `Failed to subscribe to notifications for ${account}`,
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const conversationList = await fetchConversationListQuery(account); | |
| subscribeToNotifications({ conversations: conversationList, account }); | |
| try { | |
| const conversationList = await fetchConversationListQuery(account); | |
| await subscribeToNotifications({ conversations: conversationList, account }); | |
| } catch (e) { | |
| logger.error(e, { | |
| context: `Failed to subscribe to notifications for ${account}`, | |
| }); | |
| } |
| const nativeTokenQuery = await Notifications.getDevicePushTokenAsync(); | ||
| nativePushToken = nativeTokenQuery.data; | ||
| if (nativePushToken) { | ||
| savePushToken(nativePushToken); | ||
| } else { | ||
| logger.error("[subscribeToNotifications] no native push token"); | ||
| delete subscribingByAccount[account]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle possible undefined nativeTokenQuery
Ensure nativeTokenQuery and nativeTokenQuery.data are properly checked to avoid runtime errors if getDevicePushTokenAsync fails.
Adjust the code to handle undefined nativeTokenQuery:
const nativeTokenQuery = await Notifications.getDevicePushTokenAsync();
-nativePushToken = nativeTokenQuery.data;
+nativePushToken = nativeTokenQuery?.data;
if (nativePushToken) {
savePushToken(nativePushToken);
} else {
logger.error("[subscribeToNotifications] no native push token");
delete subscribingByAccount[account];
return;
}This change ensures that if nativeTokenQuery is undefined, nativePushToken will be undefined, and the subsequent check will handle it appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nativeTokenQuery = await Notifications.getDevicePushTokenAsync(); | |
| nativePushToken = nativeTokenQuery.data; | |
| if (nativePushToken) { | |
| savePushToken(nativePushToken); | |
| } else { | |
| logger.error("[subscribeToNotifications] no native push token"); | |
| delete subscribingByAccount[account]; | |
| return; | |
| } | |
| const nativeTokenQuery = await Notifications.getDevicePushTokenAsync(); | |
| nativePushToken = nativeTokenQuery?.data; | |
| if (nativePushToken) { | |
| savePushToken(nativePushToken); | |
| } else { | |
| logger.error("[subscribeToNotifications] no native push token"); | |
| delete subscribingByAccount[account]; | |
| return; | |
| } |
| const thirtyDayPeriodsSinceEpoch = Math.floor( | ||
| Date.now() / 1000 / 60 / 60 / 24 / 30 | ||
| ); | ||
| const client = (await getXmtpClient(account)) as ConverseXmtpClientType; |
There was a problem hiding this comment.
I see we do A LOT of those const client = (await getXmtpClient(account)) as ConverseXmtpClientType; could we make it simpler and at least not need the as ... just curious
There was a problem hiding this comment.
Yeah I agree we don't need it, it's there because of old web stuff, so we should probably remove it
| await saveNotificationsSubscribe( | ||
| account, | ||
| nativePushToken, | ||
| nativeTokenQuery.type, | ||
| nativeTokenQuery.type === "android" ? "converse-notifications" : null, | ||
| topicsToUpdateForPeriod | ||
| ); |
There was a problem hiding this comment.
Many params , would love a object instead 👀
| const conversationList = await prefetchConversationListQuery(account); | ||
| // TODO: Update notification topics | ||
| const conversationList = await fetchConversationListQuery(account); | ||
| subscribeToNotifications({ conversations: conversationList, account }); | ||
| } catch (e) { | ||
| logger.error(e, { | ||
| context: `Failed to fetch persisted conversation list for ${account}`, |
There was a problem hiding this comment.
would be cool to start handling errors better. Maybe worth waiting until we have a clear strategy tho
Added notification subscribing for V3 only
Added notification subscribing for V3 only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores