CCCT-1720 App Sidebar Usability Improvement #3392
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added logic to open sidebar after successful Personal ID registration.
Optimized imports.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThis PR introduces a new public API to programmatically open the navigation drawer across three related classes. A new Sequence DiagramsequenceDiagram
participant User
participant PersonalIdManager
participant LoginActivity
participant BaseDrawerActivity
participant BaseDrawerController
participant DrawerLayout
User->>PersonalIdManager: Complete sign-in
PersonalIdManager->>PersonalIdManager: completeSignin()
PersonalIdManager->>LoginActivity: Cast parentActivity
PersonalIdManager->>BaseDrawerActivity: openDrawer()
BaseDrawerActivity->>BaseDrawerController: openDrawer()
BaseDrawerController->>DrawerLayout: openDrawer(GravityCompat.START)
DrawerLayout-->>User: Drawer displayed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/connect/PersonalIdManager.java(3 hunks)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt(2 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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-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/src/org/commcare/navdrawer/BaseDrawerActivity.ktapp/src/org/commcare/connect/PersonalIdManager.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/src/org/commcare/navdrawer/BaseDrawerActivity.ktapp/src/org/commcare/connect/PersonalIdManager.java
📚 Learning: 2025-06-20T15:51:14.157Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/models/connect/ConnectLoginJobListModel.java:79-92
Timestamp: 2025-06-20T15:51:14.157Z
Learning: The ConnectLoginJobListModel class in app/src/org/commcare/models/connect/ConnectLoginJobListModel.java does not need to implement Parcelable interface as it is not passed between Android activities or fragments.
Applied to files:
app/src/org/commcare/connect/PersonalIdManager.java
📚 Learning: 2025-06-06T20:15:21.134Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Applied to files:
app/src/org/commcare/connect/PersonalIdManager.java
🧬 Code graph analysis (1)
app/src/org/commcare/connect/PersonalIdManager.java (3)
app/src/org/commcare/activities/LoginActivity.java (1)
LoginActivity(89-1043)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
openDrawer(144-146)app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
openDrawer(253-255)
⏰ 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 (3)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
143-146: LGTM!The new
openDrawer()public API is well-implemented. The null-safe call operator ensures no issues if the drawer controller isn't initialized.app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
253-255: LGTM!The implementation correctly uses the Android drawer API and mirrors the existing
closeDrawer()pattern. Opening fromGravityCompat.STARTis appropriate for standard left-side navigation drawers.app/src/org/commcare/connect/PersonalIdManager.java (1)
199-205: Verify that opening the drawer on every sign-in completion is intended.The
completeSignin()method is invoked from multiple paths:
- Line 155: After successful biometric unlock via
unlockConnect()- Line 210: After returning from
PersonalIdActivitywith a successful resultThis means the drawer will open not only after initial Personal ID registration but also after every successful biometric unlock. Please confirm whether this broader behavior aligns with the PR objectives, which specifically mention "after a successful Personal ID registration."
Added instanceof check before using LoginActivity.
OrangeAndGreen
requested changes
Oct 31, 2025
Refactored the logic to check for the BaseDrawerActivity instead of the LoginActivity so that we cover more scenarios involving successful Personal ID signup.
shubham1g5
previously approved these changes
Oct 31, 2025
| } | ||
|
|
||
| fun openDrawer() { | ||
| drawerController?.openDrawer() |
Contributor
There was a problem hiding this comment.
can we soft assert using Logger.exception that drawerController is non null here. Just so that if we start calling this before drawerController is initialized, we should get to know about it from logs.
Contributor
Author
There was a problem hiding this comment.
Oh yeah for sure, I'll add this to closeDrawer() too
Contributor
Author
There was a problem hiding this comment.
@shubham1g5 Let me know if I can improve the wording of the logs
Added some logging to catch potential future errors.
OrangeAndGreen
approved these changes
Oct 31, 2025
shubham1g5
approved these changes
Nov 1, 2025
shubham1g5
deleted the
task/CCCT-1720-app-sidebar-usability-improvement
branch
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CCCT-1720
Product Description
I added some logic to open the app's sidebar after a successful Personal ID registration.
Screen_Recording_20251031_141544_CommCare.Debug.mp4
Technical Summary
This one was pretty straightforward.
Safety Assurance
Safety story
I verified that the app's sidebar automatically opens after successful Personal ID registration (for both recovery and a brand new user).
QA Plan
Verify that the app's sidebar automatically opens after successful Personal ID registration. We should verify this for both account recovery and brand new user registration.