Commcare 2.59.2 back-merge #3335
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…droid into commcare_2.59
Instead, sending empty hash/token to PersonalID and it can choose whether we fail.
Reporting heartbeat-triggered integrity reports as "Heartbeat". Reporting failed integrity checks during start of PersonalID configuration as "PersonalID Configuration"
Integrity token workaround for Connect
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna and
shubham1g5
shubham1g5
requested changes
Sep 10, 2025
app/AndroidManifest.xml
Outdated
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:versionCode="106" | ||
| android:versionName="2.60"> | ||
| android:versionName="2.59.2"> |
Contributor
There was a problem hiding this comment.
should be 2.60 in master
Contributor
Author
There was a problem hiding this comment.
Interesting, is this something that's missing from the hotfix scripts? I didn't see any error indicating that this step was attempted and failed. Either way, fixed here: 0835f55
Contributor
There was a problem hiding this comment.
If I'm not mistaken, this happens when you merge master into the release branch before backmerging
Contributor
There was a problem hiding this comment.
Think I do that manually everytime before backmerging for both major and hotfix releases, likely that something in scripts is not working as expected here.
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant F as PersonalIdPhoneFragment
participant H as IntegrityTokenApiRequestHelper
participant A as FirebaseAnalyticsUtil
participant S as Server (Start Configuration)
U->>F: Start configuration
F->>H: fetchIntegrityToken()
alt Token success
H-->>F: StandardIntegrityToken
F->>S: makeStartConfigurationCall(token, requestHash)
S-->>F: Response
else Token failure
H-->>F: Exception
F->>A: reportPersonalIdConfigurationIntegritySubmission(errorCode)
note right of F: Retry without token
F->>S: makeStartConfigurationCall("", requestHash)
S-->>F: Response
end
sequenceDiagram
autonumber
participant W as IntegrityReporterWorker
participant H as IntegrityTokenApiRequestHelper
participant A as FirebaseAnalyticsUtil
participant S as Server (Report Integrity)
participant P as ReportIntegrityResponseParser
W->>H: fetchIntegrityToken()
alt Token success
H-->>W: Token
W->>S: reportIntegrity(token)
alt Network/API success
S-->>W: JSON
W->>P: parse(JSON, requestId)
P->>A: reportPersonalIdHeartbeatIntegritySubmission(requestId, result_code)
P-->>W: true
W-->>W: Result.success()
else Network/API failure
S-->>W: Error
W->>A: reportPersonalIdHeartbeatIntegritySubmission(requestId, "SendError")
W-->>W: Result.retry()/failure
end
else Token failure
H-->>W: Exception
W->>H: getCodeForException(exception)
W->>A: reportPersonalIdHeartbeatIntegritySubmission(requestId, errorCode)
W->>S: reportIntegrity(device_error=errorCode)
S-->>W: (ignored)
W-->>W: Result.failure()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 2
🧹 Nitpick comments (4)
app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java (1)
23-24: Heartbeat wrapper usage — LGTM; add null-safe requestIdCasting anyInputObject to String can yield null; guard to avoid null in analytics.
- FirebaseAnalyticsUtil.reportPersonalIdHeartbeatIntegritySubmission(((String)anyInputObject), - json.optString("result_code", "NoCodeFromServer")); + final String requestId = (anyInputObject instanceof String) ? (String) anyInputObject : "UnknownRequestId"; + FirebaseAnalyticsUtil.reportPersonalIdHeartbeatIntegritySubmission( + requestId, json.optString("result_code", "NoCodeFromServer"));app/src/org/commcare/android/integrity/IntegrityReporterWorker.kt (1)
64-66: Consider retrying on transient Play Integrity errorsFor TOO_MANY_REQUESTS/GOOGLE_SERVER_UNAVAILABLE/NETWORK_ERROR/CLIENT_TRANSIENT_ERROR, a retry is likely preferable to immediate failure.
val errorCode = IntegrityTokenApiRequestHelper.getCodeForException(it) + if (it is com.google.android.play.core.integrity.StandardIntegrityException && + it.errorCode in setOf( + com.google.android.play.core.integrity.model.StandardIntegrityErrorCode.TOO_MANY_REQUESTS, + com.google.android.play.core.integrity.model.StandardIntegrityErrorCode.GOOGLE_SERVER_UNAVAILABLE, + com.google.android.play.core.integrity.model.StandardIntegrityErrorCode.NETWORK_ERROR, + com.google.android.play.core.integrity.model.StandardIntegrityErrorCode.CLIENT_TRANSIENT_ERROR + ) + ) { + return Result.retry() + } FirebaseAnalyticsUtil.reportPersonalIdHeartbeatIntegritySubmission(requestId, errorCode)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
389-394: Minor: avoid mutating method paramsUse a local for resolvedRequestHash to keep params immutable/readable.
- String token = integrityTokenResponse != null ? integrityTokenResponse.token() : ""; - if(requestHash == null) { - requestHash = ""; - } + final String token = integrityTokenResponse != null ? integrityTokenResponse.token() : ""; + final String resolvedRequestHash = (requestHash != null) ? requestHash : ""; ... - }.makeStartConfigurationCall(requireActivity(), body, token, requestHash); + }.makeStartConfigurationCall(requireActivity(), body, token, resolvedRequestHash);app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
412-419: Trigger-aware analytics — good; refine requestId and enforce param length
- Don’t set requestId to a label for configuration events; prefer empty string.
- Use the existing reportEvent(String, String[], String[]) path to auto-trim params to 100 chars.
- public static void reportPersonalIdConfigurationIntegritySubmission(String responseCode) { - String label = "PersonalID Configuration"; - reportPersonalIdIntegritySubmission(label, label, responseCode); - } + public static void reportPersonalIdConfigurationIntegritySubmission(String responseCode) { + reportPersonalIdIntegritySubmission("PersonalID Configuration", "", responseCode); + } @@ - public static void reportPersonalIdIntegritySubmission(String trigger, String requestId, String responseCode) { - Bundle b = new Bundle(); - b.putString(CCAnalyticsParam.REQUEST_ID, requestId); - b.putString(CCAnalyticsParam.TRIGGER, trigger); - b.putString(CCAnalyticsParam.RESULT_CODE, responseCode); - reportEvent(CCAnalyticsEvent.PERSONAL_ID_INTEGRITY_REPORTED, b); - } + public static void reportPersonalIdIntegritySubmission(String trigger, String requestId, String responseCode) { + reportEvent(CCAnalyticsEvent.PERSONAL_ID_INTEGRITY_REPORTED, + new String[]{CCAnalyticsParam.REQUEST_ID, CCAnalyticsParam.TRIGGER, CCAnalyticsParam.RESULT_CODE}, + new String[]{String.valueOf(requestId), String.valueOf(trigger), String.valueOf(responseCode)}); + }Also applies to: 421-427
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/AndroidManifest.xml(1 hunks)app/src/org/commcare/android/integrity/IntegrityReporterWorker.kt(2 hunks)app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt(1 hunks)app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(3 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java
🧬 Code graph analysis (3)
app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-578)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-43)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (1)
getCodeForException(156-162)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-578)
⏰ 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 (4)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
31-31: Add trigger analytics key — LGTMThe new TRIGGER key cleanly supports the expanded event payload.
app/AndroidManifest.xml (1)
5-5: Confirm versionName downgrade on masterBack-merges shouldn’t generally lower master’s versionName. Please confirm this isn’t accidental and won’t impact release pipelines or Play pre-release tracks.
If unintended, revert this hunk:
- android:versionName="2.59.2"> + android:versionName="2.60">app/src/org/commcare/android/integrity/IntegrityReporterWorker.kt (2)
64-66: Good switch to centralized error-code + heartbeat analyticsCentralizing code derivation and logging through the heartbeat wrapper looks correct.
96-96: Send-failure analytics path — LGTMHeartbeat wrapper on network failure is consistent with the new schema.
Comment on lines
+156
to
+162
| fun getCodeForException(exception: Throwable): String { | ||
| return if (exception is StandardIntegrityException) { | ||
| exception.errorCode.toString() | ||
| } else { | ||
| exception.message ?: "UnknownError" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid sending raw exception messages to analytics
Using exception.message risks leaking details; prefer a sanitized, stable code.
- fun getCodeForException(exception: Throwable): String {
- return if (exception is StandardIntegrityException) {
- exception.errorCode.toString()
- } else {
- exception.message ?: "UnknownError"
- }
- }
+ fun getCodeForException(exception: Throwable): String {
+ return if (exception is StandardIntegrityException) {
+ exception.errorCode.toString()
+ } else {
+ exception::class.simpleName ?: "UnknownError"
+ }
+ }📝 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.
Suggested change
| fun getCodeForException(exception: Throwable): String { | |
| return if (exception is StandardIntegrityException) { | |
| exception.errorCode.toString() | |
| } else { | |
| exception.message ?: "UnknownError" | |
| } | |
| } | |
| fun getCodeForException(exception: Throwable): String { | |
| return if (exception is StandardIntegrityException) { | |
| exception.errorCode.toString() | |
| } else { | |
| exception::class.simpleName ?: "UnknownError" | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt
around lines 156 to 162, the method currently returns exception.message for
non-StandardIntegrityException cases which may leak sensitive info; change it to
return a sanitized, stable identifier instead (for example
exception::class.java.simpleName or a fixed token like
"UnknownError"/"UnhandledException") so raw messages are never sent to
analytics, keeping the StandardIntegrityException branch unchanged.
Comment on lines
+270
to
274
| String errorCode = IntegrityTokenApiRequestHelper.Companion.getCodeForException(exception); | ||
| FirebaseAnalyticsUtil.reportPersonalIdConfigurationIntegritySubmission(errorCode); | ||
|
|
||
| makeStartConfigurationCall(null, body, null); | ||
| } |
There was a problem hiding this comment.
Possible NPE path: handleIntegritySubError can receive null tokenResponse
After onTokenFailure you call makeStartConfigurationCall(null, body, null). If the API then returns INTEGRITY_ERROR, the subsequent handleIntegritySubError(integrityTokenResponse, …) dereferences a null tokenResponse in showIntegrityCheckDialog(...). Add a null guard and avoid fall-through.
- case INTEGRITY_ERROR:
- handleIntegritySubError(integrityTokenResponse,
- personalIdSessionDataViewModel.getPersonalIdSessionData().getSessionFailureSubcode());
- default:
- navigateFailure(failureCode, t);
- break;
+ case INTEGRITY_ERROR:
+ if (integrityTokenResponse != null) {
+ handleIntegritySubError(
+ integrityTokenResponse,
+ personalIdSessionDataViewModel.getPersonalIdSessionData().getSessionFailureSubcode());
+ break;
+ } else {
+ onConfigurationFailure(
+ AnalyticsParamValue.START_CONFIGURATION_INTEGRITY_CHECK_FAILURE,
+ getString(R.string.personalid_configuration_process_failed_subtitle));
+ break;
+ }
+ default:
+ navigateFailure(failureCode, t);
+ break;Also applies to: 389-394
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java around
lines 270-274 (and similarly around lines 389-394), handleIntegritySubError may
be called with a null integrityTokenResponse because you call
makeStartConfigurationCall(null, body, null) after onTokenFailure; add a null
guard to avoid dereferencing a null tokenResponse: before calling
makeStartConfigurationCall or before delegating to handleIntegritySubError,
check if the integrity token response is null and either log/report the
condition and return early or call the error path that does not call
showIntegrityCheckDialog; apply the same null-check logic to the other
occurrence at lines 389-394 to prevent the NPE.
avazirna
approved these changes
Sep 10, 2025
shubham1g5
approved these changes
Sep 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Back-merging the change from the 2.59.2 hotfix