fix(map): allow local deletion of any waypoint, locked or not - #6497
Merged
Conversation
A waypoint locked to another node was permanently stuck on our own map. The google flavor answered a tap with a "Locked" toast and nothing else, and the foreign-geofence info dialog offered no removal at all — so a stale or unwanted foreign waypoint could never be cleared, even though dropping our local copy needs no mesh-wide permission. Local deletion is now always available; only the expire=1 broadcast stays gated on isModifiableBy(myNodeNum) && isConnected. - new shared DeleteWaypointDialog: "Delete for me" always, "Delete for everyone" only when we may modify the waypoint mesh-wide. Replaces the hand-rolled fdroid copy so both flavors share one dialog. - WaypointMarkers routes the locked-to-someone-else branch to the delete dialog instead of a toast. - WaypointInfoDialog gains a local-delete action, offered even when the geofence is locked to its creator. Screenshot references re-recorded; two previews added for the new dialog.
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWaypoint deletion now uses a shared confirmation dialog with separate local and mesh-wide actions. Locked waypoint interactions open the dialog, waypoint information dialogs expose local deletion, and preview screenshot coverage covers both permission states. ChangesWaypoint deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant WaypointMarkers
participant MainMapContent
participant MapView
participant DeleteWaypointDialog
participant mapViewModel
WaypointMarkers->>MainMapContent: request deletion for locked waypoint
MainMapContent->>MapView: store selected waypoint
MapView->>DeleteWaypointDialog: display deletion choices
DeleteWaypointDialog->>mapViewModel: deleteWaypoint(id)
DeleteWaypointDialog->>mapViewModel: sendWaypoint(expire = 1)
mapViewModel->>mapViewModel: deleteWaypoint(id)
🚥 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[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
🤖 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/google/kotlin/org/meshtastic/app/map/component/WaypointMarkers.kt`:
- Around line 105-107: Update the waypoint action branching around
waypoint.isModifiableBy(myNodeNum) so the edit path requires modifiability,
while every non-modifiable waypoint routes to onDeleteWaypointRequest(waypoint)
regardless of connection state. Preserve local deletion as unconditional,
including waypoints locked by another node.
In
`@feature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialog.kt`:
- Around line 47-69: Replace the inline AlertDialog in DeleteWaypointDialog with
the shared dialog-host architecture, routing the dialog through
AlertHost(alertManager) or SharedDialogs from core:ui/commonMain. Preserve the
existing delete-for-me, conditional delete-for-everyone, and cancel actions and
their ordering while removing the direct alert-rendering 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3cfeaac-f38b-4355-82bd-d969665e2a2b
⛔ Files ignored due to path filters (8)
screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogBoth_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogBoth_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogLocalOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogLocalOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogOptedIn_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogOptedIn_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogReadOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogReadOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (8)
androidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/component/WaypointMarkers.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialog.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialogPreviews.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/WaypointInfoDialog.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/WaypointInfoDialogPreviews.ktscreenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/feature/MapScreenshotTests.kt
- WaypointMarkers: the edit branch now requires isModifiableBy alone. The old `|| !isConnected` meant a waypoint locked to another node opened the (useless) editor while offline and only reached the delete dialog while connected. Every non-modifiable waypoint now routes to the delete dialog regardless of connectivity, which also drops the isConnected parameter from WaypointMarkers and MainMapContent. - DeleteWaypointDialog: built on the shared MeshtasticDialog instead of an inline AlertDialog, per the centralized-dialog rule in .skills/code-review. Confirming always deletes locally; the mesh-wide broadcast is now an opt-in checkbox shown only when we may modify the waypoint mesh-wide. That also replaces the ragged three-button column with a standard Delete/Cancel pair. Screenshot references re-recorded; the Both preview is renamed WithBroadcast to match the checkbox.
Contributor
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
`@feature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialog.kt`:
- Around line 56-84: Guard the confirmation logic in DeleteWaypointDialog with
canDeleteForEveryone so onDeleteForEveryone() is invoked only while mesh-wide
deletion remains eligible; otherwise call onDeleteForMe(). Optionally key or
reset the remembered alsoDeleteForEveryone state when canDeleteForEveryone
changes so stale selection cannot persist.
🪄 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: d941c5db-a866-42c6-8178-6d06874bc0af
⛔ Files ignored due to path filters (4)
screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogLocalOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogLocalOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogWithBroadcast_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotDeleteWaypointDialogWithBroadcast_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (5)
androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/component/WaypointMarkers.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialog.ktfeature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialogPreviews.ktscreenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/feature/MapScreenshotTests.kt
💤 Files with no reviewable changes (1)
- androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/map/src/androidMain/kotlin/org/meshtastic/feature/map/component/DeleteWaypointDialogPreviews.kt
- screenshot-tests/src/screenshotTest/kotlin/org/meshtastic/screenshots/feature/MapScreenshotTests.kt
A mid-dialog disconnect hides the "Delete for everyone" checkbox but was leaving the remembered selection set, so confirming would still broadcast the removal while no longer eligible. Key the state on canDeleteForEveryone so it resets with the checkbox, and guard the confirm on eligibility too.
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
A waypoint locked to another node was permanently stuck on our own map.
locked_tois a mesh-wide ownership rule — only the owner may broadcast an edit or removal — but the UI was treating it as a local one too:The result was that a stale, mis-placed, or simply unwanted foreign waypoint could never be removed from your own map. Dropping our local database row needs no permission from anyone, so it should always be available.
Local deletion is now unconditional. Only the
expire = 1broadcast stays gated onisModifiableBy(myNodeNum) && isConnected— the mesh-wide contract from #6343/#6348 is unchanged.🌟 Changes
DeleteWaypointDialog(feature/map), built onMeshtasticDialogfromcore:ui: confirming always drops our local copy, and broadcasting the removal to the mesh is an opt-in checkbox shown only when we may modify the waypoint mesh-wide and are connected. Local deletion is therefore the safe default action.WaypointMarkers(google): the locked-to-someone-else branch now routes toonDeleteWaypointRequestinstead of a toast, wired throughMainMapContentto adeletingWaypointdialog inMapView. The edit branch gates onisModifiableByalone — it previously also opened the editor for a foreign locked waypoint while offline, where no edit could ever be sent.WaypointInfoDialog: gained anonDeleteForMeaction, rendered in error color alongside Edit, and offered even when the geofence is locked to its creator. Both flavors pass it.🧹 Cleanup
AlertDialogis replaced by the shared composable — one copy instead of two, and it no longer bypasses the centralized-dialog rule in.skills/code-review. The now-dead "locked" toast plumbing (rememberCoroutineScope,LocalContext,showToast) and the unusedisConnectedparameter are gone fromWaypointMarkers/MainMapContent.UI
The delete dialog is a plain Delete / Cancel confirmation, gaining a "Delete for everyone" checkbox only when the removal may also be broadcast. The foreign-geofence info dialog now ends with Delete for me · Close (error-colored delete), with Edit inserted between them when the geofence is unlocked. Both dialog states are pinned by the new screenshot references in this PR.
Testing Performed
./gradlew spotlessApply spotlessCheck detekt assembleDebug test allTests— all green (738 tests)../gradlew :screenshot-tests:updateDebugScreenshotTestthenvalidateDebugScreenshotTest— green. Four existingWaypointInfoDialogreferences re-recorded for the new delete action; four new references added forDeleteWaypointDialogLocalOnlyPreview/DeleteWaypointDialogWithBroadcastPreview, which pin both gate states.Not yet exercised on-device against a real foreign locked waypoint; the gate change is UI-only and the mesh-wide permission path (
isModifiableBy) is untouched and still covered byWaypointLockTest.🤖 Generated with Claude Code
Summary by CodeRabbit