Protocol deep linking #616

csduarte · 2017-10-10T17:32:51Z

Description
Adds protocol mattermost to fire up the desktop application from links. Also adds deep linking to be able to go to the specific channel on the link, this was done without the need for a hard refresh if the application is already running.
A use case are links on emails that would allow the user to access the desktop application directly. This was added for mac and windows.
Based on this feature proposal:

read and understood our Contributing Guidelines
completed Mattermost Contributor Agreement
executed npm run lint:js for proper code formatting

Test Cases
Tested with OSX El Capitan 10.11 and Windows 8.1.
Package and run the application and run example links in a browser:

mattermost://pre-release.mattermost.com/core/channels/ps-uber
mattermost://pre-release.mattermost.com/core/channels/apiv4
mattermost://pre-release.mattermost.com/core/channels/bugs

There are two cases, when the application is already running. Or after the protocol has registered in the OS, then close the app and try the protocol from a restarted browser and it should start the application correctly and go to the right channel. (edited)

jasonblais

Thanks! @csduarte @dmeza I'm getting the following errors when trying to launch the test build

Wondering if you've seen that error before when working on this branch? For reference, the server URL on the above screenshot was https://pre-release.mattermost.com

PS: downloaded the win64.exe file from https://circleci.com/gh/mattermost/desktop/1060#artifacts

yuya-oc · 2017-10-11T13:29:29Z

@jasonblais For now, please restart the app. It seems that the problem happens while installing the app via Squirrel installer. Because it adds command line options, so the deeplink feature is confused at the first time.

yuya-oc · 2017-10-11T13:33:50Z

Updated the first comment for our readability.

yuya-oc

You should handle which tab should load the deeplink. Otherwise, non-appropriate tabs may also open the link.

Please add an test case: http://pre-release.mattermost.com is registered as second or later server. And another server is registered as the first server.

yuya-oc · 2017-10-11T13:42:29Z

src/main.js

+  // Protocol handler for win32
+  if (process.platform === 'win32') {
+    // Keep only command line / deep linked arguments
+    if (Array.isArray(process.argv.slice(1)) && process.argv.slice(1).length > 0) {


You should also handle the case, command line includes --squirrel-* options. In that case, the app should run as an installer/uninstaller. So please do not anything.

yuya-oc · 2017-10-11T13:44:52Z

src/browser/components/MattermostView.jsx

@@ -44,8 +43,18 @@ const MattermostView = createReactClass({
    var self = this;
    var webview = findDOMNode(this.refs.webview);

+    ipcRenderer.on('protocol-deeplink', (event, lastUrl) => {


You should handle which tab should load the deeplink. Otherwise, non-appropriate tabs may also open the link.

yuya-oc · 2017-10-11T13:45:45Z

src/browser/components/MattermostView.jsx

@@ -223,14 +232,18 @@ const MattermostView = createReactClass({
    if (!this.props.active) {
      classNames.push('mattermostView-hidden');
    }
+
+    const deeplinkingUrl = remote.getCurrentWindow().deeplinkingUrl;


You should handle which tab should load the deeplink. Otherwise, non-appropriate tabs may also open the link.

jasonblais · 2017-10-11T17:15:36Z

@yuya-oc Do you know if it's a lot of work to support this for Linux as well? Wondering if we could add it ourselves after this PR is merged, or if I should create a separate issue for us to look at later.

@csduarte @dmeza Is server work required to support deep linking from email notifications?

jasonblais · 2017-10-11T17:16:27Z

Also, any security concerns we should be mindful of?

dmeza · 2017-10-11T18:04:17Z

@jasonblais @yuya-oc no server work is required just to use the protocol and deep linking, but it's good practice to have a server side landing page that can show download instructions in case the native app is not installed. I have a prototype version of it ready and will create a PR for it once we have it.
It's like the last part of this tutorial:
https://developers.tune.com/sdk/deep-linking-to-your-mobile-app-from-your-website/
No security concerns that I know of. cc @csduarte

dmeza · 2017-10-11T18:15:37Z

@yuya-oc do you know of an easy way of reading the protocol value from electron-builder.json so we don't have to have it in src/main.js. The other option is to add it as a config and be able to override it. What do you think?
cc @csduarte

jasonblais · 2017-10-11T18:17:39Z

^(I'd prefer to avoid adding a config setting whenever possible)

yuya-oc · 2017-10-12T13:49:19Z

@dmeza Please use require('../electron-builder.json'). The json is imported into src/main_bundle.js by webpack. Unfortunately I'm not sure how electron-builder uses the protocol (and not documented on https://www.electron.build/configuration/configuration).

Possibly this is not security, but we should take care that users might take mistake if the tab opens incorrect server by deeplinking. As I wrote in review comments, the link should be opened in the appropriate tab.

yuya-oc · 2017-10-12T13:51:46Z

@jasonblais Possibly we can support Linux by adding MimeType entry into Mattermost.desktop file. http://archive.is/8C3zb First, we should know whether electron-builder adds it for .deb packages.

dmeza · 2017-10-14T04:53:32Z

@yuya-oc @jasonblais added logic for:

deep linking to go to the right tab and channel based on domain
take value of protocol from electron-builder.json to be able to white-label easily.

Not really sure on how to solve and test the squirrel comment.

yuya-oc · 2017-10-14T12:46:49Z

https://circleci.com/gh/mattermost/desktop/1075#artifacts

yuya-oc

Basically, the feature correctly works on macOS. Please review my comments.

yuya-oc · 2017-10-14T12:55:36Z

src/browser/components/MainPage.jsx

+    if (deeplinkingUrl !== null) {
+      for (var i = 0; i < this.props.teams.length; i++) {
+        if (deeplinkingUrl.includes(this.props.teams[i].url)) {
+          key = i;


mainWindow.deeplinkingUrl should be deleted here. In current implementation, when a user opens then closes the settings page, the main window always comes back to the deeplinking URL.

Test case:

Register https://pre-release.mattermost.com as the second server.

Quit the app.

Open mattermost://pre-release.mattermost.com/core/channels/apiv4

The app should show the second tab and the channel. [OK]

Select another channel.

Open the settings page (Ctrl/Cmd+,) then select the first server.

The app should open the first tab. [NG]

The second tab should keep the last channel. [NG]

This case was weird and hard to handle, but it's done and tested.

yuya-oc · 2017-10-14T12:59:11Z

src/main.js

+  // argv: An array of the second instance’s (command line / deep linked) arguments
+  if (process.platform === 'win32') {
+    // Keep only command line / deep linked arguments
+    if (Array.isArray(commandLine.slice(1)) && commandLine.slice(1).length > 0) {


The app needs to do not anything other than installation or related works when --squirrel-* command line flags exist. They are --squirrel-install, --squirrel-uninstall, --squirrel-updated and --squirrel-obsolete. https://github.com/electron/windows-installer#handling-squirrel-events

yuya-oc · 2017-10-14T13:06:05Z

src/main.js

@@ -313,6 +326,27 @@ ipcMain.on('download-url', (event, URL) => {
  });
 });

+let scheme;
+if (protocols && protocols[0] &&


Should we consider the case where multiple protocols are listed?

@yuya-oc I tried adding multiple protocols but after a while I realized that it's not possible because: app.setAsDefaultProtocolClient(protocol) just registers the last one.

@dmeza Got it, thanks.

yuya-oc · 2017-10-14T13:18:23Z

src/utils/util.js

+const REGEXP_DOMAIN = /(?:[^/]*\/){3}/;
+
+export function getDomain(url) {
+  const matched = url.match(REGEXP_DOMAIN);


Just curious, why not using Node.js URL module?

And I expected the return value of this function is example.com when url is https://example.com/test, but it seems that the actual return value is https://example.com/. Would you declare more right name? (It's called origin in Node.js URL module https://nodejs.org/dist/latest/docs/api/url.html)

yuya-oc · 2017-10-14T13:28:51Z

src/main.js

+app.on('open-url', (event, url) => {
+  event.preventDefault();
+  setDeeplinkingUrl(url);
+  mainWindow.webContents.send('protocol-deeplink', deeplinkingUrl);


You should also call mainWindow.show() to bring up hidden/minimized window.

yuya-oc · 2017-10-14T13:35:14Z

src/browser/components/MainPage.jsx

@@ -108,6 +121,20 @@ const MainPage = createReactClass({
    ipcRenderer.on('focus-on-webview', () => {
      this.focusOnWebView();
    });
+
+    ipcRenderer.on('protocol-deeplink', (event, lastUrl) => {
+      const mattermostViews = document.getElementsByClassName('mattermostView mattermostView-with-tab');


This is equal to props.teams.length. And you can access to the view via refs[mattermostView${i}]. So you don't have to search around DOM.

By adding MattermostView.getSrc(), MainPage doesn't have to know about internal <webview>. It should be the role of MattermostView.

jasonblais · 2017-10-17T17:30:49Z

Works on Windows as well.

@dmeza Curious why we didn't use a protocol handler that was more consistent with other apps, in the form:

mattermost://channel?id=<CHANNEL-ID>&team=<TEAM-ID>

Is this something we should consider?

Created two follow-up tickets:

Support protocol deep linking on Linux (@yuya-oc): Support protocol deep linking on Linux #627
Add a server landing page when the app isn't installed (@dmeza): Add a server landing page when deep linking to an app when it's not yet installed #628

dmeza · 2017-10-19T15:42:55Z

@jasonblais @yuya-oc the protocol handler is this part mattermost:// the other part are the parameters used for deep linking.
In the case of the desktop app we need the domain to know what tab to use and the rest of the url to go to that specific path inside of that server and not have to refresh all the webview everytime.

For the IOs and Android apps we might need to add the other parameters at the end. For example:
mattermost://pre-release.mattermost.com/core/channels/bugs?id=<CHANNEL-ID>&team=<TEAM-ID>

jasonblais · 2017-10-20T15:02:45Z

@dmeza Why would we need the additional parameters for mobile?

Makes sense that we need the server URL to know which tab to switch to (I realize now it wouldn't work without it). The existing implementation sounds good.

@dmeza Please help review Yuya's earlier comments from code review, and this should be good to merge.

Also adding @MusikPolice as a second code reviewer.

dmeza · 2017-10-20T15:09:35Z

@jasonblais the additional parameters are needed for mobile because they don't use urls like platform or desktop. We discussed with @jarredwitt that they make APIv4 calls directly.

I have the changes for @yuya-oc's comment almost ready. I'll push later today.

jasonblais · 2017-10-20T15:19:40Z

@dmeza Got it, thanks!

And that's great, thanks for your continued work on this PR

dmeza · 2017-10-20T18:29:27Z

@yuya-oc @jasonblais except for the multi protocol that's not possible, all other comments are done. Tested on mac and windows that tabs display properly based on domain and that it goes to the right channel. Also tested that it works with the app closed.
Let me know if you see any other issues.

yuya-oc · 2017-10-21T12:14:04Z

Tested https://circleci.com/gh/mattermost/desktop/1080#artifacts

Almost okay. And a permalink as deeplinking also works #622 (comment).

mattermost://pre-release.mattermost.com/core/pl/bc3he4u37fdwzqwfdtychcc1go
(You would see "v3.7.1 has been published" of Desktop App channel)

But when the application was launched by deeplinking, it keep the link even when after using the settings page. #616 (comment)

…main

… to whitelabel in an easy manner.

… windows squirrel installer.

…ow closes.

dmeza · 2017-10-24T00:05:46Z

@yuya-oc fixed this case: #616 (comment)

In that case, electron.exe is registered as the protocol client. The app would not work because app dir is not set when launching.

yuya-oc

Modified a little.

Removed a global variable.
Disabled the feature in development mode.

yuya-oc · 2017-10-24T12:47:49Z

https://circleci.com/gh/mattermost/desktop/1084#artifacts

The test case has been resolved 👍

dmeza · 2017-10-24T15:37:01Z

src/main.js

@@ -46,7 +46,7 @@ const assetsDir = path.resolve(app.getAppPath(), 'assets');
 // be closed automatically when the JavaScript object is garbage collected.
 var mainWindow = null;
 let spellChecker = null;
-let deeplinkingUrl = null;


@yuya-oc @jasonblais removing the global variable deeplinkingUrl breaks the logic to be able to deep link when the application is closed. Test case would be to have multiple tabs, close the app completely and deep link to a channel in the second tab. It stays on the first tab.

@dmeza I believed there are no cases which must refer the global variable though, I’ll check again. Thanks.

dmeza · 2017-10-24T16:12:07Z

@yuya-oc a question just to educate me, why is it a good idea to disable deep linking in dev mode?

yuya-oc · 2017-10-24T22:49:41Z

@dmeza It seems that electron.exe is registered as the protocol client in dev mode. In such case, the application couldn’t launch correctly because the second arg to specify runtime code is missing.

yuya-oc · 2017-10-25T12:38:04Z

As mentioned by @dmeza #616 (comment), the global variable is needed for mac at app.on('open-url') event. So I reverted the commit ea746ba as d2c4aa0. Thanks!

jasonblais requested changes Oct 11, 2017

View reviewed changes

jasonblais added All Platforms null Hacktoberfest null Type/Enhancement MacOS (looking for testers) Windows (looking for testers) labels Oct 11, 2017

jasonblais added this to the v3.8.0 milestone Oct 11, 2017

yuya-oc suggested changes Oct 11, 2017

View reviewed changes

dmeza force-pushed the protocol_deep-linking branch from ee59c35 to ba6e8c1 Compare October 11, 2017 18:11

dmeza force-pushed the protocol_deep-linking branch 3 times, most recently from 75ad1bf to faf649c Compare October 14, 2017 04:48

yuya-oc suggested changes Oct 14, 2017

View reviewed changes

yuya-oc removed the MacOS (looking for testers) label Oct 14, 2017

This was referenced Oct 17, 2017

Cannot find a permalink #622

Closed

Support protocol deep linking on Linux #627

Closed

Add a server landing page when deep linking to an app when it's not yet installed #628

Closed

jasonblais removed the Windows (looking for testers) label Oct 17, 2017

jasonblais requested a review from MusikPolice October 20, 2017 15:02

jasonblais approved these changes Oct 20, 2017

View reviewed changes

MusikPolice approved these changes Oct 20, 2017

View reviewed changes

David Meza added 6 commits October 23, 2017 18:55

Protocol handler for deep linking in desktop app

40892e0

Make deepliking work on windows

4aa33a1

Logic for deep linking to go to the right tab and channel based on do…

150b829

…main

Logic to take value of protocol from electron-builder.json to be able…

7a3552d

… to whitelabel in an easy manner.

Refactorings to make code cleaner. Fixes to not execute anything when…

c624a9a

… windows squirrel installer.

Fix problem with deeplinkingUrl still being set when preferences wind…

ce08b73

…ow closes.

dmeza force-pushed the protocol_deep-linking branch from 9170f6c to ce08b73 Compare October 24, 2017 00:02

yuya-oc added 2 commits October 24, 2017 21:09

Remove a global variable, deeplinkingUrl

ea746ba

Disable deeplinking when the app is running in development mode

e24cddc

In that case, electron.exe is registered as the protocol client. The app would not work because app dir is not set when launching.

yuya-oc approved these changes Oct 24, 2017

View reviewed changes

Update CHANGELOG.md for deeplinking

7440986

yuya-oc merged commit a47dabe into mattermost:master Oct 24, 2017

dmeza reviewed Oct 24, 2017

View reviewed changes

jasonblais mentioned this pull request Feb 26, 2019

enable deep linking for linux #937

Closed

3 tasks

Protocol deep linking #616

Protocol deep linking #616

Conversation

csduarte commented Oct 10, 2017 • edited by yuya-oc Loading

jasonblais left a comment • edited Loading

Choose a reason for hiding this comment

yuya-oc commented Oct 11, 2017

yuya-oc commented Oct 11, 2017

yuya-oc left a 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

jasonblais commented Oct 11, 2017

jasonblais commented Oct 11, 2017

dmeza commented Oct 11, 2017

dmeza commented Oct 11, 2017

jasonblais commented Oct 11, 2017

yuya-oc commented Oct 12, 2017

yuya-oc commented Oct 12, 2017

dmeza commented Oct 14, 2017

yuya-oc commented Oct 14, 2017

yuya-oc left a 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

Choose a reason for hiding this comment

yuya-oc Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

jasonblais commented Oct 17, 2017

dmeza commented Oct 19, 2017 • edited Loading

jasonblais commented Oct 20, 2017

dmeza commented Oct 20, 2017

jasonblais commented Oct 20, 2017

dmeza commented Oct 20, 2017

yuya-oc commented Oct 21, 2017 • edited Loading

dmeza commented Oct 24, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

yuya-oc commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmeza commented Oct 24, 2017

yuya-oc commented Oct 24, 2017

yuya-oc commented Oct 25, 2017

csduarte commented Oct 10, 2017 •

edited by yuya-oc

Loading

jasonblais left a comment •

edited

Loading

yuya-oc Oct 14, 2017 •

edited

Loading

dmeza commented Oct 19, 2017 •

edited

Loading

yuya-oc commented Oct 21, 2017 •

edited

Loading