Skip to content

fix: [SDK-5139] display foreground notifications by default #1989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fadi-george merged 5 commits into from
Sep 1, 2026
Merged

fix: [SDK-5139] display foreground notifications by default #1989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79c5bee
feat(notifications): auto-display unless preventDefault called
fadi-george Sep 1, 2026
17fbde3
docs(demo): add preventDefault/display comments
fadi-george Sep 1, 2026
8691d6c
fix(notifications): handle foreground display edge cases
fadi-george Sep 1, 2026
fbb3b0b
refactor(notifications): keep display state internal
fadi-george Sep 1, 2026
5d6ba24
docs(notifications): clarify foreground display behavior
fadi-george Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions MIGRATION_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,10 @@ OneSignal.Notifications.removeEventListener('permissionChange', permissionObserv

### Notification Lifecycle Listener

Foreground notifications display automatically unless `preventDefault()` is called synchronously
inside the listener. To delay display for asynchronous work, call `preventDefault()` first, then call
`display()` within about 25 seconds.

```typescript
OneSignal.Notifications.addEventListener(
'foregroundWillDisplay',
Expand Down
2 changes: 1 addition & 1 deletion examples/build.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ OneSignal.User.addEventListener('change', handler);

### Foreground notification handler

- `foregroundWillDisplay` calls `e.getNotification().display()` inside `useOneSignal.ts` so foreground pushes are still shown by the OS UI.
- `foregroundWillDisplay` logs the notification; foreground pushes display automatically unless the handler calls `preventDefault()`.

---

Expand Down
6 changes: 5 additions & 1 deletion examples/demo/src/hooks/useOneSignal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,11 @@ function useOneSignalState(): UseOneSignalReturn {

const handleForegroundWillDisplay = (e: NotificationWillDisplayEvent) => {
console.log(`Notification foregroundWillDisplay: ${e.getNotification().title ?? ''}`);
e.getNotification().display();
// uncomment to test preventing the default display behavior
// e.preventDefault();

// can call this after preventDefault (within ~25 seconds) to force display of notification
// e.getNotification().display();
};

const pushSubHandler = (event: PushSubscriptionChangedState) => {
Expand Down
101 changes: 99 additions & 2 deletions src/events/EventManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { beforeEach, describe, expect, test, vi } from 'vite-plus/test';

import { mockRNOneSignal } from '../../__mocks__/react-native';
import {
IN_APP_MESSAGE_CLICKED,
IN_APP_MESSAGE_DID_DISMISS,
Expand Down Expand Up @@ -50,6 +51,7 @@ describe('EventManager', () => {
let callbacks: Map<string, (payload: unknown) => void>;

beforeEach(() => {
vi.clearAllMocks();
const mock = createMockNativeModule();
mockModule = mock.module;
callbacks = mock.callbacks;
Expand Down Expand Up @@ -195,6 +197,97 @@ describe('EventManager', () => {
expect(receivedEvent).toBeInstanceOf(NotificationWillDisplayEvent);
});

test('should display a foreground notification when no handlers are registered', () => {
callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);

expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledWith('test-id');
});

test('should display a foreground notification when handlers do not prevent it', () => {
const handler1 = vi.fn();
const handler2 = vi.fn();
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, handler1);
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, handler2);

callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);

expect(handler1).toHaveBeenCalledOnce();
expect(handler2).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
});

test('should not display automatically when any handler prevents default', () => {
const observingHandler = vi.fn();
const preventingHandler = vi.fn((event: NotificationWillDisplayEvent) => {
event.preventDefault();
});
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, observingHandler);
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, preventingHandler);

callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);

expect(observingHandler).toHaveBeenCalledOnce();
expect(preventingHandler).toHaveBeenCalledOnce();
expect(mockRNOneSignal.preventDefault).toHaveBeenCalledWith('test-id');
expect(mockRNOneSignal.displayNotification).not.toHaveBeenCalled();
});

test('should allow deferred display after preventing default', () => {
let receivedEvent: NotificationWillDisplayEvent | undefined;
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, (event) => {
receivedEvent = event;
event.preventDefault();
});

callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);
expect(mockRNOneSignal.displayNotification).not.toHaveBeenCalled();

receivedEvent?.getNotification().display();

expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledWith('test-id');
});

test('should not display twice when a handler displays explicitly', () => {
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, (event) => {
event.getNotification().display();
});

callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);

expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledWith('test-id');
});

test('should display by default when a handler throws', () => {
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, () => {
throw new Error('listener failed');
});

expect(() => {
callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);
}).toThrow('listener failed');
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledWith('test-id');
});

test('should continue dispatching after a handler throws', () => {
const preventingHandler = vi.fn((event: NotificationWillDisplayEvent) => {
event.preventDefault();
});
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, () => {
throw new Error('listener failed');
});
eventManager.addEventListener(NOTIFICATION_WILL_DISPLAY, preventingHandler);

expect(() => {
callbacks.get('onNotificationWillDisplay')!(rawWillDisplayPayload);
}).toThrow('listener failed');
expect(preventingHandler).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).not.toHaveBeenCalled();
});

test('should handle PERMISSION_CHANGED events with boolean payload', () => {
const handler = vi.fn();
eventManager.addEventListener(PERMISSION_CHANGED, handler);
Expand Down Expand Up @@ -366,8 +459,12 @@ describe('EventManager', () => {

expect(permissionHandler).toHaveBeenCalledWith(true);
expect(subscriptionHandler).toHaveBeenCalledWith(pushChangedPayload);
expect(notificationWillDisplayHandler).toHaveBeenCalledWith(
new NotificationWillDisplayEvent(rawWillDisplayPayload),
expect(notificationWillDisplayHandler).toHaveBeenCalledOnce();
expect(notificationWillDisplayHandler.mock.calls[0][0]).toBeInstanceOf(
NotificationWillDisplayEvent,
);
expect(notificationWillDisplayHandler.mock.calls[0][0].getNotification().notificationId).toBe(
'test-id',
);
});

Expand Down
39 changes: 34 additions & 5 deletions src/events/EventManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ import type {
import type { NotificationClickEvent } from '../types/notificationEvents';
import type { PushSubscriptionChangedState } from '../types/subscription';
import type { UserChangedState } from '../types/user';
import NotificationWillDisplayEvent from './NotificationWillDisplayEvent';
import NotificationWillDisplayEvent, {
isDefaultPrevented,
isDisplayRequested,
} from './NotificationWillDisplayEvent';

export interface EventListenerMap {
[PERMISSION_CHANGED]: (event: boolean) => void;
Expand Down Expand Up @@ -72,10 +75,14 @@ export default class EventManager {
this.dispatchHandlers(USER_STATE_CHANGED, payload);
}),
this.RNOneSignal.onNotificationWillDisplay((payload) => {
this.dispatchHandlers(
NOTIFICATION_WILL_DISPLAY,
new NotificationWillDisplayEvent(payload as OSNotification),
);
const event = new NotificationWillDisplayEvent(payload as OSNotification);
try {
this.dispatchNotificationWillDisplayHandlers(event);
} finally {

@sherwinski sherwinski Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
    }
  }
}

@fadi-george fadi-george Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

if (!isDefaultPrevented(event) && !isDisplayRequested(event)) {
event.getNotification().display();

@sherwinski sherwinski Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Pass the Spec into NotificationWillDisplayEvent and let it use the injected handle.
  2. Call this.RNOneSignal.displayNotification(...) here, and let the event track display state only.

@fadi-george fadi-george Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

}
}
}),
this.RNOneSignal.onNotificationClicked((payload) => {
this.dispatchHandlers(NOTIFICATION_CLICKED, payload);
Expand Down Expand Up @@ -124,6 +131,28 @@ export default class EventManager {
}
}

private dispatchNotificationWillDisplayHandlers(event: NotificationWillDisplayEvent) {

@sherwinski sherwinski Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. Error isolation. NOTIFICATION_WILL_DISPLAY now 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.

@fadi-george fadi-george Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

// Every handler must run because any one of them can prevent automatic display.
const handlers = [...(this.eventListenerArrayMap.get(NOTIFICATION_WILL_DISPLAY) ?? [])];
let firstError: unknown;
let handlerThrew = false;

handlers.forEach((handler) => {
try {
handler(event);
} catch (error) {
if (!handlerThrew) {
firstError = error;
handlerThrew = true;
}
}
});

if (handlerThrew) {
throw firstError;
}
}

private dispatchHandlers(eventName: string, payload: unknown) {
const handlerArray = this.eventListenerArrayMap.get(eventName);
if (handlerArray) {
Expand Down
32 changes: 31 additions & 1 deletion src/events/NotificationWillDisplayEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import { describe, expect, test } from 'vite-plus/test';

import { mockRNOneSignal } from '../../__mocks__/react-native';
import OSNotification, { type BaseNotificationData } from '../OSNotification';
import NotificationWillDisplayEvent from './NotificationWillDisplayEvent';
import NotificationWillDisplayEvent, {
isDefaultPrevented,
isDisplayRequested,
} from './NotificationWillDisplayEvent';

describe('NotificationWillDisplayEvent', () => {
const notificationId = 'test-notification-id';
Expand Down Expand Up @@ -68,6 +71,7 @@ describe('NotificationWillDisplayEvent', () => {
const result = event.preventDefault();

expect(mockRNOneSignal.preventDefault).toHaveBeenCalledWith(notificationId);
expect(isDefaultPrevented(event)).toBe(true);
expect(result).toBeUndefined();
});

Expand All @@ -81,6 +85,32 @@ describe('NotificationWillDisplayEvent', () => {

expect(mockRNOneSignal.preventDefault).toHaveBeenCalledTimes(3);
expect(mockRNOneSignal.preventDefault).toHaveBeenCalledWith('test-notification-id');
expect(isDefaultPrevented(event)).toBe(true);
});
});

describe('isDefaultPrevented', () => {
test('should be false before preventDefault is called', () => {
const notification = new OSNotification(baseNotificationData);
const event = new NotificationWillDisplayEvent(notification);

expect(isDefaultPrevented(event)).toBe(false);
});
});

describe('isDisplayRequested', () => {
test('should track notification display calls', () => {
const notification = new OSNotification(baseNotificationData);
const event = new NotificationWillDisplayEvent(notification);

expect(isDisplayRequested(event)).toBe(false);

event.getNotification().display();
event.getNotification().display();

expect(isDisplayRequested(event)).toBe(true);
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledOnce();
expect(mockRNOneSignal.displayNotification).toHaveBeenCalledWith(notificationId);
});
});

Expand Down
28 changes: 27 additions & 1 deletion src/events/NotificationWillDisplayEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,44 @@ import NativeOneSignal from '../NativeOneSignal';
import OSNotification from '../OSNotification';
const RNOneSignal = NativeOneSignal;

const displayedNotifications = new WeakSet<OSNotification>();
const preventedEvents = new WeakSet<NotificationWillDisplayEvent>();

class ForegroundNotification extends OSNotification {
display(): void {
if (displayedNotifications.has(this)) {
return;
}
displayedNotifications.add(this);
super.display();
}
}

export default class NotificationWillDisplayEvent {
public notification: OSNotification;

constructor(displayEvent: OSNotification) {
this.notification = new OSNotification(displayEvent);
this.notification = new ForegroundNotification(displayEvent);
}

/**
* This must be called synchronously while the foreground listener is running.
* Calling it later cannot stop the automatic display.
*/
preventDefault(): void {
preventedEvents.add(this);
RNOneSignal.preventDefault(this.notification.notificationId);
}

getNotification(): OSNotification {
return this.notification;
}
}

export function isDefaultPrevented(event: NotificationWillDisplayEvent): boolean {
return preventedEvents.has(event);
}

export function isDisplayRequested(event: NotificationWillDisplayEvent): boolean {
return displayedNotifications.has(event.notification);
}
Loading