google / google/ground-android

Priority stability improvements

Open
#3,425 10 comments 0 reactions 1 assignee Claimed by @andreia-ferreira View on GitHub
type: code health
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.