fix(connections): open the region picker directly from the region card - #6743
Conversation
The "Set your region" card did not navigate itself: it fired setResponseStateLoading(ConfigRoute.LORA) and showed a PacketResponseStateDialog, reaching SettingsRoute.LoRa only from that dialog's onComplete. LORA is a fan-out route (getChannel + getConfig) but the generic ConfigRoute branch leaves Loading.total at 1, and onComplete fires from inside composition. Once the last request id is removed, completePacketResponse calls clearPacketResponse() and resets the state to Empty, so when both admin responses land before a composition observes the completed Loading state the dialog composes straight to Empty - a blank body with only a Close button, and onComplete can never fire again. Adding the missing setResponseStateTotal(2) would not fix it, because clearPacketResponse() is unconditional for non-Admin routes and Empty is always the terminal state; the navigation would still be racing it. The dialog bought nothing anyway: configComposable already calls loadConfigRoute on entry and LoRaConfigScreen renders from the connect-time snapshot while that refresh runs. The Connections screen also used an unparameterised RadioConfigViewModel - a different instance from the keyed one the Settings graph builds - so its fetch never reached the destination. Navigate straight to SettingsRoute.LoRa and drop the dead dialog, its state collection and the now-unused view model parameter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe connections feature removes its radio configuration dependency and view-model injection. Region setup and modem preset actions now navigate directly to ChangesConnections navigation cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The region card now opens the LoRa region settings directly instead of relying on the unreliable intermediate dialog; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 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 |
Same defect as the region card this PR already fixed: the progress dialog can strand the user on an empty state when the admin read completes before the dialog observes it. Also drops the now-unused feature:settings dependency from feature:connections and regenerates its README dependency graph.
|
Follow-up per adversarial review: the ModemPresetInfo tap in ChannelScreen had the identical #6586 bug (progress dialog + Also removed the now-dead Full baseline (spotlessApply/spotlessCheck/detekt/assembleDebug/test/allTests) green. No UI/rendering change (behavior-only |
|
@coderabbitai review |
|
@coderabbitai full review |
❌ Action failedReview failed.
|
✅ Action performedFull review finished. |
Fixes #6586
Tapping the "Set your region" card on Connections showed a brief spinner and then an empty dialog with only a Close button, instead of opening the region picker. Settings → LoRa → Region was unaffected, which is the workaround users found.
The card did not navigate. It called
radioConfigViewModel.setResponseStateLoading(ConfigRoute.LORA)and put up aPacketResponseStateDialog, navigating toSettingsRoute.LoRaonly from that dialog'sonComplete. Two things made that unreliable:LORAis a fan-out route (hasReadFanOut = true) — it issuesgetChannel(0)andgetConfig(LORA_CONFIG)— but the genericis ConfigRoute ->branch leavesLoading.totalat its default of1, unlikeCHANNELSandAdminRoute, which callsetResponseStateTotal(...).PacketResponseStateDialogfiresonComplete()from inside composition (if (state.completed >= state.total) onComplete()), so it only navigates if a composition happens to observe the completedLoadingstate.Once the last request id is removed,
completePacketResponsecallsclearPacketResponse(), which resetsresponseStatetoEmpty. If both admin responses land before a composition observes the completedLoadingstate, the dialog composes straight toEmpty— a blank body with a Close button, andonCompletecan never fire again. That is the reported screenshot.Note this is why simply adding the missing
setResponseStateTotal(2)is not a fix:clearPacketResponse()is unconditional for non-Admin routes, soEmptyis always the terminal state and the navigation would still be racing it, just atcompleted >= 2instead of>= 1.The dialog was never load-bearing here.
configComposablealready callsviewModel.loadConfigRoute(routeInfo)on entry, andLoRaConfigScreen/RadioConfigScreenListrender from the connect-time snapshot while that refresh runs (the overlay is suppressed for local settings, and the dialog only appears forSuccess/Error).settingsRadioConfigSession()already names "Connections -> LoRa" as a supported direct entry. Worse, the Connections screen'sRadioConfigViewModelwas an unparameterisedkoinViewModel()— a different instance from the keyed one the Settings graph builds — so the fetch it was waiting on did not benefit the destination screen anyway.Changes:
ConnectionsScreen: the "Set your region" card navigates directly withonConfigNavigate(SettingsRoute.LoRa).isWaiting/PacketResponseStateDialogblock, theradioConfigStatecollection, and theradioConfigViewModelparameter (plus the matching argument inConnectionsNavigation).On the happy path the user-visible result is identical to what the old
onCompletedid when it won the race — the same destination, minus the dialog.Not covered by this PR
onClick; its visibility gate —uiState == ConnectionUiState.CONNECTED_WITH_NODE && regionUnset && sessionAuthorized && isPhysicalDevice— is untouched, so that report is neither fixed nor made worse here.sessionAuthorizedbeing false on a not-yet-authorised node is the likely cause and wants its own fix.ChannelScreen'sModemPresetInfotap has the same defect (setResponseStateLoading(ConfigRoute.LORA)behind the same dialog). Left alone deliberately: it sharesonComplete/getNavRouteFromwith the channel-edit path, which has a legitimate multi-fetch progress attotal = maxChannels + 1. Worth a follow-up.Testing
No automated test added: this deletes a code path rather than adding logic, and the only assertion left to make (a click navigates) would need a
runComposeUiTestoverConnectionsScreenwith the full Koin graph and ~15 backing flows stubbed. Verified via the repo baseline (spotlessApply spotlessCheck detekt kmpSmokeCompile assembleDebug test allTests).Summary by CodeRabbit
New Features
Bug Fixes