Add firmware update notifications#2033
Merged
garthvh merged 5 commits intoJul 16, 2026
Merged
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds firmware update notification logic, connect-flow and UI wiring, deduplication persistence, tests, project registration, and documentation/index updates. ChangesFirmware Update Notification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccessoryManager
participant FirmwareUpdateNotifier
participant LocalNotificationManager
participant UserDefaults
AccessoryManager->>FirmwareUpdateNotifier: notifyIfNeeded(accessoryManager)
FirmwareUpdateNotifier->>LocalNotificationManager: schedule notification
FirmwareUpdateNotifier->>UserDefaults: record notification key
sequenceDiagram
participant ConnectView
participant FirmwareUpdateNotifier
participant Router
participant UIApplication
ConnectView->>FirmwareUpdateNotifier: notice(accessoryManager)
FirmwareUpdateNotifier-->>ConnectView: FirmwareUpdateNotice
ConnectView->>Router: route meshtastic URL
ConnectView->>UIApplication: open flasher URL
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Meshtastic/Resources/docs/index.json (1)
129-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSome keyword entries add little search value.
Generic terms like
"not","over","usually","during"are unlikely to help users find this doc via search and dilute the keyword list.🤖 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 `@Meshtastic/Resources/docs/index.json` around lines 129 - 154, The keyword list in the index entry includes several low-value generic terms that dilute search relevance. Update the keywords associated with the relevant document in the index data to remove generic words like “not”, “over”, “usually”, and “during”, keeping only terms that meaningfully describe the doc and improve discoverability. Use the existing keyword array for this document as the place to trim and refine the search terms.Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift (1)
132-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilently swallowed fetch errors via
try?.Both
hardwareSupportingAppOTAandlatestStableFirmwareVersiondiscard SwiftData fetch errors withtry?, unlikerefreshFirmwareDataIfStaleabove which logs failures viaLogger.services.warning. A genuine query failure here silently suppresses the entire notification for that connection with no diagnostic trail.♻️ Suggested logging on fetch failure
descriptor.fetchLimit = 1 - guard let hardware = try? context.fetch(descriptor).first, - FirmwareUpdateNotificationPolicy.supportsAppOTA(architecture: hardware.architecture) else { + let fetchResult = Result { try context.fetch(descriptor) } + guard let hardware = (try? fetchResult.get())?.first, + FirmwareUpdateNotificationPolicy.supportsAppOTA(architecture: hardware.architecture) else { + if case .failure(let error) = fetchResult { + Logger.services.warning("Failed to fetch hardware for OTA check: \(error.localizedDescription, privacy: .public)") + } return nil }🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift` around lines 132 - 158, The fetches in hardwareSupportingAppOTA and latestStableFirmwareVersion are swallowing SwiftData errors with try?, which hides query failures. Update these helpers to handle fetch exceptions explicitly, using the existing Logger.services.warning pattern from refreshFirmwareDataIfStale to log the failure with context (including the platformioTarget or stable release lookup) and then return nil. Keep the behavior of returning nil on failure, but make sure the error is observable through logging.
🤖 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 `@Meshtastic/Accessory/Accessory` Manager/AccessoryManager+Connect.swift:
- Around line 229-232: The firmware notification is launched as an untracked
fire-and-forget Task in AccessoryManager+Connect, so it can overlap later
connect steps and outlive disconnects. Update the Step 7 flow around the Task {
`@MainActor` in ... } call in AccessoryManager connect handling to either await
the notification or store and cancel the task as part of connection lifecycle
management, and ensure FirmwareUpdateNotifier.notifyIfNeeded propagates
cancellation with Task.checkCancellation() so it stops safely on teardown.
In `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift`:
- Around line 84-116: notifyIfNeeded(accessoryManager:) is fire-and-forget and
should stop when the parent Task is cancelled. Add a cancellation check
immediately after the await refreshFirmwareDataIfStale() call, before any
SwiftData fetches or notification scheduling, so the flow exits early if the
connection session ended. Keep the fix in FirmwareUpdateNotifier.notifyIfNeeded
and ensure no later use of accessoryManager, getNodeInfo, or
LocalNotificationManager happens after cancellation.
---
Nitpick comments:
In `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift`:
- Around line 132-158: The fetches in hardwareSupportingAppOTA and
latestStableFirmwareVersion are swallowing SwiftData errors with try?, which
hides query failures. Update these helpers to handle fetch exceptions
explicitly, using the existing Logger.services.warning pattern from
refreshFirmwareDataIfStale to log the failure with context (including the
platformioTarget or stable release lookup) and then return nil. Keep the
behavior of returning nil on failure, but make sure the error is observable
through logging.
In `@Meshtastic/Resources/docs/index.json`:
- Around line 129-154: The keyword list in the index entry includes several
low-value generic terms that dilute search relevance. Update the keywords
associated with the relevant document in the index data to remove generic words
like “not”, “over”, “usually”, and “during”, keeping only terms that
meaningfully describe the doc and improve discoverability. Use the existing
keyword array for this document as the place to trim and refine the search
terms.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e118829-e584-4f1f-b67f-892d1806abc2
📒 Files selected for processing (11)
Meshtastic.xcodeproj/project.pbxprojMeshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Extensions/UserDefaults.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotifier.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/firmware.mdMeshtastic/Resources/docs/user/firmware.htmlMeshtasticTests/FirmwareUpdateNotificationPolicyTests.swiftMeshtasticTests/FirmwareUpdateNotifierTests.swiftdocs/user/firmware.md
RCGV1
marked this pull request as ready for review
July 4, 2026 21:17
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swift (1)
204-231: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStep 7 has no timeout despite now doing a potentially network-bound call.
Unlike Steps 1 (
timeout: .seconds(5)), 3 (timeout: .seconds(30)), and 5 (timeout: .seconds(10.0)), Step 7 has no explicit timeout.FirmwareUpdateNotifier.notifyIfNeededcan triggerrefreshFirmwareDataIfStale()→MeshtasticAPI.shared.refreshFirmwareAPIData()(once per 24h per node) on the first connect after staleness. If that network call hangs without its own timeout, this blocks Step 7 — and thus Step 8 (MQTT/location init) and the overallconnect()completion — indefinitely.🔧 Suggested fix: add an explicit timeout to Step 7
- // Step 7: Update UI and status to connected - Step { `@MainActor` _ in + // Step 7: Update UI and status to connected + Step(timeout: .seconds(15)) { `@MainActor` _ in🤖 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 `@Meshtastic/Accessory/Accessory` Manager/AccessoryManager+Connect.swift around lines 204 - 231, Step 7 in AccessoryManager+Connect.swift can block indefinitely because FirmwareUpdateNotifier.notifyIfNeeded may perform a network-bound refresh without an explicit timeout. Update the Step { `@MainActor` _ in ... } block to use a timeout consistent with earlier steps, and make sure the timeout wraps the FirmwareUpdateNotifier.notifyIfNeeded(accessoryManager:) call so connect() can continue to Step 8 if the refresh hangs.
🧹 Nitpick comments (2)
Meshtastic/Views/Connect/Connect.swift (2)
693-726: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider hiding the decorative warning icon from VoiceOver.
.accessibilityElement(children: .combine)merges child labels; theexclamationmark.triangle.fillandchevron.rightimages have no explicit accessibility treatment and may add extraneous symbol-name announcements alongside the combined text and hint.♻️ Optional: mark decorative icons as hidden
Image(systemName: "exclamationmark.triangle.fill") .font(.title3) .foregroundColor(.orange) .padding(.top, 2) + .accessibilityHidden(true)🤖 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 `@Meshtastic/Views/Connect/Connect.swift` around lines 693 - 726, The `FirmwareUpdateConnectNotice` button is combining child accessibility labels, so the decorative `Image` icons can still introduce unwanted symbol-name announcements. Update the accessibility treatment in `FirmwareUpdateConnectNotice.body` by hiding the non-informational `exclamationmark.triangle.fill` and `chevron.right` images from VoiceOver while keeping the combined label and hint on the `Button`.
561-606: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
.subscribedguard across two functions.
refreshConnectedNodeState()(lines 563-568) already gates onaccessoryManager.state == .subscribedbefore callingrefreshFirmwareUpdateNotice()(line 589), which then re-checks the same condition (line 600) since it has no other caller shown in this file. Harmless, but worth consolidating for clarity.🤖 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 `@Meshtastic/Views/Connect/Connect.swift` around lines 561 - 606, The `.subscribed` check is duplicated between `refreshConnectedNodeState()` and `refreshFirmwareUpdateNotice()`, so consolidate the state gating for clarity. Update `refreshFirmwareUpdateNotice()` to assume the caller has already validated `accessoryManager.state` (or move the guard to a single shared entry point), and keep the existing behavior of clearing `firmwareUpdateNotice` only when the state is not subscribed. Use the `refreshConnectedNodeState` and `refreshFirmwareUpdateNotice` methods to locate the duplicated guard logic.
🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift`:
- Around line 118-136: The cancellation checks in
notifyIfNeeded(accessoryManager:) can still throw after the accessory has
already reached .subscribed, which can incorrectly bubble into
AccessoryManager.connect() and trigger closeConnection(). Update
FirmwareUpdateNotifier.notifyIfNeeded so cancellation is handled internally as a
best-effort path, either by avoiding propagation from the
Task.checkCancellation() points or by catching cancellation around the
firmware-notification work, while keeping the successful connect state intact.
---
Outside diff comments:
In `@Meshtastic/Accessory/Accessory` Manager/AccessoryManager+Connect.swift:
- Around line 204-231: Step 7 in AccessoryManager+Connect.swift can block
indefinitely because FirmwareUpdateNotifier.notifyIfNeeded may perform a
network-bound refresh without an explicit timeout. Update the Step { `@MainActor`
_ in ... } block to use a timeout consistent with earlier steps, and make sure
the timeout wraps the FirmwareUpdateNotifier.notifyIfNeeded(accessoryManager:)
call so connect() can continue to Step 8 if the refresh hangs.
---
Nitpick comments:
In `@Meshtastic/Views/Connect/Connect.swift`:
- Around line 693-726: The `FirmwareUpdateConnectNotice` button is combining
child accessibility labels, so the decorative `Image` icons can still introduce
unwanted symbol-name announcements. Update the accessibility treatment in
`FirmwareUpdateConnectNotice.body` by hiding the non-informational
`exclamationmark.triangle.fill` and `chevron.right` images from VoiceOver while
keeping the combined label and hint on the `Button`.
- Around line 561-606: The `.subscribed` check is duplicated between
`refreshConnectedNodeState()` and `refreshFirmwareUpdateNotice()`, so
consolidate the state gating for clarity. Update `refreshFirmwareUpdateNotice()`
to assume the caller has already validated `accessoryManager.state` (or move the
guard to a single shared entry point), and keep the existing behavior of
clearing `firmwareUpdateNotice` only when the state is not subscribed. Use the
`refreshConnectedNodeState` and `refreshFirmwareUpdateNotice` methods to locate
the duplicated guard 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a3a636a5-f8c3-4abc-8501-af793e82ebe5
⛔ Files ignored due to path filters (1)
docs/assets/screenshots/connectFirmwareUpdateNotice.pngis excluded by!**/*.png
📒 Files selected for processing (11)
Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotifier.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/firmware.mdMeshtastic/Resources/docs/user/firmware.htmlMeshtastic/Views/Connect/Connect.swiftMeshtasticTests/FirmwareUpdateNotificationPolicyTests.swiftMeshtasticTests/FirmwareUpdateNotifierTests.swiftdocs/user/firmware.mdscripts/build-docs.sh
✅ Files skipped from review due to trivial changes (4)
- docs/user/firmware.md
- Meshtastic/Resources/docs/user/firmware.html
- scripts/build-docs.sh
- Meshtastic/Resources/docs/markdown/user/firmware.md
🚧 Files skipped from review as they are similar to previous changes (2)
- MeshtasticTests/FirmwareUpdateNotificationPolicyTests.swift
- Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swift
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Meshtastic/MeshtasticAppDelegate.swift (1)
189-195: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider handling the
open(url:)result.
UIApplication.shared.open(url)is fire-and-forget here; if the flasher URL fails to open (e.g., no browser available in some restricted environment), there's no logging of failure. Since the branch already logs on the unsupported-URL path, consider adding a completion handler for symmetry/observability.♻️ Optional improvement
- } else if targetValue == FirmwareUpdateNotifier.flasherTarget && url.absoluteString == FirmwareUpdateNotifier.flasherPath { - UIApplication.shared.open(url) + } else if targetValue == FirmwareUpdateNotifier.flasherTarget && url.absoluteString == FirmwareUpdateNotifier.flasherPath { + UIApplication.shared.open(url) { success in + if !success { + Logger.services.error("Failed to open flasher URL: \(deepLink, privacy: .public)") + } + }🤖 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 `@Meshtastic/MeshtasticAppDelegate.swift` around lines 189 - 195, The flasher URL branch in MeshtasticAppDelegate’s URL handling ignores the result of UIApplication.shared.open(url), so failures are never observed. Update the open(url:) call to use its completion handler and log when opening fails, keeping the same style as the unsupported-URL Logger.services.error path. Use the existing URL-routing branch and FirmwareUpdateNotifier.flasherTarget/FirmwareUpdateNotifier.flasherPath checks to locate the fix.
🤖 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 `@Meshtastic/MeshtasticAppDelegate.swift`:
- Around line 189-195: The flasher URL branch in MeshtasticAppDelegate’s URL
handling ignores the result of UIApplication.shared.open(url), so failures are
never observed. Update the open(url:) call to use its completion handler and log
when opening fails, keeping the same style as the unsupported-URL
Logger.services.error path. Use the existing URL-routing branch and
FirmwareUpdateNotifier.flasherTarget/FirmwareUpdateNotifier.flasherPath checks
to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd97eb65-2f09-414e-af84-7d129f20549b
📒 Files selected for processing (4)
Meshtastic/MeshtasticAppDelegate.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotifier.swiftMeshtastic/Views/Connect/Connect.swiftMeshtasticTests/FirmwareUpdateNotifierTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift
- Meshtastic/Views/Connect/Connect.swift
# Conflicts: # Meshtastic/Extensions/UserDefaults.swift # Meshtastic/Resources/docs/index.json # Meshtastic/Views/Connect/Connect.swift
This was referenced Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
meshtastic:///settings/firmwareUpdates).Screenshot
Notes
This is split out from the broader OTA hardening work so it can be reviewed independently from ESP32/nRF flashing changes. The Connect-page notice currently uses the latest stable firmware data, matching the notification policy.
Validation
bash scripts/build-docs.sh --output Meshtastic/Resources/docsgit diff --checkMeshtasticTests/FirmwareUpdateNotificationPolicyTestsandMeshtasticTests/FirmwareUpdateNotifierTestspassed, 12/12.Meshtasticon iPhone 17 succeeded.Summary by CodeRabbit