fix: notification tap not opening app or conversation#631
fix: notification tap not opening app or conversation#631torlando-tech merged 5 commits intomainfrom
Conversation
Three issues prevented notification taps from reliably opening the relevant conversation: 1. PendingIntent used FLAG_CANCEL_CURRENT which invalidates the old PendingIntent before creating a new one. If the notification system still held a reference to the old (dead) PendingIntent, tapping did nothing. Changed to FLAG_UPDATE_CURRENT which updates extras in-place, keeping all references valid. 2. No error handling around navController.navigate() in the LaunchedEffect that processes pending navigation. If navigation threw (e.g. graph not ready on cold start), it failed silently. Added try-catch with logging. 3. No fallback for process death + task recreation edge cases where onNewIntent may not be called. Added onResume fallback that reprocesses the current intent if it still has a notification action and pendingNavigation hasn't been set. The action is cleared after processing to prevent re-triggering on subsequent resumes. https://claude.ai/code/session_01SVr6bXLKx3H7pqPv2XdQmT
Greptile SummaryThis PR correctly addresses the primary root cause of notification failures by replacing The PR also wraps the navigation dispatch in a However, two edge-case issues remain unresolved:
Confidence Score: 3/5
Last reviewed commit: 7f0dc7a |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
The new onResume fallback and processIntent action-clearing code referenced NotificationHelper.ACTION_OPEN_CONVERSATION and ACTION_OPEN_ANNOUNCE without importing the class. Also fixes the try/when indentation so the when block is properly nested inside try. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If navController.navigate() throws (e.g. graph not ready on cold start), leaving pendingNavigation set allows the onResume fallback or a recomposition to retry the navigation instead of silently dropping the user's notification tap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The onResume fallback (process-death recovery) was dead code: processIntent() in onCreate() always runs before onResume(), so by the time onResume checked intent.action it was either already handled (pendingNavigation set) or cleared (action nulled). The action-clearing in processIntent() only existed to support this fallback, so it is removed too. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // Only clear on success so a failed navigation can be retried | ||
| pendingNavigation.value = null | ||
| } catch (e: Exception) { | ||
| Log.e("ColumbaNavigation", "Failed to navigate from pending intent: $navigation", e) | ||
| } |
There was a problem hiding this comment.
Bug: If navigation fails, pendingNavigation.value is not cleared. Since the LaunchedEffect is keyed on this value, it won't re-run, and the navigation action is permanently lost.
Severity: MEDIUM
Suggested Fix
To ensure failed navigations can be retried or at least gracefully handled, clear the state in the catch block. Set pendingNavigation.value = null within the catch block, similar to how it's done in the try block on success. This will allow a new navigation intent to be processed correctly.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/main/java/com/lxmf/messenger/MainActivity.kt#L774-L778
Potential issue: The `LaunchedEffect` responsible for handling pending navigations is
keyed on `pendingNavigation.value`. If the navigation attempt within the `try` block
fails and throws an exception, the `catch` block is executed. However, this `catch`
block does not clear `pendingNavigation.value`. Because the key for the `LaunchedEffect`
does not change, the effect will not be re-launched to retry the navigation. The
intended retry mechanism via an `onResume` fallback was removed in a previous commit, so
there is no longer any process to re-attempt the failed navigation, causing the user's
navigation intent (e.g., from a notification tap) to be permanently lost.
| @@ -632,146 +632,150 @@ fun ColumbaNavigation( | |||
| // Handle pending navigation from intents | |||
| LaunchedEffect(pendingNavigation.value) { | |||
There was a problem hiding this comment.
Retry mechanism ineffective when re-tapping the same notification
The LaunchedEffect(pendingNavigation.value) is keyed on the state value itself. When navController.navigate() throws and the catch block logs the error, pendingNavigation.value remains set to the same navigation object (e.g., Conversation("abc123", "Bob")).
When the user re-taps the same notification, intentHandler.handle() sets pendingNavigation.value to an equal object with identical properties. Since PendingNavigation.Conversation is a data class, Compose compares it by structural equality—the two instances are equal, so no state change is emitted and the LaunchedEffect never re-fires.
Result: Re-tapping the same notification after a cold-start navigation failure has no effect.
To support retries, consider clearing pendingNavigation.value = null before re-setting it to a new value, or use a change counter/timestamp to ensure the LaunchedEffect key always changes on retry attempts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/MainActivity.kt
Line: 633
Comment:
**Retry mechanism ineffective when re-tapping the same notification**
The `LaunchedEffect(pendingNavigation.value)` is keyed on the state value itself. When `navController.navigate()` throws and the catch block logs the error, `pendingNavigation.value` remains set to the same navigation object (e.g., `Conversation("abc123", "Bob")`).
When the user re-taps the same notification, `intentHandler.handle()` sets `pendingNavigation.value` to an equal object with identical properties. Since `PendingNavigation.Conversation` is a data class, Compose compares it by structural equality—the two instances are equal, so no state change is emitted and the `LaunchedEffect` never re-fires.
Result: Re-tapping the same notification after a cold-start navigation failure has no effect.
To support retries, consider clearing `pendingNavigation.value = null` before re-setting it to a new value, or use a change counter/timestamp to ensure the `LaunchedEffect` key always changes on retry attempts.
How can I resolve this? If you propose a fix, please make it concise.| is PendingNavigation.AnswerCall -> { | ||
| // Set flag to prevent callState observer from overriding navigation | ||
| isAnsweringCall = true | ||
| // Navigate to voice call screen with auto-answer flag | ||
| val encodedHash = Uri.encode(navigation.identityHash) | ||
| val route = "voice_call/$encodedHash?autoAnswer=true" | ||
| Log.w("ColumbaNavigation", "📞 AnswerCall handler - navigating to $route") | ||
| Log.w("ColumbaNavigation", "📞 Current backstack: ${navController.currentBackStackEntry?.destination?.route}") | ||
| navController.navigate(route) { | ||
| launchSingleTop = true | ||
| } | ||
| Log.d("ColumbaNavigation", "Navigated to interface stats: ${navigation.interfaceId}") | ||
| Log.w("ColumbaNavigation", "📞 After navigation, current: ${navController.currentBackStackEntry?.destination?.route}") |
There was a problem hiding this comment.
isAnsweringCall flag leaks on navigation failure
isAnsweringCall = true (line 707) is set before the navController.navigate() call (line 713). If navigate() throws—the exact cold-start scenario this PR targets—the catch block logs the error, but isAnsweringCall remains true and is never reset.
The only place this flag is reset is in the callState observer (line 899-900) when the call state changes. If navigation fails and the user remains on the previous screen without a fresh incoming call, the flag stays permanently set. On the next incoming call, the observer check at line 888 (!isAnsweringCall) will incorrectly suppress navigation that should occur.
Consider moving isAnsweringCall = true to after successful navigation, or resetting it in the catch block to ensure the flag doesn't leak.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/MainActivity.kt
Line: 705-716
Comment:
**`isAnsweringCall` flag leaks on navigation failure**
`isAnsweringCall = true` (line 707) is set *before* the `navController.navigate()` call (line 713). If `navigate()` throws—the exact cold-start scenario this PR targets—the catch block logs the error, but `isAnsweringCall` remains `true` and is never reset.
The only place this flag is reset is in the `callState` observer (line 899-900) when the call state changes. If navigation fails and the user remains on the previous screen without a fresh incoming call, the flag stays permanently set. On the next incoming call, the observer check at line 888 (`!isAnsweringCall`) will incorrectly suppress navigation that should occur.
Consider moving `isAnsweringCall = true` to after successful navigation, or resetting it in the catch block to ensure the flag doesn't leak.
How can I resolve this? If you propose a fix, please make it concise.
Three issues prevented notification taps from reliably opening the
relevant conversation:
PendingIntent used FLAG_CANCEL_CURRENT which invalidates the old
PendingIntent before creating a new one. If the notification system
still held a reference to the old (dead) PendingIntent, tapping did
nothing. Changed to FLAG_UPDATE_CURRENT which updates extras in-place,
keeping all references valid.
No error handling around navController.navigate() in the
LaunchedEffect that processes pending navigation. If navigation threw
(e.g. graph not ready on cold start), it failed silently. Added
try-catch with logging.
No fallback for process death + task recreation edge cases where
onNewIntent may not be called. Added onResume fallback that
reprocesses the current intent if it still has a notification action
and pendingNavigation hasn't been set. The action is cleared after
processing to prevent re-triggering on subsequent resumes.
https://claude.ai/code/session_01SVr6bXLKx3H7pqPv2XdQmT