refactor(qr): apply channel imports atomically via edit-settings transaction

[jamesarich]

Why

QR/URL channel import currently replays each channel as an individual fire-and-forget set_channel write, then writes LoRa config separately. Because those writes aren't atomic, the path has accreted defensive scaffolding across several recent PRs (#5999, #6072): 1s per-channel pacing, 2s + 2s LoRa settle delays, and interrupted-replacement cache reconciliation. A mid-import disconnect can still leave the node half-configured, and a full import costs ~12s of artificial delay.

The firmware already exposes a begin/commit edit-settings transaction (begin_edit_settings / commit_edit_settings), and InstallProfileUseCase already uses it to install owner / config / module-config. Inside a transaction firmware defers disk persistence, radio reload/reconfiguration, and reboot until commit — which is exactly the expensive, race-prone work the old pacing/settle delays were guarding against. (Per @jeremiah-k's firmware trace: set_channel still writes each channel into firmware's in-memory table on arrival, so it isn't a full staging of channel state — but the costly persist/reload path runs once at commit.)

Credit to @jeremiah-k, who authored the normalization / dedup / slot-role logic this builds on (#5999, #6013, #6056) and reviewed this PR in depth — that logic (normalizeReplacementSettings, getChannelReplacementList, getChannelPreviewForAdd, channelIdentity) is unchanged and still drives the new path. His manual-editor PRs (#6076/#6077) are untouched. His review also caught a real Android-cache bug — see history below.

🛠️ Changes

• Add AdminController.editLocalSettings { } — local-node editSettings, mirroring the existing setLocalConfig / setLocalChannel naming.
• Merge applyReplacementChannelSet + applyImportedLoraConfigAfterChannelReplacement into one importChannelSet, called once at each import site (ChannelViewModel, ScannedQrCodeViewModel). Channels and LoRa are written inside one transaction and committed atomically — hardware-verified as a single save-to-disk pass + single reboot (see Testing).
• Remove per-channel pacing, both LoRa settle delays, and reconcileInterruptedReplacement.
• (review fix) Transactional setChannel no longer mirrors to the local cache per slot. importChannelSet owns the cache and writes it once after commit (replaceAllSettings), so the local channel cache is commit-shaped and an interrupted import leaves it untouched. Profile install is unaffected — it installs no channels.

🧹 Result

• Faster: import goes from ~12s of artificial delay (8×1s + 2×2s) to effectively instant.
• Safer: an interrupted import never commits (node keeps its stored config after reboot) and no longer partially mutates the local channel cache. (The imported LoRa config still writes through the cache-mirroring setConfig — a single trailing write, called out in the KDoc; making setConfig transaction-aware is future work, per @jeremiah-k.)
• Smaller: two functions → one, two call-site steps → one.

Out of scope (intentional)

The manual channel editor (applyManualChannelUpdatePlan, MANUAL_CHANNEL_WRITE_DELAY) is left as-is. It's ACK-gated on per-write firmware routing responses that drive its progress bar and 30s per-write timeout, so it can't move to the fire-and-forget transaction without losing that UX.

Testing Performed

• :core:service:allTests — green, incl. new RadioControllerImplTest.editLocalSettingsChannelWritesDoNotMirrorToLocalCache, which asserts against the real controller + mock repo that a transactional setChannel does not call updateChannelSettings (regression guard for the cache leak Jeremiah caught).
• :core:ui:allTests, :feature:settings:allTests — green. Import + call-site tests rewritten for the transactional semantics.
• spotlessCheck, detekt — clean. assembleDebug — app-wide compile clean (verifies the AdminController interface addition across all modules).
• ✅ Hardware-validated on a physical Heltec V3 (BLE) with an independent MCP serial oracle on the same radio, driven via real meshtastic.org/e/#… import URLs — no mocks on either side of the app boundary (full writeup):
  • Atomicity (3dd05768a): all 8 set_channel writes + the LoRa write logged Delay save … until the open transaction is committed; commit → a single save-to-disk (config + channels + device + module together) → one reboot; post-reboot get_channel_url / get_config lora round-tripped byte-for-byte (NVS-persisted, not just RAM). Stimulus→reboot was BLE-pacing-bound, not delay()-bound — consistent with ~12s → ~instant.
  • Interrupted import — the review fix (79fdc896f): force-stopping the app ~3s in showed Begin transaction → 1 of 8 writes → BLE disconnected, with no commit / save / reboot; radio untouched, and the app cache showed no partial mutation on relaunch — the exact leak the pre-fix commit would have produced.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added local-only batch editing for admin settings, enabling direct edits to the local device without selecting a destination.
  • Streamlined channel import to apply the full replacement inside a single transaction.
• Bug Fixes
  • Channel imports no longer partially update local cache/config if a channel write fails mid-session.
  • Improved channel role mapping and slot-limit handling during import.
  • LoRa settings are written only when they differ, avoiding unnecessary updates.
• Tests
  • Added regression coverage for local transactional behavior and failure scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds local-node transactional editing, rewrites channel import into a single edit transaction, updates consumers to use it, and expands tests to cover failure, normalization, and LoRa-config behavior.

Changes

Local settings transaction and channel import rework

Layer / File(s)	Summary
editLocalSettings interface and service implementation core/repository/.../AdminController.kt, core/service/.../AdminControllerImpl.kt	Adds editLocalSettings to the interface, implements it for the local node, and changes transactional setChannel to send admin messages directly.
FakeRadioController and service regression test core/testing/.../FakeRadioController.kt, core/service/src/commonTest/.../RadioControllerImplTest.kt	Adds failure simulation and recorded-config behavior to the fake controller, implements local-edit delegation, and adds a regression test for non-mirroring local channel writes.
importChannelSet transactional import implementation core/ui/.../util/ProtoExtensions.kt	Replaces the paced replacement/apply helpers with importChannelSet, which writes channels and optional lora_config in one transaction before updating the cache once.
importChannelSet test coverage core/ui/src/commonTest/.../ProtoExtensionsTest.kt	Updates tests for slot mapping, mid-session failure handling, oversized-import rejection, normalization, and transactional lora_config writes.
Consumer wiring to importChannelSet core/ui/.../qr/ScannedQrCodeViewModel.kt, feature/settings/.../ChannelViewModel.kt	Updates both view models to call importChannelSet instead of the removed replacement/apply helpers.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ChannelViewModel
  participant importChannelSet
  participant AdminController
  participant RadioConfigRepository

  ChannelViewModel->>importChannelSet: importChannelSet(channelSet, radioController, radioConfigRepository)
  importChannelSet->>AdminController: editLocalSettings { ... }
  loop for each replacement channel
    importChannelSet->>AdminController: setChannel(channel)
  end
  alt lora_config differs
    importChannelSet->>AdminController: setConfig(Config(lora))
  end
  AdminController-->>importChannelSet: transaction committed
  importChannelSet->>RadioConfigRepository: replaceAllSettings(normalizedSettings)
  RadioConfigRepository-->>ChannelViewModel: local cache updated once

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.05% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: atomic channel import via an edit-settings transaction in the QR flow.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/festive-poincare-cd0372

---

_{Comment @coderabbitai help to get the list of available commands.}

[jamesarich]

@jeremiah-k — GitHub won't let me add you to the formal Reviewers list (that needs collaborator access; you're read-only on the repo), so flagging here instead: I'd really value your review on this one, since it supersedes the transport layer from #5999 and #6072.

The single thing I most want your take on: is firmware safe receiving un-paced set_channel writes inside a begin_edit_settings/commit_edit_settings session? I'm leaning on the InstallProfileUseCase precedent for that, but if you paced the writes because you saw firmware apply them eagerly, that's exactly the gotcha to catch here. Your dedup/normalization/slot-role logic is kept unchanged and still drives the new path.

[jeremiah-k]

@jamesarich Thanks for tagging me. I pulled the Android PR head and traced the firmware path from the bundle I had. I think the direction is good, but I’d narrow a couple of claims before un-drafting.

Short version:

• I think firmware is probably safe receiving un-paced set_channel writes inside a begin_edit_settings / commit_edit_settings session for the hardware-timing concern.
• The transaction appears to defer disk persistence, reload/reconfigure, and reboot until commit.
• It does not defer all channel side effects: the in-memory channel table is still updated as each set_channel arrives.
• The main issue I see is Android-side: the real controller still updates radioConfigRepository per channel write inside the edit session, so the “interrupted import leaves cache untouched” claim is not true yet.

Firmware side

The set_channel path is still eager for in-memory channel state:

set_channel
→ AdminModule::handleSetChannel(...)
→ channels.setChannel(cc)
→ channels.onConfigChanged()
→ saveChanges(SEGMENT_CHANNELS, false)

handleSetChannel(...) calls channels.setChannel(cc), and Channels::setChannel(...) directly overwrites the channel slot. It also demotes any existing primary if the incoming channel is primary. So channel table mutation is not staged until commit.

That said, I do not think this means the old pacing is still required.

Channels::onConfigChanged() looks relatively lightweight in the firmware I checked: it fixes up channels, recalculates primaryIndex, and may start MQTT if needed. I did not see it doing the expensive LoRa reload/reconfigure/persist path.

The heavier path is behind saveChanges(...), and that is transaction-aware:

void AdminModule::saveChanges(int saveWhat, bool shouldReboot)
{
    if (!hasOpenEditTransaction) {
        service->reloadConfig(saveWhat);
    } else {
        LOG_INFO("Delay save of changes to disk until the open transaction is committed");
    }
    if (shouldReboot && !hasOpenEditTransaction) {
        reboot(DEFAULT_REBOOT_SECONDS);
    }
}

Then commit_edit_settings clears the transaction flag and calls saveChanges(...) once for the full config set.

So my firmware-side read is:

The PR can say edit-settings defers disk persistence, radio reload/reconfiguration, and reboot until commit.

I would avoid saying or implying that set_channel itself is fully staged or that there are no channel side effects before commit. The in-memory channel table and primaryIndex can change as packets arrive.

For the specific un-paced-write question, I do not see a firmware reason that an 8-channel burst should be unsafe from a hardware/reconfigure standpoint, because the expensive work appears deferred. The per-packet work is in-memory channel replacement, channel fixup, primary-index recalculation, possible MQTT start, warning/clamp handling, and delayed-save logging.

So I think the InstallProfileUseCase precedent is directionally useful, but not a perfect proof for channels, since channels do have their own eager handler.

Android side

This is the part I’d want fixed.

The new import shape is good in principle:

radioController.editLocalSettings {
    for (channel in replacements) {
        setChannel(channel)
    }
    importedLoraConfig?.let { setConfig(Config(lora = it)) }
}

withContext(NonCancellable) {
    radioConfigRepository.replaceAllSettings(normalizedSettings)
}

That should mean: send all writes inside the edit session, then update the local cache once after the session succeeds.

But the real controller still has an eager local cache update inside the transaction.

EditSettingsSession.setChannel(...) delegates to setRemoteChannel(...):

override suspend fun setChannel(channel: Channel) =
    setRemoteChannel(destNum, channel, commandSender.generatePacketId())

And setRemoteChannel(...) still does this for the local node:

override suspend fun setRemoteChannel(destNum: Int, channel: Channel, packetId: Int) {
    commandSender.sendAdmin(destNum, packetId) { AdminMessage(set_channel = channel) }
    if (destNum == nodeManager.myNodeNum.value) {
        scope.handledLaunch { radioConfigRepository.updateChannelSettings(channel) }
    }
}

Since editLocalSettings(...) uses myNodeNum, every setChannel(...) inside the transaction can still fire radioConfigRepository.updateChannelSettings(channel) immediately, per slot, before commit.

So if the edit session fails or the connection drops after one or two channel writes, the final replaceAllSettings(normalizedSettings) will not run, but those eager per-channel cache updates may already have launched. That can still leave Android’s local cache partially mutated.

The current fake test does not really prove the interrupted-import claim, because FakeRadioController.setRemoteChannel(...) records into localChannels but does not model the real controller’s radioConfigRepository.updateChannelSettings(channel) side effect. A test can pass there while the real implementation still leaks per-slot cache updates.

Suggested fix

I would not ask for a firmware change here. Based on the firmware path I checked, I do not think deferring channels.onConfigChanged() is necessary for the un-paced-write safety question.

I would fix the Android cache behavior instead:

1. Make transactional setChannel(...) send the set_channel admin packet without doing the local-node radioConfigRepository.updateChannelSettings(channel) side effect.
2. Keep replaceAllSettings(normalizedSettings) as the single post-success cache update for QR replacement.
3. If the edit session fails before commit, leave the cache alone.
4. Adjust the fake/test so the interrupted-import test would fail if a per-channel cache update leaks back in.

In other words: firmware’s expensive work is commit-shaped, but Android’s local cache is still slot-shaped. I think the cache should be commit-shaped for this import path too.

I’d also soften the PR wording/KDoc to something like:

The edit-settings transaction defers disk persistence, radio reload/reconfiguration, and reboot until commit. Channel slots are still written into firmware’s in-memory channel table as each set_channel arrives, but the expensive persist/reconfigure path runs once at commit.

Recommended stance

I’m not opposed to the direction. I think the transaction likely addresses the hardware-timing concern that the old pacing was trying to avoid.

I just would not call it ready exactly as written yet.

Before un-drafting, I’d ask for:

• wording/KDoc narrowed to persist/reload/reconfigure/reboot, not full channel staging;
• Android cache fix so per-channel updateChannelSettings(...) does not fire inside the local edit-settings channel transaction;
• test coverage that models or prevents the real eager-cache side effect;
• hardware validation with full 8-channel replacement plus LoRa config change.

My direct answer:

I think firmware is safe receiving un-paced set_channel writes inside begin_edit_settings / commit_edit_settings for the hardware-timing concern, because disk persistence and radio reload/reconfiguration appear deferred to commit.

But it is not fully atomic for live in-memory channel state, and the current Android implementation is not atomic for local cache state because of the eager updateChannelSettings(...) side effect inside setRemoteChannel(...).

I’d be comfortable with the transport direction once the Android cache side is fixed and the PR wording is narrowed.

[jamesarich]

@jeremiah-k This is a fantastic review — thank you for pulling the head and actually tracing the firmware path. You caught a real bug, and you're right on the wording. Pushed 79fdc89 addressing it.

The Android cache leak — fixed. You nailed it: EditSettingsSession.setChannel delegated to setRemoteChannel, which fires radioConfigRepository.updateChannelSettings(channel) per slot for the local node, so an interrupted import could leave the cache partially mutated — and the fake test passed without modeling that side effect. Fix:

// transactional channel write — no per-slot local-cache mirror
override suspend fun setChannel(channel: Channel) =
    commandSender.sendAdmin(destNum) { AdminMessage(set_channel = channel) }

importChannelSet now owns the cache and writes it once after commit (replaceAllSettings), so it's commit-shaped: interrupt before commit → cache untouched. This only affects the import path — InstallProfileUseCase installs no channels, so its eager mirroring (which it relies on) is untouched.

Test — moved to where it can actually catch this. You were right that the extension-level fake test couldn't prove the claim. Added RadioControllerImplTest.editLocalSettingsChannelWritesDoNotMirrorToLocalCache, which runs the real AdminControllerImpl against a mock repo and asserts verifySuspend(exactly(0)) { radioConfigRepository.updateChannelSettings(any()) } after a two-channel editLocalSettings. Re-introducing per-slot mirroring makes it fail. (I left the extension test as orchestration coverage — session throws → replaceAllSettings not reached.)

Wording — narrowed. KDoc now says the transaction defers disk persistence, radio reload/reconfiguration, and reboot to commit, and explicitly notes firmware still writes each set_channel into its in-memory channel table on arrival — no longer implies full channel staging. PR body updated to match.

On your four asks: (1) ✅ non-mirroring transactional setChannel; (2) ✅ replaceAllSettings stays the single post-success cache write; (3) ✅ fail-before-commit leaves cache alone; (4) ✅ real-controller regression test. Plus wording ✅.

Still open — the one thing I can't do from here: hardware validation (full 8-channel replacement + a LoRa change on a real device). Your firmware trace + the InstallProfileUseCase precedent both say the un-paced in-session burst is safe because the expensive path is commit-deferred, but I agree that's the gate before un-drafting. Keeping it draft until someone runs it on hardware. If you've got a device handy I'd take that data gratefully; otherwise I'll line it up.

Thanks again — this is exactly the firmware-side context I was hoping for.

[jamesarich]

Bench test: real hardware, both revisions

Tested against a physical Heltec V3 DUT (BLE) with an independent MCP serial oracle on the same radio, driving the app via adb-fired https://meshtastic.org/e/#... channel URLs (the real QR-import wire format) crafted offline with the meshtastic protobufs — no mocks on either side of the app boundary.

Rev 1 (3dd05768a) — atomicity claim

• Every one of the 8 set_channel writes and the LoRa config write logged Delay save of changes to disk until the open transaction is committed.
• Commit transaction for edited settings → a single Save changes to disk pass wrote config.proto + channels.proto + device.proto + module.proto together → one Reboot in 7 seconds.
• Post-reboot, get_channel_url / get_config lora round-tripped byte-for-byte against the crafted input — confirms NVS persistence, not just RAM.
• Wall-clock stimulus→reboot was dominated by BLE packet pacing, not artificial delay() calls — consistent with the 12s→instant claim.
• Non-blocking, unrelated finding: hit a CORRUPT HEAP / Record critical error 7 / numFreqSlots: 4294967295 during radio reinit — traced to my own test artifact (crafted LoRaConfig left bandwidth/spread_factor unset with use_preset=false), which hits a pre-existing firmware divide-by-zero-shaped path in SX126xInterface.cpp:223. Unrelated to this PR's transaction logic; disappeared once I set an explicit bandwidth in later tests.

Rev 2 (79fdc896f, addressing @jeremiah-k's review) — the actual fix

Re-ran the full happy path on the updated commit as a regression check (new channels + LoRa hop_limit change, one commit, one reboot, correct round-trip, app UI matched) — no regression from the non-mirroring change.

Then specifically exercised the bug this commit fixes: fired a distinct import, confirmed it in the UI, and force-stop'd the app ~3s in — well before commit. Firmware log showed the interrupted shape exactly: Begin transaction → only 1 of 8 channel writes landed → BLE disconnected from the kill, with no Commit transaction, no Save to disk, no Reboot.

• Radio oracle: untouched, as expected on both revisions (NVS was never at risk here).
• App cache oracle — what actually changed: relaunched the app and checked the Channels screen. It showed exactly the pre-interruption channel set, no trace of the interrupted import. On the pre-fix commit, setChannel inside the edit session mirrored each write into the local cache via setRemoteChannel immediately, so this same interrupted sequence would have left the app's UI showing a partially-mutated channel list even though the radio itself was never touched — the exact gap the review comment identified. Confirmed against a real interrupted import + real process kill + real radio, which the mock-based editLocalSettingsChannelWritesDoNotMirrorToLocalCache unit test can assert at the call level but can't fully exercise end-to-end.

Verdict

Both the original atomicity claim (radio: one reboot, no partial NVS state) and the follow-up fix (app: no partial local-channel-cache state under interruption) hold up against real hardware in both the success and interrupted-mid-transaction paths.

[jamesarich]

Hardware validation passed on a physical Heltec V3 (full writeup above) — marking ready for review.

Both claims held up on metal:

• Atomicity: all 8 set_channel writes + the LoRa write deferred (Delay save … until the open transaction is committed); commit → one save-to-disk → one reboot; byte-for-byte NVS round-trip post-reboot.
• The cache fix: interrupted import (force-stop before commit) → firmware showed Begin transaction → 1 of 8 writes → BLE disconnected, no commit/save/reboot, and the app cache showed no partial mutation on relaunch.

@jeremiah-k — the second result is your review point, confirmed end-to-end: on the pre-fix commit that same interrupted sequence would have left the app's channel list partially mutated (radio untouched, but UI wrong), because setChannel mirrored per slot via setRemoteChannel. The non-mirroring fix closes it. Thanks again for catching it — the mock unit test asserts it at the call level, but your instinct that it needed a real interrupted-import-on-real-radio exercise was right.

One unrelated finding to flag (not this PR): a crafted LoRaConfig with use_preset=false and unset bandwidth/spread_factor tripped a firmware divide-by-zero-shaped path in SX126xInterface.cpp:223 (numFreqSlots: 4294967295 → CORRUPT HEAP) during radio reinit. It was a test artifact, but it means a hand-crafted import URL could in principle crash a peer's radio — worth a firmware hardening issue upstream, and I've flagged a possible app-side import guard as a follow-up. Happy to file the firmware issue if you want.

[jeremiah-k]

@jamesarich Thanks, this is exactly the hardware validation I was hoping to see.

The Rev 1 results answer my firmware concern. As you noted:

Every one of the 8 set_channel writes and the LoRa config write logged Delay save of changes to disk until the open transaction is committed.
Commit transaction for edited settings → a single Save changes to disk pass wrote config.proto + channels.proto + device.proto + module.proto together → one Reboot in 7 seconds.

That lines up with what I saw in the firmware path: per-channel set_channel still mutates RAM as packets arrive, but the expensive persist/reload/reboot path is clearly commit-shaped. The byte-for-byte post-reboot round-trip you mentioned:

Post-reboot, get_channel_url / get_config lora round-tripped byte-for-byte against the crafted input

is a strong confirmation that the final stored state landed correctly.

The Rev 2 interrupted-import test also covers the Android cache issue I called out. This part is especially important:

Begin transaction → only 1 of 8 channel writes landed → BLE disconnected from the kill, with no Commit transaction, no Save to disk, no Reboot.

and then on the app side:

Relaunched the app and checked the Channels screen. It showed exactly the pre-interruption channel set, no trace of the interrupted import.

That’s the end-to-end behavior we want. The unit test guards the call-level behavior, and this bench test confirms the real app/radio path behaves the same way under interruption.

Your summary matches that nicely:

Both the original atomicity claim (radio: one reboot, no partial NVS state) and the follow-up fix (app: no partial local-channel-cache state under interruption) hold up against real hardware

The heap corruption note also sounds correctly scoped:

traced to my own test artifact ... hits a pre-existing firmware divide-by-zero-shaped path

Agreed that this looks unrelated to the transaction logic, but definitely worth a separate firmware hardening issue.

From my side, with the wording narrowed, the channel-cache mirroring fixed, and this real-hardware success/interruption coverage, I’m comfortable approving this PR.

Only small caveat I’d keep in mind for future work: transactional setConfig(...) still has the normal local-cache mirror behavior, so I’d avoid broad “all local config cache is atomic” wording unless that path is made transaction-aware too. But for this PR’s channel replacement/cache claim, this looks good to me.

[coderabbitai]

🧹 Nitpick comments (1)

core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt (1)

372-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Tighten the sendAdmin call-count assertion.

atLeast(2) is trivially satisfied and won't catch a regression where extra/duplicate packets are sent (e.g., a duplicate begin/commit, or an accidental double-write per channel). The transaction here sends exactly 4 packets (begin + 2 writes + commit).

♻️ Proposed tightening

-        verifySuspend(atLeast(2)) { commandSender.sendAdmin(any(), any(), any(), any()) }
+        verifySuspend(exactly(4)) { commandSender.sendAdmin(any(), any(), any(), any()) }

🤖 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
`@core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt`
around lines 372 - 390, The test in
RadioControllerImplTest.editLocalSettingsChannelWritesDoNotMirrorToLocalCache
uses a too-weak sendAdmin assertion; tighten it to verify the exact transaction
packet count instead of atLeast(2). Keep the existing
radioConfigRepository.updateChannelSettings(exactly(0)) check, and update the
sendAdmin verification around editLocalSettings/importChannelSet behavior so it
fails if begin/commit or either channel write is duplicated.

🤖 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
`@core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt`:
- Around line 372-390: The test in
RadioControllerImplTest.editLocalSettingsChannelWritesDoNotMirrorToLocalCache
uses a too-weak sendAdmin assertion; tighten it to verify the exact transaction
packet count instead of atLeast(2). Keep the existing
radioConfigRepository.updateChannelSettings(exactly(0)) check, and update the
sendAdmin verification around editLocalSettings/importChannelSet behavior so it
fails if begin/commit or either channel write is duplicated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 73a6d9eb-f024-450a-a3cf-5b73bb511ed3

📥 Commits

Reviewing files that changed from the base of the PR and between 7b5ab05 and 79fdc89.

📒 Files selected for processing (8)

• core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.kt
• core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt
• core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt
• core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt
• core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/qr/ScannedQrCodeViewModel.kt
• core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/ProtoExtensions.kt
• core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/ProtoExtensionsTest.kt
• feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/channel/ChannelViewModel.kt

[jamesarich]

@coderabbitai Good catch — tightened editLocalSettingsChannelWritesDoNotMirrorToLocalCache to verifySuspend(exactly(4)) (begin + 2 channel writes + commit) in b4eafa4; confirmed passing on JVM + androidHostTest. It now fails on a duplicated begin/commit or a double-write per channel.

🤖 Addressed by Claude Code

[jamesarich]

@jeremiah-k Thank you — appreciate the firmware trace and the approval. Acted on your setConfig caveat in b4eafa4: scoped the importChannelSet KDoc (and PR body) to the channel cache, and explicitly documented that the imported LoRa config still writes through the cache-mirroring setConfig — a single trailing write, with making setConfig transaction-aware called out as future work. No broad "all config cache is atomic" wording. Thanks again for the depth here.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@jamesarich Confirmed, thanks — exactly(4) is the right tightening here since it pins down the exact call sequence (begin, 2 writes, commit) rather than just checking that mirroring didn't happen. Glad it's green on both JVM and androidHostTest.