Solved navigation to PersonalIdMessageFragment crash #3341
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthroughRefactors navigation in PersonalIdBiometricConfigFragment.navigateToMessageDisplay to use a local NavController with guards. It retrieves the NavController, checks for a non-null currentDestination, and only navigates to PersonalIdMessageDisplay if the current destination is different. NavDirections creation remains unchanged. This avoids redundant navigation or re-navigation when already on the target fragment and prevents potential NPEs. Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Frag as PersonalIdBiometricConfigFragment
participant Nav as NavController
participant Dest as CurrentDestination
participant Msg as PersonalIdMessageDisplay
User->>Frag: Trigger navigateToMessageDisplay()
Frag->>Nav: obtain NavController
Frag->>Nav: getCurrentDestination()
Nav-->>Frag: Destination (nullable)
alt currentDestination is null
Frag->>Nav: navigate(navDirections) %% optional behavior depending on implementation
note right of Frag: Guard prevents NPE by checking null before comparing IDs
else currentDestination not null
Frag->>Frag: compare Dest.id vs MessageDisplay.id
alt Dest != MessageDisplay
Frag->>Nav: navigate(navDirections)
Nav->>Msg: Display
note over Nav,Msg: Navigate to PersonalIdMessageDisplay
else Dest == MessageDisplay
Frag->>Frag: do nothing
note right of Frag: Skip re-navigation
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
352-364: Use NavHostFragment.findNavController(this) and restrict navigation to the source destinationFile: app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (lines ~352–364) — Replace the current NavController lookup and condition so we avoid NPE when binding is null and only navigate when the current destination is the source fragment.
- NavController navController = Navigation.findNavController(binding.getRoot()); - - // Only navigate if we’re not already on PersonalIdMessageFragment - if (navController.getCurrentDestination() != null && navController.getCurrentDestination().getId() != R.id.personalid_message_display) { + NavController navController = NavHostFragment.findNavController(this); + // Only navigate if the current destination is the source of this action + if (navController.getCurrentDestination() != null + && navController.getCurrentDestination().getId() == R.id.personalid_biometric_config) { navController.navigate(navDirections); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
14-14: LGTM: appropriate NavController import.
|
@jaypanchal-13 You were able to reproduce this bug? |
|
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
@jaypanchal-13 This change will potentially cause a fragment transition to get ignored i.e. if the code is calling to show a particular message, it will never get shown to the user. As such the change is not right. Think the issue is instead a result of poor navigation or back navigation handling in some part of code and think the RCA here is incomplete. Think it would be critical to reproduce this issue for us to consider current solution here but given the low frequency of this crash, I think we should just deprioritize it if we are not able to easily repro this.
|
@jaypanchal-13 While this feels fine for message, I think introducing these kind of checks is a bad practice for overall health of code as I am pretty sure we will need to introduce similar checks for other fragment transitions as well and not just messages. If we except this solution here we will have to accept doing these checks for other fragment transitions which will decrease our overall code health in terms of managing these. In our experience with PersonalID launch, These errors are always a result of a something amiss in our code with respect to navigation stack management. We have gone back and forth to adding these checks and later realising that the actual solution was correcting the nav stack is some part of the code which was causing these errors. I also see in the crashes linked, that this always seem to happen when there is no biometric hardware present on the device, have you been able to test that workflow ? |
|
@jaypanchal-13 |
|
@jaypanchal-13 Exactly when I say internal to Android OS means its related to biometric and not related to navigation. Also, I think all Biometric related error cases are handled here. I have feeling that navigation is failing due to message fragment not having proper nav implemented. |
|
|
@jaypanchal-13 Have you tried forcing the |
|
|
@jaypanchal-13 Happy to close this out as won't do in that case, don't think we can merge this solution and also the crash is on rare side (19 events in last 30 days) |
Crash Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1619
RCA
Crashlytics reports show a IllegalArgumentException when trying to navigate from PersonalIdBiometricConfigFragment to PersonalIdMessageFragment while the app is already on PersonalIdMessageFragment. This happens because the Navigation component cannot find the action from the current destination (personalid_message_display) to itself
Solution
Adding a check against the current destination ensures we don’t attempt redundant navigation and prevents this kind of crash
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review