Added safe navigation #3045
Added safe navigation #3045
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new utility class, Sequence Diagram(s)sequenceDiagram
participant UI_Component as UI Component (Activity/Fragment)
participant NavController
participant CommCareNavController
UI_Component->>CommCareNavController: navigateSafely(navController, navDirections)
CommCareNavController->>NavController: Validate navDirections and destination
alt Valid navigation
CommCareNavController->>NavController: navigate(navDirections)
else Invalid navigation
CommCareNavController-->>UI_Component: No navigation performed
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 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: 0
🧹 Nitpick comments (1)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
139-147: Consider updating this navigation call too.The navigation call in
beginSecondaryPhoneVerificationstill uses the direct approach rather than the new safe navigation utility. Consider updating this for consistency.- controller.navigate(navDirections); + CommCareNavController.navigateSafely(controller, navDirections);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/org/commcare/activities/connect/ConnectIdActivity.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(4 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(2 hunks)app/src/org/commcare/utils/CommCareNavigationController.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
app/src/org/commcare/utils/CommCareNavigationController.kt (1)
navigateSafely(9-21)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (1)
app/src/org/commcare/utils/CommCareNavigationController.kt (1)
navigateSafely(9-21)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
app/src/org/commcare/utils/CommCareNavigationController.kt (1)
navigateSafely(9-21)
🔇 Additional comments (9)
app/src/org/commcare/utils/CommCareNavigationController.kt (1)
1-25: Well-implemented safe navigation utilityThis is a well-designed utility for preventing navigation crashes. The extension function performs comprehensive checks before navigation:
- Validates both the NavDirections and current destination
- Verifies the navigation action exists
- Ensures the destination ID is valid and exists in the navigation graph
The
@file:JvmNameannotation properly exposes this Kotlin code to Java, and the null-safety checks are thorough.app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (2)
41-41: Import added correctly for safe navigation utility.The CommCareNavController import has been properly added to support the safe navigation implementation.
615-615: Navigation call updated to use safe navigation.The direct navigation call has been properly replaced with the safer
navigateSafelyextension function, which will prevent potential crashes when navigation is attempted with invalid directions or destinations.app/src/org/commcare/activities/connect/ConnectIdActivity.java (3)
22-22: Import added correctly for safe navigation utility.The CommCareNavController import has been properly added to support the safe navigation implementation.
32-32: Improved encapsulation by making controller private.Good change - reducing the scope of the NavController from
public statictoprivateimproves encapsulation and prevents external manipulation.
135-135: Navigation call updated to use safe navigation.The direct navigation call has been properly replaced with the safer
navigateSafelyextension function, which will prevent potential crashes when navigation is attempted with invalid directions or destinations.app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (2)
36-36: Import added correctly for safe navigation utility.The CommCareNavController import has been properly added to support the safe navigation implementation.
248-248: Navigation calls consistently updated to use safe navigation.All direct navigation calls in this file have been properly replaced with the safer
navigateSafelyextension function, which provides consistent protection against navigation crashes throughout the sign-up flow.Also applies to: 253-253, 306-306, 311-311, 388-388
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
32-32: Safe navigation implementation improves stability.The change adds proper import for the utility class and replaces direct navigation with the safer
navigateSafelyutility method. This improves the robustness of the navigation by performing comprehensive validation beyond simple null checks, including verifying valid destinations and actions before attempting navigation.Also applies to: 285-287
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
If we're employing this new safety code in three of the fragments, I wonder if it's worth adding it to the other fragments now as well? I worry that it might take us a while to come back and complete this if we don't do it now... @shubham1g5 Your thoughts?
| if (navAction != null) { | ||
| val destinationId: Int = navAction.destinationId.orEmpty() | ||
| val currentNode: NavGraph? = if (currentDestination is NavGraph) currentDestination else currentDestination.parent | ||
| if (destinationId != 0 && currentNode != null && currentNode.findNode(destinationId) != null) { |
There was a problem hiding this comment.
Looking at all the conditions that need to be met on the way to finally calling navigate(), I'm wondering in which cases we might want to log a non-fatal exception to Firebase vs. silently do nothing. Is the else-case in each of the three ifs an error situation that we should track?
There was a problem hiding this comment.
@OrangeAndGreen We already have crashes in app but still not able to identify the cause as its not giving any useful information to trace the issue. Also, I have feeling that lets not add at other places before seeing the reactions from user so kept at sign up / recovery mode only.
Product Description
This is fix for bug
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-961
Feature Flag
Bug fix
Safety Assurance
Safety story
Test on staging and prod app for said changes
QA Plan
This PR needs testing the connect ID sign up and recover flow
Labels and Review