From baa66e6877c22cc9841f4aa04ee9f2e7bebceb70 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Mon, 1 Jun 2026 05:32:52 -0700 Subject: [PATCH] fix: show loading overlay immediately for remote config sub-screens (#5694) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../settings/navigation/SettingsNavigation.kt | 16 ++- .../settings/radio/RadioConfigViewModel.kt | 11 ++ .../radio/component/LoRaConfigItemList.kt | 16 ++- .../radio/RadioConfigViewModelTest.kt | 109 +++++++++++++++++- 4 files changed, 144 insertions(+), 8 deletions(-) diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt index fb300eb4fa..d9ed2c18ef 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt @@ -127,8 +127,7 @@ fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { } ConfigRoute.entries.forEach { routeInfo -> - configComposable(routeInfo.route::class, backStack) { viewModel -> - LaunchedEffect(Unit) { viewModel.setResponseStateLoading(routeInfo) } + configComposable(routeInfo.route::class, backStack, routeInfo) { viewModel -> when (routeInfo) { ConfigRoute.USER -> UserConfigScreen(viewModel, onBack = dropUnlessResumed { backStack.removeLastOrNull() }) @@ -164,8 +163,7 @@ fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { } ModuleRoute.entries.forEach { routeInfo -> - configComposable(routeInfo.route::class, backStack) { viewModel -> - LaunchedEffect(Unit) { viewModel.setResponseStateLoading(routeInfo) } + configComposable(routeInfo.route::class, backStack, routeInfo) { viewModel -> when (routeInfo) { ModuleRoute.MQTT -> MQTTConfigScreen(viewModel, onBack = dropUnlessResumed { backStack.removeLastOrNull() }) @@ -266,7 +264,15 @@ expect fun SettingsMainScreen( fun EntryProviderScope.configComposable( route: KClass, backStack: NavBackStack, + routeInfo: Enum<*>, content: @Composable (RadioConfigViewModel) -> Unit, ) { - addEntryProvider(route) { content(getRadioConfigViewModel(backStack)) } + addEntryProvider(route) { + val viewModel = getRadioConfigViewModel(backStack) + // Set loading state before content reads the StateFlow, ensuring + // LoadingOverlay is visible from the very first composition frame. + remember { viewModel.ensureLoadingForRemote().let { true } } + LaunchedEffect(Unit) { viewModel.setResponseStateLoading(routeInfo) } + content(viewModel) + } } diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt index 4b9fdcdd1b..df231ca4f8 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt @@ -476,6 +476,17 @@ open class RadioConfigViewModel( _radioConfigState.update { it.copy(responseState = ResponseState.Empty) } } + /** + * Sets the initial loading state for remote config sub-screens. Must be called before the first + * `collectAsStateWithLifecycle` read so the LoadingOverlay is visible from the very first composition frame. + */ + fun ensureLoadingForRemote() { + val state = _radioConfigState.value + if (!state.isLocal && state.responseState is ResponseState.Empty) { + _radioConfigState.update { it.copy(responseState = ResponseState.Loading()) } + } + } + fun setResponseStateLoading(route: Enum<*>) { val destNum = destNum ?: destNode.value?.num ?: return diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoRaConfigItemList.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoRaConfigItemList.kt index ced63fff66..e3cf7283c5 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoRaConfigItemList.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoRaConfigItemList.kt @@ -71,7 +71,21 @@ private val CODING_RATE_RANGE = 5..8 fun LoRaConfigScreen(viewModel: RadioConfigViewModel, onBack: () -> Unit) { val state by viewModel.radioConfigState.collectAsStateWithLifecycle() val loraConfig = state.radioConfig.lora ?: Config.LoRaConfig() - val primarySettings = state.channelList.getOrNull(0) ?: return + val primarySettings = state.channelList.getOrNull(0) + + if (primarySettings == null) { + RadioConfigScreenList( + title = stringResource(Res.string.lora), + onBack = onBack, + configState = rememberConfigState(initialValue = loraConfig), + enabled = false, + responseState = state.responseState, + onDismissPacketResponse = viewModel::clearPacketResponse, + onSave = {}, + ) {} + return + } + val formState = rememberConfigState(initialValue = loraConfig) val primaryChannel = remember(formState.value) { Channel(primarySettings, formState.value) } diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt index 0375d390f5..0793b72f47 100644 --- a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt @@ -48,6 +48,7 @@ import org.meshtastic.core.domain.usecase.settings.RadioResponseResult import org.meshtastic.core.domain.usecase.settings.ToggleAnalyticsUseCase import org.meshtastic.core.domain.usecase.settings.ToggleHomoglyphEncodingUseCase import org.meshtastic.core.model.MqttProbeStatus +import org.meshtastic.core.model.MyNodeInfo import org.meshtastic.core.model.Node import org.meshtastic.core.repository.AnalyticsPrefs import org.meshtastic.core.repository.FileService @@ -75,6 +76,7 @@ import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertTrue @OptIn(ExperimentalCoroutinesApi::class) @@ -138,8 +140,8 @@ class RadioConfigViewModelTest { Dispatchers.resetMain() } - private fun createViewModel() = RadioConfigViewModel( - destNum = null, + private fun createViewModel(destNum: Int? = null) = RadioConfigViewModel( + destNum = destNum, radioConfigRepository = radioConfigRepository, packetRepository = packetRepository, serviceRepository = serviceRepository, @@ -610,4 +612,107 @@ class RadioConfigViewModelTest { // actually sendError updates the state? No, sendError sends an event. // But the requestIds gets cleared. } + + @Test + fun `ensureLoadingForRemote sets loading state for remote nodes`() = runTest { + val localNode = Node(num = 100, user = User(id = "!100")) + val remoteNode = Node(num = 456, user = User(id = "!456")) + nodeRepository.setNodes(listOf(localNode, remoteNode)) + nodeRepository.setMyNodeInfo( + MyNodeInfo( + myNodeNum = 100, + hasGPS = false, + model = null, + firmwareVersion = null, + couldUpdate = false, + shouldUpdate = false, + currentPacketId = 0, + messageTimeoutMsec = 0, + minAppVersion = 0, + maxChannels = 8, + hasWifi = false, + channelUtilization = 0f, + airUtilTx = 0f, + deviceId = null, + ), + ) + + val remoteVm = createViewModel(destNum = 456) + + // Remote VM starts with Empty responseState + assertEquals(ResponseState.Empty, remoteVm.radioConfigState.value.responseState) + assertFalse(remoteVm.radioConfigState.value.isLocal) + + // ensureLoadingForRemote should transition to Loading + remoteVm.ensureLoadingForRemote() + assertTrue(remoteVm.radioConfigState.value.responseState is ResponseState.Loading) + } + + @Test + fun `ensureLoadingForRemote is no-op for local nodes`() = runTest { + val localNode = Node(num = 100, user = User(id = "!100")) + nodeRepository.setNodes(listOf(localNode)) + nodeRepository.setMyNodeInfo( + MyNodeInfo( + myNodeNum = 100, + hasGPS = false, + model = null, + firmwareVersion = null, + couldUpdate = false, + shouldUpdate = false, + currentPacketId = 0, + messageTimeoutMsec = 0, + minAppVersion = 0, + maxChannels = 8, + hasWifi = false, + channelUtilization = 0f, + airUtilTx = 0f, + deviceId = null, + ), + ) + + val localVm = createViewModel(destNum = 100) + + // Local VM should have isLocal = true + assertTrue(localVm.radioConfigState.value.isLocal) + + // ensureLoadingForRemote should NOT change responseState + localVm.ensureLoadingForRemote() + assertEquals(ResponseState.Empty, localVm.radioConfigState.value.responseState) + } + + @Test + fun `ensureLoadingForRemote is no-op when already loading`() = runTest { + val localNode = Node(num = 100, user = User(id = "!100")) + val remoteNode = Node(num = 456, user = User(id = "!456")) + nodeRepository.setNodes(listOf(localNode, remoteNode)) + nodeRepository.setMyNodeInfo( + MyNodeInfo( + myNodeNum = 100, + hasGPS = false, + model = null, + firmwareVersion = null, + couldUpdate = false, + shouldUpdate = false, + currentPacketId = 0, + messageTimeoutMsec = 0, + minAppVersion = 0, + maxChannels = 8, + hasWifi = false, + channelUtilization = 0f, + airUtilTx = 0f, + deviceId = null, + ), + ) + + val remoteVm = createViewModel(destNum = 456) + + // Set loading first + remoteVm.ensureLoadingForRemote() + assertTrue(remoteVm.radioConfigState.value.responseState is ResponseState.Loading) + + // Calling again should still be Loading (no-op, not a new instance) + remoteVm.ensureLoadingForRemote() + assertTrue(remoteVm.radioConfigState.value.responseState is ResponseState.Loading) + } }