fix: [SDK-5139] display foreground notifications by default - #1989
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Should-fix (not inline-able)
MIGRATION_GUIDE.md:330-341 still documents the old model. Only examples/build.md was updated. The migration guide is the file users read when they hit this behavior, and its example implies preventDefault() is the normal path. Add a line that says foreground notifications now display automatically, and that preventDefault() is only needed to hold display.
This is a behavior change for any app that registered a foregroundWillDisplay handler as a silent-suppress mechanism. Before this branch the notification never showed. Now it does. That deserves an explicit note in the guide and in the release notes, not only a comment in the demo.
| this.dispatchNotificationWillDisplayHandlers(event); | ||
| } finally { | ||
| if (!isDefaultPrevented(event) && !isDisplayRequested(event)) { | ||
| event.getNotification().display(); |
There was a problem hiding this comment.
Should-fix: this path bypasses the injected native module.
EventManager gets RNOneSignal: Spec in the constructor, but OSNotification.display() uses the module-level NativeOneSignal singleton. So the class talks to two different native handles.
The test file shows the effect: EventManager.test.ts must now import the global mockRNOneSignal to assert a call made by the object under test, while every other assertion in that file uses the injected mockModule.
Both handles resolve to the same module in production, so there is no runtime bug. But the injection is now misleading. Two options:
- Pass the
SpecintoNotificationWillDisplayEventand let it use the injected handle. - Call
this.RNOneSignal.displayNotification(...)here, and let the event track display state only.
There was a problem hiding this comment.
Both handles are the same singleton in production because EventManager is constructed with RNOneSignal. OSNotification.display() and NotificationWillDisplayEvent.preventDefault() already use that singleton, while the injected handle is used for event subscriptions. Calling the injected module only for automatic display would split display behavior and bypass the notification wrapper’s idempotency tracking, and passing Spec into the exported event would change its public constructor. I’m keeping the existing notification action pattern.
| const event = new NotificationWillDisplayEvent(payload as OSNotification); | ||
| try { | ||
| this.dispatchNotificationWillDisplayHandlers(event); | ||
| } finally { |
There was a problem hiding this comment.
Should-fix: a throw from this finally block discards the handler error.
If display() fails (for example, the native module is not loaded), the new exception replaces the pending exception from dispatchNotificationWillDisplayHandlers, and the original handler error is lost.
A local try/catch keeps the handler error as the visible one:
} finally {
if (!isDefaultPrevented(event) && !isDisplayRequested(event)) {
try {
event.getNotification().display();
} catch (error) {
console.error('OneSignal: could not display foreground notification', error);
}
}
}There was a problem hiding this comment.
This is technically possible, but it requires both a handler and the native display bridge to throw. A missing module fails earlier at getEnforcing(), and both native display implementations log and return on cache misses rather than throwing. The suggested catch would also swallow a real display failure when no handler error exists, so I’m keeping the current propagation behavior.
| } | ||
| } | ||
|
|
||
| private dispatchNotificationWillDisplayHandlers(event: NotificationWillDisplayEvent) { |
There was a problem hiding this comment.
Should-fix: this method duplicates dispatchHandlers for one event.
It is dispatchHandlers plus two changes, and both changes apply to every event, not only this one:
- The array copy on the next line fixes the case where a handler removes itself during dispatch.
dispatchHandlers(line 154) still iterates the live array, so it skips the next handler in that case. - Error isolation.
NOTIFICATION_WILL_DISPLAYnow runs all handlers when one throws. Every other event still stops at the first throw.
Suggest one of two things: fold the behavior into dispatchHandlers with an options argument, or keep the split and add a comment that says why this event is different. As written, a reader must diff the two methods to find the difference.
There was a problem hiding this comment.
Added a comment explaining that every foreground handler must run because any one can prevent automatic display. I kept this path separate to avoid changing error behavior for unrelated events.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the valid review points in 5d6ba24: updated the migration guide, clarified that late preventDefault calls cannot stop display, restored the notification ID assertion, and documented why foreground dispatch is specialized. |
2 participants
Description
One Line Summary
Display foreground notifications automatically unless a listener calls
preventDefault().Details
Motivation
The native bridges prevent foreground display before dispatching to JavaScript to avoid an asynchronous bridge race. Previously, that meant adding a log-only listener suppressed the notification unless the app explicitly called
display().Scope
preventDefault()call suppressing automatic display.Testing
Unit testing
vp checkvp test: 272 tests passedvp run buildManual 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
Made with Cursor