fix(settings): Apply manual channel writes in order#6077
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRadioConfigViewModel now serializes manual channel saves into a paced ordered batch, tracks batch progress and completion separately, and reconciles partial writes on interruption. Tests cover ordering, progress, failure handling, overlap serialization, and interruption behavior. ChangesManual Channel Write Batching
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fe56793 to
48db0f7
Compare
jeremiah-k
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
397-418: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo guard against overlapping
updateChannelsinvocations.Each call to
updateChannelslaunches its own independentsafeLaunchcoroutine that sequentially writes the whole plan. If it's invoked twice in quick succession (e.g., a double-tap save, or a save triggered while a prior save is still pacing through its 1s-per-write delays), the two write sequences can interleave on the wire, undermining the very ordering guarantee this PR introduces. Consider guarding with an in-flightJob/Mutexso a new save either cancels or awaits the previous one before starting.🤖 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/radio/RadioConfigViewModel.kt` around lines 397 - 418, The updateChannels flow in RadioConfigViewModel can start multiple overlapping safeLaunch saves, which can interleave setRemoteChannels writes and break the intended ordering. Add an in-flight guard around updateChannels—such as a Job check/cancel or a Mutex/serialization mechanism—so a new call either waits for or cancels the previous channel-save coroutine before applying applyManualChannelUpdatePlan and the subsequent packetRepository/radioConfigRepository updates.feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt (1)
392-425: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
verifySuspend(exactly(0))assertions for migrate/replaceAllSettings don't actually exercise the failure-gating behavior.
destNumresolves to123butmyNodeInfois never set in this test, sodestNum == myNodeNumisfalseregardless of whether the write plan succeeds or fails —migrateChannelsByPSK/replaceAllSettingswould never be called either way. Only theassertEquals(old, viewModel.radioConfigState.value.channelList)line meaningfully proves the failure path. To actually validate that a write failure prevents migration/replace, setmyNodeInfoso the branch would be taken on success, proving the failure suppresses it.✅ Suggested fix
fun `updateChannels keeps canonical channel list unchanged when ordered write fails`() = runTest { val node = Node(num = 123, user = User(id = "!123")) val channelA = ChannelSettings(name = "A") val channelB = ChannelSettings(name = "B") val channelC = ChannelSettings(name = "C") val channelD = ChannelSettings(name = "D") val old = listOf(channelA, channelB, channelC, channelD) val new = listOf(channelA, channelD) val writtenIndexes = mutableListOf<Int>() every { radioConfigRepository.channelSetFlow } returns MutableStateFlow(ChannelSet(settings = old)) nodeRepository.setNodes(listOf(node)) + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) viewModel = createViewModel() runCurrent()🤖 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/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt` around lines 392 - 425, The failure-path test in RadioConfigViewModelTest is not proving that a failed channel write blocks downstream work because `myNodeInfo` is never set, so `updateChannels` can’t reach the `migrateChannelsByPSK`/`replaceAllSettings` branch even on success. Update the test setup in `updateChannels keeps canonical channel list unchanged when ordered write fails` to initialize `myNodeInfo` (so `destNum == myNodeNum` would allow the success path), then keep the simulated `setRemoteChannel` failure and assert that `radioConfigState.channelList` stays unchanged while the migration/replacement calls are skipped.
🤖 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.
Nitpick comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Around line 397-418: The updateChannels flow in RadioConfigViewModel can start
multiple overlapping safeLaunch saves, which can interleave setRemoteChannels
writes and break the intended ordering. Add an in-flight guard around
updateChannels—such as a Job check/cancel or a Mutex/serialization mechanism—so
a new call either waits for or cancels the previous channel-save coroutine
before applying applyManualChannelUpdatePlan and the subsequent
packetRepository/radioConfigRepository updates.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Around line 392-425: The failure-path test in RadioConfigViewModelTest is not
proving that a failed channel write blocks downstream work because `myNodeInfo`
is never set, so `updateChannels` can’t reach the
`migrateChannelsByPSK`/`replaceAllSettings` branch even on success. Update the
test setup in `updateChannels keeps canonical channel list unchanged when
ordered write fails` to initialize `myNodeInfo` (so `destNum == myNodeNum` would
allow the success path), then keep the simulated `setRemoteChannel` failure and
assert that `radioConfigState.channelList` stays unchanged while the
migration/replacement calls are skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74714fdc-e6fb-4bbd-9422-017cf45b1a1e
📒 Files selected for processing (2)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt (1)
392-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated channel/old-list setup across tests.
The
channelA/channelB/channelC/channelD+oldlist boilerplate is duplicated verbatim across the failure test and the overlapping-saves test. Consider extracting a small helper (e.g.fun fourChannelFixture(): List<ChannelSettings>) to reduce duplication as more manual-channel tests are added.🤖 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/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt` around lines 392 - 467, The `updateChannels` test setup repeats the same four-channel fixture (`channelA`/`channelB`/`channelC`/`channelD` and the `old` list) in multiple tests, so extract that shared setup into a small helper in `RadioConfigViewModelTest` and reuse it from both `updateChannels keeps canonical channel list unchanged when ordered write fails` and `updateChannels serializes overlapping channel saves` to keep the tests DRY and easier to extend.
🤖 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.
Nitpick comments:
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Around line 392-467: The `updateChannels` test setup repeats the same
four-channel fixture (`channelA`/`channelB`/`channelC`/`channelD` and the `old`
list) in multiple tests, so extract that shared setup into a small helper in
`RadioConfigViewModelTest` and reuse it from both `updateChannels keeps
canonical channel list unchanged when ordered write fails` and `updateChannels
serializes overlapping channel saves` to keep the tests DRY and easier to
extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ea5a8f0-6f76-4da9-8e55-adba1f3dca58
📒 Files selected for processing (2)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
The sequencing fix itself is right — the concurrent per-slot safeLaunch writes were the real bug — but the 1s pacing opens response-state races the concurrent code never had, and the failure paths don't do what the PR body says yet (details inline).
Minor notes:
sortedBy { it.index }ingetManualChannelUpdatePlanis a no-op —getChannelListiterates0..maxascending, so the output is already index-sorted.ManualChannelUpdateResult's return value is ignored by the only production caller; the tests could assert on theregisterRequestIdcapture instead.
e2e5d73 to
7ee77b9
Compare
|
@jamesarich Thanks, this was a good catch. I pushed a rebased update that tightens the ordered-save path around the cases you called out. The save plan is now computed inside the mutex against current canonical state, overlapping saves are serialized, batch success is held until enqueueing completes, enqueue failures surface an error, and interrupted batches reconcile the writes that were already sent instead of leaving app/device state silently divergent. I also added focused coverage for the premature-success path, overlapping saves, enqueue failure, stale-baseline saves, and interrupted reconciliation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt (1)
508-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct unit test for the interrupted/partial-write path of
applyManualChannelUpdatePlan.This test only covers the happy-path pacing behavior. The
onInterruptedbranch (partialappliedSettings/appliedWriteCounton mid-plan failure) is currently only exercised indirectly through theupdateChannelsintegration test at Line 438. A focused unit test callingapplyManualChannelUpdatePlandirectly with a failingwriteChannelwould pin down that contract independently of the view model's mutex/state-machine plumbing.🤖 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/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt` around lines 508 - 536, Add a focused unit test for the interrupted path of applyManualChannelUpdatePlan by making writeChannel fail on a mid-plan channel and asserting the onInterrupted result. Verify the partial appliedSettings and appliedWriteCount returned by applyManualChannelUpdatePlan when the write stops early, so this contract is covered independently of updateChannels and the view model mutex/state-machine logic.feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
1091-1091: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading field:
appliedSettingsholdsfinalSettings, not what was applied.The success-path result sets
appliedSettings = finalSettings, while the trackedappliedSettingsmutable list (andInterruptedManualChannelUpdate.appliedSettings) means "settings actually enqueued so far." A future caller readingManualChannelUpdateResult.appliedSettingswould get the intended target, not the applied state. Since the return value is currently unused byupdateChannels, consider renaming this field (e.g.finalSettings) to avoid a latent footgun.🤖 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/radio/RadioConfigViewModel.kt` at line 1091, The success result from ManualChannelUpdateResult is misleading because appliedSettings is populated with finalSettings rather than the settings actually applied so far. Update the ManualChannelUpdateResult type and its construction in RadioConfigViewModel so the field name reflects its meaning, such as finalSettings, and keep the InterruptedManualChannelUpdate.appliedSettings naming only for the in-flight applied state. Ensure all references to ManualChannelUpdateResult are updated consistently.
🤖 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/radio/RadioConfigViewModel.kt`:
- Around line 646-649: The backup guard in RadioConfigViewModel is showing the
restore-failure string during a backup/save flow, which is misleading to users.
Update the snackbar call in the empty-key check inside the backup path to use a
backup-specific or neutral resource instead of key_backup_restore_failed. If no
suitable string exists, add one and use it in this branch so the message matches
the action being performed.
---
Nitpick comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Line 1091: The success result from ManualChannelUpdateResult is misleading
because appliedSettings is populated with finalSettings rather than the settings
actually applied so far. Update the ManualChannelUpdateResult type and its
construction in RadioConfigViewModel so the field name reflects its meaning,
such as finalSettings, and keep the
InterruptedManualChannelUpdate.appliedSettings naming only for the in-flight
applied state. Ensure all references to ManualChannelUpdateResult are updated
consistently.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Around line 508-536: Add a focused unit test for the interrupted path of
applyManualChannelUpdatePlan by making writeChannel fail on a mid-plan channel
and asserting the onInterrupted result. Verify the partial appliedSettings and
appliedWriteCount returned by applyManualChannelUpdatePlan when the write stops
early, so this contract is covered independently of updateChannels and the view
model mutex/state-machine logic.
🪄 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: d19d5008-b4e8-46d7-b2a9-8a9758328a8b
📒 Files selected for processing (2)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt (1)
508-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct unit test for the interrupted/partial-write path of
applyManualChannelUpdatePlan.This test only covers the happy-path pacing behavior. The
onInterruptedbranch (partialappliedSettings/appliedWriteCounton mid-plan failure) is currently only exercised indirectly through theupdateChannelsintegration test at Line 438. A focused unit test callingapplyManualChannelUpdatePlandirectly with a failingwriteChannelwould pin down that contract independently of the view model's mutex/state-machine plumbing.🤖 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/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt` around lines 508 - 536, Add a focused unit test for the interrupted path of applyManualChannelUpdatePlan by making writeChannel fail on a mid-plan channel and asserting the onInterrupted result. Verify the partial appliedSettings and appliedWriteCount returned by applyManualChannelUpdatePlan when the write stops early, so this contract is covered independently of updateChannels and the view model mutex/state-machine logic.feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
1091-1091: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading field:
appliedSettingsholdsfinalSettings, not what was applied.The success-path result sets
appliedSettings = finalSettings, while the trackedappliedSettingsmutable list (andInterruptedManualChannelUpdate.appliedSettings) means "settings actually enqueued so far." A future caller readingManualChannelUpdateResult.appliedSettingswould get the intended target, not the applied state. Since the return value is currently unused byupdateChannels, consider renaming this field (e.g.finalSettings) to avoid a latent footgun.🤖 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/radio/RadioConfigViewModel.kt` at line 1091, The success result from ManualChannelUpdateResult is misleading because appliedSettings is populated with finalSettings rather than the settings actually applied so far. Update the ManualChannelUpdateResult type and its construction in RadioConfigViewModel so the field name reflects its meaning, such as finalSettings, and keep the InterruptedManualChannelUpdate.appliedSettings naming only for the in-flight applied state. Ensure all references to ManualChannelUpdateResult are updated consistently.
🤖 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/radio/RadioConfigViewModel.kt`:
- Around line 646-649: The backup guard in RadioConfigViewModel is showing the
restore-failure string during a backup/save flow, which is misleading to users.
Update the snackbar call in the empty-key check inside the backup path to use a
backup-specific or neutral resource instead of key_backup_restore_failed. If no
suitable string exists, add one and use it in this branch so the message matches
the action being performed.
---
Nitpick comments:
In
`@feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt`:
- Line 1091: The success result from ManualChannelUpdateResult is misleading
because appliedSettings is populated with finalSettings rather than the settings
actually applied so far. Update the ManualChannelUpdateResult type and its
construction in RadioConfigViewModel so the field name reflects its meaning,
such as finalSettings, and keep the
InterruptedManualChannelUpdate.appliedSettings naming only for the in-flight
applied state. Ensure all references to ManualChannelUpdateResult are updated
consistently.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Around line 508-536: Add a focused unit test for the interrupted path of
applyManualChannelUpdatePlan by making writeChannel fail on a mid-plan channel
and asserting the onInterrupted result. Verify the partial appliedSettings and
appliedWriteCount returned by applyManualChannelUpdatePlan when the write stops
early, so this contract is covered independently of updateChannels and the view
model mutex/state-machine logic.
🪄 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: d19d5008-b4e8-46d7-b2a9-8a9758328a8b
📒 Files selected for processing (2)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt
🛑 Comments failed to post (1)
feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt (1)
646-649: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong user-facing string on the backup-guard path.
This guard runs when a backup is attempted with empty keys, but it surfaces
key_backup_restore_failed("restore failed"), which is misleading during a save/backup action. Consider a dedicated backup-failed resource (or a more neutral message).🤖 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/radio/RadioConfigViewModel.kt` around lines 646 - 649, The backup guard in RadioConfigViewModel is showing the restore-failure string during a backup/save flow, which is misleading to users. Update the snackbar call in the empty-key check inside the backup path to use a backup-specific or neutral resource instead of key_backup_restore_failed. If no suitable string exists, add one and use it in this branch so the message matches the action being performed.
Apply manual channel saves through a single ordered channel update sequence instead of launching one coroutine per changed slot. The update plan is sorted by channel index so delete, re-add, and reorder-style edits enqueue radio writes deterministically while preserving each row's ChannelSettings payload, including its PSK. Register every returned request ID from the ordered write loop so existing packet response tracking remains in place. Local channel migration and cache replacement now run after the ordered send loop for local nodes, and the visible channel list is updated after the same sequence completes. Add focused ViewModel coverage that records write order and verifies changed channel writes do not overlap when setRemoteChannel suspends.
Apply a one-second pace between manual channel write requests so delete and reorder batches are not enqueued faster than the radio/admin path can materialize slot changes. The delay is applied only between writes and not after the final channel write. Extract the ordered write loop into a focused helper that keeps request ID registration beside each write and accepts a delay function for fast unit coverage. The existing updateChannels path still builds a sorted plan, writes from one coroutine, and performs local migration/cache updates after the ordered write sequence. Add coverage for pacing behavior, write order, request ID registration, and the no-delay-after-final-write contract.
Make manual channel save semantics explicit: channel writes are treated as an ordered batch, and canonical local channel state is updated only after every write request has been enqueued. If a write enqueue throws, the safeLaunch path reports the failure and leaves saved state unchanged rather than committing a partial delete or reorder result. Return a ManualChannelUpdateResult from the ordered write helper so callers and tests can inspect the packet IDs produced by a batch. Request IDs are still registered immediately after each write request to preserve response tracking for fast admin responses. Add focused coverage for mid-batch failure behavior and extend the pacing helper test to assert returned packet IDs while preserving immediate registration and no final delay.
Guard manual channel save batches with a mutex so overlapping updateChannels calls cannot interleave ordered write plans on the radio link. Keep canonical channel state updates inside the serialized batch after channel writes complete, preserving the existing all-or-no-local-commit behavior for enqueue failures. Strengthen ViewModel coverage for failed local-node saves by setting myNodeInfo, and add coverage for overlapping manual channel saves executing as serialized batches.
Addresses review nitpick: the channelA/B/C/D + old list setup was duplicated verbatim in two updateChannels tests. Extract into a fourChannelFixture() helper and destructure for named access, keeping the new/firstNew/secondNew lists test-local.
Diff queued manual channel saves against the current canonical channel list inside the serialization mutex so a second save cannot skip writes based on stale state. Hold the save dialog in Loading while the paced write loop is still registering packet IDs, surface enqueue failures as response errors, and reconcile any already-enqueued local channel writes in NonCancellable cleanup.
The updated detekt baseline in develop no longer suppresses the catch-all in updateChannels. Add an explicit @Suppress annotation matching the convention used elsewhere (DatabaseManager.withDb, BleOtaTransport.connect). Also restore the blank line between the last test and the fourChannelFixture helper that was lost during conflict resolution.
Rename ManualChannelUpdateResult.appliedSettings to finalSettings so the field name matches its meaning — the intended final settings after a successful ordered update. InterruptedManualChannelUpdate. appliedSettings keeps its name since it genuinely means "partial settings already applied before interruption." Add a focused unit test for applyManualChannelUpdatePlan's interrupted path: writeChannel fails on a mid-plan channel, the exception propagates, onInterrupted fires with the partial appliedSettings and appliedWriteCount reflecting only the writes completed before failure. Skipped: CodeRabbit flagged key_backup_restore_failed being shown in the backup guard (L646-649). That code is from develop's security-key backup feature, not part of this PR's channel-sequencing scope.
7ee77b9 to
4f6484a
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push — all four of my earlier findings are fixed, with tests:
- ✅ Premature Success — batch seeds
Loading(total)up front and routes mid-batch ACKs throughcompleteSetRequestOrProgressBatch()/incrementCompleted(); Success deferred to batch end. - ✅ Silent failure —
try/catchnowsendErrors →ResponseState.Error(asserted by a test). - ✅ Partial reconciliation —
reconcileInterruptedManualChannelUpdatein awithContext(NonCancellable)block, matchingapplyReplacementChannelSet. - ✅ Stale baseline — plan now computed inside
withLockfrom livechannelList, each batch commits unconditionally.
Requesting changes on a set of related issues the new batch state-machine introduced: it reaches into the shared response-state and can corrupt a concurrent non-batch admin flow (inline). If channel saves can never overlap another admin request on this screen these are benign, but that isn't enforced anywhere — worth scoping the batch bookkeeping to its own request-ids.
Minor carryovers: sortedBy { it.index } is still a no-op (getChannelList is already ascending), ManualChannelUpdateResult is still discarded by the only caller, and applyManualChannelUpdatePlan duplicates applyReplacementChannelSet's paced-write engine (incl. a second 1s constant).
Both blocking findings addressed: batch request-ids are now isolated (only the batch's own ids/timeouts are cleared on abort) and beginManualChannelBatch guards against clobbering a concurrent flow. Posting new non-blocking observations on the queued-saves commit separately.
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed at 6fbbddc — both things I blocked on are fixed, with tests:
- ✅
abortManualChannelBatch(batchRequestIds)now removes/cancels only the batch's own request-ids and timeouts; unrelated in-flight requests survive (aborted manual batch preserves unrelated pending request). - ✅
beginManualChannelBatchnow returns false and bails when a non-batch request is pending/registering, instead of clobbering itsroute/Loadingstate.
Dismissing my changes-requested. Three smaller things the allow queued manual channel saves commit introduced — none blocking, but worth a look:
- Silent save drop (worth checking).
hasPendingRequestRegistration()is true wheneverrequestIds.isEmpty() && manualChannelBatchRequestIds.isEmpty() && responseState is Loading, so a Save tapped while the pane is in a bare Loading state (e.g. right afterensureLoadingForRemote(), before any request registers) is dropped with only a log — no user feedback. If that state is reachable with Save enabled, the user retries blind. - Progress-bar mix (cosmetic). Queue a second save while the first batch's ACKs are still outstanding: the first batch's un-acked ids count as 'related', so the second
beginManualChannelBatchproceeds and itsLoading(total)absorbs the leftover ids — completed/total then mixes both batches. Writes stay ordered under the mutex, so no data loss, just a wrong count. - No commit rollback (low likelihood). If
replaceAllSettingsthrows aftermigrateChannelsByPSKsucceeds in the commit,packetRepositoryis migrated whileradioConfigRepository/state aren't, with no reconciliation. Both are local DB writes so unlikely, but there's no rollback.
2 participants
Overview
This PR makes manual channel saves deterministic by writing channel changes in order instead of allowing overlapping write batches.
Previously, multi-channel edits could enqueue several radio writes without pacing or serialization. On constrained links, that made save, delete, and reorder flows more fragile and allowed overlapping saves to interleave.
This change builds an ordered channel update plan, applies writes sequentially, keeps the response state tied to the full batch, and reconciles already-enqueued writes if a batch is interrupted.
Key Changes
Testing
git diff --checkpasses.Migration Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests