[Fix] very rare bug where app doesn't opening from notification tap when it cold starts the app

[jkasten2]

Description

One Line Summary

Fix very rare bug where app doesn't opening from notification tap when it cold starts the app.

Details

In rare cases when tapping on a notification the app would not open, as well as other issues like the click not being counted either.

The issue was due to a race condition between
NotificationListener.start and processing the click event. To fix and to avoid this mistake in the future moved all the code from NotificationListener into NotificationLifecycleService. There wasn't a clear set of responsibility difference between these to classes so this is also a win to remove complexity.

Motivation

By default it is expected that the app should open when tapping on a notification.

Scope

Code changes are only around notification event handling.

Testing

Unit testing

Added new NotificationLifecycleServiceTests test class.

Manual testing

Was able to reproduce the issue 1/5 times on an Android 14 (no extension level) emulator. After testing 20 times I am no longer able to reproduce the issue now.

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
  • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
• 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 

[jinliu9508]

I added some logs in this branch and triggered a notification. The logs appear in the correct order, which suggests that this implementation should work as expected.

That said, I had an additional thought. Converting NotificationListener into a bootstrappable service and moving the logic from start() to bootstrap() could also address this issue. Currently, all code within startableService.start() is executed on a background thread after initWithContext, which introduces the potential for a race condition—specifically, where intLifeCycleHandler is not subscribed before notificationReceived is invoked. This alternative approach might also resolve the timing issue related to the intLifecycleCallback call in canReceiveNotification.

However, as you mentioned, the current PR simplifies the architecture by removing the now-unnecessary NotificationLifeCycleEventHandler, which I’m also aligned with. I can approve this unless you want to try the other approach.

[jinliu9508]

This line is not used, should we remove it?

[jkasten2]

This line was pre-existing, the whole function was copied over from the other file. However, I cleaned it up since were are looking at it now.

[jkasten2]

I added some logs in this branch and triggered a notification. The logs appear in the correct order, which suggests that this implementation should work as expected.

That said, I had an additional thought. Converting NotificationListener into a bootstrappable service and moving the logic from start() to bootstrap() could also address this issue. Currently, all code within startableService.start() is executed on a background thread after initWithContext, which introduces the potential for a race condition—specifically, where intLifeCycleHandler is not subscribed before notificationReceived is invoked. This alternative approach might also resolve the timing issue related to the intLifecycleCallback call in canReceiveNotification.

However, as you mentioned, the current PR simplifies the architecture by removing the now-unnecessary NotificationLifeCycleEventHandler, which I’m also aligned with. I can approve this unless you want to try the other approach.

bootstrap runs on the main thread and given the dependencies I believe it would result in disk I/O on the main thread I didn't want to reintroduce. Even if that wasn't an issue it makes it fragile as if we decide to change some initialization code it could easily reintroduce the race condition.

In this case it made sense to merge the classes, since they had the same responsibilities anyway, but this isn't a solution across the code base. One idea is on all passed in dependencies we call start() on them automatically, if it hasn't been called already, and it is an IStartableService of course.