Introduce desktop notifications #146

ncosta-ic · 2023-11-27T10:07:18Z

Targets #135

Depends on Icinga/icinga-notifications#136
Depends on Icinga/icinga-notifications#215

ncosta-ic · 2024-03-01T13:37:14Z

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

library/Notifications/Model/Daemon/Event.php

library/Notifications/Model/Daemon/EventIdentifier.php

library/Notifications/Daemon/Daemon.php

public/js/notifications-worker.js

application/clicommands/DaemonCommand.php

application/controllers/DaemonController.php

library/Notifications/Daemon/Sender.php

library/Notifications/Model/Daemon/BrowserSession.php

library/Notifications/Daemon/Server.php

library/Notifications/ProvidedHook/SessionStorage.php

application/controllers/DaemonController.php

library/Notifications/Daemon/Daemon.php

library/Notifications/ProvidedHook/SessionStorage.php

library/Notifications/Model/Daemon/Event.php

run.php

library/Notifications/Daemon/Daemon.php

raviks789

So far looks good to me.

public/js/notifications-worker.js

public/js/notifications.js

It's not used at the moment anyway

No further version handling is currently implemented. In the future, legacy versions should be supported to some extent. This requires changes to `Event`, and probably other classses.

…v6 socket bindings This fixes an issue which caused the URI mapping of the connections to fail when not using the IPv6 notation for both IPv4 and IPv6 addresses. It now supports both formats (IPv4 and IPv6 notation) and additionally resolves IPv4 addresses that are passed in the IPv6 notation. Tested with: native IPv4, native IPv4 in IPv6 notation, native IPv6

nilmerg · 2024-06-12T13:53:13Z

@ncosta-ic Please hit "Merge pull request" once Icinga/icinga-notifications#215 is merged. But please make sure it's still up to date with main prior.

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Nov 27, 2023

ncosta-ic linked an issue Nov 27, 2023 that may be closed by this pull request

Desktop Notifications #135

Closed

ncosta-ic force-pushed the desktop-notifications-poc branch from f1c6c63 to 39a10c5 Compare November 27, 2023 11:21

julianbrost mentioned this pull request Nov 30, 2023

Introduce table for session management Icinga/icinga-notifications#136

Merged

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 force-pushed the desktop-notifications-poc branch from 9bda33d to 73d685d Compare February 13, 2024 20:14

ncosta-ic requested a review from nilmerg February 27, 2024 12:51

raviks789 reviewed Mar 15, 2024

View reviewed changes

raviks789 requested changes Mar 28, 2024

View reviewed changes

nilmerg assigned ncosta-ic Apr 9, 2024

nilmerg added the enhancement New feature or improvement label Apr 9, 2024

nilmerg added this to the Beta milestone Apr 9, 2024

nilmerg marked this pull request as ready for review April 9, 2024 14:40

ncosta-ic marked this pull request as draft April 17, 2024 11:02

ncosta-ic force-pushed the desktop-notifications-poc branch from ddcd2e0 to 5853886 Compare April 23, 2024 15:10

ncosta-ic requested a review from raviks789 April 23, 2024 15:11

raviks789 requested changes May 2, 2024

View reviewed changes

ncosta-ic requested a review from raviks789 May 7, 2024 08:20

ncosta-ic force-pushed the desktop-notifications-poc branch from 274952d to 0a3eca3 Compare May 7, 2024 08:30

raviks789 previously requested changes May 7, 2024

View reviewed changes

library/Notifications/Daemon/Daemon.php Outdated Show resolved Hide resolved

raviks789 reviewed May 7, 2024

View reviewed changes

sukhwinder33445 reviewed May 8, 2024

View reviewed changes

public/js/notifications-worker.js Outdated Show resolved Hide resolved

ncosta-ic force-pushed the desktop-notifications-poc branch from b5110b6 to f17f8a0 Compare May 16, 2024 09:00

sukhwinder33445 reviewed May 16, 2024

View reviewed changes

sukhwinder33445 reviewed May 17, 2024

View reviewed changes

public/js/notifications.js Outdated Show resolved Hide resolved

sukhwinder33445 reviewed May 17, 2024

View reviewed changes

public/js/notifications.js Outdated Show resolved Hide resolved

ncosta-ic force-pushed the desktop-notifications-poc branch from 491cb1c to 305d415 Compare May 17, 2024 16:10

ncosta-ic marked this pull request as ready for review May 17, 2024 16:11

ncosta-ic and others added 26 commits June 12, 2024 15:48

feat(Model\Daemon): mappings for new database table browser_session

9b8d6bb

feat: introduce daemon backend, daemon data classes

7592ca2

refactor(Model): type-hint contact identifier

8cac093

feat: add session storage hook

84819f3

feat: add notification banner icons

4b432ff

feat(JS): add service worker and JS module

f8217b1

feat(Controller): load logic and JS file routing

42cbb5b

docs: add desktop notifications documentation

0ba0adb

js: Simplify constructor, split dependency checks and initial load

b28343c

js: Attempt to open an event stream during init

386d03d

IncidentHistory: Add group by workaround

666fbb7

Daemon: Only fetch transmitted notifications

002e0bf

js: Expect the server to render an object's name

ea0dae9

js: Expect the server to provide a notification message

92c7f59

DaemonCommand: Add documentation

ec3dd10

Event: Omit id property

535aba7

It's not used at the moment anyway

js: Add note that new workers cannot take over active ones

1f59884

Use a versioned route to subscribe to notification messages

86c990e

No further version handling is currently implemented. In the future, legacy versions should be supported to some extent. This requires changes to `Event`, and probably other classses.

Introduce default systemd file for the background daemon

0f789dc

doc: Document the desktop notification feature

fae1346

daemon/script: Make absolutely sure to prevent traversal attacks

fb306e9

Daemon: Let integrations render an notification's title

5aa604c

Daemon\Server: Enhance error logging

3be1b33

refactor(SessionStorage): add username trimming

0028789

feat(Session): provide authenticated_at programmatically

e9163b4

nilmerg force-pushed the desktop-notifications-poc branch from d1db5d4 to e9163b4 Compare June 12, 2024 13:48

ncosta-ic merged commit ba627b4 into main Jun 13, 2024
22 checks passed

ncosta-ic deleted the desktop-notifications-poc branch June 13, 2024 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce desktop notifications #146

Introduce desktop notifications #146

ncosta-ic commented Nov 27, 2023 •

edited

Loading

ncosta-ic commented Mar 1, 2024 •

edited

Loading

raviks789 left a comment

nilmerg commented Jun 12, 2024

Introduce desktop notifications #146

Introduce desktop notifications #146

Conversation

ncosta-ic commented Nov 27, 2023 • edited Loading

ncosta-ic commented Mar 1, 2024 • edited Loading

Current observations

iOS

Android

Possible fix?

raviks789 left a comment

Choose a reason for hiding this comment

nilmerg commented Jun 12, 2024

ncosta-ic commented Nov 27, 2023 •

edited

Loading

ncosta-ic commented Mar 1, 2024 •

edited

Loading