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 from closed state #880

Closed
wants to merge 3 commits into from
Closed

Fix push from closed state #880

wants to merge 3 commits into from

Conversation

StefanLuecke
Copy link

Fix push notification opening from closed state as suggested in #835 (comment)

remove empty asset array
Comment on lines +212 to +219
try
{
response.putBundle("notification", mNotificationProps.asBundle());
mJsIOHelper.sendEventToJS(NOTIFICATION_OPENED_EVENT_NAME, response, mAppLifecycleFacade.getRunningReactContext());
} catch (NullPointerException e)
{
Log.e(LOGTAG, "notifyOpenedToJS: Null pointer exception");
}
Copy link

Choose a reason for hiding this comment

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

It actually won't fix the issue but will only prevent the app from crashing: instead of receiving a callback on JS side it will only log the "null pointer exception"

Copy link

Choose a reason for hiding this comment

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

In this case @artyorsh I would argue this solution is sufficient, taking into account:

  • It has been fixed similarly in another place in the codebase, where asBundle() call can also throw null pointer exceptions/
    public void getInitialNotification(final Promise promise) {
    if(BuildConfig.DEBUG) Log.d(LOGTAG, "Native method invocation: getInitialNotification");
    Object result = null;
    try {
    final PushNotificationProps notification = InitialNotificationHolder.getInstance().get();
    if (notification == null) {
    return;
    }
    result = Arguments.fromBundle(notification.asBundle());
    InitialNotificationHolder.getInstance().clear();
    } catch (NullPointerException e) {
    Log.e(LOGTAG, "getInitialNotification: Null pointer exception");
    } finally {
    promise.resolve(result);
  • The "intended" behavior of the library is still very much intact, push notifications are received and open properly. Just on some specific platforms (android 12) in some cases, it seems that the bundle can be null, and it would be better to just "bail" when there is no notification to handle rather than crash the app altogether 😅

A "root" cause for that might be more related to #837 , but @DanielEliraz should know more about that?

Choose a reason for hiding this comment

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

Definitely better to have it caught than having the app crashing - true. But will it mean that in the bailed cases there will be no callback on JS? I think another scenario which might be related to the issue is that this function is called twice for some reasons - first time with "good" arguments and the second time with "bad". The fix will have more sense then 🙂

Copy link

@cchaabane cchaabane Jul 12, 2022

Choose a reason for hiding this comment

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

after some testing, @artyorsh 's guess is right. the function is called twice, first with the null bundle, then with the correct data.

@DanielEliraz can we have your review please ?

Choose a reason for hiding this comment

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

@DanielEliraz is this being reviewed?

@@ -192,5 +192,3 @@ package-lock.json

.history

# Typescript build
lib/dist/
Copy link

Choose a reason for hiding this comment

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

No need

@williamtran29
Copy link

williamtran29 commented Sep 13, 2022

Hope this will be merged soon!

@stale
Copy link

stale bot commented Oct 28, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the 🏚 stale label Oct 28, 2022
@stale
Copy link

stale bot commented Nov 9, 2022

The issue has been closed for inactivity.

@stale stale bot closed this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants