Side nav drawer in App Home page #3299
Conversation
…ng if user hasn't already unlocked to login to app. Logging out of app when user selects a new app from side navigation menu, and going to Login for the selected app. Moved a couple constants to decouple activities.
…n menu. Added activity-level control for when to highlight the seated app. Removed some unused imports.
📝 WalkthroughTip 🔌 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: 1
🧹 Nitpick comments (6)
app/res/layout/nav_drawer_sublist_item.xml (1)
12-19: Align initial visibility with adapter logic and mark decorative icon as not important for accessibility.The adapter toggles the highlight between VISIBLE and INVISIBLE. Keeping the initial state as GONE can cause one-time layout shifts on first bind. Also, declare the icon as decorative for a11y.
Apply:
- android:visibility="gone"/> + android:visibility="invisible" + android:importantForAccessibility="no"/>app/src/org/commcare/navdrawer/NavDrawerAdapter.kt (2)
110-121: Highlight visibility handling is correct; ensure XML default matches INVISIBLE.Reserving space via INVISIBLE is the right call to keep titles aligned. Make sure the layout’s sublist_highlight_icon defaults to INVISIBLE (instead of GONE) to avoid first-bind jumps. See my layout comment for a small XML tweak.
127-131: Consider DiffUtil/ListAdapter to avoid full refreshes.notifyDataSetChanged() will redraw the entire drawer, which can cause unnecessary jank. Optional: adopt ListAdapter with DiffUtil to compute minimal updates when expanding/collapsing parents or toggling highlights.
app/src/org/commcare/activities/LoginActivity.java (1)
1024-1033: Minor UX tweak: close the drawer before starting SeatAppActivity.Closing the drawer immediately provides a snappier transition and avoids any transient overlap while the activity starts.
- seatAppIfNeeded(recordId); - closeDrawer(); + closeDrawer(); + seatAppIfNeeded(recordId);app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
119-121: Drawer visibility gating and sign-in UI states: confirm intent vs. implementation
- The drawer content currently appears only when PersonalIdManager.getInstance().isloggedIn() is true. The PR objectives mention “when PersonalID is configured” (not necessarily signed in). If configuration and login are distinct states, this might not match product behavior.
- setSignedInState toggles signoutView to VISIBLE when not signed in, which reads inverted. Also, signInText is never hidden when signed in.
Please confirm:
- Whether the gating should be “configured” rather than “logged in”.
- Whether signoutView is actually a “sign-in” affordance (naming mismatch), or the visibility should be inverted.
- Whether signInText should be hidden when signed in.
If needed, adjust visibility handling:
// Inside setSignedInState(isSignedIn) binding.signInText.visibility = if (isSignedIn) View.GONE else View.VISIBLE binding.signoutView.visibility = if (isSignedIn) View.VISIBLE else View.GONE // if this is truly a sign-out viewAlso applies to: 182-186
app/src/org/commcare/activities/DispatchActivity.java (1)
500-509: Double-check RESULT_OK handling from HOME_SCREEN sets userTriggeredLogout intentionallyWhen returning RESULT_OK from StandardHomeActivity after selecting another app, userTriggeredLogout is set to true. If this flag controls “user chose to log out” UX (toasts, telemetry, etc.), confirm this is desired for the “switch app” flow as well. If not, consider a separate reason code or flag to avoid conflating intents.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/res/layout/nav_drawer_sublist_item.xml(1 hunks)app/src/org/commcare/activities/CommCareActivity.java(0 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(0 hunks)app/src/org/commcare/activities/DispatchActivity.java(7 hunks)app/src/org/commcare/activities/LoginActivity.java(2 hunks)app/src/org/commcare/activities/StandardHomeActivity.java(4 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt(4 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(3 hunks)app/src/org/commcare/navdrawer/NavDrawerAdapter.kt(1 hunks)app/src/org/commcare/navdrawer/NavDrawerParentItem.kt(1 hunks)
💤 Files with no reviewable changes (2)
- app/src/org/commcare/activities/CommCareSetupActivity.java
- app/src/org/commcare/activities/CommCareActivity.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-21T18:48:08.330Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.
Applied to files:
app/src/org/commcare/connect/ConnectConstants.java
📚 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/StandardHomeActivity.javaapp/src/org/commcare/activities/DispatchActivity.java
🧬 Code Graph Analysis (3)
app/src/org/commcare/activities/StandardHomeActivity.java (5)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-48)commcare-support-library/src/main/java/org/commcare/commcaresupportlibrary/CommCareLauncher.java (1)
CommCareLauncher(11-45)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (5)
handleDrawerItemClick(43-51)closeDrawer(81-83)navigateToConnectMenu(61-69)navigateToMessaging(71-79)shouldHighlightSeatedApp(26-28)app/src/org/commcare/navdrawer/BaseDrawerController.kt (2)
OPPORTUNITIES(36-42)closeDrawer(188-190)app/src/org/commcare/connect/ConnectNavHelper.kt (2)
goToConnectJobsList(45-48)goToMessaging(27-30)
app/src/org/commcare/activities/LoginActivity.java (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-48)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (3)
shouldHighlightSeatedApp(26-28)handleDrawerItemClick(43-51)closeDrawer(81-83)app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
closeDrawer(188-190)
app/src/org/commcare/activities/DispatchActivity.java (2)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-48)app/src/org/commcare/activities/LoginActivity.java (1)
LoginActivity(93-1038)
🔇 Additional comments (18)
app/src/org/commcare/connect/ConnectConstants.java (1)
15-16: No stale definitions/usages found — constants are centralizedRan the repo-wide search you suggested. The only occurrences are the two definitions in ConnectConstants; no other definitions, raw string usages, or LoginActivity references were found.
- app/src/org/commcare/connect/ConnectConstants.java — lines 15–16:
- PERSONALID_MANAGED_LOGIN = "personalid-managed-login"
- CONNECT_MANAGED_LOGIN = "connect-managed-login"
app/res/layout/nav_drawer_sublist_item.xml (1)
6-6: Padding reduction is consistent with introducing a leading highlight glyph.Changing start padding from 48dp to 24dp makes sense given the new 24dp highlight icon before the existing icon; net left offset remains consistent.
app/src/org/commcare/navdrawer/NavDrawerParentItem.kt (1)
15-20: Adding isHighlighted is fine; double-check equality semantics of ChildItem.Making ChildItem a data class means equals/hashCode now include isHighlighted. If any code treats ChildItem identity as (childTitle, recordId, parentType), toggling highlight will change identity semantics and may affect set membership, caching, or DiffUtil behavior.
If identity should ignore UI state, consider one of:
- Switch to a regular class with custom equals/hashCode that exclude isHighlighted; or
- Keep data class, but ensure any DiffUtil or collections use a stable ID separate from equality.
app/src/org/commcare/activities/LoginActivity.java (3)
5-6: Good: use shared ConnectConstants for intent extras.Replacing local keys with static imports keeps intent contract centralized and reduces drift.
1015-1018: Correct: always highlight seated app on Login.This matches the PR objective of showing the seated app selection on Login.
537-546: Confirmed: constants centralized and no raw-string usages remainConnectConstants defines the keys at app/src/org/commcare/connect/ConnectConstants.java:15–16. A repo-wide search found no other definitions, no raw string occurrences of "personalid-managed-login" or "connect-managed-login", and no usages of LoginActivity.PERSONALID_MANAGED_LOGIN / CONNECT_MANAGED_LOGIN.
No changes required here — resolved.
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
28-29: Constructor change verified — no remaining old-arity callsites foundRepo-wide search for "BaseDrawerController(" shows only the class declaration and the single instantiation in BaseDrawerActivity, which already supplies shouldHighlightSeatedApp().
- app/src/org/commcare/navdrawer/BaseDrawerController.kt — class declaration (around line 25)
- app/src/org/commcare/navdrawer/BaseDrawerActivity.kt — instantiation (around lines 33–36) passing shouldHighlightSeatedApp()
app/src/org/commcare/activities/StandardHomeActivity.java (3)
63-65: Wiring PERSONALID_MANAGED_LOGIN looks goodReading the flag at session creation time is correct and aligns with the navigation logic further below.
318-321: Enabling seated-app highlighting: LGTMOverriding shouldHighlightSeatedApp() to true for StandardHomeActivity aligns with the PR goals to highlight the seated app.
249-282: Use of Java “switch arrow” may break Android builds; prefer classic switch-caseAndroid projects commonly target language levels where “switch (x) { case A -> { … } }” isn’t supported by the toolchain/desugaring across all AGP/JDK combos. To avoid build/runtime compatibility issues, switch to classic switch-case with breaks.
Apply:
- protected void handleDrawerItemClick(@NonNull BaseDrawerController.NavItemType itemType, String recordId) { - switch (itemType) { - case COMMCARE_APPS -> { - if(recordId != null) { - String currentSeatedId = CommCareApplication.instance().getCurrentApp().getUniqueId(); - if(!recordId.equals(currentSeatedId)) { - //Navigate to LoginActivity for selected app - CommCareApplication.instance().closeUserSession(); - Intent i = new Intent(); - i.putExtra(CommCareLauncher.SESSION_ENDPOINT_APP_ID, recordId); - setResult(RESULT_OK, i); - finish(); - } - } - } - case OPPORTUNITIES -> { - if(personalIdManagedLogin) { - ConnectNavHelper.INSTANCE.goToConnectJobsList(this); - closeDrawer(); - } else { - navigateToConnectMenu(); - } - } - case MESSAGING -> { - if(personalIdManagedLogin) { - ConnectNavHelper.INSTANCE.goToMessaging(this); - closeDrawer(); - } else { - navigateToMessaging(); - } - } - } - } + protected void handleDrawerItemClick(@NonNull BaseDrawerController.NavItemType itemType, String recordId) { + switch (itemType) { + case COMMCARE_APPS: { + if (recordId != null) { + String currentSeatedId = CommCareApplication.instance().getCurrentApp().getUniqueId(); + if (!recordId.equals(currentSeatedId)) { + // Navigate to LoginActivity for selected app + CommCareApplication.instance().closeUserSession(); + Intent i = new Intent(); + i.putExtra(CommCareLauncher.SESSION_ENDPOINT_APP_ID, recordId); + setResult(RESULT_OK, i); + finish(); + } + } + break; + } + case OPPORTUNITIES: { + if (personalIdManagedLogin) { + ConnectNavHelper.INSTANCE.goToConnectJobsList(this); + closeDrawer(); + } else { + navigateToConnectMenu(); + } + break; + } + case MESSAGING: { + if (personalIdManagedLogin) { + ConnectNavHelper.INSTANCE.goToMessaging(this); + closeDrawer(); + } else { + navigateToMessaging(); + } + break; + } + default: + break; + } + }⛔ Skipped due to learnings
Learnt from: OrangeAndGreen PR: dimagi/commcare-android#3108 File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:313-320 Timestamp: 2025-06-06T20:41:41.740Z Learning: The dimagi/commcare-android project uses Java 17, which supports modern Java syntax features including comma-separated case labels in switch statements.app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (3)
26-29: Extensibility hook for highlighting: LGTMProviding a protected open shouldHighlightSeatedApp() with a default false is a clean extension point. Callers can opt-in (as StandardHomeActivity does).
35-37: Plumbing highlight flag through to the controller: LGTMPassing shouldHighlightSeatedApp() into BaseDrawerController keeps the controller stateless with respect to Activity type.
61-83: Protected navigation helpers: LGTMChanging navigateToConnectMenu, navigateToMessaging, and closeDrawer to protected enables subclasses to tailor behavior and is consistent with StandardHomeActivity’s override-driven logic.
app/src/org/commcare/activities/DispatchActivity.java (5)
90-91: Transport for cross-app login redirection: LGTMredirectToLoginAppId is a clear addition to carry app-id across flows; good naming and scope.
302-309: Robust app-id selection for LoginActivity: LGTMFallback from SESSION_ENDPOINT_APP_ID to redirectToLoginAppId and passing via LoginActivity.EXTRA_APP_ID looks correct. Clearing redirectToLoginAppId after consumption prevents stale state.
345-346: Passing PERSONALID_MANAGED_LOGIN to home: LGTMThis propagates the login mode so UI can skip further unlock prompts where appropriate.
463-467: Capturing redirectToLoginAppId from results: LGTMThis enables the “select another app” flow from App Home to feed back into login correctly.
491-496: Using ConnectConstants for managed-login flags: LGTMSwitching to centralized constants reduces coupling to LoginActivity and improves consistency.
| val seatedApp = if(highlightSeatedApp) | ||
| CommCareApplication.instance().currentApp.uniqueId else null | ||
|
|
||
| val commcareApps = MultipleAppsUtil.getUsableAppRecords().map { | ||
| NavDrawerItem.ChildItem(it.displayName, it.uniqueId, NavItemType.COMMCARE_APPS) | ||
| NavDrawerItem.ChildItem(it.displayName, it.uniqueId, NavItemType.COMMCARE_APPS, | ||
| it.uniqueId == seatedApp) | ||
| } |
There was a problem hiding this comment.
Possible NPE when resolving current app; make lookup null-safe
CommCareApplication.instance().currentApp can be null in edge cases (e.g., before an app is seated). Use a null-safe access to avoid crashes and keep highlighting off when no app is seated.
Apply:
- val seatedApp = if(highlightSeatedApp)
- CommCareApplication.instance().currentApp.uniqueId else null
+ val seatedApp = if (highlightSeatedApp) {
+ CommCareApplication.instance().currentApp?.uniqueId
+ } else {
+ null
+ }📝 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.
| val seatedApp = if(highlightSeatedApp) | |
| CommCareApplication.instance().currentApp.uniqueId else null | |
| val commcareApps = MultipleAppsUtil.getUsableAppRecords().map { | |
| NavDrawerItem.ChildItem(it.displayName, it.uniqueId, NavItemType.COMMCARE_APPS) | |
| NavDrawerItem.ChildItem(it.displayName, it.uniqueId, NavItemType.COMMCARE_APPS, | |
| it.uniqueId == seatedApp) | |
| } | |
| val seatedApp = if (highlightSeatedApp) { | |
| CommCareApplication.instance().currentApp?.uniqueId | |
| } else { | |
| null | |
| } | |
| val commcareApps = MultipleAppsUtil.getUsableAppRecords().map { | |
| NavDrawerItem.ChildItem(it.displayName, it.uniqueId, NavItemType.COMMCARE_APPS, | |
| it.uniqueId == seatedApp) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/navdrawer/BaseDrawerController.kt around lines 132 to
138, computing seatedApp uses CommCareApplication.instance().currentApp which
can be null; change the lookup to a null-safe access (e.g.,
CommCareApplication.instance().currentApp?.uniqueId) so seatedApp becomes null
when no app is seated, and ensure the highlight condition uses that nullable
value (keep highlighting off when seatedApp is null) to avoid an NPE.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Left a nit but looks good in general
| //Navigate to LoginActivity for selected app | ||
| CommCareApplication.instance().closeUserSession(); | ||
| Intent i = new Intent(); | ||
| i.putExtra(CommCareLauncher.SESSION_ENDPOINT_APP_ID, recordId); |
There was a problem hiding this comment.
It seems a bit confusing to me that we are using 2 different constants in different places to denote the app id - LoginActivity.EXTRA_APP_ID and CommCareLauncher.SESSION_ENDPOINT_APP_ID. I would prefer we can instead use LoginActivity.EXTRA_APP_ID itself here and not tie it to SESSION_ENDPOINT_APP_ID
…ough dispatch to login.
https://dimagi.atlassian.net/browse/CCCT-1614
Product Description
Showing the side navigation drawer in the App Home page when PersonalID is configured on the device.
Seated app highlighted in side nav menu on Login and App Home pages.
Navigating to Opportunities/Messages from App Home only requires unlock if it wasn't performed to access the current app.
Feature Flag
PersonalID, Side Navigation Drawer
Safety Assurance
Safety story
Dev tested
Automated test coverage
None
QA Plan