feat(firmware): Confirm local firmware files before flashing#6083
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR changes local firmware handling to a select-then-confirm flow. It adds filename resolution, validation, pending-selection state, confirmation UI, updated strings, and expanded tests for bundle extraction, cleanup, and cancellation. ChangesLocal firmware confirmation flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FirmwareUpdateScreen
participant FirmwareUpdateViewModel
participant FirmwareFileHandler
participant FirmwareUpdateManager
User->>FirmwareUpdateScreen: pick local firmware file
FirmwareUpdateScreen->>FirmwareUpdateViewModel: prepareLocalFirmwareFile(uri)
FirmwareUpdateViewModel->>FirmwareFileHandler: getDisplayName(uri)
FirmwareFileHandler-->>FirmwareUpdateViewModel: filename
FirmwareUpdateViewModel->>FirmwareFileHandler: validateLocalFirmwareFileName(...)
FirmwareFileHandler-->>FirmwareUpdateViewModel: validation result
FirmwareUpdateViewModel->>FirmwareUpdateViewModel: set pendingLocalFirmwareFile
FirmwareUpdateScreen->>User: show confirmation dialog
User->>FirmwareUpdateScreen: confirm or dismiss
FirmwareUpdateScreen->>FirmwareUpdateViewModel: confirmLocalFirmwareFile() / dismissLocalFirmwareFile()
FirmwareUpdateViewModel->>FirmwareFileHandler: validatePendingLocalFirmwareFile(...)
FirmwareFileHandler-->>FirmwareUpdateViewModel: valid or invalid reason
FirmwareUpdateViewModel->>FirmwareUpdateManager: startUpdate(uri, pendingArtifact)
FirmwareUpdateManager-->>FirmwareUpdateViewModel: update state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
feature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt (1)
599-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test or add coverage for the non-Ready cleanup path
cancelUpdate()clearspendingLocalFirmwareFile, soconfirmLocalFirmwareFile()returns at the null guard here and never reachescleanupPendingLocalFirmwareArtifact(...). This test only covers the no-op-after-cancel case; add a separate case with a still-set pending selection and non-Ready state, or rename it to match the asserted behavior.🤖 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/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt` around lines 599 - 623, The test currently named confirmLocalFirmwareFile cleans up artifact when state is not Ready only verifies the no-op path after cancelUpdate clears pendingLocalFirmwareFile, so it never exercises cleanupPendingLocalFirmwareArtifact(...) in confirmLocalFirmwareFile. Either rename this test to reflect the null-guard/no-op behavior, or add a separate case in FirmwareUpdateViewModelFileTest that keeps pendingLocalFirmwareFile set while moving the ViewModel state away from Ready so the cleanup path is actually covered.feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.kt (1)
817-819: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate target-fallback logic.
effectiveTarget()(platformioTarget.ifEmpty { hwModelSlug }) duplicates the identical fallback already used inJvmFirmwareFileHandler.extractFromZipFile(hardware.platformioTarget.ifEmpty { hardware.hwModelSlug }, feature/firmware/src/jvmMain/kotlin/org/meshtastic/feature/firmware/JvmFirmwareFileHandler.kt:217), and is likely re-implemented again inside the validation logic inFirmwareFileHandler.ktthat this ViewModel calls into. Worth consolidating into a singleDeviceHardwareextension in the common model to avoid future divergence between the value shown to users and the value actually used for matching.♻️ Suggested consolidation
- /** pioEnv-aware target: falls back to the hwModel slug when [DeviceHardware.platformioTarget] is unset. */ - private fun DeviceHardware.effectiveTarget(): String = platformioTarget.ifEmpty { hwModelSlug } + // Replace with a shared `DeviceHardware.effectiveTarget` extension defined once in the common model + // module, and reuse it from JvmFirmwareFileHandler / FirmwareFileHandler as well.🤖 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/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.kt` around lines 817 - 819, Consolidate the target fallback logic so it is defined once on DeviceHardware and reused everywhere instead of being duplicated in FirmwareUpdateViewModel.effectiveTarget, JvmFirmwareFileHandler.extractFromZipFile, and the validation path in FirmwareFileHandler. Move the platformioTarget.ifEmpty { hwModelSlug } behavior into a shared common extension or helper on DeviceHardware, then update the ViewModel and file handlers to call that shared symbol so the displayed target and matching logic always stay in sync.
🤖 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/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareFileHandler.kt`:
- Around line 142-164: The validation order in validatePendingLocalFirmwareFile
should prioritize stale selection detection before rechecking the filename
against the new Ready state. Move the currentTarget/ConfirmationContextChanged
checks ahead of validateLocalFirmwareFileName so that when updateMethod or
address has changed since selection, the function returns
LocalFirmwareFileValidation.Invalid(LocalFirmwareFileValidationReason.ConfirmationContextChanged)
instead of a misleading method-specific filename error. Keep the existing
TargetMismatch handling and only run validateLocalFirmwareFileName after
confirming the context still matches.
---
Nitpick comments:
In
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.kt`:
- Around line 817-819: Consolidate the target fallback logic so it is defined
once on DeviceHardware and reused everywhere instead of being duplicated in
FirmwareUpdateViewModel.effectiveTarget,
JvmFirmwareFileHandler.extractFromZipFile, and the validation path in
FirmwareFileHandler. Move the platformioTarget.ifEmpty { hwModelSlug } behavior
into a shared common extension or helper on DeviceHardware, then update the
ViewModel and file handlers to call that shared symbol so the displayed target
and matching logic always stay in sync.
In
`@feature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt`:
- Around line 599-623: The test currently named confirmLocalFirmwareFile cleans
up artifact when state is not Ready only verifies the no-op path after
cancelUpdate clears pendingLocalFirmwareFile, so it never exercises
cleanupPendingLocalFirmwareArtifact(...) in confirmLocalFirmwareFile. Either
rename this test to reflect the null-guard/no-op behavior, or add a separate
case in FirmwareUpdateViewModelFileTest that keeps pendingLocalFirmwareFile set
while moving the ViewModel state away from Ready so the cleanup path is actually
covered.
🪄 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: da917a8c-b6ab-47ca-ae1b-73dd4146114a
📒 Files selected for processing (11)
.skills/compose-ui/strings-index.txtcore/resources/src/commonMain/composeResources/values/strings.xmlfeature/firmware/src/androidMain/kotlin/org/meshtastic/feature/firmware/AndroidFirmwareFileHandler.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareFileHandler.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateActions.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateScreen.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/CommonFirmwareRetrieverTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/IsValidFirmwareFileTest.ktfeature/firmware/src/jvmMain/kotlin/org/meshtastic/feature/firmware/JvmFirmwareFileHandler.ktfeature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt
efa29bd to
0fdba9a
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. The confirmation-context ordering fix landed (context change now surfaces "reselect" instead of an extension error), and most temp-file orphan paths are cleaned up — thanks. But two confirmed holes defeat the feature's core purpose (flashing the right firmware), so I'm requesting changes. Details inline. Minor: one new string (firmware_update_invalid_local_file_detail) has unescaped "%1$s" quotes while its sibling escapes them — sort-strings.py/lint may flag it.
| LocalFirmwareResolution.Invalid(reason = fallbackReason, fileName = fileName) | ||
| } else { | ||
| val extractedArtifact = | ||
| safeCatching { fileHandler.extractFirmware(uri, state.deviceHardware, payloadExtension) } |
There was a problem hiding this comment.
This extracts with payloadExtension (".bin") and no preferredFilename, so AndroidFirmwareFileHandler falls back to minByOrNull { it.first.name.length } and picks the shorter plain firmware-<target>-<ver>.bin. On pre-2.7.17 bundles that's the merged bootloader+partition+app image FirmwareRetriever's own comment says esp_ota_end rejects — it deliberately prefers the manifest then -update.bin. Reuse that preference chain (pass preferredFilename) so a side-loaded official bundle doesn't extract an un-flashable image that then passes validation and gets confirmed.
| } | ||
|
|
||
| private fun validateTargetMatch(fileName: String, target: String, fileExtension: String): LocalFirmwareFileValidation = | ||
| if (isValidFirmwareFile(fileName, target, fileExtension)) { |
There was a problem hiding this comment.
The target match here delegates to isValidFirmwareFile, whose regex .*[-_]<target>[-_.].* accepts delimiter-bounded superstrings: a tbeam device validates firmware-tbeam-s3-core-*.bin as Valid (real colliding pairs in device_hardware.json: tbeam/tbeam-s3-core, t-echo/t-echo-lite, t-deck/t-deck-pro, nano-g1/nano-g1-explorer). Since this feature exists to block wrong-board files, the match needs to require the target be immediately followed by a version/extension delimiter (i.e. anchor the tail), not just appear delimiter-bounded.
| } | ||
| } | ||
|
|
||
| private suspend fun resolveLocalFirmwareBundle( |
There was a problem hiding this comment.
Bundle resolution here runs getDisplayName + a full multi-MB zip extraction with no user-visible progress — the old path set Processing(firmware_update_extracting) but it was removed and the string isn't even imported. The screen sits at Ready during extraction; a user seeing nothing happen may re-tap and spawn a second prepare. Set an extracting/Processing state around the resolve.
| // The user selected the file under a different connection (method or address) than the | ||
| // one active now — the file may still match the target, but the confirmation context is stale. | ||
| currentState.updateMethod != pendingFile.updateMethod || currentState.address != pendingFile.address -> | ||
| LocalFirmwareFileValidation.Invalid(LocalFirmwareFileValidationReason.ConfirmationContextChanged) |
There was a problem hiding this comment.
The context-changed fix only covers same-target/different-transport. TargetMismatch is still checked before this branch, so switching to a different device mid-dialog (new address and target) shows the scary "file does not match " error instead of the intended "connection changed, reselect". Check ConfirmationContextChanged (address/method delta) before the target match.
b14ab0d to
37a6b5a
Compare
Add filename validation and a confirmation dialog before flashing local firmware files, preventing users from accidentally flashing wrong-target firmware. Supports both specific firmware files and release .zip bundles. Validation flow: - User picks a file via the system file picker (MIME */*). - prepareLocalFirmwareFile resolves the file: tries direct filename validation first; if the file is a .zip bundle, extracts the matching per-target entry via extractFirmware with a per-platform extension (.zip for nRF52 BLE DFU, .bin for ESP32, .uf2 for USB). - The confirmation dialog shows the resolved filename (extracted entry name for bundles) so the user verifies the correct target. - confirmLocalFirmwareFile re-validates against current device state before starting the update, catching hardware/connection changes. State management: - A stale-state guard makes prepareLocalFirmwareFile a complete no-op if the Ready state changes during the suspend call (cancelUpdate, checkForUpdates, device disconnect). - A prepareJob is tracked and cancelled on dismiss/cancel to prevent double-prepare temp-file leaks. - Extracted temp artifacts are cleaned on dismiss, cancel, onCleared, and on every error path. Per-platform extension mapping (localFirmwarePayloadExtension) fixes a pre-existing bug where ESP32-over-BLE bundle extraction searched for .zip entries instead of .bin.
Add localized strings for the confirmation dialog, validation error messages, and archive-missing-target error. Update strings-index.txt.
Add unit tests for filename validation (all platforms), bundle extraction (nRF52/ESP32/USB), stale-state no-op, filename-unavailable error, temp artifact cleanup on dismiss, pending re-validation on confirm, and checkForUpdates clearing pending selection.
Prioritize stale confirmation context before target and filename checks so device or transport changes ask the user to reselect the file. Prefer ESP32 -update.bin payloads when resolving side-loaded release bundles, show extracting progress, and tighten target matching to reject colliding board-name prefixes.
Stop cleanupTemporaryFiles from calling cleanupAllTemporaryFiles on
every invocation — the recursive directory delete would prematurely
destroy other temp files (e.g. extracted .uf2 for USB updates) that
live in the same directory. Individual file cleanup now only deletes
the specific artifact; directory-wide cleanup is reserved for
onCleared terminal teardown.
Wrap prepareLocalFirmwareFile's coroutine body in try/catch so
unexpected exceptions from validation or payload helpers surface as
FirmwareUpdateState.Error instead of silently cancelling the
coroutine and leaving the UI stuck.
Consolidate AndroidFirmwareFileHandler on hardware.effectiveTarget
instead of duplicating the platformioTarget.ifEmpty { hwModelSlug }
pattern, matching JvmFirmwareFileHandler.
Wrap the extracting-state block in try-finally so that if extractLocalFirmwareArchive or validateExtractedLocalFirmware throws unexpectedly, the Processing state is always restored to the previous Ready state instead of leaving the UI permanently stuck on the extracting spinner.
Allow target followed by a firmware marker segment (e.g. heltec-v3-firmware-2.7.17.bin, heltec-v3.firmware.bin) in addition to the standard release-style names. The regex now accepts target + delimiter + (version | firmware) as a valid continuation, while still rejecting target-prefix collisions like tbeam vs tbeam-s3-core.
37a6b5a to
434cf19
Compare
|
@jamesarich I pushed a refreshed branch with the local-file edge cases tightened. Official ESP32 release bundles now prefer the |
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Traced the core logic (no deadlock / correct temp-file lifecycle & validation respectively) — LGTM. Details in review pass.
2 participants
Overview
This PR makes local firmware file selection safer.
Previously, selecting a file from the Android picker immediately started the update. On devices where long filenames are truncated, it was easy to select the wrong release bundle or firmware artifact without seeing the full name first.
This change resolves and validates the selected file, shows the full filename, and requires explicit confirmation before flashing begins.
Key Changes
-update.binpayloads when extracting official release bundles.Testing
git diff --checkpasses.Migration Notes
Summary by CodeRabbit
New Features
Bug Fixes