feat(notifications): use Pinghub websocket #46070

nsakaimbo · 2020-09-30T23:08:07Z

Description

Uses direct Pinghub websocket connection to source realtime notification events.

Context

HMTL5 PushManager service workers are unsupported by Electron
Calypso/renderer websocket connection cannot work in the Desktop app, because it sends the Origin header to the backend. Unfortunately, a bug in Electron's Chromium implementation prevents us from modifying headers of websocket connection requests made in the renderer process. 💩
To enable timely notifications, we have to directly connect to Pinghub (via OAuth) from Electron's Node process

Implementation Overview

Use npm package ws for websocket connection
Use npm package keytar to persist sensitive user credentials to the keychain (OAuth token) (cross-platform keychain support for Windows, Linux and Mac)
Use Automattic npm package wpcom-xhr-request to interface with other notification APIs (fetch note, mark-note-as-read)

Limitations/To-Dos

Cannot sync notifications with badge count (requires dedicated server-side support). We might be able to infer this from Calypso, except:
- Calypso only tracks the unseen, not unread count. Unseen count = all notifications received while the notifications panel is hidden (dot icon), zeroed out when bell is clicked (even though notifications may be unread).
~~Trying to avoid using window events and instead use redux state to force a notifications panel refresh. Not sure if possible?~~ Solved.

matticbot · 2020-09-30T23:08:09Z

Test live: https://calypso.live/?branch=desktop-notifications

matticbot · 2020-09-30T23:12:52Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~72 bytes removed 📉 [gzipped])

name        parsed_size           gzip_size
entry-main       -283 B  (-0.0%)      -72 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~88 bytes removed 📉 [gzipped])

name              parsed_size           gzip_size
security               -103 B  (-0.0%)      -46 B  (-0.0%)
gutenberg-editor       -103 B  (-0.0%)      -42 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~278 bytes removed 📉 [gzipped])

name                                           parsed_size           gzip_size
async-load-automattic-notifications-index-jsx       -841 B  (-0.7%)     -278 B  (-0.8%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

nsakaimbo · 2020-10-01T15:57:08Z

apps/notifications/index.jsx

@@ -279,10 +274,11 @@ export class Notifications extends Component {
 export default connect(
 	( state ) => ( {
 		currentLocaleSlug: getCurrentLocaleVariant( state ) || getCurrentLocaleSlug( state ),
+		forceRefresh: shouldForceRefresh( state ),


Update: Confirmed that the Redux event is firing in Calypso, but is not being picked up here (i.e. doesn't trigger a re-render of the component). I'm not quite sure how the existing getCurrentLocaleVariant getter (above) works in this case.

Update: finally got this working thanks to corrections proposed by @tyxla (d7aec3a). Works like a charm now - thank you for your help!

Glad it worked, @nsakaimbo 🎉

kwight · 2020-10-08T19:42:56Z

@nsakaimbo Is the idea to split implementation for the web (master) and the desktop app going forward? Or are you proposing this be merged to master too, just after the 6.2 release?

kwight · 2020-10-09T14:41:39Z

@nsakaimbo Are there updated testing instructions anywhere for testing PRs like this? I didn't realize wp-desktop was no more, and fully in Calypso (but I don't see a readme explaining know to get it all up and running for testing).

Looking through, it seems you're mostly removing special desktop handling, so that the desktop app uses pinghub the same way Calypso does (but with allowances for Origin issues). Would this change anything for Calypso on web?

nsakaimbo · 2020-10-09T15:50:18Z

Thanks for taking a peek, @kwight! Yes, the desktop app is fully in Calypso now (pbwngY-7c-p2).

Apologies for the lack of documentation, but this is something that will be landing fairly shortly! (Developer docs in #45916).

nsakaimbo · 2020-10-09T15:54:50Z

Looking through, it seems you're mostly removing special desktop handling, so that the desktop app uses pinghub the same way Calypso does (but with allowances for Origin issues). Would this change anything for Calypso on web?

@kwight This is correct. The special desktop handling was added fairly recently (by me 😅 ) in #45793, and while this mostly worked we discovered that Calypso's existing Pinghub connection does not work in the Desktop app (due to a low-level limitation in Electron that we can't even work around) - see pbwngY-7U-p2. For more robust/timely notification support, we're using the OAuth token to connect Electron's Node process to Pinghub directly.

Would this change anything for Calypso on web?

Since we're reverting specialized logic that was only recently added to Calypso (i.e. now the Desktop/Electron will do the heavy lifting w.r.t. Pinghub), Calypso's behavior should remain unchanged (reverting to Calypso's behavior before the initial changes were made to apps/notifications).

belcherj

Just some small things. Looks good. Great Work!!

client/desktop/lib/debounce/index.js

client/desktop/lib/keychain/index.js

client/desktop/lib/notifications/api/notes.js

client/desktop/lib/window-manager/index.js

belcherj

Great, thank you for the changes.

jsnajdr · 2020-10-30T10:43:59Z

client/lib/desktop/index.js

-			}, 1000 );
-		}
+	onNotificationsPanelRefresh: function () {
+		this.store.dispatch( forceNotificationsRefresh( true ) );


I think this should simply import the refreshNotes method from the notifications module and call it. Adding the Redux action and reducer framework around it should be completely unnecessary.

I agree this seems a little convoluted but I can't find a prior example of the client code importing helpers from apps/ directory. I gave this approach a try in 497823d3b9b119fbaa29379d61a1974f24f5e476 but ultimately reverted due to a couple issues:

for some reason, it didn't work to refresh the notifications panel (confirmed by logging activity from the notifications rest-client). Not sure why.

the linter also complained about the import in client/lib/desktop/index.js:

import { refreshNotes } from '../../../apps/notifications/src/panel/notifications'; // error: file is not under 'rootDir'. `rootDir` is expected to contain all source files.

I can't find a prior example of client code importing something from the apps directory, but happy to revisit this as a refactor, though.

client/desktop/lib/cookie-auth/index.js

client/desktop/lib/keychain/index.js

jsnajdr · 2020-10-30T13:06:54Z

client/desktop/lib/notifications/api/notes.js

+
+module.exports = {
+	fetchNote: ( noteId ) => promiseTimeout( 300, fetchNote( noteId ) ),
+	markReadStatus: ( noteId, isRead ) => promiseTimeout( 1000, markReadStatus( noteId, isRead ) ), // response time of this endpoint is very slow!


Why is there only a 0.3s timeout for fetchNote? That's very unfriendly for users on slow connections. And why do we need hard timeouts at all?

I went ahead and bumped the timeouts to a more forgiving value.

IMO it seems having timeout is useful from a user responsiveness perspective - at the very least, this gives us the opportunity to display some sort of error message, rather than leave them potentially hanging indefinitely (or for too long).

However if there's some sort of "natural timeout" that happens anyway and adding a hard timeout is truly redundant (either from the server or perhaps even the network request itself) then I'd be happy to take this out.

If the fetchNote or markReadStatus request takes a lot of time, it doesn't block the UI. It's only that the incoming WebSocket notifications take long to appear in the UI, or that marking a notification as read takes a long time or is never really performed on server. The user can continue to use the UI normally.

Nowhere else in Calypso do we put extra timeouts on REST APIs. And even the 2 seconds still are way too short for, e.g., users in Australia or South Africa who are connecting to US servers.

I'd remove them everywhere except the initial boot sequence, which has a potential to brick the UI completely.

jsnajdr · 2020-10-30T13:08:02Z

client/desktop/lib/notifications/api/notes.js

+			},
+			( error, body ) => {
+				if ( error ) {
+					reject( error );


The callback should return after rejecting.

client/desktop/lib/notifications/viewmodel/index.js

client/desktop/window-handlers/notifications/index.js

This reverts commit 2ffb095.

This reverts commit 497823d3b9b119fbaa29379d61a1974f24f5e476.

nsakaimbo requested a review from belcherj September 30, 2020 23:11

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 30, 2020

nsakaimbo force-pushed the desktop-notifications branch 2 times, most recently from 59df8b8 to 0e12e20 Compare October 1, 2020 15:51

nsakaimbo commented Oct 1, 2020

View reviewed changes

nsakaimbo force-pushed the desktop-notifications branch from 0e12e20 to 2ffb095 Compare October 6, 2020 14:36

nsakaimbo changed the base branch from master to release/6.2.0 October 8, 2020 16:55

nsakaimbo requested review from a team as code owners October 8, 2020 16:55

nsakaimbo force-pushed the desktop-notifications branch from ea56feb to 23289f7 Compare October 8, 2020 17:05

belcherj suggested changes Oct 9, 2020

View reviewed changes

nsakaimbo mentioned this pull request Oct 9, 2020

fix: remove pinghub origin header #45886

Closed

1 task

Base automatically changed from release/6.2.0 to master October 20, 2020 00:15

belcherj approved these changes Oct 26, 2020

View reviewed changes

nsakaimbo force-pushed the desktop-notifications branch from eead598 to 6a406a9 Compare October 29, 2020 17:19

jsnajdr reviewed Oct 30, 2020

View reviewed changes

nsakaimbo force-pushed the desktop-notifications branch from a945a4f to b172b17 Compare October 30, 2020 20:53

nsakaimbo added 6 commits October 30, 2020 20:05

feat(notifications): use Pinghub websocket

495d69d

test: add event to middleware

4604af6

selector: use correct reducer key

0cfe424

fix: call refreshNotes

fb7a8dd

notification handler tweaks

d32053c

Revert "test: add event to middleware"

b3e8e14

This reverts commit 2ffb095.

nsakaimbo and others added 15 commits October 30, 2020 20:05

windows: setAppUserModelId

145ff97

comment: notifications/read request

7d63191

comment: setAppUserModelId

62ab69d

refactor: use forEach

208bd10

refactor debounce

2d3bcda

minor reorg of viewmodel

68f01a1

fix await on keychain clear

0b6d58b

fetchNote: return reject

a24419b

api calls: use more forgiving timeouts

4c36a7e

use arrow fn for this

6dedce6

add missing await for delay

f1b0e42

try calling refreshNotes directly

ba429c4

Revert "try calling refreshNotes directly"

209d2ad

This reverts commit 497823d3b9b119fbaa29379d61a1974f24f5e476.

fix imports

a6fbba6

tweak user-login-status handler

6f916f7

nsakaimbo force-pushed the desktop-notifications branch from b172b17 to 6f916f7 Compare October 31, 2020 00:27

fix approved status check

0dc7bd8

nsakaimbo merged commit 7c41344 into master Oct 31, 2020

nsakaimbo deleted the desktop-notifications branch October 31, 2020 01:35

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 31, 2020

jsnajdr mentioned this pull request Nov 2, 2020

Add pinghub handler to wpcom-xhr-wrapper #45863

Closed

andrewserong mentioned this pull request Nov 20, 2020

Coming Soon: check to see if the ETK plugin is installed and hide the setting if so #47569

Merged

nsakaimbo mentioned this pull request Jan 4, 2021

Pinghub refactors #48656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(notifications): use Pinghub websocket #46070

feat(notifications): use Pinghub websocket #46070

nsakaimbo commented Sep 30, 2020 •

edited

Loading

matticbot commented Sep 30, 2020

matticbot commented Sep 30, 2020 •

edited

Loading

nsakaimbo Oct 1, 2020 •

edited

Loading

nsakaimbo Oct 6, 2020

tyxla Oct 7, 2020

kwight commented Oct 8, 2020

kwight commented Oct 9, 2020

nsakaimbo commented Oct 9, 2020

nsakaimbo commented Oct 9, 2020 •

edited

Loading

belcherj left a comment

belcherj left a comment

jsnajdr Oct 30, 2020

nsakaimbo Oct 30, 2020

jsnajdr Oct 30, 2020

nsakaimbo Oct 30, 2020 •

edited

Loading

jsnajdr Nov 3, 2020

jsnajdr Oct 30, 2020

feat(notifications): use Pinghub websocket #46070

feat(notifications): use Pinghub websocket #46070

Conversation

nsakaimbo commented Sep 30, 2020 • edited Loading

Description

Context

Implementation Overview

Limitations/To-Dos

matticbot commented Sep 30, 2020

matticbot commented Sep 30, 2020 • edited Loading

nsakaimbo Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

nsakaimbo Oct 6, 2020

Choose a reason for hiding this comment

tyxla Oct 7, 2020

Choose a reason for hiding this comment

kwight commented Oct 8, 2020

kwight commented Oct 9, 2020

nsakaimbo commented Oct 9, 2020

nsakaimbo commented Oct 9, 2020 • edited Loading

belcherj left a comment

Choose a reason for hiding this comment

belcherj left a comment

Choose a reason for hiding this comment

jsnajdr Oct 30, 2020

Choose a reason for hiding this comment

nsakaimbo Oct 30, 2020

Choose a reason for hiding this comment

jsnajdr Oct 30, 2020

Choose a reason for hiding this comment

nsakaimbo Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

jsnajdr Nov 3, 2020

Choose a reason for hiding this comment

jsnajdr Oct 30, 2020

Choose a reason for hiding this comment

nsakaimbo commented Sep 30, 2020 •

edited

Loading

matticbot commented Sep 30, 2020 •

edited

Loading

nsakaimbo Oct 1, 2020 •

edited

Loading

nsakaimbo commented Oct 9, 2020 •

edited

Loading

nsakaimbo Oct 30, 2020 •

edited

Loading