Managed messages unread counter in side nav bar #3398
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis PR introduces a message counter badge feature in the navigation drawer. It adds a new red background drawable resource, modifies the navigation drawer layout to include a badge overlay with a message count display, updates the Sequence DiagramsequenceDiagram
participant OldFlow as Old: Icon Switching
participant NewFlow as New: Badge Display
rect rgb(200, 220, 240)
Note over OldFlow: Previous Approach
OldFlow->>OldFlow: Check unread messages
OldFlow->>OldFlow: Select icon (read/unread)
OldFlow->>OldFlow: Render with icon
end
rect rgb(220, 240, 220)
Note over NewFlow: New Approach
NewFlow->>NewFlow: Count unread messages
NewFlow->>NewFlow: Create messageCount string
NewFlow->>NewFlow: Pass to ParentItem
NewFlow->>NewFlow: Bind count to badge
NewFlow->>NewFlow: Display badge (if count > 0)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 1
🧹 Nitpick comments (2)
app/src/org/commcare/navdrawer/NavDrawerAdapter.kt (1)
82-90: Consider using Int? instead of String? for messageCount.The badge logic correctly handles display and capping at "9+". However, the round-trip conversion (Int→String in BaseDrawerController line 191, then String→Int here) sacrifices type safety. Since messageCount always represents a numeric count, consider changing the
messageCountfield inNavDrawerParentItemtoInt?instead ofString?. This would eliminate parsing logic, improve type safety, and make the intent clearer.If you keep String?, the current implementation is safe due to
toIntOrNull(), but Int? would be more idiomatic for numeric counts.app/src/org/commcare/navdrawer/NavDrawerParentItem.kt (1)
12-13: Consider Int? type for better type safety.The
messageCountfield usesString?but represents a numeric count, requiring parsing inNavDrawerAdapter.kt(line 82). UsingInt?instead would:
- Eliminate parsing logic and potential parsing failures
- Provide compile-time type safety
- Make the field's intent clearer (it's always a count, never arbitrary text)
The current implementation works safely due to
toIntOrNull(), butInt?would be more idiomatic for a numeric count.Apply this diff if you choose to refactor:
val children: List<ChildItem> = emptyList(), - val messageCount: String? = null + val messageCount: Int? = nullThen update BaseDrawerController line 191 to pass the Int directly, and simplify the adapter logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/drawable/bg_msg_count_red.xml(1 hunks)app/res/layout/nav_drawer_list_item.xml(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(1 hunks)app/src/org/commcare/navdrawer/NavDrawerAdapter.kt(2 hunks)app/src/org/commcare/navdrawer/NavDrawerParentItem.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/navdrawer/BaseDrawerController.kt
⏰ 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/res/drawable/bg_msg_count_red.xml (1)
1-4: LGTM!The drawable resource is correctly defined and appropriate for a notification badge background.
app/src/org/commcare/navdrawer/NavDrawerAdapter.kt (1)
7-7: LGTM!The new import and view references are correctly added to support the message count badge UI.
Also applies to: 73-74
app/res/layout/nav_drawer_list_item.xml (1)
10-11: LGTM!The clip attributes are necessary to allow the badge to overflow the icon boundaries and appear correctly positioned.
app/src/org/commcare/navdrawer/BaseDrawerController.kt (2)
193-200: LGTM!The messaging item construction correctly passes the calculated message count and uses a single icon, letting the badge convey unread state. This is a cleaner approach than toggling between read/unread icons.
190-191: Review comment is verified as accurate. Performance concerns are justified and refactoring suggestions should be addressed.The verification confirms:
getUnviewedMessages()fetches all unviewed message records from the database (line 143-146 in ConnectMessagingDatabaseHelper.java).sizeis called on the entire returned list, fetching full objects unnecessarily- Called on drawer slide callback (line 73,
onDrawerSlide), which runs on the main thread- No COUNT-based alternative method exists in the current API
The suggested optimizations are sound. Implement one of:
- COUNT query: Most efficient—add a
getUnviewedMessageCount()method to avoid fetching full records- Caching: Cache the count and invalidate on message updates
- Reactive pattern: Use LiveData/Flow to observe count changes instead of querying on every slide
⛔ Skipped due to learnings
Learnt from: pm-dimagi Repo: dimagi/commcare-android PR: 2956 File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60 Timestamp: 2025-02-19T15:15:01.935Z Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.Learnt from: OrangeAndGreen Repo: dimagi/commcare-android PR: 3248 File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0 Timestamp: 2025-07-29T14:14:07.954Z Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
| <FrameLayout | ||
| android:id="@+id/flMessageCounter" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="end|top" | ||
| android:background="@drawable/bg_msg_count_red" | ||
| android:clipChildren="false" | ||
| android:clipToPadding="false" | ||
| android:paddingHorizontal="4dp" | ||
| android:paddingVertical="1dp" | ||
| android:translationX="6dp" | ||
| android:translationY="-6dp" | ||
| android:visibility="gone"> | ||
|
|
||
| <TextView | ||
| android:id="@+id/tvMessageCount" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:gravity="center" | ||
| android:includeFontPadding="false" | ||
| android:textColor="@android:color/white" | ||
| android:textSize="11sp" /> | ||
| </FrameLayout> |
There was a problem hiding this comment.
Address RTL layout support and accessibility.
The badge implementation has a few concerns:
-
RTL layout support:
translationX="6dp"(line 37) will position the badge incorrectly in right-to-left locales. In RTL,layout_gravity="end"positions at the left, buttranslationXstill moves right. Consider usinglayout_marginEndor handling RTL explicitly. -
Accessibility: The badge TextView lacks a
contentDescription. Screen readers won't announce the unread message count, making this information inaccessible to users with visual impairments. Add appropriate content descriptions to communicate the count. -
API level:
paddingHorizontal(line 35) requires API 26+. Verify that yourminSdkVersionsupports this, or usepaddingStart/paddingEndorpaddingLeft/paddingRightfor broader compatibility.
For RTL, consider replacing the translation with proper margin handling:
- android:translationX="6dp"
- android:translationY="-6dp"
+ android:layout_marginEnd="-6dp"
+ android:layout_marginTop="-6dp"For accessibility, update the adapter binding logic to set content description dynamically when the badge is shown.
🤖 Prompt for AI Agents
In app/res/layout/nav_drawer_list_item.xml around lines 27 to 49, update the
badge to be RTL-safe, accessible and API-compatible: remove the fixed
translationX and instead position the badge using layout_marginEnd (or
layout_marginStart when appropriate) combined with layout_gravity="end" so it
flips automatically in RTL; replace paddingHorizontal with paddingStart and
paddingEnd (or explicit left/right fallbacks if you must support pre-API26
minSdk) to avoid API-26-only attributes; and add a contentDescription attribute
placeholder on the TextView and change your adapter binding to set its
contentDescription dynamically when the badge is visible (e.g., "X unread
messages") so screen readers announce the count. Ensure you only set the
contentDescription when the badge is shown and clear it when hidden.
There was a problem hiding this comment.
@jaypanchal-13 all of these needs to be addressed.
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!
| var isExpanded: Boolean = false, | ||
| val children: List<ChildItem> = emptyList() | ||
| val children: List<ChildItem> = emptyList(), | ||
| val messageCount: String? = null |
There was a problem hiding this comment.
lets keep it generic and call it badgeCount
| val countText = item.messageCount?.toIntOrNull()?.let { | ||
| if (it > 9) "9+" else it.toString() | ||
| } | ||
| if (!countText.isNullOrEmpty()) { | ||
| messageCount.text = countText | ||
| flMessageCounter.visibility = View.VISIBLE | ||
| } else { | ||
| flMessageCounter.visibility = View.GONE | ||
| } |
There was a problem hiding this comment.
can be abstracted into another method
| android:src="@drawable/commcare_actionbar_logo" /> | ||
|
|
||
| <FrameLayout | ||
| android:id="@+id/flMessageCounter" |
There was a problem hiding this comment.
rename id to badge_layout (ids are underscored)
| android:visibility="gone"> | ||
|
|
||
| <TextView | ||
| android:id="@+id/tvMessageCount" |
| <FrameLayout | ||
| android:id="@+id/flMessageCounter" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="end|top" | ||
| android:background="@drawable/bg_msg_count_red" | ||
| android:clipChildren="false" | ||
| android:clipToPadding="false" | ||
| android:paddingHorizontal="4dp" | ||
| android:paddingVertical="1dp" | ||
| android:translationX="6dp" | ||
| android:translationY="-6dp" | ||
| android:visibility="gone"> | ||
|
|
||
| <TextView | ||
| android:id="@+id/tvMessageCount" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:gravity="center" | ||
| android:includeFontPadding="false" | ||
| android:textColor="@android:color/white" | ||
| android:textSize="11sp" /> | ||
| </FrameLayout> |
There was a problem hiding this comment.
@jaypanchal-13 all of these needs to be addressed.
| var isExpanded: Boolean = false, | ||
| val children: List<ChildItem> = emptyList() | ||
| val children: List<ChildItem> = emptyList(), | ||
| val messageCount: String? = null |
There was a problem hiding this comment.
messageCount can be int only to stop conversions
There was a problem hiding this comment.
@Jignesh-dimagi In any way it will need conversion as it needs to show in textview
There was a problem hiding this comment.
Now it is being converted many times and count should be int only
| private fun shouldShowNotiifcations(): Boolean { | ||
| return PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS) | ||
| } | ||
| private fun shouldShowNotiifcations(): Boolean = PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS) |
| android:layout_height="wrap_content" | ||
| android:gravity="center" | ||
| android:includeFontPadding="false" | ||
| android:contentDescription="@null" |
There was a problem hiding this comment.
what are we trying to do here by specifying contentDescription as "@null"
There was a problem hiding this comment.
@shubham1g5 Satisfying coderabbit changes
There was a problem hiding this comment.
Is not this same as not having contentDescription at all ? I am not sure how it addresses anything related to accessibility.
There was a problem hiding this comment.
@shubham1g5 Yes correct. It was passed to satisy coderabbit. We can also use importantForAccessibility="no"
There was a problem hiding this comment.
it should have a string text set that accessibility readers can read out aloud when the view is in focus - Like unread count 5 .
There was a problem hiding this comment.
Actually I see that none of the views in this layout have contentDescription , so think fine to skip them out and we will need to prioritize them separately, but please remove android:contentDescription="@null" from all views in this file
jaypanchal-13
dismissed stale reviews from conroy-ricketts, OrangeAndGreen, and Jignesh-dimagi
via
eb9adb9
|
@jaypanchal-13 Reminder to take a pass on lint errors before merging here |
…o-msg-icon-drawer
jaypanchal-13
dismissed stale reviews from OrangeAndGreen, conroy-ricketts, Jignesh-dimagi, and shubham1g5
via
5cc43a2
|
@damagatchi retest this please |
|
@damagatchi retest this please |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1802
Technical Summary
Screenshot ->
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review