Skip to content

fix: [SDK-5149] keep prevented notifications displayable after delay - #47

Merged
fadi-george merged 2 commits into
mainfrom
fadi/sdk-5149
Sep 2, 2026
Merged

fix: [SDK-5149] keep prevented notifications displayable after delay#47
fadi-george merged 2 commits into
mainfrom
fadi/sdk-5149

Conversation

@fadi-george

@fadi-george fadi-george commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Keep preventDefault() events in cache so e.getNotification().display() still works after a delay (for example a 20s setTimeout).

Details

Motivation

foregroundWillDisplay is supposed to support preventDefault() plus a later event.getNotification().display() after async work. The Capacitor bridge was removing the cached native event in proceedWithWillDisplay as soon as JS listeners finished, so a delayed display() (for example after setTimeout) silently did nothing on Android and iOS.

Fixes SDK-5149.

Scope

  • Android and iOS proceedWithWillDisplay now leave prevented events in cache until display() or preventDefault(true) discard.
  • preventDefault now honors the existing discard argument.
  • Demo comment notes the native ~25s display window.
  • iOS example scripts accept a non-interactive simulator choice for vp runs without a TTY.

Does not change click handling, permission listeners, or when notifications auto-display if preventDefault() is not called.

Testing

Unit testing

Existing JS preventDefault / displayNotification unit tests cover the plugin call contract. The cache lifetime change is native-only. vp pack, all 179 JS tests, Android Kotlin compilation, and the iOS plugin build pass. vp lint remains blocked by a pre-existing mocks/capacitor.ts import warning in an untouched file.

Manual testing

Tested on Android and iOS with:

  • handleForegroundWillDisplay containing only a log.
  • preventDefault() uncommented.
  • preventDefault() followed by display().
  • preventDefault() followed by display() after approximately 25 seconds.

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
  • 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

Final pass

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

@fadi-george
fadi-george requested a review from a team September 1, 2026 19:57

@sherwinski sherwinski left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review summary

The discard work is correct in the common paths. It also fixes a real bug: before this branch, the documented preventDefault(false) then preventDefault(true) sequence failed, and a late display() did nothing, because proceedWithWillDisplay consumed the cache entry. The two iOS shell scripts are clean and match the Android scripts.

One problem blocks the merge. The change removes the only bound on cache size, and it deletes the comments that recorded that bound. See the inline notes on proceedWithWillDisplay in both native files.

Must fix: 1 problem (2 inline notes, one per platform).
Should fix: 3 problems (5 inline notes).

Nits

  • examples/demo_pods/src/hooks/useOneSignal.ts:219 — the docs commit changed only examples/demo. That line is now the one difference between the two hook files.
  • ios/Sources/OneSignalCapacitorPlugin/OneSignalCapacitorPlugin.swift:374event.preventDefault() is not needed. onWillDisplay (line 632) already called it. The Android call at line 510 is different, because it sends discard.
  • examples/run-ios.sh:76 and examples/dev-ios.sh:110 — the message "Invalid selection." gives no range. The interactive prompt shows [1-N]. Please show the range in the non-interactive message too.
  • examples/run-ios.sh:75 and examples/dev-ios.sh:109 — the loop reads ${1:-} again on each pass. A flag such as non_interactive=1, set once before the loop, is easier to read.
  • examples/run-android.sh:41 uses 1 as the default for empty input. The iOS scripts treat empty input as invalid and ask again. The Android code is older, but this PR is the parity pass, so please align the two.

Questions

  1. Is the retained event in proceedWithWillDisplay a deliberate trade — keep the event alive so a late display() works, and accept the growth? If yes, please say so in the handleOnDestroy comment instead of deleting it.
  2. Do we want the iOS discard timing to match Android? Full parity needs a change in the iOS SDK, not in this wrapper.
  3. vp run ios 2 does send 2 to the script — I checked vp run --help. The preios pre-task is vp run setup. Does vp send 2 to the pre-task too? setup.sh ignores extra arguments, so this is cosmetic.
  4. There are no native test targets (android/src/test and ios/Tests do not exist). The new cache state machine has no test. Is a test target in scope here, or tracked in another ticket?

Comment thread ios/Sources/OneSignalCapacitorPlugin/OneSignalCapacitorPlugin.swift
Comment thread ios/Sources/OneSignalCapacitorPlugin/OneSignalCapacitorPlugin.swift
Comment thread ios/Sources/OneSignalCapacitorPlugin/OneSignalCapacitorPlugin.swift
Comment thread examples/demo/src/hooks/useOneSignal.ts
@fadi-george
fadi-george force-pushed the fadi/sdk-5149 branch 2 times, most recently from 8eaa0c0 to 3bfedae Compare September 1, 2026 22:17
Keep prevented foreground notification events available until explicit display or discard so display() works after asynchronous processing. Honor the existing discard argument and document the cross-platform display window.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george
fadi-george merged commit 935bd63 into main Sep 2, 2026
6 checks passed
@fadi-george
fadi-george deleted the fadi/sdk-5149 branch September 2, 2026 01:15
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