Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 48.48%. Comparing base (f9082f0) to head (dd1418a).
⚠️ Report is 3 commits behind head on develop.

@@             Coverage Diff             @@
##           develop    #4545      +/-   ##
===========================================
+ Coverage    48.46%   48.48%   +0.02%     
===========================================
  Files          575      575              
  Lines        19803    19805       +2     
  Branches      3313     3313              
===========================================
+ Hits          9598     9603       +5     
+ Misses        9190     9187       -3     
  Partials      1015     1015              

@claude review

PR Review: Refactor App Lock Check to Non-Blocking Flow

Summary

This PR successfully removes the blocking runBlocking call from isAppLockPasscodeSet() and refactors it to use a reactive Flow-based approach. The change shows significant performance improvements in startup benchmarks (~26% faster median TTID).

✅ Positive Aspects

Performance Improvements

• Excellent benchmark data: The PR includes comprehensive before/after metrics showing substantial improvements:
  • Median TTID: 26.4% faster (1.36s saved)
  • Frame rendering quality: up to 63.7% smoother at P95
  • Significantly reduced frame overruns
• Eliminates blocking I/O on main thread: Removes runBlocking which was likely causing the startup delays

Code Quality

• Proper reactive pattern: Uses Flow collection with state updates instead of blocking calls
• Good test coverage: Tests were updated to reflect the new Flow-based API
• Minimal scope: Focused change that only touches what's necessary

⚠️ Issues & Concerns

1. CRITICAL: Potential Memory Leak in ViewModel

Location: FeatureFlagNotificationViewModel.kt:279-281

private suspend fun isAppLockSet() = globalDataStore.get().isAppLockPasscodeSetFlow().collect { isSet ->
    featureFlagState = featureFlagState.copy(isUserAppLockSet = isSet)
}

Problem: The collect call is a suspending infinite loop that never completes. This function is launched in init block and will continue collecting forever, even if the ViewModel is cleared.

Impact:

• Memory leak: The coroutine continues running after ViewModel destruction
• Resource waste: Unnecessary Flow collection after component is disposed

Recommended Fix:

private fun observeAppLockSet() {
    viewModelScope.launch {
        globalDataStore.get().isAppLockPasscodeSetFlow().collect { isSet ->
            featureFlagState = featureFlagState.copy(isUserAppLockSet = isSet)
        }
    }
}

init {
    viewModelScope.launch { initialSync() }
    observeAppLockSet()  // Launch as separate coroutine
}

Or better yet, integrate it into observeStatesAfterInitialSync:

private suspend fun observeStatesAfterInitialSync(userId: UserId) {
    coroutineScope {
        launch { setFileSharingState(userId) }
        launch { observeTeamSettingsSelfDeletionStatus(userId) }
        launch { setGuestRoomLinkFeatureFlag(userId) }
        launch { setE2EIRequiredState(userId) }
        launch { setTeamAppLockFeatureFlag(userId) }
        launch { observeCallEndedBecauseOfConversationDegraded(userId) }
        launch { observeShouldNotifyForRevokedCertificate(userId) }
        launch { observeAppLockSet() }  // Add here
    }
}

2. Race Condition Risk

Location: FeatureFlagNotificationViewModel.kt:161 and WireActivity.kt:461

The app lock status is now checked from state (featureFlagState.isUserAppLockSet) but the Flow collection is launched independently in init. There's a timing issue:

1. init block launches initialSync()
2. init block calls isAppLockSet() which starts collecting
3. setTeamAppLockFeatureFlag() checks featureFlagState.isUserAppLockSet at line 161

Problem: The state might not be initialized yet when setTeamAppLockFeatureFlag() first executes, leading to incorrect shouldBlockApp calculation.

Impact: User might see the app lock dialog unnecessarily on first app launch.

Recommended Fix: Ensure app lock state is initialized before calling observeStatesAfterInitialSync, or add initial value:

init {
    viewModelScope.launch {
        // Get initial value first
        val isSet = globalDataStore.get().isAppLockPasscodeSetFlow().first()
        featureFlagState = featureFlagState.copy(isUserAppLockSet = isSet)
        
        // Then start observing
        initialSync()
        launch {
            globalDataStore.get().isAppLockPasscodeSetFlow().collect { isSet ->
                featureFlagState = featureFlagState.copy(isUserAppLockSet = isSet)
            }
        }
    }
}

3. Leftover runBlocking Import

Location: GlobalDataStore.kt:38

import kotlinx.coroutines.runBlocking

This import is now unused after removing the isAppLockPasscodeSet() function and should be removed for cleanliness.

4. Missing Function Removal in GlobalDataStore

Location: GlobalDataStore.kt:164-168

The PR diff shows that isAppLockPasscodeSet() blocking function was removed, but I notice it might still exist in the current file at lines 164-168. Please verify this function is completely removed:

fun isAppLockPasscodeSet(): Boolean = runBlocking {
    context.dataStore.data.map {
        it.contains(APP_LOCK_PASSCODE)
    }.first()
}

This function should not exist in the final version.

5. Test Coverage Gap

Location: FeatureFlagNotificationViewModelTest.kt

The test was updated to mock isAppLockPasscodeSetFlow() as a Flow, but there's no test verifying:

1. The state is properly initialized at startup
2. The state updates when the Flow emits new values
3. The race condition scenario (team app lock check before state initialization)

Recommended: Add a test like:

@Test
fun givenAppLockFlow_whenValueChanges_thenStateIsUpdated() = runTest {
    val appLockFlow = MutableStateFlow(false)
    val (_, viewModel) = Arrangement()
        .withCurrentSessionsFlow(flowOf(CurrentSessionResult.Success(AccountInfo.Valid(TestUser.USER_ID))))
        .withIsAppLockSetup(appLockFlow)
        .arrange()
    
    advanceUntilIdle()
    assertEquals(false, viewModel.featureFlagState.isUserAppLockSet)
    
    appLockFlow.emit(true)
    advanceUntilIdle()
    assertEquals(true, viewModel.featureFlagState.isUserAppLockSet)
}

🔍 Minor Observations

Import Ordering

Location: WireActivity.kt:93-97

The import order was changed (moved LogManagementScreenDestination up). While functionally fine, this seems unintentional. Consider keeping imports alphabetically sorted or IDE-organized.

Documentation

The PR description and benchmark data are excellent. Consider adding a comment in the code explaining why the Flow-based approach was chosen (performance).

🎯 Recommendations

Must Fix (Blocking)

1. ⚠️ Fix the infinite collect call in isAppLockSet() to prevent memory leak
2. ⚠️ Ensure app lock state initialization happens before it's first accessed
3. 🧹 Remove unused runBlocking import

Should Fix

4. Add test coverage for state initialization and updates
5. Verify isAppLockPasscodeSet() blocking function is fully removed

Nice to Have

6. Fix import ordering in WireActivity.kt
7. Add code comment explaining the performance motivation

📊 Overall Assessment

Code Quality: Good ⭐⭐⭐⭐☆
Test Coverage: Adequate ⭐⭐⭐☆☆
Performance Impact: Excellent ⭐⭐⭐⭐⭐
Security: No concerns ✅

The performance improvements are impressive and well-documented. The refactoring follows Android/Kotlin best practices by eliminating blocking calls. However, the implementation has a critical memory leak issue that must be addressed before merge.

Review completed by Claude Code

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

latest results comparisson