[🐛] getInitialNotification returns old notification data on Android about react-native-firebase HOT 15 CLOSED

@klaevv I think I'm facing the same issue. Can you expand on what your workaround is? Would you call getInitialNotification() after log("onNotificationOpenedApp") in my code here?

const useRegisterNotificationActionHandlers = (): void => {
  const linkTo = useLinkTo()
  useEffect(() => {
    messaging()
      .getInitialNotification()
      .then(
        message => {
          if (message == null) return
          log("getInitialNotification")
          handleRemoteMessage(message, linkTo)
        },
        e => logError("Error loading initial notification", e)
      )
    return messaging().onNotificationOpenedApp(remoteMessage => {
      log("onNotificationOpenedApp")
      handleRemoteMessage(remoteMessage, linkTo)
    })
  }, [linkTo])
}

from react-native-firebase.

Yes, calling getInitialNotification solves the issue:

const useListenNotificationOpenEvent = () => {
  return useCallback((navigationRef: MutableRefObject<any>) => {
    return messaging().onNotificationOpenedApp((notification) => {
      if (Platform.OS === 'android') {
        messaging().getInitialNotification()
      }
      navigateFromNotification(navigationRef, notification)
    })
  }, [])
}

from react-native-firebase.

@klaevv thank you for responding!

I'm a little confused, in your example you don't change the notification you're passing to navigateFromNotification, why would just calling messaging.getInitialNotification() help?

from react-native-firebase.

This workaound with seemingly unnecessary getInitialNotification call to get the initial notification is a fix for an issue in the react-native-firebase library where they are storing notification data as a result of the Android's onNewIntent() call, and the getInitialNotification() clears the stored data. We don't need the stored data since we have our own implementation of the onNewIntent() in our MainActivity where we update the current intent.

from react-native-firebase.

@klaevv I see, that makes sense. Would you mind sharing your MainActivity code?

from react-native-firebase.

Here:

@Override
    public void onNewIntent(Intent intent) {
        super.onNewIntent(intent);
        setIntent(intent);
    }

from react-native-firebase.

This looks like it is not actionable here - we expect onNewIntent to be called, but this is definitely a developer trap, as our onNewIntent will only be called if people correctly call the super ReactActivity method when they override

Java (and Kotlin I suppose) have a way to avoid this trap though, I wonder if adding a @CallSuper annotation here would be the correct thing:

https://github.com/facebook/react-native/blob/b38f80aeb6ad986c64fd03f53b2e01a7990e1533/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactActivity.java#L110

...probably on the other methods as well, as they all can potentially have side-effects that module developers expect to happen, but the call chain can be broken if someone overrides the methods in MainActivity without calling the correct super methods in ReactActivity above them in the inheritance chain...

Not something we can handle here but if you open an issue / propose a PR in react-native, putting a link to this issue would be interesting as it would allow for followup knowledge

from react-native-firebase.

@klaevv thank you so much for sharing

@mikehardy Can you expand on what the trap here is? Prior to me trying to work around this I had not overridden onNewIntent

from react-native-firebase.

@klaevv thank you so much for sharing

@mikehardy Can you expand on what the trap here is? Prior to me trying to work around this I had not overridden onNewIntent

That's the trap. That people don't know a super call exists and they need to override it in order for objects you inherit from to meet their contracts.

A CallSuper annotated method will fail lint checks at least, notifying the developer they are missing the call to super, avoiding the broken contract trap

from react-native-firebase.

I'm still not following. Let me play it back and you can tell me where I'm wrong:

Option 1: There's a bug in this library that can be work around by overriding onNewIntent. If so, then this issue should not be closed.

Option 2: There's a bug in the react-native implementation of the ReactActivity. If so, I think the workaround should at least be documented in this library and this issue should remain open.

Option 3: The bug is somewhere in my code of how I use this library.

from react-native-firebase.

@mikehardy wanted to follow up on this!

from react-native-firebase.

Option 1: There's a bug in this library that can be work around by overriding onNewIntent. If so, then this issue should not be closed.

This library is fine. It is a normal expectation for this library to expect onNewIntent to be called. In fact it's the only way to do what we need to do

Option 2: There's a bug in the react-native implementation of the ReactActivity. If so, I think the workaround should at least be documented in this library and this issue should remain open.

No, the bug in react-native should be opened as a react-native bug where they should add the @CallSuper annotation

Option 3: The bug is somewhere in my code of how I use this library.

It is also this. You are not calling super class methods when you override them. If you override a method and you don't call super, that's your issue. Any time you are programming in a language that has inheritance, you have to determine every single time you override a super-class method if you need to also delegate to that method so the super class meets its contracts. You haven't done that analysis correctly and then determined you need to call the super class method (which you do), so the superclass can't meet it's contract. So you have a bug.

Call the superclass method you have overridden, as part of your overridden method implementation

from react-native-firebase.

@mikehardy but I had this issue without overriding anything. There was no super call I was not calling.

from react-native-firebase.

@LA-Johan as you are posting a lot on someone else's issue, it's possible that we are talking at cross-purposes.

All of my discussion of calling a super class is based on this from the original issue author #7749 (comment)

It appears you can just do this and have things working #7749 (comment)

The original poster explained in the description that this is because we store the notification contents of initial notifications (when app is not running) but not notifications when app is running but in background. And if that storage isn't cleared out with a getInitialNotification call then you can get stale data. So they are doing an otherwise-unnecessary getInitialNotification call.

Note that this is all determined from the original poster's description. I believe it answers all your questions and in that followup comment I linked offers you a workaround.

I have not personally had time to investigate, but would welcome a PR if there was some way to handle the notification storage clearance issue when notifications are posted but the app is already running

from react-native-firebase.

@mikehardy I think you're right. I've been getting user reports of notifications leading to the wrong URL on Android. I have not been able to reproduce personally.

After implementing the workarounds suggested by @klaevv the user now reports that they no longer end up at the wrong URL. I had not previously overridden onNewIntent so I do believe that there is a bug in this library, but the workaround works for me.

from react-native-firebase.