From 3b51efb33eafadc6282c6cb3d91d82129958df82 Mon Sep 17 00:00:00 2001 From: Jeremiah K <17190268+jeremiah-k@users.noreply.github.com> Date: Thu, 23 Jul 2026 22:24:08 -0500 Subject: [PATCH 1/4] fix(settings): retain config session across navigation Resolve RadioConfigViewModel lazily from the app-shell ViewModelStoreOwner instead of each Navigation 3 entry so moving between Settings submenus no longer destroys and recreates the same radio-configuration session. Use deterministic local and destination-specific keys to preserve remote-administration isolation while retaining a session for the activity or desktop-window lifetime. Keep the entry-scoped provider as the default for standalone graph consumers and align Android and Desktop hosts. Derive the active destination from one immutable back-stack snapshot and reject only exact duplicate top-route pushes, preventing rapid double taps from creating redundant Settings entries without changing ordinary back navigation. Keep host-level ViewModel forwarding explicit, remove obsolete navigation parameters, and isolate initial-route selection so the Android and Desktop assembly points remain within static-analysis limits. Cover destination inheritance, return to a newer local Settings root, and duplicate-route admission. --- .../main/kotlin/org/meshtastic/app/ui/Main.kt | 16 ++--- .../desktop/navigation/DesktopNavigation.kt | 5 +- .../desktop/ui/DesktopMainScreen.kt | 16 ++++- .../settings/navigation/SettingsNavigation.kt | 69 ++++++++++++++----- .../navigation/SettingsNavigationTest.kt | 59 ++++++++++++++++ 5 files changed, 137 insertions(+), 28 deletions(-) create mode 100644 feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt diff --git a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt index a840ca83410..dbf8df90c3d 100644 --- a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt +++ b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt @@ -27,6 +27,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.lifecycle.compose.collectAsStateWithLifecycle +import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation3.runtime.NavKey import androidx.navigation3.runtime.entryProvider import co.touchlab.kermit.Logger @@ -55,6 +56,7 @@ import org.meshtastic.feature.messaging.navigation.contactsGraph import org.meshtastic.feature.node.navigation.nodesGraph import org.meshtastic.feature.settings.lockdown.LockdownDialog import org.meshtastic.feature.settings.navigation.settingsGraph +import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel import org.meshtastic.feature.settings.radio.channel.channelsGraph import org.meshtastic.feature.wifiprovision.navigation.wifiProvisionGraph @@ -62,16 +64,11 @@ import org.meshtastic.feature.wifiprovision.navigation.wifiProvisionGraph fun MainScreen() { val viewModel: UIViewModel = koinViewModel() // Land on Connections for first-run / no-device-selected; otherwise on Nodes (seeded from prefs). - val initialTab = remember { - if (viewModel.currentDeviceAddressFlow.value.isNullOrSelectedNone()) { - TopLevelDestination.Connect.route - } else { - NodesRoute.Nodes - } - } + val initialTab = remember { initialRoute(viewModel.currentDeviceAddressFlow.value) } val multiBackstack = rememberMultiBackstack(initialTab) val backStack = multiBackstack.activeBackStack val scrollToTopEvents = viewModel.scrollToTopEventFlow + val appViewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) AndroidAppVersionCheck(viewModel) @@ -113,7 +110,7 @@ fun MainScreen() { channelsGraph(backStack) connectionsGraph(backStack) discoveryGraph(backStack) - settingsGraph(backStack) + settingsGraph(backStack) { settingsRadioConfigViewModel(backStack, appViewModelStoreOwner) } docsEntries(backStack) firmwareGraph(backStack) wifiProvisionGraph(backStack) @@ -125,6 +122,9 @@ fun MainScreen() { } } +private fun initialRoute(deviceAddress: String?): NavKey = + if (deviceAddress.isNullOrSelectedNone()) TopLevelDestination.Connect.route else NodesRoute.Nodes + /** True when no device address is persisted, or the address is the "none" sentinel (`"n"`). */ private fun String?.isNullOrSelectedNone(): Boolean = isNullOrBlank() || this == "n" diff --git a/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt b/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt index 5e1fdacfe73..5c70b6a1720 100644 --- a/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt +++ b/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt @@ -16,6 +16,7 @@ */ package org.meshtastic.desktop.navigation +import androidx.compose.runtime.Composable import androidx.navigation3.runtime.EntryProviderScope import androidx.navigation3.runtime.NavBackStack import androidx.navigation3.runtime.NavKey @@ -30,6 +31,7 @@ import org.meshtastic.feature.map.navigation.mapGraph import org.meshtastic.feature.messaging.navigation.contactsGraph import org.meshtastic.feature.node.navigation.nodesGraph import org.meshtastic.feature.settings.navigation.settingsGraph +import org.meshtastic.feature.settings.radio.RadioConfigViewModel import org.meshtastic.feature.settings.radio.channel.channelsGraph import org.meshtastic.feature.wifiprovision.navigation.wifiProvisionGraph @@ -43,6 +45,7 @@ fun EntryProviderScope.desktopNavGraph( backStack: NavBackStack, uiViewModel: UIViewModel, multiBackstack: MultiBackstack, + settingsRadioConfigViewModel: @Composable () -> RadioConfigViewModel, ) { nodesGraph( backStack = backStack, @@ -57,7 +60,7 @@ fun EntryProviderScope.desktopNavGraph( ) mapGraph(backStack) firmwareGraph(backStack) - settingsGraph(backStack) + settingsGraph(backStack, settingsRadioConfigViewModel) docsEntries(backStack) channelsGraph(backStack) connectionsGraph(backStack) diff --git a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt index a12fcd294bb..0ddac428d3d 100644 --- a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt +++ b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt @@ -22,6 +22,7 @@ import androidx.compose.material3.Surface import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.unit.dp +import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation3.runtime.NavKey import androidx.navigation3.runtime.entryProvider import org.meshtastic.core.navigation.MultiBackstack @@ -30,14 +31,17 @@ import org.meshtastic.core.ui.component.MeshtasticNavDisplay import org.meshtastic.core.ui.component.MeshtasticNavigationSuite import org.meshtastic.core.ui.viewmodel.UIViewModel import org.meshtastic.desktop.navigation.desktopNavGraph +import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel /** * Desktop main screen — assembles the shared [MeshtasticAppShell], [MeshtasticNavigationSuite], and * [MeshtasticNavDisplay] with the desktop-specific [desktopNavGraph] entry provider. */ +@Suppress("ViewModelForwarding", "ModifierMissing") @Composable fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) { val backStack = multiBackstack.activeBackStack + val appViewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) Surface(modifier = Modifier.fillMaxSize()) { MeshtasticAppShell( @@ -50,7 +54,17 @@ fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) uiViewModel = uiViewModel, modifier = Modifier.fillMaxSize(), ) { - val provider = entryProvider { desktopNavGraph(backStack, uiViewModel, multiBackstack) } + val provider = + entryProvider { + desktopNavGraph( + backStack = backStack, + uiViewModel = uiViewModel, + multiBackstack = multiBackstack, + settingsRadioConfigViewModel = { + settingsRadioConfigViewModel(backStack, appViewModelStoreOwner) + }, + ) + } MeshtasticNavDisplay( multiBackstack = multiBackstack, entryProvider = provider, 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 de647978291..17ceafe0826 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 @@ -20,6 +20,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.remember +import androidx.lifecycle.ViewModelStoreOwner import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.compose.dropUnlessResumed import androidx.navigation3.runtime.EntryProviderScope @@ -75,34 +76,66 @@ import org.meshtastic.feature.settings.radio.component.TelemetryConfigScreen import org.meshtastic.feature.settings.radio.component.UserConfigScreen import kotlin.reflect.KClass +/** + * Resolves the settings [RadioConfigViewModel] from the app-shell [androidx.lifecycle.ViewModelStoreOwner]. + * + * Navigation 3 gives each entry its own store. Resolving this ViewModel inside every settings entry therefore destroys + * and recreates the same radio-config session while moving between settings menus. Resolving against the app-shell + * owner keeps one local or destination-keyed session for the activity/window lifetime while remaining lazy until a + * settings entry actually needs it. + */ @Composable -fun getRadioConfigViewModel(backStack: NavBackStack, destNumOverride: Int? = null): RadioConfigViewModel { - val destNum = - destNumOverride - ?: remember(backStack.toList()) { - backStack.lastOrNull { it is SettingsRoute.Settings }?.let { (it as SettingsRoute.Settings).destNum } - } +fun settingsRadioConfigViewModel( + backStack: NavBackStack, + viewModelStoreOwner: ViewModelStoreOwner, +): RadioConfigViewModel { + val stackSnapshot = backStack.toList() + val destNum = remember(stackSnapshot) { settingsDestination(stackSnapshot) } + val key = destNum?.let { "settings-remote-$it" } ?: "settings-local" + return koinViewModel(key = key, viewModelStoreOwner = viewModelStoreOwner) { + parametersOf(destNum) + } +} + +@Composable +private fun entryScopedRadioConfigViewModel(backStack: NavBackStack): RadioConfigViewModel { + val stackSnapshot = backStack.toList() + val destNum = remember(stackSnapshot) { settingsDestination(stackSnapshot) } return koinViewModel(key = destNum?.toString()) { parametersOf(destNum) } } +internal fun settingsDestination(backStack: List): Int? = + backStack.filterIsInstance().lastOrNull()?.destNum + +internal fun shouldAddSettingsRoute(current: NavKey?, route: Route): Boolean = current != route + +private fun NavBackStack.addSettingsRoute(route: Route) { + if (shouldAddSettingsRoute(lastOrNull(), route)) add(route) +} + @Suppress("LongMethod", "CyclomaticComplexMethod") -fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { +fun EntryProviderScope.settingsGraph( + backStack: NavBackStack, + radioConfigViewModelProvider: @Composable () -> RadioConfigViewModel = { + entryScopedRadioConfigViewModel(backStack) + }, +) { entry { args -> val isTabRoot = backStack.firstOrNull() == args SettingsMainScreen( settingsViewModel = koinViewModel(), - radioConfigViewModel = getRadioConfigViewModel(backStack, destNumOverride = args.destNum), + radioConfigViewModel = radioConfigViewModelProvider(), onClickNodeChip = { backStack.add(NodesRoute.NodeDetail(it)) }, - onNavigate = { backStack.add(it) }, + onNavigate = backStack::addSettingsRoute, onBack = if (isTabRoot) null else dropUnlessResumed { backStack.removeLastOrNull() }, ) } entry { DeviceConfigurationScreen( - viewModel = getRadioConfigViewModel(backStack), + viewModel = radioConfigViewModelProvider(), onBack = dropUnlessResumed { backStack.removeLastOrNull() }, - onNavigate = { route -> backStack.add(route) }, + onNavigate = backStack::addSettingsRoute, ) } @@ -110,16 +143,16 @@ fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { val settingsViewModel: SettingsViewModel = koinViewModel() val hiddenFeaturesUnlocked by settingsViewModel.hiddenFeaturesUnlocked.collectAsStateWithLifecycle() ModuleConfigurationScreen( - viewModel = getRadioConfigViewModel(backStack), + viewModel = radioConfigViewModelProvider(), hiddenFeaturesUnlocked = hiddenFeaturesUnlocked, onBack = dropUnlessResumed { backStack.removeLastOrNull() }, - onNavigate = { route -> backStack.add(route) }, + onNavigate = backStack::addSettingsRoute, ) } entry { AdministrationScreen( - viewModel = getRadioConfigViewModel(backStack), + viewModel = radioConfigViewModelProvider(), onBack = dropUnlessResumed { backStack.removeLastOrNull() }, ) } @@ -130,7 +163,7 @@ fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { } ConfigRoute.entries.forEach { routeInfo -> - configComposable(routeInfo.route::class, backStack, routeInfo) { viewModel -> + configComposable(routeInfo.route::class, routeInfo, radioConfigViewModelProvider) { viewModel -> when (routeInfo) { ConfigRoute.USER -> UserConfigScreen(viewModel, onBack = dropUnlessResumed { backStack.removeLastOrNull() }) @@ -166,7 +199,7 @@ fun EntryProviderScope.settingsGraph(backStack: NavBackStack) { } ModuleRoute.entries.forEach { routeInfo -> - configComposable(routeInfo.route::class, backStack, routeInfo) { viewModel -> + configComposable(routeInfo.route::class, routeInfo, radioConfigViewModelProvider) { viewModel -> when (routeInfo) { ModuleRoute.MQTT -> MQTTConfigScreen(viewModel, onBack = dropUnlessResumed { backStack.removeLastOrNull() }) @@ -274,12 +307,12 @@ expect fun SettingsMainScreen( /** Expect declarations for platform-specific config screens. */ fun EntryProviderScope.configComposable( route: KClass, - backStack: NavBackStack, routeInfo: Enum<*>, + radioConfigViewModelProvider: @Composable () -> RadioConfigViewModel, content: @Composable (RadioConfigViewModel) -> Unit, ) { addEntryProvider(route) { - val viewModel = getRadioConfigViewModel(backStack) + val viewModel = radioConfigViewModelProvider() // Set loading state before content reads the StateFlow, ensuring // LoadingOverlay is visible from the very first composition frame. remember { viewModel.ensureLoadingForRemote().let { true } } diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt new file mode 100644 index 00000000000..0fa6e3e8698 --- /dev/null +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.settings.navigation + +import androidx.navigation3.runtime.NavKey +import org.meshtastic.core.navigation.SettingsRoute +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class SettingsNavigationTest { + + @Test + fun `settings destination follows the latest settings root through submenus`() { + val stack = + listOf( + SettingsRoute.Settings(destNum = 1234), + SettingsRoute.DeviceConfiguration, + SettingsRoute.LoRa, + ) + + assertEquals(1234, settingsDestination(stack)) + } + + @Test + fun `settings destination returns to local for a newer local root`() { + val stack = + listOf( + SettingsRoute.Settings(destNum = 1234), + SettingsRoute.DeviceConfiguration, + SettingsRoute.Settings(), + SettingsRoute.ModuleConfiguration, + ) + + assertNull(settingsDestination(stack)) + } + + @Test + fun `duplicate current route is not pushed again`() { + assertFalse(shouldAddSettingsRoute(SettingsRoute.DeviceConfiguration, SettingsRoute.DeviceConfiguration)) + assertTrue(shouldAddSettingsRoute(SettingsRoute.DeviceConfiguration, SettingsRoute.ModuleConfiguration)) + } +} From 51e742226b3bb4bc72c912beca9758dc94c996ee Mon Sep 17 00:00:00 2001 From: Jeremiah K <17190268+jeremiah-k@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:34:13 -0500 Subject: [PATCH 2/4] fix(settings): scope config sessions to active destination Derive the selected destination and both app-shell and entry-scoped ViewModel keys from one SettingsRadioConfigSession so Android and Desktop navigation cannot diverge between ownership paths. Retain the active RadioConfigViewModel across that destination's Settings submenus in a dedicated ViewModelStore. Replace and clear the store when the local or remote destination changes, cancelling collectors owned by an abandoned remote session instead of retaining every administered node for the activity or window lifetime. Cover local and remote key contracts and verify store disposal clears its ViewModels. --- .../main/kotlin/org/meshtastic/app/ui/Main.kt | 8 ++- .../desktop/ui/DesktopMainScreen.kt | 6 +- .../settings/navigation/SettingsNavigation.kt | 58 ++++++++++++++----- .../navigation/SettingsNavigationTest.kt | 34 +++++++++++ 4 files changed, 87 insertions(+), 19 deletions(-) diff --git a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt index dbf8df90c3d..31b024f42db 100644 --- a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt +++ b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt @@ -27,7 +27,6 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.lifecycle.compose.collectAsStateWithLifecycle -import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation3.runtime.NavKey import androidx.navigation3.runtime.entryProvider import co.touchlab.kermit.Logger @@ -55,6 +54,7 @@ import org.meshtastic.feature.map.navigation.mapGraph import org.meshtastic.feature.messaging.navigation.contactsGraph import org.meshtastic.feature.node.navigation.nodesGraph import org.meshtastic.feature.settings.lockdown.LockdownDialog +import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelStoreOwner import org.meshtastic.feature.settings.navigation.settingsGraph import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel import org.meshtastic.feature.settings.radio.channel.channelsGraph @@ -68,7 +68,7 @@ fun MainScreen() { val multiBackstack = rememberMultiBackstack(initialTab) val backStack = multiBackstack.activeBackStack val scrollToTopEvents = viewModel.scrollToTopEventFlow - val appViewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) + val settingsViewModelStoreOwner = rememberSettingsRadioConfigViewModelStoreOwner(backStack) AndroidAppVersionCheck(viewModel) @@ -110,7 +110,9 @@ fun MainScreen() { channelsGraph(backStack) connectionsGraph(backStack) discoveryGraph(backStack) - settingsGraph(backStack) { settingsRadioConfigViewModel(backStack, appViewModelStoreOwner) } + settingsGraph(backStack) { + settingsRadioConfigViewModel(backStack, settingsViewModelStoreOwner) + } docsEntries(backStack) firmwareGraph(backStack) wifiProvisionGraph(backStack) diff --git a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt index 0ddac428d3d..4bf9184a3c8 100644 --- a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt +++ b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt @@ -22,7 +22,6 @@ import androidx.compose.material3.Surface import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.unit.dp -import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation3.runtime.NavKey import androidx.navigation3.runtime.entryProvider import org.meshtastic.core.navigation.MultiBackstack @@ -31,6 +30,7 @@ import org.meshtastic.core.ui.component.MeshtasticNavDisplay import org.meshtastic.core.ui.component.MeshtasticNavigationSuite import org.meshtastic.core.ui.viewmodel.UIViewModel import org.meshtastic.desktop.navigation.desktopNavGraph +import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelStoreOwner import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel /** @@ -41,7 +41,7 @@ import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel @Composable fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) { val backStack = multiBackstack.activeBackStack - val appViewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) + val settingsViewModelStoreOwner = rememberSettingsRadioConfigViewModelStoreOwner(backStack) Surface(modifier = Modifier.fillMaxSize()) { MeshtasticAppShell( @@ -61,7 +61,7 @@ fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) uiViewModel = uiViewModel, multiBackstack = multiBackstack, settingsRadioConfigViewModel = { - settingsRadioConfigViewModel(backStack, appViewModelStoreOwner) + settingsRadioConfigViewModel(backStack, settingsViewModelStoreOwner) }, ) } 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 17ceafe0826..60c27317967 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 @@ -17,9 +17,11 @@ package org.meshtastic.feature.settings.navigation import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.remember +import androidx.lifecycle.ViewModelStore import androidx.lifecycle.ViewModelStoreOwner import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.compose.dropUnlessResumed @@ -77,35 +79,65 @@ import org.meshtastic.feature.settings.radio.component.UserConfigScreen import kotlin.reflect.KClass /** - * Resolves the settings [RadioConfigViewModel] from the app-shell [androidx.lifecycle.ViewModelStoreOwner]. + * Resolves the settings [RadioConfigViewModel] from a session-owned [androidx.lifecycle.ViewModelStoreOwner]. * * Navigation 3 gives each entry its own store. Resolving this ViewModel inside every settings entry therefore destroys - * and recreates the same radio-config session while moving between settings menus. Resolving against the app-shell - * owner keeps one local or destination-keyed session for the activity/window lifetime while remaining lazy until a - * settings entry actually needs it. + * and recreates the same radio-config session while moving between settings menus. A store keyed to the active local or + * remote session preserves that session across its submenu entries and clears it when the destination changes, so an + * abandoned remote session cannot retain collectors for the rest of the activity or window lifetime. */ +private const val LOCAL_SETTINGS_VIEW_MODEL_KEY = "settings-local" +private const val REMOTE_SETTINGS_VIEW_MODEL_KEY_PREFIX = "settings-remote-" + +internal data class SettingsRadioConfigSession(val destination: Int?) { + val viewModelKey: String + get() = destination?.let { "$REMOTE_SETTINGS_VIEW_MODEL_KEY_PREFIX$it" } ?: LOCAL_SETTINGS_VIEW_MODEL_KEY + + val entryKey: String? + get() = destination?.toString() +} + +internal class SettingsRadioConfigViewModelStoreOwner : ViewModelStoreOwner { + override val viewModelStore = ViewModelStore() + + fun clear() = viewModelStore.clear() +} + +@Composable +fun rememberSettingsRadioConfigViewModelStoreOwner(backStack: NavBackStack): ViewModelStoreOwner { + val session = rememberSettingsRadioConfigSession(backStack) + val owner = remember(session) { SettingsRadioConfigViewModelStoreOwner() } + DisposableEffect(owner) { onDispose(owner::clear) } + return owner +} + @Composable fun settingsRadioConfigViewModel( backStack: NavBackStack, viewModelStoreOwner: ViewModelStoreOwner, ): RadioConfigViewModel { - val stackSnapshot = backStack.toList() - val destNum = remember(stackSnapshot) { settingsDestination(stackSnapshot) } - val key = destNum?.let { "settings-remote-$it" } ?: "settings-local" - return koinViewModel(key = key, viewModelStoreOwner = viewModelStoreOwner) { - parametersOf(destNum) + val session = rememberSettingsRadioConfigSession(backStack) + return koinViewModel(key = session.viewModelKey, viewModelStoreOwner = viewModelStoreOwner) { + parametersOf(session.destination) } } @Composable private fun entryScopedRadioConfigViewModel(backStack: NavBackStack): RadioConfigViewModel { + val session = rememberSettingsRadioConfigSession(backStack) + return koinViewModel(key = session.entryKey) { parametersOf(session.destination) } +} + +@Composable +private fun rememberSettingsRadioConfigSession(backStack: NavBackStack): SettingsRadioConfigSession { val stackSnapshot = backStack.toList() - val destNum = remember(stackSnapshot) { settingsDestination(stackSnapshot) } - return koinViewModel(key = destNum?.toString()) { parametersOf(destNum) } + return remember(stackSnapshot) { settingsRadioConfigSession(stackSnapshot) } } -internal fun settingsDestination(backStack: List): Int? = - backStack.filterIsInstance().lastOrNull()?.destNum +internal fun settingsRadioConfigSession(backStack: List): SettingsRadioConfigSession = + SettingsRadioConfigSession(backStack.filterIsInstance().lastOrNull()?.destNum) + +internal fun settingsDestination(backStack: List): Int? = settingsRadioConfigSession(backStack).destination internal fun shouldAddSettingsRoute(current: NavKey?, route: Route): Boolean = current != route diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt index 0fa6e3e8698..20c95dc9dfd 100644 --- a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt @@ -16,6 +16,7 @@ */ package org.meshtastic.feature.settings.navigation +import androidx.lifecycle.ViewModel import androidx.navigation3.runtime.NavKey import org.meshtastic.core.navigation.SettingsRoute import kotlin.test.Test @@ -51,9 +52,42 @@ class SettingsNavigationTest { assertNull(settingsDestination(stack)) } + @Test + fun `settings session uses stable local and remote keys`() { + val localSession = settingsRadioConfigSession(listOf(SettingsRoute.Settings())) + val remoteSession = settingsRadioConfigSession(listOf(SettingsRoute.Settings(destNum = 1234))) + + assertNull(localSession.destination) + assertEquals("settings-local", localSession.viewModelKey) + assertNull(localSession.entryKey) + assertEquals(1234, remoteSession.destination) + assertEquals("settings-remote-1234", remoteSession.viewModelKey) + assertEquals("1234", remoteSession.entryKey) + } + + @Test + fun `clearing a settings session store clears its view models`() { + val owner = SettingsRadioConfigViewModelStoreOwner() + val viewModel = TrackingViewModel() + owner.viewModelStore.put("remote", viewModel) + + owner.clear() + + assertTrue(viewModel.wasCleared) + } + @Test fun `duplicate current route is not pushed again`() { assertFalse(shouldAddSettingsRoute(SettingsRoute.DeviceConfiguration, SettingsRoute.DeviceConfiguration)) assertTrue(shouldAddSettingsRoute(SettingsRoute.DeviceConfiguration, SettingsRoute.ModuleConfiguration)) } + + private class TrackingViewModel : ViewModel() { + var wasCleared = false + private set + + override fun onCleared() { + wasCleared = true + } + } } From 1d5cba0ec6baa2a884c2945630b3f8690cfb1079 Mon Sep 17 00:00:00 2001 From: Jeremiah K <17190268+jeremiah-k@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:10:25 +0000 Subject: [PATCH 3/4] fix(settings): retain active config sessions safely Keep child ViewModelStores in a shell-scoped holder ViewModel so an active local or remote configuration session survives Android configuration changes. Model inactive, local, and per-node remote states separately, and observe only logical session changes instead of invalidating the shell for every submenu mutation. Activate sessions after successful composition and lease each store for the lifetime of its navigation entry. Destination changes and Settings-tab exits therefore clear abandoned collectors only after the outgoing crossfade entry is disposed. Capture each Settings root's exact destination so overlapping local and remote entries cannot resolve one another's RadioConfigViewModel. Remove the unused entry-scoped production fallback and narrow the Desktop suppression to ViewModel forwarding. Cover store reuse across submenus and configuration recreation, destination and tab eviction, transition fallback, root-destination precedence, and duplicate-route admission. --- .../main/kotlin/org/meshtastic/app/ui/Main.kt | 9 +- .../app/ui/NavigationAssemblyTest.kt | 2 +- .../desktop/navigation/DesktopNavigation.kt | 3 +- .../desktop/ui/DesktopMainScreen.kt | 15 +- .../settings/navigation/SettingsNavigation.kt | 196 ++++++++++++++---- .../navigation/SettingsNavigationTest.kt | 166 +++++++++++++-- 6 files changed, 316 insertions(+), 75 deletions(-) diff --git a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt index 31b024f42db..a8a4f2aea77 100644 --- a/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt +++ b/androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.kt @@ -54,9 +54,8 @@ import org.meshtastic.feature.map.navigation.mapGraph import org.meshtastic.feature.messaging.navigation.contactsGraph import org.meshtastic.feature.node.navigation.nodesGraph import org.meshtastic.feature.settings.lockdown.LockdownDialog -import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelStoreOwner +import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelProvider import org.meshtastic.feature.settings.navigation.settingsGraph -import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel import org.meshtastic.feature.settings.radio.channel.channelsGraph import org.meshtastic.feature.wifiprovision.navigation.wifiProvisionGraph @@ -68,7 +67,7 @@ fun MainScreen() { val multiBackstack = rememberMultiBackstack(initialTab) val backStack = multiBackstack.activeBackStack val scrollToTopEvents = viewModel.scrollToTopEventFlow - val settingsViewModelStoreOwner = rememberSettingsRadioConfigViewModelStoreOwner(backStack) + val settingsRadioConfigViewModelProvider = rememberSettingsRadioConfigViewModelProvider(backStack) AndroidAppVersionCheck(viewModel) @@ -110,9 +109,7 @@ fun MainScreen() { channelsGraph(backStack) connectionsGraph(backStack) discoveryGraph(backStack) - settingsGraph(backStack) { - settingsRadioConfigViewModel(backStack, settingsViewModelStoreOwner) - } + settingsGraph(backStack, settingsRadioConfigViewModelProvider) docsEntries(backStack) firmwareGraph(backStack) wifiProvisionGraph(backStack) diff --git a/androidApp/src/test/kotlin/org/meshtastic/app/ui/NavigationAssemblyTest.kt b/androidApp/src/test/kotlin/org/meshtastic/app/ui/NavigationAssemblyTest.kt index eefe7765f04..a8d82a6b28f 100644 --- a/androidApp/src/test/kotlin/org/meshtastic/app/ui/NavigationAssemblyTest.kt +++ b/androidApp/src/test/kotlin/org/meshtastic/app/ui/NavigationAssemblyTest.kt @@ -54,7 +54,7 @@ class NavigationAssemblyTest { channelsGraph(backStack) connectionsGraph(backStack) discoveryGraph(backStack) - settingsGraph(backStack) + settingsGraph(backStack) { _ -> error("Settings ViewModel is not composed in this assembly test") } firmwareGraph(backStack) } } diff --git a/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt b/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt index 5c70b6a1720..90e93e13142 100644 --- a/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt +++ b/desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt @@ -21,6 +21,7 @@ import androidx.navigation3.runtime.EntryProviderScope import androidx.navigation3.runtime.NavBackStack import androidx.navigation3.runtime.NavKey import org.meshtastic.core.navigation.MultiBackstack +import org.meshtastic.core.navigation.SettingsRoute import org.meshtastic.core.navigation.TopLevelDestination import org.meshtastic.core.ui.viewmodel.UIViewModel import org.meshtastic.feature.connections.navigation.connectionsGraph @@ -45,7 +46,7 @@ fun EntryProviderScope.desktopNavGraph( backStack: NavBackStack, uiViewModel: UIViewModel, multiBackstack: MultiBackstack, - settingsRadioConfigViewModel: @Composable () -> RadioConfigViewModel, + settingsRadioConfigViewModel: @Composable (SettingsRoute.Settings?) -> RadioConfigViewModel, ) { nodesGraph( backStack = backStack, diff --git a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt index 4bf9184a3c8..fd709878eb7 100644 --- a/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt +++ b/desktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.kt @@ -30,20 +30,19 @@ import org.meshtastic.core.ui.component.MeshtasticNavDisplay import org.meshtastic.core.ui.component.MeshtasticNavigationSuite import org.meshtastic.core.ui.viewmodel.UIViewModel import org.meshtastic.desktop.navigation.desktopNavGraph -import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelStoreOwner -import org.meshtastic.feature.settings.navigation.settingsRadioConfigViewModel +import org.meshtastic.feature.settings.navigation.rememberSettingsRadioConfigViewModelProvider /** * Desktop main screen — assembles the shared [MeshtasticAppShell], [MeshtasticNavigationSuite], and * [MeshtasticNavDisplay] with the desktop-specific [desktopNavGraph] entry provider. */ -@Suppress("ViewModelForwarding", "ModifierMissing") +@Suppress("ViewModelForwarding") @Composable -fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) { +fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack, modifier: Modifier = Modifier) { val backStack = multiBackstack.activeBackStack - val settingsViewModelStoreOwner = rememberSettingsRadioConfigViewModelStoreOwner(backStack) + val settingsRadioConfigViewModelProvider = rememberSettingsRadioConfigViewModelProvider(backStack) - Surface(modifier = Modifier.fillMaxSize()) { + Surface(modifier = modifier.fillMaxSize()) { MeshtasticAppShell( multiBackstack = multiBackstack, uiViewModel = uiViewModel, @@ -60,9 +59,7 @@ fun DesktopMainScreen(uiViewModel: UIViewModel, multiBackstack: MultiBackstack) backStack = backStack, uiViewModel = uiViewModel, multiBackstack = multiBackstack, - settingsRadioConfigViewModel = { - settingsRadioConfigViewModel(backStack, settingsViewModelStoreOwner) - }, + settingsRadioConfigViewModel = settingsRadioConfigViewModelProvider, ) } MeshtasticNavDisplay( 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 60c27317967..93dfe81627e 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 @@ -19,8 +19,12 @@ package org.meshtastic.feature.settings.navigation import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.SideEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.produceState import androidx.compose.runtime.remember +import androidx.compose.runtime.snapshotFlow +import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelStore import androidx.lifecycle.ViewModelStoreOwner import androidx.lifecycle.compose.collectAsStateWithLifecycle @@ -28,7 +32,9 @@ import androidx.lifecycle.compose.dropUnlessResumed import androidx.navigation3.runtime.EntryProviderScope import androidx.navigation3.runtime.NavBackStack import androidx.navigation3.runtime.NavKey +import kotlinx.coroutines.flow.distinctUntilChanged import org.koin.compose.viewmodel.koinViewModel +import org.koin.core.annotation.KoinViewModel import org.koin.core.parameter.parametersOf import org.meshtastic.core.navigation.NodesRoute import org.meshtastic.core.navigation.Route @@ -79,22 +85,25 @@ import org.meshtastic.feature.settings.radio.component.UserConfigScreen import kotlin.reflect.KClass /** - * Resolves the settings [RadioConfigViewModel] from a session-owned [androidx.lifecycle.ViewModelStoreOwner]. - * - * Navigation 3 gives each entry its own store. Resolving this ViewModel inside every settings entry therefore destroys - * and recreates the same radio-config session while moving between settings menus. A store keyed to the active local or - * remote session preserves that session across its submenu entries and clears it when the destination changes, so an - * abandoned remote session cannot retain collectors for the rest of the activity or window lifetime. + * Identifies whether the active back stack is outside settings, local settings, or remote settings for one node. + * Keeping these states distinct prevents a local settings ViewModel from remaining active on unrelated tabs. */ -private const val LOCAL_SETTINGS_VIEW_MODEL_KEY = "settings-local" -private const val REMOTE_SETTINGS_VIEW_MODEL_KEY_PREFIX = "settings-remote-" +internal sealed interface SettingsRadioConfigSession { + data object Inactive : SettingsRadioConfigSession -internal data class SettingsRadioConfigSession(val destination: Int?) { - val viewModelKey: String - get() = destination?.let { "$REMOTE_SETTINGS_VIEW_MODEL_KEY_PREFIX$it" } ?: LOCAL_SETTINGS_VIEW_MODEL_KEY + sealed interface Active : SettingsRadioConfigSession { + val destination: Int? + val viewModelKey: String + } - val entryKey: String? - get() = destination?.toString() + data object Local : Active { + override val destination: Int? = null + override val viewModelKey: String = "settings-local" + } + + data class Remote(override val destination: Int) : Active { + override val viewModelKey: String = "settings-remote-$destination" + } } internal class SettingsRadioConfigViewModelStoreOwner : ViewModelStoreOwner { @@ -103,41 +112,142 @@ internal class SettingsRadioConfigViewModelStoreOwner : ViewModelStoreOwner { fun clear() = viewModelStore.clear() } -@Composable -fun rememberSettingsRadioConfigViewModelStoreOwner(backStack: NavBackStack): ViewModelStoreOwner { - val session = rememberSettingsRadioConfigSession(backStack) - val owner = remember(session) { SettingsRadioConfigViewModelStoreOwner() } - DisposableEffect(owner) { onDispose(owner::clear) } - return owner +/** + * Retains settings-session stores across configuration changes. A store remains available while its session is the + * active settings destination or while a navigation entry still holds a lease during an exit transition. Once neither + * condition applies, the store is cleared and its [RadioConfigViewModel] collectors are cancelled. + */ +@KoinViewModel +internal class SettingsRadioConfigSessionHolder : ViewModel() { + private data class SessionStore( + val owner: SettingsRadioConfigViewModelStoreOwner = SettingsRadioConfigViewModelStoreOwner(), + var leases: Int = 0, + ) + + private val stores = mutableMapOf() + private var activeSession: SettingsRadioConfigSession = SettingsRadioConfigSession.Inactive + private var lastActiveSession: SettingsRadioConfigSession.Active? = null + + fun activate(session: SettingsRadioConfigSession) { + activeSession = session + if (session is SettingsRadioConfigSession.Active) lastActiveSession = session + clearUnusedStores() + } + + /** + * Resolves the session captured by a settings entry. During an exit transition the active back stack may already be + * outside settings, so the most recently active session is used instead of throwing from the outgoing composition. + */ + fun resolveEntrySession(session: SettingsRadioConfigSession): SettingsRadioConfigSession.Active = + (session as? SettingsRadioConfigSession.Active) ?: lastActiveSession ?: SettingsRadioConfigSession.Local + + fun ownerFor(session: SettingsRadioConfigSession.Active): ViewModelStoreOwner = storeFor(session).owner + + fun retain(session: SettingsRadioConfigSession.Active) { + storeFor(session).leases += 1 + } + + fun release(session: SettingsRadioConfigSession.Active) { + stores[session]?.let { store -> if (store.leases > 0) store.leases -= 1 } + clearUnusedStores() + } + + private fun storeFor(session: SettingsRadioConfigSession.Active): SessionStore = + stores.getOrPut(session, ::SessionStore) + + private fun clearUnusedStores() { + val current = activeSession as? SettingsRadioConfigSession.Active + val iterator = stores.iterator() + while (iterator.hasNext()) { + val (session, store) = iterator.next() + if (session != current && store.leases == 0) { + store.owner.clear() + iterator.remove() + } + } + } + + override fun onCleared() { + stores.values.forEach { it.owner.clear() } + stores.clear() + super.onCleared() + } } +/** + * Observes back-stack mutations without invalidating the app shell for every settings submenu push or pop. The returned + * state changes only when the logical settings session changes. + */ @Composable -fun settingsRadioConfigViewModel( - backStack: NavBackStack, +internal fun rememberSettingsRadioConfigSession(backStack: NavBackStack) = + produceState(initialValue = settingsRadioConfigSession(backStack.toList()), backStack) { + snapshotFlow { settingsRadioConfigSession(backStack.toList()) }.distinctUntilChanged().collect { value = it } + } + +@Composable +internal fun settingsRadioConfigViewModel( + session: SettingsRadioConfigSession.Active, viewModelStoreOwner: ViewModelStoreOwner, -): RadioConfigViewModel { - val session = rememberSettingsRadioConfigSession(backStack) - return koinViewModel(key = session.viewModelKey, viewModelStoreOwner = viewModelStoreOwner) { +): RadioConfigViewModel = + koinViewModel(key = session.viewModelKey, viewModelStoreOwner = viewModelStoreOwner) { parametersOf(session.destination) } -} +/** Returns the retained provider used by all entries in the active settings session. */ @Composable -private fun entryScopedRadioConfigViewModel(backStack: NavBackStack): RadioConfigViewModel { - val session = rememberSettingsRadioConfigSession(backStack) - return koinViewModel(key = session.entryKey) { parametersOf(session.destination) } +fun rememberSettingsRadioConfigViewModelProvider( + backStack: NavBackStack, +): @Composable (SettingsRoute.Settings?) -> RadioConfigViewModel { + val session by rememberSettingsRadioConfigSession(backStack) + val holder: SettingsRadioConfigSessionHolder = koinViewModel() + SideEffect { holder.activate(session) } + + return remember(backStack, holder) { + @Composable { settingsRoot -> + // Capture the session once for this navigation entry. An outgoing entry keeps its original local/remote + // session during crossfades even after the active top-level back stack has changed. + val entrySession = + remember(settingsRoot, holder) { + val candidate = + if (settingsRoot != null) { + settingsRoot.destNum?.let(SettingsRadioConfigSession::Remote) + ?: SettingsRadioConfigSession.Local + } else { + settingsRadioConfigSession(backStack.toList()) + } + holder.resolveEntrySession(candidate) + } + val owner = remember(entrySession, holder) { holder.ownerFor(entrySession) } + + DisposableEffect(holder, entrySession) { + holder.retain(entrySession) + onDispose { holder.release(entrySession) } + } + + settingsRadioConfigViewModel(session = entrySession, viewModelStoreOwner = owner) + } + } } -@Composable -private fun rememberSettingsRadioConfigSession(backStack: NavBackStack): SettingsRadioConfigSession { - val stackSnapshot = backStack.toList() - return remember(stackSnapshot) { settingsRadioConfigSession(stackSnapshot) } +internal fun settingsRadioConfigSession(backStack: List): SettingsRadioConfigSession { + if (backStack.none(NavKey::usesRadioConfigSettingsSession)) return SettingsRadioConfigSession.Inactive + + // Remote administration always carries a Settings root with its destination. Local configuration can also be + // opened directly from another graph (currently Connections -> LoRa), so the absence of a root is still an active + // local session while a radio-config route remains on the active back stack. + val settingsRoot = backStack.filterIsInstance().lastOrNull() + return settingsRoot?.destNum?.let(SettingsRadioConfigSession::Remote) ?: SettingsRadioConfigSession.Local } -internal fun settingsRadioConfigSession(backStack: List): SettingsRadioConfigSession = - SettingsRadioConfigSession(backStack.filterIsInstance().lastOrNull()?.destNum) +private fun NavKey.usesRadioConfigSettingsSession(): Boolean = this is SettingsRoute.Settings || + this == SettingsRoute.DeviceConfiguration || + this == SettingsRoute.ModuleConfiguration || + this == SettingsRoute.Administration || + ConfigRoute.entries.any { it.route == this } || + ModuleRoute.entries.any { it.route == this } -internal fun settingsDestination(backStack: List): Int? = settingsRadioConfigSession(backStack).destination +internal fun settingsDestination(backStack: List): Int? = + (settingsRadioConfigSession(backStack) as? SettingsRadioConfigSession.Active)?.destination internal fun shouldAddSettingsRoute(current: NavKey?, route: Route): Boolean = current != route @@ -148,15 +258,13 @@ private fun NavBackStack.addSettingsRoute(route: Route) { @Suppress("LongMethod", "CyclomaticComplexMethod") fun EntryProviderScope.settingsGraph( backStack: NavBackStack, - radioConfigViewModelProvider: @Composable () -> RadioConfigViewModel = { - entryScopedRadioConfigViewModel(backStack) - }, + radioConfigViewModelProvider: @Composable (SettingsRoute.Settings?) -> RadioConfigViewModel, ) { entry { args -> val isTabRoot = backStack.firstOrNull() == args SettingsMainScreen( settingsViewModel = koinViewModel(), - radioConfigViewModel = radioConfigViewModelProvider(), + radioConfigViewModel = radioConfigViewModelProvider(args), onClickNodeChip = { backStack.add(NodesRoute.NodeDetail(it)) }, onNavigate = backStack::addSettingsRoute, onBack = if (isTabRoot) null else dropUnlessResumed { backStack.removeLastOrNull() }, @@ -165,7 +273,7 @@ fun EntryProviderScope.settingsGraph( entry { DeviceConfigurationScreen( - viewModel = radioConfigViewModelProvider(), + viewModel = radioConfigViewModelProvider(null), onBack = dropUnlessResumed { backStack.removeLastOrNull() }, onNavigate = backStack::addSettingsRoute, ) @@ -175,7 +283,7 @@ fun EntryProviderScope.settingsGraph( val settingsViewModel: SettingsViewModel = koinViewModel() val hiddenFeaturesUnlocked by settingsViewModel.hiddenFeaturesUnlocked.collectAsStateWithLifecycle() ModuleConfigurationScreen( - viewModel = radioConfigViewModelProvider(), + viewModel = radioConfigViewModelProvider(null), hiddenFeaturesUnlocked = hiddenFeaturesUnlocked, onBack = dropUnlessResumed { backStack.removeLastOrNull() }, onNavigate = backStack::addSettingsRoute, @@ -184,7 +292,7 @@ fun EntryProviderScope.settingsGraph( entry { AdministrationScreen( - viewModel = radioConfigViewModelProvider(), + viewModel = radioConfigViewModelProvider(null), onBack = dropUnlessResumed { backStack.removeLastOrNull() }, ) } @@ -340,11 +448,11 @@ expect fun SettingsMainScreen( fun EntryProviderScope.configComposable( route: KClass, routeInfo: Enum<*>, - radioConfigViewModelProvider: @Composable () -> RadioConfigViewModel, + radioConfigViewModelProvider: @Composable (SettingsRoute.Settings?) -> RadioConfigViewModel, content: @Composable (RadioConfigViewModel) -> Unit, ) { addEntryProvider(route) { - val viewModel = radioConfigViewModelProvider() + val viewModel = radioConfigViewModelProvider(null) // Set loading state before content reads the StateFlow, ensuring // LoadingOverlay is visible from the very first composition frame. remember { viewModel.ensureLoadingForRemote().let { true } } diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt index 20c95dc9dfd..6f69affd784 100644 --- a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt @@ -18,11 +18,15 @@ package org.meshtastic.feature.settings.navigation import androidx.lifecycle.ViewModel import androidx.navigation3.runtime.NavKey +import org.meshtastic.core.navigation.ConnectionsRoute +import org.meshtastic.core.navigation.NodesRoute import org.meshtastic.core.navigation.SettingsRoute import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNotSame import kotlin.test.assertNull +import kotlin.test.assertSame import kotlin.test.assertTrue class SettingsNavigationTest { @@ -39,6 +43,30 @@ class SettingsNavigationTest { assertEquals(1234, settingsDestination(stack)) } + @Test + fun `settings sessions distinguish inactive local and remote states`() { + assertEquals(SettingsRadioConfigSession.Inactive, settingsRadioConfigSession(listOf(NodesRoute.Nodes))) + assertEquals(SettingsRadioConfigSession.Local, settingsRadioConfigSession(listOf(SettingsRoute.Settings()))) + assertEquals( + SettingsRadioConfigSession.Remote(1234), + settingsRadioConfigSession(listOf(SettingsRoute.Settings(destNum = 1234))), + ) + } + + @Test + fun `direct local config route outside settings tab keeps a local session active`() { + val stack = listOf(ConnectionsRoute.Connections(), SettingsRoute.LoRa) + + assertEquals(SettingsRadioConfigSession.Local, settingsRadioConfigSession(stack)) + } + + @Test + fun `non radio settings route outside settings tab stays inactive`() { + val stack = listOf(NodesRoute.Nodes, SettingsRoute.FilterSettings) + + assertEquals(SettingsRadioConfigSession.Inactive, settingsRadioConfigSession(stack)) + } + @Test fun `settings destination returns to local for a newer local root`() { val stack = @@ -50,32 +78,142 @@ class SettingsNavigationTest { ) assertNull(settingsDestination(stack)) + assertEquals(SettingsRadioConfigSession.Local, settingsRadioConfigSession(stack)) + } + + @Test + fun `same settings session retains its view model store`() { + val holder = SettingsRadioConfigSessionHolder() + val session = SettingsRadioConfigSession.Remote(1234) + holder.activate(session) + holder.retain(session) + val firstOwner = holder.ownerFor(session) + val viewModel = TrackingViewModel() + firstOwner.viewModelStore.put("radio", viewModel) + + holder.activate(session) + val secondOwner = holder.ownerFor(session) + + assertSame(firstOwner, secondOwner) + assertFalse(viewModel.wasCleared) + holder.release(session) + } + + @Test + fun `direct local route transition does not clear its active store between entries`() { + val holder = SettingsRadioConfigSessionHolder() + val session = settingsRadioConfigSession(listOf(ConnectionsRoute.Connections(), SettingsRoute.LoRa)) + assertEquals(SettingsRadioConfigSession.Local, session) + + holder.activate(session) + val activeSession = session as SettingsRadioConfigSession.Active + holder.retain(activeSession) + val firstOwner = holder.ownerFor(activeSession) + val viewModel = TrackingViewModel() + firstOwner.viewModelStore.put("radio", viewModel) + + // The outgoing entry can release before the incoming entry acquires its lease. Because the direct route is an + // active local settings session, that zero-lease transition must not evict the store. + holder.release(activeSession) + val secondOwner = holder.ownerFor(activeSession) + holder.retain(activeSession) + + assertSame(firstOwner, secondOwner) + assertFalse(viewModel.wasCleared) + holder.release(activeSession) } @Test - fun `settings session uses stable local and remote keys`() { - val localSession = settingsRadioConfigSession(listOf(SettingsRoute.Settings())) - val remoteSession = settingsRadioConfigSession(listOf(SettingsRoute.Settings(destNum = 1234))) - - assertNull(localSession.destination) - assertEquals("settings-local", localSession.viewModelKey) - assertNull(localSession.entryKey) - assertEquals(1234, remoteSession.destination) - assertEquals("settings-remote-1234", remoteSession.viewModelKey) - assertEquals("1234", remoteSession.entryKey) + fun `configuration recreation keeps the active settings store`() { + val holder = SettingsRadioConfigSessionHolder() + val session = SettingsRadioConfigSession.Local + holder.activate(session) + holder.retain(session) + val firstOwner = holder.ownerFor(session) + val viewModel = TrackingViewModel() + firstOwner.viewModelStore.put("radio", viewModel) + + // The old composition releases its lease during recreation, but the retained holder still marks this + // session active. + holder.release(session) + val secondOwner = holder.ownerFor(session) + holder.retain(session) + + assertSame(firstOwner, secondOwner) + assertFalse(viewModel.wasCleared) + holder.release(session) + } + + @Test + fun `changing destination clears the previous store after its exit lease ends`() { + val holder = SettingsRadioConfigSessionHolder() + val firstSession = SettingsRadioConfigSession.Remote(1234) + val secondSession = SettingsRadioConfigSession.Remote(5678) + holder.activate(firstSession) + holder.retain(firstSession) + val firstOwner = holder.ownerFor(firstSession) + val firstViewModel = TrackingViewModel() + firstOwner.viewModelStore.put("radio", firstViewModel) + + holder.activate(secondSession) + val secondOwner = holder.ownerFor(secondSession) + + assertFalse(firstViewModel.wasCleared) + assertNotSame(firstOwner, secondOwner) + + holder.release(firstSession) + + assertTrue(firstViewModel.wasCleared) } @Test - fun `clearing a settings session store clears its view models`() { - val owner = SettingsRadioConfigViewModelStoreOwner() + fun `leaving settings clears the active store after its exit lease ends`() { + val holder = SettingsRadioConfigSessionHolder() + val session = SettingsRadioConfigSession.Local + holder.activate(session) + holder.retain(session) + val owner = holder.ownerFor(session) val viewModel = TrackingViewModel() - owner.viewModelStore.put("remote", viewModel) + owner.viewModelStore.put("radio", viewModel) + + holder.activate(SettingsRadioConfigSession.Inactive) + + assertFalse(viewModel.wasCleared) - owner.clear() + holder.release(session) assertTrue(viewModel.wasCleared) } + @Test + fun `explicit entry session wins over the previous active destination`() { + val holder = SettingsRadioConfigSessionHolder() + holder.activate(SettingsRadioConfigSession.Remote(1234)) + + assertEquals(SettingsRadioConfigSession.Local, holder.resolveEntrySession(SettingsRadioConfigSession.Local)) + assertEquals( + SettingsRadioConfigSession.Remote(5678), + holder.resolveEntrySession(SettingsRadioConfigSession.Remote(5678)), + ) + } + + @Test + fun `inactive exit transition resolves the most recent active session`() { + val holder = SettingsRadioConfigSessionHolder() + val session = SettingsRadioConfigSession.Remote(1234) + holder.activate(session) + holder.activate(SettingsRadioConfigSession.Inactive) + + assertEquals(session, holder.resolveEntrySession(SettingsRadioConfigSession.Inactive)) + } + + @Test + fun `inactive provider without prior settings uses a safe local fallback`() { + val holder = SettingsRadioConfigSessionHolder() + + assertEquals(SettingsRadioConfigSession.Local, holder.resolveEntrySession(SettingsRadioConfigSession.Inactive)) + } + @Test fun `duplicate current route is not pushed again`() { assertFalse(shouldAddSettingsRoute(SettingsRoute.DeviceConfiguration, SettingsRoute.DeviceConfiguration)) From 82c533aabef1e75ac08affee4ed079a3ba28472e Mon Sep 17 00:00:00 2001 From: Jeremiah K <17190268+jeremiah-k@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:22:16 +0000 Subject: [PATCH 4/4] fix(settings): avoid local refresh loading flash --- .../settings/navigation/SettingsNavigation.kt | 6 +- .../settings/radio/RadioConfigViewModel.kt | 75 +++++++++++-------- .../feature/settings/radio/ResponseState.kt | 9 ++- .../radio/component/LoadingOverlay.kt | 9 ++- .../radio/RadioConfigViewModelTest.kt | 56 +++++++++++--- 5 files changed, 106 insertions(+), 49 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 93dfe81627e..4a116f1430c 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 @@ -453,10 +453,10 @@ fun EntryProviderScope.configComposable( ) { addEntryProvider(route) { val viewModel = radioConfigViewModelProvider(null) - // Set loading state before content reads the StateFlow, ensuring - // LoadingOverlay is visible from the very first composition frame. + // Remote settings need a blocking progress overlay from the first frame. Local settings already have their + // connect-time repository snapshot, so their route refresh stays non-blocking and does not flash a 0% overlay. remember { viewModel.ensureLoadingForRemote().let { true } } - LaunchedEffect(Unit) { viewModel.setResponseStateLoading(routeInfo) } + LaunchedEffect(Unit) { viewModel.loadConfigRoute(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 623170c6c8a..29bb17745f3 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 @@ -736,12 +736,21 @@ open class RadioConfigViewModel( */ fun ensureLoadingForRemote() { val state = _radioConfigState.value - if (!state.isLocal && state.responseState is ResponseState.Empty) { + if (destNum != null && state.responseState is ResponseState.Empty) { _radioConfigState.update { it.copy(responseState = ResponseState.Loading()) } } } + /** Refreshes a config route while keeping the connect-time local snapshot visible. */ + fun loadConfigRoute(route: Enum<*>) { + setResponseStateLoading(route = route, showOverlay = destNum != null) + } + fun setResponseStateLoading(route: Enum<*>) { + setResponseStateLoading(route = route, showOverlay = true) + } + + private fun setResponseStateLoading(route: Enum<*>, showOverlay: Boolean) { val destNum = destNum ?: destNode.value?.num ?: return // A module without a per-module get (no ModuleConfigType, e.g. MeshBeacon) reads from the connect-time config @@ -751,7 +760,9 @@ open class RadioConfigViewModel( return } - _radioConfigState.update { it.copy(route = route.name, responseState = ResponseState.Loading()) } + _radioConfigState.update { + it.copy(route = route.name, responseState = ResponseState.Loading(showOverlay = showOverlay)) + } when (route) { ConfigRoute.USER -> @@ -783,38 +794,42 @@ open class RadioConfigViewModel( setResponseStateTotal(2) } - is ConfigRoute -> { - if (route == ConfigRoute.LORA) { - safeLaunch(tag = "getChannel0ForLora") { - radioConfigUseCase.getChannel(destNum, 0, onRequestId = ::registerRequestId) - } - } - if (route == ConfigRoute.NETWORK) { - safeLaunch(tag = "getConnectionStatus") { - radioConfigUseCase.getDeviceConnectionStatus(destNum, onRequestId = ::registerRequestId) - } - } - safeLaunch(tag = "getConfig") { - radioConfigUseCase.getConfig(destNum, route.type, onRequestId = ::registerRequestId) - } + is ConfigRoute -> loadConfigRoute(destNum, route) + + is ModuleRoute -> loadModuleRoute(destNum, route) + } + } + + private fun loadConfigRoute(destNum: Int, route: ConfigRoute) { + if (route == ConfigRoute.LORA) { + safeLaunch(tag = "getChannel0ForLora") { + radioConfigUseCase.getChannel(destNum, 0, onRequestId = ::registerRequestId) } + } + if (route == ConfigRoute.NETWORK) { + safeLaunch(tag = "getConnectionStatus") { + radioConfigUseCase.getDeviceConnectionStatus(destNum, onRequestId = ::registerRequestId) + } + } + safeLaunch(tag = "getConfig") { + radioConfigUseCase.getConfig(destNum, route.type, onRequestId = ::registerRequestId) + } + } - is ModuleRoute -> { - if (route == ModuleRoute.CANNED_MESSAGE) { - safeLaunch(tag = "getCannedMessages") { - radioConfigUseCase.getCannedMessages(destNum, onRequestId = ::registerRequestId) - } - } - if (route == ModuleRoute.EXT_NOTIFICATION) { - safeLaunch(tag = "getRingtone") { - radioConfigUseCase.getRingtone(destNum, onRequestId = ::registerRequestId) - } - } - safeLaunch(tag = "getModuleConfig") { - radioConfigUseCase.getModuleConfig(destNum, route.type, onRequestId = ::registerRequestId) - } + private fun loadModuleRoute(destNum: Int, route: ModuleRoute) { + if (route == ModuleRoute.CANNED_MESSAGE) { + safeLaunch(tag = "getCannedMessages") { + radioConfigUseCase.getCannedMessages(destNum, onRequestId = ::registerRequestId) } } + if (route == ModuleRoute.EXT_NOTIFICATION) { + safeLaunch(tag = "getRingtone") { + radioConfigUseCase.getRingtone(destNum, onRequestId = ::registerRequestId) + } + } + safeLaunch(tag = "getModuleConfig") { + radioConfigUseCase.getModuleConfig(destNum, route.type, onRequestId = ::registerRequestId) + } } fun shouldReportLocation(nodeNum: Int?) = mapConsentPrefs.shouldReportLocation(nodeNum) diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/ResponseState.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/ResponseState.kt index d0af1403924..6d89abea40a 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/ResponseState.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/ResponseState.kt @@ -22,8 +22,13 @@ import org.meshtastic.core.resources.UiText sealed class ResponseState { data object Empty : ResponseState() - data class Loading(var total: Int = 1, var completed: Int = 0, var status: String? = null) : - ResponseState() + data class Loading( + var total: Int = 1, + var completed: Int = 0, + var status: String? = null, + /** Whether this request should obscure the current screen with the full-size progress UI. */ + val showOverlay: Boolean = true, + ) : ResponseState() data class Success(val result: T) : ResponseState() diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoadingOverlay.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoadingOverlay.kt index 2f269533191..61da6b9320c 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoadingOverlay.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoadingOverlay.kt @@ -47,7 +47,8 @@ private const val PERCENTAGE_FACTOR = 100 @Composable fun LoadingOverlay(state: ResponseState<*>, modifier: Modifier = Modifier) { - AnimatedVisibility(visible = state is ResponseState.Loading, enter = fadeIn(), exit = fadeOut()) { + val loading = state as? ResponseState.Loading + AnimatedVisibility(visible = loading?.showOverlay == true, enter = fadeIn(), exit = fadeOut()) { Box( modifier = modifier @@ -61,9 +62,9 @@ fun LoadingOverlay(state: ResponseState<*>, modifier: Modifier = Modifier) { horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.spacedBy(24.dp), ) { - if (state is ResponseState.Loading) { + if (loading != null) { val clampedProgress = - (state.completed.toFloat() / state.total.coerceAtLeast(1).toFloat()).coerceIn(0f, 1f) + (loading.completed.toFloat() / loading.total.coerceAtLeast(1).toFloat()).coerceIn(0f, 1f) val progress by animateFloatAsState(targetValue = clampedProgress, label = "loadingProgress") Box(contentAlignment = Alignment.Center) { @@ -79,7 +80,7 @@ fun LoadingOverlay(state: ResponseState<*>, modifier: Modifier = Modifier) { ) } - state.status?.let { status -> + loading.status?.let { status -> Text( text = status, style = MaterialTheme.typography.bodyLarge, 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 6db0c2b4f1f..5415317d46f 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 @@ -1125,6 +1125,7 @@ class RadioConfigViewModelTest { } viewModel.setResponseStateLoading(ConfigRoute.USER) verifySuspend { radioConfigUseCase.getOwner(123, any()) } + assertTrue((viewModel.radioConfigState.value.responseState as ResponseState.Loading).showOverlay) // CHANNELS everySuspend { radioConfigUseCase.getChannel(any(), any(), any()) } returns 42 @@ -1140,6 +1141,49 @@ class RadioConfigViewModelTest { verifySuspend { radioConfigUseCase.getConfig(123, ConfigRoute.LORA.type, any()) } } + @Test + fun `loadConfigRoute hides progress overlay for local settings refresh`() = runTest { + val localNode = Node(num = 123, user = User(id = "!123")) + nodeRepository.setNodes(listOf(localNode)) + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) + viewModel = createViewModel(destNum = null) + + everySuspend { radioConfigUseCase.getOwner(any(), any()) } calls + { + it.args.onRequestIdArg()(42) + 42 + } + + viewModel.loadConfigRoute(ConfigRoute.USER) + runCurrent() + + val loading = viewModel.radioConfigState.value.responseState as ResponseState.Loading + assertFalse(loading.showOverlay) + verifySuspend { radioConfigUseCase.getOwner(123, any()) } + } + + @Test + fun `loadConfigRoute shows progress overlay for remote settings refresh`() = 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)) + viewModel = createViewModel(destNum = 456) + + everySuspend { radioConfigUseCase.getOwner(any(), any()) } calls + { + it.args.onRequestIdArg()(42) + 42 + } + + viewModel.loadConfigRoute(ConfigRoute.USER) + runCurrent() + + val loading = viewModel.radioConfigState.value.responseState as ResponseState.Loading + assertTrue(loading.showOverlay) + verifySuspend { radioConfigUseCase.getOwner(456, any()) } + } + @Test fun `registerRequestId timeout clears request and sets error`() = runTest { val node = Node(num = 123, user = User(id = "!123")) @@ -1186,17 +1230,9 @@ class RadioConfigViewModelTest { } @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)) - - val localVm = createViewModel(destNum = 100) - - // Local VM should have isLocal = true - assertTrue(localVm.radioConfigState.value.isLocal) + fun `ensureLoadingForRemote is no-op for local session before node identity resolves`() = runTest { + val localVm = createViewModel(destNum = null) - // ensureLoadingForRemote should NOT change responseState localVm.ensureLoadingForRemote() assertEquals(ResponseState.Empty, localVm.radioConfigState.value.responseState) }