Solved Android 15 edge to edge issue + added loading blocking UI #3421
Conversation
📝 WalkthroughWalkthroughThe PR refactors the push notification activity architecture by migrating Sequence DiagramsequenceDiagram
participant Activity as PushNotificationActivity
participant VM as PushNotificationViewModel
participant DB as Database/API
Note over Activity,DB: Before: AppCompatActivity, context via getApplication()
Activity->>VM: loadNotifications(isRefreshed)
VM->>DB: Access via getApplication()
Note over Activity,DB: After: CommCareActivity, context passed explicitly
Activity->>VM: loadNotifications(isRefreshed, context)
VM->>DB: Access via provided context
Activity->>Activity: generateProgressDialog(taskId)
Note over Activity: onResume: No immediate callback
Note over Activity: onPause: Broadcast receiver unregistered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (1)
app/src/org/commcare/activities/PushNotificationActivity.kt (1)
123-123: Use named parameter for consistency.Line 123 uses a positional argument while lines 34 and 111 use the named parameter
isRefreshed =. For consistency and readability, use the named parameter here as well.Apply this diff:
- pushNotificationViewModel.loadNotifications(false, this) + pushNotificationViewModel.loadNotifications(isRefreshed = false, this)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/res/layout/activity_push_notification.xml(0 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(4 hunks)app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt(3 hunks)app/src/org/commcare/pn/helper/NotificationBroadcastHelper.kt(1 hunks)
💤 Files with no reviewable changes (1)
- app/res/layout/activity_push_notification.xml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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-04-21T15:02:17.492Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.
Applied to files:
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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/activities/PushNotificationActivity.kt
🧬 Code graph analysis (1)
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (1)
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
retrieveLatestPushNotifications(62-65)
⏰ 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 (5)
app/src/org/commcare/pn/helper/NotificationBroadcastHelper.kt (1)
10-41: LGTM! Formatting improvements follow Kotlin conventions.The multiline formatting with trailing commas improves readability and follows Kotlin best practices. The lifecycle management with receiver registration/unregistration remains correct.
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (2)
17-19: LGTM! Constructor formatting improved.The multiline formatting enhances readability without changing behavior.
30-68: No action required—code is correct.The
NotificationsSyncWorkerManagerhas a secondary constructor (line 145–152) that accepts(Context, List<PushNotificationRecord>?, Boolean, Boolean?). The call at line 54 passesapplication(aContextsubclass), the API response list, andfalse—matching the signature exactly. UsingApplicationinstead of the activitycontextis intentional and correct forWorkManagerto avoid context leaks.app/src/org/commcare/activities/PushNotificationActivity.kt (2)
21-21: LGTM! Inheritance change addresses Android 15 edge-to-edge issue.Migrating from
AppCompatActivitytoCommCareActivityenables reuse of the edge-to-edge handling logic, addressing the primary PR objective.
127-128: Progress dialog implementation verified and properly integrated.The
generateProgressDialogoverride is correctly wired into the CommCareActivity dialog lifecycle. When BaseApi initiates network calls, it invokesshowProgressDialog(NETWORK_ACTIVITY_ID), which calls your override to create the CustomProgressDialog with the taskId, then displays it viashowNow(). Upon completion, BaseApi callsdismissProgressDialogForTask(NETWORK_ACTIVITY_ID), which retrieves the dialog by taskId and dismisses it. The dialog lifecycle is properly managed and blocking as intended.
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
A couple nits and one thing to verify but otherwise looks good to me
| observeRetrieveNotificationApi() | ||
| registerForNewNotification() | ||
| pushNotificationViewModel.loadNotifications(isRefreshed = false) | ||
| pushNotificationViewModel.loadNotifications(isRefreshed = false, this) |
There was a problem hiding this comment.
Nit: Having an unlabeled input after a labeled one feels odd to me, thinking maybe better to remove the label here? So it looks like line 123
|
|
||
| R.id.notification_cloud_sync -> { | ||
| pushNotificationViewModel.loadNotifications(isRefreshed = true) | ||
| pushNotificationViewModel.loadNotifications(isRefreshed = true, this) |
| owner.lifecycle.addObserver( | ||
| object : DefaultLifecycleObserver { | ||
| override fun onResume(owner: LifecycleOwner) { | ||
| manager.registerReceiver(receiver, IntentFilter(ACTION_NEW_NOTIFICATIONS)) |
There was a problem hiding this comment.
I see onNewNotification was removed here, just making sure that was intentional?
There was a problem hiding this comment.
Yes it was intentional. Whenever any activity register for new notification broadcast, it was calling onNewNotification even though there were no new notifications.
Technical Summary
https://dimagi.atlassian.net/browse/QA-8230
CommCareActivity has handling for edge to edge issue, which might arise between Android versions greater and less than 15. PushNotificationActivity was showing correctly for Android 15+ but having this issue for <Android 15. So now PushNotificationActivity is extended from CommCareActivity. As PushNotification is extended from CommCareActivity, changes have been made to show the blocking loading await popup also whenever API is being called.
Labels and Review