Ml/conv 1130 inconsistent notification deep link to chat #1325
Conversation
WalkthroughThe pull request includes modifications across several components, primarily focusing on improving logging, simplifying control flows, and enhancing error handling. Key changes involve the Changes
Assessment against linked 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 (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const senderAccount = | ||
| /* TODO notification.request.content.data?.["sender"]; */ "todo"; |
There was a problem hiding this comment.
Setting senderAccount to the hardcoded string "todo" will cause incorrect notification behavior. The userIsNotSender check will evaluate to true whenever currentAccount() is not "todo", rather than comparing against the actual sender's address. Consider using notification.request.content.data?.["sender"] directly, or if that data isn't available yet, implement proper sender tracking before enabling this check.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
components/StateHandlers/HydrationStateHandler.tsx (1)
Line range hint
21-31: Add error handling and improve hydration completion trackingThe hydration process is critical for notification handling but lacks:
- Error handling for getXmtpClient calls
- Clear indication when all XMTP clients are ready
- Proper sequencing with notification handling
This could explain the 50% success rate mentioned in the PR objectives.
Consider this implementation:
- accounts.map((a) => getXmtpClient(a)); + try { + await Promise.all(accounts.map(async (a) => { + await getXmtpClient(a); + })); + logger.debug('[Hydration] All XMTP clients ready'); + } catch (error) { + logger.error('[Hydration] Failed to initialize XMTP clients:', error); + }components/StateHandlers/InitialStateHandler.tsx (1)
11-14: Remove unused importsThe
setTopicToNavigateToandtopicToNavigateToimports appear to be unused. If they're intended for notification handling, their implementation is incomplete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
components/StateHandlers/HydrationStateHandler.tsx(1 hunks)components/StateHandlers/InitialStateHandler.tsx(3 hunks)data/store/accountsStore.ts(1 hunks)features/notifications/utils/handleForegroundNotification.ts(1 hunks)features/notifications/utils/onInteractWithNotification.ts(2 hunks)utils/navigation.ts(1 hunks)
🔇 Additional comments (6)
features/notifications/utils/handleForegroundNotification.ts (1)
17-18:
Critical: Fix hardcoded sender address
The hardcoded "todo" value for senderAccount will cause incorrect notification behavior, as previously noted.
features/notifications/utils/onInteractWithNotification.ts (3)
61-63: Good addition: Ensuring conversation list is fetched before navigation
This is a crucial fix that addresses the hydration issue by ensuring the conversation data is available before navigation.
66-68: Good cleanup: Proper state management after navigation
The addition of setTopicToNavigateTo(undefined) and resetNotifications() ensures clean state after navigation.
58-60: 🛠️ Refactor suggestion
Consider handling potential undefined account more robustly
The fallback to currentAccount could be problematic if the store isn't properly hydrated yet.
Consider adding a guard clause:
- const account =
- notificationData["account"] || useAccountsStore.getState().currentAccount;
+ const account = notificationData["account"];
+ if (!account) {
+ const currentAccount = useAccountsStore.getState().currentAccount;
+ if (!currentAccount || currentAccount === TEMPORARY_ACCOUNT_NAME) {
+ logger.warn("[Notification] No valid account found for notification");
+ return;
+ }
+ account = currentAccount;
+ }utils/navigation.ts (1)
29-32: Good improvement: Better navigation state management
The change to allow undefined as a valid state for topicToNavigateTo improves the state management lifecycle and helps prevent stale navigation targets.
data/store/accountsStore.ts (1)
204-209: Good addition: Enhanced hydration debugging
The added debug logging will help track and diagnose hydration-related issues, which is particularly valuable given the notification deep linking problems were related to hydration state.
| // note(lustig) I don't think this does anything? | ||
| getInstalledWallets(false); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
The getInstalledWallets call is redundant and should be removed
The implementation shows that getInstalledWallets caches its results using the hasCheckedInstalled flag. When called with refresh: false, it returns the cached results without performing any checks. In the HydrationStateHandler, the second call with false is redundant since it's called immediately after the first awaited call, also with false. This second call will simply return the cached results without any effect.
- Remove the redundant call at line 20 in
components/StateHandlers/HydrationStateHandler.tsx
🔗 Analysis chain
Remove or justify the questionable getInstalledWallets call
The commented note suggests uncertainty about this call's purpose. If it's unnecessary, it should be removed to avoid potential performance impact during hydration.
Let me analyze the usage of getInstalledWallets further to understand its purpose and the impact of the call.
Let me try a different pattern to understand the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of getInstalledWallets to understand its purpose
rg "getInstalledWallets" -A 5
Length of output: 2646
Script:
#!/bin/bash
# Check the implementation and usage of getInstalledWallets
ast-grep --pattern 'const getInstalledWallets = async ($$$) => {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Get the content of the file containing getInstalledWallets implementation
cat components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts
Length of output: 6987
There was a problem hiding this comment.
this is inaccurate. there's an if else
There was a problem hiding this comment.
@technoplato, thank you for pointing out the oversight in my analysis. You're correct; the calls to getInstalledWallets(false); are in separate branches of the if-else statement, and they are necessary in both scenarios. I apologize for the confusion.
✏️ Learnings added
Learnt from: technoplato
PR: ephemeraHQ/converse-app#1325
File: components/StateHandlers/HydrationStateHandler.tsx:18-20
Timestamp: 2024-12-09T14:51:51.030Z
Learning: In `components/StateHandlers/HydrationStateHandler.tsx`, the calls to `getInstalledWallets(false);` are in separate branches of an `if-else` statement and are required for both cases when `accounts.length === 0` and when `accounts.length > 0`. I should carefully examine conditional structures before suggesting code is redundant.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (isDevelopmentClientURL(initialURL)) { | ||
| return; | ||
| } | ||
|
|
||
| Linking.openURL(initialURL); | ||
| // Once opened, let's remove so we don't navigate twice | ||
| // when logging out / relogging in | ||
| initialURL = ""; |
There was a problem hiding this comment.
Potential race condition with notification deep links
The current implementation clears initialURL after opening, which could interfere with notification-triggered navigation if the notification is processed during app startup.
Consider:
- Coordinating between notification and deep link handling
- Preserving navigation intent from notifications during hydration
- Linking.openURL(initialURL);
- // Once opened, let's remove so we don't navigate twice
- // when logging out / relogging in
- initialURL = "";
+ if (initialURL) {
+ try {
+ await Linking.openURL(initialURL);
+ logger.debug('[Navigation] Successfully opened initial URL:', initialURL);
+ } catch (error) {
+ logger.error('[Navigation] Failed to open initial URL:', error);
+ } finally {
+ // Store the last processed URL to prevent duplicate navigation
+ lastProcessedURL = initialURL;
+ initialURL = "";
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| const isViewingNotifiedConversation = /* TODO */ false; | ||
| const hackSoWeDontShowNotificationToSender = | ||
| /* todo(lustig): remove once we get senderAddress */ recipientAccount !== | ||
| currentAccount(); | ||
| const shouldShowAlert = | ||
| userIsNotSender && | ||
| !isViewingNotifiedConversation && | ||
| hackSoWeDontShowNotificationToSender; |
There was a problem hiding this comment.
Implement proper conversation state tracking
Several issues in the notification logic:
isViewingNotifiedConversationis hardcoded to false- The hack to prevent sender notifications is unreliable
- Multiple TODOs indicate incomplete implementation
This could contribute to the inconsistent notification behavior.
Consider this implementation:
- const isViewingNotifiedConversation = /* TODO */ false;
- const hackSoWeDontShowNotificationToSender =
- /* todo(lustig): remove once we get senderAddress */ recipientAccount !==
- currentAccount();
+ const conversationId = notification.request.content.data?.["conversationId"];
+ const isViewingNotifiedConversation = conversationId === getCurrentConversationId();
+ const senderAddress = notification.request.content.data?.["sender"];
+ const userIsNotSender = senderAddress && senderAddress !== currentAccount();Committable suggestion skipped: line range outside the PR's diff.
| // note(lustig): these two handlers are the same - do we need both? | ||
| const handleiOSForegroundNotification = async ( |
There was a problem hiding this comment.
💡 Codebase verification
Consolidate notification handlers - iOS-specific logic is incomplete
The iOS handler contains incomplete TODOs and temporary hacks, while the default handler implements a simpler, complete version of the same logic. Specifically:
- The iOS handler has unimplemented
TODOs forsenderAccountandisViewingNotifiedConversation - Contains a temporary hack
hackSoWeDontShowNotificationToSender - Both handlers essentially check if the notification is for the current account
Recommendations:
- Remove the iOS-specific handler and use the simpler
handleDefaultForegroundNotificationfor all platforms - If iOS-specific features are needed in the future, implement them within a single handler using platform-specific conditionals
🔗 Analysis chain
Clarify the need for separate notification handlers
The comment questions whether both handlers are needed. This should be investigated and documented.
Let me gather more information about the implementation details of these handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for platform-specific notification handling differences
rg -A 10 "handleiOSForegroundNotification|handleDefaultForegroundNotification"
# Search for iOS-specific notification code
rg "notification.*ios" -g "!*.test.*"
Length of output: 2822
Script:
#!/bin/bash
# Search for the entire content of the handleForegroundNotification.ts file
cat features/notifications/utils/handleForegroundNotification.ts
# Look for any other files that might be importing or using these handlers
rg "handleiOSForegroundNotification|handleDefaultForegroundNotification" --type ts
Length of output: 2880
| const recipientAccount = notification.request.content.data?.["account"]; | ||
|
|
||
| const senderAccount = | ||
| /* TODO notification.request.content.data?.["sender"]; */ "todo"; |
There was a problem hiding this comment.
I've left this half baked code in intentionally and was planning on a follow up, so wanted to leave breadcrumbs for the logic we need.
You cool leaving or want me to remove and make an Issue instead?
| setTopicToNavigateTo(conversationTopic); | ||
| } | ||
| navigateToTopic(conversationTopic as ConversationTopic); | ||
| setTopicToNavigateTo(undefined); |
There was a problem hiding this comment.
Is this doing anything any more?
There was a problem hiding this comment.
I think this lingering around was responsible for the bug where you'd come back into the app and it'd try and deep link you again, so yes!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/StateHandlers/InitialStateHandler.tsx (2)
Line range hint
31-35: Add error handling and consider using React state instead of global variable.While the debug logging is a good addition, there are two concerns:
- Missing error handling for
Linking.getInitialURL()- Using a global
initialURLvariable could lead to race conditions and make the component's behavior unpredictableConsider this improvement:
- export let initialURL = ""; + const InitialStateHandler: React.FC = () => { + const [initialURL, setInitialURL] = useState<string>(""); const handleInitialDeeplink = async () => { logger.debug("[InitialStateHandler] Handling initial deeplink"); - let openedViaURL = (await Linking.getInitialURL()) || ""; - openedViaURL = getSchemedURLFromUniversalURL(openedViaURL); - initialURL = openedViaURL; + try { + const url = await Linking.getInitialURL(); + if (url) { + const schemedURL = getSchemedURLFromUniversalURL(url); + setInitialURL(schemedURL); + logger.debug("[InitialStateHandler] Initial URL processed:", schemedURL); + } + } catch (error) { + logger.error("[InitialStateHandler] Failed to process initial URL:", error); + } };
Line range hint
1-63: Consider implementing a unified navigation coordinatorTo achieve the PR's objective of consistent navigation from notifications, consider implementing a dedicated navigation coordinator that:
- Maintains a single source of truth for navigation intents
- Coordinates between different navigation triggers (deep links, notifications)
- Ensures navigation happens only after hydration is complete
- Provides proper error handling and logging
Here's a suggested approach:
- Create a
NavigationCoordinatorclass:class NavigationCoordinator { private static instance: NavigationCoordinator; private pendingNavigations: Map<string, NavigationIntent> = new Map(); static getInstance() { if (!this.instance) { this.instance = new NavigationCoordinator(); } return this.instance; } addNavigation(intent: NavigationIntent) { this.pendingNavigations.set(intent.id, intent); this.processPendingNavigations(); } private async processPendingNavigations() { if (!store.getState().hydrationDone) return; for (const [id, intent] of this.pendingNavigations) { try { await this.executeNavigation(intent); } catch (error) { logger.error(`Navigation failed for ${intent.source}:`, error); } finally { this.pendingNavigations.delete(id); } } } }
- Update the notification handler to use the coordinator:
onNotification(notification) { NavigationCoordinator.getInstance().addNavigation({ id: notification.id, url: notification.data.url, source: 'notification', timestamp: Date.now() }); }
- Update InitialStateHandler to use the coordinator:
handleInitialDeeplink() { const url = await Linking.getInitialURL(); if (url) { NavigationCoordinator.getInstance().addNavigation({ id: uuid(), url, source: 'deeplink', timestamp: Date.now() }); } }This approach would:
- Ensure consistent navigation handling
- Prevent race conditions
- Provide better debugging capabilities
- Handle both notification and deep link scenarios uniformly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/StateHandlers/InitialStateHandler.tsx(3 hunks)
🔇 Additional comments (2)
components/StateHandlers/InitialStateHandler.tsx (2)
8-8: LGTM! Good addition of logging capabilities.
The addition of the logger import will help with debugging navigation issues, which is particularly relevant for the notification deep linking problems being addressed.
Also applies to: 10-10
48-55:
Critical: Navigation handling needs improvement to prevent race conditions
The current implementation has several issues that could affect the reliability of notification deep linking:
- No error handling for
Linking.openURL - Immediate clearing of
initialURLcould interfere with notification handling - No coordination between notification and deep link navigation
Consider this safer implementation:
- if (isDevelopmentClientURL(initialURL)) {
- return;
- }
- Linking.openURL(initialURL);
- // Once opened, let's remove so we don't navigate twice
- // when logging out / relogging in
- initialURL = "";
+ if (initialURL && !isDevelopmentClientURL(initialURL)) {
+ try {
+ await Linking.openURL(initialURL);
+ logger.debug('[Navigation] Successfully opened URL:', initialURL);
+ } catch (error) {
+ logger.error('[Navigation] Failed to open URL:', error);
+ }
+ }Additionally, consider implementing a navigation queue to handle both notification and deep link navigation intents:
type NavigationIntent = {
url: string;
source: 'notification' | 'deeplink';
timestamp: number;
};
const navigationQueue = new Set<NavigationIntent>();
// Add to queue when receiving notification or deep link
const addNavigationIntent = (intent: NavigationIntent) => {
navigationQueue.add(intent);
processNavigationQueue();
};
// Process queue when hydration is complete
const processNavigationQueue = async () => {
if (!hydrationDone) return;
for (const intent of navigationQueue) {
try {
await Linking.openURL(intent.url);
logger.debug(`[Navigation] Processed ${intent.source} navigation to: ${intent.url}`);
} catch (error) {
logger.error(`[Navigation] Failed to process ${intent.source} navigation:`, error);
}
navigationQueue.delete(intent);
}
};Let's verify the notification handling implementation:
#!/bin/bash
# Search for notification handling code that might interact with navigation
rg -A 5 "notification.*navigation|navigation.*notification"
# Look for other places where Linking.openURL is used
ast-grep --pattern 'Linking.openURL($_)'…ion-deep-link-to-chat
…ion-deep-link-to-chat
|
Android appears to be broken from the jump - unrelated to these changes. cc @alexrisch |
technoplato
deleted the
ml/conv-1130-inconsistent-notification-deep-link-to-chat
branch
* all * fix-notification-opening-chats * remove exploration * Remove comments/unused code --------- Co-authored-by: Alex Risch <[email protected]>
Fixes #1130
Fixes #1182
https://www.loom.com/share/3db3a0cd75524e08b9b4752e0a5784cd?sid=0bb5c799-330e-4db4-adf2-2c2160cd848d
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes