Skip to content
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.

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

Fix push notification bug #168

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

ArielDemarco
Copy link
Collaborator

Overview

This PR fixes #113, where push notifications were sometimes not reaching the app when users did not implement UNUserNotificationCenterDelegate methods.

Description

We’ve introduced conditional logic for proxying UNUserNotificationCenterDelegate methods:

  • The SDK now only responds to a selector if the original delegate also implements it.
  • This prevents scenarios where the SDK intercepts notifications without forwarding them, ensuring they always reach the app’s implementation when it exists (or the user).

To account for cases where users don't implement UNUserNotificationCenterDelegate methods, we have added a fallback swizzling of UIApplicationDelegate.application(_:didReceiveRemoteNotification:fetchCompletionHandler:). This ensures that notifications are still captured by the SDK, even when no delegate is explicitly handling them.

Warning

This does not handle cases where an app receives push notifications but does not implement any handling logic (e.g., neither UNUserNotificationCenterDelegate methods nor UIApplicationDelegate.application(_:didReceiveRemoteNotification:fetchCompletionHandler:)). In such scenarios, push notifications will still be received by iOS but we'll not be able to capture them.

Copy link

github-actions bot commented Jan 29, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

Copy link

github-actions bot commented Jan 29, 2025

Warnings
⚠️ No CHANGELOG entry added.
⚠️ No tests added / modified.

Generated by 🚫 Danger Swift against aafc5c4

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 89.87%. Comparing base (55fc0a9) to head (1641f32).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...Notifications/PushNotificationCaptureService.swift 0.00% 23 Missing ⚠️
...ations/UNUserNotificationCenterDelegateProxy.swift 0.00% 7 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   90.43%   89.87%   -0.56%     
==========================================
  Files         422      428       +6     
  Lines       28491    29173     +682     
==========================================
+ Hits        25766    26220     +454     
- Misses       2725     2953     +228     
Files with missing lines Coverage Δ
...ces/EmbraceIO/Capture/CaptureService+Helpers.swift 100.00% <ø> (ø)
...ations/UNUserNotificationCenterDelegateProxy.swift 4.54% <0.00%> (-0.30%) ⬇️
...Notifications/PushNotificationCaptureService.swift 12.32% <0.00%> (-5.68%) ⬇️

... and 9 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data loss issue with Push Notification Capture service
1 participant