fix: [SDK-5149] keep prevented notifications displayable after delay - #47
Merged
Conversation
sherwinski
reviewed
Sep 1, 2026
sherwinski
left a comment
There was a problem hiding this comment.
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 onlyexamples/demo. That line is now the one difference between the two hook files.ios/Sources/OneSignalCapacitorPlugin/OneSignalCapacitorPlugin.swift:374—event.preventDefault()is not needed.onWillDisplay(line 632) already called it. The Android call at line 510 is different, because it sendsdiscard.examples/run-ios.sh:76andexamples/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:75andexamples/dev-ios.sh:109— the loop reads${1:-}again on each pass. A flag such asnon_interactive=1, set once before the loop, is easier to read.examples/run-android.sh:41uses1as 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
- Is the retained event in
proceedWithWillDisplaya deliberate trade — keep the event alive so a latedisplay()works, and accept the growth? If yes, please say so in thehandleOnDestroycomment instead of deleting it. - Do we want the iOS discard timing to match Android? Full parity needs a change in the iOS SDK, not in this wrapper.
vp run ios 2does send2to the script — I checkedvp run --help. Thepreiospre-task isvp run setup. Doesvpsend2to the pre-task too?setup.shignores extra arguments, so this is cosmetic.- There are no native test targets (
android/src/testandios/Testsdo not exist). The new cache state machine has no test. Is a test target in scope here, or tracked in another ticket?
fadi-george
force-pushed
the
fadi/sdk-5149
branch
2 times, most recently
from
September 1, 2026 22:17
8eaa0c0 to
3bfedae
Compare
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
force-pushed
the
fadi/sdk-5149
branch
from
September 2, 2026 00:22
3bfedae to
9f1bae5
Compare
sherwinski
approved these changes
Sep 2, 2026
Merged
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
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.
Description
One Line Summary
Keep
preventDefault()events in cache soe.getNotification().display()still works after a delay (for example a 20ssetTimeout).Details
Motivation
foregroundWillDisplayis supposed to supportpreventDefault()plus a laterevent.getNotification().display()after async work. The Capacitor bridge was removing the cached native event inproceedWithWillDisplayas soon as JS listeners finished, so a delayeddisplay()(for example aftersetTimeout) silently did nothing on Android and iOS.Fixes SDK-5149.
Scope
proceedWithWillDisplaynow leave prevented events in cache untildisplay()orpreventDefault(true)discard.preventDefaultnow honors the existingdiscardargument.vpruns 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/displayNotificationunit 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 lintremains blocked by a pre-existingmocks/capacitor.tsimport warning in an untouched file.Manual testing
Tested on Android and iOS with:
handleForegroundWillDisplaycontaining only a log.preventDefault()uncommented.preventDefault()followed bydisplay().preventDefault()followed bydisplay()after approximately 25 seconds.Affected code checklist
Checklist
Overview
Testing
Final pass