Signalising approach in FCM #3347
Conversation
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/services/FCMMessageData.java (1)
115-129: Fix serialization mismatch between writeExternal/readExternal (deserialization will crash)writeExternal writes the action String first, but readExternal expects the first field to be ActionTypes. This will throw IllegalArgumentException at runtime and break PN handling.
Apply this backward‑compatible diff:
@@ @Override public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { - actionType = ActionTypes.valueOf(ExtUtil.readString(in)); - username = ExtUtil.readString(in); - domain = ExtUtil.readString(in); - creationTime = new DateTime(ExtUtil.readLong(in)); + String first = ExtUtil.readString(in); + try { + // New format: first is enum name + actionType = ActionTypes.valueOf(first); + action = ExtUtil.readString(in); + } catch (IllegalArgumentException e) { + // Back-compat: first was 'action' + action = first; + actionType = getActionType(action); + } + username = ExtUtil.readString(in); + domain = ExtUtil.readString(in); + creationTime = new DateTime(ExtUtil.readLong(in)); } @@ @Override public void writeExternal(DataOutputStream out) throws IOException { - ExtUtil.writeString(out, action.toString()); - ExtUtil.writeString(out, username); - ExtUtil.writeString(out, domain); - ExtUtil.writeNumeric(out, creationTime.getMillis()); + // Write enum first, then action string, then the rest + ExtUtil.writeString(out, actionType != null ? actionType.name() : ActionTypes.INVALID.name()); + ExtUtil.writeString(out, action != null ? action : ""); + ExtUtil.writeString(out, username); + ExtUtil.writeString(out, domain); + ExtUtil.writeNumeric(out, creationTime.getMillis()); }app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
199-204: Avoid running potentially heavy init on push path; tweak log textcccCheckPassed(context) currently calls PersonalIdManager.init(context), which can do I/O. This path may run on the main thread via handleNotification; risk of jank/ANR. Also minor grammar nit in log.
Apply this diff:
- if(!cccCheckPassed(context)) { // app doesn't show notification related to CCC if user is not logged in - Logger.log(LogTypes.TYPE_FCM,"CCC push notification sent while user is logout"); + if(!cccCheckPassed(context)) { // app doesn't show notification related to CCC if user is not logged in + Logger.log(LogTypes.TYPE_FCM,"CCC push notification sent while user is logged out"); return null; }
🧹 Nitpick comments (10)
app/res/values/strings.xml (1)
388-388: Improve notification copy for clarity and accessibility“Click here to know more” is vague and not touch‑target friendly. Prefer action‑oriented, self‑contained text.
Apply this diff:
- <string name="fcm_sync_failed_body_text">Click here to know more</string> + <string name="fcm_sync_failed_body_text">We couldn’t sync your data. Tap to learn more.</string>app/src/org/commcare/pn/workers/PNApiResponseStatus.kt (1)
3-3: Remove unused importPersonalIdOrConnectApiErrorCodes isn’t used.
Apply this diff:
-import org.commcare.connect.network.base.BaseApiHandler.PersonalIdOrConnectApiErrorCodesapp/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt (3)
25-26: Decode with UTF‑8 and simplify emptiness checkAvoid platform default charset and prefer isNotEmpty().
Apply this diff:
- val responseAsString = String(StreamsUtil.inputStreamToByteArray(`in`)) - if (!responseAsString.isEmpty()) { + val responseAsString = String(StreamsUtil.inputStreamToByteArray(`in`), Charsets.UTF_8) + if (responseAsString.isNotEmpty()) {
40-42: Guard the unsafe Context cast and record empty responsesClassCastException is possible if anyInputObject isn’t a Context. Also consider reporting analytics when response is empty.
Apply this diff:
- val newJobs = ConnectJobUtils.storeJobs(anyInputObject as Context, jobs, true) - reportApiCall(true, jobs.size, newJobs) + val ctx = (anyInputObject as? Context) + ?: error("ConnectOpportunitiesParser requires a Context via createCallback(parser, context)") + val newJobs = ConnectJobUtils.storeJobs(ctx, jobs, true) + reportApiCall(true, jobs.size, newJobs) } + else { + reportApiCall(false, 0, 0) + }
17-17: Optional: inject Context via constructor instead of anyInputObjectPassing Context explicitly to the parser removes the need for runtime casts and makes the contract clear.
Example:
class ConnectOpportunitiesParser<T>(private val context: Context) : BaseApiResponseParser<T> { /* ... */ }And call createCallback(ConnectOpportunitiesParser(context)).
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
46-51: Use primitive boolean for helper returnMethod never returns null; prefer boolean over Boolean.
Apply this diff:
- private Boolean startSyncForNotification(RemoteMessage remoteMessage){ + private boolean startSyncForNotification(RemoteMessage remoteMessage){app/src/org/commcare/pn/workers/PNApiSyncWorker.kt (1)
25-27: Reuse a single Gson instance.Avoid per‑call allocations.
- private var pnData:Map<String,String>?=null - private var syncAction:SYNC_ACTION?=null + private val gson = Gson() + private var pnData: Map<String, String>? = null + private var syncAction: SYNC_ACTION? = null @@ - Result.success(workDataOf(PN_DATA to Gson().toJson(pnData))) + Result.success(workDataOf(PN_DATA to gson.toJson(pnData))) @@ - pnData = Gson().fromJson(pnJsonString, mapType) + pnData = gson.fromJson(pnJsonString, mapType) @@ - syncAction = Gson().fromJson(syncActionJsonString, enumType) + syncAction = gson.fromJson(syncActionJsonString, enumType)Also applies to: 48-48, 62-62, 69-69
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (3)
44-46: Remove unused constant.SYNC_TYPE_STRING is never referenced.
- val SYNC_TYPE_STRING = "SYNC_TYPE_STRING" -
193-197: Simplify enum I/O: store action.name and parse via valueOf (drop Gson for ACTION).Less overhead and fewer parsing pitfalls. Requires a tiny change in PNApiSyncWorker.
- val syncActionString = Gson().toJson(action) + val syncActionString = action.name val inputData = Data.Builder() .putString(PN_DATA, pnJsonString) .putString(ACTION,syncActionString) .build()Apply in PNApiSyncWorker.kt:
- val syncActionJsonString = inputData.getString(ACTION) - if (syncActionJsonString != null) { - val enumType = object : TypeToken<SYNC_ACTION>() {}.type - syncAction = Gson().fromJson(syncActionJsonString, enumType) - } + val syncActionJsonString = inputData.getString(ACTION) + if (!syncActionJsonString.isNullOrBlank()) { + syncAction = enumValueOf<SYNC_ACTION>(syncActionJsonString) + }
71-81: Minor: startPNApiSync returns immediately; consider not launching from constructor.Side‑effects in constructors complicate lifecycle and testing; call startPNApiSync() explicitly from the caller instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(0 hunks)app/src/org/commcare/connect/ConnectJobHelper.kt(2 hunks)app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt(1 hunks)app/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(0 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(0 hunks)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt(1 hunks)app/src/org/commcare/pn/workers/PNApiResponseStatus.kt(1 hunks)app/src/org/commcare/pn/workers/PNApiSyncWorker.kt(1 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)app/src/org/commcare/services/FCMMessageData.java(2 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(3 hunks)
💤 Files with no reviewable changes (3)
- app/src/org/commcare/activities/CommCareSetupActivity.java
- app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
- app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
🧰 Additional context used
🧠 Learnings (3)
📓 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-07-29T14:11:36.386Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Applied to files:
app/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt
📚 Learning: 2025-07-29T14:10:58.243Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.
Applied to files:
app/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt
🧬 Code graph analysis (3)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (2)
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
context(38-249)app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-621)
app/src/org/commcare/services/FCMMessageData.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-49)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(67-498)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (2)
FCM(48-51)startPNApiSync(71-81)
🔇 Additional comments (5)
app/src/org/commcare/services/FCMMessageData.java (1)
3-4: Good: use REDIRECT_ACTION constant consistentlySwitching to the shared REDIRECT_ACTION key avoids hard‑coded literals and keeps routing aligned with ConnectConstants.
Also applies to: 57-57, 63-63
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
39-44: Sensible fallback when no sync is startedIf PN sync can’t be enqueued, falling back to direct notification handling is correct.
app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt (1)
22-24: Pass applicationContext into createCallback — avoid leaking an Activity ContextgetConnectOpportunities (app/src/org/commcare/connect/network/connect/ConnectApiHandler.kt — lines ~19–24) passes the incoming Context into createCallback; change to pass context.applicationContext (or a known app-level context) and audit other createCallback call sites to ensure no Activity instances are forwarded.
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
220-222: Avoid WorkInfo.stopReason — fall back to state/runAttemptCountFile: app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (lines 220–222)
stopReason may not exist on older WorkManager releases and will cause compile errors; log WorkInfo.state and WorkInfo.runAttemptCount instead.
- Logger.exception("WorkRequest Failed to complete the task-${workInfo.stopReason}", Throwable("WorkRequest Failed with ${workInfo.stopReason}")) + Logger.exception("WorkRequest failed. state=${workInfo.state}, attempts=${workInfo.runAttemptCount}", Throwable("WorkRequest failed"))Verify the WorkManager dependency/version in your build files and search the repo for other stopReason usages before keeping stopReason.
app/src/org/commcare/pn/workers/PNApiSyncWorker.kt (1)
146-149: Harden OPPORTUNITY_ID parsing to avoid crashes on bad input.Integer.parseInt throws; treat invalid/missing IDs as no‑op job.
- val opportunityId = pnData?.get(OPPORTUNITY_ID) - return if(TextUtils.isEmpty(opportunityId)) null else ConnectJobUtils.getCompositeJob(appContext, Integer.parseInt(opportunityId!!)) + val opportunityId = pnData?.get(OPPORTUNITY_ID)?.trim() + val id = opportunityId?.toIntOrNull() ?: return null + return ConnectJobUtils.getCompositeJob(appContext, id)⛔ Skipped due to learnings
Learnt from: OrangeAndGreen PR: dimagi/commcare-android#3248 File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0 Timestamp: 2025-07-29T14:10:55.131Z Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.Learnt from: OrangeAndGreen PR: dimagi/commcare-android#3248 File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0 Timestamp: 2025-07-29T14:11:36.386Z Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
| fun retrieveOpportunities(context: Context, | ||
| listener: ConnectActivityCompleteListener){ | ||
| val user = ConnectUserDatabaseUtil.getUser(context) | ||
| object : ConnectApiHandler<ConnectOpportunitiesResponseModel?>() { | ||
| override fun onFailure( | ||
| errorCode: PersonalIdOrConnectApiErrorCodes, | ||
| t: Throwable? | ||
| ) { | ||
| listener.connectActivityComplete(false) | ||
| } | ||
|
|
||
| override fun onSuccess(data: ConnectOpportunitiesResponseModel?) { | ||
| listener.connectActivityComplete(true) | ||
| } | ||
| }.getConnectOpportunities(context, user!!) | ||
| } |
There was a problem hiding this comment.
Handle null user instead of non‑null assertion
user!! can crash. Fail gracefully and notify listener.
Apply this diff:
- fun retrieveOpportunities(context: Context,
- listener: ConnectActivityCompleteListener){
- val user = ConnectUserDatabaseUtil.getUser(context)
+ fun retrieveOpportunities(
+ context: Context,
+ listener: ConnectActivityCompleteListener
+ ){
+ val user = ConnectUserDatabaseUtil.getUser(context)
+ if (user == null) {
+ listener.connectActivityComplete(false)
+ return
+ }
object : ConnectApiHandler<ConnectOpportunitiesResponseModel?>() {
override fun onFailure(
errorCode: PersonalIdOrConnectApiErrorCodes,
t: Throwable?
) {
listener.connectActivityComplete(false)
}
override fun onSuccess(data: ConnectOpportunitiesResponseModel?) {
listener.connectActivityComplete(true)
}
- }.getConnectOpportunities(context, user!!)
+ }.getConnectOpportunities(context.applicationContext, user)
}Also prefer passing applicationContext to avoid leaking an Activity.
📝 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.
| fun retrieveOpportunities(context: Context, | |
| listener: ConnectActivityCompleteListener){ | |
| val user = ConnectUserDatabaseUtil.getUser(context) | |
| object : ConnectApiHandler<ConnectOpportunitiesResponseModel?>() { | |
| override fun onFailure( | |
| errorCode: PersonalIdOrConnectApiErrorCodes, | |
| t: Throwable? | |
| ) { | |
| listener.connectActivityComplete(false) | |
| } | |
| override fun onSuccess(data: ConnectOpportunitiesResponseModel?) { | |
| listener.connectActivityComplete(true) | |
| } | |
| }.getConnectOpportunities(context, user!!) | |
| } | |
| fun retrieveOpportunities( | |
| context: Context, | |
| listener: ConnectActivityCompleteListener | |
| ){ | |
| val user = ConnectUserDatabaseUtil.getUser(context) | |
| if (user == null) { | |
| listener.connectActivityComplete(false) | |
| return | |
| } | |
| object : ConnectApiHandler<ConnectOpportunitiesResponseModel?>() { | |
| override fun onFailure( | |
| errorCode: PersonalIdOrConnectApiErrorCodes, | |
| t: Throwable? | |
| ) { | |
| listener.connectActivityComplete(false) | |
| } | |
| override fun onSuccess(data: ConnectOpportunitiesResponseModel?) { | |
| listener.connectActivityComplete(true) | |
| } | |
| }.getConnectOpportunities(context.applicationContext, user) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/ConnectJobHelper.kt around lines 138 to 153,
replace the user!! non‑null assertion with a null check: retrieve the user, if
null call listener.connectActivityComplete(false) (and optionally log) and
return early; otherwise call getConnectOpportunities using
context.applicationContext (not the incoming Context) to avoid leaking an
Activity. Ensure on null you do not attempt the API call and that the listener
is always notified.
| var syncFailedCount = 0 | ||
| var syncPassedCount = 0 | ||
|
|
There was a problem hiding this comment.
Make counters thread‑safe.
Multiple coroutines update counts; use AtomicInteger.
+import java.util.concurrent.atomic.AtomicInteger
@@
- var syncFailedCount = 0
- var syncPassedCount = 0
+ private val syncFailedCount = AtomicInteger(0)
+ private val syncPassedCount = AtomicInteger(0)
@@
- syncPassedCount++
- if(syncPassedCount==requiredWorkerThread.size && syncType== SYNC_TYPE.FCM) {
+ val passed = syncPassedCount.incrementAndGet()
+ if (passed == requiredWorkerThread.size && syncType == SYNC_TYPE.FCM) {
@@
- Logger.exception("WorkRequest Failed to complete the task-${workInfo.stopReason}", Throwable("WorkRequest Failed with ${workInfo.stopReason}"))
- syncFailedCount++
- if(syncType == SYNC_TYPE.FCM && syncFailedCount ==1){ // raise the notification on first failure and not multiple times
+ Logger.exception("WorkRequest failed. state=${workInfo.state}, attempts=${workInfo.runAttemptCount}", Throwable("WorkRequest failed"))
+ val failed = syncFailedCount.incrementAndGet()
+ if (syncType == SYNC_TYPE.FCM && failed == 1) { // raise onceAlso applies to: 211-214, 220-224, 33-34
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt around lines
58-60 (and also apply the same change at 33-34, 211-214, 220-224), the integer
counters are non-thread-safe and are mutated by multiple coroutines; replace the
mutable Int vars with java.util.concurrent.atomic.AtomicInteger fields,
initialize them appropriately, and update all increments/decrements/usages to
call AtomicInteger methods (incrementAndGet/getAndIncrement/get/set/getAndAdd as
needed) so concurrent updates are safe; add the AtomicInteger import at the top
of the file and ensure any equality or read operations use .get() when comparing
or logging the current value.
| private suspend fun startListeningToWorkerRequest(workRequest: WorkRequest){ | ||
|
|
||
| WorkManager.getInstance(context).enqueue(workRequest) | ||
| WorkManager.getInstance(context).getWorkInfoByIdFlow(workRequest.id) | ||
| .collect { workInfo -> | ||
| if (workInfo != null) { | ||
| when (workInfo.state) { | ||
| WorkInfo.State.SUCCEEDED -> { | ||
| processAfterSuccessfulSync(workInfo) | ||
| } | ||
|
|
||
| WorkInfo.State.FAILED -> { | ||
| processAfterSyncFailed(workInfo) | ||
| } | ||
|
|
||
| WorkInfo.State.BLOCKED, WorkInfo.State.CANCELLED -> { | ||
| processAfterSyncFailed(workInfo) | ||
| } | ||
|
|
||
| else -> { | ||
| // Work is still running or in other states | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop collecting flow after terminal state to avoid leaked coroutines.
Collecting forever keeps background coroutines alive.
+import kotlinx.coroutines.flow.first
@@
- private suspend fun startListeningToWorkerRequest(workRequest: WorkRequest){
-
- WorkManager.getInstance(context).enqueue(workRequest)
- WorkManager.getInstance(context).getWorkInfoByIdFlow(workRequest.id)
- .collect { workInfo ->
- if (workInfo != null) {
- when (workInfo.state) {
- WorkInfo.State.SUCCEEDED -> {
- processAfterSuccessfulSync(workInfo)
- }
- WorkInfo.State.FAILED -> {
- processAfterSyncFailed(workInfo)
- }
- WorkInfo.State.BLOCKED, WorkInfo.State.CANCELLED -> {
- processAfterSyncFailed(workInfo)
- }
- else -> {
- // Work is still running or in other states
- }
- }
- }
- }
- }
+ private suspend fun startListeningToWorkerRequest(workRequest: WorkRequest) {
+ WorkManager.getInstance(context).enqueue(workRequest)
+ val workInfo = WorkManager.getInstance(context)
+ .getWorkInfoByIdFlow(workRequest.id)
+ .first { it != null && it.state.isFinished }
+ when (workInfo.state) {
+ WorkInfo.State.SUCCEEDED -> processAfterSuccessfulSync(workInfo)
+ WorkInfo.State.FAILED, WorkInfo.State.BLOCKED, WorkInfo.State.CANCELLED -> processAfterSyncFailed(workInfo)
+ else -> { /* no-op */ }
+ }
+ }Also applies to: 13-15
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt around lines
126 to 151, the flow from getWorkInfoByIdFlow is collected indefinitely which
leaks coroutines; change the collection to stop after a terminal state is
observed (SUCCEEDED, FAILED, BLOCKED, CANCELLED) by using a terminal-aware
operator (for example use first/firstOrNull with a predicate that checks for
terminal states, or apply takeWhile to continue only while the state is
non-terminal) and then handle the terminal state once and return so the
coroutine can complete.
| val pnJsonString = workInfo.outputData.getString(PN_DATA) | ||
| if (pnJsonString != null) { | ||
| val mapType = object : TypeToken<HashMap<String, Any>>() {}.type | ||
| return Gson().fromJson(pnJsonString, mapType) | ||
| } |
There was a problem hiding this comment.
Fix PN output parsing type (compile error).
Actual payload is Map<String, String>.
- if (pnJsonString != null) {
- val mapType = object : TypeToken<HashMap<String, Any>>() {}.type
- return Gson().fromJson(pnJsonString, mapType)
- }
+ if (pnJsonString != null) {
+ val mapType = object : TypeToken<HashMap<String, String>>() {}.type
+ return Gson().fromJson<HashMap<String, String>>(pnJsonString, mapType)
+ }📝 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.
| val pnJsonString = workInfo.outputData.getString(PN_DATA) | |
| if (pnJsonString != null) { | |
| val mapType = object : TypeToken<HashMap<String, Any>>() {}.type | |
| return Gson().fromJson(pnJsonString, mapType) | |
| } | |
| val pnJsonString = workInfo.outputData.getString(PN_DATA) | |
| if (pnJsonString != null) { | |
| val mapType = object : TypeToken<HashMap<String, String>>() {}.type | |
| return Gson().fromJson<HashMap<String, String>>(pnJsonString, mapType) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt around lines
240 to 244, the TypeToken used for parsing PN_DATA is HashMap<String, Any>
causing a compile type mismatch; change it to Map<String, String> (or
HashMap<String, String>) so Gson parses the actual payload type. Replace "object
: TypeToken<HashMap<String, Any>>() {}.type" with "object :
TypeToken<Map<String, String>>() {}.type" (or HashMap variant) and ensure the
method's return type matches Map<String, String>.
| import kotlin.coroutines.resume | ||
| import kotlin.coroutines.suspendCoroutine | ||
|
|
||
| class PNApiSyncWorker (val appContext: Context, val workerParams: WorkerParameters) : CoroutineWorker(appContext, workerParams) { |
There was a problem hiding this comment.
Fix constructor: name collision with inherited workerParams (won’t compile).
ListenableWorker already exposes workerParams; redeclaring it as a property causes a JVM signature clash.
-class PNApiSyncWorker (val appContext: Context, val workerParams: WorkerParameters) : CoroutineWorker(appContext, workerParams) {
+class PNApiSyncWorker(appContext: Context, params: WorkerParameters) : CoroutineWorker(appContext, params) {
+ private val appContext = applicationContext📝 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.
| class PNApiSyncWorker (val appContext: Context, val workerParams: WorkerParameters) : CoroutineWorker(appContext, workerParams) { | |
| class PNApiSyncWorker(appContext: Context, params: WorkerParameters) : CoroutineWorker(appContext, params) { | |
| private val appContext = applicationContext |
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workers/PNApiSyncWorker.kt around line 23, the
primary constructor declares "val workerParams: WorkerParameters" which
conflicts with the inherited WorkerParameters property and causes a JVM
signature clash; remove the "val" modifier from the workerParams parameter
(i.e., accept it as a constructor parameter only) so it is passed to the
CoroutineWorker supertype without creating a new property, and keep the super
call intact; optionally, if you need to store workerParams as a field, rename it
to avoid colliding with the inherited name.
There was a problem hiding this comment.
@Jignesh-dimagi this Code Rabbit comment seems worth looking into.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| val pnApiStatus = startAppropriateSync() | ||
| if (pnApiStatus.success) { | ||
| Result.success(workDataOf(PN_DATA to Gson().toJson(pnData))) | ||
| } else if (pnApiStatus.retry && workerParams.runAttemptCount < MAX_RETRIES) { |
There was a problem hiding this comment.
Use Worker.runAttemptCount, not WorkerParameters.
After fixing the constructor, this reference also breaks; prefer the built‑in property.
- } else if (pnApiStatus.retry && workerParams.runAttemptCount < MAX_RETRIES) {
+ } else if (pnApiStatus.retry && runAttemptCount < MAX_RETRIES) {📝 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.
| } else if (pnApiStatus.retry && workerParams.runAttemptCount < MAX_RETRIES) { | |
| } else if (pnApiStatus.retry && runAttemptCount < MAX_RETRIES) { |
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workers/PNApiSyncWorker.kt around line 49, the code
currently checks workerParams.runAttemptCount but should use the Worker's
built-in runAttemptCount property; replace the reference to
workerParams.runAttemptCount with runAttemptCount (e.g., if (pnApiStatus.retry
&& runAttemptCount < MAX_RETRIES)) so the worker uses its own attempt counter
and the reference remains valid after the constructor change.
| val mapType = object : TypeToken<Map<String, Any>>() {}.type | ||
| pnData = Gson().fromJson(pnJsonString, mapType) | ||
| } |
There was a problem hiding this comment.
Correct PN_DATA deserialization type (compile error).
You parse Map<String, Any> but assign to Map<String, String>. Make both sides String.
- val mapType = object : TypeToken<Map<String, Any>>() {}.type
- pnData = Gson().fromJson(pnJsonString, mapType)
+ val mapType = object : TypeToken<Map<String, String>>() {}.type
+ pnData = Gson().fromJson<Map<String, String>>(pnJsonString, mapType)📝 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.
| val mapType = object : TypeToken<Map<String, Any>>() {}.type | |
| pnData = Gson().fromJson(pnJsonString, mapType) | |
| } | |
| val mapType = object : TypeToken<Map<String, String>>() {}.type | |
| pnData = Gson().fromJson<Map<String, String>>(pnJsonString, mapType) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workers/PNApiSyncWorker.kt around lines 61 to 63, the
Gson deserialization currently uses TypeToken<Map<String, Any>> but assigns the
result to a Map<String, String>, causing a compile type mismatch; change the
TypeToken to Map<String, String> (i.e., object : TypeToken<Map<String,
String>>() {}.type) so the deserialized type matches the variable, and ensure
pnData is declared as Map<String, String> (or both sides use String) to resolve
the compile error.
| public static boolean cccCheckPassed(Context context){ | ||
| PersonalIdManager personalIdManager = PersonalIdManager.getInstance(); | ||
| personalIdManager.init(context); | ||
| return personalIdManager.isloggedIn(); | ||
| } |
There was a problem hiding this comment.
Make cccCheckPassed lightweight (no network/DB init on UI thread)
Use the stored user record to decide login status; avoid PersonalIdManager.init here.
Apply this diff:
- public static boolean cccCheckPassed(Context context){
- PersonalIdManager personalIdManager = PersonalIdManager.getInstance();
- personalIdManager.init(context);
- return personalIdManager.isloggedIn();
- }
+ public static boolean cccCheckPassed(Context context){
+ // Lightweight gate: user exists and is fully registered
+ var user = ConnectUserDatabaseUtil.getUser(context);
+ return user != null && user.getRegistrationPhase() == ConnectConstants.PERSONALID_NO_ACTIVITY;
+ }Consider removing the now‑unused import of PersonalIdManager at Line 31.
📝 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.
| public static boolean cccCheckPassed(Context context){ | |
| PersonalIdManager personalIdManager = PersonalIdManager.getInstance(); | |
| personalIdManager.init(context); | |
| return personalIdManager.isloggedIn(); | |
| } | |
| public static boolean cccCheckPassed(Context context){ | |
| // Lightweight gate: user exists and is fully registered | |
| var user = ConnectUserDatabaseUtil.getUser(context); | |
| return user != null && user.getRegistrationPhase() == ConnectConstants.PERSONALID_NO_ACTIVITY; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 492-496,
replace the current cccCheckPassed implementation that calls
PersonalIdManager.init (which triggers DB/network work on the UI thread) with a
lightweight check that reads the persisted/stored user record/session state and
returns whether a user is logged in (e.g., retrieve the stored user/session from
the app's persistent storage or existing session accessor and check for
non-null/active login flag) without initializing PersonalIdManager; after
changing the implementation, remove the now-unused PersonalIdManager import at
line 31.
|
@shubham1g5 Bumping this for review, thanks |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
I don't think I am understanding the approach right here, left some high-level comments to get more info here.
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt
Show resolved
Hide resolved
| import kotlin.coroutines.resume | ||
| import kotlin.coroutines.suspendCoroutine | ||
|
|
||
| class PNApiSyncWorker (val appContext: Context, val workerParams: WorkerParameters) : CoroutineWorker(appContext, workerParams) { |
There was a problem hiding this comment.
@Jignesh-dimagi this Code Rabbit comment seems worth looking into.
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt
Outdated
Show resolved
Hide resolved
| return createSyncWorkerRequest() | ||
| } | ||
|
|
||
| fun createSyncWorkerRequest() : Boolean { |
There was a problem hiding this comment.
think if this work manager is successful, we should also schedule a work manager to sync retrieve_notifications endpoint as it's likely that user has internet at the moment and will reduce cases when a notification is present in the tray but not in the notification history.
There was a problem hiding this comment.
I was thinking same but due to following I didn't implemented:
Along with retrieve_notifications its required to call the update_notification_received also. So in case if retrieve_notifications succeed and if update_notification_received fails, there may be case that we might have duplicate PNs in our local DB.
There was a problem hiding this comment.
So in case if retrieve_notifications succeed and if update_notification_received fails
we can wrap this under a single db transaction so that if it fails we rollback the entire db change.
There was a problem hiding this comment.
Offlined with Jignesh and we decided on doing it in another PR. @Jignesh-dimagi will you be able to respond with a ticket once you have it. thanks!
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
@Jignesh-dimagi the string translations are pending but nothing blocking.
Added now |
|
@damagatchi retest this please |
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1717
Feature Flag
https://dimagi.atlassian.net/browse/CCCT-1567
Labels and Review