Skip to content

feat: [SDK-5011] detect restored notifications and stop them re-alerting - #2723

Open
nan-li wants to merge 3 commits into
nan/sdk-5083from
nan/sdk-5011
Open

feat: [SDK-5011] detect restored notifications and stop them re-alerting#2723
nan-li wants to merge 3 commits into
nan/sdk-5083from
nan/sdk-5011

Conversation

@nan-li

@nan-li nan-li commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

One Line Summary

(API Addition)
Let apps detect restored notifications, and stop restored ones from re-alerting or reappearing after they are suppressed.

Details

Motivation

When Android clears the shade (reboot, app update, force-stop) the SDK re-posts saved notifications and re-fires onNotificationReceived. Three problems fell out of that:

  1. Apps could not tell a restore from a new push, so once-only work (analytics, network calls, local writes) ran again on every restore. v3 exposed a restoring flag; it was dropped in the v4 refactor.
  2. preventDefault() during a restore did not stick. markNotificationAsDismissed bailed out unless isNotificationToDisplay was set, which never happens when display is suppressed, so the row was never marked dismissed and the notification returned on every later restore.
  3. An extender calling setChannelId moved a restored notification off the silent restored_OS_notifications channel. The channel governs alerting on Android O+, so removeNotifyOptions() could not keep it quiet and it arrived as a heads-up banner.

Scope

  • INotificationReceivedEvent gains a read-only restoring: Boolean, defaulted so existing implementations still compile. True for both a shade restore and a group collapsing to one member.
  • The restore channel is re-asserted after the extender runs, including on group collapse, so neither path re-alerts. Extenders still control the channel for a first-time notification.
  • Shade restore + preventDefault(true) marks the row dismissed, leaves the shade alone, and updates the dependent group summary the same way a real dismissal would. A suppressed last child consumes its orphaned summary row, and a suppressed sibling refreshes the summary count. Group collapse + preventDefault is a no-op; the remaining child stays, including its summary.
  • Drops NotificationGenerationJob.orgFlags, written after every extender and read nowhere. Its sibling orgSound is kept, still needed by the pre-Android 7.0 grouped-sound workaround.

Testing

Unit testing

NotificationDisplayerTests covers the restore channel, an extender trying to override it, an extender keeping its channel on a normal notification, and the payload-vs-extender sound split that orgSound exists for.
NotificationGenerationProcessorTests covers restoring for both reasons, shade preventDefault(true), no-arg shade preventDefault(), and regroup not dismissing. It also covers the summary follow-up. A suppressed shade restore reconciles the dependent summary. A repeat suppression on an already-dismissed row does not. A sibling that displayed earlier in the same pass stays put, and regroup still touches nothing.
NotificationRepositoryTests covers markAsDismissedWithoutCancel leaving the shade.
NotificationGenerationWorkManagerTests covers readRestoreReason for the enum, the legacy boolean, and an unknown name.

JaCoCo does not attribute Robolectric-executed lines, so this PR uses Skip Coverage Check. The processor and work-manager cases are plain JVM and do count.

Manual testing

Confirmed behavior before and after with force kill

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing — every change is in the restored-notification path
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible — see Manual testing

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

@nan-li
nan-li requested a review from a team as a code owner August 21, 2026 16:57
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • INotificationReceivedEvent.kt: 0/1 touched executable lines (0.0%) (16 touched lines in diff)
    • 1 uncovered touched lines in this file
  • NotificationReceivedEvent.kt: 1/1 touched executable lines (100.0%) (1 touched lines in diff)
  • NotificationBundleProcessor.kt: 0/3 touched executable lines (0.0%) (4 touched lines in diff)
    • 3 uncovered touched lines in this file
  • NotificationRestoreReason.kt: 2/2 touched executable lines (100.0%) (13 touched lines in diff)
  • NotificationRepository.kt: 0/8 touched executable lines (0.0%) (21 touched lines in diff)
    • 8 uncovered touched lines in this file
  • NotificationDisplayBuilder.kt: 0/2 touched executable lines (0.0%) (4 touched lines in diff)
    • 2 uncovered touched lines in this file
  • NotificationDisplayer.kt: 0/3 touched executable lines (0.0%) (5 touched lines in diff)
    • 3 uncovered touched lines in this file
  • SummaryNotificationDisplayer.kt: 0/3 touched executable lines (0.0%) (4 touched lines in diff)
    • 3 uncovered touched lines in this file
  • NotificationGenerationProcessor.kt: 13/13 touched executable lines (100.0%) (30 touched lines in diff)
  • NotificationGenerationWorkManager.kt: 0/2 touched executable lines (0.0%) (20 touched lines in diff)
    • 2 uncovered touched lines in this file
  • NotificationRestoreProcessor.kt: 0/5 touched executable lines (0.0%) (8 touched lines in diff)
    • 5 uncovered touched lines in this file
  • NotificationSummaryManager.kt: 0/3 touched executable lines (0.0%) (5 touched lines in diff)
    • 3 uncovered touched lines in this file

Overall (aggregate gate)

16/46 touched executable lines covered (34.8% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • INotificationReceivedEvent.kt: 0.0% (1 uncovered touched lines)

  • NotificationBundleProcessor.kt: 0.0% (3 uncovered touched lines)

  • NotificationRepository.kt: 0.0% (8 uncovered touched lines)

  • NotificationDisplayBuilder.kt: 0.0% (2 uncovered touched lines)

  • NotificationDisplayer.kt: 0.0% (3 uncovered touched lines)

  • SummaryNotificationDisplayer.kt: 0.0% (3 uncovered touched lines)

  • NotificationGenerationWorkManager.kt: 0.0% (2 uncovered touched lines)

  • NotificationRestoreProcessor.kt: 0.0% (5 uncovered touched lines)

  • NotificationSummaryManager.kt: 0.0% (3 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 34.8% (minimum 80%).

📥 View workflow run

@nan-li
nan-li marked this pull request as draft August 21, 2026 17:58
@nan-li nan-li added the Skip Coverage Check To skip coverage check in the run label Aug 21, 2026
@nan-li
nan-li marked this pull request as ready for review August 21, 2026 21:56
@cursor

cursor Bot commented Aug 24, 2026

Copy link
Copy Markdown

SDK review — request changes

Restore-reason split, sticky shade dismiss, and re-asserting the restore channel after the extender all look right.

Grouped restore + mixed suppress. SHADE_RESTORE + preventDefault(true) never calls updatePossibleDependentSummaryOnDismiss. Skipping the 1-remaining restoreSummary re-enqueue is right (that sibling is already in the restore pass). Skipping 0 remaining (orphan summary) and 2+ (stale inbox/count) is not. If child B displays first, the summary can already include outstanding A; suppressing A later leaves that inbox stale. The new exactly = 0 verify locks the broad skip in.

restoring vs preventDefault(true). The flag is true for shade restore and group collapse, but discard only sticks for shade restore. if (event.restoring) preventDefault(true) will skip the collapse rebuild. Expose the reason (or a shade-only flag), or make blanket suppress safe on regroup. Minor: KDoc says extenders keep customizations; channel/sound are still overridden on restore.

Nits: log the suppress-branch outcome; markAsDismissedWithoutCancel still refreshes the badge while leaving the shade (API 21–22 / failed getActiveNotifications).

@nan-li nan-li changed the title fix: [SDK-5011] detect and correctly suppress restored notifications feat: [SDK-5011] detect restored notifications and stop them re-alerting Aug 25, 2026
@nan-li

nan-li commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Both suppress branches log their outcome now.

The badge call is deliberate, and it now carries a comment saying why. One correction on the window, though. update() returns early on API 26+, and on 23 to 25 it counts the shade itself through getActiveNotifications. Only API 21 and 22 take the count from undismissed rows, so that is the only place the badge and the shade can disagree here.

@nan-li
nan-li changed the base branch from main to nan/sdk-5083 September 1, 2026 04:43
nan-li added 3 commits August 31, 2026 21:48
Apps could not tell a restore from a new push, so once-only work such as
analytics and network calls ran again every time Android cleared the shade
and the SDK re-posted its saved notifications. v3 exposed a restoring flag
and the v4 refactor dropped it.

The property is read-only and defaulted, so existing implementations still
compile. It reports true for a shade restore and for a group collapsing to
one member, since both mean the app has already seen this notification.
…nnel

An extender calling setChannelId moved a restored notification off
restored_OS_notifications and onto the app's own channel. The channel
governs alerting on Android O+, so a high-importance one turned every
restore into a heads-up banner. The displayer now records the channel
OneSignal picked and re-asserts it after the extender runs.

That also explains the dead code this removes. removeNotifyOptions strips
flags and sound to keep restores quiet, which predates notification channels
and cannot win against one on O+. NotificationGenerationJob loses orgFlags,
written after every extender and read nowhere. Its sibling orgSound stays,
still needed by the pre-Android 7.0 grouped-sound workaround in
SummaryNotificationDisplayer.
preventDefault() during a restore did nothing durable.
markNotificationAsDismissed bailed out unless isNotificationToDisplay was
set, which never happens when display is suppressed, so the row kept
dismissed = 0 and the restore query picked it up again on every later cold
start.

A suppressed shade restore now marks the row dismissed without clearing the
shade, since the restore pass can include notifications still showing on API
21 and 22, and whenever getActiveNotifications fails. It then updates the
dependent group summary the same way a real dismissal does, so a suppressed
child cannot orphan its summary or leave it counting a notification that is
gone.

Telling the two restore triggers apart is what makes that safe, so the
internal isRestoring boolean becomes a NotificationRestoreReason. A shade
restore is gone from the shade and should stay gone. A group collapsing to
one member is still on screen and the user never dismissed it, so suppressing
that rebuild leaves it alone. The work manager still reads the legacy
is_restoring boolean from work enqueued by an older version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Coverage Check To skip coverage check in the run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant