Priority stability improvements

[gino-m]

Generated by Gemini 3 Pro via Antigravity:

Android Codebase Stability Analysis

Executive Summary

This report analyzes the stability issues in the google/ground-android codebase, specifically focusing on app/src/main/java/org/groundplatform/android/. The analysis confirms that a significant portion of reported instability stems from improper Coroutine/Flow usage and Android Lifecycle mismanagement.

Critical Findings:

1. Dangerous Flow Collection: UI components collect flows effectively "forever" (until destruction) rather than pausing when the UI is stopped, leading to background crashes and battery drain.
2. View Memory Leaks: Fragments capture View references (via Binding and Coroutines) in long-lived scopes (Fragment.lifecycleScope), causing massive leaks when navigating.
3. Race Conditions: Map initialization relies on hardcoded invalid delays (delay(3000)), causing flakes.

Key Stability Issues

1. Unsafe Flow Collection in UI (Critical)

Location: MainActivity.kt, HomeScreenFragment.kt
Pattern: lifecycleScope.launch { flow.collect { ... } }

Collections launched directly in lifecycleScope do not stop when the Activity/Fragment is stopped (e.g., put in background). They only stop when destroyed. This causes code designed for the UI to run when the UI is not visible, leading to:

• Crashes: Trying to update Views that are detached or not laid out.
• State Inconsistency: Processing navigation events while in the background.
• Performance/Battery: Wasting resources processing updates that the user can't see.

Code Evidence (MainActivity.kt):

// BAD: Runs even when app is in background
lifecycleScope.launch {
  activityStreams.activityRequests.collect { callback: ActivityCallback ->
    callback(this@MainActivity)
  }
}

Recommended Fix:
Use repeatOnLifecycle(Lifecycle.State.STARTED) to ensure collection only happens when the UI is active.

// GOOD: Suspends when stopped, resumes when started
lifecycleScope.launch {
  repeatOnLifecycle(Lifecycle.State.STARTED) {
    activityStreams.activityRequests.collect { callback ->
      callback(this@MainActivity)
    }
  }
}

2. Fragment Scope vs. View Scope Mismatch (Critical Leak)

Location: HomeScreenFragment.kt
Pattern: using lifecycleScope instead of viewLifecycleOwner.lifecycleScope for View updates.

Fragment instances live longer than their Views (especially with Navigation Component). Using lifecycleScope (tied to Fragment instance) to update UI means:

1. Duplicate Subscribers: Every time the view is recreated (e.g., rotating, back navigation), a new coroutine is launched, but the old one is still running.
2. Memory Leaks: The coroutine holds a strict reference to this (Fragment), which holds binding, which holds View. The old View cannot be garbage collected.

Code Evidence (HomeScreenFragment.kt):

override fun onViewCreated(...) {
  // BAD: Tied to Fragment lifecycle, outlives the View
  lifecycleScope.launch { 
    homeScreenViewModel.surveyRepository.activeSurveyFlow.collect { ... } 
  }
}

Recommended Fix:
Always use viewLifecycleOwner.lifecycleScope in onViewCreated.

3. View Binding Memory Leaks

Location: HomeScreenFragment.kt (and likely others)
Pattern: lateinit var binding without onDestroyView cleanup.

When a Fragment is on the back stack, its View is destroyed, but the Fragment instance remains. Holding a strong reference to binding prevents the entire View hierarchy from being garbage collected. Combined with Issue #2, this is a major source of OOMs.

Code Evidence:

class HomeScreenFragment : AbstractFragment() {
  private lateinit var binding: HomeScreenFragBinding // Holds strong ref to View
  // Missing onDestroyView() to set binding = null
}

Recommended Fix:
Implement a AutoClearedValue delegate or manually null out binding in onDestroyView.

4. Race Conditions in Map Initialization

Location: BaseMapViewModel.kt
Pattern: delay(3000)

Using hardcoded delays to "wait" for animations or initialization is a classic source of flakey behavior. It fails on slower devices or under load.

Code Evidence:

// BAD: Guessing that 3 seconds is enough
if (index == 1) {
  delay(3000)
}
NewCameraPositionViaCoordinates(coordinates, shouldAnimate = true)

Recommended Fix:
Refactor to use a deterministic state stream that signals when the map is ready or animation is complete.

Proposed Action Plan

1. Global Search & Replace: Audit all Fragment classes.
   • Change lifecycleScope.launch -> viewLifecycleOwner.lifecycleScope.launch (for UI work).
   • Wrap flow collections in repeatOnLifecycle(Lifecycle.State.STARTED).
2. Fix Bindings: Add onDestroyView { _binding = null } snippet to all Fragments using ViewBinding.
3. Refactor MainActivity: Wrap activityStreams collection in repeatOnLifecycle.
4. Modernize Flows: Remove delay() hacks in ViewModels and replace with event-driven logic.

This remediation will directly address the "instability due to coroutines and Flows" cited by the team.

[gino-m]

@andreia-ferreira Can you please confirm the validity of this assessment? If valid, please let me know if you'd like me to queue up these fixes!

[andreia-ferreira]

@gino-m great starting point, thx! Some comments regarding the assessment:

• 1 and 2: are good immediate stability fixes and quick wins. They seem pretty straightforward and can have great impact on the performance and in addressing potential active crashes

• 3: while this is a good point, if we're moving towards KMP and refactoring the UI to Compose, then this may become less relevant over time, as legacy views are migrated.
  Instead, it may be more impactful to prioritize code health of the Compose components. There are currently some anti-patterns that can be a risk for stability:

  • Use of imperative patterns instead of declarative stateless components (eg. TaskButton and HomeScreenFragment.showSignOutConfirmationDialogs): this breaks the single source of truth principle and can lead to inconsistent states, unpredictable recompositions and memory leaks
  • Components managing their own state instead of hoisting it (eg. DataSharingTermsDialog; LoiJobSheet etc): makes them less reusable and testable, it may also cause issues with state persistence during recomposition and configuration changes

• 4: potentially less impact than the previous points but still worth addressing in order to reduce flakiness