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

Introduce desktop notifications #146

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

ncosta-ic
Copy link
Member

@ncosta-ic ncosta-ic commented Nov 27, 2023

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Nov 27, 2023
@ncosta-ic ncosta-ic linked an issue Nov 27, 2023 that may be closed by this pull request
@ncosta-ic ncosta-ic force-pushed the desktop-notifications-poc branch 2 times, most recently from e76e36b to 9bda33d Compare February 1, 2024 09:45
@ncosta-ic
Copy link
Member Author

ncosta-ic commented Mar 1, 2024

Current observations

The general logic that triggers the desktop notifications is pretty stable now. However, there are some issues with mobile devices, that are currently unsolved.

iOS

  • iOS 16 or greater is required, as the Notification API only got introduced with Safari (iOS) 16.4.
  • A web application is only allowed to make use of the Notification API if it's being run as a home-screen application (share -> add to home-screen).
  • The logic itself works fine for as long as the home-screen application stays active.
  • Leaving the application or locking the phone results in the operating system (iOS) freezing the browser activities after a short amount of time (around 5 seconds).
    This behaviour kills all external connections while not running any browser logic, which causes some unwanted issues.
  • After unlocking the phone or by going back to the home-screen application, the browser itself gets unfreezed, which causes the tabs and web workers to continue their workflows.
  • The event-stream should normally reconnect once it looses its connection (see second to last paragraph). This won't happen as it's proxied by the service worker, which itself pipes the proper event-stream from the daemon back to the tab. During the freeze phase, this connection gets dropped and the stream stays empty and locked (only an assumption, it's not possible to remote debug service workers on iOS).

This results in the app stopping to show notifications after the home-screen application was put into background or if the phone got locked.

Android

  • Untested

Possible fix?

One way around this problem, albeit being a real ugly one, would be to start a freshness timer in the service worker context once the tab gets the visiblestate event fired with a value of hidden. This event is always fired once the browser stops rendering a tab, even on mobile phones and even when putting a PWA into the background or locking the screen.
As the service worker is going to get freezed as well, the freshness checks will get delayed. Once it unfreezes again, it continues with its freshness checks but notices a time delay from the last check to the current.
This could then in return trigger a reconnection on the tabs and the whole feature would continue to work. The repeating freshness checks could then be stopped as the visibilitychange event would be retriggered with a value of visible.

application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
library/Notifications/Model/Daemon/Event.php Outdated Show resolved Hide resolved
library/Notifications/Model/Daemon/EventIdentifier.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Daemon.php Show resolved Hide resolved
public/js/notifications-worker.js Outdated Show resolved Hide resolved
application/clicommands/DaemonCommand.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Sender.php Outdated Show resolved Hide resolved
library/Notifications/Model/Daemon/BrowserSession.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Server.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Server.php Outdated Show resolved Hide resolved
library/Notifications/ProvidedHook/SessionStorage.php Outdated Show resolved Hide resolved
library/Notifications/ProvidedHook/SessionStorage.php Outdated Show resolved Hide resolved
library/Notifications/ProvidedHook/SessionStorage.php Outdated Show resolved Hide resolved
@nilmerg nilmerg added the enhancement New feature or improvement label Apr 9, 2024
@nilmerg nilmerg added this to the Beta milestone Apr 9, 2024
@nilmerg nilmerg marked this pull request as ready for review April 9, 2024 14:40
@ncosta-ic ncosta-ic marked this pull request as draft April 17, 2024 11:02
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
application/controllers/DaemonController.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Daemon.php Outdated Show resolved Hide resolved
library/Notifications/ProvidedHook/SessionStorage.php Outdated Show resolved Hide resolved
library/Notifications/ProvidedHook/SessionStorage.php Outdated Show resolved Hide resolved
library/Notifications/Model/Daemon/Event.php Show resolved Hide resolved
run.php Outdated Show resolved Hide resolved
library/Notifications/Daemon/Daemon.php Outdated Show resolved Hide resolved
Copy link
Contributor

@raviks789 raviks789 left a comment

Choose a reason for hiding this comment

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

So far looks good to me.

public/js/notifications-worker.js Outdated Show resolved Hide resolved
public/js/notifications-worker.js Outdated Show resolved Hide resolved
public/js/notifications-worker.js Outdated Show resolved Hide resolved
public/js/notifications-worker.js Outdated Show resolved Hide resolved
public/js/notifications-worker.js Outdated Show resolved Hide resolved
public/js/notifications.js Outdated Show resolved Hide resolved
public/js/notifications.js Outdated Show resolved Hide resolved
public/js/notifications.js Outdated Show resolved Hide resolved
public/js/notifications.js Outdated Show resolved Hide resolved
public/js/notifications.js Outdated Show resolved Hide resolved
@ncosta-ic ncosta-ic marked this pull request as ready for review May 17, 2024 16:11
@ncosta-ic
Copy link
Member Author

@raviks789

So far looks good to me.

As it seems like you're fine with the changes that I implemented, you might want to approve your review. It still needs the approval of @sukhwinder33445 and @nilmerg either way.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desktop Notifications
4 participants