[RaphaelFakhri]

Description

1 Line Summary

Show a push notification after 2 seconds if no visible page answers the NotificationWillDisplay broadcast.

Details

shouldPreventNotificationDisplay waits for a visible window client to answer the NotificationWillDisplay broadcast before it shows a push notification, with no timeout. clients.matchAll({ includeUncontrolled: true }) also returns visible same-origin pages that do not run the SDK. Those pages never answer, so the notification is never shown and the push event stays pending.

This change resolves the pending check with false (show the notification) after NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS (2 seconds). A page that runs the SDK still answers right away, because preventDefault() is synchronous, so preventDefault() and display() behave as before. It also stops a notification from being lost when a second push event overwrites self.notificationDisplayStatus before the first response arrives.

The fallback uses setTimeout directly instead of delay(), which the service worker tests mock to resolve immediately.

Systems Affected

• WebSDK
• Backend
• Dashboard

Validation

Tests

Info

• New test in src/sw/serviceWorker/ServiceWorker.test.ts: a visible client exists but never responds. The test fails without the fix (showNotification is never called) and passes with it.
• vp test run: 62 files, 559 tests pass.
• vp check: no errors and no new warnings.
• vp run build:prod: OneSignalSDK.sw.js is 12.49 kB gzipped, within the 12.5 kB limit.

Checklist

• All the automated tests pass or I explained why that is not possible
• I have personally tested this on my machine or explained why that is not possible
• I have included test coverage for these changes or explained why they are not needed

Manual browser testing was not done. The new unit test covers the scenario.

Programming Checklist
Interfaces:

• Don't use default export
• New interfaces are in model files

Functions:

• Don't use default export
• All function signatures have return types
• Helpers should not access any data but rather be given the data to operate on.

Typescript:

• No Typescript warnings
• Avoid silencing null/undefined warnings with the exclamation point

Other:

• Iteration: refrain from using elem of array syntax. Prefer forEach or use map
• Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Not needed. The change affects service worker logic only and has no UI.

Checklist

• I have included screenshots/recordings of the intended results or explained why they are not needed

---

Related Tickets

Fixes #1536

---

[abdulraqeeb33]

The 2 second fallback shows the notification when no visible page answers. A page that runs the SDK still answers in the same turn, and that answer settles the promise first.

A main thread that does not reach the message within 2 seconds still gets the notification, and a later display() alerts again because renotify is true on the same tag. Notifications in one push wait one after another, so several unanswered checks can add up toward the roughly 10 second push-handler budget.

[abdulraqeeb33]

This advances 5 seconds and only checks that showNotification ran. Any delay up to 5 seconds would pass. Assert it has not been called at 1999 ms, then that it has been called at 2000 ms.

[RaphaelFakhri]

Done in 2eb90be. The test now waits for the will-display broadcast and advances NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS - 1, then asserts that showNotification hasn't run. It then advances 1 ms. Showing the notification runs a few more short timers after the fallback fires, so the test advances another 10 ms before it asserts the call.

[abdulraqeeb33]

This timer settles the promise and leaves notificationDisplayStatus set. Two overlapping push events for the same notification id share that slot, so a late answer for the first can settle the second. Clear the slot when the timer wins, and cancel the timer when a response arrives.

[RaphaelFakhri]

Fixed in 2eb90be with the per-notification map from the thread below. A response clears its own timer, and the timeout removes its own entry.

[fadi-george]

Ill check with Sherwin too

[fadi-george]

should export this and use in the test instead of the 5_000 value

[RaphaelFakhri]

Exported in 2eb90be. The test uses it instead of 5_000.

[fadi-george]

Overall the timeout fixes the hang. One issue with overlapping pushes below.

[fadi-george]

If a second push arrives before the first one gets its response, this still drops the first page's preventDefault. Push B overwrites notificationDisplayStatus, A's response fails the id check at L208, and then this timer resolves A with false, so A shows anyway.

Repro: dispatch push A, then push B, then send responses A: preventDefault=true and B: false. Both A and B get shown. On main, A just hung.

Could we key the pending waits by notificationId so each response settles its own?

const pendingDisplayChecks = new Map<
  string,
  { resolve: (preventDefault: boolean) => void; timer: ReturnType<typeof setTimeout> }
>();

function settleDisplayCheck(notificationId: string, preventDefault: boolean): void {
  const pending = pendingDisplayChecks.get(notificationId);
  if (!pending) return;
  clearTimeout(pending.timer);
  pendingDisplayChecks.delete(notificationId);
  pending.resolve(preventDefault);
}

// in shouldPreventNotificationDisplay
const preventDefaultPromise = new Promise<boolean>((resolve) => {
  const timer = setTimeout(
    () => settleDisplayCheck(notif.notificationId, false),
    NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS,
  );
  pendingDisplayChecks.set(notif.notificationId, { resolve, timer });
});

This also clears the timer when a response comes in and removes the entry when the timeout fires, which neither happens today.

The response handler at L208 then becomes:

workerMessenger._on(
  WorkerMessengerCommand._NotificationWillDisplayResponse,
  (payload: NotificationWillDisplayResponsePayload) => {
    Log._debug('[SW] NotifWillDisplay response', payload);
    settleDisplayCheck(payload.notificationId, payload.preventDefault);
  },
);

Then notificationDisplayStatus can be removed from OSServiceWorkerFields in types.ts. The existing preventDefault tests that call self.notificationDisplayStatus!.resolve(...) would need to send a _NotificationWillDisplayResponse message instead.

[RaphaelFakhri]

Applied in 2eb90be. Pending checks are keyed by notification id in pendingDisplayChecks, settled through settleDisplayCheck, and notificationDisplayStatus is removed from OSServiceWorkerFields. The existing preventDefault tests now answer with a _NotificationWillDisplayResponse message instead of calling resolve directly.

[fadi-george]

Could we add a test for overlapping pushes? This one fails on the current branch (shows ['B', 'A']) and passes with the map change:

test('honors preventDefault when pushes overlap', async () => {
  const showNotificationSpy = vi.spyOn(self.registration, 'showNotification');
  const payloadA = mockOSMinifiedNotificationPayload({ title: 'A', custom: { i: 'id-a' } });
  const payloadB = mockOSMinifiedNotificationPayload({ title: 'B', custom: { i: 'id-b' } });

  self.dispatchEvent(new PushEvent('push', payloadA));
  await flush();
  self.dispatchEvent(new PushEvent('push', payloadB));
  await flush();

  isServiceWorker = true;
  await dispatchEvent(
    new ExtendableMessageEvent('message', {
      command: WorkerMessengerCommand._NotificationWillDisplayResponse,
      payload: { notificationId: 'id-a', preventDefault: true },
    }),
  );
  await dispatchEvent(
    new ExtendableMessageEvent('message', {
      command: WorkerMessengerCommand._NotificationWillDisplayResponse,
      payload: { notificationId: 'id-b', preventDefault: false },
    }),
  );

  // let A's timeout fire
  await new Promise((r) => setTimeout(r, 2_500));
  await flush();

  const titles = showNotificationSpy.mock.calls.map(([title]) => title);
  expect(titles).toEqual(['B']);
}, 10_000);

Also nit: could we export NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS and advance by that instead of 5_000 here? That way the test keeps up if the timeout changes.

[RaphaelFakhri]

Added in 2eb90be. It fails on the previous commit with ['Test Notification', 'B'] and passes with the map. It waits NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS + 500 instead of a fixed 2_500.

[sherwinski]

vi.useRealTimers() only runs if advanceTimersByTimeAsync resolves. If it throws, the rest of this file runs with fake timers, and the tick/flush helpers below stop working. Later tests will then fail for reasons not related to their actual assertions.

We should wrap the fake-timer section in try/finally:

vi.useFakeTimers();
try {
  self.dispatchEvent(new PushEvent('push', payload));
  await vi.advanceTimersByTimeAsync(NOTIFICATION_WILL_DISPLAY_TIMEOUT_MS);
} finally {
  vi.useRealTimers();
}
await flush();

[RaphaelFakhri]

Wrapped in try/finally in 2eb90be.

[RaphaelFakhri]

All review comments are addressed in 2eb90be. ServiceWorker.test.ts passes (29 tests) and vp check is clean. One unrelated test, message event > session upsert event > with non-safari client, fails intermittently. It does the same on the base commit (3 of 6 local runs).

[sherwinski]

I pushed one change related to test waits and bumped the package size limits. Otherwise, this LGTM, thanks for the contribution @RaphaelFakhri!