refactor: use immutable destNum in RadioConfigViewModel (#5436)

feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt

@@ -20,12 +20,14 @@ import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.remember
+import androidx.lifecycle.SavedStateHandle
 import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import androidx.lifecycle.compose.dropUnlessResumed
 import androidx.navigation3.runtime.EntryProviderScope
 import androidx.navigation3.runtime.NavBackStack
 import androidx.navigation3.runtime.NavKey
 import org.koin.compose.viewmodel.koinViewModel
+import org.koin.core.parameter.parametersOf
 import org.meshtastic.core.navigation.NodesRoute
 import org.meshtastic.core.navigation.Route
 import org.meshtastic.core.navigation.SettingsRoute
@@ -71,7 +73,6 @@ import kotlin.reflect.KClass
 
 @Composable
 fun getRadioConfigViewModel(backStack: NavBackStack<NavKey>): RadioConfigViewModel {
-    val viewModel = koinViewModel<RadioConfigViewModel>()
     val destNum =
         remember(backStack.toList()) {
             backStack.lastOrNull { it is SettingsRoute.Settings }?.let { (it as SettingsRoute.Settings).destNum }
@@ -79,8 +80,9 @@ fun getRadioConfigViewModel(backStack: NavBackStack<NavKey>): RadioConfigViewMod
                     .lastOrNull { it is SettingsRoute.SettingsGraph }
                     ?.let { (it as SettingsRoute.SettingsGraph).destNum }
         }
-    LaunchedEffect(destNum) { viewModel.initDestNum(destNum) }
-    return viewModel
+    return koinViewModel<RadioConfigViewModel>(key = destNum?.toString()) {
+        parametersOf(SavedStateHandle(mapOf("destNum" to destNum)))
+    }
 }
 
 @Suppress("LongMethod", "CyclomaticComplexMethod")

feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt

@@ -27,7 +27,10 @@ import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.flow.distinctUntilChanged
+import kotlinx.coroutines.flow.flatMapLatest
+import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.flow.launchIn
+import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.flow.update
 import kotlinx.coroutines.launch
@@ -181,13 +184,7 @@ open class RadioConfigViewModel(
         _mqttProbeStatus.value = null
     }
 
-    private val destNumFlow = MutableStateFlow(savedStateHandle.get<Int>("destNum"))
-
-    fun initDestNum(id: Int?) {
-        if (destNumFlow.value != id) {
-            destNumFlow.value = id
-        }
-    }
+    private val destNum: Int? = savedStateHandle.get<Int>("destNum")
 
     private val _destNode = MutableStateFlow<Node?>(null)
     val destNode: StateFlow<Node?>
@@ -209,7 +206,8 @@ open class RadioConfigViewModel(
         locationService.getCurrentLocation()
 
     init {
-        combine(destNumFlow, nodeRepository.nodeDBbyNum) { id, nodes -> nodes[id] ?: nodes.values.firstOrNull() }
+        nodeRepository.nodeDBbyNum
+            .map { nodes -> if (destNum != null) nodes[destNum] else nodes.values.firstOrNull() }
             .distinctUntilChanged()
             .onEach {
                 _destNode.value = it
@@ -219,19 +217,40 @@ open class RadioConfigViewModel(
 
         radioConfigRepository.deviceProfileFlow.onEach { _currentDeviceProfile.value = it }.launchIn(viewModelScope)
 
-        radioConfigRepository.localConfigFlow
-            .onEach { lc -> if (radioConfigState.value.isLocal) _radioConfigState.update { it.copy(radioConfig = lc) } }
-            .launchIn(viewModelScope)
-
-        radioConfigRepository.channelSetFlow
-            .onEach { cs ->
-                if (radioConfigState.value.isLocal) _radioConfigState.update { it.copy(channelList = cs.settings) }
+        // Derive isLocal from the immutable destNum and the (possibly changing) myNodeInfo.
+        // flatMapLatest cancels the previous inner flow on every change, so there is
+        // no window where stale local config can leak through.
+        nodeRepository.myNodeInfo
+            .map { ni ->
+                val isLocal = (destNum == null) || (destNum == ni?.myNodeNum)
+                isLocal
             }
-            .launchIn(viewModelScope)
-
-        radioConfigRepository.moduleConfigFlow
-            .onEach { lmc ->
-                if (radioConfigState.value.isLocal) _radioConfigState.update { it.copy(moduleConfig = lmc) }
+            .distinctUntilChanged()
+            .flatMapLatest { isLocal ->
+                if (isLocal) {
+                    combine(
+                        radioConfigRepository.channelSetFlow,
+                        radioConfigRepository.localConfigFlow,
+                        radioConfigRepository.moduleConfigFlow,
+                    ) { cs, lc, mc ->
+                        _radioConfigState.update {
+                            it.copy(isLocal = true, channelList = cs.settings, radioConfig = lc, moduleConfig = mc)
+                        }
+                    }
+                } else {
+                    // Remote admin: clear local config once, then stay idle.
+                    // Remote responses arrive via processPacketResponse.
+                    flowOf(Unit).onEach {
+                        _radioConfigState.update {
+                            it.copy(
+                                isLocal = false,
+                                channelList = emptyList(),
+                                radioConfig = LocalConfig(),
+                                moduleConfig = LocalModuleConfig(),
+                            )
+                        }
+                    }
+                }
             }
             .launchIn(viewModelScope)
 
@@ -250,11 +269,6 @@ open class RadioConfigViewModel(
         }
             .launchIn(viewModelScope)
 
-        combine(nodeRepository.myNodeInfo, destNumFlow) { ni, id ->
-            _radioConfigState.update { it.copy(isLocal = (id == null) || (id == ni?.myNodeNum)) }
-        }
-            .launchIn(viewModelScope)
-
         Logger.d { "RadioConfigViewModel created" }
     }
 
@@ -284,7 +298,7 @@ open class RadioConfigViewModel(
     }
 
     fun setOwner(user: User) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "setOwner") {
             _radioConfigState.update { it.copy(userConfig = user) }
             val packetId = radioConfigUseCase.setOwner(destNum, user)
@@ -293,7 +307,7 @@ open class RadioConfigViewModel(
     }
 
     fun updateChannels(new: List<ChannelSettings>, old: List<ChannelSettings>) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         getChannelList(new, old).forEach { channel ->
             safeLaunch(tag = "setRemoteChannel") {
                 val packetId = radioConfigUseCase.setRemoteChannel(destNum, channel)
@@ -311,7 +325,7 @@ open class RadioConfigViewModel(
     }
 
     fun setConfig(config: Config) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "setConfig") {
             _radioConfigState.update { state ->
                 state.copy(
@@ -335,7 +349,7 @@ open class RadioConfigViewModel(
 
     @Suppress("CyclomaticComplexMethod")
     fun setModuleConfig(config: ModuleConfig) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "setModuleConfig") {
             _radioConfigState.update { state ->
                 state.copy(
@@ -367,13 +381,13 @@ open class RadioConfigViewModel(
     }
 
     fun setRingtone(ringtone: String) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         _radioConfigState.update { it.copy(ringtone = ringtone) }
         safeLaunch(tag = "setRingtone") { radioConfigUseCase.setRingtone(destNum, ringtone) }
     }
 
     fun setCannedMessages(messages: String) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         _radioConfigState.update { it.copy(cannedMessageMessages = messages) }
         safeLaunch(tag = "setCannedMessages") { radioConfigUseCase.setCannedMessages(destNum, messages) }
     }
@@ -420,12 +434,12 @@ open class RadioConfigViewModel(
     }
 
     fun setFixedPosition(position: Position) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "setFixedPosition") { radioConfigUseCase.setFixedPosition(destNum, position) }
     }
 
     fun removeFixedPosition() {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "removeFixedPosition") { radioConfigUseCase.removeFixedPosition(destNum) }
     }
 
@@ -456,7 +470,7 @@ open class RadioConfigViewModel(
     }
 
     fun installProfile(protobuf: DeviceProfile) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         safeLaunch(tag = "installProfile") { installProfileUseCase(destNum, protobuf, destNode.value?.user) }
     }
 
@@ -466,7 +480,7 @@ open class RadioConfigViewModel(
     }
 
     fun setResponseStateLoading(route: Enum<*>) {
-        val destNum = destNumFlow.value ?: destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
 
         _radioConfigState.update { it.copy(route = route.name, responseState = ResponseState.Loading()) }
 
@@ -613,7 +627,7 @@ open class RadioConfigViewModel(
     }
 
     private fun processPacketResponse(packet: MeshPacket) {
-        val destNum = destNode.value?.num ?: return
+        val destNum = destNum ?: destNode.value?.num ?: return
         val result = processRadioResponseUseCase(packet, destNum, requestIds.value) ?: return
         val route = radioConfigState.value.route
 

feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt

@@ -402,19 +402,34 @@ class RadioConfigViewModelTest {
     }
 
     @Test
-    fun `initDestNum updates value correctly including null`() = runTest {
-        viewModel = createViewModel()
-
-        // Initial setup should take the flow value, but let's just force update it
-        viewModel.initDestNum(123)
-        assertEquals(
-            123,
-            viewModel.destNode.value?.num ?: 123,
-        ) // the flow combine might need yielding, but we can just check it doesn't crash
-
-        // The bug was that null was ignored. Here we test we can pass null
-        // Since we can't easily read destNumFlow directly, we can just call it to ensure no crashes
-        viewModel.initDestNum(null)
+    fun `destNum from SavedStateHandle resolves destNode`() = runTest {
+        val node = Node(num = 456, user = User(id = "!456"))
+        nodeRepository.setNodes(listOf(node))
+        viewModel =
+            RadioConfigViewModel(
+                savedStateHandle = SavedStateHandle(mapOf("destNum" to 456)),
+                radioConfigRepository = radioConfigRepository,
+                packetRepository = packetRepository,
+                serviceRepository = serviceRepository,
+                nodeRepository = nodeRepository,
+                locationRepository = locationRepository,
+                mapConsentPrefs = mapConsentPrefs,
+                analyticsPrefs = analyticsPrefs,
+                homoglyphEncodingPrefs = homoglyphEncodingPrefs,
+                toggleAnalyticsUseCase = toggleAnalyticsUseCase,
+                toggleHomoglyphEncodingUseCase = toggleHomoglyphEncodingUseCase,
+                importProfileUseCase = importProfileUseCase,
+                exportProfileUseCase = exportProfileUseCase,
+                exportSecurityConfigUseCase = exportSecurityConfigUseCase,
+                installProfileUseCase = installProfileUseCase,
+                radioConfigUseCase = radioConfigUseCase,
+                adminActionsUseCase = adminActionsUseCase,
+                processRadioResponseUseCase = processRadioResponseUseCase,
+                locationService = locationService,
+                fileService = fileService,
+                mqttManager = mqttManager,
+            )
+        assertEquals(456, viewModel.destNode.value?.num)
     }
 
     @Test