Update Super Linter #3388
Update Super Linter #3388
Conversation
shubham1g5
force-pushed
the
testLinter
branch
from
3d0b393 to
08da34f
Compare
📝 WalkthroughWalkthroughThis PR replaces the PNApiSyncWorkerManager/PNApiSyncWorker architecture with a new NotificationsSyncWorkerManager/NotificationsSyncWorker implementation for push-notification synchronization. It adds the new manager and worker classes, updates CommCareApplication to schedule periodic notification syncs at startup, integrates scheduling into PersonalIdManager for sign-in/forget flows, updates CommCareFirebaseMessagingService and PushNotificationViewModel to use the new manager, deletes PNApiSyncWorkerManager and PNApiSyncWorker, and updates CI linting workflow (.github/workflows/linter.yml) to newer actions and super-linter version with targeted PR linting. Sequence Diagram(s)sequenceDiagram
actor User
participant App as CommCareApplication
participant PM as PersonalIdManager
participant NSWM as NotificationsSyncWorkerManager
participant WM as WorkManager
participant NSW as NotificationsSyncWorker
User->>App: App starts (onCreate)
App->>NSWM: schedulePeriodicPushNotificationRetrieval
NSWM->>WM: enqueueUniquePeriodicWork
User->>PM: Sign in
PM->>NSWM: schedulePeriodicPushNotificationRetrievalChecked
NSWM->>PM: init + check logged in
NSWM->>WM: Schedule periodic work
rect rgb(200, 220, 255)
Note over WM,NSW: Periodic execution (interval-based)
WM->>NSW: Execute notification sync
NSW->>NSW: Parse input & route by ACTION
NSW->>NSW: Execute specific sync (Opportunity/PersonalId/Delivery/Learning)
NSW->>NSW: Return Result (success/retry/failure)
end
User->>PM: Logout/Forget
PM->>NSWM: cancelPeriodicPushNotificationRetrieval
NSWM->>WM: Cancel periodic work
sequenceDiagram
participant FCM as CommCareFirebaseMessagingService
participant NSWM as NotificationsSyncWorkerManager
participant WM as WorkManager
participant NSW as NotificationsSyncWorker
FCM->>FCM: onMessageReceived(RemoteMessage)
FCM->>NSWM: new NotificationsSyncWorkerManager(context, pns, true, true)
NSWM->>NSWM: Initialize with payload
FCM->>NSWM: startPNApiSync()
rect rgb(255, 220, 200)
Note over NSWM,NSW: Immediate execution (one-time work)
NSWM->>WM: enqueueUniqueWork(OneTimeWorkRequest)
WM->>NSW: Execute worker
NSW->>NSW: Parse and validate input
NSW->>NSW: Execute sync based on ACTION
NSW->>NSW: Return Result with payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (3)
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
70-74: Use a string-typed TypeToken when rehydrating the payload.We expect this payload to stay
Map<String, String>so downstream consumers (e.g.FirebaseMessagingUtil.handleNotification) never see non-string values. Feeding Gson aHashMap<String, Any>type allows it to materialize numbers/booleans as non-string types, which can trip later casts. Please deserialize with aHashMap<String, String>TypeToken instead.- if (notificationPayloadJson != null) { - val mapType = object : TypeToken<HashMap<String, Any>>() {}.type - notificationPayload = Gson().fromJson<HashMap<String, String>>(notificationPayloadJson, mapType) - } + if (notificationPayloadJson != null) { + val mapType = object : TypeToken<HashMap<String, String>>() {}.type + notificationPayload = Gson().fromJson(notificationPayloadJson, mapType) + }app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (2)
200-220: Consider safer Map access pattern.Lines 202 and 213 use
.get()which returns a nullableString?. While the containing checks at lines 201 and 212 ensure the key exists, using the safe access operator or non-null assertion would make the null-safety more explicit and prevent potential issues if the code is refactored.Apply this diff:
private fun startLearningSyncWorker(notificationPayload: Map<String, String>) { if (notificationPayload.containsKey(OPPORTUNITY_ID) && cccCheckPassed(context)) { - val opportunityId = notificationPayload.get(OPPORTUNITY_ID) + val opportunityId = notificationPayload[OPPORTUNITY_ID]!! startWorkRequest( notificationPayload, SyncAction.SYNC_LEARNING_PROGRESS, SyncAction.SYNC_LEARNING_PROGRESS.toString() + "-$opportunityId" ) } } private fun startDeliverySyncWorker(notificationPayload: Map<String, String>) { if (notificationPayload.containsKey(OPPORTUNITY_ID) && cccCheckPassed(context)) { - val opportunityId = notificationPayload.get(OPPORTUNITY_ID) + val opportunityId = notificationPayload[OPPORTUNITY_ID]!! startWorkRequest( notificationPayload, SyncAction.SYNC_DELIVERY_PROGRESS, SyncAction.SYNC_DELIVERY_PROGRESS.toString() + "-$opportunityId" ) } }
242-245: Consider using isNotEmpty() for better readability.Line 242 uses
isEmpty()with negation. UsingisNotEmpty()would be more idiomatic Kotlin.Apply this diff:
- if (!notificationPayload.isEmpty()) { + if (notificationPayload.isNotEmpty()) { val pnJsonString = Gson().toJson(notificationPayload) inputDataBuilder.putString(NOTIFICATION_PAYLOAD, pnJsonString) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/linter.yml(1 hunks)app/src/org/commcare/CommCareApplication.java(4 hunks)app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt(3 hunks)app/src/org/commcare/connect/PersonalIdManager.java(3 hunks)app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt(1 hunks)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt(0 hunks)app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt(1 hunks)app/src/org/commcare/pn/workers/PNApiSyncWorker.kt(0 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)
💤 Files with no reviewable changes (2)
- app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt
- app/src/org/commcare/pn/workers/PNApiSyncWorker.kt
🧰 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-08T11:08:18.530Z
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.
Applied to files:
app/src/org/commcare/pn/workers/NotificationsSyncWorker.ktapp/src/org/commcare/connect/PersonalIdManager.javaapp/src/org/commcare/CommCareApplication.javaapp/src/org/commcare/services/CommCareFirebaseMessagingService.java
🧬 Code graph analysis (4)
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (1)
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
convertPNRecordsToPayload(84-92)
app/src/org/commcare/connect/PersonalIdManager.java (2)
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (2)
schedulePeriodicPushNotificationRetrieval(61-92)cancelPeriodicPushNotificationRetrieval(98-103)app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(158-1282)
app/src/org/commcare/CommCareApplication.java (1)
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (1)
schedulePeriodicPushNotificationRetrievalChecked(48-55)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (1)
startPNApiSync(142-144)
⏰ 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 (2)
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (2)
113-136: LGTM!The secondary constructors properly delegate to the primary constructor and initialize all necessary properties, including the conversion from
PushNotificationRecordto payload format.
142-184: LGTM!The routing logic correctly handles different notification types and the fallback notification sync (lines 179-182) ensures notifications are synced even when no CCC_MESSAGE action is present, which aligns with the stated intent.
| ProcessLifecycleOwner.get().getLifecycle().addObserver(this); | ||
| NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrievalChecked( | ||
| CommCareApplication.instance()); | ||
| } |
There was a problem hiding this comment.
Honor the background-work toggle before queuing periodic PN sync.
HiddenPreferences.shouldDisableBackgroundWork() governs whether we schedule WorkManager jobs throughout this class. Calling schedulePeriodicPushNotificationRetrievalChecked() unconditionally will enqueue a periodic sync even when the user has disabled background work, undermining that preference. Please wrap this call in the same guard (and optionally cancel the work when the preference is off) so the notifications worker follows the existing background-work contract.
- NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrievalChecked(
- CommCareApplication.instance());
+ if (!HiddenPreferences.shouldDisableBackgroundWork()) {
+ NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrievalChecked(
+ CommCareApplication.instance());
+ } else {
+ NotificationsSyncWorkerManager.cancelPeriodicPushNotificationRetrieval(
+ CommCareApplication.instance());
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/CommCareApplication.java around lines 277 to 280, the
call to
NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrievalChecked(...)
is executed unconditionally and ignores the
HiddenPreferences.shouldDisableBackgroundWork() flag; wrap that scheduling call
in the same guard that protects other WorkManager jobs (i.e., check
HiddenPreferences.shouldDisableBackgroundWork() and only schedule when
background work is enabled), and when the preference indicates background work
is disabled, ensure any existing periodic notification work is cancelled (call
the existing cancel/unschedule method on NotificationsSyncWorkerManager or add
one if missing) so notification sync respects the background-work toggle.
| personalIdSatus = PersonalIdStatus.LoggedIn; | ||
| scheduleHeartbeat(); | ||
| NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval(CommCareApplication.instance()); | ||
| CrashUtil.registerUserData(); |
There was a problem hiding this comment.
Respect background-work disablement before scheduling PN worker.
We already honour HiddenPreferences.shouldDisableBackgroundWork() when wiring other background jobs (form submission, auto-update). Without the same guard here we will enqueue a periodic notifications sync even when the user explicitly disabled background work, defeating that setting. Please gate this call (and add the corresponding HiddenPreferences import) so we skip scheduling whenever background work is turned off, or cancel an existing schedule in that state.
- NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval(CommCareApplication.instance());
+ if (!HiddenPreferences.shouldDisableBackgroundWork()) {
+ NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval(
+ CommCareApplication.instance());
+ }📝 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.
| personalIdSatus = PersonalIdStatus.LoggedIn; | |
| scheduleHeartbeat(); | |
| NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval(CommCareApplication.instance()); | |
| CrashUtil.registerUserData(); | |
| personalIdSatus = PersonalIdStatus.LoggedIn; | |
| scheduleHeartbeat(); | |
| if (!HiddenPreferences.shouldDisableBackgroundWork()) { | |
| NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval( | |
| CommCareApplication.instance()); | |
| } | |
| CrashUtil.registerUserData(); |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around lines 207 to 210,
add an import for HiddenPreferences and guard the call to
NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval(CommCareApplication.instance())
so it only runs when !HiddenPreferences.shouldDisableBackgroundWork(); if
background work is disabled, call
NotificationsSyncWorkerManager.cancelPeriodicPushNotificationRetrieval(CommCareApplication.instance())
(or the equivalent cancel method) instead to remove any existing schedule.
| */ | ||
| @JvmStatic | ||
| fun schedulePeriodicPushNotificationRetrievalChecked(context: Context) { | ||
| val perosnalIdManager = PersonalIdManager.getInstance() |
There was a problem hiding this comment.
Fix typo in variable name.
The variable name perosnalIdManager should be personalIdManager.
Apply this diff:
- val perosnalIdManager = PersonalIdManager.getInstance()
- perosnalIdManager.init(context)
- if (perosnalIdManager.isloggedIn()) {
+ val personalIdManager = PersonalIdManager.getInstance()
+ personalIdManager.init(context)
+ if (personalIdManager.isloggedIn()) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt
around line 50, the local variable is misspelled as `perosnalIdManager`; rename
it to `personalIdManager` and update all references in the same scope to use the
corrected name so the code compiles and reads clearly.
| } | ||
| } | ||
|
|
||
| lateinit var notificationsPayload: ArrayList<Map<String, String>> |
There was a problem hiding this comment.
Potential UninitializedPropertyAccessException with lateinit var.
The notificationsPayload property is declared as lateinit var but the primary constructor does not initialize it. If code uses the primary constructor and then calls startPNApiSync(), it will crash when trying to iterate over the uninitialized property at line 148.
Consider one of these solutions:
- Initialize with an empty list by default:
- lateinit var notificationsPayload: ArrayList<Map<String, String>>
+ var notificationsPayload: ArrayList<Map<String, String>> = ArrayList()- Add a check in
startSyncWorker():
private fun startSyncWorker(): Boolean {
var isNotificationSyncScheduled = false
+ if (!::notificationsPayload.isInitialized) {
+ return false
+ }
for (notificationPayload in notificationsPayload) {📝 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.
| lateinit var notificationsPayload: ArrayList<Map<String, String>> | |
| var notificationsPayload: ArrayList<Map<String, String>> = ArrayList() |
🤖 Prompt for AI Agents
In app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt
around line 106 (property) and line 148 (iteration), the lateinit var
notificationsPayload may be uninitialized causing a Potential
UninitializedPropertyAccessException when startPNApiSync() iterates it; fix by
initializing the property to an empty list (e.g., = ArrayList()) or make it
nullable and guard the iteration (or use ::notificationsPayload.isInitialized)
before iterating so startPNApiSync() never reads an uninitialized value; update
the property declaration and the iteration site accordingly.
10ed9e5 to
69eeda9
Compare
shubham1g5
force-pushed
the
testLinter
branch
from
949271f to
2b78e44
Compare
shubham1g5
force-pushed
the
testLinter
branch
from
2b78e44 to
7e917b2
Compare
|
@damagatchi retest this please |
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
|
@damagatchi retest this please |
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
I am very concerned about the readability of the code caused by formatting changes from the linter. Can we verify that this linter follows best/standard practices for writing Java code? I'm open to having a discussion about this with the team or just to hear other people's thoughts
| Thread.setDefaultUncaughtExceptionHandler( | ||
| new CommCareExceptionHandler(Thread.getDefaultUncaughtExceptionHandler(), this)); |
There was a problem hiding this comment.
This is very interesting. We don't want closing parentheses to be on a new line in scenarios like this? Is that standard practice? For example:
Object.method(
argument
);
There was a problem hiding this comment.
I don't have strong thoughts on this lint, think we copied this from google checkstyle so I would think it's a standard practice. I do think Kotlin follows it more but is not opposed to us implementing it for Java as such.
There was a problem hiding this comment.
I'd be okay with either here as well. In my personal experience I'm more accustomed to leaving those closing symbols on the same line as the previous. I like that it's a bit more visually compact, and it's difficult for bugs to slip through due to those characters missing (much more likely to be caught as a syntax error) so doesn't seem like it needs to be visually separated.
That being said, if it's more in line with Kotlin standards and we're moving more toward Kotlin anyway then making this change might be nice for consistency.
| AndroidClassHasher.registerAndroidClassHashStrategy(); | ||
|
|
||
| ActivityManager am = (ActivityManager) getSystemService(ACTIVITY_SERVICE); | ||
| ActivityManager am = (ActivityManager)getSystemService(ACTIVITY_SERVICE); |
There was a problem hiding this comment.
Similar comment here. Removing that space is standard practice? Doesn't that hinder readability?
There was a problem hiding this comment.
This is a legacy thing where we don't want to change the style to standard because it will cause a lot of formatting changes in our code base.
| PerformanceTuningUtil.updateMaxPrefetchCaseBlock( | ||
| PerformanceTuningUtil.guessLargestSupportedBulkCaseFetchSizeFromHeap((long) memoryClass * 1024 * 1024)); | ||
| PerformanceTuningUtil.guessLargestSupportedBulkCaseFetchSizeFromHeap( | ||
| (long)memoryClass * 1024 * 1024)); | ||
| } |
There was a problem hiding this comment.
This seems a bit strange. Wouldn't something like this be more readable?:
PerformanceTuningUtil.updateMaxPrefetchCaseBlock(
PerformanceTuningUtil.guessLargestSupportedBulkCaseFetchSizeFromHeap(
(long) memoryClass * 1024 * 1024
)
);
I'm open for thoughts here
| public <T extends Persistable> HybridFileBackedSqlStorage<T> getFileBackedUserStorage(String storage, | ||
| Class<T> c) { | ||
| return new HybridFileBackedSqlStorage<>(storage, c, buildUserDbHandle(), getUserKeyRecordId(), | ||
| CommCareApplication.instance().getCurrentApp()); |
There was a problem hiding this comment.
This could be a lot more readable too. For example:
| public <T extends Persistable> HybridFileBackedSqlStorage<T> getFileBackedUserStorage(String storage, | |
| Class<T> c) { | |
| return new HybridFileBackedSqlStorage<>(storage, c, buildUserDbHandle(), getUserKeyRecordId(), | |
| CommCareApplication.instance().getCurrentApp()); | |
| public <T extends Persistable> HybridFileBackedSqlStorage<T> getFileBackedUserStorage( | |
| String storage, | |
| Class<T> c | |
| ) { | |
| return new HybridFileBackedSqlStorage<>( | |
| storage, | |
| c, | |
| buildUserDbHandle(), | |
| getUserKeyRecordId(), | |
| CommCareApplication.instance().getCurrentApp() | |
| ); | |
| } | |
There was a problem hiding this comment.
agree, think we might not be enforcing single line arguments in case of multi-line arguments for Java.
|
@conroy-ricketts think all your suggestions are valid here but think they are probably best taken separately out of scope of this PR. Also think you will be noticing a bunch of these as you work more with the code base and it would be nice to collate a list of suggestions in a doc or Jira ticket that we can decide on as a team. |
|
@damagatchi retest this please |
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Updating my review to non-blocking. I'd like a second set of eyes on this before merging
This updates the super linter to work correctly for Java files, I have disabled some linters to remove noise as we start implementing lint checks on PRs and we can enable them one by one in future if we feel the need to add additional validation.
Some linters doesn't really support operating just on changed files but scan the entire codebase, I have disabled them too except markdown one as there are not many markdown files in the repo, I have formatted the markdown correctly so that it passes the check and doesn't cause issues on the PR.
I have also formatted a few Java files to induce changes in the PR in a way to demonstrate that it's working correctly for Java files.
Safery Story