Cancel notifications by tag and id, not id alone

[geekygecko]

Description

NotificationHelperImpl.removeNotification() cancelled notifications by ID alone. When a notification is posted with a tag, NotificationManager.cancel(id) does not dismiss it, because Android keys active notifications by the (tag, id) pair. As a result, tagged notifications (e.g. playback error notifications) could linger after the user acted on them.

This method is currently only used by the bookmarks feature. I’m not entirely sure the fix is necessary, as tapping the “Change title” action in the notification appears to dismiss it already. That said, I’m not certain whether this behavior is consistent across older Android versions.

This issue was identified during our AI bug hunt, and the fix is straightforward. It also makes sense to address it now in case this method is used elsewhere in the app in the future.

Testing Instructions

1. Trigger a notification that is posted with a tag (for example, a playback error notification).
2. Perform the action that invokes removeNotification (e.g. tap the action that dismisses/handles the notification).
3. Confirm the notification is removed from the notification shade.
4. Repeat with a notification posted without a tag and confirm it is still dismissed correctly (no regression).

Screenshots or Screencast

Not applicable, no UI changes.

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
• Any jetpack compose components I added or changed are covered by compose previews
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

[claude]

Claude finished @geekygecko's task in 1m 1s —— View job

---

PR Review: NOTIF-02 — Cancel notifications by tag and id

• Read the changed file and surrounding code
• Compare against existing cancel logic (NotificationBroadcastReceiver)
• Check callers of removeNotification
• Assess correctness, bugs, security, performance

---

Summary

The change is correct and low-risk. Android keys active notifications by the (tag, id) pair, so cancel(id) alone silently fails to dismiss a notification that was posted with notify(tag, id, …). Reading the tag from the intent extras and cancelling with cancel(tag, id) when present — with a fallback to cancel(id) — is the right fix, and it correctly mirrors the existing logic in NotificationBroadcastReceiver.onReceive() (NotificationBroadcastReceiver.kt:80-83). 👍

Correctness

• ✅ The tag/id branching matches how tagged notifications are posted (e.g. PlaybackManager.kt:2215, RefreshPodcastsThread.kt:575 use notify(tag, id, …)).
• ✅ getString(key, null) + isNullOrBlank() guards both the missing-extra and empty-string cases, so the fallback is safe.
• ✅ Behaviourally a no-op for the previous code path when no tag is present, so no regression risk.

Observations (non-blocking)

1. The only current caller posts without a tag. removeNotification is called from two spots in MainActivity.kt:1601 and 1611, both for Settings.NotificationId.BOOKMARK. The bookmark notification is posted without a tag (BookmarkHelper.kt:128 → .notify(Settings.NotificationId.BOOKMARK.value, notification)). So for today's callers the new tag branch won't actually fire — the fix is primarily defensive / future-proofing and consistency-oriented rather than fixing an observable bookmark bug. Worth confirming the PR's "playback error notification lingering" scenario actually routes through removeNotification (those errors appear to be dismissed via NotificationBroadcastReceiver, not this helper). Not a problem with the code — just make sure the linked ticket's repro is covered.

2. Duplicated cancel logic. This block is now a near-exact copy of NotificationBroadcastReceiver.kt:80-83. Consider extracting a small shared helper (or having the receiver delegate to NotificationHelper) so the two paths can't drift. Optional cleanup.

3. Pre-existing: redundant NotificationManager lookup. The class already holds notificationManager (line 24), but removeNotification re-fetches the service via getSystemService(...) (line 230). Not introduced by this PR, but this method could reuse the field for consistency. (Note the field is nullable as? while the local is a hard as cast — minor inconsistency.)

Tests

removeNotification is easy to unit-test with a mocked NotificationManager — one case asserting cancel(tag, id) when a tag extra is set, one asserting cancel(id) when it's absent/blank. Given the checklist item on tests is unchecked, adding these two would lock in the intended behaviour cheaply. Add tests →

Verdict

Looks good to merge. The two-line change is correct, safe, and consistent with existing code. The notes above are optional follow-ups, not blockers.

[claude]

Claude finished @geekygecko's task in 40s —— View job

---

PR Review: NOTIF-02 — Cancel notifications by tag and id

• Read the changed file and surrounding code
• Compare against existing cancel logic (NotificationBroadcastReceiver)
• Check callers of removeNotification
• Assess correctness, bugs, security, performance

---

Summary

The change is correct and low-risk. Android keys active notifications by the (tag, id) pair, so NotificationManager.cancel(id) silently fails to dismiss a notification posted with notify(tag, id, …). Reading the tag from the intent extras and cancelling with cancel(tag, id) when present — falling back to cancel(id) — is the right fix and mirrors the existing logic in NotificationBroadcastReceiver.onReceive() (NotificationBroadcastReceiver.kt:80-83). ✅

Correctness

• ✅ getString(key, null) + isNullOrBlank() guards both the missing-extra and empty-string cases, so the fallback is safe.
• ✅ Behaviourally a no-op for the previous code path when no tag is present — no regression risk.
• ✅ The tag constant (INTENT_EXTRA_NOTIFICATION_TAG) is imported from the same source used by both post and cancel paths, so tag values stay consistent.

Observations (non-blocking)

1. Both current callers post without a tag. The two callers in MainActivity.kt:1601 / 1611 pass Settings.NotificationId.BOOKMARK, and the bookmark notification is posted without a tag. So for today's callers the new tag branch won't fire — this is primarily defensive / consistency hardening rather than a fix for an observable bookmark bug. Worth confirming the ticket's "playback error notification lingering" repro actually routes through removeNotification (playback-error dismissal appears to go through NotificationBroadcastReceiver, not this helper).

2. Duplicated cancel logic. This block is now a near-copy of NotificationBroadcastReceiver.kt:80-83. Consider extracting a shared helper (or having the receiver delegate to NotificationHelper.removeNotification) so the two paths can't drift. Optional. Refactor this →

3. Pre-existing: redundant NotificationManager lookup. The class already holds notificationManager (line 24, nullable as?), but removeNotification re-fetches the service with a hard as cast (line 230). Not introduced by this PR, but reusing the field would be more consistent and avoids a potential ClassCastException/null divergence between the two.

Tests

removeNotification is easy to unit-test with a mocked NotificationManager: one case asserting cancel(tag, id) when the tag extra is set, one asserting cancel(id) when it's absent/blank. The tests checklist item is unchecked — adding these two would lock in the intended behaviour cheaply. Add tests →

Verdict

Looks good to merge. The two-line change is correct, safe, and consistent with existing code. The notes above are optional follow-ups, not blockers.

[geekygecko]

I tried the self approve skill on this PR.

┌─────────────┬─────────┬─────────────────────────────────────────────────────────────────────┐
│    Gate     │ Verdict │                               Reason                                │
├─────────────┼─────────┼─────────────────────────────────────────────────────────────────────┤
│ 1.          │         │                                                                     │
│ Ownership & │ ✅ PASS │ Author geekygecko = you, a member of the pocket-casts-android team. │
│  platform   │         │                                                                     │
├─────────────┼─────────┼─────────────────────────────────────────────────────────────────────┤
│ 2. CI       │ ✅ PASS │ All checks pass; dangermattic skipped (acceptable); overall         │
│ checks      │         │ Buildkite "passed and blocked" (held deploy, not a failure).        │
├─────────────┼─────────┼─────────────────────────────────────────────────────────────────────┤
│ 3. Feedback │ ✅ PASS │ No reviews and no review threads outstanding.                       │
│  addressed  │         │                                                                     │
├─────────────┼─────────┼─────────────────────────────────────────────────────────────────────┤
│ 4. No       │         │ Touches only                                                        │
│ critical    │ ✅ PASS │ .../repositories/notification/NotificationHelperImpl.kt;            │
│ areas       │         │ notification is not a gated area.                                   │
├─────────────┼─────────┼─────────────────────────────────────────────────────────────────────┤
│ 5. Low risk │ ✅ PASS │ 2-line addition, 1 file, base main; self-contained bug fix (cancel  │
│             │         │ by tag+id when a tag exists, else by id alone).                     │
└─────────────┴─────────┴─────────────────────────────────────────────────────────────────────┘