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: Notification click not foreground the app in the first click if app is closed and no clickListener is added #2259

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Feb 20, 2025

Description

One Line Summary

This PR fixes a bug where clicking on a notification does not always bring the app to the foreground under certain conditions.

Details

Motivation

Some users have reported that clicking a notification for the first time does not foreground the app when it is fully closed. This creates unexpected behavior, and we aim to ensure the app opens correctly when a notification is clicked.

Scope

This PR will result in two app behavior changes:

  • Notifications are now by default openable as long as the notifications module is included, instead of depending on whether there is an external click listener.
  • 'InAppMessagePreviewHandler' is now subscribed to 'NotificationLifecycleService' during 'initWithContext()', instead of subscribing in a background thread AFTER the 'initWithContext()', to prevent the incorrect subscribing status if it is accessed during the initialization.

OPTIONAL - Root cause explained

  1. When a notification is clicked, 'NotificationOpenedProcessor' determines whether the notification should open.
  2. 'NotificationLifecycleService.canOpenNotification' is accessed, and its result depends on whether:
    * There is a subscriber in 'extOpenedCallback', or
    * 'setInternalNotificationLifecycleCallback' is called and subscribed.
  3. If 'addClickListener' is used, 'extOpenedCallback' has a subscriber, so 'canOpenNotification' returns 'true', allowing the app to open. This explains why adding 'addClickListener()' works as a workaround.
  4. However, if 'addClickListener' is never used, we rely on 'setInternalNotificationLifecycleCallback', which is only called in 'InAppMessagePreviewHandler.start()'. Since this runs asynchronously after 'initWithContext', accessing it before 'start()' always returns 'false'.
  5. This also explains why the second click opens the app—because 'start()' has already been called after the first click.

Testing

Unit testing

Manual testing

To reproduce the scenario:

  1. Switch to the first commit that removes all click listeners.
  2. Run the app, consent to privacy, and grant push notification permissions.
  3. Send a push notification.
  4. Move the app to the background and force close it.
  5. Click on the notification.
    Expected: The notification should open the app.

Observed (before fix): The first click neither opened the app nor cleared the notification.
After the fix, we can observe that the first click always open the app and clear the notification tray.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • PR includes a refactoring commit that separates two interfaces into two files instead of one and updates the comments of each interface to be more detailed.
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@jinliu9508 jinliu9508 added the WIP Work In Progress label Feb 20, 2025
@jinliu9508 jinliu9508 force-pushed the notification_click_not_foregrounded branch 2 times, most recently from f9ccc01 to f21b96d Compare February 21, 2025 20:03
@jinliu9508 jinliu9508 removed the WIP Work In Progress label Feb 21, 2025
@jinliu9508 jinliu9508 force-pushed the notification_click_not_foregrounded branch from f21b96d to f5cde9a Compare February 24, 2025 16:34
@jinliu9508
Copy link
Contributor Author

Squashing and removing commits around reproducing the issue.

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.

2 participants