fix(geofence): restrict crossing alerts to creator, add per-geofence opt-in#6117
Merged
Conversation
…opt-in Geofence waypoints are mesh-broadcast, so every device in range stored the waypoint and GeofenceMonitor evaluated crossings for all of them — meaning *every* user got the creator's crossing notifications (the reported bug). Now the monitor only tracks geofences this device created (DataPacket.isFromLocal), combined with myNodeNum so the filter recomputes once our node number is known. Receivers can still opt in to a specific foreign geofence: tapping one opens a new read-only WaypointInfoDialog (reached for both locked and unlocked foreign geofences, so the previously dead-end locked case now has a view) with a "Notify me of crossings" toggle. The toggle writes a local pref only — it never edits or re-broadcasts the waypoint. Unlocked foreign geofences keep an Edit button into the full editor. The opt-in set is stored insertion-ordered and capped (oldest evicted past the max) so it can't grow unbounded as waypoints churn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThis PR adds per-geofence crossing alert opt-in support across preferences, geofence monitoring, map view models, and map UI. It also adds a new waypoint info dialog for foreign geofence waypoints with localization, previews, and screenshot coverage. ChangesGeofence Alert Opt-In Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5 tasks
![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: 1
🧹 Nitpick comments (6)
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
FakeNotificationPrefsinstead of mocking here.
core/testing.FakeNotificationPrefsalready defaultsgeofenceAlertOptInstoemptySet(), so this could benotificationPrefs = FakeNotificationPrefs()instead of manually mocking and stubbing the property — less boilerplate, consistent with the fake-based approach already used formeshBeaconRepository/fakeBeaconPrefsin this same file.Also applies to: 151-154
core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt (1)
98-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the "retoggle moves to most-recent" guarantee.
Existing tests cover basic add/remove and cap eviction (FIFO), but not the specific behavior documented in
NotificationPrefsImpl.setGeofenceAlertOptInwhere re-enabling an already-opted-in id resets its eviction priority to newest. A test filling to the cap, then retoggling the oldest surviving id, then adding one more to trigger eviction, would confirm that behavior and guard against regressions.🤖 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/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt` around lines 98 - 118, The current tests in NotificationPrefsTest cover add/remove and FIFO eviction, but they do not verify the retoggle-to-newest behavior implemented in NotificationPrefsImpl.setGeofenceAlertOptIn. Add a test that fills geofenceAlertOptIns to MAX_GEOFENCE_OPT_INS, re-enables an already present id to make it most recent, then adds one more id to force eviction and assert the retoggled id is preserved while the true oldest is removed. Use the existing geofenceAlertOptIns and setGeofenceAlertOptIn symbols to keep the test aligned with the implementation.core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/GeofenceMonitorTest.kt (1)
84-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an end-to-end positive test for opted-in foreign geofences.
The harness already supports
optedInIds, but no test exercises "foreign + opted-in → notification fires" through the fullGeofenceMonitorpipeline (onlyremoteCreatedGeofenceDoesNotNotifycovers the negative case here; the positive path is only unit-tested inActiveWaypointsTest.kt). Adding one would close the integration-level coverage gap for the new opt-in feature.🤖 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/commonTest/kotlin/org/meshtastic/core/data/manager/GeofenceMonitorTest.kt` around lines 84 - 104, Add an end-to-end positive test in GeofenceMonitorTest that exercises the full GeofenceMonitor pipeline for a foreign geofence with opted-inIds set, verifying that a notification is emitted when the remote creator is opted in. Reuse the existing mocks(...) helper and the GeofenceMonitor flow setup, and mirror the negative remote-created case with an asserted positive notification path so the opt-in behavior is covered at integration level.core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/GeofenceMonitor.kt (1)
94-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCombined flow has no retry after a single failure.
.catch { ... }on the combined flow logs and then completes the flow — there's noretry/retryWhen, so any single exception fromgetWaypoints(),myNodeNum, ornotificationPrefs.geofenceAlertOptInspermanently stops geofence tracking for the rest of the session. This is now a 3-way surface (previously only the waypoints stream), andgeofenceAlertOptInsis backed by a DataStore-preferences read that can throwIOExceptionon a read/corruption failure.♻️ Add retry before collecting
combine( packetRepository.value.getWaypoints(), nodeManager.myNodeNum, notificationPrefs.geofenceAlertOptIns, ) { packets, myNodeNum, optIns -> packets.activeWaypointPackets(nowSeconds).values.geofencesToMonitor(myNodeNum, optIns) } + .retryWhen { cause, attempt -> + Logger.e(cause) { "Geofence waypoint stream failed (attempt $attempt); retrying" } + delay(RETRY_BACKOFF_MS) + true + } .catch { Logger.e(it) { "Geofence waypoint stream failed; geofence tracking paused" } } .collect { active ->Please confirm whether the DataStore instance backing
NotificationPrefsImpl.geofenceAlertOptInsconfigures acorruptionHandler/upstream.catchthat already convertsIOExceptionto a fallback value, which would reduce the likelihood of this reachingGeofenceMonitor.🤖 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/commonMain/kotlin/org/meshtastic/core/data/manager/GeofenceMonitor.kt` around lines 94 - 106, The combined geofence flow in GeofenceMonitor currently logs failures with catch and then ends, so tracking can stop permanently after one transient error. Update the flow built in the scope.launch/combine chain to retry before collect, using a retry or retryWhen strategy around the upstream getWaypoints, myNodeNum, and notificationPrefs.geofenceAlertOptIns sources while preserving the existing error log in the catch block. Also verify whether NotificationPrefsImpl.geofenceAlertOptIns already handles IOException via a corruptionHandler or upstream catch so the retry scope is only applied where needed.feature/map/src/commonMain/kotlin/org/meshtastic/feature/map/BaseMapViewModel.kt (1)
111-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a unit test for
isMyWaypoint.The new public
isMyWaypoint(id)has no direct test coverage inBaseMapViewModelTest(only the constructor wiring was updated). Given it now gates whether the UI shows an editable vs. read-only dialog, a couple of focused tests (local waypoint, foreign waypoint, waypoint missing from map) would guard against regressions.🤖 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/map/src/commonMain/kotlin/org/meshtastic/feature/map/BaseMapViewModel.kt` around lines 111 - 119, The new public BaseMapViewModel.isMyWaypoint(id) helper is not directly covered by tests. Add focused unit tests in BaseMapViewModelTest that exercise the isMyWaypoint path for a local waypoint, a foreign waypoint, and a missing waypoint, using the BaseMapViewModel constructor setup and waypoint state so the method’s return value is verified independently of the UI.androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt (1)
725-745: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStale waypoint snapshot while dialog is open.
waypointis captured once fromgeofenceInfoWaypointwhen the dialog opens; if the underlying waypoint packet updates (or expires) while the dialog is showing, the dialog won't reflect the change (e.g.locked_totoggling would leave a stale Edit button enablement). Low impact since the dialog is short-lived and self-corrects on next open.🤖 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 `@androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt` around lines 725 - 745, The dialog is using a stale waypoint snapshot from geofenceInfoWaypoint, so changes to the underlying packet while WaypointInfoDialog is open won’t be reflected. Update MapView.kt so the dialog reads the latest waypoint state on each recomposition instead of relying on the captured waypoint value, especially for fields like locked_to that control onEdit. Use the existing geofenceInfoWaypoint selection and MapViewModel state to resolve the current waypoint data before passing it into WaypointInfoDialog.
🤖 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 `@androidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.kt`:
- Around line 828-848: The edit action in WaypointInfoDialog is currently
enabled for any unlocked foreign geofence, even when the app is disconnected.
Update the onEdit assignment in MapView.kt for both map flavors so it is only
provided when waypoint.locked_to == 0 and isConnected is true, keeping onEdit
null otherwise; use the existing showGeofenceInfoDialog branch and
WaypointInfoDialog callsite to apply the same gate consistently.
---
Nitpick comments:
In `@androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt`:
- Around line 725-745: The dialog is using a stale waypoint snapshot from
geofenceInfoWaypoint, so changes to the underlying packet while
WaypointInfoDialog is open won’t be reflected. Update MapView.kt so the dialog
reads the latest waypoint state on each recomposition instead of relying on the
captured waypoint value, especially for fields like locked_to that control
onEdit. Use the existing geofenceInfoWaypoint selection and MapViewModel state
to resolve the current waypoint data before passing it into WaypointInfoDialog.
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/GeofenceMonitor.kt`:
- Around line 94-106: The combined geofence flow in GeofenceMonitor currently
logs failures with catch and then ends, so tracking can stop permanently after
one transient error. Update the flow built in the scope.launch/combine chain to
retry before collect, using a retry or retryWhen strategy around the upstream
getWaypoints, myNodeNum, and notificationPrefs.geofenceAlertOptIns sources while
preserving the existing error log in the catch block. Also verify whether
NotificationPrefsImpl.geofenceAlertOptIns already handles IOException via a
corruptionHandler or upstream catch so the retry scope is only applied where
needed.
In
`@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/GeofenceMonitorTest.kt`:
- Around line 84-104: Add an end-to-end positive test in GeofenceMonitorTest
that exercises the full GeofenceMonitor pipeline for a foreign geofence with
opted-inIds set, verifying that a notification is emitted when the remote
creator is opted in. Reuse the existing mocks(...) helper and the
GeofenceMonitor flow setup, and mirror the negative remote-created case with an
asserted positive notification path so the opt-in behavior is covered at
integration level.
In
`@core/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/notification/NotificationPrefsTest.kt`:
- Around line 98-118: The current tests in NotificationPrefsTest cover
add/remove and FIFO eviction, but they do not verify the retoggle-to-newest
behavior implemented in NotificationPrefsImpl.setGeofenceAlertOptIn. Add a test
that fills geofenceAlertOptIns to MAX_GEOFENCE_OPT_INS, re-enables an already
present id to make it most recent, then adds one more id to force eviction and
assert the retoggled id is preserved while the true oldest is removed. Use the
existing geofenceAlertOptIns and setGeofenceAlertOptIn symbols to keep the test
aligned with the implementation.
In
`@feature/map/src/commonMain/kotlin/org/meshtastic/feature/map/BaseMapViewModel.kt`:
- Around line 111-119: The new public BaseMapViewModel.isMyWaypoint(id) helper
is not directly covered by tests. Add focused unit tests in BaseMapViewModelTest
that exercise the isMyWaypoint path for a local waypoint, a foreign waypoint,
and a missing waypoint, using the BaseMapViewModel constructor setup and
waypoint state so the method’s return value is verified independently of the UI.
🪄 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: 005a4a27-3c54-4453-b169-20e640e72777
📒 Files selected for processing (20)
.skills/compose-ui/strings-index.txtandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapViewModel.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapViewModel.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/component/WaypointMarkers.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/GeofenceMonitor.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/GeofenceMonitorTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/geofence/ActiveWaypoints.ktcore/model/src/commonTest/kotlin/org/meshtastic/core/model/geofence/ActiveWaypointsTest.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/resources/src/commonMain/composeResources/values/strings.xmlcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeNotificationPrefs.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/WaypointInfoDialog.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/BaseMapViewModel.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/SharedMapViewModel.ktfeature/map/src/commonTest/kotlin/org/meshtastic/feature/map/BaseMapViewModelTest.kt
Address CodeRabbit review on #6117: - WaypointInfoDialog's Edit button now requires `isConnected` in both map flavors — editing a foreign geofence re-broadcasts it, which needs a live connection, so the affordance is hidden when offline. - MeshDataHandlerTest uses FakeNotificationPrefs instead of a hand-stubbed mock. - Add a NotificationPrefs test for retoggle-refreshes-eviction-order on the capped opt-in set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jamesarich
enabled auto-merge
July 6, 2026 19:07
Two previews (locked read-only, unlocked opted-in with Edit) × light/dark, wired into MapScreenshotTests with generated reference PNGs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DesktopNotificationManagerTest has its own private FakeNotificationPrefs; add geofenceAlertOptIns + setGeofenceAlertOptIn so :desktopApp:test compiles after the interface gained the geofence opt-in members. (The shard-app CI shard runs :desktopApp:test, which wasn't covered by the earlier assembleDebug check.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Why
Users reported that every node in range was receiving geofence crossing notifications, not just the geofence's creator. Geofence waypoints are mesh-broadcast, so every device stored the waypoint and
GeofenceMonitorevaluated crossings for all of them — so everyone got the creator's alerts.🐛 Fix
GeofenceMonitornow tracks only geofences this device created (DataPacket.isFromLocal), combined withmyNodeNumso the filter recomputes once our node number is known (a waypoint can arrive before we're connected).🌟 Per-geofence opt-in
Receivers can still choose to be alerted on someone else's geofence:
WaypointInfoDialog— reached for both locked and unlocked foreign geofences, so the previously dead-end locked case (which used to only toast / show a delete dialog) now has a view.GeofenceMonitortracks a geofence whenisFromLocal(myNodeNum) || id in optedIn.🧹 Housekeeping
Collection<DataPacket>.geofencesToMonitor(myNodeNum, optedInIds)incore:model.WaypointInfoDialog's toggle row reuses the sharedcore:uiBasicListItemrather than a hand-rolled layout.Reviewer notes
NotificationPrefsinterface gainedgeofenceAlertOptIns+setGeofenceAlertOptIn;BaseMapViewModelgained the opt-in accessors andisMyWaypoint, rippling to the flavor ViewModels.Testing Performed
ActiveWaypointsTest(creator-only / opt-in-adds-foreign / drops-silent),NotificationPrefsTest(set round-trip + cap eviction),GeofenceMonitorTest(remote-created geofence stays silent), plusMeshDataHandlerTest/BaseMapViewModelTestconstructor updates../gradlew spotlessCheck detekt— clean../gradlew :core:data:allTests :feature:map:allTests :core:prefs:allTests— green../gradlew assembleDebug— bothgoogleandfdroidflavors compile.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Screenshots
New read-only
WaypointInfoDialog, shown when you tap a geofence you didn't create. Locked geofences are view-only; unlocked ones keep an Edit path into the full editor. The "Notify me of crossings" switch is the receiver-local opt-in.