feat(events): add native firmware edition branding - #2222
Conversation
📝 WalkthroughWalkthroughEvent firmware metadata now uses defensive decoding, throttled refreshes, merge-safe caching, HTTPS and image validation, scoped presentation theming, policy-driven notifications, refreshed artwork/docs, and a new FAB edition. Numerous SwiftUI controls also adopt standard system styling. ChangesEvent firmware platform
SwiftUI control styling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
Meshtastic/Model/EventFirmwarePresentation.swift (2)
1-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
// MARK: -separators between the file's logical sections.The file defines four distinct concerns (
EventFirmwarePresentation, the environment keys/values,EventFirmwareTintScope,EventFirmwarePaletteRule) with no// MARK: -separators.As per coding guidelines, "Use
// MARK: -comments to separate logical sections within a file."🤖 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/EventFirmwarePresentation.swift` around lines 1 - 108, Add // MARK: - separators between the four logical sections: EventFirmwarePresentation, the environment keys and EnvironmentValues extension, EventFirmwareTintScope, and EventFirmwarePaletteRule. Keep the existing implementations unchanged.Source: Coding guidelines
71-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the event tint instead of recomputing contrast math on every render.
tintre-runsresolve→accessibleTintHex(for:)→color(fromHex:)every timebodyevaluates. SinceEventFirmwareTintScopewraps the whole app tree (main window and Mesh Map window inMeshtasticApp.swift) and observesaccessoryManagervia@EnvironmentObject, any unrelated published change onaccessoryManager(discovery, connection state, etc.) re-triggers this contrast calculation, not just edition/connection/colorScheme changes that actually affect the result.Consider caching the resolved tint in
@Stateand updating it via.onChange(of:)on the specific inputs that matter (accessoryManager.isConnected,accessoryManager.firmwareEdition,colorScheme,useEventTheme, and the queried metadata), rather than recomputing on every body pass.♻️ Suggested direction
- private var tint: Color { - guard useEventTheme, - let presentation = EventFirmwarePresentation.resolve(...) ... - return tint - } - - var body: some View { - content.tint(tint) - } + `@State` private var cachedTint: Color = .accentColor + + var body: some View { + content + .tint(cachedTint) + .onChange(of: tintInputs) { _, _ in cachedTint = computeTint() } + .task { cachedTint = computeTint() } + }🤖 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/EventFirmwarePresentation.swift` around lines 71 - 89, Cache the resolved event tint in `@State` within EventFirmwareTintScope instead of computing it in the tint property on every body evaluation. Add a focused update helper that performs the existing resolve/accessibility/color conversion and invoke it on appearance and when accessoryManager.isConnected, accessoryManager.firmwareEdition, colorScheme, useEventTheme, or eventFirmwareEditions changes; have body use the cached tint while preserving .accentColor as the fallback.Meshtastic/Views/Connect/Connect.swift (1)
552-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
EventFirmwarePresentation.resolveinstead of re-implementing the lookup.
eventFirmwareduplicates the edition→metadata matching logic already centralized inEventFirmwarePresentation.resolve, but omits theisConnectedcheck that helper enforces. Currently harmless since the only call site (Lines 147-152) is already gated onisConnected || isConnecting, but the duplication risks silent divergence if either implementation changes oreventFirmwaregets reused elsewhere without that same gating.♻️ Suggested fix
private var eventFirmware: EventFirmwareEntity? { - guard accessoryManager.firmwareEdition.isEvent else { return nil } - let key = accessoryManager.firmwareEdition.editionKey - return eventFirmwareEditions.first { $0.edition == key } + EventFirmwarePresentation.resolve( + isConnected: accessoryManager.isConnected, + edition: accessoryManager.firmwareEdition, + metadata: eventFirmwareEditions, + deviceFirmwareVersion: accessoryManager.connectedVersion + )?.info }🤖 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 552 - 558, Update the eventFirmware computed property to reuse EventFirmwarePresentation.resolve for edition-to-metadata resolution instead of manually checking firmwareEdition.isEvent and searching eventFirmwareEditions. Pass the existing connection state required by resolve so its isConnected validation is preserved, and keep the nil behavior when no presentation resolves.MeshtasticTests/EventFirmwareMetadataTests.swift (2)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.faboverFirmwareEditions(rawValue: 20).The surrounding assertions use named cases; the raw-value form (also at Lines 257 and 292) spreads the magic number through the tests and loses compile-time safety. If the intent is to pin the wire value, assert
FirmwareEditions.fab.rawValue == 20once and use.fabelsewhere.🤖 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 `@MeshtasticTests/EventFirmwareMetadataTests.swift` at line 32, Replace the FirmwareEditions(rawValue: 20) assertions with FirmwareEditions.fab where testing editionKey, and add a single assertion that FirmwareEditions.fab.rawValue equals 20 to preserve the wire-value check. Apply this consistently at the referenced assertions.
483-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood isolation; consider adding a case for palette/links merge.
The suite covers scalar fields only. A case where a live payload sends an all-invalid
theme.paletteor all-non-HTTPSlinksover a populated row would lock the merge contract discussed onMeshtastic/API/MeshtasticAPI.swiftLines 1041-1064. Want me to draft it?🤖 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 `@MeshtasticTests/EventFirmwareMetadataTests.swift` around lines 483 - 560, The EventFirmwareCacheMergeTests suite lacks coverage for preserving valid cached palette and link data when an incoming partial payload contains no valid entries. Add a test using importEventEditions that first stores populated theme.palette and HTTPS links, then imports a payload with all-invalid palette values or all-non-HTTPS links, and assert the existing valid values remain unchanged while other merge behavior is preserved.Meshtastic/API/MeshtasticAPI.swift (1)
865-868: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider decoding links leniently so one bad link doesn't drop a whole edition.
EventFirmwareLinkPayloadrequires non-optionallabel/url, so a single malformed link entry fails the entireEventFirmwarePayloaddecode and the edition loses all branding (this is exactly whatmalformedManifestEntryDoesNotDiscardValidEntriesasserts forBURNING_MAN). Decodinglinksinto a lenient wrapper that skips bad entries would preserve the rest of the edition, consistent with the stated per-edition defensive goal.🤖 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/API/MeshtasticAPI.swift` around lines 865 - 868, Update EventFirmwareLinkPayload and the EventFirmwarePayload links decoding flow to tolerate malformed link entries by skipping invalid elements instead of failing the entire edition decode. Preserve valid links and the remaining edition data, including the behavior covered by malformedManifestEntryDoesNotDiscardValidEntries.Meshtastic/Model/EventFirmwareEntity.swift (1)
365-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the hex→RGB byte decode.
color(fromHex:),relativeLuminance(fromHex:), andcontrastAdjustedTintHex(_:for:)each re-implement the same#RRGGBBparse and channel split. A singlergbComponents(fromHex:) -> (Double, Double, Double)?helper would keep the strictness rules in one place as the tint logic evolves.🤖 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/EventFirmwareEntity.swift` around lines 365 - 393, The hex parsing and RGB channel extraction are duplicated across color(fromHex:), relativeLuminance(fromHex:), and contrastAdjustedTintHex(_:for:). Extract a shared rgbComponents(fromHex:) -> (Double, Double, Double)? helper containing the existing strict `#RRGGBB` validation and normalized channel decoding, then reuse it from all three methods without changing their behavior.Meshtastic/Accessory/Accessory Manager/AccessoryManager+FromRadio.swift (1)
14-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the notification policy types out of
AccessoryManager+FromRadio.swift.
EventFirmwareNotificationSettings/EventFirmwareNotificationPolicyare pure, self-contained domain logic (already unit-tested independently inEventFirmwareNotificationTests.swift) with no dependency on FromRadio packet handling. Extracting them into their own file would better match the project's convention of grouping extensions/logic by functionality.As per coding guidelines, "Group extensions by functionality in separate files, such as
AccessoryManager+ToRadio.swift."🤖 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+FromRadio.swift around lines 14 - 56, Move the self-contained EventFirmwareNotificationSettings and EventFirmwareNotificationPolicy types out of AccessoryManager+FromRadio.swift into a dedicated file for event firmware notification logic. Preserve both types’ APIs and behavior unchanged, and leave AccessoryManager+FromRadio.swift focused on FromRadio packet handling.Source: Coding guidelines
🤖 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+FromRadio.swift:
- Around line 14-56: Align EventFirmwareNotificationPolicy.updatedSettings with
DeviceOnboarding’s raw firmware-edition handling so unrecognized FirmwareEdition
values are treated consistently as non-event firmware and do not disable
notifications unexpectedly. Update the edition check in updatedSettings, or
reuse the raw-value event classification, while preserving explicit .vanilla
behavior and existing user-override handling.
In `@Meshtastic/API/MeshtasticAPI.swift`:
- Around line 925-932: Update EventFirmwareRefreshPolicy.shouldRefresh to treat
a lastAttempt timestamp later than now as immediately due, while retaining the
minimumAttemptInterval check for timestamps at or before now. Ensure backward
clock changes cannot suppress firmware refreshes until wall-clock time catches
up.
- Around line 1041-1064: Update the live payload merge around setLinks and
themePalette so malformed data cannot erase existing cached values: only call
setLinks when the mapped links produce a non-empty valid result, and only assign
themePalette when its filtered palette is non-empty. Preserve the current
filtering and retain cached fields when all supplied links or palette entries
are rejected.
In `@Meshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/Contents.json`:
- Around line 3-7: Update the event artwork image catalogs to prevent Retina
upscaling: in
Meshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/Contents.json lines 3-7,
provide 2x/3x defcon34.png variants or register it as a single-scale unscaled
image; apply the same change to fab26.png in
Meshtastic/Assets.xcassets/EventFirmwareFAB.imageset/Contents.json lines 3-7.
In `@Meshtastic/Enums/FirmwareEditionEnum.swift`:
- Line 19: Align the FirmwareEdition enum with the current protobuf definition
by removing the unsupported fab case/value and ensuring its declared cases and
raw values match the generated protobuf enum. Update FirmwareEditions(from:) as
needed so unsupported values are not incorrectly treated as FAB and retain the
intended vanilla fallback behavior.
In `@Meshtastic/Model/EventFirmwareEntity.swift`:
- Around line 60-65: Update the SwiftData documentation in swiftdata.md and
architecture.md to reflect the EventFirmwareEntity `@Model` changes: document
removal of the obsolete stored field and persistence of themePrimaryColor,
themeSecondaryColor, and themeAccentColor, including any relevant schema or
migration notes.
In `@Meshtastic/Persistence/UpdateSwiftData.swift`:
- Around line 226-228: The database-clearing behavior is inconsistent because
UpdateSwiftData.clearDatabase() skips EventFirmwareEntity while
PersistenceController.clearDatabase() still deletes it. Update the full app
reset path in PersistenceController.clearDatabase() to preserve
EventFirmwareEntity as well, or route full resets away from the device-clear
path, keeping both reset behaviors synchronized.
---
Nitpick comments:
In `@Meshtastic/Accessory/Accessory` Manager/AccessoryManager+FromRadio.swift:
- Around line 14-56: Move the self-contained EventFirmwareNotificationSettings
and EventFirmwareNotificationPolicy types out of
AccessoryManager+FromRadio.swift into a dedicated file for event firmware
notification logic. Preserve both types’ APIs and behavior unchanged, and leave
AccessoryManager+FromRadio.swift focused on FromRadio packet handling.
In `@Meshtastic/API/MeshtasticAPI.swift`:
- Around line 865-868: Update EventFirmwareLinkPayload and the
EventFirmwarePayload links decoding flow to tolerate malformed link entries by
skipping invalid elements instead of failing the entire edition decode. Preserve
valid links and the remaining edition data, including the behavior covered by
malformedManifestEntryDoesNotDiscardValidEntries.
In `@Meshtastic/Model/EventFirmwareEntity.swift`:
- Around line 365-393: The hex parsing and RGB channel extraction are duplicated
across color(fromHex:), relativeLuminance(fromHex:), and
contrastAdjustedTintHex(_:for:). Extract a shared rgbComponents(fromHex:) ->
(Double, Double, Double)? helper containing the existing strict `#RRGGBB`
validation and normalized channel decoding, then reuse it from all three methods
without changing their behavior.
In `@Meshtastic/Model/EventFirmwarePresentation.swift`:
- Around line 1-108: Add // MARK: - separators between the four logical
sections: EventFirmwarePresentation, the environment keys and EnvironmentValues
extension, EventFirmwareTintScope, and EventFirmwarePaletteRule. Keep the
existing implementations unchanged.
- Around line 71-89: Cache the resolved event tint in `@State` within
EventFirmwareTintScope instead of computing it in the tint property on every
body evaluation. Add a focused update helper that performs the existing
resolve/accessibility/color conversion and invoke it on appearance and when
accessoryManager.isConnected, accessoryManager.firmwareEdition, colorScheme,
useEventTheme, or eventFirmwareEditions changes; have body use the cached tint
while preserving .accentColor as the fallback.
In `@Meshtastic/Views/Connect/Connect.swift`:
- Around line 552-558: Update the eventFirmware computed property to reuse
EventFirmwarePresentation.resolve for edition-to-metadata resolution instead of
manually checking firmwareEdition.isEvent and searching eventFirmwareEditions.
Pass the existing connection state required by resolve so its isConnected
validation is preserved, and keep the nil behavior when no presentation
resolves.
In `@MeshtasticTests/EventFirmwareMetadataTests.swift`:
- Line 32: Replace the FirmwareEditions(rawValue: 20) assertions with
FirmwareEditions.fab where testing editionKey, and add a single assertion that
FirmwareEditions.fab.rawValue equals 20 to preserve the wire-value check. Apply
this consistently at the referenced assertions.
- Around line 483-560: The EventFirmwareCacheMergeTests suite lacks coverage for
preserving valid cached palette and link data when an incoming partial payload
contains no valid entries. Add a test using importEventEditions that first
stores populated theme.palette and HTTPS links, then imports a payload with
all-invalid palette values or all-non-HTTPS links, and assert the existing valid
values remain unchanged while other merge behavior is preserved.
🪄 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: 35195fee-a9ef-4150-87cf-b79cfbc51a74
⛔ Files ignored due to path filters (3)
Meshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/defcon34.pngis excluded by!**/*.pngMeshtastic/Assets.xcassets/EventFirmwareFAB.imageset/fab26.pngis excluded by!**/*.pngMeshtastic/Assets.xcassets/EventFirmwareHAMVENTION.imageset/hamvention.pngis excluded by!**/*.png
📒 Files selected for processing (60)
Meshtastic/API/MeshtasticAPI.swiftMeshtastic/Accessory/Accessory Manager/AccessoryManager+FromRadio.swiftMeshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/Contents.jsonMeshtastic/Assets.xcassets/EventFirmwareFAB.imageset/Contents.jsonMeshtastic/Assets.xcassets/EventFirmwareHAMVENTION.imageset/Contents.jsonMeshtastic/Enums/FirmwareEditionEnum.swiftMeshtastic/Extensions/UserDefaults.swiftMeshtastic/MeshtasticApp.swiftMeshtastic/Model/EventFirmwareEntity.swiftMeshtastic/Model/EventFirmwarePresentation.swiftMeshtastic/Persistence/MarketingCapture.swiftMeshtastic/Persistence/UpdateSwiftData.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/firmware.mdMeshtastic/Resources/docs/user/firmware.htmlMeshtastic/Resources/event_firmware.jsonMeshtastic/Views/Connect/Connect.swiftMeshtastic/Views/Connect/EventFirmwareInfoView.swiftMeshtastic/Views/ContentView.swiftMeshtastic/Views/Helpers/Compact Widgets/ParticulateMatterCompactWidget.swiftMeshtastic/Views/Helpers/Compact Widgets/RadiationCompactWidget.swiftMeshtastic/Views/Helpers/MeshtasticLogo.swiftMeshtastic/Views/Nodes/Helpers/Map/MapSettingsForm.swiftMeshtastic/Views/Nodes/Helpers/Map/WaypointForm.swiftMeshtastic/Views/Nodes/Helpers/NodeListFilter.swiftMeshtastic/Views/Onboarding/DeviceOnboarding.swiftMeshtastic/Views/Provisioning/WifiProvisioningView.swiftMeshtastic/Views/Settings/AppData.swiftMeshtastic/Views/Settings/AppSettings.swiftMeshtastic/Views/Settings/Channels/ChannelForm.swiftMeshtastic/Views/Settings/Config/BluetoothConfig.swiftMeshtastic/Views/Settings/Config/DeviceConfig.swiftMeshtastic/Views/Settings/Config/DisplayConfig.swiftMeshtastic/Views/Settings/Config/LoRaConfig.swiftMeshtastic/Views/Settings/Config/Module/AmbientLightingConfig.swiftMeshtastic/Views/Settings/Config/Module/AudioConfig.swiftMeshtastic/Views/Settings/Config/Module/CannedMessagesConfig.swiftMeshtastic/Views/Settings/Config/Module/DetectionSensorConfig.swiftMeshtastic/Views/Settings/Config/Module/ExternalNotificationConfig.swiftMeshtastic/Views/Settings/Config/Module/MQTTConfig.swiftMeshtastic/Views/Settings/Config/Module/MeshBeaconConfig.swiftMeshtastic/Views/Settings/Config/Module/NeighborInfoConfig.swiftMeshtastic/Views/Settings/Config/Module/PaxCounterConfig.swiftMeshtastic/Views/Settings/Config/Module/RangeTestConfig.swiftMeshtastic/Views/Settings/Config/Module/SerialConfig.swiftMeshtastic/Views/Settings/Config/Module/StoreForwardConfig.swiftMeshtastic/Views/Settings/Config/Module/TelemetryConfig.swiftMeshtastic/Views/Settings/Config/Module/TrafficManagementConfig.swiftMeshtastic/Views/Settings/Config/NetworkConfig.swiftMeshtastic/Views/Settings/Config/PositionConfig.swiftMeshtastic/Views/Settings/Config/PowerConfig.swiftMeshtastic/Views/Settings/Config/SecurityConfig.swiftMeshtastic/Views/Settings/Discovery/DiscoveryScanView.swiftMeshtastic/Views/Settings/Routes.swiftMeshtastic/Views/Settings/ShareChannels.swiftMeshtastic/Views/Settings/TAKServerConfig.swiftMeshtastic/Views/Settings/UserConfig.swiftMeshtasticTests/EventFirmwareMetadataTests.swiftMeshtasticTests/EventFirmwareNotificationTests.swiftdocs/user/firmware.md
💤 Files with no reviewable changes (26)
- Meshtastic/Views/Helpers/Compact Widgets/ParticulateMatterCompactWidget.swift
- Meshtastic/Views/Settings/Config/Module/SerialConfig.swift
- Meshtastic/Views/Settings/AppData.swift
- Meshtastic/Views/Settings/Config/Module/AmbientLightingConfig.swift
- Meshtastic/Views/Provisioning/WifiProvisioningView.swift
- Meshtastic/Views/Helpers/Compact Widgets/RadiationCompactWidget.swift
- Meshtastic/Views/Settings/ShareChannels.swift
- Meshtastic/Views/Settings/Config/Module/MeshBeaconConfig.swift
- Meshtastic/Views/Settings/Config/Module/NeighborInfoConfig.swift
- Meshtastic/Views/Settings/Config/Module/AudioConfig.swift
- Meshtastic/Views/Settings/Config/Module/CannedMessagesConfig.swift
- Meshtastic/Views/Settings/Config/Module/DetectionSensorConfig.swift
- Meshtastic/Views/Settings/Config/DisplayConfig.swift
- Meshtastic/Views/Settings/Config/Module/TrafficManagementConfig.swift
- Meshtastic/Views/Settings/Config/SecurityConfig.swift
- Meshtastic/Views/Settings/Config/NetworkConfig.swift
- Meshtastic/Views/Settings/Discovery/DiscoveryScanView.swift
- Meshtastic/Views/Settings/AppSettings.swift
- Meshtastic/Views/Settings/Config/PowerConfig.swift
- Meshtastic/Views/Settings/Config/Module/StoreForwardConfig.swift
- Meshtastic/Views/Settings/Config/Module/ExternalNotificationConfig.swift
- Meshtastic/Views/Settings/Config/Module/RangeTestConfig.swift
- Meshtastic/Views/Settings/Config/LoRaConfig.swift
- Meshtastic/Views/Settings/Config/Module/MQTTConfig.swift
- Meshtastic/Views/Settings/Config/PositionConfig.swift
- Meshtastic/Views/Settings/Config/Module/TelemetryConfig.swift
| struct EventFirmwareNotificationSettings: Equatable { | ||
| let newNodeNotifications: Bool | ||
| let autoDisabledForEvent: Bool | ||
| let userOverrideForEvent: Bool | ||
| } | ||
|
|
||
| enum EventFirmwareNotificationPolicy { | ||
|
|
||
| static func userUpdatedSettings( | ||
| newNodeNotifications: Bool, | ||
| isEventFirmware: Bool | ||
| ) -> EventFirmwareNotificationSettings { | ||
| EventFirmwareNotificationSettings( | ||
| newNodeNotifications: newNodeNotifications, | ||
| autoDisabledForEvent: false, | ||
| userOverrideForEvent: isEventFirmware | ||
| ) | ||
| } | ||
|
|
||
| static func updatedSettings( | ||
| for edition: FirmwareEdition, | ||
| current: EventFirmwareNotificationSettings | ||
| ) -> EventFirmwareNotificationSettings { | ||
| if edition == .vanilla { | ||
| return EventFirmwareNotificationSettings( | ||
| newNodeNotifications: current.autoDisabledForEvent ? true : current.newNodeNotifications, | ||
| autoDisabledForEvent: false, | ||
| userOverrideForEvent: false | ||
| ) | ||
| } | ||
|
|
||
| guard !current.userOverrideForEvent, | ||
| current.newNodeNotifications, | ||
| !current.autoDisabledForEvent else { | ||
| return current | ||
| } | ||
| return EventFirmwareNotificationSettings( | ||
| newNodeNotifications: false, | ||
| autoDisabledForEvent: true, | ||
| userOverrideForEvent: false | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect FirmwareEditions' conversion from the raw protobuf FirmwareEdition, especially its default/UNRECOGNIZED handling
rg -n -A15 'init\(from' Meshtastic/Enums/FirmwareEditionEnum.swift
rg -n -A25 'enum FirmwareEditions' Meshtastic/Enums/FirmwareEditionEnum.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 1370
🏁 Script executed:
#!/bin/bash
# Inspect FirmwareEditions.rawValue/isEvent behavior and the notification policy tests/usages to ensure the mismatch is real.
set -euo pipefail
sed -n '1,180p' Meshtastic/Enums/FirmwareEditionEnum.swift
printf '\n--- applyEventFirmwareNotificationDefaults ---\n'
rg -n -A30 'applyEventFirmwareNotificationDefaults' Meshtastic/Accessory/Accessory\ Manager/AccessoryManager+FromRadio.swift
printf '\n--- firmware edition isEvent usages ---\n'
rg -n -A5 -B5 'firmwareEdition\.isEvent|FirmwareEditions\(from:' Meshtastic || true
printf '\n--- relevant EventFirmwareNotificationTests ---\n'
rg -n -A8 'unknownNonVanillaEditionDisablesNotifications|class EventFirmwareNotificationTests|func|isEvent' Meshtastic -g '*EventFirmwareNotifications*' -g '*.swift'Repository: meshtastic/Meshtastic-Apple
Length of output: 50384
🏁 Script executed:
#!/bin/bash
# Narrowed read-only checks for the two firmware-enum definitions/usages and policy tests without dumping unrelated code.
set -euo pipefail
printf '%s\n' '--- FirmwareEditionEnum ---'
sed -n '1,140p' Meshtastic/Enums/FirmwareEditionEnum.swift
printf '%s\n' '--- FromRadio policy helper ---'
sed -n '1,90p' Meshtastic/Accessory/Accessory\ Manager/AccessoryManager+FromRadio.swift
sed -n '220,255p' Meshtastic/Accessory/Accessory\ Manager/AccessoryManager+FromRadio.swift
printf '%s\n' '--- device onboarding usage ---'
rg -n -A8 -B8 'FirmwareEditions\(from:|firmwareEdition\.isEvent' Meshtastic/Views/Onboarding/DeviceOnboarding.swift Meshtastic/Enums/FirmwareEditionEnum.swift Meshtastic -g '*.swift'
printf '%s\n' '--- EventFirmwareNotificationTests ---'
rg -n -A12 'class EventFirmwareNotificationTests|unknown.*Disables|isEvent|updatedSettings' Meshtastic -g '*EventFirmware*'Repository: meshtastic/Meshtastic-Apple
Length of output: 17179
Align unrecognized firmware editions between both notification-policy paths.
FirmwareEditions(from:) maps raw FirmwareEdition values only via rawValue, falling back to .vanilla; therefore DeviceOnboarding treats an unrecognized edition as non-event, while applyEventFirmwareNotificationDefaults(myNodeInfo.firmwareEdition) disables notifications for the same raw value because it is non-.vanilla. Use the same raw/non-vanilla event-logic at both inputs, or avoid mapping through FirmwareEditions for this policy.
🤖 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+FromRadio.swift
around lines 14 - 56, Align EventFirmwareNotificationPolicy.updatedSettings with
DeviceOnboarding’s raw firmware-edition handling so unrecognized FirmwareEdition
values are treated consistently as non-event firmware and do not disable
notifications unexpectedly. Update the edition check in updatedSettings, or
reuse the raw-value event classification, while preserving explicit .vanilla
behavior and existing user-override handling.
| case defcon = 17 | ||
| case burningMan = 18 | ||
| case hamvention = 19 | ||
| case fab = 20 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the generated FirmwareEdition proto enum and its raw values
rg -nP --type=swift -C2 'enum FirmwareEdition\b'
rg -nPi -C1 '\bfab\b\s*=\s*20|case fab' --type=swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg -i 'FirmwareEdition|firmware|protobuf|proto|Protobuf|protos?' || true
echo
echo "== exact enum/enum names containing edition/firmware =="
rg -n --hidden -i --glob '*.swift' --glob '!Pods/**' --glob '!DerivedData/**' \
'enum\s+[A-Za-z0-9_]*[Ee]dition[A-Za-z0-9_]*\s*|enum\s+.*Firmware[A-Za-z0-9_]*|case\s+fab\s*=|case\s+vanilla\s*=' . || true
echo
echo "== nearby FirmwareEditionEnum =="
fd -i 'FirmwareEditionEnum.swift' . -x sh -c 'echo "--- {}; line count"; wc -l "{}"; echo; cat -n "{}"'Repository: meshtastic/Meshtastic-Apple
Length of output: 12008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generated FirmwareEdition enum =="
sed -n '1268,1315p' MeshtasticProtobufs/Sources/meshtastic/mesh.pb.swift
echo
echo "== source repo references to FirmwareEdition constants =="
rg -n -i --glob '*.proto' --glob '*.swift' --glob '!MeshtasticProtobufs/**' \
'enum\s+FirmwareEdition|case\s+fab|FAB|FirmwareEdition' . || true
echo
echo "== generated enum constant search in MeshtasticProtobufs =="
rg -n -i 'enum\s+FirmwareEdition|case\s+fab|FAB' MeshtasticProtobufs/Sources . 2>/dev/null || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 28233
🏁 Script executed:
#!/_bin/bash
set -euo pipefail
echo "== source proto FirmwareEdition =="
sed -n '2135,2185p' protobufs/meshtastic/mesh.proto
echo
echo "== generated FirmwareEdition init/serialize cases from generated file =="
sed -n '1275,1435p' MeshtasticProtobufs/Sources/meshtastic/mesh.pb.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 5019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== source proto FirmwareEdition =="
sed -n '2135,2185p' protobufs/meshtastic/mesh.proto
echo
echo "== generated FirmwareEdition init/serialize cases from generated file =="
sed -n '1275,1435p' MeshtasticProtobufs/Sources/meshtastic/mesh.pb.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 5019
Regenerate/align FirmwareEdition raw values with the current protobuf enum.
The generated Swift enum source only defines VANILLA/SMART_CITIZEN/OPEN_SAUCE/DEFCON/BURNING_MAN/HAMVENTION/DIY_EDITION; .fab = 20 has no matching FirmwareEdition case, so any FAB firmware-edition value becomes .UNRECOGNIZED(20) and FirmwareEditions(from:) falls back to .vanilla.
🤖 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/Enums/FirmwareEditionEnum.swift` at line 19, Align the
FirmwareEdition enum with the current protobuf definition by removing the
unsupported fab case/value and ensuring its declared cases and raw values match
the generated protobuf enum. Update FirmwareEditions(from:) as needed so
unsupported values are not incorrectly treated as FAB and retain the intended
vanilla fallback behavior.
| /// Named brand colors used when a manifest does not provide an authored palette. | ||
| var themePrimaryColor: String? | ||
| var themeSecondaryColor: String? | ||
| /// The theme's high-energy accent candidate. It is used for small marks only after | ||
| /// satisfying the 3:1 graphical-object contrast requirement against the current surface. | ||
| var themeAccentColor: String? |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'swiftdata.md|architecture.md' docs
git diff --name-only origin/HEAD...HEAD -- docs | head -50
rg -n 'firmwareZipUrl' -g '!*.xcodeproj*' .Repository: meshtastic/Meshtastic-Apple
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -e
git diff --stat -- docs
printf '\nChanged docs names:\n'
git diff --name-status -- docs
printf '\nceek EventFirmwareEntity outline/contents:\n'
wc -l Meshtastic/Model/EventFirmwareEntity.swift
ast-grep outline Meshtastic/Model/EventFirmwareEntity.swift || true
printf '\nRelevant EventFirmwareEntity lines:\n'
sed -n '1,140p' Meshtastic/Model/EventFirmwareEntity.swift
printf '\nSearch EventFirmwareEntity references:\n'
rg -n 'EventFirmwareEntity|firmwareZipUrl|themePrimaryColor|themeSecondaryColor|themeAccentColor|Entity' -g '!*.xcodeproj*' -g '!Derived/**' . | head -200Repository: meshtastic/Meshtastic-Apple
Length of output: 32339
Update SwiftData documentation for the model change.
Meshtastic/Model/EventFirmwareEntity.swift is a @Model change, but docs/developer/swiftdata.md and docs/developer/architecture.md are unchanged. Add the relevant schema/model notes there, especially around the removed stored field and persisting the theme values.
🤖 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/EventFirmwareEntity.swift` around lines 60 - 65, Update the
SwiftData documentation in swiftdata.md and architecture.md to reflect the
EventFirmwareEntity `@Model` changes: document removal of the obsolete stored
field and persistence of themePrimaryColor, themeSecondaryColor, and
themeAccentColor, including any relevant schema or migration notes.
Source: Path instructions
| if modelType == EventFirmwareEntity.self { | ||
| continue // global display cache, not data owned by the connected radio | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether PersistenceController.clearDatabase also excludes EventFirmwareEntity
rg -n -A5 -B5 'EventFirmwareEntity' Meshtastic/Persistence/Persistence.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files containing UpdateSwiftData.swift:"
fd -a 'UpdateSwiftData.swift|Persistence.swift' . || true
echo
echo "Occurrences of clearDatabase and EventFirmwareEntity:"
rg -n 'clearDatabase|EventFirmwareEntity' Meshtastic || true
echo
echo "Candidate file line counts:"
for f in $(fd -a 'UpdateSwiftData.swift|Persistence.swift' Meshtastic); do
wc -l "$f"
doneRepository: meshtastic/Meshtastic-Apple
Length of output: 7742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Meshtastic/Persistence/Persistence.swift: clearDatabase section"
sed -n '223,280p' Meshtastic/Persistence/Persistence.swift | nl -ba -v223
echo
echo "Meshtastic/Persistence/UpdateSwiftData.swift: clearDatabase excerpt"
sed -n '167,235p' Meshtastic/Persistence/UpdateSwiftData.swift | nl -ba -v167
echo
echo "MeshPackets.clearDatabase excerpt"
sed -n '80,135p' Meshtastic/Helpers/MeshPackets.swift | nl -ba -v80
echo
echo "Read-only structural check: model type lists and EventFirmwareEntity exclusions"
python3 - <<'PY'
from pathlib import Path
for path in [
Path("Meshtastic/Persistence/Persistence.swift"),
Path("Meshtastic/Persistence/UpdateSwiftData.swift"),
Path("Meshtastic/Helpers/MeshPackets.swift"),
]:
text = path.read_text()
print(f"\n--- {path} ---")
if "clearDatabase" in text:
idx = text.index("public func clearDatabase")
snippet = text[idx:text.index("\n}", idx)+2]
print("contains EventFirmwareEntity.self:", "EventFirmwareEntity.self" in snippet)
print("contains EventFirmwareEntity (plain):", "EventFirmwareEntity" in snippet and "EventFirmwareEntity.self" not in snippet)
for line in snippet.splitlines():
s=line.strip()
if "ModelDescriptor" in s or "EventFirmwareEntity" in s or "for entity" in s or "delete" in s:
print(s)
PYRepository: meshtastic/Meshtastic-Apple
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Meshtastic/Persistence/Persistence.swift: clearDatabase section"
sed -n '223,280p' Meshtastic/Persistence/Persistence.swift
echo
echo "Meshtastic/Persistence/UpdateSwiftData.swift: clearDatabase excerpt"
sed -n '167,235p' Meshtastic/Persistence/UpdateSwiftData.swift
echo
echo "MeshPackets.clearDatabase excerpt"
sed -n '80,135p' Meshtastic/Helpers/MeshPackets.swift
echo
echo "Read-only structural check: model type lists and EventFirmwareEntity exclusions"
python3 - <<'PY'
from pathlib import Path
for path in [
Path("Meshtastic/Persistence/Persistence.swift"),
Path("Meshtastic/Persistence/UpdateSwiftData.swift"),
Path("Meshtastic/Helpers/MeshPackets.swift"),
]:
text = path.read_text()
print(f"\n--- {path} ---")
if "public func clearDatabase" in text:
idx = text.index("public func clearDatabase")
# Find the next top-level } closing the function declaration block conservatively
brace_start = text.index("{", idx)
depth = 0
end = None
for i in range(brace_start, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
if end is None:
snippet = text[idx:]
else:
snippet = text[idx:end]
print("contains EventFirmwareEntity.self:", "EventFirmwareEntity.self" in snippet)
print("contains EventFirmwareEntity (plain):", "EventFirmwareEntity" in snippet and "EventFirmwareEntity.self" not in snippet)
for line in snippet.splitlines():
s=line.strip()
if "ModelDescriptor" in s or "EventFirmwareEntity" in s or "for entity" in s or "delete" in s:
print(s)
PYRepository: meshtastic/Meshtastic-Apple
Length of output: 11126
Keep full app data clear in sync with device clear.
UpdateSwiftData.clearDatabase() now skips EventFirmwareEntity, but PersistenceController.clearDatabase() still deletes every entry in MeshtasticSchema.allModels, including event firmware. A full app reset would silently wipe the cached event branding while device-switch resets preserve it; either make both paths skip EventFirmwareEntity or avoid calling this device-clear path for full app resets.
🤖 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/Persistence/UpdateSwiftData.swift` around lines 226 - 228, The
database-clearing behavior is inconsistent because
UpdateSwiftData.clearDatabase() skips EventFirmwareEntity while
PersistenceController.clearDatabase() still deletes it. Update the full app
reset path in PersistenceController.clearDatabase() to preserve
EventFirmwareEntity as well, or route full resets away from the device-clear
path, keeping both reset behaviors synchronized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/developer/swiftdata.md`:
- Around line 124-126: The SwiftData documentation incorrectly describes the V1
schema as a persisted row and may imply firmwareZipUrl remains; update the
wording in docs/developer/swiftdata.md:124-126 to refer to the model or clarify
that V1 is unreleased, then regenerate synchronized bundled outputs in
Meshtastic/Resources/docs/markdown/developer/swiftdata.md:124-126 and
Meshtastic/Resources/docs/developer/swiftdata.html:144-146.
🪄 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: 48cd0311-f4b9-4c45-ab2d-82928f1d8069
📒 Files selected for processing (12)
Meshtastic/API/MeshtasticAPI.swiftMeshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/Contents.jsonMeshtastic/Assets.xcassets/EventFirmwareFAB.imageset/Contents.jsonMeshtastic/Persistence/Persistence.swiftMeshtastic/Resources/docs/developer/architecture.htmlMeshtastic/Resources/docs/developer/swiftdata.htmlMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/developer/architecture.mdMeshtastic/Resources/docs/markdown/developer/swiftdata.mdMeshtasticTests/EventFirmwareMetadataTests.swiftdocs/developer/architecture.mddocs/developer/swiftdata.md
🚧 Files skipped from review as they are similar to previous changes (4)
- Meshtastic/Assets.xcassets/EventFirmwareFAB.imageset/Contents.json
- Meshtastic/Assets.xcassets/EventFirmwareDEFCON.imageset/Contents.json
- MeshtasticTests/EventFirmwareMetadataTests.swift
- Meshtastic/API/MeshtasticAPI.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/EventFirmwareNotificationPolicy.swift`:
- Around line 1-5: Update the file header in
EventFirmwareNotificationPolicy.swift by replacing the filename banner with the
prescribed file-level “// MARK: FileName” marker or the project’s standard
copyright header, keeping it at the top of the file.
🪄 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: b2c1f83f-671d-4e45-a503-f3b1ad574419
📒 Files selected for processing (7)
Meshtastic/API/MeshtasticAPI.swiftMeshtastic/Accessory/Accessory Manager/AccessoryManager+FromRadio.swiftMeshtastic/Model/EventFirmwareEntity.swiftMeshtastic/Model/EventFirmwareNotificationPolicy.swiftMeshtastic/Model/EventFirmwarePresentation.swiftMeshtastic/Views/Connect/Connect.swiftMeshtasticTests/EventFirmwareMetadataTests.swift
💤 Files with no reviewable changes (1)
- Meshtastic/Accessory/Accessory Manager/AccessoryManager+FromRadio.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Meshtastic/Model/EventFirmwarePresentation.swift
- Meshtastic/Views/Connect/Connect.swift
- Meshtastic/API/MeshtasticAPI.swift
- MeshtasticTests/EventFirmwareMetadataTests.swift
- Meshtastic/Model/EventFirmwareEntity.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Meshtastic/Resources/docs/index.json (1)
128-156: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCheck
docs-deploy.ymlas part of the docs bundle update.These
index.jsonkeyword/charCountchanges look generated, but the deployment job only checks thatcharCountvalues are non-zero, anddocs-indexis no longer in the workflow. Make sure the generated index is validated againstscripts/build-docs.sh --output Meshtastic/Resources/docsso these entries cannot drift again.🤖 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 128 - 156, Update the docs bundle validation workflow around docs-deploy.yml and scripts/build-docs.sh so the generated Meshtastic/Resources/docs/index.json is rebuilt and compared against the checked-in output, rather than only checking non-zero charCount values. Restore or add the docs-index validation step and ensure keyword and charCount changes cannot drift from the build output.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Meshtastic/Resources/docs/index.json`:
- Around line 128-156: Update the docs bundle validation workflow around
docs-deploy.yml and scripts/build-docs.sh so the generated
Meshtastic/Resources/docs/index.json is rebuilt and compared against the
checked-in output, rather than only checking non-zero charCount values. Restore
or add the docs-index validation step and ensure keyword and charCount changes
cannot drift from the build output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 797801eb-0e7d-405c-9ede-47a9792edbe4
📒 Files selected for processing (5)
Meshtastic/Model/EventFirmwareNotificationPolicy.swiftMeshtastic/Resources/docs/developer/swiftdata.htmlMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/developer/swiftdata.mddocs/developer/swiftdata.md
🚧 Files skipped from review as they are similar to previous changes (4)
- Meshtastic/Model/EventFirmwareNotificationPolicy.swift
- Meshtastic/Resources/docs/developer/swiftdata.html
- Meshtastic/Resources/docs/markdown/developer/swiftdata.md
- docs/developer/swiftdata.md
Summary
Safety boundary
This is a branding and presentation PR only. The event metadata feed is informational and cannot authorize or install firmware. It contains no executable artifact URLs or hashes, and this change adds no OTA routing, geofence, Live Activity, or event firmware installer. Those pieces require a separate reviewed artifact contract.
Validation
Screenshots
Summary by CodeRabbit