Added toolbar in some activities for overlaying side nav drawer #3442
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis PR systematically adds Material Design AppBar and Toolbar components across multiple activities and layouts. The root layout for several activities transitions from ConstraintLayout/ScrollView to LinearLayout with AppBarLayout/CoordinatorLayout structures. Activity code is updated to initialize Toolbar instances, set them as support action bars, and enable up navigation. Title setting previously delegated to overridden setTitle methods is now handled through toolbar/navigation updates. Theme configuration changes from DarkActionBar to NoActionBar with WhiteNavIcon styling. Navigation drawer integration includes toolbar parameter updates to ActionBarDrawerToggle and new close button handling. Fragment title updates are moved from fragment initialization to activity-level navigation listener responses. Sequence DiagramsequenceDiagram
actor User
participant Activity
participant Toolbar
participant ActionBar
participant DrawerToggle
participant Fragment
User->>Activity: App launches
Activity->>Toolbar: findViewById(R.id.toolbar)
Activity->>ActionBar: setSupportActionBar(toolbar)
Activity->>ActionBar: setDisplayHomeAsUpEnabled(true)
Activity->>DrawerToggle: new ActionBarDrawerToggle(..., toolbar, ...)
DrawerToggle->>Toolbar: Update drawer icon color
rect rgba(100, 200, 150, 0.2)
Note over Activity,Fragment: Navigation Event
end
User->>Activity: Navigate to fragment
Activity->>Fragment: Destination listener triggered
Fragment->>ActionBar: Update title from navigation args
ActionBar->>Toolbar: Display updated title
User->>Activity: Tap drawer close button
Activity->>DrawerToggle: Close drawer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
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: 3
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/activities/connect/PersonalIdActivity.java (1)
81-84: Add null check to prevent potential NPE.The
setTitlemethod callsgetSupportActionBar()without a null check, unlikeupdateBackButton()which properly guards against null. This inconsistency could lead to a crash.@Override public void setTitle(CharSequence title) { super.setTitle(title); - getSupportActionBar().setTitle(title); + ActionBar actionBar = getSupportActionBar(); + if (actionBar != null) { + actionBar.setTitle(title); + } }app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt (1)
51-66: Remove duplicate action bar configuration.The action bar setup is performed twice with identical settings: once in
setUpUi()(lines 62-66) and again inonPostCreate()(lines 53-57). SincesetUpUi()is called fromonCreate()which runs beforeonPostCreate(), theonPostCreateconfiguration is redundant.Remove the duplicate configuration from
onPostCreate:override fun onPostCreate(savedInstanceState: Bundle?) { super.onPostCreate(savedInstanceState) - supportActionBar!!.apply { - title = getString(R.string.personalid_work_history_title) - setDisplayShowHomeEnabled(true) - setDisplayHomeAsUpEnabled(true) - } }Or, if you prefer to keep configuration in
onPostCreate(which is a valid pattern for action bar setup), remove it fromsetUpUiinstead.
🧹 Nitpick comments (11)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
103-103: Consider using theactivityfield consistently.Line 103 calls
requireActivity().setTitle(...)while the rest of the file uses theactivityfield (initialized at line 101). For consistency, either useactivity.setTitle(R.string.connect_registration_title)here or refactor the entire file to userequireActivity()directly instead of caching it in theactivityfield.app/src/org/commcare/activities/PushNotificationActivity.kt (1)
66-81: Avoid duplicating action bar configuration betweenonPostCreateandinitViews
supportActionBaris configured twice: once inonPostCreateand again right aftersetSupportActionBar(binding.toolbarNotification)ininitViews. This is redundant and risks future divergence (e.g., one place updated, the other forgotten).Since you now set the toolbar as the support action bar in
initViews, you can centralize all configuration there and remove theonPostCreateoverride.Example refactor:
- override fun onPostCreate(savedInstanceState: Bundle?) { - super.onPostCreate(savedInstanceState) - supportActionBar!!.apply { - title = getString(R.string.personalid_notification) - setDisplayShowHomeEnabled(true) - setDisplayHomeAsUpEnabled(true) - } - } - - private fun initViews() { - setSupportActionBar(binding.toolbarNotification) - supportActionBar!!.apply { - title = getString(R.string.personalid_notification) - setDisplayShowHomeEnabled(true) - setDisplayHomeAsUpEnabled(true) - } + private fun initViews() { + setSupportActionBar(binding.toolbarNotification) + supportActionBar!!.apply { + title = getString(R.string.personalid_notification) + setDisplayShowHomeEnabled(true) + setDisplayHomeAsUpEnabled(true) + }This keeps all toolbar/title logic in a single, obvious place.
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
188-190: Consider adding null safety for toolbar initialization.If the layout
R.layout.first_start_screen_moderndoes not contain a view withR.id.toolbar,findViewByIdreturnsnull, and passingnulltosetSupportActionBarfollowed bygetSupportActionBar()calls will cause issues.Toolbar toolbar = findViewById(R.id.toolbar); -setSupportActionBar(toolbar); -getSupportActionBar().setDisplayHomeAsUpEnabled(true); +if (toolbar != null) { + setSupportActionBar(toolbar); + ActionBar actionBar = getSupportActionBar(); + if (actionBar != null) { + actionBar.setDisplayHomeAsUpEnabled(true); + } +}app/src/org/commcare/activities/AppManagerActivity.java (1)
54-56: Consider adding a null check for defensive coding.While the toolbar should be present given the layout changes, adding a null check would make the code more resilient to future layout modifications.
Toolbar toolbar = findViewById(R.id.toolbar); - setSupportActionBar(toolbar); - getSupportActionBar().setDisplayHomeAsUpEnabled(true); + if (toolbar != null) { + setSupportActionBar(toolbar); + getSupportActionBar().setDisplayHomeAsUpEnabled(true); + }app/res/layout/screen_connect.xml (1)
14-15: Consider consistent toolbar id naming across layouts.This file uses
toolbar_connect_activitywhile other layouts in this PR use justtoolbar. If intentional (e.g., to avoid conflicts), this is fine; otherwise, consider standardizing for consistency.app/res/values/themes.xml (1)
35-36: Validate global icon tinting and toolbar nav style impactSetting
toolbarNavigationButtonStyletoWhiteNavIconandcolorControlNormalto@android:color/whitewill make many controls and icons render white by default, not just toolbar navigation buttons. Verify across key flows (forms, lists, dialogs) that this global tint does not reduce contrast or make non-toolbar icons invisible on light backgrounds; if it does, consider narrowing the white tint to just toolbar/navigation icons.Also applies to: 112-115
app/res/layout/update_activity.xml (1)
2-153: Minor layout polish for scrolling and button row spacingThe CoordinatorLayout/AppBarLayout/Toolbar setup looks correct, and the IDs match the controller expectations. Consider (a) adding
android:fillViewport="true"to theScrollViewso short-content screens still stretch to full height, and (b) replacing the hard-coded100dppaddings in@id/update_check_cancel_buttonswith a dimen resource or a centering approach (e.g.,gravity="center_horizontal"plus smaller margins) to avoid overly cramped buttons on small screens.app/res/layout/home_screen.xml (1)
28-110: Tighten NestedScrollView sizing for more predictable scrollingThe
NestedScrollViewinside the verticalLinearLayoutcurrently usesandroid:layout_height="wrap_content", which can lead to slightly odd behavior when content is short or when the parent height changes. Consider switching to a fill pattern, e.g.:- <androidx.core.widget.NestedScrollView + <androidx.core.widget.NestedScrollView android:id="@+id/nsv_home_screen" android:layout_width="match_parent" - android:layout_height="wrap_content"> + android:layout_height="match_parent" + android:fillViewport="true">This keeps the content pinned correctly under the toolbar and avoids partial-height scroll containers.
app/src/org/commcare/activities/LoginActivity.java (1)
26-27: Make toolbar setup null-safe and confirm layout is inflated before useThe new Toolbar wiring is straightforward, but it assumes (a) that a view with
@id/toolbaris present in all layouts used by this activity, and (b) thatgetSupportActionBar()is non-null:Toolbar toolbar = findViewById(R.id.toolbar); setSupportActionBar(toolbar); getSupportActionBar().setDisplayHomeAsUpEnabled(true);To harden this, add null checks and, given the BaseDrawerActivity hierarchy, double-check that the content view is set (via the base class or UI controller) before calling
findViewById:- Toolbar toolbar = findViewById(R.id.toolbar); - setSupportActionBar(toolbar); - getSupportActionBar().setDisplayHomeAsUpEnabled(true); + Toolbar toolbar = findViewById(R.id.toolbar); + if (toolbar != null) { + setSupportActionBar(toolbar); + if (getSupportActionBar() != null) { + getSupportActionBar().setDisplayHomeAsUpEnabled(true); + } + }Please also verify that this does not conflict with any toolbar/drawer wiring already performed in
BaseDrawerActivity.Also applies to: 133-139
app/src/org/commcare/navdrawer/DrawerViewRefs.kt (1)
9-10: Avoid hard non-null assumptions for toolbar/closeButton in shared drawer refs
DrawerViewRefsnow assumes every root view using the drawer has bothR.id.toolbarandR.id.nav_drawer_close:val toolbar: Toolbar = rootView.findViewById(R.id.toolbar) val closeButton: ImageView = rootView.findViewById(R.id.nav_drawer_close)If any screen with a drawer omits either view, you'll get an immediate NPE during construction. Either (a) ensure all drawer layouts define these IDs, or (b) make these properties nullable (and handled accordingly in
BaseDrawerActivity/controllers), e.g.val toolbar: Toolbar? = ..., to keep the shared helper resilient.Also applies to: 29-30
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
167-171: Remove commented code.Dead code should be deleted rather than commented out. Git history preserves the old implementation if needed.
Apply this diff:
- /*@Override - public void setTitle(CharSequence title) { - super.setTitle(title); - getSupportActionBar().setTitle(title); - }*/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
app/res/layout/activity_connect_id.xml(1 hunks)app/res/layout/activity_connect_messaging.xml(1 hunks)app/res/layout/activity_personal_id_work_history.xml(2 hunks)app/res/layout/activity_push_notification.xml(3 hunks)app/res/layout/app_manager.xml(1 hunks)app/res/layout/first_start_screen_modern.xml(1 hunks)app/res/layout/home_screen.xml(1 hunks)app/res/layout/nav_drawer_header.xml(1 hunks)app/res/layout/screen_connect.xml(1 hunks)app/res/layout/screen_login.xml(2 hunks)app/res/layout/update_activity.xml(1 hunks)app/res/navigation/nav_graph_connect_messaging.xml(1 hunks)app/res/values/themes.xml(3 hunks)app/src/org/commcare/activities/AppManagerActivity.java(2 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(2 hunks)app/src/org/commcare/activities/LoginActivity.java(2 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(2 hunks)app/src/org/commcare/activities/UpdateActivity.java(2 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(3 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(3 hunks)app/src/org/commcare/activities/connect/PersonalIdActivity.java(2 hunks)app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(2 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(4 hunks)app/src/org/commcare/navdrawer/DrawerViewRefs.kt(2 hunks)
💤 Files with no reviewable changes (1)
- app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
🧰 Additional context used
🧠 Learnings (13)
📓 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-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/res/navigation/nav_graph_connect_messaging.xmlapp/res/layout/activity_connect_messaging.xmlapp/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/res/layout/activity_connect_id.xmlapp/res/layout/screen_connect.xmlapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
📚 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/res/navigation/nav_graph_connect_messaging.xmlapp/res/layout/home_screen.xmlapp/res/layout/activity_connect_messaging.xmlapp/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/res/layout/activity_connect_id.xmlapp/res/layout/screen_connect.xmlapp/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-02-19T15:15:01.935Z
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.
Applied to files:
app/res/navigation/nav_graph_connect_messaging.xmlapp/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/res/navigation/nav_graph_connect_messaging.xmlapp/src/org/commcare/activities/PushNotificationActivity.kt
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Applied to files:
app/res/navigation/nav_graph_connect_messaging.xmlapp/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/src/org/commcare/activities/connect/PersonalIdActivity.javaapp/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-07-29T14:09:49.805Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Applied to files:
app/res/navigation/nav_graph_connect_messaging.xml
📚 Learning: 2025-07-29T14:14:07.954Z
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.
Applied to files:
app/res/navigation/nav_graph_connect_messaging.xmlapp/res/layout/activity_connect_messaging.xml
📚 Learning: 2025-05-07T06:49:29.295Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3070
File: app/res/values/attrs.xml:98-113
Timestamp: 2025-05-07T06:49:29.295Z
Learning: In Android resource files, attributes like `editTextHintColor` and `editTextHintSize` can be defined once globally with format declarations and then reused in multiple styleables without repeating the format. This is the proper pattern for attribute reuse in Android's resource system.
Applied to files:
app/res/values/themes.xml
📚 Learning: 2025-05-07T06:48:32.621Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3070
File: app/res/values/themes.xml:80-80
Timestamp: 2025-05-07T06:48:32.621Z
Learning: The CommCare Android app doesn't require backward compatibility for support library features like the vector drawable compatibility provided by app:srcCompat. It uses android:src directly for drawable references in themes.
Applied to files:
app/res/values/themes.xml
📚 Learning: 2025-05-09T13:45:18.661Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/layout/activity_connect_messaging.xml:0-0
Timestamp: 2025-05-09T13:45:18.661Z
Learning: In Android ConstraintLayout, use `layout_constraintStart_toStartOf` and `layout_constraintEnd_toEndOf` instead of the deprecated `layout_constraintLeft_toLeftOf` and `layout_constraintRight_toRightOf` to ensure proper RTL (Right-to-Left) layout support for languages like Arabic, Hebrew, and Persian.
Applied to files:
app/res/layout/activity_push_notification.xml
📚 Learning: 2025-05-09T13:45:18.661Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/layout/activity_connect_messaging.xml:0-0
Timestamp: 2025-05-09T13:45:18.661Z
Learning: In Android layouts, use `layout_constraintStart_toStartOf` and `layout_constraintEnd_toEndOf` instead of the deprecated `layout_constraintLeft_toLeftOf` and `layout_constraintRight_toRightOf` to ensure proper RTL (Right-to-Left) layout support.
Applied to files:
app/res/layout/activity_push_notification.xml
📚 Learning: 2025-05-09T07:05:19.320Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/layout/dialog_payment_confirmation.xml:57-65
Timestamp: 2025-05-09T07:05:19.320Z
Learning: In a ConstraintLayout, a view with width set to wrap_content only needs a single horizontal constraint (either start or end) for proper positioning, as opposed to views with match_constraint (0dp) width which require both horizontal constraints to determine their size.
Applied to files:
app/res/layout/activity_push_notification.xml
🧬 Code graph analysis (1)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
closeDrawer(136-145)
⏰ 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 (18)
app/src/org/commcare/activities/PushNotificationActivity.kt (1)
4-4: Import addition is fineAdding
android.util.Logis consistent with typical Android usage; no issues here.app/src/org/commcare/activities/connect/PersonalIdActivity.java (1)
21-22: LGTM - Toolbar setup follows the established pattern.The toolbar initialization is consistent with other activities in this PR.
app/res/layout/activity_personal_id_work_history.xml (2)
9-24: LGTM - AppBarLayout with Toolbar correctly integrated.The toolbar structure follows the established pattern in this PR, and the
toolbarID correctly matches the binding reference inPersonalIdWorkHistoryActivity.kt(binding.toolbar).
26-34: LGTM - TabLayout positioning updated correctly.The TabLayout is now properly constrained below the new app bar, maintaining the correct visual hierarchy.
app/res/layout/activity_push_notification.xml (1)
10-25: LGTM - AppBarLayout and Toolbar structure is correct.The layout follows the Material Design pattern with proper constraints. The toolbar ID
toolbar_notificationis activity-specific and correctly referenced in PushNotificationActivity via data binding (binding.toolbarNotification).app/res/layout/app_manager.xml (1)
9-22: LGTM!The Toolbar is correctly positioned at the top of the
RelativeLayout, and theFrameLayoutis properly constrained below it usinglayout_below. The structure maintains correct content flow.app/res/layout/screen_connect.xml (1)
7-20: LGTM - ConstraintLayout approach is correct.Using
0dpdimensions with constraints properly fills the remaining space inConstraintLayout. ThepopupThemeaddition ensures consistent overflow menu styling.app/res/values/themes.xml (1)
4-4: Confirm all activities are compatible with NoActionBar base themeChanging
CommonThemetoTheme.MaterialComponents.Light.NoActionBar.Bridgeremoves the framework action bar; any activity still assuming a built-in action bar or callinggetSupportActionBar()without setting a Toolbar will break. Please confirm all such activities have either been updated to provide a Toolbar or guarded appropriately.app/res/layout/first_start_screen_modern.xml (1)
2-49: Toolbar + drawer integration looks consistentThe new DrawerLayout/CoordinatorLayout/AppBarLayout/Toolbar structure with
@id/nav_drawer_framealigns with the updated home/login screens and should work well with the shared drawer controller. No issues from this diff.app/res/layout/screen_login.xml (1)
6-6: Toolbar integration on login screen looks good; confirm drawer wiringThe added
AppBarLayout/Toolbarat the top of the login content is consistent with the other updated drawer-based layouts and provides a proper app bar for LoginActivity. Please just confirm that this@id/toolbaris the oneDrawerViewRefs/BaseDrawerActivityexpect to use for drawer toggling and that no duplicate toolbars exist in alternative login layouts (e.g., flavors/orientations).Also applies to: 21-35
app/src/org/commcare/activities/UpdateActivity.java (1)
109-113: Guard Toolbar/action bar setup against nulls
findViewById(R.id.toolbar)andgetSupportActionBar()are both assumed non-null; any alternate layout for this activity without a@id/toolbar(or a failure to set the support action bar) will crash here. Wrap this block in null checks so the activity remains safe even if the toolbar is absent in some configurations:- Toolbar toolbar = findViewById(R.id.toolbar); - setSupportActionBar(toolbar); - getSupportActionBar().setDisplayHomeAsUpEnabled(true); + Toolbar toolbar = findViewById(R.id.toolbar); + if (toolbar != null) { + setSupportActionBar(toolbar); + if (getSupportActionBar() != null) { + getSupportActionBar().setDisplayHomeAsUpEnabled(true); + } + }⛔ Skipped due to learnings
Learnt from: OrangeAndGreen Repo: dimagi/commcare-android PR: 3121 File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364 Timestamp: 2025-05-22T14:28:35.959Z Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.app/res/navigation/nav_graph_connect_messaging.xml (1)
39-39: LGTM! Dynamic title handling moved to activity level.The empty label aligns with the dynamic title setting implemented in ConnectMessagingActivity's OnDestinationChangedListener (lines 61-73 of ConnectMessagingActivity.java), where the action bar title is now set at runtime based on the channel data.
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
70-70: LGTM! Toolbar integration follows Android best practices.The toolbar is properly initialized and set as the support action bar with the Up button enabled.
Also applies to: 75-77
app/src/org/commcare/navdrawer/BaseDrawerController.kt (2)
58-60: LGTM! Toolbar-based action bar setup with consistent white theming.The drawer controller properly integrates the toolbar as the action bar and applies white color to both the title text and navigation icon, aligning with the WhiteNavIcon theme introduced in this PR.
Also applies to: 65-65, 91-91
119-119: LGTM! Close button properly wired to drawer close action.The click listener correctly invokes the closeDrawer() method.
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (2)
30-30: LGTM! Toolbar properly initialized.The toolbar is correctly set as the support action bar following the standard Android pattern.
Also applies to: 36-37
51-74: No action required—the null handling is already correct.The navigation argument
channel_idis declared asapp:nullable="false"in the navigation graph and always provided throughshowConnectMessageFragment(). The null checks on lines 62-64 are defensive and will always pass. If the database lookup fails on line 66, not updating the title is correct behavior and preserves the previous fragment's title appropriately.app/res/layout/nav_drawer_header.xml (1)
25-67: LGTM! Profile section properly restructured.The CardView-based profile layout with horizontal arrangement of avatar and user info follows Material Design patterns and maintains all necessary view IDs for code compatibility.
app/res/layout/nav_drawer_header.xml
Outdated
| <ImageView | ||
| android:id="@+id/nav_drawer_close" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:padding="16dp" | ||
| android:orientation="horizontal"> | ||
| <ImageView | ||
| android:id="@+id/image_user_profile" | ||
| android:layout_width="64dp" | ||
| android:layout_height="wrap_content" | ||
| android:contentDescription="@null" | ||
| android:scaleType="centerCrop" | ||
| app:shapeAppearanceOverlay="@style/CircularImageView" | ||
| android:src="@drawable/nav_drawer_person_avatar"/> | ||
| android:src="@drawable/ic_connect_close" | ||
| app:tint="@color/white" | ||
| android:layout_gravity="start" | ||
| android:layout_marginBottom="16dp"/> |
There was a problem hiding this comment.
Add contentDescription for accessibility.
The close button ImageView is missing a contentDescription attribute, which is needed for screen readers.
Apply this diff:
<ImageView
android:id="@+id/nav_drawer_close"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:src="@drawable/ic_connect_close"
+ android:contentDescription="@string/close_navigation_drawer"
app:tint="@color/white"
android:layout_gravity="start"
android:layout_marginBottom="16dp"/>Note: You'll need to add the string resource close_navigation_drawer to your strings.xml file.
📝 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.
| <ImageView | |
| android:id="@+id/nav_drawer_close" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:padding="16dp" | |
| android:orientation="horizontal"> | |
| <ImageView | |
| android:id="@+id/image_user_profile" | |
| android:layout_width="64dp" | |
| android:layout_height="wrap_content" | |
| android:contentDescription="@null" | |
| android:scaleType="centerCrop" | |
| app:shapeAppearanceOverlay="@style/CircularImageView" | |
| android:src="@drawable/nav_drawer_person_avatar"/> | |
| android:src="@drawable/ic_connect_close" | |
| app:tint="@color/white" | |
| android:layout_gravity="start" | |
| android:layout_marginBottom="16dp"/> | |
| <ImageView | |
| android:id="@+id/nav_drawer_close" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:src="@drawable/ic_connect_close" | |
| android:contentDescription="@string/close_navigation_drawer" | |
| app:tint="@color/white" | |
| android:layout_gravity="start" | |
| android:layout_marginBottom="16dp"/> |
🤖 Prompt for AI Agents
In app/res/layout/nav_drawer_header.xml around lines 10 to 17, the ImageView
with id nav_drawer_close lacks a contentDescription which breaks accessibility;
add the attribute android:contentDescription="@string/close_navigation_drawer"
to the ImageView and then add a string resource named close_navigation_drawer to
your res/values/strings.xml (e.g., "Close navigation drawer") so screen readers
can describe the control.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Love this change! I just had a few minor comments
Also, I think it'd be worth it to address what Code Rabbit said too, including the nitpick comments (besides maybe the comments about null checks)
| android:background="?attr/colorPrimary" | ||
| app:titleTextColor="@android:color/white"/> |
There was a problem hiding this comment.
This does not need to change, but out of curiosity, why does the team directly use color resources in some places but use theme attributes in others?
Using theme attributes as much as possible makes it a lot easier to add/edit new app themes in the future (e.g. a dark mode).
There was a problem hiding this comment.
+1 for using theme attributes. I am not sure but I think there is not evidence of giving dark mode support as of now that can be reason, we are using color resources in some place
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
Outdated
Show resolved
Hide resolved
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Can we add before and after screenshots for all screen we are replacing the action bar with a tool bar on, the PR should also be labelled as QA note and specify testing notes. It should also specify a safety story confirming you have tested each of the screen and action bar interactions for all screens you are making this change on.
Also please try to separate out these changes in different commits or PRs if applicable, it's not alright to push all changes in a bulk single commit.
|
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Updating my review to non-blocking, but I think Shubham's suggestion should be addressed before merging this code in
|
@damagatchi retest this please |
|
@jaypanchal-13 Also, for activity if we can try to put this code in parent activity and not in individual activity screen. We need to check if toolbar present or not. This way we don't have to change code in individual activity every time but just include appbar_layout.xml in it's layout. |
|
I think that's should be Ok but defining AppBarLayout in each layout and having same duplicate code in each screen can be avoided. |
|
@jaypanchal-13 Also, in the side left navigation drawer, having cross X button on left with Commcare below looks somewhat odd, is that something designer / product has approved? |
|
Ok. I think we need to reduce the vertical gap between the X cross and CommCare logo which is in Figma design. |
|
@damagatchi retest this please |
Android 16 
Android 12 
|
|
@damagatchi retest this please |
jaypanchal-13
dismissed stale reviews from OrangeAndGreen, shubham1g5, conroy-ricketts, and Jignesh-dimagi
via
bcc9d54
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
There is comment pending but making my review non-blocking as I am offline next week.
jaypanchal-13
dismissed stale reviews from shubham1g5 and conroy-ricketts
via
958e0b4
|
@jaypanchal-13 This looks good now |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1905
https://dimagi.atlassian.net/browse/CCCT-1941
Screen Shot's->
Left side img -> before and right side img -> After
video ->
Screen_recording_20251205_192123.mp4
Screen_recording_20251208_155736.mp4
Video for on loading menu item should work->
Screen_recording_20251209_145737.mp4
Technical Summary
Feature Flag
Safety Assurance
Safety story
All screens updated in this PR were manually verified. Toolbar interactions such as navigation icon clicks, overflow menu actions, contextual actions, and title updates were tested. No crashes, double action bars, or unexpected layout shifts were observed. Each modified screen has been tested for its full workflow.
Automated test coverage
QA Plan
Labels and Review
QA Note: Pay Special attention to the menus and app bar contents and check that the toolbar state remains consistent on doing menu actions and returning back to the page, on coming out of the app and getting into the app, on rotating the device.