fix(ios): Remote issues: #1041 / #1096 onForegroundEvent() and getInitialNotification()

LunatiqueCoder commented 1 month ago

👋 Hello!

This PR fixes

912
🤔 Perhaps #999
1041
1076
1096.
1109

🙏 Please keep in mind I just learned Objective C and Swift in order to fix those issues.

What's changed?

onForeground() is called with getInitialNotification() for PRESS and PRESS_ACTION events when app is opened from a terminated state.
remote payload is picked up when local payload is not available. Thanks to this, getInitialNotification() will return the correct payload on app start.
With the above, I think this code will be unreachable so I removed it
onForeground() is called for DELIVERED events by removing this condition, but I have mixed feelings about it. Might need a deeper look.

How to test

Be sure to configure Notifee to handle remote notifications
If you're using patch-package, in your patches folder create a file called @notifee+react-native+9.1.1.patch
Paste the following:

diff --git a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
index 560a62c..c010367 100644
--- a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
+++ b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
@@ -58,13 +58,18 @@
 #pragma mark Application Notifications

 - (void)application_onDidFinishLaunchingNotification:(nonnull NSNotification *)notification {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-  UILocalNotification *launchNotification =
-      (UILocalNotification *)notification.userInfo[UIApplicationLaunchOptionsLocalNotificationKey];
-  [[NotifeeCoreUNUserNotificationCenter instance]
-      onDidFinishLaunchingNotification:launchNotification.userInfo];
-  [[NotifeeCoreUNUserNotificationCenter instance] getInitialNotification];
+  NSDictionary *notifUserInfo =
+      notification.userInfo[UIApplicationLaunchOptionsLocalNotificationKey];
+    
+  if (!notifUserInfo) {
+      // Fallback to remote notification key if local notification key is not available
+      notifUserInfo = notification.userInfo[UIApplicationLaunchOptionsRemoteNotificationKey];
+  }
+
+  if (notifUserInfo) {
+      [[NotifeeCoreUNUserNotificationCenter instance] onDidFinishLaunchingNotification:notifUserInfo];
+      [[NotifeeCoreUNUserNotificationCenter instance] getInitialNotification];
+  }

   [[NotifeeCoreUNUserNotificationCenter instance] observe];
 }
diff --git a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
index beaa360..ff53bef 100644
--- a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
+++ b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
@@ -21,7 +21,6 @@
 #import "NotifeeCoreUtil.h"

 @implementation NotifeeCoreUNUserNotificationCenter
-
 struct {
   unsigned int willPresentNotification : 1;
   unsigned int didReceiveNotificationResponse : 1;
@@ -73,12 +72,31 @@ struct {

 - (nullable NSDictionary *)getInitialNotification {
   if (_initialNotificationGathered && _initialNotificationBlock != nil) {
+
     // copying initial notification
     if (_initialNotification != nil &&
         [_initialNoticationID isEqualToString:_notificationOpenedAppID]) {
-      NSDictionary *initialNotificationCopy = [_initialNotification copy];
+
+      NSMutableDictionary *event = [NSMutableDictionary dictionary];
+      NSMutableDictionary *initialNotificationCopy = [_initialNotification mutableCopy];
+      initialNotificationCopy[@"initialNotification"] = @1;
+
       _initialNotification = nil;
-      _initialNotificationBlock(nil, initialNotificationCopy);
+
+      // Sets the return payload for getInitialNotification()
+     _initialNotificationBlock(nil, initialNotificationCopy);
+
+      // Prepare onForegroundEvent() payload
+      event[@"detail"] = [initialNotificationCopy copy];
+      if ([event[@"detail"][@"pressAction"][@"id"] isEqualToString:@"default"]) {
+          event[@"type"] = @1;  // PRESS
+      } else {
+          event[@"type"] = @2;  // ACTION_PRESS
+      }
+
+      // Call onForegroundEvent() on Javascript side with payload:
+      [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:event];
+
     } else {
       _initialNotificationBlock(nil, nil);
     }
@@ -104,7 +122,6 @@ struct {
   NSDictionary *notifeeNotification =
       notification.request.content.userInfo[kNotifeeUserInfoNotification];

-  // we only care about notifications created through notifee
   if (notifeeNotification != nil) {
     UNNotificationPresentationOptions presentationOptions = UNNotificationPresentationOptionNone;
     NSDictionary *foregroundPresentationOptions =
@@ -148,23 +165,16 @@ struct {
       presentationOptions |= UNNotificationPresentationOptionAlert;
     }

-    NSDictionary *notifeeTrigger = notification.request.content.userInfo[kNotifeeUserInfoTrigger];
-    if (notifeeTrigger != nil) {
-      // post DELIVERED event
-      [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:@{
+    // post DELIVERED event
+    [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:@{
         @"type" : @(NotifeeCoreEventTypeDelivered),
         @"detail" : @{
           @"notification" : notifeeNotification,
         }
-      }];
-    }
+    }];

     completionHandler(presentationOptions);

-  } else if (_originalDelegate != nil && originalUNCDelegateRespondsTo.willPresentNotification) {
-    [_originalDelegate userNotificationCenter:center
-                      willPresentNotification:notification
-                        withCompletionHandler:completionHandler];
   }
 }

mikehardy commented 1 month ago

Hey @LunatiqueCoder 👋 ! Haven't hacked on things with you in a while, good to see you

I need to focus first on making sure Notifee is compatible with react-native 0.76 as I believe we have New Architecture incompatibilities even with the compat mode

But right after that I was planning on tackling all of these issues related to FCM delivery and events not firing and such - and it looks like you have beaten me to it with this PR

This is an area I really only want to change once (or...one more time) though so I need to think pretty deeply about how exactly this should work (should it consume the initial notification or not? how exactly should it interoperate with react-native-firebase? etc). So please be patient as it may be a little bit before I get to this, but it is pretty important to get right and I'm excited to see you've already thought through it with this patch here

mikehardy commented 1 month ago

For one specific question:

This will also enable getInitialNotification() for iOS if the app was opened from a terminated state by pressing the notification. However, the docs say it should be deprecated. How are we supposed to handle this scenario without getInitialNotification()? Should we remove that part from the docs?

The app is started and brought to the foreground and I believe a Notifee event handler is called, to deliver the event?

LunatiqueCoder commented 1 month ago

@mikehardy 🍺 Hehe, long time indeed. Happy to talk to you again

Click to see devs chit chat

I 'm still tinkering, but I was very excited to see I might have fixed some Github issues while I was working on my stuff, especially when I saw you're a core contributor here as well. I'm also planning to open source an Expo config plugin for Notifee's iOS remote notifications. Basically it follows these steps here: https://notifee.app/react-native/docs/ios/remote-notification-support

Anyway, back to our issues:

For one specific question:

This will also enable getInitialNotification() for iOS if the app was opened from a terminated state by pressing the notification. However, the docs say it should be deprecated. How are we supposed to handle this scenario without getInitialNotification()? Should we remove that part from the docs?

The app is started and brought to the foreground and I believe a Notifee event handler is called, to deliver the event?

EDIT: ☝️ Updated first post with answer

As a side note, since apns-push-type: background it's almost useless, we had to rely on remote notifications for iOS without breaking Android.

We ended up using this following snippet with Firebase Node.js SDK which I think it would be very useful to add in the docs):

import type {NextApiRequest, NextApiResponse} from 'next';
import type {Notification} from '@notifee/react-native/src/types/Notification';
import {AndroidImportance} from '@notifee/react-native/src/types/NotificationAndroid';
import {MulticastMessage} from 'firebase-admin/lib/messaging/messaging-api';

const notifeeOptions: Notification = {
  id: Date.now().toString(),
  title: 'Title',
  subtitle: 'Subtitle',
  body: 'Main body content of the notification',
  android: {
    channelId: 'default',
    importance: AndroidImportance.HIGH,
    pressAction: 
  },
  ios: {
    sound: 'default',
    criticalVolume: 1,
    critical: true,
    foregroundPresentationOptions: {
      badge: true,
      banner: true,
      list: true,
      sound: true,
    },
  },
};

const message: MulticastMessage = {
  // ✅ We can continue using local notifcation for Android
  // 👍 while triggering iOS remote notifications from `apns`
  data: {notifee_options: JSON.stringify(notifeeOptions)}, 
  tokens: [],
  android: {
    priority: 'high',
  },
  apns: {
    payload: {
      notifee_options: notifeeOptions,
      aps: {
        alert: {
          // 🚧 This is needed to trigger an alert/remote notification only for iOS
          // 👍 but Android will continue using data-only notifications
          title: 'Hello World',
        },
        mutableContent: true,
      },
    },
  },
};

export default async (req: NextApiRequest, res: NextApiResponse) => {
  try {
    await admin.messaging().sendEachForMulticast(message);

    res.status(200).end();
  } catch (e) {
    res.status(400).end();
  }
};

dprevost-LMI commented 1 month ago

FYI, I just tested the fix, and on my side, the first index in the patch differed from mine.

Also, after some JS debugging, the call to await notified.getInitialNotification(); was not returning at all, aka the promise was hanging there. I'm not an iOS expert, so something else I'm not aware from our project could interfere!

LunatiqueCoder commented 1 month ago

Hmm, just noticed those changes were made with @notifee/react-native@9.0.2. If I update to the latest version, the patch fails. I'll try again and see how it goes. Thanks for the input!

dprevost-LMI commented 1 month ago

I'm also on "@notifee/react-native@9.0.2":

mateoabrbt commented 1 month ago

Amazing work hope to see news about this PR soon 👀 thanks !

shonie2xx commented 1 month ago

Did I just read "just learned Objective C and Swift in order to fix those issues." Great stuff man, looking forward to see this PR merged, thanks!

TestWts2017 commented 4 weeks ago

when we can expect the latest version with the updated code?

mikehardy commented 4 weeks ago

@TestWts2017 you can't. It is open source and I am always very clear with people - when you interact with open source you must abandon your expectations of release dates. If it is important to you, use patch-package and integrate the changes in your project directly, no need to wait

LunatiqueCoder commented 3 weeks ago

☝️ the referenced issues above might be duplicated

mikehardy commented 3 weeks ago

They probably are, most of the complaints in this repo are all actually the same complaint / disagreement with how it functions, or some ridiculously niche thing which is hard to solve 😆

Been traveling all week but will be back to work next week

mateoabrbt commented 3 weeks ago

Hey there! I partly agree with this because the Notifee documentation can be pretty unclear, especially when it comes to integration. It often feels like you have to dig through issues or PRs to get answers. Also, I wonder why Invertase promotes Notifee in react-native-firebase if it’s considered niche? Notifee is such a powerful library and a great addition to any React Native project. Ideally, it should work seamlessly on both iOS and Android with Firebase—if not, maybe it shouldn’t be featured as an option!

mikehardy commented 3 weeks ago

Just to answer one part: Notifee is not at all considered niche! However, a full screen notification is a niche case, as is a permanent notification attached to a foreground service of a niche type, as is scheduling exact alarms. Each of those is a niche feature just based on query traffic here related to them. Non-niche use is "I want to receive an FCM and post a notification, perhaps with styling and an image, in the foreground and background case"

It just so happens that this PR is right in that area of work, which is great. But also why it needs careful consideration as it will affect the majority case.

However, my mention of the other stuff as niche is not to say Notifee shouldn't be supporting it. And in either case, react-native-firebase had to eject the local-notification device API surface area specifically because all those niche use cases (which are each important to someone!) were a huge drag on the core purpose of react-native-firebase which is to wrap firebase APIs.

It is not possible (we tried!) to support the local device notification APIs people want and all the firebase APIs in the react-native-firebase repos, we had to split it out, thus Notifee exists.

And unfortunately, with the pace of device-local notification API change (typically in the form of increasing restrictions to curb abusive behavior from app developers...) and the pace of change of device-local power management which affects message delivery (typically in the form of increasing restrictions to curb abusive behavior from app developers...) there is no simple integration. Troubleshooting unfortunately always starts with a tree of questions of the form "okay, iOS or Android? okay is it a notification, data, or mixed FCM? okay, what version of the operating system?" and gets worse from there.

There is no seemless unfortunately. The work in this PR should make it a great deal better though because yes, it is rough going right now.

Hope that's sort of clear with regard to maintainer goals and repo motivation at least?

mateoabrbt commented 3 weeks ago

I completely agree that separating Notifee from react-native-firebase was the right approach! I also understand the difficulty of meeting OS requirements for both Android and iOS—each has its own set of complexities. Over the past few months, I’ve read through many Notifee issues and comments to better understand how notifications work across platforms, and I want to acknowledge and appreciate all the hard work put into this package.

I’m excited to see the improvements in the PR, as they’ll make these features more accessible. That said, I know that adding new functionality without impacting other areas is a significant challenge, especially given the wide range of use cases Notifee supports. Enhanced documentation, particularly around Firebase integration, could also help more developers avoid common pitfalls and make setup easier. I’ve noticed that this repo used to get frequent responses from you and other maintainers, so better documentation could help lighten that load too.

Thanks for the insight—it really clarifies the goals and challenges behind Notifee!

mikehardy commented 2 weeks ago

I know there is a lot of interest in this one and I am as well. Just wanted to let everyone know it won't sit forever but I'm still backlogged on urgent items - that is there is one repo left that still does not seem to be behaving well with new architecture at all in the form of @invertase/react-native-apple-authentication and I must get that working first. cocoapods/xcodeproj blowing up all react-native-firebase builds last week was unexpected and took time as well.

There is one other new architecture issue here that came up where foreground services appear to freeze and I'll need to handle that as a regression before coming back to this, but once all the new arch stuff is done, this is first. It will happen but I'll appreciate the patience in the meantime

mikehardy commented 4 days ago

Getting closer on this - apple auth qualified on new arch, and got reproduction scaffolding setup in react-native-firebase in order to test this stuff over the weekend https://github.com/invertase/react-native-firebase/pull/8143 - there is methodical progress towards having real test coverage for all the foreground / background stuff. I continue to appreciate the patience

dhruvvbhavsar commented 4 days ago

Thanks man! This fix really helped!!!! Keep doing the good man's work!

focux commented 1 day ago

I'm wondering if this PR also fix the problem where you can't update the badge in the onBackgroundEvent handler... I'm asking because I see that this PR also fix some issues the background event.

invertase / notifee