fix(event): honor disabled node events, gate brand URLs, observe the manifest - #6499
Conversation
…manifest Three defects found while reconciling the cross-platform spec (meshtastic/design#132) against what actually shipped. **Node-event notifications could be silently switched back on.** Connecting to event firmware set the auto-disabled flag without checking whether node events were even on, so a user who had turned them off themselves got the flag set — and the vanilla branch then re-enabled them on the next ordinary connection, discarding a preference we never changed. Only claim the restore when we are the one turning them off. **Manifest URLs reached the URI handler unchecked.** Event links went straight to openUri() after only a blank check, and iconUrl straight to the image loader. The manifest is first-party but arrives over the network, and a URI handler honors whatever scheme it is given. Require an absolute https URL with a real host, rejecting cleartext, protocol-relative, non-http schemes, and userinfo that lets a hostile host masquerade as a trusted one. **A manifest refresh never reached branding already on screen.** eventEdition resolved the edition with a suspending one-shot inside combine(), so metadata landing after connection sat in the cache until the user reconnected. Add observeEdition() backed by a Room Flow and flatMapLatest onto it, so a refresh re-emits into the live UI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEvent firmware editions now support reactive cache observation and asynchronous refresh updates in the UI. Event branding links and icons require safe HTTPS URLs. Firmware notification defaults preserve user-disabled settings while managing automatic non-VANILLA disabling. ChangesReactive event-edition observation
Event branding URL safety
Firmware notification defaults
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UIViewModel
participant EventFirmwareRepositoryImpl
participant EventFirmwareEditionDao
participant Refresher
UIViewModel->>EventFirmwareRepositoryImpl: observeEdition(editionName)
EventFirmwareRepositoryImpl->>Refresher: trigger non-blocking refresh
EventFirmwareRepositoryImpl->>EventFirmwareEditionDao: observe edition row
EventFirmwareEditionDao-->>UIViewModel: emit cached edition
Refresher->>EventFirmwareEditionDao: store refreshed edition
EventFirmwareEditionDao-->>UIViewModel: emit updated edition
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.kt`:
- Around line 497-508: The preference decision and paired updates in the
non-VANILLA/VANILLA handling must be atomic. Add or reuse a
serialized/transactional operation on NotificationPrefs that reads
nodeEventsEnabled and nodeEventsAutoDisabledForEvent and performs the
conditional updates together; update this flow to invoke that operation instead
of separate reads and asynchronous writes, preserving the existing restore
behavior.
In
`@core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.kt`:
- Line 37: Update the SQL query used by observeByEdition in
EventFirmwareEditionDao to include LIMIT 1, preserving the existing edition
filter and single-entity observable contract.
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt`:
- Around line 68-74: Update isSafeBrandUrl in
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt:68-74
to parse the URL with the project’s multiplatform URI implementation and require
a valid, non-empty hostname, replacing manual authority slicing while preserving
HTTPS-only validation. Add rejection tests in
core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt:128-169
for https://:443, malformed IPv6 authorities, and hosts containing whitespace.
🪄 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: e833b038-a201-4e3d-a4da-892728cbccd6
📒 Files selected for processing (11)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt
Addresses CodeRabbit review on #6499. isSafeBrandUrl only checked that the authority was non-empty and free of '@', so "https://:443" (a port with no host) and "https://ho st/x" both passed. Match the authority whole instead — a registered name or bracketed IPv6 literal plus an optional numeric port — which also rejects truncated IPv6 literals and userinfo, since none of those characters are in the permitted set. Add LIMIT 1 to observeByEdition, per the repo guideline for single-row queries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeRabbit was right on #6499: the guard read nodeEventsEnabled and nodeEventsAutoDisabledForEvent and then issued two writes, and those writes are asynchronous — every NotificationPrefs setter is scope.launch { dataStore.edit } and the StateFlows are stateIn over dataStore.data, so they lag the write. The window is not a couple of instructions; it spans until the launched coroutine runs. A user toggling node events in that window can be clobbered last-write-wins, and back-to-back handleMyInfo calls read stale values. Move the whole read-decide-write into NotificationPrefs as one DataStore edit block. DataStore serializes edits, so the values examined are the same snapshot written, and the decision can no longer interleave with a user toggle. The decision table now lives in NotificationPrefsTest against a real DataStore (including the full off-by-user → event → vanilla round trip that started this); MeshConfigFlowManagerImplTest keeps only the delegation, plus a case pinning DIY_EDITION as event firmware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt (1)
46-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate local fake — reuse
core:testing'sFakeNotificationPrefs.This test defines its own
NotificationPrefsfake instead of the shared one; the newly-added no-op override at Line 69-71 is direct evidence of the double-maintenance cost — every interface addition must be replicated here separately. Since the shared fake already implements the realapplyEventFirmwareNodeEventDefaultbranch logic, switching to it (using its default constructor + setter calls instead of named constructor params) removes this duplication going forward.As per path instructions, "Place shared test fakes, doubles, and utilities in
core:testing."🤖 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 `@desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt` around lines 46 - 82, Remove the local FakeNotificationPrefs class from DesktopNotificationManagerTest and reuse core:testing’s shared FakeNotificationPrefs instead. Update the tests to construct the shared fake with its default constructor and configure messages, node events, and low-battery settings through the existing setter methods, preserving the current test behavior.Source: Path instructions
🤖 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
`@core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt`:
- Around line 137-153: The test observeEditionReEmitsWhenTheCacheIsRefreshed
currently bypasses the refresh path by directly upserting into local. Update it
to verify collection triggers the refresh, using eventFirmwareCalls or a gated
fake response so the observed DEFCON update is produced by the repository
refresh; retain the initial null assertion and final emission assertion.
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt`:
- Around line 73-81: Normalize branding URLs once and propagate the normalized
values to every consumer: update isSafeBrandUrl/safeLinks in
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt
(lines 73-81) to return and retain trimmed safe URLs, use that normalized icon
URL for Coil at lines 97-97, and render/open normalized links in
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt
(lines 129-130). Add a whitespace-padded URL regression case in
core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt
(lines 168-183).
---
Nitpick comments:
In
`@desktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt`:
- Around line 46-82: Remove the local FakeNotificationPrefs class from
DesktopNotificationManagerTest and reuse core:testing’s shared
FakeNotificationPrefs instead. Update the tests to construct the shared fake
with its default constructor and configure messages, node events, and
low-battery settings through the existing setter methods, preserving the current
test behavior.
🪄 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: 7d1be482-c7e9-47e4-bdd4-1e1716619da8
📒 Files selected for processing (16)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConfigFlowManagerImplTest.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.ktcore/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsImpl.ktcore/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EventFirmwareRepository.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.ktdesktopApp/src/test/kotlin/org/meshtastic/desktop/notification/DesktopNotificationManagerTest.kt
Addresses the full CodeRabbit review on #6499. The validator trimmed its input but callers forwarded the *original* string, so a whitespace-padded URL passed validation and then reached Coil and the URI handler malformed. safeBrandUrlOrNull() now returns the validated form, safeLinks() carries it, and the icon uses it; isSafeBrandUrl() is the boolean wrapper. The re-emission test wrote the new edition into the cache by hand, so it proved Room re-emits but not that observeEdition() refreshes at all — it would have kept passing if maybeRefresh() were removed. The edition now arrives only via the network response, and the fetch count is asserted, so collection driving the refresh is what the test actually pins. No assertion on the first emission: these tests run on Dispatchers.Unconfined, where the refresh can complete inline before emitAll starts, so observing the seed-only null is timing rather than contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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)
core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt (1)
20-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse Turbine for Flow assertions.
observeEditionReEmitsWhenTheCacheIsRefreshed()mutates a sharedMutableListfrom the collector and polling coroutine, which can race across the dispatched Room/Flow path. Use Turbine to await the optional initial emission and the refreshed"defcon"emission, then cancel remaining events; this also satisfies the Flow-testing guideline.Proposed shape
- val emissions = mutableListOf<String?>() - val collector = launch { repository.observeEdition("DEFCON").collect { emissions += it?.displayName } } - - withTimeout(EMISSION_TIMEOUT_MS) { while (emissions.lastOrNull() == null) delay(EMISSION_POLL_MS) } - assertEquals("defcon", emissions.last()) - assertEquals(1, api.eventFirmwareCalls) - collector.cancel() + repository.observeEdition("DEFCON").test { + val first = awaitItem() + val refreshed = if (first == null) awaitItem() else first + assertEquals("defcon", refreshed) + assertEquals(1, api.eventFirmwareCalls) + cancelAndIgnoreRemainingEvents() + }🤖 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/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt` around lines 20 - 24, Update observeEditionReEmitsWhenTheCacheIsRefreshed() to use Turbine assertions instead of a shared MutableList, delay, launch, and polling. Await the optional initial emission and then the refreshed "defcon" emission through Turbine, and cancel remaining events when complete.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
`@core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.kt`:
- Around line 20-24: Update observeEditionReEmitsWhenTheCacheIsRefreshed() to
use Turbine assertions instead of a shared MutableList, delay, launch, and
polling. Await the optional initial emission and then the refreshed "defcon"
emission through Turbine, and cancel remaining events when complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b312136-2342-4ec6-bc46-f6a5e4ba116f
📒 Files selected for processing (3)
core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.kt
- core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.kt
Addresses a CodeRabbit nitpick on #6499. desktopApp's notification test carried its own NotificationPrefs fake, so adding applyEventFirmwareNodeEventDefault to the interface meant writing a second no-op override here — the double-maintenance cost the repo's "shared fakes live in core:testing" rule exists to avoid. Drop the local copy and depend on core:testing. Constructor flags become setter calls, since the shared fake exposes MutableStateFlow properties. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Picked up the remaining nitpick from the full review in fd9385e: You were right that the no-op override I added was the evidence — the local copy meant every interface addition had to be written twice, against the repo's "shared fakes live in
Left alone: the pre-existing 🤖 Addressed by Claude Code |
Addresses the outside-diff finding on #6499. The re-emission test collected into a MutableList from the collector coroutine while a polling loop read it from another — Room emits on its own executor, so that was a genuine data race on an ArrayList, and it also sidestepped the repo's Flow-testing guideline. Turbine's test {} replaces the list, the polling loop, and the manual timeout. The optional first null is awaited explicitly rather than polled for: whether the refresh completes before collection starts is timing under Dispatchers.Unconfined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Picked up the outside-diff Turbine finding in 9c14d5c. Valid, and the race was real rather than theoretical:
🤖 Addressed by Claude Code |
1 participant
🌟 Why
Refreshing the cross-platform event-firmware spec (meshtastic/design#132) meant checking each documented behavior against the code rather than against the PRs that shipped it. Three of them didn't match, and each mismatch was a defect rather than a stale doc. This fixes all three and closes the Android sub-tasks that spec now tracks.
🐛 A user's disabled node notifications could be silently switched back on
applyEventFirmwareNotificationDefaultsset the auto-disabled flag whenever it saw event firmware, without checking whether node-event notifications were even on:So for a user who had turned node events off themselves: connecting to event firmware set the flag, and the next ordinary connection to vanilla firmware hit the
elsebranch and enabled them — discarding a preference the app never changed. The flag means "we turned this off, we owe a restore", so it must only be set when we actually turn it off.🔒 Manifest URLs reached the URI handler and image loader unchecked
EventInfoSheetpassedlinks[].urltoopenUri()after only a blank check, andiconUrlwent straight to Coil. The manifest is first-party, but it arrives over the network, and a platform URI handler honors whatever scheme it is handed.New
isSafeBrandUrl()requires an absolutehttpsURL with a real host. It rejects:http://…//host/xjavascript:,file:,intent://,meshtastic://https://,https:///pathhttps://api.meshtastic.org@evil.example/x.pngUnsafe links are dropped from the sheet rather than rendered-and-refused; an unsafe
iconUrlfalls through to the existing bundled → logo chain, so there is never an empty slot.🐛 A manifest refresh never reached branding already on screen
eventEditionresolved the edition with a suspending one-shot insidecombine(firmwareEdition, connectionState), and the repository exposed noFlow. The background refresh did land in the DB — and then nothing re-read it. An edition added to the hosted manifest after the user connected stayed invisible until they reconnected, which is precisely the case that matters when an event goes live during a session.EventFirmwareRepository.observeEdition()now returns a Room-backedFlow, and the ViewModelflatMapLatestes onto it. The observable triggers a refresh on collection but deliberately doesn't wait on it (maxWaitMs = 0) — the emissions carry whatever lands, so blocking would only delay the cached value the UI could already be showing.getEdition()keeps its bounded wait for one-shot callers; the shared staleness/cooldown logic moved intomaybeRefresh().🧪 Testing performed
./gradlew spotlessCheck detekt assembleDebug test allTests— green.MeshConfigFlowManagerImplTest— the regression (already-off node events must not claim the restore), plus a guard that the auto-disable still fires when they're on, so the new check can't be over-tightened into never disabling.EventBrandingTest—isSafeBrandUrl()accept/reject including every scheme above and the userinfo case, andsafeLinks()dropping unsafe entries while preserving order.EventFirmwareRepositoryImplTest—observeEditionReEmitsWhenTheCacheIsRefreshedcollects an edition absent from the seed, writes it, and asserts the live flow re-emits it; plus null for an unknown edition.Not exercised on a physically connected event device — branding only renders while on event firmware. The three fixes are unit-covered, and the URL and notification paths are pure logic.
🔗 Related
🤖 Generated with Claude Code
Summary by CodeRabbit