fix(settings): retain config session across navigation - #6449
Conversation
📝 WalkthroughWalkthroughSettings navigation now resolves ChangesSettings radio configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MainScreen
participant DesktopMainScreen
participant settingsGraph
participant settingsRadioConfigViewModelProvider
participant RadioConfigViewModel
MainScreen->>settingsGraph: pass session-aware provider
DesktopMainScreen->>settingsGraph: pass session-aware provider
settingsGraph->>settingsRadioConfigViewModelProvider: resolve entry session
settingsRadioConfigViewModelProvider->>RadioConfigViewModel: create or retrieve scoped instance
RadioConfigViewModel-->>settingsGraph: provide route configuration state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt`:
- Around line 90-107: The settingsRadioConfigViewModel flow must stop retaining
abandoned remote RadioConfigViewModel instances in the app-shell ViewModelStore.
Tie each session to the relevant backstack-entry store/lifetime, or explicitly
remove/reset the previous settings-remote-* entry when session.destination
changes, ensuring collectors from the old destination are cancelled while
preserving the active local or remote session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 423fea01-d290-4997-8d26-5bd182e4cf35
📒 Files selected for processing (5)
androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.kt
38d29eb to
c19fc6c
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
The diagnosis here is right and worth fixing: Navigation 3 gives every entry its own ViewModelStoreOwner, so getRadioConfigViewModel was minting a separate RadioConfigViewModel per settings entry — each one re-running eight launchIn(viewModelScope) collectors and its own remote fetch, with the "session" living in whichever instance the visible screen happened to resolve. Hoisting to one store keyed by destination is the right shape, and :feature:settings:allTests is green locally.
Two things I'd want addressed before merge, both downstream of holding the store in a plain remember.
1. Rotation now destroys the config session
SettingsNavigation.kt:107-111 keeps the owner in a plain remember and clears it from a DisposableEffect. MainActivity declares no configChanges (there is no configChanges/screenOrientation attribute anywhere in androidApp/src/main/AndroidManifest.xml), so rotation recreates the activity, disposes the composition, fires onDispose { owner.clear() }, and kills the RadioConfigViewModel mid-session — losing _radioConfigState, any in-flight responseState/pending save, and forcing ensureLoadingForRemote to refetch.
The entry-scoped path this replaces did not have that problem. MeshtasticNavDisplay.kt:132 installs rememberViewModelStoreNavEntryDecorator, whose stores are held by a ViewModelStoreProvider parented to the activity's store; upstream is explicit about it:
removeViewModelStoreOnPop: This parameter was a workaround for detecting configuration changes … Configuration changes are now handled internally.
Entry stores are cleared on onPop, not on composition disposal, so a config session survived rotation before this change and won't after. That's the same class of bug the PR is fixing, just triggered by a different gesture — and rotating mid-config is a pretty ordinary thing to do.
Suggested fix: park the per-session ViewModelStores in a retained holder ViewModel obtained at the shell scope (koinViewModel() in MainScreen/DesktopMainScreen), keyed by session.viewModelKey, clearing stores for keys that are no longer the active session. That keeps the cross-entry sharing this PR wants and the config-change retention nav3 was already giving you.
2. The local session is retained for the whole app-shell lifetime
settingsRadioConfigSession (SettingsNavigation.kt:137) returns destination = null whenever the active backstack contains no SettingsRoute.Settings — which is true on every non-settings tab, not just in local settings. SettingsRadioConfigSession is a data class over a single nullable Int, so remember(session) at :108 sees the same value on the Nodes tab, the Map tab, everywhere, and hands back the same owner. Net effect: once a user opens local settings, that RadioConfigViewModel is never cleared for the life of the composition, and its collectors keep running everywhere in the app — including serviceRepository.meshPacketFlow.onEach(::processPacketResponse) (RadioConfigViewModel.kt:344), which processes every mesh packet.
That's precisely the invariant the new KDoc claims to establish:
…and clears it when the destination changes, so an abandoned remote session cannot retain collectors for the rest of the activity or window lifetime.
True for remote sessions, inverted for the local one. SettingsRadioConfigSession needs to distinguish "no settings destination in the stack" (→ drop the session) from "local settings destination" (→ keep it), rather than collapsing both to null.
Non-blocking
-
Tab switching still tears the session down.
activeBackStackswaps on tab change (MultiBackstack.kt:39-40), so leaving the Settings tab mid-remote-session flips the session to local, clears the remote owner, and returning rebuilds the ViewModel from scratch. Fine as a design choice, but the description's "retain the sameRadioConfigViewModelwhile navigating within one local or remote settings session" reads stronger than what ships — worth stating the boundary. -
The shell now recomposes on every navigation.
rememberSettingsRadioConfigSessioncallsbackStack.toList()outsideremember(:134), soMainScreen/DesktopMainScreensubscribe to backstack mutations; previously the shell only readcurrentTabRoute. Every push/pop now invalidates the shell and rebuilds theentryProvider. Probably minor next to the ViewModel churn you're removing, but it does cut against the "improves perceived UI responsiveness" claim, so I'd either measure it or confine the read below the shell. -
entryScopedRadioConfigViewModelis dead in production. Both Android (Main.kt:113) and Desktop pass a provider; the only caller of the default isandroidApp/src/test/.../NavigationAssemblyTest.kt:57. So the default keeps a second code path compiling that nothing ships, and lets the assembly test assert a path no user hits. Either drop the default and update the test, or add a comment on why it stays. -
@Suppress("ViewModelForwarding", "ModifierMissing")onDesktopMainScreen—ModifierMissingis unrelated to this change and the suppression is function-wide. Worth narrowing to just what the new lambda triggers. -
Nit:
entry<SettingsRoute.Settings>(:159) no longer honorsargs.destNum, deriving destNum from the lastSettingskey instead. Settings is single-pane so these agree at rest; while pushingSettings(1234)ontoSettings()both entries are briefly composed and the outgoing local screen resolves the remote ViewModel for the crossfade. Cosmetic, and awkward to avoid given the shared owner — flagging only because the override was deliberate. -
Tests. The new coverage is over the pure key-derivation helpers, plus one case (
clearing a settings session store clears its view models) that mostly assertsViewModelStore.clear()callsonCleared()— androidx's behavior, not this PR's. Neither of the two claims that actually matter is covered: that local →DeviceConfiguration→LoRayields the same ViewModel instance, and that changing destination clears the previous store. A Compose test oversettingsRadioConfigViewModelwith a fake owner would pin both, and would have caught (2).
c19fc6c to
83c7091
Compare
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.
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.
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.
83c7091 to
82c533a
Compare
|
@jamesarich Thanks for the detailed review. Both lifecycle blockers and the related implementation notes have now been addressed. The session stores are owned by a shell-scoped retained ViewModel, so Android configuration recreation no longer clears an active Each composed navigation entry leases its captured session store. Destination changes and configuration-flow exits therefore retain the outgoing ViewModel only through its transition, then clear the abandoned store after the final lease is released. Settings roots pass their exact Back-stack observation now uses I also covered direct configuration routes such as Connections → LoRa, which do not have a Settings root but still require a retained local session. Local route refreshes now keep the existing connect-time configuration visible instead of briefly showing a The expanded tests cover same-session reuse, configuration recreation, direct-route handoff, destination changes, configuration-flow exit, transition-safe eviction, exact-root precedence, inactive fallback, duplicate-route suppression, and local-versus-remote loading behavior. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt (1)
454-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey the pre-composition loading hook to the ViewModel.
remember { viewModel.ensureLoadingForRemote().let { true } }is unkeyed, so if the provider resolves a differentRadioConfigViewModelfor this entry (session switch), the blocking overlay is never primed for the new instance. The.let { true }wrapper is also obscure.♻️ Suggested change
- remember { viewModel.ensureLoadingForRemote().let { true } } + remember(viewModel) { + // Must run before the first collectAsStateWithLifecycle read so the overlay shows on frame one. + viewModel.ensureLoadingForRemote() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt` around lines 454 - 459, Update the pre-composition loading hook in the addEntryProvider block to key remember by the resolved viewModel, ensuring ensureLoadingForRemote() runs for each new RadioConfigViewModel instance. Replace the obscure .let { true } wrapper with a direct remembered invocation while preserving the blocking overlay behavior for remote settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt`:
- Around line 155-168: Update the store creation and retention flow around
storeFor/ownerFor so a newly created SessionStore for the current navigation
entry is protected from clearUnusedStores() before DisposableEffect retains it.
Seed one lease atomically when creating the current store, then release that
lease during disposal, while preserving existing lease-based cleanup for other
stores.
---
Nitpick comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt`:
- Around line 454-459: Update the pre-composition loading hook in the
addEntryProvider block to key remember by the resolved viewModel, ensuring
ensureLoadingForRemote() runs for each new RadioConfigViewModel instance.
Replace the obscure .let { true } wrapper with a direct remembered invocation
while preserving the blocking overlay behavior for remote settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 833157c0-8e1c-410f-ab4f-d68c35ed05c1
📒 Files selected for processing (10)
androidApp/src/main/kotlin/org/meshtastic/app/ui/Main.ktandroidApp/src/test/kotlin/org/meshtastic/app/ui/NavigationAssemblyTest.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/ui/DesktopMainScreen.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/ResponseState.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/LoadingOverlay.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- desktopApp/src/main/kotlin/org/meshtastic/desktop/navigation/DesktopNavigation.kt
| 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() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any test covering a cross-session transition where the outgoing entry releases
# after the incoming entry's store is created.
rg -n -C5 'release\(|retain\(|ownerFor\(' feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigationTest.ktRepository: meshtastic/Meshtastic-Android
Length of output: 4507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt --view expanded || true
echo "== relevant settings navigation source =="
wc -l feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt
sed -n '80,185p' feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt | cat -n
echo "== resolveEntrySession and usages/Navigation call sites =="
rg -n -C4 'resolveEntrySession|SettingsRadioConfigSessionHolder|SettingsRadioConfigViewModel|settingsRadioConfigSession|ownerFor|activate\(|retain\(|release\(' feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.ktRepository: meshtastic/Meshtastic-Android
Length of output: 12931
Protect zero-lease stores created for the current navigation entry.
In a cross-session transition, the incoming entry can create a SessionStore during composition while a different outgoing entry disposes in the same apply pass; because the incoming store has not yet been retained by DisposableEffect, clearUnusedStores() clears it and removes it from the map. Make creation and retention atomic, for example by seeding the lease when calling ownerFor and releasing it on dispose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/navigation/SettingsNavigation.kt`
around lines 155 - 168, Update the store creation and retention flow around
storeFor/ownerFor so a newly created SessionStore for the current navigation
entry is protected from clearUnusedStores() before DisposableEffect retains it.
Seed one lease atomically when creating the current store, then release that
lease during disposal, while preserving existing lease-based cleanup for other
stores.
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed at 82c533a. Both blockers are properly addressed, and the fixes go further than what I asked for — thanks for that.
Rotation (was blocking). SettingsRadioConfigSessionHolder is a retained @KoinViewModel resolved from the shell's LocalViewModelStoreOwner (the activity), so the per-session stores now outlive activity recreation. The lease design is the part I'd have gotten wrong: activate() marking a session current, plus refcounted retain/release from each entry's DisposableEffect, means the store survives the window where the old composition has released but the new one hasn't acquired. I traced both interleavings — release-then-activate and activate-then-release — and they converge on the same eviction either way, which is what configuration recreation keeps the active settings store and direct local route transition does not clear its active store between entries pin down. onCleared sweeping the map closes the last exit.
Local session retention (was blocking). The sealed Inactive/Local/Remote split is the right shape, and usesRadioConfigSettingsSession catching the Connections -> LoRa direct-entry case is a path I'd missed entirely — good catch. meshPacketFlow collectors no longer survive on unrelated tabs.
Also resolved: the dead default provider is gone (NavigationAssemblyTest now fails loudly instead of exercising a shipped-nowhere path), ModifierMissing is fixed properly with a real modifier param rather than suppressed, entry<SettingsRoute.Settings> takes args again so the crossfade nit is moot, and the new holder tests cover same-instance retention and cross-destination eviction directly.
I also went looking for a stuck overlay in avoid local refresh loading flash — ensureLoadingForRemote now fires on destNum != null (synchronous) instead of !state.isLocal (async, defaults false), so a remote route sets Loading on the first frame; if loadConfigRoute then early-returned for a non-refreshable module the overlay would never resolve. It doesn't: setResponseStateLoading writes ResponseState.Empty on that path (RadioConfigViewModel.kt:757-760). Not a bug. Worth noting the destNum != null gate does change behavior for remote-administering your own node (destNum == myNodeNum was isLocal, so no overlay; now it shows one) — that reads as a fix to me, since admin round-trips really are in flight there, but it's a deliberate change rather than a no-op refactor.
:feature:settings:allTests, :androidApp:testFdroidDebugUnitTest, and :feature:settings:detekt are green locally.
One non-blocking item, and it's specifically about the comment rather than the behavior:
Observes back-stack mutations without invalidating the app shell for every settings submenu push or pop.
I don't think that holds. produceState's initialValue is an ordinary argument expression, re-evaluated on every recomposition, and it calls settingsRadioConfigSession(backStack.toList()) — so the snapshot read of the backstack still happens on the way in. Because rememberSettingsRadioConfigSession and rememberSettingsRadioConfigViewModelProvider both return values, neither is a restartable composable, so that read is attributed to the nearest restartable ancestor: MainScreen / DesktopMainScreen. Every push and pop should still invalidate the shell.
What produceState + distinctUntilChanged does buy is that the returned State only changes on real session transitions, so holder.activate isn't churning — worth keeping. But the shell invalidation is unchanged from before, just now with an extra coroutine hop that delays activate by a frame (harmless as far as I can tell — I walked the eviction orderings above with the lag in mind).
derivedStateOf would actually deliver what the comment claims, and is simpler and synchronous:
@Composable
internal fun rememberSettingsRadioConfigSession(backStack: NavBackStack<NavKey>) =
remember(backStack) { derivedStateOf { settingsRadioConfigSession(backStack.toList()) } }Reads inside a derivedStateOf are tracked by the derived-state object, not the enclosing scope, so the shell only invalidates when the session value actually changes. Worth confirming with a recomposition counter before changing anything — this is a subtle enough corner of the compiler that I'd rather you verify than take my word for it. Either way, please reword the comment to match whichever behavior ships.
Two smaller nits, neither worth another round:
remember(entrySession, holder) { holder.ownerFor(entrySession) }mutatesstoresviagetOrPutfrom inside aremembercalculation. An abandoned composition would leave a zero-lease entry behind — self-healing on the nextclearUnusedStores(), so it's benign, but it is a side effect in a slot-table calculation.usesRadioConfigSettingsSessionderives theConfigRoute/ModuleRoutecases automatically but hardcodes the four hand-written entries (Settings,DeviceConfiguration,ModuleConfiguration,Administration). A future entry added tosettingsGraphhas to be mirrored here. The lease keeps a live screen safe if someone forgets, so this is a maintenance note, not a hazard.
Approving.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
2 participants
Overview
Navigation 3 gives each navigation entry its own
ViewModelStoreOwner. ResolvingRadioConfigViewModelindependently from each entry therefore recreated the radio-configuration session while moving between configuration pages, restarting its collectors and losing transient state.This change introduces retained local and per-node remote configuration sessions shared across their navigation entries on Android and Desktop. Session stores survive Android configuration changes, remain available while outgoing transition entries are still composed, and are cleared once the corresponding configuration flow is no longer active and its final entry lease has been released.
Direct local configuration routes, including Connections → LoRa, participate in the same retained-session lifecycle even when the back stack does not contain a Settings root.
Session lifecycle
ViewModelStores in a shell-scoped holderViewModelso the active session survives Android configuration recreation.RadioConfigViewModelwhile navigating within the same local or remote configuration session.Loading behavior
0%progress overlay.Navigation integration
snapshotFlowand emit only logical session changes.Modifierparameter toDesktopMainScreen.ViewModelForwarding.Testing
Added focused coverage for:
Scope
Summary by CodeRabbit
Improvements
Bug Fixes