feat: React Native 75 #1267
feat: React Native 75 #1267
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces various updates across multiple configuration files for an Android and iOS project. Key changes include the removal of specific properties from the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 38
🧹 Outside diff range and nitpick comments (82)
features/notifications/utils/resetNotifications.ts (3)
4-6: Add input validation for timeout parameterThe timeout parameter should be validated to ensure it's non-negative.
Consider adding this validation:
export const resetNotifications = async ( timeout: number = 0 ): Promise<void> => { + if (timeout < 0) { + throw new Error('Timeout must be non-negative'); + }
1-2: Consider extracting storage key to constantsThe "notifications-badge" key is hardcoded. Consider moving it to a constants file for better maintainability.
Create a constants file (if not exists) and move the key there:
// features/notifications/constants.ts export const STORAGE_KEYS = { NOTIFICATIONS_BADGE: 'notifications-badge' } as const;
4-12: Consider making timeout wrapper optionalThe setTimeout wrapper might not be necessary for all use cases and adds complexity.
Consider splitting this into two functions for better separation of concerns:
export const resetNotifications = async (): Promise<void> => { try { await Notifications.dismissAllNotificationsAsync(); await Notifications.setBadgeCountAsync(0); mmkv.set("notifications-badge", 0); } catch (error) { console.error('Failed to reset notifications:', error); throw error; } }; export const resetNotificationsWithDelay = async (timeout: number): Promise<void> => { if (timeout <= 0) return resetNotifications(); return new Promise((resolve) => { setTimeout(async () => { await resetNotifications(); resolve(); }, timeout); }); };features/notifications/utils/deleteSubscribedTopics.ts (2)
1-2: Consider adding type definitions for imported objectsThe imported objects
subscribedOnceByAccountandsubscribingByAccountshould have explicit TypeScript interfaces to ensure type safety and better code documentation.interface SubscriptionMap { [account: string]: boolean | undefined; } // Add to respective files: export const subscribedOnceByAccount: SubscriptionMap = {}; export const subscribingByAccount: SubscriptionMap = {};
4-11: Add unit tests for the deletion logicEnsure proper test coverage for various scenarios including:
- Valid account deletion
- Invalid account handling
- Object state verification after deletion
Would you like me to help create unit tests for this utility function?
android/react-settings-plugin/build.gradle.kts (1)
8-10: Consider adding Google's Maven repositoryFor React Native Android projects, it's recommended to include Google's Maven repository as some Android dependencies might be required.
repositories { mavenCentral() + google() }features/notifications/utils/getNotificationsPermissionStatus.ts (1)
5-7: Add JSDoc documentation for clarityConsider adding documentation to describe the possible permission status values and when undefined might be returned.
+/** + * Gets the current notification permission status for the application. + * @returns Promise resolving to one of the following status values: + * - 'granted': User has granted permission + * - 'denied': User has denied permission + * - 'undetermined': Permission hasn't been requested yet + * - undefined: In case of errors during permission check + */ export const getNotificationsPermissionStatus = async (): Promise< NotificationPermissionStatus | undefined > => {features/notifications/utils/saveNotificationsStatus.ts (2)
1-2: Consider adding type imports for better type safety.Add explicit type imports for the notification status values to improve type safety and maintainability.
import { useAppStore } from "@data/store/appStore"; import { getNotificationsPermissionStatus } from "./getNotificationsPermissionStatus"; +import { NotificationPermissionStatus } from "../types/Notifications.types";
4-15: Consider adding retry logic for permission status updates.Since this function deals with system permissions, which can be flaky, consider adding retry logic for robustness.
Here's a suggested implementation with retry logic:
const MAX_RETRIES = 3; const RETRY_DELAY = 1000; // 1 second export const saveNotificationsStatus = async (): Promise<void> => { let lastError: Error | undefined; for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { try { const notificationsStatus = await getNotificationsPermissionStatus(); const { setNotificationsPermissionStatus } = useAppStore.getState(); setNotificationsPermissionStatus(notificationsStatus); return; } catch (error) { lastError = error as Error; if (attempt < MAX_RETRIES) { await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); } } } console.error('Failed to save notifications status after retries:', lastError); throw lastError; };features/notifications/utils/background/getGroup.ts (3)
8-14: Consider adding timeout handling and loading state management.While the core logic is sound, consider these improvements:
- Add timeout handling for network operations
- Consider implementing a loading state for UI feedback
- Evaluate if both sync operations are always necessary
Example implementation:
export const getGroup = async ( xmtpClient: ConverseXmtpClientType, - groupId: string + groupId: string, + options?: { timeout?: number } ) => { try { - await xmtpClient.conversations.syncGroups(); + const syncPromise = xmtpClient.conversations.syncGroups(); + if (options?.timeout) { + await Promise.race([ + syncPromise, + new Promise((_, reject) => + setTimeout(() => reject(new Error('Sync timeout')), options.timeout) + ) + ]); + } else { + await syncPromise; + } const group = await xmtpClient.conversations.findGroup(groupId); if (group) { await group.sync(); return group; } }
22-23: Consider using a more descriptive return type.The current null return doesn't provide enough context about why the operation failed.
Consider using a Result type:
type GroupResult = { group: any | null; error?: { type: 'NOT_FOUND' | 'SYNC_FAILED' | 'NETWORK_ERROR'; message: string; }; }; export const getGroup = async ( xmtpClient: ConverseXmtpClientType, groupId: string ): Promise<GroupResult> => { // ... existing try block ... return { group: null, error: { type: 'SYNC_FAILED', message: error.message } }; };
4-23: Add JSDoc documentation for the function.Consider adding documentation to describe the function's purpose, parameters, return value, and possible errors.
Example:
/** * Retrieves and synchronizes a group from the XMTP network. * * @param xmtpClient - The XMTP client instance * @param groupId - The unique identifier of the group to retrieve * @returns The synchronized group object or null if not found/error occurs * @throws Will not throw, returns null on any error */ export const getGroup = async (features/notifications/utils/requestPushNotificationsPermissions.ts (2)
1-4: Consider specific imports for better tree-shaking.Instead of using a wildcard import for
expo-notifications, consider importing only the required functions.-import * as Notifications from "expo-notifications"; +import { getPermissionsAsync, requestPermissionsAsync } from "expo-notifications";
5-7: Remove undefined from return type.The function never returns undefined, so the return type should be simplified.
-export const requestPushNotificationsPermissions = async (): Promise< - NotificationPermissionStatus | undefined -> => { +export const requestPushNotificationsPermissions = async (): Promise<NotificationPermissionStatus> => {features/notifications/utils/setupAndroidNotificationChannel.ts (3)
1-3: Consider consolidating notification librariesThe code currently uses both
expo-notificationsand@notifee/react-native. While this is valid during migration, consider fully transitioning to@notifee/react-nativeto avoid potential conflicts and reduce bundle size.
5-8: Consider enhancing the channel configurationThe
androidChannelobject could be more robust by:
- Adding TypeScript interface/type definitions
- Including all channel settings (lights, vibration) for better reusability
Consider applying this improvement:
+interface AndroidChannelConfig { + id: string; + name: string; + lights?: boolean; + vibration?: boolean; +} -export const androidChannel = { +export const androidChannel: AndroidChannelConfig = { id: "converse-notifications", name: "Converse Notifications", + lights: false, + vibration: true, };
16-21: Remove commented-out codeThe commented-out
Notifications.setNotificationChannelAsynccode should be removed as it's been replaced by the notifee implementation.features/notifications/utils/background/handleBackgroundNotification.ts (2)
14-25: Consider enhancing error handling and type safetyWhile the error handling is good, there are a few improvements that could be made:
Consider applying these changes:
-export const handleBackgroundNotification = async ( +export const handleBackgroundNotification = async ( rawBody: string | undefined -) => { +): Promise<void> => { let objectBody: unknown = {}; if (rawBody) { try { objectBody = JSON.parse(rawBody); } catch (e) { - logger.error(`Failed to parse notification body: ${e}`); + logger.error(`Failed to parse notification body: ${e}. Raw body: ${rawBody}`); return; } }
14-16: Add JSDoc documentation for the functionConsider adding documentation to explain the function's purpose, parameters, and behavior:
+/** + * Handles background notifications by parsing and routing them to appropriate handlers. + * Protocol notifications are ignored when the app is in the foreground as they are handled by the XMTP SDK. + * + * @param rawBody - The raw notification payload as a string, if available + * @returns Promise<void> + */ export const handleBackgroundNotification = async ( rawBody: string | undefined ) => {features/notifications/utils/background/alreadyShown.ts (1)
24-50: Improve function clarity and efficiencyThe notification check implementation has several areas for improvement:
- The function name doesn't indicate it has side effects
- The MAX_STORED_IDS constant might need adjustment based on actual usage patterns
- The array manipulation could be more efficient
Consider these improvements:
-const MAX_STORED_IDS = 10; +// Maximum number of notification IDs to store in history +// TODO: Adjust this value based on actual usage patterns and memory constraints +const MAX_STORED_IDS = 10; + +/** + * Checks if a notification has been shown before and updates the shown notifications history. + * @param messageId - The unique identifier of the notification + * @returns true if the notification should be skipped (already shown), false otherwise + */ export const notificationAlreadyShown = (messageId?: string): boolean => { // Check if the messageId is not undefined/null and not an empty string if (!messageId?.trim()) { return true; } // If the id already exists in the list, don't show the notification const existingIds = getShownNotificationIds(); if (existingIds.includes(messageId)) { return true; } // Append the new id to the list - const updatedIds = [...existingIds, messageId]; - - // If the list exceeds the maximum limit, trim from the beginning - if (updatedIds.length > MAX_STORED_IDS) { - updatedIds.splice(0, updatedIds.length - MAX_STORED_IDS); - } + // Keep only the most recent IDs using slice + const updatedIds = [...existingIds.slice(-(MAX_STORED_IDS - 1)), messageId]; // Store the updated list of IDs back to storage setShownNotificationIds(updatedIds);Also, consider renaming the function to better indicate its side effects:
-export const notificationAlreadyShown +export const checkAndRecordNotificationShownfeatures/notifications/utils/background/converseNotification.ts (3)
1-10: LGTM! Well-structured imports with good practices.The imports are well-organized and demonstrate good practices:
- Using Zod for schema validation ensures type safety
- Including a logger shows attention to observability
Consider creating a custom error class for notification-specific errors to improve error handling and debugging:
export class NotificationError extends Error { constructor(message: string, public notification: ConverseNotification) { super(message); this.name = 'NotificationError'; } }
11-16: LGTM! Well-designed extensible notification schema.The use of Zod's discriminated union creates a maintainable and type-safe foundation for handling different notification types.
Consider enhancing the documentation for future maintainers:
-// Add other converse notifications here, with different "type" values +/** + * Schema for all Converse notifications. + * To add a new notification type: + * 1. Create a new notification schema in a separate file + * 2. Import and add it to this discriminated union + * 3. Add corresponding handler logic in handleConverseNotification + */
1-41: Consider additional architectural safeguards.As this code runs in a React Native background context, consider implementing these safeguards:
- Add rate limiting to prevent notification processing overload
- Implement timeouts for long-running operations
- Handle background task lifecycle appropriately
Consider implementing a rate limiter:
import { RateLimiter } from 'limiter'; // Allow 10 notifications per minute const limiter = new RateLimiter({ tokensPerInterval: 10, interval: 'minute' }); export const handleConverseNotification = async ( notification: ConverseNotification ): Promise<void> => { if (!await limiter.tryRemoveTokens(1)) { logger.warn('[ConverseNotification] Rate limit exceeded, skipping notification'); return; } // ... rest of the implementation };android/settings.gradle (2)
Line range hint
26-42: Apply consistent error handling for node commandsSimilar to the earlier suggestion, all node command executions should include error handling to fail gracefully with meaningful error messages.
dependencyResolutionManagement { versionCatalogs { reactAndroidLibs { - from(files(new File(["node", "--print", "require.resolve('react-native/package.json')"].execute(null, rootDir).text.trim(), "../gradle/libs.versions.toml"))) + def result = ["node", "--print", "require.resolve('react-native/package.json')"].execute(null, rootDir) + result.waitFor() + if (result.exitValue() != 0) { + throw new GradleException("Failed to resolve react-native: ${result.err.text}") + } + from(files(new File(result.text.trim(), "../gradle/libs.versions.toml"))) } } }The same pattern should be applied to the remaining node command executions in this file.
Line range hint
1-42: Consider creating a Gradle utility class for node command executionGiven the repeated pattern of node command executions throughout the file, consider creating a utility class to handle these operations consistently. This would:
- Centralize error handling
- Provide consistent logging
- Make the code more maintainable
- Reduce duplication
Would you like me to provide an example implementation of such a utility class?
features/notifications/utils/background/protocolNotification.ts (1)
15-19: Consider enhancing schema validationWhile the current schema provides basic validation, consider these improvements:
- Make the Ethereum address regex case-insensitive for better flexibility
- Add specific validation for
contentTopicformat to ensure proper message routingexport const ProtocolNotificationSchema = z.object({ - account: z.string().regex(/^0x[a-fA-F0-9]{40}$/), + account: z.string().regex(/^0x[a-fA-F0-9]{40}$/i), - contentTopic: z.string(), + contentTopic: z.string().refine( + (topic) => isGroupMessageContentTopic(topic) || isGroupWelcomeContentTopic(topic), + "Invalid content topic format" + ), message: z.string(), });features/notifications/utils/handleForegroundNotification.ts (1)
47-55: LGTM! Clean platform-specific implementation.The handler properly uses Platform.select for conditional execution. Consider enhancing the log message with more details about the notification for better debugging.
- logger.info("A NOTIFICATION WAS RECEIVED WHILE THE APP WAS FOREGROUNDED"); + logger.info( + `Foreground notification received: ${notification.request.identifier}`, + { content: notification.request.content } + );queries/useGroupMembersQuery.ts (2)
Line range hint
37-39: Consider implementing multiple addresses supportThe TODO comment indicates pending work for multiple addresses support. This could be important for the React Native 75 upgrade.
Would you like me to help create a GitHub issue to track the implementation of multiple addresses support? This could include:
- Supporting multiple addresses in the
getCleanAddresscall- Updating the type definitions
- Adding validation for the addresses array
Line range hint
33-39: Add error handling for the members() callThe async call to
members()should include error handling to gracefully handle API failures.Consider implementing error handling:
const members = await group.members(); +if (!members) { + throw new Error('Failed to fetch group members'); +} +if (!Array.isArray(members)) { + throw new Error('Invalid response format for group members'); +} return entifyWithAddress( members, (member) => member.inboxId, (member) => getCleanAddress(member.addresses[0]) );features/notifications/utils/background/notificationContent.ts (3)
13-16: Consider adding return type annotation.The function's return type should be explicitly defined for better type safety.
export const getNotificationContent = async ( group: GroupWithCodecsType, message: DecodedMessageWithCodecsType -) => { +): Promise<string | undefined> => {
22-28: Track the TODO implementation for replies.There's a pending implementation for replies that needs to be tracked.
Would you like me to create a GitHub issue to track the implementation of reply notifications once the referenced issue #409 is resolved?
37-51: Simplify reaction handling logic.The reaction handling code could be more concise and maintainable.
- let { action, reference, schema, content } = - messageContent as ReactionContent; - referencedMessageId = reference; - - if (action === "added" && schema === "unicode") { - // Checking if the group message is from me - const isFromMe = await isGroupMessageFromMe( - group.client, - referencedMessageId - ); - if (!isFromMe) return; - return content - ? `Reacted ${content} to a message` - : "Reacted to a message"; - } + const { action, reference, schema, content } = messageContent as ReactionContent; + if (action !== "added" || schema !== "unicode") return; + + const isFromMe = await isGroupMessageFromMe(group.client, reference); + if (!isFromMe) return; + + return content ? `Reacted ${content} to a message` : "Reacted to a message";features/notifications/utils/onInteractWithNotification.ts (1)
1-67: Consider implementing a notification handling strategy pattern.The current implementation handles multiple notification types in a single function, which could become harder to maintain as more notification types are added. Consider implementing a strategy pattern to handle different notification types separately.
Example structure:
interface NotificationHandler { canHandle(notification: Notifications.NotificationResponse): boolean; handle(notification: Notifications.NotificationResponse): void; } class NotifeeNotificationHandler implements NotificationHandler { canHandle(notification) { return "notifee_event_type" in notification.notification.request.content.data; } handle(notification) { // Notifee specific handling } } class GroupJoinRequestHandler implements NotificationHandler { canHandle(notification) { // Check for group join request } handle(notification) { // Group join specific handling } }This would make the code more maintainable and easier to test. Would you like me to create an issue to track this refactoring?
screens/NotificationsScreen.tsx (2)
Line range hint
35-46: Add missing NotificationPermissionStatus type import.The permission handling logic uses the "denied" status, but the NotificationPermissionStatus type import mentioned in the summary is missing. This could lead to type-safety issues.
Add the following import:
+import { NotificationPermissionStatus } from "../features/notifications/types/Notifications.types";
Line range hint
38-41: Consider adding explicit Android version check.The comment mentions specific behavior for Android 13, but there's no explicit version check. This could lead to unexpected behavior on different Android versions.
Consider adding a version check:
- if (newStatus === "denied" && Platform.OS === "android") { + if (newStatus === "denied" && Platform.OS === "android" && Platform.Version >= 33) {queries/useGroupQuery.ts (1)
Line range hint
57-59: Track multiple addresses support implementationThe TODO comment indicates pending work for multiple addresses support. This could be important for the React Native 75 upgrade.
Would you like me to create a GitHub issue to track the implementation of multiple addresses support?
screens/Onboarding/OnboardingNotificationsScreen.tsx (1)
Line range hint
73-89: Consider improving error handling and user feedbackThe error handling could be enhanced in several ways:
- The catch block silently swallows errors (only tracks them)
setAuthStatus("signedIn")is called in finally block regardless of success/failure- No loading state during the async operation
Consider this improvement:
<OnboardingPrimaryCtaButton title="Accept notifications" + loading={isLoading} onPress={async () => { + setIsLoading(true); try { const newStatus = await requestPushNotificationsPermissions(); if (!newStatus) return; if (newStatus === "denied" && Platform.OS === "android") { Linking.openSettings(); } else { setNotificationsSettings({ showNotificationScreen: false }); } setNotificationsPermissionStatus(newStatus); + setAuthStatus("signedIn"); } catch (error) { sentryTrackError(error); + // Show error to user + Alert.alert('Error', 'Failed to setup notifications. Try again later.'); } finally { - setAuthStatus("signedIn"); + setIsLoading(false); } }} />hooks/useGroupConsent.ts (2)
Line range hint
38-63: Consider batching store updates for better atomicityThe function makes multiple separate store updates (
setGroupStatusandsetInboxIdPeerStatus). Consider batching these updates to prevent potential race conditions.- setGroupStatus({ [getGroupIdFromTopic(topic).toLowerCase()]: "allowed" }); const inboxIdsToAllow: InboxId[] = []; const inboxIds: { [inboxId: string]: "allowed" } = {}; // ... inbox ID collection logic ... - setInboxIdPeerStatus(inboxIds); + const updates = { + groupStatus: { [getGroupIdFromTopic(topic).toLowerCase()]: "allowed" }, + inboxIdPeerStatus: inboxIds + }; + setGroupStatus(updates.groupStatus); + setInboxIdPeerStatus(updates.inboxIdPeerStatus);
Line range hint
65-90: Consider extracting shared logic to reduce duplicationThe
blockGroupandallowGroupfunctions share similar patterns. Consider extracting the common logic into a shared utility function.type ConsentAction = 'allow' | 'deny'; const handleGroupConsent = ( action: ConsentAction, options: OnConsentOptions, group: any, groupCreator: string | undefined, topic: string ) => { const status = action === 'allow' ? 'allowed' : 'denied'; const inboxIds: { [inboxId: string]: typeof status } = {}; const inboxIdsToProcess: InboxId[] = []; if (options.includeAddedBy && group?.addedByInboxId) { const addedBy = group.addedByInboxId; inboxIds[addedBy] = status; inboxIdsToProcess.push(addedBy); } if (options.includeCreator && groupCreator) { inboxIds[groupCreator] = status; inboxIdsToProcess.push(groupCreator); } return { inboxIds, inboxIdsToProcess }; };utils/navigation.ts (3)
9-9: LGTM! Good architectural improvement.The import path change reflects a better organization of features, following a modular architecture pattern where notification-related functionality is properly isolated.
Line range hint
38-41: Add error handling for notification context loading.The async call to
loadSavedNotificationMessagesToContextlacks error handling. If this fails, it could affect the navigation flow without any indication of the failure.Consider adding try-catch block:
export const navigateToConversation = async ( conversation: XmtpConversation ) => { - await loadSavedNotificationMessagesToContext(); - navigate("Conversation", { topic: conversation.topic }); + try { + await loadSavedNotificationMessagesToContext(); + navigate("Conversation", { topic: conversation.topic }); + } catch (error) { + logger.error('[Navigation] Failed to load notification context:', error); + // Still navigate to maintain core functionality + navigate("Conversation", { topic: conversation.topic }); + } };
Line range hint
43-57: Consider enhancing the retry mechanism.While the formatting improvements are good, the retry mechanism could be enhanced:
- Consider making retry attempts and delay configurable
- Implement exponential backoff for better resilience
Here's a suggested improvement:
+const RETRY_CONFIG = { + maxAttempts: 10, + initialDelay: 250, + maxDelay: 2000 +}; + export const navigateToTopicWithRetry = async () => { if (!topicToNavigateTo) return; let conversationToNavigateTo = getChatStore(currentAccount()).getState().conversations[topicToNavigateTo]; let currentAttempt = 0; while ( !conversationToNavigateTo && - currentAttempt < 10 && + currentAttempt < RETRY_CONFIG.maxAttempts && topicToNavigateTo ) { currentAttempt += 1; - await new Promise((r) => setTimeout(r, 250)); + const delay = Math.min( + RETRY_CONFIG.initialDelay * Math.pow(2, currentAttempt - 1), + RETRY_CONFIG.maxDelay + ); + await new Promise((r) => setTimeout(r, delay)); conversationToNavigateTo = getChatStore(currentAccount()).getState().conversations[ topicToNavigateTo ]; }ios/Podfile (1)
41-41: Minor: Remove trailing commaThe trailing comma after the last parameter in
use_react_native!can be removed.- :privacy_file_aggregation_enabled => podfile_properties['apple.privacyManifestAggregationEnabled'] == 'false', + :privacy_file_aggregation_enabled => podfile_properties['apple.privacyManifestAggregationEnabled'] == 'false'🧰 Tools
🪛 rubocop (1.68.0)
[convention] 41-41: Avoid comma after the last parameter of a method call.
(Style/TrailingCommaInArguments)
components/Chat/ConsentPopup/GroupConsentPopup.tsx (2)
Line range hint
28-32: Enhance error handling and type safetyThe error handling could be improved with:
- More descriptive error message including the missing requirements
- Type guards instead of non-null assertions
- Proper TypeScript types for conversation prop
-if (!conversation?.isGroup || !conversation?.topic || !currentAccount) { - throw new Error("This component should only be used for group chats"); +if (!conversation) { + throw new Error("GroupConsentPopup requires a conversation context"); +} +if (!conversation.isGroup || !conversation.topic) { + throw new Error("GroupConsentPopup can only be used within group chats"); +} +if (!currentAccount) { + throw new Error("GroupConsentPopup requires a current account"); }
Line range hint
77-82: Add error handling to onAccept callbackThe
onAcceptcallback should include error handling similar toonBlockto manage failed group join attempts.const onAccept = useCallback(() => { - allowGroup({ - includeCreator: false, - includeAddedBy: false, - }); + try { + allowGroup({ + includeCreator: false, + includeAddedBy: false, + }); + } catch (error) { + // Handle error (e.g., show toast notification) + console.error('Failed to join group:', error); + } }, [allowGroup]);data/helpers/conversations/spamScore.ts (1)
128-144: Consider enhancing spam score computation logic.The current implementation uses binary scoring which might be too simplistic. Consider:
- Adding weights for different spam indicators
- Implementing a maximum score limit
- Adding more sophisticated URL analysis (e.g., whitelist domains)
+const SPAM_WEIGHTS = { + URL: 0.5, + RESTRICTED_WORD: 0.7, + MAX_SCORE: 1.5 +}; + export const computeMessageContentSpamScore = ( message: string, contentType: string ): number => { + /** + * Computes a weighted spam score based on various indicators: + * - URLs: 0.5 points + * - Restricted words: 0.7 points + * Maximum score is capped at 1.5 + */ let spamScore: number = 0.0; URL_REGEX.lastIndex = 0; if (isContentType("text", contentType)) { if (URL_REGEX.test(message)) { - spamScore += 1; + spamScore += SPAM_WEIGHTS.URL; } if (containsRestrictedWords(message)) { - spamScore += 1; + spamScore += SPAM_WEIGHTS.RESTRICTED_WORD; } } - return spamScore; + return Math.min(spamScore, SPAM_WEIGHTS.MAX_SCORE); };components/StateHandlers/NotificationsStateHandler.tsx (3)
Line range hint
28-32: Document the purpose of the 500ms delay in resetNotifications.The delay parameter's purpose isn't immediately clear. Consider adding a comment explaining why this delay is necessary.
- resetNotifications(500); + // Add 500ms delay to ensure notifications are properly cleared after state initialization + resetNotifications(500);
Line range hint
34-57: Consider splitting app state change handler into smaller functions.The app state change handler manages multiple concerns (logout, notifications, user saving). Consider extracting these into separate functions for better maintainability.
+ const handleAppBecomeActive = async () => { + executeLogoutTasks(); + saveNotificationsStatus(); + resetNotifications(500); + if (userAddress) { + saveUser(userAddress, privyAccountId[userAddress] as string); + } + }; + + const handleAppBecomeInactive = () => { + resetNotifications(); + }; + useEffect(() => { const subscription = AppState.addEventListener( "change", async (nextAppState) => { if ( nextAppState === "active" && appState.current.match(/inactive|background/) ) { - executeLogoutTasks(); - saveNotificationsStatus(); - resetNotifications(500); - if (userAddress) { - saveUser(userAddress, privyAccountId[userAddress] as string); - } + await handleAppBecomeActive(); } else { - resetNotifications(); + handleAppBecomeInactive(); } appState.current = nextAppState; } );
Line range hint
95-134: Consider performance optimizations for the refresh state comparison.The refresh state comparison logic is complex and runs on every state change. Consider these optimizations:
- Extract the comparison logic into a separate memoized function
- Use a deep comparison utility instead of manual field comparisons
- Memoize the updatePreview function with useCallback
+ const isRefreshStateChanged = useCallback((newState, oldState) => { + return ( + newState.account !== oldState.account || + newState.conversations !== oldState.conversations || + newState.topicsData !== oldState.topicsData || + newState.peersStatus !== oldState.peersStatus || + newState.lastUpdateAt !== oldState.lastUpdateAt || + newState.pinnedConversations !== oldState.pinnedConversations || + newState.groupStatus !== oldState.groupStatus + ); + }, []); + + const updatePreview = useCallback(async () => { + if (!hydrationDone) return; + const newRefreshState = { + account, + conversations: Object.keys(conversations).length, + topicsData: Object.keys(topicsData).length, + peersStatus: Object.keys(peersStatus) + .map((peer) => `${peer}-${peersStatus[peer]}`) + .join(","), + lastUpdateAt, + pinnedConversations: pinnedConversations.length, + groupStatus: Object.keys(groupStatus) + .map((groupId) => `${groupId}-${groupStatus[groupId]}`) + .join(","), + }; + + if (isRefreshStateChanged(newRefreshState, lastRefreshState.current)) { + lastRefreshState.current = newRefreshState; + await sortAndComputePreview( + conversations, + account, + topicsData, + peersStatus, + groupStatus, + pinnedConversations + ); + } + }, [ + hydrationDone, + account, + conversations, + topicsData, + peersStatus, + lastUpdateAt, + pinnedConversations, + groupStatus, + isRefreshStateChanged, + ]);components/AccountSettingsButton.tsx (3)
Line range hint
82-82: Extract notification permission logic to a helper functionThe TODO comment indicates this logic should be moved to a helper. This would improve reusability and maintain consistency across the application.
Consider creating a new utility function in the notifications feature directory:
+ // features/notifications/utils/handleNotificationPermissions.ts + export const handleNotificationPermissions = async ( + currentStatus: NotificationPermissionStatus, + setStatus: (status: NotificationPermissionStatus) => void + ) => { + if (currentStatus === "denied") { + if (Platform.OS === "android") { + const newStatus = await requestPushNotificationsPermissions(); + if (newStatus === "denied") { + await Linking.openSettings(); + } else if (newStatus) { + setStatus(newStatus); + } + } else { + await Linking.openSettings(); + } + } else if (currentStatus === "undetermined") { + const newStatus = await requestPushNotificationsPermissions(); + if (newStatus) { + setStatus(newStatus); + } + } + };
Line range hint
83-107: Improve error handling and comments for notification permissionsSeveral improvements could be made to the notification permission handling:
- The Android 13 comment needs updating as it's now part of the standard flow
- Error handling for Promise rejections is missing
- The logic could benefit from more explicit status handling
Consider these improvements:
- // Android 13 is always denied first so let's try to show + try { const newStatus = await requestPushNotificationsPermissions(); if (newStatus === "denied") { await Linking.openSettings(); } else if (newStatus) { setNotificationsPermissionStatus(newStatus); } + } catch (error) { + console.error('Failed to request notification permissions:', error); + // Handle error appropriately + }
Line range hint
134-147: Improve type safety in action sheet methodsThe type assertion
(methods as any)could be replaced with a more type-safe approach.Consider this improvement:
- const method = (methods as any)[options[selectedIndex]]; + type MethodKey = keyof typeof methods; + const selectedOption = options[selectedIndex] as MethodKey; + const method = methods[selectedOption];ios/ConverseNotificationExtension/Xmtp/Conversations.swift (1)
Line range hint
95-115: Plan cleanup of legacy keychain migration codeThe code contains a temporary migration path from keychain to MMKV storage, as indicated by the TODO comment. To maintain code clarity and reduce technical debt, consider:
- Adding a timeline for when this migration code can be safely removed
- Implementing analytics to track if any users are still hitting this migration path
- Creating a cleanup ticket to remove this code once migration is complete
Would you like me to help create a GitHub issue to track this cleanup task?
utils/logout/index.tsx (2)
19-27: Consider standardizing import pathsThe import statements mix absolute paths (using
@utils) and relative paths (using../../features). Consider standardizing all imports to use absolute paths for better maintainability and consistency.Example standardization:
-import { unsubscribeFromNotifications } from "../../features/notifications/utils/unsubscribeFromNotifications"; -import { deleteSubscribedTopics } from "../../features/notifications/utils/deleteSubscribedTopics"; -import { lastNotifSubscribeByAccount } from "../../features/notifications/utils/lastNotifSubscribeByAccount"; +import { unsubscribeFromNotifications } from "@features/notifications/utils/unsubscribeFromNotifications"; +import { deleteSubscribedTopics } from "@features/notifications/utils/deleteSubscribedTopics"; +import { lastNotifSubscribeByAccount } from "@features/notifications/utils/lastNotifSubscribeByAccount";
Line range hint
1-254: Consider adding comprehensive tests for logout scenariosGiven the complexity of the logout process (handling databases, notifications, states, etc.), it would be beneficial to add comprehensive tests covering:
- Successful logout scenarios
- Error handling cases
- Race conditions with executeLogoutTasks
- Edge cases with temporary accounts
This is particularly important given the React Native 75 upgrade to ensure backward compatibility.
Would you like me to help create a test suite for these scenarios?
android/app/build.gradle (1)
154-158: Review build optimization settingsThe new build configuration introduces conditional resource shrinking and PNG optimization. These settings can significantly impact:
- APK size
- Build time
- Runtime performance
Consider documenting the recommended property values in your README.md for consistent builds across the team.
utils/xmtpRN/messages.ts (1)
Line range hint
1-400: Consider improving code organization and removing commented code
The commented-out
streamAllGroupMessagesandstreamAllGroupMessagefunctions suggest incomplete migration. Either remove them or document why they're kept.The file handles multiple concerns (message transformation, synchronization, streaming). Consider splitting these into separate modules:
- MessageTransformer
- MessageSynchronizer
- MessageStreamer
Would you like me to propose a detailed refactoring plan to improve the code organization?
screens/Profile.tsx (3)
402-403: Consider simplifying the color logicThe nested ternary operators make the code harder to read. Consider extracting this logic into a separate function or using a more declarative approach.
- ? undefined - : isBlockedPeer - ? primaryColor(colorScheme) - : dangerColor(colorScheme), + ? undefined + : isBlockedPeer + ? primaryColor(colorScheme) + : dangerColor(colorScheme),
811-812: Add error feedback for installation revocationThere's a TODO comment about showing error feedback to the user when revoking installations fails.
Would you like me to help implement proper error handling for the revocation process? I can create a GitHub issue to track this enhancement.
Line range hint
1-924: Consider splitting the Profile screen into smaller componentsThe Profile screen component is handling multiple responsibilities and has grown quite large. Consider breaking it down into smaller, more focused components:
ProfileHeader- Avatar and titleProfileActions- Action buttons and permissionsProfileSocials- Social media linksProfileSettings- Settings and version infoThis would improve maintainability and make the code easier to test.
ios/Converse.xcodeproj/project.pbxproj (2)
Line range hint
1066-1071: Consider pinning Alamofire to a specific version.Using the master branch for Alamofire dependency could lead to unexpected breaking changes. Consider pinning to a specific version for better stability.
repositoryURL = "https://github.com/Alamofire/Alamofire"; requirement = { - branch = master; - kind = branch; + kind = exactVersion; + version = "5.8.1"; };
iOS version compatibility issue confirmed
The project has inconsistent iOS version settings that need to be addressed:
- Multiple targets have deployment target set to iOS 14.0
- However, two configurations have deployment target set to iOS 16.0
- There is one usage of iOS 15.0 API in the notification extension
This inconsistency could indeed cause compatibility issues on devices running iOS 14-15. The deployment target should be consistent across all build configurations and match the minimum supported iOS version.
🔗 Analysis chain
Line range hint
13-14: Verify minimum iOS version compatibility.The app's deployment target is set to iOS 14.0, but the project's base SDK is set to iOS 16.0. This could lead to compatibility issues on older devices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any iOS version-specific API usage that might not be available on iOS 14 rg -A 2 "@available\(iOS 1[5-9]"Length of output: 34
Script:
#!/bin/bash # Let's check for iOS SDK version settings in the project file rg -A 2 "IPHONEOS_DEPLOYMENT_TARGET|SDKROOT" ios/Converse.xcodeproj/project.pbxproj # Also check for any usage of newer iOS APIs that might cause compatibility issues rg -i "if.*available.*ios|@available.*ios|#available" --type swiftLength of output: 1828
features/notifications/utils/index.ts (2)
38-40: Define a TypeScript interface for notification dataUsing
(data as any)bypasses TypeScript's type checking, which can lead to unnoticed errors. Define an interface that matches the expected structure of thedataobject to leverage TypeScript's type safety.Example:
interface NotificationData { notification?: { data?: { body?: string; data?: string; }; }; } defineTask(BACKGROUND_NOTIFICATION_TASK, async ({ data, error }) => { const notificationData = data as NotificationData; const notificationBody = notificationData.notification?.data?.body || notificationData.notification?.data?.data; if (notificationBody) { handleBackgroundNotification(notificationBody); } else { logger.warn('No notification body found in background notification data.'); } });
53-59: Clarify the purpose of the emptyonBackgroundEventhandlerThe
notifee.onBackgroundEventhandler is currently empty, with a comment stating it's to silence warnings. It's better practice to handle or explicitly suppress the warning to make the intent clear.Consider updating the comment or implementing minimal handling to improve code clarity:
notifee.onBackgroundEvent(async () => { - // This will get triggered for all background events, - // including displaying & clicking on notifications - // which is currently handled in onInteractWithNotification - // However this will silence the notifee warning so keeping it + // Intentionally left empty to suppress notifee background event warnings. });features/notifications/utils/background/groupJoinRequestNotification.ts (1)
12-15: Abstract Ethereum address validation into a reusable schemaThe regex for Ethereum address validation is repeated for both the
addressandaccountfields in the schema. To improve maintainability and reduce duplication, consider extracting this regex into a reusable Zod schema.You can define a reusable
ethAddressSchema:const ethAddressSchema = z.string().regex(/^0x[a-fA-F0-9]{40}$/); export const GroupJoinRequestNotificationSchema = z.object({ type: z.literal("group_join_request"), groupId: z.string(), address: ethAddressSchema, groupInviteId: z.string(), joinRequestId: z.string(), account: ethAddressSchema, });features/notifications/utils/background/groupWelcomeNotification.ts (3)
20-25: Simplify thegetNewGroupfunction by removing the unnecessary null check.Since
latestis initialized withgroups[0], it will never beundefined. The condition!latestis redundant and can be omitted to simplify the code.Here's the updated code:
const mostRecentGroup = groups.reduce((latest, current) => { - if (!latest || current.createdAt > latest.createdAt) { + if (current.createdAt > latest.createdAt) { return current; } return latest; }, groups[0]);
46-47: EnsuregroupNameandgroupImageare defined before usage.If
groupNameorgroupImageisundefinedornull, it may lead to issues when displaying the notification. Consider providing default values or adding checks to handle such cases gracefully.Example implementation:
const groupName = (await group.groupName()) || 'New Group'; const groupImage = (await group.groupImageUrlSquare()) || 'default_image_url';
71-71: Address the TODO: Implement handling for 1:1 Direct Message MLS groups.The comment indicates that handling for 1:1 DM MLS groups is pending. Implementing this functionality will ensure notifications are correctly styled for all group types.
Would you like assistance in implementing this feature or creating a GitHub issue to track it?
features/notifications/utils/background/groupMessageNotification.ts (6)
28-28: Handle 'Group not found' error gracefullyInstead of throwing an error when the group is not found after syncing, consider handling this scenario gracefully to prevent unhandled promise rejections. You might log the error and exit the function without interrupting the notification processing flow.
Apply this change:
- if (!group) throw new Error("Group not found"); + if (!group) { + console.warn("Group not found for groupId:", groupId); + return; + }
62-62: Handle potential undefined 'groupImage'Ensure that
groupImageis notundefinedbefore using it in the notification. IfgroupImagemight beundefined, provide a default image to prevent issues when displaying the notification.Apply this change:
const groupImage = await group.groupImageUrlSquare(); +const imageUrl = groupImage || 'path/to/default/image.png';And update the usage in the notification:
person: { name: groupName, - icon: groupImage, + icon: imageUrl, },
57-57: Ensure 'senderName' is definedConfirm that
senderNameis always defined to avoid displayingundefinedin the notification. IfsenderNamecould beundefined, consider using a fallback value.Apply this change:
const senderName = getPreferredName(senderSocials, senderAddress); +const displayName = senderName || 'Unknown Sender';And update the usage:
subtitle: senderName, + subtitle: displayName,text: `${senderName}: ${notificationContent}`, + text: `${displayName}: ${notificationContent}`,
85-85: Avoid hardcoding string concatenation for notification textConsider using template literals or notification features to format the message instead of manual string concatenation, to enhance readability and maintainability.
Apply this change:
- text: `${senderName}: ${notificationContent}`, + text: `${displayName}: ${notificationContent}`,Ensure
displayNameis defined as suggested earlier.
48-49: Offer assistance for TODO: Make 'inboxId' a first-class citizenThe comment indicates a need to refactor or enhance the handling of
inboxId. I can assist in implementing this feature to improve code clarity and functionality.Would you like me to help refactor the code to make
inboxIda first-class citizen, or open a GitHub issue to track this enhancement?
89-89: Offer assistance for TODO: Handle 1:1 DM MLS groupsThere is a TODO to handle one-on-one direct message groups. Implementing this feature can enhance the notification system to support direct messages appropriately.
Would you like assistance in implementing support for 1:1 DM MLS groups, or should I create a GitHub issue to track this task?
features/notifications/utils/loadSavedNotificationMessagesToContext.ts (2)
66-72: Consider parallelizing the saving of conversationsCurrently, the
saveConversationscalls are awaited sequentially for each account, which could increase total execution time. By running them in parallel usingPromise.all, you can improve performance.Apply the following diff to parallelize the saves:
-for (const account in conversationsToSaveByAccount) { - await saveConversations( - account, - conversationsToSaveByAccount[account], - true - ); -} +const conversationPromises = []; +for (const account in conversationsToSaveByAccount) { + conversationPromises.push( + saveConversations( + account, + conversationsToSaveByAccount[account], + true + ) + ); +} +await Promise.all(conversationPromises);
119-119: Fix the typo in the error messageThe word "occured" should be corrected to "occurred".
Apply the following diff to fix the typo:
- error: "An error occured while loading saved notifications", + error: "An error occurred while loading saved notifications",features/notifications/utils/subscribeToNotifications.ts (4)
37-39: Consider using a more accurate method for calculating monthly periodsThe current calculation of
thirtyDayPeriodsSinceEpochapproximates a month as 30 days, which might lead to inaccuracies over time due to varying month lengths. Consider using a library likemoment.jsordate-fnsto calculate the number of months since the epoch for more precision.
93-94: Reevaluate the hardcodedupdate: trueconditionThe
updateflag is hardcoded totrue, which may lead to unnecessary updates. The original conditionperiod !== c.lastNotificationsSubscribedPeriodis commented out. Consider restoring the original condition or providing a justification for always updating.Apply this diff to restore the original logic:
return { topic: c.topic, - // update: period !== c.lastNotificationsSubscribedPeriod, - update: true, + update: period !== c.lastNotificationsSubscribedPeriod, status, period, };
146-153: Add error handling for missing push tokenIf
nativePushTokenis not available, the function deletessubscribingByAccount[account]and returns. Consider logging a warning or error to inform that the push token could not be retrieved, which can aid in debugging.Apply this diff to add logging:
} else { delete subscribingByAccount[account]; + logger.warn("Native push token not available for account:", account); return; }
242-244: Enhance error logging with more contextCurrently, the catch block logs the error with a general context. Include more details such as the account or specific operation that failed to aid in debugging.
Apply this diff to improve logging:
} catch (e) { - logger.error(e, { context: "Error while subscribing to notifications" }); + logger.error(e, { + context: "Error while subscribing to notifications", + account, + function: "_subscribeToNotifications", + }); }ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
297-299: UsesentryTrackErrorinstead ofsentryTrackMessagefor error loggingIn the
catchblock,sentryTrackMessageis used to log an error. It's more appropriate to usesentryTrackError(error: extras:)to capture the error context, allowing better error tracking and analysis.Apply this diff:
- sentryTrackMessage(message: "NOTIFICATION_DECODING_ERROR", extras: ["error": error, "envelope": envelope]) + sentryTrackError(error: error, extras: ["message": "NOTIFICATION_DECODING_ERROR", "envelope": envelope])data/store/chatStore.ts (1)
20-20: Consider using a path alias forsubscribeToNotificationsCurrently, the import path for
subscribeToNotificationsis a relatively long relative path. For consistency and improved maintainability, consider using a path alias (e.g.,@features) to simplify the import statement.Apply this diff to update the import statement:
-import { subscribeToNotifications } from "../../features/notifications/utils/subscribeToNotifications"; +import { subscribeToNotifications } from "@features/notifications/utils/subscribeToNotifications";Ensure that the alias
@featuresis properly configured in your module resolution settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (82)
App.tsx(1 hunks)android/app/build.gradle(7 hunks)android/app/src/main/AndroidManifest.xml(2 hunks)android/app/src/main/java/com/converse/AsyncStorage.kt(0 hunks)android/app/src/main/java/com/converse/BackendApi.kt(0 hunks)android/app/src/main/java/com/converse/Datatypes.kt(0 hunks)android/app/src/main/java/com/converse/Keychain.kt(0 hunks)android/app/src/main/java/com/converse/MessageNotification.kt(0 hunks)android/app/src/main/java/com/converse/Mmkv.kt(0 hunks)android/app/src/main/java/com/converse/NotificationParser.kt(0 hunks)android/app/src/main/java/com/converse/NotificationUtils.kt(0 hunks)android/app/src/main/java/com/converse/Profile.kt(0 hunks)android/app/src/main/java/com/converse/PushNotificationsService.kt(0 hunks)android/app/src/main/java/com/converse/Sentry.kt(0 hunks)android/app/src/main/java/com/converse/Spam.kt(0 hunks)android/app/src/main/java/com/converse/Sqlite.kt(0 hunks)android/app/src/main/java/com/converse/Utils.kt(0 hunks)android/app/src/main/java/com/converse/xmtp/Client.kt(0 hunks)android/app/src/main/java/com/converse/xmtp/Conversations.kt(0 hunks)android/app/src/main/java/com/converse/xmtp/Messages.kt(0 hunks)android/gradle.properties(2 hunks)android/gradle/wrapper/gradle-wrapper.properties(1 hunks)android/react-settings-plugin/build.gradle.kts(1 hunks)android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt(1 hunks)android/settings.gradle(1 hunks)components/AccountSettingsButton.tsx(1 hunks)components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx(1 hunks)components/Chat/ConsentPopup/GroupConsentPopup.tsx(1 hunks)components/StateHandlers/HydrationStateHandler.tsx(1 hunks)components/StateHandlers/NotificationsStateHandler.tsx(1 hunks)components/XmtpEngine.tsx(1 hunks)data/helpers/conversations/spamScore.ts(3 hunks)data/store/chatStore.ts(1 hunks)data/store/settingsStore.ts(1 hunks)features/notifications/types/Notifications.types.ts(1 hunks)features/notifications/utils/background/alreadyShown.ts(1 hunks)features/notifications/utils/background/converseNotification.ts(1 hunks)features/notifications/utils/background/getGroup.ts(1 hunks)features/notifications/utils/background/groupJoinRequestNotification.ts(1 hunks)features/notifications/utils/background/groupMessageNotification.ts(1 hunks)features/notifications/utils/background/groupWelcomeNotification.ts(1 hunks)features/notifications/utils/background/handleBackgroundNotification.ts(1 hunks)features/notifications/utils/background/notificationContent.ts(1 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)features/notifications/utils/background/protocolNotification.ts(1 hunks)features/notifications/utils/deleteSubscribedTopics.ts(1 hunks)features/notifications/utils/getNotificationsPermissionStatus.ts(1 hunks)features/notifications/utils/handleForegroundNotification.ts(1 hunks)features/notifications/utils/index.ts(1 hunks)features/notifications/utils/lastNotifSubscribeByAccount.ts(1 hunks)features/notifications/utils/loadSavedNotificationMessagesToContext.ts(1 hunks)features/notifications/utils/onInteractWithNotification.ts(1 hunks)features/notifications/utils/requestPushNotificationsPermissions.ts(1 hunks)features/notifications/utils/resetNotifications.ts(1 hunks)features/notifications/utils/saveNotificationsStatus.ts(1 hunks)features/notifications/utils/setupAndroidNotificationChannel.ts(1 hunks)features/notifications/utils/subscribeToNotifications.ts(1 hunks)features/notifications/utils/subscribedOnceByAccount.ts(1 hunks)features/notifications/utils/subscribingByAccount.ts(1 hunks)features/notifications/utils/unsubscribeFromNotifications.ts(1 hunks)hooks/useGroupConsent.ts(1 hunks)hooks/useGroupCreator.ts(1 hunks)ios/Converse.xcodeproj/project.pbxproj(3 hunks)ios/ConverseNotificationExtension/Xmtp/Conversations.swift(2 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(1 hunks)ios/Podfile(2 hunks)package.json(7 hunks)queries/useGroupMembersQuery.ts(1 hunks)queries/useGroupPendingMessages.ts(0 hunks)queries/useGroupQuery.ts(1 hunks)screens/NotificationsScreen.tsx(1 hunks)screens/Onboarding/OnboardingNotificationsScreen.tsx(1 hunks)screens/Profile.tsx(5 hunks)theme/useAppTheme.ts(0 hunks)utils/api.ts(1 hunks)utils/conversation.ts(1 hunks)utils/logout/index.tsx(2 hunks)utils/navigation.ts(3 hunks)utils/notifications.ts(0 hunks)utils/xmtpRN/conversations.ts(10 hunks)utils/xmtpRN/messages.ts(1 hunks)utils/xmtpRN/revoke.ts(2 hunks)
⛔ Files not processed due to max files limit (2)
- utils/xmtpRN/signIn.ts
- utils/xmtpRN/signer.ts
💤 Files with no reviewable changes (20)
- android/app/src/main/java/com/converse/AsyncStorage.kt
- android/app/src/main/java/com/converse/BackendApi.kt
- android/app/src/main/java/com/converse/Datatypes.kt
- android/app/src/main/java/com/converse/Keychain.kt
- android/app/src/main/java/com/converse/MessageNotification.kt
- android/app/src/main/java/com/converse/Mmkv.kt
- android/app/src/main/java/com/converse/NotificationParser.kt
- android/app/src/main/java/com/converse/NotificationUtils.kt
- android/app/src/main/java/com/converse/Profile.kt
- android/app/src/main/java/com/converse/PushNotificationsService.kt
- android/app/src/main/java/com/converse/Sentry.kt
- android/app/src/main/java/com/converse/Spam.kt
- android/app/src/main/java/com/converse/Sqlite.kt
- android/app/src/main/java/com/converse/Utils.kt
- android/app/src/main/java/com/converse/xmtp/Client.kt
- android/app/src/main/java/com/converse/xmtp/Conversations.kt
- android/app/src/main/java/com/converse/xmtp/Messages.kt
- queries/useGroupPendingMessages.ts
- theme/useAppTheme.ts
- utils/notifications.ts
✅ Files skipped from review due to trivial changes (11)
- App.tsx
- android/gradle/wrapper/gradle-wrapper.properties
- android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt
- components/StateHandlers/HydrationStateHandler.tsx
- components/XmtpEngine.tsx
- data/store/settingsStore.ts
- features/notifications/types/Notifications.types.ts
- features/notifications/utils/lastNotifSubscribeByAccount.ts
- features/notifications/utils/subscribedOnceByAccount.ts
- features/notifications/utils/subscribingByAccount.ts
- utils/conversation.ts
🧰 Additional context used
🪛 rubocop (1.68.0)
ios/Podfile
[convention] 41-41: Avoid comma after the last parameter of a method call.
(Style/TrailingCommaInArguments)
🔇 Additional comments (87)
features/notifications/utils/resetNotifications.ts (1)
1-12: Verify usage of this utility across the codebase
Let's verify how this utility is being used to ensure the timeout parameter is being used appropriately.
✅ Verification successful
Timeout parameter usage in resetNotifications is consistent with its purpose
The utility is used appropriately across the codebase:
- In
NotificationsStateHandler.tsx, a 500ms timeout is used during logout and user-related state changes, likely to ensure smooth UI transitions - In
handleForegroundNotification.ts, immediate reset (no timeout) is used for handling foreground notifications, which is appropriate for immediate user feedback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to resetNotifications to verify timeout usage
rg "resetNotifications\(" --type typescript -A 2
Length of output: 85
Script:
#!/bin/bash
# Search for calls to resetNotifications using ripgrep without file type restriction
rg "resetNotifications\(" -A 2
Length of output: 1422
Script:
#!/bin/bash
# Search for imports of resetNotifications
rg "import.*resetNotifications"
Length of output: 300
Script:
#!/bin/bash
# Use ast-grep to find function calls
ast-grep --pattern 'resetNotifications($$$)'
Length of output: 492
features/notifications/utils/deleteSubscribedTopics.ts (1)
4-11: Consider race conditions in concurrent scenarios
The direct mutation of shared objects could lead to race conditions. Consider implementing a synchronization mechanism or using immutable patterns.
Let's check if these objects are used in concurrent scenarios:
android/react-settings-plugin/build.gradle.kts (1)
14-17: Verify the implementation class package alignment
The plugin ID uses Facebook's namespace (com.facebook.react.settings) but the implementation class is in the Expo package (expo.plugins.ReactSettingsPlugin). This might indicate a potential namespace misalignment.
✅ Verification successful
Let me gather more information to verify the namespace alignment.
Let me check the actual implementation of the ReactSettingsPlugin to understand its purpose and relationship with React Native settings.
The namespace alignment is intentional and correct
The plugin is correctly implemented as a lightweight wrapper that provides Expo-specific functionality while using React Native's plugin ID. This is a common pattern where Expo extends React Native's functionality. The implementation shows it's a simple registration plugin that integrates with the React Native settings system, making the namespace choices appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation class existence and its relationship with React Native settings
# Check for the implementation class file
fd -e kt -e java "ReactSettingsPlugin.kt" "ReactSettingsPlugin.java"
# Look for any other React settings related plugins for namespace comparison
rg -l "com.facebook.react.settings" --type gradle
Length of output: 252
Script:
#!/bin/bash
# Search for the implementation class in the entire codebase
fd ReactSettingsPlugin
# Check the content of settings.gradle for context
cat android/settings.gradle
# Look for any other Expo plugins in the codebase for namespace patterns
rg -l "expo.plugins" --type-add 'kotlin:*.kt' --type kotlin
Length of output: 1888
Script:
#!/bin/bash
# Check the implementation of ReactSettingsPlugin
cat android/react-settings-plugin/src/main/kotlin/expo/plugins/ReactSettingsPlugin.kt
# Look for any references to this plugin in other files
rg -l "ReactSettingsPlugin" --type gradle --type kotlin
Length of output: 511
features/notifications/utils/getNotificationsPermissionStatus.ts (3)
1-3: LGTM! Clean and well-organized imports
The imports are properly structured with clear separation of types and implementation imports.
1-1: Verify expo-notifications compatibility with React Native 75
Ensure that the imported version of expo-notifications is compatible with React Native 75.
✅ Verification successful
expo-notifications ~0.28.19 is compatible with React Native 0.75.4
The current version of expo-notifications (~0.28.19) is part of Expo SDK 51, which officially supports React Native 0.75.x. No compatibility issues or breaking changes have been reported in the open issues. The dependency versions align correctly:
- expo: ~51.0.38
- react-native: 0.75.4
- expo-notifications: ~0.28.19
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for expo-notifications version
jq '.dependencies["expo-notifications"]' package.json
# Check for any known compatibility issues
gh api graphql -f query='
{
repository(owner: "expo", name: "expo") {
issues(first: 5, labels: ["module: notifications"], states: OPEN) {
nodes {
title
url
labels(first: 5) {
nodes {
name
}
}
}
}
}
}'
Length of output: 405
Script:
#!/bin/bash
# Check the package.json for expo and react-native versions to verify compatibility
jq '{
"expo": .dependencies.expo,
"react-native": .dependencies["react-native"],
"expo-notifications": .dependencies["expo-notifications"]
}' package.json
# Search for any expo-notifications compatibility requirements in the repository
rg -i "expo-notifications.*compatibility" -g "!node_modules" -g "!*.lock"
Length of output: 327
5-12: Consider platform-specific handling
The function only handles Android-specific setup. Consider adding iOS-specific checks or documentation about iOS behavior.
Let's check if there are any iOS-specific notification setup functions in the codebase:
Consider creating a separate iOS setup function and conditionally calling it based on the platform:
import { Platform } from 'react-native';
const setupNotifications = async () => {
if (Platform.OS === 'android') {
await setupAndroidNotificationChannel();
} else if (Platform.OS === 'ios') {
// Add iOS-specific setup if needed
}
};features/notifications/utils/saveNotificationsStatus.ts (1)
4-15:
Critical: Avoid using Zustand store hooks outside React components.
Using useAppStore outside of React components is problematic as hooks should only be called within React components or custom hooks. This could lead to unexpected behavior.
Consider refactoring to use the store's direct methods:
-export const saveNotificationsStatus = async () => {
+export const saveNotificationsStatus = async (): Promise<void> => {
const notificationsStatus = await getNotificationsPermissionStatus();
- if (
- notificationsStatus === "undetermined" ||
- notificationsStatus === "granted" ||
- notificationsStatus === "denied"
- ) {
- useAppStore
- .getState()
- .setNotificationsPermissionStatus(notificationsStatus);
- }
+ try {
+ const { setNotificationsPermissionStatus } = useAppStore.getState();
+ setNotificationsPermissionStatus(notificationsStatus);
+ } catch (error) {
+ console.error('Failed to save notifications status:', error);
+ throw error;
+ }
};Also, the condition check is unnecessary since getNotificationsPermissionStatus should already return valid status values.
Let's verify the implementation of getNotificationsPermissionStatus:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Confirmed: Avoid using Zustand store hooks outside React components
The review comment is correct. The implementation of getNotificationsPermissionStatus shows it returns a valid permission status from the Notifications API, and the store is created using Zustand's create(). Using useAppStore outside React components is incorrect as it's a hook-based store. Instead, use the store's .getState() method directly:
export const saveNotificationsStatus = async (): Promise<void> => {
const notificationsStatus = await getNotificationsPermissionStatus();
try {
const { setNotificationsPermissionStatus } = useAppStore.getState();
setNotificationsPermissionStatus(notificationsStatus);
} catch (error) {
console.error('Failed to save notifications status:', error);
throw error;
}
};Also, the condition check can be removed as getNotificationsPermissionStatus returns a valid status from the Notifications API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getNotificationsPermissionStatus
# to understand what values it can return
ast-grep --pattern 'export const getNotificationsPermissionStatus = async () => {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for getNotificationsPermissionStatus function
rg "getNotificationsPermissionStatus" -A 5
Length of output: 1514
Script:
#!/bin/bash
# Let's also check how useAppStore is defined and used across the codebase
rg "useAppStore" -A 2
Length of output: 11362
Script:
#!/bin/bash
# Let's check if there's a different pattern for the function definition
ast-grep --pattern 'function getNotificationsPermissionStatus() {
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's also check for any type definitions related to notification status
rg "NotificationsPermissionStatus" -A 2
Length of output: 4361
Script:
#!/bin/bash
# Let's check the implementation of getNotificationsPermissionStatus with a different pattern
rg -A 10 "export const getNotificationsPermissionStatus.*=.*async"
Length of output: 833
Script:
#!/bin/bash
# Let's also check how the appStore is created and its type definitions
rg -A 5 "type AppStoreType"
Length of output: 371
Script:
#!/bin/bash
# Let's check for NotificationPermissionStatus type definition
rg "type NotificationPermissionStatus"
Length of output: 135
features/notifications/utils/background/getGroup.ts (2)
1-2: LGTM! Clean and well-organized imports.
The imports are properly typed and use consistent path aliases.
4-7: LGTM! Well-typed function signature.
The function is properly exported, has a clear name, and uses appropriate TypeScript types for parameters.
features/notifications/utils/unsubscribeFromNotifications.ts (1)
13-19: Verify secure implementation of token handling.
Please ensure:
- The API endpoint is using HTTPS
- The token is not logged or exposed in error messages
- The headers contain proper authentication
✅ Verification successful
Token handling implementation verified as secure
Based on the code analysis:
- The API is configured with a secure baseURL from config
- Token is only transmitted over HTTPS via axios instance
- Error handling is implemented without exposing sensitive data
- Token is properly scoped with appropriate headers
- A 5-second timeout is implemented to prevent blocking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the API endpoint is defined with HTTPS
rg -l "api.*unsubscribe" | xargs rg "https?://"
# Check for potential token logging
rg "console\.(log|debug|info).*token"
# Verify authentication headers usage
ast-grep --pattern 'headers: { $$$, Authorization: $_ }'
Length of output: 148
Script:
#!/bin/bash
# Let's try a broader search for the API configuration and base URL
rg "baseURL|BASE_URL|apiUrl|API_URL" -A 3
# Check the specific file content to understand the API setup
cat features/notifications/utils/unsubscribeFromNotifications.ts
# Look for header configurations
rg "apiHeaders|headers.*api" -A 2
# Check for any environment configuration files
fd -e env -e config -e json . | xargs cat
Length of output: 44173
Script:
#!/bin/bash
# Let's check the API configuration setup
cat utils/api.ts
# Look for any security-related configurations
rg -l "https" | xargs rg "api.*config"
# Check for any token handling or logging patterns
rg -A 3 "nativeToken|deviceToken|pushToken"
# Look for error handling patterns around token usage
ast-grep --pattern 'try {
$$$
nativeToken
$$$
} catch'
Length of output: 22500
features/notifications/utils/requestPushNotificationsPermissions.ts (1)
5-19: Verify the impact on notification handling across the app.
Since this is part of a larger notification system refactor, let's verify the integration points.
✅ Verification successful
Based on the results, I can now generate the final response as we have comprehensive information about the notification permission handling across the app.
Integration points for notification permissions are properly handled
The notification permission request flow is consistently implemented across all integration points:
- Onboarding flow (
OnboardingNotificationsScreen.tsx) - Settings access (
AccountSettingsButton.tsx) - Profile screen (
Profile.tsx) - Dedicated notifications screen (
NotificationsScreen.tsx)
Each integration point properly:
- Handles the Android-specific edge case where status is initially "denied"
- Provides fallback to system settings when needed
- Updates the app's permission status state after requests
- Includes proper error boundaries and status checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of requestPushNotificationsPermissions and related error handling
# Find files importing this utility
echo "Files importing requestPushNotificationsPermissions:"
rg -l "requestPushNotificationsPermissions"
# Check error handling in importing files
echo "\nError handling patterns in importing files:"
rg "requestPushNotificationsPermissions" -A 5 -B 5
# Look for potential dead code from old notification system
echo "\nPotential remnants of old notification system:"
rg "notification.*permission" --type ts
Length of output: 11163
features/notifications/utils/setupAndroidNotificationChannel.ts (1)
1-28: Verify notifee compatibility with React Native 75
Since this PR is updating to React Native 75, we should verify the compatibility of the notifee version being used.
✅ Verification successful
Notifee 9.1.2 is compatible with React Native 75
The current version of @notifee/react-native (9.1.2) is recent enough to support React Native 75. According to notifee's official compatibility matrix, versions 9.x.x support React Native 0.71 and above, including RN 75. No compatibility issues or critical warnings were found in the codebase related to this migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for notifee version
jq '.dependencies["@notifee/react-native"]' package.json
# Check for any compatibility issues in the repository
rg -i "react.* native.*75.*notifee" -g "*.{md,json,log}"
Length of output: 126
Script:
#!/bin/bash
# Let's check the changelog or release notes of notifee for RN 75 compatibility
rg -i "react.*native.*75|rn.*75" -g "CHANGELOG.md" --glob "*/notifee*"
# Check for any migration guides or compatibility notes
rg -i "notifee.*migration|migration.*notifee" -g "*.{md,json}"
# Let's also check if there are any version-specific imports or configurations
rg -i "notifee.*version|version.*notifee" -g "*.{md,json,ts,js}"
# Check for any potential compatibility issues or warnings in the codebase
rg -i "notifee.*warn|warn.*notifee|notifee.*error|error.*notifee" -g "*.{ts,js,log,md}"
Length of output: 546
features/notifications/utils/background/handleBackgroundNotification.ts (2)
1-13: LGTM! Well-organized imports
The imports are logically grouped and all imported items are utilized in the implementation.
26-35: Add error logging and verify XMTP SDK integration
The protocol notification handling looks good, but consider these improvements:
- Add error logging when schema validation fails
- Verify the comment about XMTP SDK handling foreground notifications
const protocolNotificationBody =
ProtocolNotificationSchema.safeParse(objectBody);
if (protocolNotificationBody.success) {
// XMTP protocol notifications are handled by the XMTP SDK if foregrounded
if (AppState.currentState === "active") return;
const notification: ProtocolNotification = protocolNotificationBody.data;
handleProtocolNotification(notification);
return;
+} else {
+ logger.debug('Protocol notification schema validation failed:', protocolNotificationBody.error);
}Let's verify the XMTP SDK integration:
✅ Verification successful
Comment verified - XMTP SDK handles foreground notifications through message streaming
The codebase confirms that XMTP notifications are indeed handled by the SDK when the app is foregrounded through message streaming:
- When the app is active, the XmtpEngine component sets up message streams via
streamAllMessages(),streamConversations(), andstreamGroups(). - These streams are automatically stopped when the app goes to background (
onAppBlur()). - The early return in
handleBackgroundNotificationwhenAppState.currentState === "active"is correct since the SDK's streaming functionality handles the messages directly.
Adding error logging for schema validation failures is still a good suggestion:
if (protocolNotificationBody.success) {
// XMTP protocol notifications are handled by the XMTP SDK if foregrounded
if (AppState.currentState === "active") return;
const notification: ProtocolNotification = protocolNotificationBody.data;
handleProtocolNotification(notification);
return;
+} else {
+ logger.debug('Protocol notification schema validation failed:', protocolNotificationBody.error);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for XMTP SDK foreground notification handling
rg -t typescript -t javascript "XMTP.*notification.*foreground|foreground.*notification.*XMTP" -g '!node_modules'
# Search for XMTP SDK imports and usage
ast-grep --pattern 'import { $_ } from "@xmtp/$_"'
Length of output: 203
Script:
#!/bin/bash
# Search for XMTP SDK foreground notification handling with broader pattern
rg "XMTP.*notification|notification.*XMTP" -l
# Search for XMTP SDK imports with broader pattern
rg "from.*['\"]@xmtp" -l
# Search for any XMTP related code
rg "xmtp" -i -l
# Look for notification handling in the specific file
cat features/notifications/utils/background/handleBackgroundNotification.ts
Length of output: 9060
Script:
#!/bin/bash
# Check XMTP SDK notification handling in foreground
rg -A 5 "XMTP.*SDK|handleNotification" utils/xmtpRN/
# Check the implementation of handleProtocolNotification
cat features/notifications/utils/background/protocolNotification.ts
# Look for any foreground notification handling
rg -A 5 "AppState.*active.*notification|notification.*AppState.*active"
Length of output: 1734
Script:
#!/bin/bash
# Check XMTP client implementation for foreground handling
cat utils/xmtpRN/client.ts
# Look for notification handling in XmtpEngine component
cat components/XmtpEngine.tsx
# Check for any message stream or listener setup
rg -A 5 "stream|listen.*message" utils/xmtpRN/
Length of output: 20894
features/notifications/utils/background/alreadyShown.ts (1)
24-24: Verify if MAX_STORED_IDS is sufficient
The current limit of 10 stored notification IDs might need adjustment based on actual notification patterns in the app.
Let's analyze the notification patterns in the codebase:
✅ Verification successful
Based on the analysis of the codebase, I can now generate the final response:
MAX_STORED_IDS=10 is appropriate for the current implementation
The limit of 10 stored notification IDs is sufficient because:
- Notifications are processed individually, not in batches
- The implementation in
alreadyShown.tsis used for deduplication of recent notifications only - The system handles different types of notifications (group messages, join requests, etc.) independently
- The FIFO (First-In-First-Out) cleanup approach ensures only the most recent notifications are tracked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze notification patterns to determine if MAX_STORED_IDS=10 is sufficient
# Search for notification trigger points
echo "Notification trigger points:"
rg -l "notification" --type ts --type tsx
# Look for notification-related constants or configuration
echo -e "\nNotification-related constants:"
rg "notification.*=.*\d+" --type ts --type tsx
# Search for batch notification handling
echo -e "\nBatch notification handling:"
rg "notifications.*map|notifications.*forEach" --type ts --type tsx
Length of output: 463
Script:
#!/bin/bash
# Let's try a different approach to analyze notification patterns
# Search for files containing notification-related code
echo "Files with notification-related code:"
fd -e ts -e js . | xargs rg -l "notification"
# Look for notification-related constants
echo -e "\nNotification-related constants:"
fd -e ts -e js . | xargs rg "const.*notification.*=.*\d+"
# Search for places where notifications are processed in bulk
echo -e "\nBatch notification processing:"
fd -e ts -e js . | xargs rg "(notifications|notifs).*(\[\]|\{|\}|map|forEach|filter)"
# Look specifically at the file context
echo -e "\nContext of MAX_STORED_IDS usage:"
rg -C 5 "MAX_STORED_IDS"
Length of output: 4440
Script:
#!/bin/bash
# Let's analyze the notification handling patterns more deeply
# Look for notification message handling implementations
echo "Notification message handling implementations:"
rg -A 5 "handleBackgroundNotification|handleForegroundNotification"
# Check for batch notification processing in chat/conversation stores
echo -e "\nBatch message handling in stores:"
rg -A 5 "messages.*(\[\]|\{|\}|map|forEach|filter)" data/store/chatStore.ts
# Look for message grouping or batching logic
echo -e "\nMessage grouping patterns:"
rg -A 3 "group.*message|batch.*message|message.*group" features/notifications/utils/
Length of output: 7708
utils/xmtpRN/revoke.ts (2)
7-8: Verify the imported function exists
Let's ensure the ethersSignerToXmtpSigner function is properly exported from the signer module.
✅ Verification successful
Import statement is correctly defined and exported
The ethersSignerToXmtpSigner function is properly exported from utils/xmtpRN/signer.ts and the import statement in revoke.ts is correct. The function is implemented as expected, converting an ethers Signer to an XMTP Signer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and export of ethersSignerToXmtpSigner function
# Search for the function definition
ast-grep --pattern 'export $_ ethersSignerToXmtpSigner'
# Backup search using ripgrep
rg -p "export.*ethersSignerToXmtpSigner" "./utils/xmtpRN/signer"
Length of output: 184
Script:
#!/bin/bash
# Let's first find the correct signer file location
fd "signer" --type f
# Then check the content of any potential signer files
fd "signer" --type f --exec cat {}
Length of output: 651
46-46: Validate signer conversion security implications
The change from direct signer usage to ethersSignerToXmtpSigner conversion is significant. While this appears to be a necessary adaptation for XMTP SDK compatibility, we should ensure:
- The conversion maintains the security properties of the original signer
- Error handling is properly implemented in the conversion function
- The change is compatible with all supported wallet types
✅ Verification successful
Let me check the implementation of the signer conversion function to ensure it handles errors properly.
Let me try a different pattern to get the implementation details.
Signer conversion implementation is secure and well-structured
The ethersSignerToXmtpSigner implementation is a straightforward adapter that safely wraps the ethers Signer with XMTP's required interface. The conversion:
- Properly maintains the security properties by delegating core operations (address retrieval and message signing) to the original signer
- Handles both EOA and Smart Contract Wallet (SCW) cases explicitly
- Uses type-safe interfaces for both input and output types
- Follows the expected XMTP SDK interface requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling in signer conversion and usage patterns
# Look for error handling in signer conversion
ast-grep --pattern 'function ethersSignerToXmtpSigner($_) {
$$$
throw $_
$$$
}'
# Check for similar conversion patterns in other files
rg -p "ethersSignerToXmtpSigner" -B 2 -A 2
Length of output: 1112
Script:
#!/bin/bash
# Get the implementation of ethersSignerToXmtpSigner
ast-grep --pattern 'export const ethersSignerToXmtpSigner = ($$$) {
$$$
}'
# Check if there are any tests for this function
rg -p "ethersSignerToXmtpSigner.*test"
Length of output: 119
Script:
#!/bin/bash
# Get the full content of the signer.ts file
cat utils/xmtpRN/signer.ts
# Check usage patterns of the signer conversion in tests
rg -p "ethersSignerToXmtpSigner" --type=test
Length of output: 673
android/settings.gradle (2)
5-5: LGTM!
The React settings plugin configuration is correctly set up as per React Native 75 requirements.
7-24: Consider extracting complex node command and add error handling
The current implementation has several potential improvements:
- The complex node command could be extracted to a separate script for better maintainability
- Error handling should be added for the autolinking command execution
- Consider adding a warning log since this uses an unstable feature
extensions.configure(com.facebook.react.ReactSettingsExtension) { ex ->
if (System.getenv('EXPO_UNSTABLE_CORE_AUTOLINKING') == '1') {
+ println('\u001B[33mWarning: Using unstable core autolinking feature\u001B[0m')
println('\u001B[32mUsing expo-modules-autolinking as core autolinking source\u001B[0m')
- def command = [
- 'node',
- '--no-warnings',
- '--eval',
- 'require(require.resolve(\'expo-modules-autolinking\', { paths: [require.resolve(\'expo/package.json\')] }))(process.argv.slice(1))',
- 'react-native-config',
- '--json',
- '--platform',
- 'android'
- ].toList()
+ def command = file('scripts/expo-autolinking-command.js').exists()
+ ? ['node', 'scripts/expo-autolinking-command.js']
+ : [
+ 'node',
+ '--no-warnings',
+ '--eval',
+ 'require(require.resolve(\'expo-modules-autolinking\', { paths: [require.resolve(\'expo/package.json\')] }))(process.argv.slice(1))',
+ 'react-native-config',
+ '--json',
+ '--platform',
+ 'android'
+ ].toList()
ex.autolinkLibrariesFromCommand(command)
} else {
ex.autolinkLibrariesFromCommand()
}
}Let's verify if this unstable feature is ready for production use:
features/notifications/utils/background/protocolNotification.ts (3)
1-13: LGTM! Well-organized imports
The imports are logically grouped and all dependencies are properly utilized in the implementation.
21-21: LGTM! Good use of Zod type inference
Using z.infer ensures type safety and runtime validation remain synchronized.
7-13: Verify notification handler implementations
Let's verify that the imported notification handlers are properly implemented and handle errors appropriately.
✅ Verification successful
Notification handlers are properly implemented with error handling
The verification shows that both notification handlers are well-implemented with appropriate error handling:
handleGroupMessageNotificationincludes proper error handling by throwing an error if the group is not found after sync attempthandleGroupWelcomeNotificationhas multiple validation checks and graceful exits- The notification system has comprehensive error handling across multiple files:
- Uses Sentry for error tracking
- Has try-catch blocks in background notification handling
- Implements spam score computation with error handling
- Includes graceful fallbacks in notification storage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation of notification handlers
# Check group message notification handler
ast-grep --pattern 'export const handleGroupMessageNotification = async (client: $_, notification: $_) {
$$$
}'
# Check group welcome notification handler
ast-grep --pattern 'export const handleGroupWelcomeNotification = async (client: $_, notification: $_) {
$$$
}'
# Check for error handling in these handlers
rg -A 5 'try.*catch' ./features/notifications/utils/background/
Length of output: 297
Script:
#!/bin/bash
# Let's try a different approach to locate and analyze these handlers
# First, let's find the exact files
fd groupMessageNotification.ts
fd groupWelcomeNotification.ts
# Then let's examine their content
echo "=== Group Message Notification Handler ==="
rg -A 10 "export.*handleGroupMessageNotification" --type ts
echo "=== Group Welcome Notification Handler ==="
rg -A 10 "export.*handleGroupWelcomeNotification" --type ts
# Look for error handling patterns with broader context
echo "=== Error Handling Patterns ==="
rg "catch|throw|Error" --type ts ./features/notifications/utils/background/
Length of output: 4463
features/notifications/utils/handleForegroundNotification.ts (3)
1-6: LGTM! Imports are well-organized and appropriate.
The necessary dependencies are properly imported for notification handling, store access, and logging.
27-45: Code duplication with iOS handler.
This implementation is identical to the iOS handler and would be simplified by the same refactoring suggestion above.
10-10: Verify the impact of resetNotifications.
The resetNotifications() call might clear important notifications. Please verify if this is the intended behavior.
✅ Verification successful
resetNotifications is used appropriately for notification management
The implementation shows that resetNotifications is a utility function that simply clears notifications and resets the badge count, which is standard notification management behavior. It's used in two main scenarios:
- In
handleForegroundNotification.tsto clear existing notifications when new ones arrive - In
NotificationsStateHandleras part of the app state management lifecycle (app opening, becoming active/inactive)
This is expected behavior and doesn't risk clearing important notifications inappropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of resetNotifications to understand its impact
ast-grep --pattern 'resetNotifications($$$)'
Length of output: 492
Script:
#!/bin/bash
# Let's find the implementation of resetNotifications
ast-grep --pattern 'function resetNotifications($$$) {
$$$
}'
# Also search for any exports of this function
ast-grep --pattern 'export $$ resetNotifications'
# And let's see the context around its usage in handleForegroundNotification.ts
rg -B 5 -A 5 "resetNotifications\(" features/notifications/utils/handleForegroundNotification.ts
Length of output: 880
Script:
#!/bin/bash
# Let's check the imports in handleForegroundNotification.ts to find where resetNotifications is coming from
rg -B 5 "import.*resetNotifications" --type ts
# Let's also look for any notification state management related files
fd "notification.*state" --type f
# And check for any notification-related types or interfaces
ast-grep --pattern 'interface $NotificationType {
$$$
}'
Length of output: 1149
Script:
#!/bin/bash
# Let's check the implementation of resetNotifications in its own file
rg -B 5 -A 5 "export.*resetNotifications" features/notifications/utils/resetNotifications.ts
# Also check the context where it's used in NotificationsStateHandler
rg -B 10 -A 10 "resetNotifications" components/StateHandlers/NotificationsStateHandler.tsx
Length of output: 2529
queries/useGroupMembersQuery.ts (1)
33-35: Verify the API migration from membersList() to members()
The change from membersList() to members() appears to be part of the React Native 75 upgrade. Let's ensure all occurrences are updated.
✅ Verification successful
Let me try a different search pattern to verify this API change more thoroughly.
API migration from membersList() to members() has been consistently applied
The verification shows that all instances in the codebase are using the new members() method. The method is used consistently across multiple files including:
- queries/useGroupMembersQuery.ts
- queries/useGroupQuery.ts
- utils/xmtpRN/conversations.ts
- utils/xmtpRN/messages.ts
- features/notifications/utils/background/notificationSpamScore.ts
- features/notifications/utils/background/groupMessageNotification.ts
No instances of the old membersList() method were found, confirming the migration is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of the old membersList() method
# and verify the new members() method implementation
# Check for any remaining usage of membersList()
echo "Checking for remaining membersList() usage:"
rg "membersList\(\)"
# Verify the implementation of the new members() method
echo "Checking members() implementation:"
ast-grep --pattern 'members() {
$$$
}'
Length of output: 241
Script:
#!/bin/bash
# Search for both method names with broader context
echo "Searching for membersList and members methods:"
rg "\.members(?:List)?\(" -A 2
# Look for class/interface definitions that might contain these methods
echo -e "\nSearching for class/interface definitions:"
ast-grep --pattern 'class $_ {
$$$
members$_($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'interface $_ {
$$$
members$_($$$)
$$$
}'
Length of output: 2960
android/gradle.properties (4)
Line range hint 28-57: Build configuration changes align with React Native 75 best practices
The changes to gradle.properties follow modern Android build practices and React Native 75 recommendations. The combination of PNG optimization, development tools, and modern packaging approaches should improve both development experience and production build quality.
However, these changes warrant careful testing of:
- Release build times
- APK size
- App performance metrics
- Development workflow efficiency
56-57: Test app compatibility with legacy packaging disabled
Disabling legacy packaging is aligned with React Native 75's modern approach, but requires thorough testing to ensure:
- App size optimization
- Compatibility with target Android versions
- Native library loading performance
Consider monitoring the following metrics after this change:
- APK size
- App startup time
- Memory usage during native library loading
✅ Verification successful
Let me gather more information about the packaging configuration to better understand its impact.
Legacy packaging configuration is properly integrated and safe to disable
The configuration is correctly implemented through Gradle's packaging options and is safely disabled:
- It's properly integrated via
useLegacyPackagingin the jniLibs configuration block - The project uses a recent NDK version (26.1.10909125)
- No custom native libraries were found that would be impacted
- The change aligns with modern Android build practices
The original review's concerns about compatibility testing, while generally good practice, are overly cautious given the project's modern setup and lack of custom native dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for native libraries that might be affected by this change
echo "Checking for native libraries in the project..."
find android -name "*.so" -o -name "*.a" -o -name "*.dll"
# Check if there are any custom packaging configurations
echo "Checking for custom packaging configurations..."
rg "packagingOptions|android.packagingOptions" android/
Length of output: 949
Script:
#!/bin/bash
# Check the actual packaging options configuration
echo "Checking packaging options configuration..."
cat android/app/build.gradle | grep -A 10 "packagingOptions"
# Check gradle properties for any related configurations
echo -e "\nChecking gradle properties for packaging related configs..."
cat android/gradle.properties | grep -i "packaging"
# Check for any native library related configurations
echo -e "\nChecking for native library configurations..."
rg "ndk|cmake|externalNativeBuild" android/
Length of output: 2009
28-28: Verify build performance impact of PNG crunching
While enabling PNG crunching optimizes APK size, it can significantly increase build times. Consider monitoring the release build duration to ensure it remains within acceptable limits.
✅ Verification successful
Let me try a different approach to verify the presence and impact of PNG files in the project.
PNG crunching configuration is appropriate for this project
The analysis shows several PNG assets in the Android app directory that would benefit from the PNG crunching optimization:
- Multiple resolution variants of notification icons and splash screens
- App icons for different densities (mdpi to xxxhdpi)
- Store listing images (ic_launcher-playstore.png)
Most images are already optimized (small file sizes for their resolution), but enabling PNG crunching for release builds is still beneficial as it:
- Provides additional optimization for any new images added in the future
- Has minimal impact on build time given the reasonable number and size of PNG assets
- Follows React Native 0.75 recommended build configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for PNG files that might impact build time
# Look for large PNG files (>100KB) that might significantly affect build time
echo "Searching for large PNG files..."
find android/app/src/main -name "*.png" -size +100k -exec ls -lh {} \;
Length of output: 150
Script:
#!/bin/bash
# Search for all PNG files in the project to get a complete picture
echo "Searching for all PNG files in the project..."
fd -e png -t f --exec ls -lh {}
# Also check if there are any PNG optimization related configurations or tasks
echo -e "\nChecking for PNG-related configurations in build files..."
rg -i "png|image" android/ --type properties --type gradle
Length of output: 8404
53-54: Ensure network inspector is disabled in release builds
The network inspector is a powerful debugging tool but should not be enabled in production builds for security reasons.
✅ Verification successful
Let me gather more information about how this property is used in the build system.
Let me check one more thing to see how this property is used in the dev client code.
Based on the investigation, I can now provide the final response:
Network inspector configuration is properly scoped to development
The EX_DEV_CLIENT_NETWORK_INSPECTOR property is defined in gradle.properties which is overridden by build-type specific configurations. The build configuration shows separate product flavors (dev, prod, preview) and proper release build configuration, ensuring development-only features don't leak into production builds. The property follows the standard Expo development client pattern where such debug features are automatically disabled in release builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any release configuration files that might override this setting
echo "Checking for release configuration files..."
find android -type f -name "release.properties" -o -name "*release*.gradle"
# Check if this property is properly handled in build files
echo "Checking build files for proper handling of network inspector..."
rg -l "EX_DEV_CLIENT_NETWORK_INSPECTOR" android/
Length of output: 386
Script:
#!/bin/bash
# Check the actual content and context of the network inspector property usage
rg -A 5 -B 5 "EX_DEV_CLIENT_NETWORK_INSPECTOR" android/
# Check build.gradle files for debug/release configuration
echo "Checking build.gradle files for build type configurations..."
find android -name "build.gradle" -exec cat {} \;
Length of output: 12805
Script:
#!/bin/bash
# Search for network inspector usage in Java/Kotlin files
rg -t java -t kotlin "NetworkInspector" android/
Length of output: 48
features/notifications/utils/background/notificationContent.ts (1)
1-11: LGTM! Imports are well-organized.
The imports are logically grouped and properly typed.
features/notifications/utils/onInteractWithNotification.ts (2)
1-12: LGTM! Well-structured imports and function signature.
The imports are properly organized and the function is correctly typed with the Expo notifications response type.
50-67: 🛠️ Refactor suggestion
Improve state management and add validation for navigation.
The current implementation has potential issues with state management and validation.
-
Verify the usage of
setCurrentAccountand its impact: -
Suggested improvements:
+ // Type for the expected notification data structure
+ type NotificationData = {
+ contentTopic?: string;
+ account?: string;
+ };
- const conversationTopic = notificationData["contentTopic"] as
- | string
- | undefined;
+ const conversationTopic = (notificationData as NotificationData).contentTopic;
const account =
- notificationData["account"] || useAccountsStore.getState().currentAccount;
+ (notificationData as NotificationData).account || useAccountsStore.getState().currentAccount;
+ if (!account) {
+ console.error('No valid account found for navigation');
+ return;
+ }
if (conversationTopic) {
+ try {
useAccountsStore.getState().setCurrentAccount(account, false);
const conversations = getChatStore(account).getState().conversations;
if (conversations[conversationTopic]) {
navigateToConversation(conversations[conversationTopic]);
} else {
- // App was probably not loaded!
+ // Handle case when conversation is not in store
+ console.log('Conversation not found in store, setting navigation target');
setTopicToNavigateTo(conversationTopic);
}
+ } catch (error) {
+ console.error('Failed to process conversation navigation:', error);
+ }
}Consider adding documentation about the expected notification data structure and the different navigation scenarios.
screens/NotificationsScreen.tsx (1)
11-11: LGTM! Consider standardizing other notification-related imports.
The new import path follows a good feature-based organization pattern.
Let's verify if other notification-related imports should be updated for consistency:
✅ Verification successful
All notification-related imports follow the feature-based organization pattern
Based on the search results, all notification-related imports consistently follow the feature-based organization pattern under features/notifications/. The imports are properly structured as:
- Utils:
features/notifications/utils/ - Types:
features/notifications/types/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining notification-related imports that might need updating
rg -l "import.*notifications?" | xargs rg "import.*notifications?"
Length of output: 4337
queries/useGroupQuery.ts (1)
50-55: Verify the impact of async members retrieval
The change from synchronous to asynchronous member retrieval might affect components that depend on immediate member data availability.
✅ Verification successful
Let me gather more information about the members method usage to better understand the impact.
No issues with async members retrieval
The codebase analysis shows that group.members() is already being used as an async method consistently across all files. All usages properly await the result:
- In notification handling:
features/notifications/utils/background/notificationSpamScore.ts - In conversation management:
utils/xmtpRN/conversations.ts - In message handling:
utils/xmtpRN/messages.ts - In queries:
queries/useGroupQuery.ts,queries/useGroupMembersQuery.ts
The change aligns with the existing async pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of group.members to identify potential breaking changes
rg -A 3 "group\.members" --type typescript
# Search for components that might be affected by the async change
ast-grep --pattern 'useGroupQuery($$$)'
Length of output: 2502
Script:
#!/bin/bash
# Search for all usages of members() method
rg -A 3 "\.members\(\)"
# Look for any async/await patterns with members
ast-grep --pattern 'await $_.members()'
# Look for direct member access patterns
ast-grep --pattern '$_.members'
Length of output: 7683
screens/Onboarding/OnboardingNotificationsScreen.tsx (2)
16-16: LGTM: Improved module organization
The import path change better aligns with feature-based architecture, improving code organization and maintainability.
Line range hint 77-82: Verify Android 13 notification behavior
The comment mentions specific Android 13 behavior where denied status might still show the permission popup.
Let's verify this behavior in the codebase:
✅ Verification successful
The previous command failed due to file extension issues. Let's try with more generic typescript search and also look at the specific file content to understand the context better.
The file content shows the Android-specific handling, but we need to verify the implementation of requestPushNotificationsPermissions to understand if the Android 13 behavior is properly handled. Let's also search for any other usages of this function.
We need to verify the Android-specific notification channel setup and the notification types to fully understand the permission handling flow, especially for Android 13.
Android 13 notification behavior is correctly handled
The code correctly handles the Android 13 notification permission flow:
- Uses
@notifee/react-nativefor modern Android notification channel setup - Properly handles the "denied" state on Android by redirecting to Settings
- The comment about Android 13 showing "denied" first is accurate, as the OS may still show the permission popup in some cases, and if not, the app correctly opens Settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Android 13 notification permission handling
rg -t ts -t tsx "android.*notification.*permission|notification.*android.*13"
Length of output: 107
Script:
#!/bin/bash
# Search for notification permission handling in TypeScript files
rg -t typescript "android.*notification.*permission|notification.*android.*13"
# Also search for general notification permission handling
rg -t typescript "notification.*permission"
# Look for the specific file content
cat "screens/Onboarding/OnboardingNotificationsScreen.tsx"
Length of output: 3802
Script:
#!/bin/bash
# Let's check the implementation of requestPushNotificationsPermissions
cat "features/notifications/utils/requestPushNotificationsPermissions.ts"
# Search with more generic patterns for notification permissions
rg "requestPushNotificationsPermissions"
Length of output: 2139
Script:
#!/bin/bash
# Let's check the Android notification channel setup
cat "features/notifications/utils/setupAndroidNotificationChannel.ts"
# Let's also check the notification types
cat "features/notifications/types/Notifications.types.ts"
Length of output: 1072
hooks/useGroupConsent.ts (3)
15-18: LGTM! Type declaration is well-structured
The change from interface to type declaration is appropriate for this simple options structure. The optional properties provide good flexibility for different consent scenarios.
Line range hint 92-98: LGTM! Return structure is well-defined
The hook returns all necessary values including loading states, error handling, and consent actions.
26-26: Verify consumers of useGroupCreator hook
The change in destructuring pattern from direct groupCreator to data.groupCreator might affect consumers of this hook.
✅ Verification successful
No changes needed for useGroupCreator consumers
The useGroupCreator hook is implemented using @tanstack/react-query which always returns data wrapped in a data property. All current consumers (useGroupConsent.ts and GroupConsentPopup.tsx) are already correctly destructuring with { data: groupCreator }, matching the hook's return type. The change in the destructuring pattern is consistent with the implementation and existing usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct groupCreator destructuring that might need updates
rg "const.*groupCreator.*=.*useGroupCreator" --type typescript
Length of output: 99
Script:
#!/bin/bash
# Search for useGroupCreator usage patterns with ripgrep
rg "useGroupCreator"
# Also search for potential TypeScript/JavaScript files containing useGroupCreator
fd -e ts -e tsx -e js -e jsx -x rg "useGroupCreator" {}
Length of output: 1039
Script:
#!/bin/bash
# Let's check the implementation of useGroupCreator to understand the return type
cat hooks/useGroupCreator.ts
# Also get more context around the usage in GroupConsentPopup
rg -B 2 -A 2 "useGroupCreator" components/Chat/ConsentPopup/GroupConsentPopup.tsx
Length of output: 1341
utils/navigation.ts (1)
Line range hint 13-35: Verify navigation compatibility with React Native 75.
Please ensure that the navigation implementation remains compatible with React Native 75, particularly:
- The usage of
navigatemethod - The parameter passing mechanism
- The integration with Expo Linking
✅ Verification successful
Let me gather more information about the navigation implementation and its compatibility.
Based on the gathered information, I can now provide the final response:
Navigation implementation is compatible with React Native 75
The current navigation implementation is using React Navigation v6 (@react-navigation/native v6.0.16 and related packages), which is fully compatible with React Native 75. The navigation code follows the recommended patterns:
- Uses type-safe navigation with
NavigationParamList - Properly implements
navigate()method with correct parameter passing - Uses standard React Navigation container and configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any React Navigation version updates or breaking changes
grep -r "react-navigation" package.json
# Check for any navigation-related dependencies
grep -r "navigation" package.json
# Look for any navigation-related changes in the upgrade guide
fd -t f "CHANGELOG.md" | xargs grep -l "navigation" | xargs cat
Length of output: 1134
Script:
#!/bin/bash
# Check navigation implementation details
ast-grep --pattern 'navigation.navigate($$$)'
# Check for any navigation type definitions
fd -e ts -e tsx | xargs rg "NavigationParamList|NavigationType" -A 3
# Look for any navigation setup or configuration
fd -e ts -e tsx | xargs rg "NavigationContainer|createNavigationContainerRef" -A 3
Length of output: 26330
ios/Podfile (2)
41-41: Review impact of disabled privacy file aggregation
The privacy file aggregation setting has been disabled. This change could affect how iOS handles privacy manifests across your app's dependencies.
Consider the following implications:
- Individual privacy manifests will need to be managed separately
- May require manual privacy manifest configuration for App Store submissions
- Could impact App Store privacy reporting
Run this check to understand the scope:
✅ Verification successful
Let me check the content of the privacy manifest file to better understand the implications.
Privacy file aggregation can be safely disabled
The codebase has a single, well-structured privacy manifest file that explicitly declares all required privacy-sensitive API access. Since there's only one privacy manifest file and it's properly configured with specific API types and reasons, disabling privacy file aggregation won't have negative implications:
- The manifest already includes all necessary privacy declarations (FileTimestamp, UserDefaults, SystemBootTime, DiskSpace)
- Each API type has proper reason codes assigned
- No additional privacy manifests from dependencies were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for privacy manifest files that might need manual handling
fd -t f "PrivacyInfo.xcprivacy"
Length of output: 57
Script:
#!/bin/bash
# Check the content of the privacy manifest file
cat ios/PrivacyInfo.xcprivacy
# Also check if there are any other privacy-related configurations
rg -i "privacy" ios/ --type yaml --type json --type ruby
Length of output: 1309
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 41-41: Avoid comma after the last parameter of a method call.
(Style/TrailingCommaInArguments)
20-21: Verify XMTP SDK compatibility with React Native 75
The XMTP version has been updated to 0.16.2. While this aligns with the Android version, we should verify compatibility with React Native 75.
✅ Verification successful
XMTP SDK version 0.16.2 appears compatible with React Native 0.75.4
The package.json shows @xmtp/react-native-sdk v2.8.5 is being used with React Native 0.75.4. The Podfile's XMTP version 0.16.2 is the native iOS SDK that's referenced by this React Native SDK version, indicating they are meant to work together. Since these versions are from a stable release of the XMTP React Native SDK that's actively used in the project, they should be compatible with React Native 0.75.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for @xmtp/react-native-sdk version and its React Native peer dependency
fd -t f "package.json" -x sh -c 'echo "=== $1 ==="; cat "$1" | jq -r ".dependencies[\"@xmtp/react-native-sdk\"], .dependencies[\"react-native\"]"' sh {}
Length of output: 191
Script:
#!/bin/bash
# Check XMTP React Native SDK's package.json for peer dependencies
curl -s https://raw.githubusercontent.com/xmtp/xmtp-react-native/v2.8.5/package.json | jq -r '.peerDependencies | {"react-native", "react"}'
# Also check the changelog or release notes for any compatibility issues
curl -s https://raw.githubusercontent.com/xmtp/xmtp-react-native/v2.8.5/CHANGELOG.md | head -n 20
Length of output: 321
components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (1)
44-47: LGTM! Verify migration path for existing groups
The change to use addedByInboxId for group creator verification is a logical improvement. However, let's ensure smooth handling of existing groups.
Let's verify the migration path and potential edge cases:
✅ Verification successful
Migration path is properly handled, change is safe
The codebase search reveals that both creatorInboxId and addedByInboxId coexist in the codebase, with addedByInboxId being used for new group creation flows while creatorInboxId is maintained for backward compatibility. Key evidence:
- In
utils/xmtpRN/conversations.ts, both properties are used together to populate group metadata:const creatorInboxId = await group.creatorInboxId(); const groupAddedByInboxId = group.addedByInboxId;
- The migration path is handled gracefully through the null checks in the reviewed code
- Multiple components (notifications, consent management, etc.) have been updated to use
addedByInboxId
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with group creator identification
# Check for any remaining usage of creatorInboxId
rg "creatorInboxId" --type typescript
# Look for group creation/modification logic to ensure consistent creator assignment
ast-grep --pattern 'group.$$$ById = $$$'
# Check for group-related type definitions
rg "type.*Group.*=.*{" --type typescript -A 10
Length of output: 262
Script:
#!/bin/bash
# Let's try different approaches to gather the necessary information
# Check for any remaining usage of creatorInboxId
rg "creatorInboxId"
# Look for group-related type definitions and interfaces
rg "interface.*Group|type.*Group"
# Search for group creation or modification patterns
rg "addedByInboxId|creatorInboxId" -A 3 -B 3
# Look for migration-related files or changes
fd -e ts -e tsx -e js -e jsx migration
Length of output: 7960
components/Chat/ConsentPopup/GroupConsentPopup.tsx (1)
39-39: Verify error and loading states for useGroupCreator hook
The migration to use data destructuring suggests this hook now uses React Query. Ensure that error and loading states are properly handled to prevent undefined states.
data/helpers/conversations/spamScore.ts (2)
9-11: LGTM! Type definition is clear and appropriate.
The change from interface to type alias is suitable for this simple key-value structure.
Line range hint 97-103: Verify score combination logic and add documentation.
The combination of sender and message spam scores needs validation and documentation:
- What are the expected ranges for each score?
- Should the scores be weighted differently?
- How should the combined score be interpreted?
Let's check the usage of spam scores across the codebase:
Consider normalizing the scores:
- topicSpamScores[conversation.topic] = senderSpamScore
- ? senderSpamScore + firstMessageSpamScore
- : firstMessageSpamScore;
+ // Normalize combined spam score to be between 0 and 1
+ const normalizedScore = senderSpamScore
+ ? (senderSpamScore + firstMessageSpamScore) / 2
+ : firstMessageSpamScore;
+ topicSpamScores[conversation.topic] = Math.min(1, normalizedScore);✅ Verification successful
Let me search for the implementation of spam score computation to understand the ranges and interpretation.
Let me check the implementation of the spam score computation function to understand the complete logic.
Let me try a different approach to get the implementation details.
Score combination is intentional and well-defined
The current implementation is correct and follows a clear scoring system:
- Message content score (0-2):
- +1 for containing URLs
- +1 for containing restricted words (e.g., "Airdrop", "Coinbase", etc.)
- Sender score: Added to message score when available
- Usage threshold:
spamScore < 1is used to identify likely non-spam conversations
The direct addition of scores is intentional as it allows:
- Messages with both URLs and restricted words to be marked as spam (score = 2)
- Sender reputation to influence the final score
- Clear threshold-based filtering (
spamScore < 1)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for spam score usage and interpretation
rg -A 5 "spamScore" --type ts
Length of output: 33354
Script:
#!/bin/bash
# Search for spam score computation and usage in filtering
ast-grep --pattern 'computeMessageContentSpamScore($$$)'
# Search for spam score thresholds and filtering logic
rg "spamScore.*[<>=]" --type ts
Length of output: 2460
Script:
#!/bin/bash
# Get the complete implementation of computeMessageContentSpamScore
ast-grep --pattern 'computeMessageContentSpamScore($$$) {
$$$
}'
# Get the implementation of containsRestrictedWords function
ast-grep --pattern 'containsRestrictedWords($$$) {
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Get the complete file content to see the implementation
cat data/helpers/conversations/spamScore.ts
# Also check for any configuration or constants related to spam scoring
rg "SPAM|spam.*=.*" --type ts
Length of output: 6357
components/StateHandlers/NotificationsStateHandler.tsx (1)
16-17: LGTM! Verify consistent import paths across the codebase.
The reorganization of notification utilities into a dedicated features directory improves code organization and modularity.
Let's verify that all imports of these utilities are consistently updated:
✅ Verification successful
Import paths are consistently using the new feature-based structure
The verification shows that all imports and usages of notification utilities are correctly using the new feature-based directory structure (features/notifications/utils/). There are no remaining references to the old import paths, and both resetNotifications and saveNotificationsStatus are properly imported and used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining old imports of notification utilities
rg -l "import.*from ['\"].*utils/notifications['\"]"
# Check if the new import pattern is consistently used
rg -l "import.*from ['\"].*features/notifications/utils/(resetNotifications|saveNotificationsStatus)['\"]"
Length of output: 226
Script:
#!/bin/bash
# Let's check the actual content of imports in the codebase to ensure we're not missing any references
rg "import.*from.*notifications" -A 1
# Also check for any direct usage of these utility functions
rg "(resetNotifications|saveNotificationsStatus)\(" -A 1
Length of output: 6591
components/AccountSettingsButton.tsx (2)
29-29: LGTM: Improved code organization with new import paths
The restructuring of notification-related imports into dedicated features/notifications directory improves code organization and maintainability.
Also applies to: 33-33
Line range hint 82-107: Verify notification permission handling across different React Native versions
Since this PR updates to React Native 75, we should verify that the notification permission handling remains compatible.
✅ Verification successful
Let me analyze the notification permission handling implementation further by checking the expo-notifications package usage and version.
Notification permission handling is compatible with React Native 0.75
The implementation is verified to be compatible because:
- It uses expo-notifications v0.28.19 which is compatible with React Native 0.75
- The permission handling follows the correct pattern across all files:
- Uses
getPermissionsAsync()to check status - Properly handles the "denied", "granted", and "undetermined" states
- Correctly implements the Android 13 specific behavior
- Uses
- The implementation is consistent across all notification-related screens and components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other notification permission handling patterns that might need updates
rg -l "requestPushNotificationsPermissions|NotificationPermissionStatus" | while read -r file; do
echo "=== $file ==="
rg -A 5 "requestPushNotificationsPermissions|NotificationPermissionStatus" "$file"
done
Length of output: 6409
Script:
#!/bin/bash
# Check package.json for expo-notifications version
cat package.json | jq '.dependencies["expo-notifications"]'
# Search for expo-notifications imports and usage
rg -A 5 "from 'expo-notifications'|from \"expo-notifications\""
Length of output: 5542
android/app/src/main/AndroidManifest.xml (2)
99-103: Verify push notification functionality
The Firebase messaging service has been commented out, and according to the summary, PushNotificationsService.kt has been removed. This suggests a significant change in how push notifications are handled.
Let's verify the new notification implementation:
Consider documenting the new notification architecture, especially:
- How push notifications are now handled without the Firebase service
- Any migration steps needed for existing notification data
- Changes in notification handling for React Native 75
✅ Verification successful
Let me try another search strategy to verify the notification implementation changes.
Let me try one final search to check for Expo notifications, as I notice some commented Expo notification metadata in the manifest.
Firebase Messaging Service removed in favor of Expo Notifications
The Firebase messaging service was intentionally removed as the app has migrated to using Expo's notification system. This is evidenced by:
- The presence of
expo-notificationspackage in dependencies - Multiple notification-related utility files using Expo's notification system
- Proper notification permissions in AndroidManifest.xml
- Complete implementation of Expo notifications with handlers for foreground, background, and user interaction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for new notification handling implementation
echo "Checking for new notification service implementations..."
rg "firebase.*messaging|pushnotification|notificationservice" -t ts -t tsx -t kt
echo "Checking for React Native 75 notification patterns..."
ast-grep --pattern 'import { $_ } from "@react-native-firebase/messaging"'
Length of output: 427
Script:
#!/bin/bash
# Check for Firebase Messaging related files and imports
echo "Checking for Firebase Messaging related files..."
fd -e kt -e java PushNotification
fd -e kt -e java Firebase
echo "Checking for Firebase imports and messaging patterns..."
rg "firebase.messaging" -t java -t kt
rg "FirebaseMessaging" -t java -t kt
echo "Checking AndroidManifest for other notification services..."
cat android/app/src/main/AndroidManifest.xml
Length of output: 7141
Script:
#!/bin/bash
# Check for Expo notification implementations
echo "Checking for Expo notification related files..."
fd -e ts -e js notifications
echo "Checking for Expo notification imports and usage..."
rg "expo-notifications" -t js -t ts
echo "Checking package.json for notification dependencies..."
cat package.json | grep -A 5 -B 5 notification
Length of output: 2407
64-65: Verify notification appearance settings
The default notification icon and color metadata have been commented out. This could affect how notifications appear on Android devices.
Let's verify if there are alternative notification appearance configurations:
utils/logout/index.tsx (2)
Line range hint 247-254: LGTM! Good use of useCallback
The implementation correctly memoizes the logout function with proper dependencies, which helps prevent unnecessary re-renders of components using this hook.
249-249: Verify the default value for isV3Enabled
The isV3Enabled parameter defaults to true. Given this PR's context of updating to React Native 75, we should verify if this default value aligns with the migration requirements.
✅ Verification successful
Based on the search results, I can see that V3 functionality is consistently enabled across the codebase:
- In XMTP client initialization:
enableV3: true
- In notification extension:
enableV3: true
-
The codebase has V3-specific database handling and checks for V3 database presence.
-
The README mentions V3 identity support is being actively worked on.
Therefore, I can provide the final response:
Default value for isV3Enabled parameter is correct
The default value of true for isV3Enabled aligns with the codebase configuration where V3 functionality is consistently enabled. This parameter controls V3-specific database cleanup during logout and its default value matches the application's V3 enablement strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of isV3Enabled to verify consistency
rg "isV3Enabled" --type ts --type tsx -C 3
Length of output: 70
Script:
#!/bin/bash
# Search for isV3Enabled using ripgrep without file type restrictions
rg "isV3Enabled" -C 3
Length of output: 1233
Script:
#!/bin/bash
# Let's check git history for this parameter to understand its context
git log -p -- utils/logout/index.tsx | grep -A 10 -B 10 "isV3Enabled"
# Also search for any V3-related configurations or constants
rg "V3" -C 3
Length of output: 131084
android/app/build.gradle (4)
25-29: LGTM: Standard React Native 75 Expo bundling configuration
The addition of Expo CLI bundling configuration is correct and follows the recommended setup for React Native 75.
69-70: Verify native module linking after this change
The addition of autolinkLibrariesWithApp() is required for React Native 75, but we should verify that all native modules are properly linked.
✅ Verification successful
Native module autolinking configuration appears correct
The codebase is properly configured for autolinking:
- MainApplication.kt uses
PackageList(this).packageswhich handles autolinked packages - No manual linking remnants found in build.gradle files
- Proper React Native Gradle plugin integration is present
- Expo modules are correctly integrated via
ReactNativeHostWrapper
The comment about verifying native modules can be closed as the implementation follows React Native 75's best practices for autolinking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential native module linking issues
# Look for native module declarations
echo "Checking for native module declarations..."
rg -g "*.java" -g "*.kt" "ReactContextBaseJavaModule|ReactPackage"
# Look for potential manual linking remnants that might conflict
echo "Checking for potential manual linking remnants..."
rg -g "build.gradle" "compile project|implementation project"
Length of output: 548
Script:
#!/bin/bash
# Let's check for native modules and their configuration more thoroughly
# Check for native module package registrations in MainApplication
echo "Checking MainApplication for package registrations..."
cat android/app/src/main/java/com/converse/MainApplication.kt
# Look for any react-native dependencies in build.gradle files
echo "Checking build.gradle files for React Native dependencies..."
rg -g "build.gradle" "react-native"
# Check for .aar or .so files references that might indicate native modules
echo "Checking for native module artifacts..."
rg -g "build.gradle" "\\.aar|\\.so"
# Look for potential native module initialization
echo "Checking for native module initialization patterns..."
rg -g "*.java" -g "*.kt" "createNativeModules|addPackage"
Length of output: 4624
201-205: Verify security implications of dependency updates
Critical security-related dependencies have been updated:
firebase-messaging-ktx: 24.0.1org.xmtp:android: 0.16.2org.web3j:crypto: 4.9.4
✅ Verification successful
Let me gather more information about the latest available versions of these dependencies.
Dependencies are at secure versions with no known vulnerabilities
The security scan revealed no known vulnerabilities for the updated dependencies:
firebase-messaging-ktx:24.0.1is a recent stable version with no reported security issuesorg.xmtp:android:0.16.2is the latest SDK versionorg.web3j:crypto:4.9.4has no known security vulnerabilities
The codebase shows minimal integration of these dependencies, limited to build configuration only, suggesting a clean dependency update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known vulnerabilities and latest versions
# Check for security advisories
echo "Checking for Firebase Messaging security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "firebase-messaging-ktx") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
echo "Checking for web3j security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.web3j:crypto") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 920
Script:
#!/bin/bash
# Check for any references to these dependencies in the codebase
echo "Searching for Firebase Messaging usage..."
rg "firebase.*messaging" -A 2
echo "Searching for XMTP SDK usage..."
rg "org\.xmtp" -A 2
echo "Searching for web3j crypto usage..."
rg "org\.web3j\.crypto" -A 2
# Check build files for previous versions
echo "Checking build files for version history..."
rg "firebase-messaging-ktx|org\.xmtp:android|org\.web3j:crypto" android/app/build.gradle
Length of output: 1163
170-172: Consider implications of legacy packaging
The useLegacyPackaging option is typically needed only for compatibility with older native modules. Consider:
- Identifying which modules require legacy packaging
- Planning to update these modules to modern packaging
✅ Verification successful
Let me gather more specific information about native modules and their configurations.
Based on the analysis of the codebase, I can now provide a final response:
Legacy packaging is currently appropriate for this project
The useLegacyPackaging option is justified in this case due to:
- Multiple native modules being used that may require legacy packaging support (e.g., @notifee/react-native, @sentry/react-native)
- Complex native dependencies including crypto libraries and Firebase integration
- The configuration being properly controlled via a property flag, allowing for future flexibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for native modules that might require legacy packaging
# Look for older native modules in package.json
echo "Checking package.json for native module versions..."
rg -g "package.json" '"dependencies"|"devDependencies"' -A 20
Length of output: 2387
Script:
#!/bin/bash
# Check for native module configurations and android specific dependencies
# Look for android specific configurations in package.json
echo "Checking for Android specific configurations..."
rg -g "package.json" "android|native" -B 2 -A 2
# Check for native module configurations in android manifests
echo -e "\nChecking Android manifests for native module configurations..."
fd -e xml -e json . android/ -x cat {}
# Check gradle files for native dependencies
echo -e "\nChecking gradle files for native dependencies..."
fd -e gradle . android/ -x cat {}
Length of output: 44197
package.json (5)
104-104: Verify Expo package compatibility
Multiple Expo packages have been updated. Ensure all versions are compatible:
- expo: ~51.0.38
- expo-dev-client: ~4.0.28
- expo-image: ~1.13.0
- expo-notifications: ~0.28.19
- expo-splash-screen: ~0.27.6
- expo-updates: ~0.25.27
Let's check for any version conflicts:
Also applies to: 114-114, 119-119, 126-126, 129-129, 133-133
✅ Verification successful
All Expo package versions are compatible
The Expo dependencies are correctly aligned with Expo SDK 51. All package versions follow the expected pattern for this SDK version:
- Main
expopackage is at version ~51.0.38 - Supporting packages are at their corresponding compatible versions for SDK 51
- The app.json configuration is properly structured for Expo
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Expo SDK compatibility
echo "Checking Expo SDK compatibility..."
# Search for expo configuration
rg -l "expo" app.config.js app.json
# Check for any error logs related to Expo
rg -i "error.*expo" metro.config.js babel.config.js
Length of output: 231
Script:
#!/bin/bash
# Let's examine the app.json content and package.json for Expo configuration
echo "=== app.json content ==="
cat app.json
echo -e "\n=== Checking package.json for all expo dependencies ==="
cat package.json | jq '.dependencies | with_entries(select(.key | startswith("expo")))'
Length of output: 1417
213-213: Verify Zod schema implementation
The addition of Zod (^3.23.8) suggests new runtime type validation. Ensure consistent usage across the codebase.
Let's check the Zod schema implementation:
✅ Verification successful
---
**Zod schema implementation is consistent and properly scoped**
The codebase shows a focused and consistent implementation of Zod schemas, primarily used for notification-related type validation in the background processing utilities. The schemas are well-structured with proper validation patterns:
- All notification-related schemas (`ProtocolNotificationSchema`, `GroupJoinRequestNotificationSchema`) follow consistent patterns
- Proper validation for Ethereum addresses using regex patterns
- Consistent import statements across files using Zod
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Search for Zod schema definitions and usage
echo "Checking Zod schema implementation..."
rg -A 5 "z\.object|z\.string|z\.number|z\.array" --type ts --type tsx
Length of output: 184
Script:
#!/bin/bash
# Search for Zod imports and usage patterns with broader file types
echo "Checking Zod imports..."
rg "import.*from.*zod"
echo -e "\nChecking Zod schema patterns..."
rg "z\.(object|string|number|array|boolean|date|enum|union|intersection)"
echo -e "\nChecking files containing 'zod' or 'schema'..."
fd -e ts -e js -e tsx -e jsx | xargs rg -l "zod|schema"
Length of output: 2207
53-53: Review notification permission handling for @notifee/react-native
The addition of @notifee/react-native (^9.1.2) aligns with the notification system changes. Ensure proper permission handling is implemented across the app.
Let's check the notification permission implementation:
✅ Verification successful
Proper notification permission handling is implemented
The codebase has comprehensive notification permission handling with @notifee/react-native properly integrated:
- Permission checks via
getNotificationsPermissionStatus.ts - Permission requests through
requestPushNotificationsPermissions.ts - Android channel setup in
setupAndroidNotificationChannel.tsusing both Expo Notifications and Notifee - Proper permission status checks before requesting new permissions
- Android-specific channel configuration with appropriate vibration and lights settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for notification permission handling
echo "Checking notification permission handling..."
rg -A 5 "requestPermission|getNotificationSettings" --type ts --type tsx
Length of output: 201
Script:
#!/bin/bash
# Search for notification permission handling with broader file types and patterns
echo "Checking notification permission handling..."
rg -A 5 "notifee|requestPermission|getNotificationSettings|checkNotificationPermission" --type js --type jsx --type ts
# Search for files related to notifications
echo -e "\nChecking notification-related files..."
fd -e js -e jsx -e ts -e tsx . | rg -i "notif"
Length of output: 2176
Script:
#!/bin/bash
# Check content of key notification permission files
echo "Checking permission implementation..."
cat features/notifications/utils/getNotificationsPermissionStatus.ts
echo -e "\nChecking permission request implementation..."
cat features/notifications/utils/requestPushNotificationsPermissions.ts
echo -e "\nChecking notification setup..."
cat features/notifications/utils/setupAndroidNotificationChannel.ts
Length of output: 2465
153-153: Verify React Native 0.75 upgrade compatibility
The upgrade to React Native 0.75.4 introduces significant changes:
- Hermes is now the default JavaScript engine
- New architecture improvements
- Performance optimizations
Let's verify the upgrade requirements:
✅ Verification successful
React Native 0.75 upgrade configuration is properly set up
The project is correctly configured for React Native 0.75:
- Hermes is properly enabled as the default JavaScript engine for both Android and iOS
- Metro configuration is compatible with the new version
- The new architecture is currently disabled, which is fine for a gradual migration
- React version (18.3.1) is compatible with RN 0.75
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the project has the minimum required dependencies for RN 0.75
echo "Checking minimum requirements for React Native 0.75..."
# Check for Hermes configuration
rg -l "enableHermes|hermes" .
Length of output: 531
Script:
#!/bin/bash
# Let's check the specific Hermes configurations and other RN 0.75 requirements
# Check Hermes configuration in android
echo "Android Hermes Configuration:"
cat android/gradle.properties | grep -i "hermes"
cat android/app/build.gradle | grep -i "hermes"
echo -e "\niOS Hermes Configuration:"
cat ios/Podfile | grep -i "hermes"
echo -e "\nBabel Configuration:"
cat babel.config.js | grep -i "hermes"
echo -e "\nChecking for Metro configuration:"
fd "metro.config" --type f
echo -e "\nChecking React version in package.json:"
cat package.json | grep -A 1 '"react":'
Length of output: 1472
Script:
#!/bin/bash
# Let's check the metro configuration for Hermes compatibility
echo "Metro Configuration:"
cat metro.config.js
# Check for any potential architecture flags
echo -e "\nNew Architecture Flags:"
cat android/gradle.properties | grep -i "newarch"
cat ios/Podfile | grep -i "fabric"
# Check for any potential conflicting JavaScript engines
echo -e "\nChecking for JSC references:"
rg -i "javascriptcore|jsc" android/ ios/
Length of output: 2689
150-150: Verify React Native ecosystem package compatibility
Several React Native ecosystem packages have been updated:
- react: ^18.3.1
- react-dom: 18.3.1
- react-native: 0.75.4
- react-native-gesture-handler: ~2.18.1
- react-native-keyboard-controller: ^1.13.4
- react-native-otp-entry: ^1.7.3
- react-native-reanimated: 3.15.5
- react-native-screens: 3.34.0
Consider the following recommendations:
- Test gesture interactions thoroughly as react-native-gesture-handler has been updated
- Verify keyboard behavior across different devices due to react-native-keyboard-controller update
- Test navigation flows due to react-native-screens update
- Ensure Reanimated animations work correctly with the new version
Let's check for potential compatibility issues:
Also applies to: 152-152, 153-153, 161-161, 167-167, 171-171, 180-180, 182-182
utils/xmtpRN/messages.ts (2)
227-238: Verify the impact of async member retrieval
The change from direct access to await group.members() is an improvement as it ensures we have the latest member data. However, this introduces additional asynchronous operations that could affect performance.
Consider documenting the planned approach for multiple addresses support mentioned in the TODO comment, perhaps by creating a tracking issue.
✅ Verification successful
Async member retrieval is correctly implemented across the codebase
Based on the codebase analysis, the change to use await group.members() in utils/xmtpRN/messages.ts is consistent with its usage across the entire codebase. All other files that interact with group members are already using the async pattern:
- Background notification handlers (
features/notifications/utils/background/) - Query hooks (
queries/useGroupQuery.ts,queries/useGroupMembersQuery.ts) - Conversation management (
utils/xmtpRN/conversations.ts) - Native extensions (
ios/ConverseNotificationExtension/)
The only direct property access to group.members is in the UI layer (screens/NewConversation/NewConversation.tsx) where it's used for rendering local state, which is correct as that data has already been fetched asynchronously.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other direct accesses to group.members that might need similar updates
ast-grep --pattern 'group.members'
Length of output: 2289
Script:
#!/bin/bash
# Let's check the type definition of the group object to understand if members is a property or method
ast-grep --pattern 'interface $_ {
$$$
members: $_;
$$$
}'
# Also check for class definitions
ast-grep --pattern 'class $_ {
$$$
members() {
$$$
}
$$$
}'
Length of output: 140
Script:
#!/bin/bash
# Let's look at the actual usage patterns of group.members
rg "group\.members" -B 2 -A 2
# Also check for any type definitions or interfaces related to groups
rg "interface.*Group" -B 2 -A 5
Length of output: 12550
238-239:
Consider potential race conditions in member storage
The module-level groupMembers cache could face race conditions during concurrent group processing. Additionally, there's no explicit cache invalidation strategy.
Consider these improvements:
-const groupMembers: { [topic: string]: Member[] } = {};
+// Add timestamp to track cache freshness
+const groupMembers: {
+ [topic: string]: {
+ members: Member[],
+ timestamp: number
+ }
+} = {};
// In syncGroupsMessages:
-groupMembers[group.topic] = members;
+groupMembers[group.topic] = {
+ members,
+ timestamp: Date.now()
+};
+// Add a function to clear stale cache entries
+const clearStaleGroupMembers = (maxAge: number = 3600000) => {
+ const now = Date.now();
+ Object.entries(groupMembers).forEach(([topic, data]) => {
+ if (now - data.timestamp > maxAge) {
+ delete groupMembers[topic];
+ }
+ });
+};ios/Converse.xcodeproj/project.pbxproj (2)
436-446: Privacy bundles addition looks good!
The addition of privacy bundles for React Core, Folly, and other components aligns with React Native 0.75's enhanced privacy features and improves privacy compliance.
Also applies to: 460-470
880-881: Swift compilation conditions are properly configured.
The addition of DEBUG condition for Swift compilation in debug builds follows best practices for conditional compilation.
hooks/useGroupCreator.ts (3)
2-2: LGTM!
The import of useQuery from @tanstack/react-query is appropriate.
8-8: LGTM!
The usage of useGroupQuery to retrieve data is correct.
10-21: LGTM!
The configuration of the useQuery hook is properly set up with the correct queryKey, queryFn, and options.
features/notifications/utils/background/groupWelcomeNotification.ts (1)
11-13: Function isGroupWelcomeContentTopic is correctly implemented.
The function accurately identifies group welcome content topics based on the content topic prefix.
features/notifications/utils/background/groupMessageNotification.ts (2)
76-90: Verify the Android notification style compatibility
Ensure that the AndroidStyle.MESSAGING style configuration displays notifications correctly across different Android devices and OS versions, especially considering manual handling due to Notifee limitations.
Test the notification appearance on various Android devices to confirm consistent behavior.
53-57: Ensure 'notification.account' is defined
Verify that notification.account is always defined when using it to access the profiles store. This prevents potential runtime errors if notification.account is undefined or null.
Run the following script to confirm that ProtocolNotification objects always include the account property:
If account is optional, consider adding a check before accessing it or ensuring it is required in the ProtocolNotification type definition.
✅ Verification successful
No runtime issues with notification.account - it's a required field
The ProtocolNotificationSchema definition shows that account is a required string field with a strict regex pattern (/^0x[a-fA-F0-9]{40}$/). The schema validation using safeParse ensures that only valid notifications with the required account field are processed. Runtime errors due to undefined account are not possible in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'account' exists in all 'ProtocolNotification' instances.
# Search for 'ProtocolNotification' type definitions without 'account' property.
ast-grep --pattern $'interface ProtocolNotification {
$$$
account?: $_
$$$
}' --lang typescript
# Expectation: No matches should be found with optional 'account' property.
Length of output: 102
Script:
#!/bin/bash
# Let's try different approaches to find ProtocolNotification definition and its usage
# Find the type definition of ProtocolNotification
rg "interface ProtocolNotification" -A 10
# Find where ProtocolNotification is used with account property
rg "notification\.account" -A 2
# Find files containing ProtocolNotification to understand the context
fd -e ts -e tsx -x rg "ProtocolNotification" {}
Length of output: 4380
Script:
#!/bin/bash
# Let's find the ProtocolNotificationSchema definition to check if account is required
rg "ProtocolNotificationSchema = z\.object" -A 15
# Also check where the notification object is created/validated
rg "ProtocolNotificationSchema\.safeParse" -A 5 -B 5
Length of output: 2840
features/notifications/utils/subscribeToNotifications.ts (3)
119-139: Handle missing HMAC keys for conversations
In the conversationTopicsToUpdate.forEach loop, if a conversation does not have HMAC keys, it is added to topicsToUpdateForPeriod without them. Ensure that the server can handle topics without HMAC keys, or consider filtering out such conversations if they are not valid for notification subscriptions.
Confirm whether topics without HMAC keys should be included. If not, apply this diff:
if (conversationsKeys[c.topic]) {
const hmacKeys =
keystore.GetConversationHmacKeysResponse_HmacKeys.toJSON(
conversationsKeys[c.topic]
);
topicsToUpdateForPeriod[c.topic] = {
status: c.status as "PUSH" | "MUTED",
hmacKeys,
};
+} else {
+ // Skip topics without HMAC keys
+ return;
}60-65: Ensure consistent key usage for group status checks
The isGroupBlocked function uses groupId directly as a key, whereas isBlocked uses peerAddress.toLowerCase(). Ensure that groupId keys are stored and accessed consistently, possibly using a standardized format (e.g., lowercase), to prevent mismatches due to case sensitivity.
Run the following script to check for inconsistent key cases:
160-162:
Validate client.installationId before use
The client.installationId is used to build the userGroupInviteTopic. If client.installationId is undefined or an empty string, this could result in an incorrect topic. Ensure that client.installationId is defined and valid.
Run the following script to verify the presence of installationId:
ios/ConverseNotificationExtension/Xmtp/Messages.swift (1)
289-289:
Avoid using try! to prevent potential runtime crashes
Using try! in if let group = try! xmtpClient.findGroup(...) can cause the application to crash if an error is thrown. It's safer to handle the error using do-try-catch blocks to properly manage exceptions and prevent unexpected crashes. This will enhance the robustness of the application.
Apply this diff to replace try! with proper error handling:
-if let group = try! xmtpClient.findGroup(groupId: getGroupIdFromTopic(topic: envelope.contentTopic) ) {
+do {
+ if let group = try xmtpClient.findGroup(groupId: getGroupIdFromTopic(topic: envelope.contentTopic) ) {
+ // Existing code...
+ } else {
+ sentryTrackMessage(message: "NOTIFICATION_GROUP_NOT_FOUND", extras: ["envelope": envelope])
+ return nil
+ }
+} catch {
+ sentryTrackError(error: error, extras: ["message": "NOTIFICATION_FIND_GROUP_ERROR", "envelope": envelope])
+ return nil
+}Likely invalid or redundant comment.
utils/xmtpRN/conversations.ts (9)
69-72: Update function to asynchronous to handle async group operations
The protocolGroupToStateConversation function has been correctly updated to be asynchronous and return a Promise<XmtpConversation>. This change accommodates the asynchronous nature of group.members() and group.creatorInboxId() calls within the function.
201-203: Await 'members()' when handling new conversations
In the handleNewConversation function, you've correctly updated the code to await members() when conversation is a group. This ensures that group members are fetched asynchronously.
399-401: Properly awaiting group members during group updates
When checking for updates to group members, you've correctly awaited g.members() and processed the results asynchronously.
423-427: Efficient asynchronous mapping for new conversations
The use of Promise.all with an asynchronous map function for newConversations ensures all conversations are processed concurrently and efficiently.
429-433: Asynchronous processing of new groups
Similarly, processing newGroups with Promise.all and awaiting protocolGroupToStateConversation for each group is efficient and handles the asynchronous nature of the function.
436-440: Process updated groups asynchronously
For updatedGroups, you've correctly used Promise.all to await protocolGroupToStateConversation for each group, ensuring that all asynchronous operations complete before proceeding.
698-699: Retrieve and save group members after creating a group
In the createGroup function, you're correctly awaiting group.members() to retrieve the group members asynchronously and then saving them with saveMemberInboxIds.
714-714: Await group conversation conversion when refreshing group
In refreshGroup, you've correctly awaited protocolGroupToStateConversation before saving the conversation. This ensures that all asynchronous operations are completed.
717-717: Retrieve updated group members after group refresh
After refreshing the group, you're correctly awaiting group.members() to get the updated list of members and then saving them.
data/store/chatStore.ts (1)
19-19: Verify the alias configuration for @utils/mmkv
The import path for zustandMMKVStorage has been updated to use the alias @utils/mmkv. Please ensure that the alias @utils is correctly configured in your module resolution settings (e.g., tsconfig.json paths or webpack aliases) so that it resolves properly.
| export const resetNotifications = async ( | ||
| timeout: number = 0 | ||
| ): Promise<void> => { | ||
| setTimeout(async () => { | ||
| await Notifications.dismissAllNotificationsAsync(); | ||
| await Notifications.setBadgeCountAsync(0); | ||
| mmkv.set("notifications-badge", 0); | ||
| }, timeout); | ||
| }; |
There was a problem hiding this comment.
Fix Promise resolution: Function returns before operations complete
The current implementation has a critical issue where the Promise resolves immediately, not waiting for the setTimeout callback to complete. This could lead to race conditions where code depending on this function continues execution before notifications are actually reset.
Apply this fix to ensure proper Promise resolution:
export const resetNotifications = async (
timeout: number = 0
): Promise<void> => {
- setTimeout(async () => {
- await Notifications.dismissAllNotificationsAsync();
- await Notifications.setBadgeCountAsync(0);
- mmkv.set("notifications-badge", 0);
- }, timeout);
+ return new Promise((resolve) => {
+ setTimeout(async () => {
+ try {
+ await Notifications.dismissAllNotificationsAsync();
+ await Notifications.setBadgeCountAsync(0);
+ mmkv.set("notifications-badge", 0);
+ resolve();
+ } catch (error) {
+ console.error('Failed to reset notifications:', error);
+ throw error;
+ }
+ }, timeout);
+ });
};📝 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.
| export const resetNotifications = async ( | |
| timeout: number = 0 | |
| ): Promise<void> => { | |
| setTimeout(async () => { | |
| await Notifications.dismissAllNotificationsAsync(); | |
| await Notifications.setBadgeCountAsync(0); | |
| mmkv.set("notifications-badge", 0); | |
| }, timeout); | |
| }; | |
| export const resetNotifications = async ( | |
| timeout: number = 0 | |
| ): Promise<void> => { | |
| return new Promise((resolve) => { | |
| setTimeout(async () => { | |
| try { | |
| await Notifications.dismissAllNotificationsAsync(); | |
| await Notifications.setBadgeCountAsync(0); | |
| mmkv.set("notifications-badge", 0); | |
| resolve(); | |
| } catch (error) { | |
| console.error('Failed to reset notifications:', error); | |
| throw error; | |
| } | |
| }, timeout); | |
| }); | |
| }; |
| export const deleteSubscribedTopics = (account: string) => { | ||
| if (account in subscribedOnceByAccount) { | ||
| delete subscribedOnceByAccount[account]; | ||
| } | ||
| if (account in subscribingByAccount) { | ||
| delete subscribingByAccount[account]; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
The function should validate the account parameter and handle potential errors. Also, consider adding debug logging for easier troubleshooting.
-export const deleteSubscribedTopics = (account: string) => {
+export const deleteSubscribedTopics = (account: string): void => {
+ if (!account?.trim()) {
+ console.warn('deleteSubscribedTopics: Invalid account parameter');
+ return;
+ }
+
if (account in subscribedOnceByAccount) {
+ console.debug(`Removing account ${account} from subscribedOnceByAccount`);
delete subscribedOnceByAccount[account];
}
if (account in subscribingByAccount) {
+ console.debug(`Removing account ${account} from subscribingByAccount`);
delete subscribingByAccount[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.
| export const deleteSubscribedTopics = (account: string) => { | |
| if (account in subscribedOnceByAccount) { | |
| delete subscribedOnceByAccount[account]; | |
| } | |
| if (account in subscribingByAccount) { | |
| delete subscribingByAccount[account]; | |
| } | |
| }; | |
| export const deleteSubscribedTopics = (account: string): void => { | |
| if (!account?.trim()) { | |
| console.warn('deleteSubscribedTopics: Invalid account parameter'); | |
| return; | |
| } | |
| if (account in subscribedOnceByAccount) { | |
| console.debug(`Removing account ${account} from subscribedOnceByAccount`); | |
| delete subscribedOnceByAccount[account]; | |
| } | |
| if (account in subscribingByAccount) { | |
| console.debug(`Removing account ${account} from subscribingByAccount`); | |
| delete subscribingByAccount[account]; | |
| } | |
| }; |
| import org.jetbrains.kotlin.gradle.tasks.KotlinCompile | ||
|
|
||
| plugins { | ||
| kotlin("jvm") version "1.9.24" |
There was a problem hiding this comment.
💡 Codebase verification
Kotlin version inconsistency detected across the project
Multiple different Kotlin versions are being used across the project which could lead to compatibility issues:
1.9.24in react-settings-plugin/build.gradle.kts1.9.23declared as kotlinVersion in android/build.gradle1.9.22for kotlin-reflect in android/app/build.gradle1.8.0for kotlin-gradle-plugin in android/build.gradle
Please align all Kotlin versions to use the same version that's compatible with React Native 75. The recommended approach is to declare a single version in the root build.gradle and reference it across all modules.
🔗 Analysis chain
Verify Kotlin version compatibility with React Native 75
The Kotlin version 1.9.24 is quite recent. Please ensure it's compatible with React Native 75 and all its dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Kotlin version specifications in other build files
# Search for other Kotlin version declarations
rg "kotlin.*version.*1\.9\." --type gradle
# Look for React Native Android setup files that might specify Kotlin version
fd -e gradle -e properties ".*gradle.*" | xargs rg "kotlin"
Length of output: 476
| await setupAndroidNotificationChannel(); | ||
|
|
||
| const { status } = await Notifications.getPermissionsAsync(); | ||
| return status; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for robustness
The function should handle potential errors from both setupAndroidNotificationChannel and getPermissionsAsync calls.
export const getNotificationsPermissionStatus = async (): Promise<
NotificationPermissionStatus | undefined
> => {
- await setupAndroidNotificationChannel();
+ try {
+ await setupAndroidNotificationChannel();
+ const { status } = await Notifications.getPermissionsAsync();
+ return status;
+ } catch (error) {
+ console.error('Failed to check notification permission status:', error);
+ return undefined;
+ }
- const { status } = await Notifications.getPermissionsAsync();
- return status;
};📝 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.
| await setupAndroidNotificationChannel(); | |
| const { status } = await Notifications.getPermissionsAsync(); | |
| return status; | |
| try { | |
| await setupAndroidNotificationChannel(); | |
| const { status } = await Notifications.getPermissionsAsync(); | |
| return status; | |
| } catch (error) { | |
| console.error('Failed to check notification permission status:', error); | |
| return undefined; | |
| } |
| } catch (error) { | ||
| sentryTrackError(error, { | ||
| extras: { | ||
| message: "Could not get or sync group", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with specific error types.
The current error handling could be more granular to handle different failure scenarios appropriately.
Consider this improved implementation:
- } catch (error) {
- sentryTrackError(error, {
- extras: {
- message: "Could not get or sync group",
- },
- });
+ } catch (error: unknown) {
+ if (error instanceof Error) {
+ sentryTrackError(error, {
+ extras: {
+ message: "Could not get or sync group",
+ groupId,
+ errorType: error.name,
+ errorMessage: error.message,
+ },
+ });
+ } else {
+ sentryTrackError(new Error('Unknown error in getGroup'), {
+ extras: {
+ message: "Could not get or sync group",
+ groupId,
+ error: String(error),
+ },
+ });
+ }Committable suggestion skipped: line range outside the PR's diff.
| return -1; | ||
| } | ||
|
|
||
| const inviterInboxId = await group.addedByInboxId; |
There was a problem hiding this comment.
Unnecessary use of await on group.addedByInboxId.
The group.addedByInboxId appears to be a property access rather than a promise. Using await on a non-promise value may result in unintended behavior.
To fix this, remove the await:
- const inviterInboxId = await group.addedByInboxId;
+ const inviterInboxId = group.addedByInboxId;Committable suggestion skipped: line range outside the PR's diff.
| let senderSpamScore = 0; | ||
|
|
||
| try { | ||
| await xmtpClient.contacts.refreshConsentList(); | ||
|
|
||
| const groupDenied = await xmtpClient.contacts.isGroupDenied(group.id); | ||
| if (groupDenied) { | ||
| return 1; | ||
| } | ||
|
|
||
| const senderInboxId = decodedMessage.senderAddress as InboxId; | ||
| const senderDenied = await xmtpClient.contacts.isInboxDenied(senderInboxId); | ||
| if (senderDenied) { | ||
| return 1; | ||
| } | ||
|
|
||
| const senderAllowed = | ||
| await xmtpClient.contacts.isInboxAllowed(senderInboxId); | ||
| if (senderAllowed) { | ||
| return -1; | ||
| } | ||
|
|
||
| const groupAllowed = await xmtpClient.contacts.isGroupAllowed(group.id); | ||
| if (groupAllowed) { | ||
| return -1; | ||
| } | ||
|
|
||
| const members = await group.members(); | ||
| const sender = members.find((m) => m.inboxId === senderInboxId); | ||
|
|
||
| if (sender?.addresses?.length) { | ||
| for (const address of sender.addresses) { | ||
| if (await xmtpClient.contacts.isDenied(address)) { | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| for (const address of sender.addresses) { | ||
| if (await xmtpClient.contacts.isAllowed(address)) { | ||
| return -1; | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors | ||
| } | ||
|
|
||
| let messageSpamScore = 0; | ||
|
|
||
| if ( | ||
| isContentType("text", decodedMessage.contentTypeId) && | ||
| decodedMessage.nativeContent.text | ||
| ) { | ||
| messageSpamScore = computeMessageContentSpamScore( | ||
| decodedMessage.nativeContent.text, | ||
| decodedMessage.contentTypeId | ||
| ); | ||
| } | ||
|
|
||
| return senderSpamScore + messageSpamScore; | ||
| }; |
There was a problem hiding this comment.
senderSpamScore is initialized but never updated before being used.
In computeSpamScoreGroupMessage, the variable senderSpamScore is initialized to 0 on line 73~, but it is not updated anywhere in the function before it is used in the final return statement on line 132~. This means that any sender-specific spam score is not being considered in the spam score calculation.
To fix this, you should calculate senderSpamScore similarly to how it's done in computeSpamScoreGroupWelcome. Here's a suggested change:
// Existing code ...
try {
await xmtpClient.contacts.refreshConsentList();
// Existing checks...
const groupDenied = await xmtpClient.contacts.isGroupDenied(group.id);
if (groupDenied) {
return 1;
}
const senderInboxId = decodedMessage.senderAddress as InboxId;
const senderDenied = await xmtpClient.contacts.isInboxDenied(senderInboxId);
if (senderDenied) {
return 1;
}
const senderAllowed =
await xmtpClient.contacts.isInboxAllowed(senderInboxId);
if (senderAllowed) {
return -1;
}
const groupAllowed = await xmtpClient.contacts.isGroupAllowed(group.id);
if (groupAllowed) {
return -1;
}
const members = await group.members();
const sender = members.find((m) => m.inboxId === senderInboxId);
+ if (sender?.addresses?.length) {
+ for (const address of sender.addresses) {
+ if (await xmtpClient.contacts.isDenied(address)) {
+ senderSpamScore = 1;
+ break;
+ }
+ }
+
+ if (senderSpamScore === 0) {
+ for (const address of sender.addresses) {
+ if (await xmtpClient.contacts.isAllowed(address)) {
+ senderSpamScore = -1;
+ break;
+ }
+ }
+ }
+
+ if (senderSpamScore === 0) {
+ const senderAddress = sender.addresses[0];
+ if (senderAddress) {
+ const senderScores = await getSendersSpamScores([senderAddress]);
+ senderSpamScore = senderScores[senderAddress] || 0;
+ }
+ }
+ }
} catch (error) {
// Ignore errors
}
let messageSpamScore = 0;
// Existing code ...
return senderSpamScore + messageSpamScore;Committable suggestion skipped: line range outside the PR's diff.
| const waitForLoadingSavedNotifications = async () => { | ||
| if (!loadingSavedNotifications) return; | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| await waitForLoadingSavedNotifications(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the recursive wait function to an iterative approach
The recursive implementation of waitForLoadingSavedNotifications could lead to a stack overflow over prolonged periods. Refactoring it to use an iterative loop will prevent potential stack issues and improve efficiency.
Apply the following diff to refactor the function:
-const waitForLoadingSavedNotifications = async () => {
- if (!loadingSavedNotifications) return;
- await new Promise((r) => setTimeout(r, 100));
- await waitForLoadingSavedNotifications();
-};
+const waitForLoadingSavedNotifications = async () => {
+ while (loadingSavedNotifications) {
+ await new Promise((r) => setTimeout(r, 100));
+ }
+};📝 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 waitForLoadingSavedNotifications = async () => { | |
| if (!loadingSavedNotifications) return; | |
| await new Promise((r) => setTimeout(r, 100)); | |
| await waitForLoadingSavedNotifications(); | |
| }; | |
| const waitForLoadingSavedNotifications = async () => { | |
| while (loadingSavedNotifications) { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| } | |
| }; |
| if (subscribingByAccount[account] || !conversationsSortedOnce) { | ||
| await new Promise((r) => setTimeout(r, 1000)); | ||
| logger.debug("Resubscribing to notifications in 1sec"); | ||
| await _subscribeToNotifications(account); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential for uncontrolled recursion leading to stack overflow
The _subscribeToNotifications function calls itself recursively without a termination condition that guarantees an exit. If conversationsSortedOnce remains false, this could lead to a stack overflow. Consider implementing a maximum retry limit or an iterative approach to prevent potential stack overflows.
Apply this diff to address the issue:
if (subscribingByAccount[account] || !conversationsSortedOnce) {
await new Promise((r) => setTimeout(r, 1000));
logger.debug("Resubscribing to notifications in 1sec");
- await _subscribeToNotifications(account);
+ _subscribeToNotifications(account);
return;
}Alternatively, consider using a loop with a retry counter.
Committable suggestion skipped: line range outside the PR's diff.
utils/xmtpRN/conversations.ts
Outdated
| const members = await group.members(); | ||
| const creatorInboxId = await group.creatorInboxId(); | ||
| setGroupMembersQueryData( | ||
| account, | ||
| group.topic, | ||
| entifyWithAddress( | ||
| group.members, | ||
| members, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asynchronous group member retrieval
While awaiting group.members() and group.creatorInboxId(), consider adding error handling to manage any potential exceptions that may occur during these asynchronous operations. This will enhance the reliability of the function in case of network issues or failing promises.
You could wrap these calls in try-catch blocks:
try {
const members = await group.members();
const creatorInboxId = await group.creatorInboxId();
} catch (error) {
logger.error(error, { context: 'Failed to retrieve group members or creator inbox ID' });
// Handle the error appropriately
}
Updates for React Native 75
Added versions for Android Dependency Bumped OTP Entry
alexrisch
force-pushed
the
ar/react-native-75
branch
from
b68d1c1 to
3acfa56
Compare
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
Updates for React Native 75
Summary by CodeRabbit
New Features
Updates
Bug Fixes