Skip to content

Fix read messages leaving notifications visible#2125

Merged
garthvh merged 1 commit into
meshtastic:mainfrom
RCGV1:codex/dismiss-read-notifications
Jul 20, 2026
Merged

Fix read messages leaving notifications visible#2125
garthvh merged 1 commit into
meshtastic:mainfrom
RCGV1:codex/dismiss-read-notifications

Conversation

@RCGV1

@RCGV1 RCGV1 commented Jul 19, 2026

Copy link
Copy Markdown
Member

Summary

  • remove delivered Meshtastic message notifications when their messages are read in-app
  • retain the existing cancellation of pending requests
  • cover the delivered-notification identifier cleanup with a focused regression test

Root cause

The read path only removed pending local notification requests. Once a notification had already been delivered, iOS continued showing it on the Lock Screen and in Notification Center.

Validation

  • xcodebuild test -project Meshtastic.xcodeproj -scheme Meshtastic -destination platform=iOS\ Simulator,id=5669CB03-8FA2-4A97-BE04-A8566204116D -only-testing:MeshtasticTests/LocalMessageNotificationCleanupTests -derivedDataPath /private/tmp/meshtastic-notification-dismiss-derived-data CODE_SIGNING_ALLOWED=NO
    • 1 test passed

Notes

The local SwiftLint pre-commit hook could not run because its installed SourceKit bridge crashes with the active Xcode (sourcekitdInProc load failure). The focused Xcode simulator test completed successfully.

Summary by CodeRabbit

  • Bug Fixes

    • Canceling notifications by message ID now also removes notifications that have already been delivered, preventing stale notifications from remaining visible.
  • Tests

    • Added coverage verifying that notifications for multiple message IDs are canceled correctly.

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d9c2382-f9a7-427d-84ce-5f64cb3dfb6d

📥 Commits

Reviewing files that changed from the base of the PR and between e531f15 and 55673ea.

📒 Files selected for processing (2)
  • Meshtastic/Helpers/LocalNotificationManager.swift
  • MeshtasticTests/MeshPacketsAndTelemetryTests.swift

📝 Walkthrough

Walkthrough

LocalNotificationManager now removes delivered notifications when canceling message IDs through an injectable closure. A test verifies the expected notification identifiers are passed for multiple message IDs.

Changes

Notification cleanup

Layer / File(s) Summary
Delivered-notification removal injection
Meshtastic/Helpers/LocalNotificationManager.swift
Adds an injectable removeDeliveredNotifications closure with a default implementation backed by UNUserNotificationCenter.
Message cancellation and validation
Meshtastic/Helpers/LocalNotificationManager.swift, MeshtasticTests/MeshPacketsAndTelemetryTests.swift
Removes delivered notification identifiers derived from message IDs and tests the identifiers for messages 123 and 456.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: garthvh

Poem

I’m a rabbit with notices tucked in a row,
Now stale ones vanish when messages go.
IDs hop cleanly, two, three, four—
Delivered alerts don’t linger anymore.
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: clearing visible notifications when read messages are handled.
Description check ✅ Passed The description covers what changed, why, and how it was tested, with only minor template sections like screenshots and checklist items left unfilled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@RCGV1
RCGV1 marked this pull request as ready for review July 19, 2026 17:19

@garthvh garthvh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Reviewed the diff: correct fix for delivered notifications lingering after in-app read. The new work is properly gated behind the existing unread early-out (never runs on the repeated poll-tick path once messages are read), makes a single non-blocking batched removeDeliveredNotifications call, and preserves the batching optimization from #1901 — no hot-path regression. All CI green.

@garthvh
garthvh merged commit 94ff367 into meshtastic:main Jul 20, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants