google / google/ground-android
Priority stability improvements
- Dominant language
- Kotlin
- Stars
- 293
- Forks
- 149
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 42
Description
Generated by Gemini 3 Pro via Antigravity:
# Android Codebase Stability Analysis
## Executive Summary
This report analyzes the stability issues in the [google/ground-android](file:///Users/gmiceli/Git/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`):**
```kotlin
// 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.
```kotlin
// 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`):**
```kotlin
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:**
```kotlin
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:**
```kotlin
// 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.
Contributor guide
Assessment
This issue has not been assessed yet.