Fixes navigtation drawer to only be visible for required Screens #3292
Conversation
📝 WalkthroughWalkthroughThis change gates navigation drawer initialization behind a new opt-in method and login status. BaseDrawerActivity adds protected shouldShowDrawer() (default false) and only initializes the drawer in onCreate when shouldShowDrawer() is true and the user is logged in (via a new isPersonalIdLoggedIn()). setupDrawerController() is made private. CommCareSetupActivity, LoginActivity, and StandardHomeActivity override shouldShowDrawer() to return true. LoginActivity adds lazy initialization for PersonalIdManager via initPersonaIdManager(). Sequence Diagram(s)sequenceDiagram
participant Activity as BaseDrawerActivity (subclass)
participant PIM as PersonalIdManager
participant Drawer as DrawerController
Activity->>Activity: onCreate()
alt shouldShowDrawer() == true
Activity->>Activity: isPersonalIdLoggedIn()
Activity->>PIM: getInstance().init(context)
PIM-->>Activity: isLoggedIn()
alt logged in
Activity->>Activity: setupDrawerController() [private]
Activity->>Drawer: create(controller, clickHandler)
Drawer-->>Activity: ready
else not logged in
Activity-->>Activity: skip drawer setup
end
else shouldShowDrawer() == false
Activity-->>Activity: skip drawer setup
end
sequenceDiagram
participant Login as LoginActivity
participant PIM as PersonalIdManager
Login->>Login: onCreate()
Login->>Login: initPersonaIdManager()
alt personalIdManager == null
Login->>PIM: getInstance()
PIM-->>Login: instance
Login->>PIM: init(this)
else already initialized
Login-->>Login: no-op
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested reviewers
✨ 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 comments)
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: 2
🧹 Nitpick comments (2)
app/src/org/commcare/activities/LoginActivity.java (2)
152-152: Initialize PersonalIdManager early: good; minor naming nitCalling the initializer early in onCreate is correct. Consider renaming the helper to initPersonalIdManager for consistency.
Apply within this range:
- initPersonaIdManager(); + initPersonalIdManager();
178-183: Nit: fix method name typo for clarityRename to align with PersonalIdManager naming.
- private void initPersonaIdManager() { + private void initPersonalIdManager() { if (personalIdManager == null) { personalIdManager = PersonalIdManager.getInstance(); personalIdManager.init(this); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/org/commcare/activities/CommCareSetupActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivity.java(3 hunks)app/src/org/commcare/activities/StandardHomeActivity.java(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/src/org/commcare/activities/CommCareSetupActivity.java
🧬 Code Graph Analysis (3)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
shouldShowDrawer(27-29)
app/src/org/commcare/activities/StandardHomeActivity.java (1)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
shouldShowDrawer(27-29)
app/src/org/commcare/activities/LoginActivity.java (2)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-622)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
shouldShowDrawer(27-29)
🔇 Additional comments (5)
app/src/org/commcare/activities/StandardHomeActivity.java (1)
277-280: LGTM: Home screen enables drawerOpting in here is appropriate; users are logged in on home, so the drawer will be initialized by the base.
app/src/org/commcare/activities/LoginActivity.java (1)
1013-1016: LGTM: LoginActivity opts into drawer; base will still hide it pre-loginThe override aligns with the new gating. Since the base requires a logged-in state, the drawer won’t appear pre-login, which matches intent.
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (3)
21-26: Confirm init() idempotence and costisPersonalIdLoggedIn() calls init(this) each time. Ensure init() is idempotent and lightweight; with the suggested onResume check, this runs more than once per activity lifecycle.
Would you prefer a cheap “ensureInitialized()” in PersonalIdManager to make intent explicit?
27-30: LGTM: Opt-in API and default handlerDefault shouldShowDrawer() and no-op handler keep the base safe and extensible.
Also applies to: 43-44
31-41: Safe to privatize setupDrawerController()Ripgrep search for
setupDrawerController(returned only two hits in BaseDrawerActivity.kt (the private declaration at line 31 and its internal invocation at line 17). No subclasses or external callers exist. It’s safe to keep this method private.
| @Override | ||
| protected boolean shouldShowDrawer() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Drawer visibility opt-in added; ensure late initialization after login is handled
Overriding shouldShowDrawer() is correct. However, the base now initializes the drawer only in onCreate when isPersonalIdLoggedIn() is true. If a user signs in (e.g., via Connect) while this activity remains in the foreground, the drawer will not appear until the activity is recreated.
Recommend adding a late-init hook in BaseDrawerActivity (e.g., in onResume) to set up the drawer when login state changes, or an exposed method to “ensure” initialization.
If desired, I can provide a small base-class patch to initialize the drawer in onResume when shouldShowDrawer() and login are satisfied.
I’ll inspect both BaseDrawerActivity’s initialization logic and CommCareSetupActivity’s override to confirm the drawer gating behavior.
🏁 Script executed:
#!/bin/bash
# Find BaseDrawerActivity
BASE_FILE=$(rg -l "class BaseDrawerActivity")
echo "BaseDrawerActivity file: $BASE_FILE"
rg -n "onCreate" $BASE_FILE
rg -n "shouldShowDrawer" $BASE_FILE
rg -n "drawer" $BASE_FILE | grep -i init
rg -n "onResume" $BASE_FILE
# Find CommCareSetupActivity
SETUP_FILE=$(rg -l "class CommCareSetupActivity")
echo "CommCareSetupActivity file: $SETUP_FILE"
rg -n "extends BaseDrawerActivity" $SETUP_FILE
rg -n "shouldShowDrawer" $SETUP_FILE
# Check where login state is evaluated
rg -n "isPersonalIdLoggedIn" -n .Length of output: 2222
Ensure drawer appears after login in CommCareSetupActivity
BaseDrawerActivity (app/src/org/commcare/navdrawer/BaseDrawerActivity.kt:14-18) only sets up the nav-drawer in onCreate when both shouldShowDrawer() and isPersonalIdLoggedIn() are true.
CommCareSetupActivity (app/src/org/commcare/activities/CommCareSetupActivity.java:318-321) opts in to showing the drawer, but if the user logs in (via Connect) after onCreate, the drawer never initializes until the activity is recreated.
Recommend one of:
- In BaseDrawerActivity, override onResume() to run your existing drawer-init logic when shouldShowDrawer() && isPersonalIdLoggedIn() && drawer isn’t yet initialized.
- Or expose a protected ensureDrawerInitialized() method in BaseDrawerActivity and invoke it from CommCareSetupActivity immediately after login completes.
This will guarantee the drawer appears without forcing an activity restart.
| if (shouldShowDrawer() && isPersonalIdLoggedIn()) { | ||
| setupDrawerController() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drawer setup gated in onCreate only — add late-init on resume
The gating is correct, but initialization occurs only in onCreate. If login state flips to “logged-in” while an activity remains active (e.g., user signs in on Setup), the drawer won’t appear until recreation.
Add an onResume check to initialize when conditions become true and drawerController is null.
Example addition (outside this range):
override fun onResume() {
super.onResume()
if (drawerController == null && shouldShowDrawer() && isPersonalIdLoggedIn()) {
setupDrawerController()
}
}🤖 Prompt for AI Agents
In app/src/org/commcare/navdrawer/BaseDrawerActivity.kt around lines 16 to 19,
the drawer is only initialized in onCreate so if the user becomes logged-in
while the activity is still running the drawer won’t appear; add an onResume
override that checks if drawerController is null and shouldShowDrawer() &&
isPersonalIdLoggedIn() and calls setupDrawerController() so the drawer is lazily
initialized when login state flips while activity is active.
|
@damagatchi retest this please |
| } | ||
|
|
||
| private void initPersonaIdManager() { | ||
| if (personalIdManager == null) { |
There was a problem hiding this comment.
Nit: Seems like personalIdManager will always be null here since this method is called in onCreate.
There was a problem hiding this comment.
think I was afraid if someone call it from somewhere else we will end up re-initing it, so just more of a safe keeping but can remove if you prefer that.
Product Description
Fixes a bug in nav drawer implementation which was causing this to be visible for all screens.
Labels and Review