fix: handle iOS exceptions when requesting notification permission #2071
Conversation
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
iOS forbids to ask for permissions in the async task
WalkthroughAdjusted browser push permission handling: read Notification.permission, request only when 'default', wrap requestPermission in try/catch to handle platform exceptions, emit specific failure events on errors or denial, and keep subsequent push subscription logic unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant U as Caller
participant G as getW3CDeviceDetails
participant N as Notification API
participant P as Push API
participant E as EventEmitter
U->>G: invoke
G->>N: read Notification.permission
alt permission == "default"
G->>N: requestPermission()
note over N,G: Some platforms (e.g., iOS) may throw
alt success
N-->>G: permission result
else throws
G-->>E: emit GettingPushDeviceDetailsFailed(ErrorInfo("Failed to request permission...", 400, 40000))
G-->>U: return
end
end
alt permission != "granted"
G-->>E: emit GettingPushDeviceDetailsFailed("User denied permission to send notifications")
G-->>U: return
else permission == "granted"
G->>P: proceed with push subscription (unchanged)
P-->>G: subscription details
G-->>U: return subscription details
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/push/getW3CDeviceDetails.ts (1)
44-47: Swap ErrorInfo code/status for permission denialUse code=40000 (Ably error code) and statusCode=400 (HTTP). Current order is reversed.
- machine.handleEvent( - new GettingPushDeviceDetailsFailed(new ErrorInfo('User denied permission to send notifications', 400, 40000)), - ); + machine.handleEvent( + new GettingPushDeviceDetailsFailed(new ErrorInfo('User denied permission to send notifications', 40000, 400)), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/plugins/push/getW3CDeviceDetails.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/plugins/push/getW3CDeviceDetails.ts (1)
src/plugins/push/pushactivation.ts (1)
GettingPushDeviceDetailsFailed(431-438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
Comment on lines
+28
to
+42
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 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
ErrorInfoparameters (code/status) for all three instances insrc/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
erras thecausewhen constructingErrorInfo. - 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.
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.
iOS forbids to ask for permissions in the async task
Summary by CodeRabbit