Added PushNotificationActivity and handle navigation for it #3313
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
|
@jaypanchal-13 can we add screenshots for UI changes in PR. |
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ConnectActivity
participant AndroidOS as Android OS
participant PushNotificationActivity
User->>ConnectActivity: Tap bell (action_bell)
ConnectActivity->>AndroidOS: startActivity(PushNotificationActivity)
AndroidOS-->>PushNotificationActivity: onCreate()
PushNotificationActivity->>PushNotificationActivity: Setup ActionBar (title, Up)
PushNotificationActivity->>PushNotificationActivity: Inflate menu_notification
Note over PushNotificationActivity: Await user action
alt User selects Up
User->>PushNotificationActivity: Tap Up
PushNotificationActivity->>AndroidOS: finish()
else User selects Cloud Sync
User->>PushNotificationActivity: Tap notification_cloud_sync
PushNotificationActivity->>PushNotificationActivity: Placeholder: trigger sync API
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 5
🧹 Nitpick comments (7)
app/res/layout/activity_push_notification.xml (1)
2-10: Use a lighter root for an empty screenConstraintLayout adds unnecessary cost when there are no child constraints. Prefer FrameLayout until content is added.
- <androidx.constraintlayout.widget.ConstraintLayout + <FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools" android:id="@+id/main" android:layout_width="match_parent" android:layout_height="match_parent" tools:context="org.commcare.activities.PushNotificationActivity"> - </androidx.constraintlayout.widget.ConstraintLayout> + </FrameLayout>app/AndroidManifest.xml (1)
569-571: Remove unused PersonalIdCredentialActivity and its manifest entry
Verified no references beyond its declaration in AndroidManifest.xml (lines 569–571) and implementation in app/src/org/commcare/activities/connect/PersonalIdCredentialActivity.kt:17.app/res/menu/menu_connect.xml (2)
16-21: Bell action looks good; add tooltip and align tint with other icons.Add tooltip; consider white tint for visual parity with other actions.
Apply:
<item android:id="@+id/action_bell" android:icon="@drawable/ic_bell" android:title="@string/personalid_notification" + android:tooltipText="@string/personalid_notification" + app:iconTint="@color/white" app:showAsAction="always" />
10-14: Delete hidden, obsolete menu item.If credentials action is retired, remove it rather than keeping it invisible.
Apply:
- <item - android:id="@+id/action_credential" - android:icon="@drawable/ic_credential" - android:title="@string/personalid_credential" - android:visible="false" - app:showAsAction="always" />app/src/org/commcare/activities/PushNotificationActivity.kt (3)
43-46: Stub action should give feedback and prevent double-taps.Add a TODO, provide user feedback, and guard against repeat taps until the sync completes.
Apply:
- R.id.notification_cloud_sync -> { - //api call to sync notification - true - } + R.id.notification_cloud_sync -> { + // TODO(CCCT-1623): Trigger PN history sync (e.g., via ViewModel/WorkManager) + item.isEnabled = false + // Toast.makeText(this, R.string.sync_started, Toast.LENGTH_SHORT).show() + // on completion, re-enable item + true + }
11-19: Minor: prefer lateinit init over lazy for view binding.Keeps binding lifecycle explicit and avoids accessing
layoutInflaterbeforeonCreate.Apply:
-class PushNotificationActivity : AppCompatActivity() { - private val binding: ActivityPushNotificationBinding by lazy { - ActivityPushNotificationBinding.inflate(layoutInflater) - } +class PushNotificationActivity : AppCompatActivity() { + private lateinit var binding: ActivityPushNotificationBinding @@ override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) - setContentView(binding.root) + binding = ActivityPushNotificationBinding.inflate(layoutInflater) + setContentView(binding.root) setUpUi() }
10-10: Architecture: consider a Fragment under Connect instead of a separate Activity.Connect uses Navigation Components; a
PushNotificationFragmentinside the Connect graph would keep toolbar/back behavior and state management consistent.If this stays an Activity, please document rationale in the PR and ensure consistent theming and navigation expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
app/AndroidManifest.xml(1 hunks)app/res/drawable/ic_bell.xml(1 hunks)app/res/layout/activity_push_notification.xml(1 hunks)app/res/menu/menu_connect.xml(1 hunks)app/res/menu/menu_notification.xml(1 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(2 hunks)
🧰 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/activities/connect/ConnectActivity.java
⏰ 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 (10)
app/res/drawable/ic_bell.xml (1)
1-9: Make ic_bell.xml theme-tintable and accessible
- Add
android:autoMirrored="true"andandroid:tint="?attr/colorControlNormal"on the<vector>root.- Remove the hard-coded
android:fillColor="#ffffff"from the<path>so the tint drives the icon color.- Verify the menu item (or wherever this drawable is used) sets
android:contentDescription="@string/personalid_notification".app/res/values-es/strings.xml (1)
490-491: LGTM: Spanish localization added for notifications.Key and capitalization look consistent with UI usage.
app/res/values-fr/strings.xml (1)
492-493: LGTM: French localization added for notifications.Matches the English plural; no diacritics issues.
app/res/values-ti/strings.xml (1)
482-484: Confirm Tigrinya phrasing for personalid_notification
The string “መፍለጥታትን” ends with “ን” (object marker), which may be extraneous—verify with a Tigrinya reviewer whether “መፍለጥታት” (without “ን”) is the intended form.app/res/values-hi/strings.xml (1)
488-489: LGTM: Hindi localization added for notifications.Term “सूचनाएं” is idiomatic for system/app notifications.
app/res/values/strings.xml (1)
679-680: String addition LGTMKey/name is consistent with existing PersonalID keys and will localize the bell action/title correctly.
app/res/values-sw/strings.xml (1)
498-499: Swahili translation LGTM“Arifa” reads well for Notifications and is consistent with earlier usages in this file.
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
27-27: Import LGTMCorrect package for PushNotificationActivity.
app/res/values-pt/strings.xml (1)
498-499: Portuguese translation LGTM“Notificações” is correct.
app/res/menu/menu_connect.xml (1)
16-21: No action required: handler foraction_bellconfirmed. Verified thatConnectActivitychecks forR.id.action_belland launchesPushNotificationActivity.
| <activity android:name="org.commcare.activities.PushNotificationActivity" /> | ||
| </application> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden component declaration: explicitly set exported, parent activity, and label.
The activity has no intent-filters, so it should not be exported. Also wire Up-navigation at the manifest level and set the screen label.
Apply:
- <activity android:name="org.commcare.activities.PushNotificationActivity" />
+ <activity
+ android:name="org.commcare.activities.PushNotificationActivity"
+ android:exported="false"
+ android:label="@string/personalid_notification"
+ android:parentActivityName="org.commcare.activities.connect.ConnectActivity">
+ <meta-data
+ android:name="android.support.PARENT_ACTIVITY"
+ android:value="org.commcare.activities.connect.ConnectActivity" />
+ </activity>🤖 Prompt for AI Agents
In app/AndroidManifest.xml around lines 583-584, the PushNotificationActivity
declaration must be hardened: add android:exported="false", set a descriptive
android:label (e.g. a new or existing string resource like
@string/push_notification_label), and declare Up-navigation by adding
android:parentActivityName pointing to your app's main/home activity (and
include the legacy meta-data entry
android:name="android.support.PARENT_ACTIVITY" with the same value for pre-API
16 support). Ensure no intent-filter is added so exported remains false.
| <item | ||
| android:id="@+id/notification_cloud_sync" | ||
| android:icon="@drawable/ic_backup" | ||
| app:iconTint="@color/white" | ||
| android:title="@string/personalid_sync_menu_title" | ||
| app:showAsAction="always" /> | ||
| </menu> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add tooltip and verify string exists across locales.
- Add tooltip for better accessibility/UX.
- Please verify
@string/personalid_sync_menu_titleexists in all targeted locales.
Apply:
<item
android:id="@+id/notification_cloud_sync"
android:icon="@drawable/ic_backup"
app:iconTint="@color/white"
android:title="@string/personalid_sync_menu_title"
+ android:tooltipText="@string/personalid_sync_menu_title"
app:showAsAction="always" />Run to verify strings:
🏁 Script executed:
#!/bin/bash
rg -nP '<string name="personalid_sync_menu_title"\b' app/res/values*/*.xml -C0Length of output: 1401
Add tooltip and define missing string resource
- Add
android:tooltipText="@string/personalid_sync_menu_title"to the menu item in app/res/menu/menu_notification.xml - Define
<string name="personalid_sync_menu_title">…</string>in app/res/values/strings.xml and add corresponding translations in all targeted locale folders
<item
android:id="@+id/notification_cloud_sync"
android:icon="@drawable/ic_backup"
app:iconTint="@color/white"
android:title="@string/personalid_sync_menu_title"
+ android:tooltipText="@string/personalid_sync_menu_title"
app:showAsAction="always" />📝 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.
| <item | |
| android:id="@+id/notification_cloud_sync" | |
| android:icon="@drawable/ic_backup" | |
| app:iconTint="@color/white" | |
| android:title="@string/personalid_sync_menu_title" | |
| app:showAsAction="always" /> | |
| </menu> | |
| <item | |
| android:id="@+id/notification_cloud_sync" | |
| android:icon="@drawable/ic_backup" | |
| app:iconTint="@color/white" | |
| android:title="@string/personalid_sync_menu_title" | |
| android:tooltipText="@string/personalid_sync_menu_title" | |
| app:showAsAction="always" /> |
🤖 Prompt for AI Agents
In app/res/menu/menu_notification.xml around lines 5 to 11, the menu item lacks
a tooltip and references a string that may be missing; add
android:tooltipText="@string/personalid_sync_menu_title" to the <item>
declaration so the title is shown as a tooltip, and then in
app/res/values/strings.xml define the resource <string
name="personalid_sync_menu_title">Your sync title here</string> (replace with
the actual copy), and add the same string key with translated values in each
targeted locale folder (e.g., app/res/values-XX/strings.xml) to ensure all
locales have the translation.
| if (item.getItemId() == R.id.action_bell) { | ||
| startActivity(new Intent(this, PushNotificationActivity.class)); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle Up navigation and menu visibility for the bell
Launching is fine. Please verify:
- Manifest declares parentActivityName for PushNotificationActivity so Up works consistently.
- action_bell visibility mirrors other actions in onPrepareOptionsMenu.
If you want parity with sync icon tinting done in onCreateOptionsMenu, also set the bell icon’s tint there or via menu XML (app:iconTint).
🏁 Script executed:
#!/bin/bash
# Verify bell menu wiring and parent activity
rg -nC2 'id/action_bell' app/res/menu/menu_connect.xml
rg -nC3 'PushNotificationActivity' app/AndroidManifest.xml
rg -n 'parentActivityName=.*PushNotificationActivity' app/AndroidManifest.xml || echo "No parentActivityName on PushNotificationActivity"
# Optional: ensure visibility handled similarly to others
rg -nC2 'action_bell' app/src | rg -n 'setVisible' -n -C2 || echo "action_bell visibility not explicitly controlled"Length of output: 904
Add parentActivityName to manifest and toggle action_bell visibility
- In app/AndroidManifest.xml, add
android:parentActivityName="…ConnectActivity"(or appropriate parent) on the<activity>entry for PushNotificationActivity so Up navigation works. - In ConnectActivity’s onPrepareOptionsMenu, mirror other menu items by calling
menu.findItem(R.id.action_bell).setVisible(…)(or apply icon tint in XML) to control bell visibility.
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/connect/ConnectActivity.java around lines 192
to 195, the review asks to enable proper Up navigation for
PushNotificationActivity and to control the bell action visibility like other
menu items; update AndroidManifest.xml to add
android:parentActivityName="…ConnectActivity" on the PushNotificationActivity
<activity> entry (and add the corresponding <meta-data> or support parent in
activities if using older APIs), and in ConnectActivity.onPrepareOptionsMenu
mirror other menu handling by retrieving the bell menu item with
menu.findItem(R.id.action_bell) and calling setVisible(...) based on the same
visibility logic you use for other actions (or alternatively apply the icon
tint/visibility in the menu XML) so the bell icon is shown/hidden consistently.
| override fun onOptionsItemSelected(item: MenuItem): Boolean { | ||
| return when (item.itemId) { | ||
| android.R.id.home -> { | ||
| if (!isFinishing) { | ||
| finish() | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer onSupportNavigateUp()/onBackPressedDispatcher over manual finish check.
finish() is idempotent; isFinishing check is unnecessary. Using the dispatcher preserves back-stack semantics.
Apply:
- override fun onOptionsItemSelected(item: MenuItem): Boolean {
- return when (item.itemId) {
- android.R.id.home -> {
- if (!isFinishing) {
- finish()
- }
- true
- }
+ override fun onOptionsItemSelected(item: MenuItem): Boolean {
+ return when (item.itemId) {
+ android.R.id.home -> {
+ onBackPressedDispatcher.onBackPressed()
+ true
+ }Optionally also add:
+ override fun onSupportNavigateUp(): Boolean {
+ onBackPressedDispatcher.onBackPressed()
+ return true
+ }📝 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.
| override fun onOptionsItemSelected(item: MenuItem): Boolean { | |
| return when (item.itemId) { | |
| android.R.id.home -> { | |
| if (!isFinishing) { | |
| finish() | |
| } | |
| true | |
| } | |
| // Replace the manual finish() check with the back-press dispatcher | |
| override fun onOptionsItemSelected(item: MenuItem): Boolean { | |
| return when (item.itemId) { | |
| android.R.id.home -> { | |
| onBackPressedDispatcher.onBackPressed() | |
| true | |
| } | |
| else -> super.onOptionsItemSelected(item) | |
| } | |
| } | |
| // Optionally, also handle Up navigation via the dispatcher | |
| override fun onSupportNavigateUp(): Boolean { | |
| onBackPressedDispatcher.onBackPressed() | |
| return true | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/PushNotificationActivity.kt around lines 34
to 41, the options item handler checks isFinishing before calling finish(),
which is unnecessary and bypasses back-stack semantics; remove the isFinishing
check and instead call the proper navigation handler (either return
onSupportNavigateUp() or call onBackPressedDispatcher.onBackPressed()) when
handling android.R.id.home so the system/back-stack behavior is preserved and
finish() is not invoked directly.
There was a problem hiding this comment.
@shubham1g5 Thank you for confirming the refactor suggestion! The improved navigation handling will provide better user experience by properly respecting the back stack semantics instead of directly calling finish().
🧠 Learnings used
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.
There was a problem hiding this comment.
@jaypanchal-13 seems like code rabbit suggestion here is valid and is not resolved.
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!
There was a problem hiding this comment.
@jaypanchal-13 seems like code rabbit suggestion here is valid and is not resolved.
@shubham1g5 In code we are following same method for back navigation so I did not changed it
There was a problem hiding this comment.
this would be a great place to try out the new pattern then given the old pattern doesn't really make sense.
| android:visible="false" | ||
| app:showAsAction="always" /> | ||
|
|
||
| <item |
There was a problem hiding this comment.
lets hide it for now on UI untill we decide to publish this out to the users
There was a problem hiding this comment.
Also, we need to show icon with dot if there are new push notifications available.
There was a problem hiding this comment.
@Jignesh-dimagi I think this can be managed using flag while creating db schema for push notification
There was a problem hiding this comment.
@jaypanchal-13 do you plan to address that in a separate PR or this one ?
There was a problem hiding this comment.
@shubham1g5 there is different PR for notification db schema in that we can add flag for it and manage it there. What's your suggestion?
There was a problem hiding this comment.
@jaypanchal-13 I agree for the logic but we need to have bell icon with dot in this PR only so that it can directly used in your next PR.
There was a problem hiding this comment.
@Jignesh-dimagi Added new notification bell icon
| override fun onOptionsItemSelected(item: MenuItem): Boolean { | ||
| return when (item.itemId) { | ||
| android.R.id.home -> { | ||
| if (!isFinishing) { | ||
| finish() | ||
| } | ||
| true | ||
| } |
| startActivity(new Intent(this, PushNotificationActivity.class)); | ||
| return true; |
There was a problem hiding this comment.
@jaypanchal-13 We might want to start activity for result as might need to update the bell icon (with / without dot to indicate the new push notification)
There was a problem hiding this comment.
@Jignesh-dimagi Same we can create flag for it in db and use it. It will be managed while working on notification db
| android:visible="false" | ||
| app:showAsAction="always" /> | ||
|
|
||
| <item |
There was a problem hiding this comment.
Also, we need to show icon with dot if there are new push notifications available.
…-drawer # Conflicts: # app/res/values-hi/strings.xml # app/res/values-ti/strings.xml
…-pn-history' into CCCT-1624-pn-in-nav-drawer
…on-button-for-pn-history # Conflicts: # app/res/values-hi/strings.xml # app/res/values-ti/strings.xml
…1624-pn-in-nav-drawer
| } | ||
|
|
||
| if (item.getItemId() == R.id.action_bell) { | ||
| startActivity(new Intent(this, PushNotificationActivity.class)); |
There was a problem hiding this comment.
can we use ConnectNavHelper.goToNotification here as well
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
There are pending comments on PR from last review.
|
Added goToNotification
…-pn-history' into CCCT-1624-pn-in-nav-drawer # Conflicts: # app/src/org/commcare/connect/ConnectNavHelper.kt
|
@Jignesh-dimagi waiting for your review |
…1624-pn-in-nav-drawer
…1624-pn-in-nav-drawer
…1624-pn-in-nav-drawer
Added PN history in side nav drawer
|
@damagatchi retest this please |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1623
Added PushNotificationActivity and handle navigation for it
ScreenShot->
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review