[MM-55152] Add new Desktop API endpoints, improve preload script, some clean-up #2900

devinbinnie · 2023-11-03T19:40:46Z

Summary

This PR introduces the concept of a new Desktop App/Web App API, provided by the preload script in the form of methods exposed on the window.desktopAPI object. These are exposed securely by the contextBridge module of Electron that allows us to share some functionality directly to the renderer process, without exposing any modules directly. This way, we can avoid relying on window.postMessage listeners and work with a clean, typed API in the Web App when Desktop App communication is necessary.

In this change we have:

Added the contextBridge API and all the current functionality in the form of functions that the web app can call
Reworked and organized all of the legacy message-passing code that will still need to exist to support older web app versions for the time being
Removed some of the extra code in the preload script that should be in the Main Process to avoid bloating it
Converted the preload script to TypeScript and created a types package to be shared with the Web App
Some additional changes/clean up to support the changes (see comments below).

We'll be waiting to merge this PR until after it has been tested and integrated properly with the Web App. There are also some concerns about running the new API alongside the old one, so we'll have to see what challenges that presents.

Related PR

mattermost/mattermost#25438: Web App PR utilizing the new API

Ticket Link

https://mattermost.atlassian.net/browse/MM-55152

Reworked and updated the preload script to use an updated and more robust Web App API

…logging

devinbinnie · 2023-11-03T19:41:40Z

src/app/serverViewState.ts

@@ -68,7 +68,7 @@ export class ServerViewState {
    }

    getCurrentServer = () => {
-        log.debug('getCurrentServer');
+        log.silly('getCurrentServer');


These logs kept just getting in the way as some of these methods are called very frequently (ie. on every click) so I changed the logging level on them.

devinbinnie · 2023-11-03T19:42:38Z

src/main/app/intercom.ts

@@ -114,7 +113,7 @@ export function handleWelcomeScreenModal() {
    }
 }

-export function handleMentionNotification(event: IpcMainEvent, title: string, body: string, channel: {id: string}, teamId: string, url: string, silent: boolean, data: MentionData) {
+export function handleMentionNotification(event: IpcMainEvent, title: string, body: string, channel: {id: string}, teamId: string, url: string, silent: boolean, data: {soundName: string}) {


Some types, like MentionData and some of the calls types weren't as necessary since we have more control over what is sent up and down through the API, so I've removed most of the extra ones.

devinbinnie · 2023-11-03T19:43:30Z

src/main/preload/callsWidget.js

@@ -1,94 +0,0 @@
-// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.


There was no need for a separate calls widget script since we can assume that everything that is exposed is accessible remotely, and it was easier to just manage one for all use cases.

devinbinnie · 2023-11-03T19:44:12Z

src/main/views/MattermostBrowserView.ts

@@ -89,6 +91,12 @@ export class MattermostBrowserView extends EventEmitter {
        if (process.platform !== 'darwin') {
            this.browserView.webContents.on('before-input-event', this.handleInputEvents);
        }
+        this.browserView.webContents.on('input-event', (_, inputEvent) => {


This was transferred over from the preload script, that checks when the user clicks on the main view to close any of the dropdowns.

devinbinnie · 2023-11-03T19:48:02Z

src/main/windows/callsWidgetWindow.ts


-        if (!this.isAllowedEvent(ev)) {
-            log.warn('Disallowed calls event');
+        if (this.mainView?.webContentsId !== ev.sender.id) {


I decided that it made sense to be more deliberately restrictive on some of these calls methods, so I've removed and reworked all the checks for one-way message passing between the two. It also was necessary to do to merge the preload scripts as the message passing code was getting confused.

…gh the API

…alling

devinbinnie · 2023-11-14T16:27:52Z

src/common/appState.ts

@@ -25,7 +25,7 @@ export class AppState extends EventEmitter {
    updateExpired = (viewId: string, expired: boolean) => {
        ServerManager.getViewLog(viewId, 'AppState').silly('updateExpired', expired);

-        this.unreads.set(viewId, expired);
+        this.expired.set(viewId, expired);


This was just wrong :P

devinbinnie · 2023-11-14T16:29:09Z

src/main/app/intercom.ts

-export function handleMentionNotification(event: IpcMainEvent, title: string, body: string, channel: {id: string}, teamId: string, url: string, silent: boolean, data: MentionData) {
-    log.debug('handleMentionNotification', {title, body, channel, teamId, url, silent, data});
-    NotificationManager.displayMention(title, body, channel, teamId, url, silent, event.sender, data);
+export function handleMentionNotification(event: IpcMainEvent, title: string, body: string, channelId: string, teamId: string, url: string, silent: boolean, soundName: string) {


Simplified this since we should be more specific about what these calls expect.

Does changing this cause any issues for old web apps, or is it this just called from the desktop app itself?

Just called from the Desktop itself, I do some "magic" in the preload script to account for the legacy case so that we could fix it properly here.

hmhealey · 2023-11-22T20:14:04Z

src/main/views/serverDropdownView.ts

@@ -75,7 +75,7 @@ export class ServerDropdownView {

    private init = () => {
        log.info('init');
-        const preload = getLocalPreload('desktopAPI.js');
+        const preload = getLocalPreload('internalAPI.js');


I didn't realize all the BrowserViews that make up the desktop app also needed this preload script. I guess it makes sense since it's also how they access the desktop API

devinbinnie · 2023-11-22T20:29:31Z

@streamer45 Gentle ping to review :)

streamer45 · 2023-11-22T21:52:11Z

@devinbinnie Could you confirm whether this is supposed to be a backwards compatible change? What I mean is, is the expectation that running this Desktop branch against a non-updated webapp (or plugin) should work as before?

streamer45

Gave this a quick pass, mostly focusing on Calls related changes. Overall it looks great, left a couple of comments.

streamer45 · 2023-11-22T22:02:16Z

src/main/windows/callsWidgetWindow.ts

+
+                if (!this.isCallsWidget(ev.sender.id)) {
+                    log.debug('onJoinedCall', 'blocked on wrong webContentsId');
+                    return;


I suppose that case shouldn't really happen in a sane environment but no objections with properly rejecting.

src/main/preload/externalAPI.ts

streamer45

Great stuff, thank you for being patient with me 🤝

@cpoile Would be good if you could do some smoketesting purely for Calls ringing functionality on top of this PR. Not blocking, just something we may want to cover in case I missed anything.

devinbinnie · 2023-11-23T18:40:14Z

Great stuff, thank you for being patient with me 🤝

@cpoile Would be good if you could do some smoketesting purely for Calls ringing functionality on top of this PR. Not blocking, just something we may want to cover in case I missed anything.

I did some preliminary testing, made sure that all the calls functionality worked as I saw, but yeah feel free to do another pass :)

yasserfaraazkhan

@devinbinnie
Tested this on Mac and windows (5.7 version from the artifacts)

inviting user, license upload, custom emoji, slash command, desktop notification on mac and windows, link preview, creating GM, converting gm to private, draft counts, GM Unread message count, creating teams.

jupenur · 2023-12-13T13:35:45Z

package.json

@@ -143,6 +143,7 @@
    "@electron/fuses": "1.6.0",
    "@electron/universal": "1.3.1",
    "@mattermost/compass-icons": "0.1.32",
+    "@mattermost/desktop-api": "*",


Can we depend on a version number here? Not a big deal, but would make it easier to see which version we were actually using in a specific release, if we ever need to figure that out in the future. The lockfile has all that info too, but it's not exactly easy to read.

We actually do this same thing here for the webapp, I believe we do that to make sure we use the local package instead of the remote one while developing.

We want the web app to depend on a specific version so we know how the compatibility works, but for the Desktop App it should always use whatever types are available within the same commit as the app was built.

jupenur · 2023-12-13T14:26:27Z

src/main/preload/externalAPI.ts

+    LEGACY_OFF,
+} from 'common/communication';
+
+const createListener: ExternalAPI['createListener'] = (channel: string, listener: (...args: never[]) => void) => {


This is really nicely handled. Something to be extremely careful with is accidentally returning privileged objects to the webapp context. IpcRendererEvent is one such object and this covers that case well.

FWIW, I believe Electron will not send anything that isn't serializable, so eg. we can't send a function callback by accident.

It'll serialize all objects and proxy all functions. Prototypes chains will be ignored though.

devinbinnie added 7 commits October 27, 2023 10:15

Add constants for app info, add to API

37c95d6

Migrate history button

3f03ee3

Converted calls API over to context bridge, removed some unnecessary …

0f21faf

…logging

Merge remote-tracking branch 'upstream/master' into MM-55152

dcd925d

Convert to TS, add types for web app to consume

2f00c90

Fix tests, prune

4ef50bb

Fix lint

f64aad0

devinbinnie added the Do Not Merge Should not be merged until this label is removed label Nov 3, 2023

devinbinnie requested a review from streamer45 November 3, 2023 19:40

mm-cloud-bot added the release-note label Nov 3, 2023

devinbinnie removed the request for review from streamer45 November 3, 2023 19:40

devinbinnie commented Nov 3, 2023

View reviewed changes

devinbinnie added 5 commits November 10, 2023 16:11

Merge remote-tracking branch 'upstream/master' into MM-55152

8a8d984

More changes to support the legacy API

8ede07c

Force legacy code off, add support for unreads/mentions/expired throu…

1479976

…gh the API

Fix issues with cross-tab login, removed need for log in/log out sign…

f1065c2

…alling

Merge remote-tracking branch 'upstream/master' into MM-55152

6aff875

devinbinnie mentioned this pull request Nov 14, 2023

[MM-55153] Consolidate Desktop App API, use new contextBridge endpoints when available mattermost/mattermost#25438

Merged

Fixed test, typos

da4c97a

devinbinnie requested a review from streamer45 November 14, 2023 16:22

devinbinnie added 2: Dev Review Requires review by a core committer and removed Do Not Merge Should not be merged until this label is removed labels Nov 14, 2023

devinbinnie added this to the v5.7.0 milestone Nov 14, 2023

devinbinnie requested a review from harshilsharma63 November 14, 2023 16:22

devinbinnie commented Nov 14, 2023

View reviewed changes

devinbinnie requested a review from hmhealey November 16, 2023 19:43

devinbinnie added 5 commits November 20, 2023 11:52

More feedback

ccf282b

Use npm package

77e5f5b

Change types and API to provide off listeners

52704b0

Version number

3975133

Lock

965c49a

hmhealey reviewed Nov 22, 2023

View reviewed changes

hmhealey approved these changes Nov 22, 2023

View reviewed changes

streamer45 reviewed Nov 22, 2023

View reviewed changes

Fix typo

d9bece0

devinbinnie requested a review from streamer45 November 23, 2023 14:28

Add sessionID for calls

02d80b1

streamer45 approved these changes Nov 23, 2023

View reviewed changes

devinbinnie requested a review from yasserfaraazkhan November 23, 2023 18:40

devinbinnie removed the 2: Dev Review Requires review by a core committer label Nov 23, 2023

yasserfaraazkhan added Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests and removed Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests labels Nov 26, 2023

devinbinnie added the 3: Security Review Review requested from Security Team label Nov 30, 2023

devinbinnie requested a review from jupenur November 30, 2023 16:13

yasserfaraazkhan approved these changes Dec 4, 2023

View reviewed changes

devinbinnie removed the 3: QA Review Requires review by a QA tester label Dec 4, 2023

jupenur approved these changes Dec 13, 2023

View reviewed changes

devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 3: Security Review Review requested from Security Team labels Dec 13, 2023

devinbinnie merged commit 0cab09b into mattermost:master Dec 13, 2023
21 of 23 checks passed

streamer45 mentioned this pull request Dec 28, 2023

[MM-55503] Add support for new DesktopAPI mattermost/mattermost-plugin-calls#602

Merged

jwilander added the kind/refactor label Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-55152] Add new Desktop API endpoints, improve preload script, some clean-up #2900

[MM-55152] Add new Desktop API endpoints, improve preload script, some clean-up #2900

devinbinnie commented Nov 3, 2023 •

edited

Loading

devinbinnie Nov 3, 2023

devinbinnie Nov 3, 2023

devinbinnie Nov 3, 2023

devinbinnie Nov 3, 2023

devinbinnie Nov 3, 2023

devinbinnie Nov 14, 2023

devinbinnie Nov 14, 2023

hmhealey Nov 15, 2023

devinbinnie Nov 16, 2023

hmhealey Nov 22, 2023

devinbinnie commented Nov 22, 2023

streamer45 commented Nov 22, 2023

streamer45 left a comment

streamer45 Nov 22, 2023

streamer45 left a comment

devinbinnie commented Nov 23, 2023

yasserfaraazkhan left a comment

jupenur Dec 13, 2023

devinbinnie Dec 13, 2023

jupenur Dec 13, 2023 •

edited

Loading

devinbinnie Dec 13, 2023

jupenur Dec 13, 2023

		@@ -1,94 +0,0 @@
		// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.

[MM-55152] Add new Desktop API endpoints, improve preload script, some clean-up #2900

[MM-55152] Add new Desktop API endpoints, improve preload script, some clean-up #2900

Conversation

devinbinnie commented Nov 3, 2023 • edited Loading

Summary

Related PR

Ticket Link

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinbinnie commented Nov 22, 2023

streamer45 commented Nov 22, 2023

streamer45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamer45 left a comment

Choose a reason for hiding this comment

devinbinnie commented Nov 23, 2023

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupenur Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinbinnie commented Nov 3, 2023 •

edited

Loading

jupenur Dec 13, 2023 •

edited

Loading