Skip to content

fix: handle iOS exceptions when requesting notification permission #2071

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: handle iOS exceptions when requesting notification permission #2071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion src/plugins/push/getW3CDeviceDetails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,21 @@ export async function getW3CPushDeviceDetails(machine: ActivationStateMachine) {
const GotPushDeviceDetails = machine.GotPushDeviceDetails;
const { ErrorInfo, Defaults } = machine.client;

const permission = await Notification.requestPermission();
let permission: NotificationPermission = Notification.permission;

if (permission === 'default') {
try {
// throws exception in iOS
permission = await Notification.requestPermission();
} catch (err) {
machine.handleEvent(
new GettingPushDeviceDetailsFailed(
new ErrorInfo('Failed to request permission to send notifications', 400, 40000),
),
);
return;
}
}
Comment on lines +28 to +42
Copy link

@coderabbitai coderabbitai bot Aug 19, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

iOS exception handling looks good; fix ErrorInfo code/status order, include cause, and add a Notifications API guard

  • Good: Gating on Notification.permission and wrapping requestPermission in try/catch addresses the iOS “must be user-gesture” exception.
  • Issues:
    • ErrorInfo arguments are reversed (code/status). Elsewhere in this file you use (50000, 500). Here it’s (400, 40000). That will break consumers relying on Ably error code semantics.
    • The caught error isn’t included as the cause. Passing it improves debuggability.
    • Optional: Guard for SSR/unsupported browsers; referencing Notification can throw if not available.

Apply:

-  let permission: NotificationPermission = Notification.permission;
+  // Guard: Notifications API availability (SSR or unsupported browsers)
+  if (typeof window === 'undefined' || !('Notification' in window)) {
+    machine.handleEvent(
+      new GettingPushDeviceDetailsFailed(new ErrorInfo('Notifications API not supported', 40000, 400)),
+    );
+    return;
+  }
+  let permission: NotificationPermission = Notification.permission;
-      machine.handleEvent(
-        new GettingPushDeviceDetailsFailed(
-          new ErrorInfo('Failed to request permission to send notifications', 400, 40000),
-        ),
-      );
+      machine.handleEvent(
+        new GettingPushDeviceDetailsFailed(
+          new ErrorInfo('Failed to request permission to send notifications', 40000, 400, err as Error),
+        ),
+      );

To catch any other instances of reversed ErrorInfo code/status in the repo, run:

🏁 Script executed:

#!/bin/bash
# Find suspicious ErrorInfo usages with status/code potentially swapped
rg -nP -C2 '\bnew\s+ErrorInfo\([^,]+,\s*400\s*,\s*40000\b'
rg -nP -C2 '\bnew\s+ErrorInfo\([^,]+,\s*500\s*,\s*50000\b'

Length of output: 2544

Fix ErrorInfo argument order, include cause, and guard Notification API

  • Critical: Swap the ErrorInfo parameters (code/status) for all three instances in src/plugins/push/getW3CDeviceDetails.ts:
    • “Failed to request permission to send notifications” (line 37)
    • “User denied permission to send notifications” (line 46)
    • “Missing ClientOptions.pushServiceWorkerUrl” (line 54)
  • Critical: In the catch block, include the caught err as the cause when constructing ErrorInfo.
  • Optional: Add an SSR/unsupported‐browser guard before referencing Notification.

Apply the following diffs:

--- a/src/plugins/push/getW3CDeviceDetails.ts
+++ b/src/plugins/push/getW3CDeviceDetails.ts
@@ -28,7 +28,16 @@ export async function getW3CDeviceDetails(machine: PushMachineInterpreter, cli
   // throws exception in iOS
   try {
-    let permission: NotificationPermission = Notification.permission;
+    // Guard: Notifications API availability (SSR or unsupported browsers)
+    if (typeof window === 'undefined' || !('Notification' in window)) {
+      machine.handleEvent(
+        new GettingPushDeviceDetailsFailed(
+          new ErrorInfo('Notifications API not supported', 40000, 400),
+        ),
+      );
+      return;
+    }
+    let permission: NotificationPermission = Notification.permission;
 
     if (permission === 'default') {
       permission = await Notification.requestPermission();
@@ -35,7 +44,11 @@ export async function getW3CDeviceDetails(machine: PushMachineInterpreter, cli
       machine.handleEvent(
         new GettingPushDeviceDetailsFailed(
-          new ErrorInfo('Failed to request permission to send notifications', 400, 40000),
+          new ErrorInfo(
+            'Failed to request permission to send notifications',
+            40000,
+            400,
+            err as Error,
+          ),
         ),
       );
       return;
@@ -44,7 +57,7 @@ export async function getW3CDeviceDetails(machine: PushMachineInterpreter, cli
   if (permission !== 'granted') {
     machine.handleEvent(
-      new GettingPushDeviceDetailsFailed(new ErrorInfo('User denied permission to send notifications', 400, 40000)),
+      new GettingPushDeviceDetailsFailed(new ErrorInfo('User denied permission to send notifications', 40000, 400)),
     );
     return;
   }
@@ -52,7 +65,7 @@ export async function getW3CDeviceDetails(machine: PushMachineInterpreter, cli
   if (!swUrl) {
     machine.handleEvent(
-      new GettingPushDeviceDetailsFailed(new ErrorInfo('Missing ClientOptions.pushServiceWorkerUrl', 400, 40000)),
+      new GettingPushDeviceDetailsFailed(new ErrorInfo('Missing ClientOptions.pushServiceWorkerUrl', 40000, 400)),
     );
     return;
   }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/plugins/push/getW3CDeviceDetails.ts around lines 28-42 (and also update
the related locations at lines 46 and 54), the ErrorInfo numeric arguments are
reversed and the catch doesn't include the caught error as the cause; fix by
swapping the two numeric arguments for the three ErrorInfo constructions
("Failed to request permission to send notifications" at ~37, "User denied
permission to send notifications" at ~46, and "Missing
ClientOptions.pushServiceWorkerUrl" at ~54) so the status and code are in the
correct order, include the caught err as the cause when constructing ErrorInfo
in the catch block, and add a guard before referencing Notification (e.g.,
ensure window is defined and Notification exists or check typeof Notification
!== "undefined") to avoid SSR/unsupported-browser runtime errors.


if (permission !== 'granted') {
machine.handleEvent(
Expand Down
Loading