Fix Nav Drawer Photo Launcher Registration Crash

[conroy-ricketts]

Technical Summary

This PR fixes a flaky BrowserStack CI test failure.

The nav drawer's photo-edit launcher was being registered during drawer setup, which can run after the activity reaches STARTED and violates the registerForActivityResult contract. It now registers once in BaseDrawerActivity.onCreate and is injected into BaseDrawerController, so rebuilding the controller no longer re-registers it.

In CI the illegal re-registration crashed the activity mid-test whenever a home-screen test triggered a UI rebuild after the activity had resumed. Because drawer visibility is gated on a persisted preference, whether a given test hit the crash depended on shard ordering, which is why it surfaced as a flaky cascade of Espresso failures across home-screen tests.

Safety Assurance

Safety story

What gives me confidence:

• I ran the change on a device and verified there are no regressions in the PersonalID profile photo update feature (open drawer, take/replace photo, failed-upload warning state).
• The diff is narrow and the registration now follows the documented Android contract (register before STARTED); I verified the lifecycle ordering so BaseDrawerActivity.onCreate registers the launcher before onCreateSessionSafe → setupUI ever builds a controller.

Risks to review:

• BaseDrawerActivity is the shared base for drawer-bearing activities, so the registration move affects all of them, not just the home screen.
• The original failure only reproduces on the BrowserStack Espresso suite (needs a device); I could not run that suite locally, so CI is the real confirmation that the cascade of home-screen test failures clears.

[conroy-ricketts]

Suggested Review Order

• app/src/org/commcare/navdrawer/BaseDrawerController.kt — shows the removed in-controller registerForActivityResult and the new injected launcher + handlePhotoResult.
• app/src/org/commcare/navdrawer/BaseDrawerActivity.kt — shows where registration moved to: once in onCreate (before STARTED), then passed into the controller.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors activity result handling for photo capture by moving launcher management from BaseDrawerController to BaseDrawerActivity. The activity now creates an ActivityResultLauncher in onCreate, registers it with ActivityResultContracts.StartActivityForResult, and passes it to the controller constructor. The controller no longer initializes its own launcher; instead, it receives the launcher as a constructor parameter and exposes a new handlePhotoResult(ActivityResult) method to process capture results and upload photos.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

skip-integration-tests

Suggested reviewers

• OrangeAndGreen
• Jignesh-dimagi

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: moving photo launcher registration to prevent crashes caused by registration after activity STARTED.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	PR description includes Technical Summary and Safety Assurance sections with good detail, but is missing Product Description, Automated Test Coverage details, and RELEASES.md update checkboxes from the template.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch failing-browserstack-test

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.55%. Comparing base (7fa6e40) to head (4bc6b31).
⚠️ Report is 27 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3758      +/-   ##
============================================
+ Coverage     25.23%   25.55%   +0.32%     
- Complexity     4332     4368      +36     
============================================
  Files           950      950              
  Lines         57645    57646       +1     
  Branches       6894     6894              
============================================
+ Hits          14544    14732     +188     
+ Misses        41286    41088     -198     
- Partials       1815     1826      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.