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

Desktop deeplink #13606

Merged
merged 19 commits into from
Jan 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions assets/images/expensify-wordmark.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
286 changes: 286 additions & 0 deletions assets/images/product-illustrations/rocket--blue.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions config/electronBuilder.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,8 @@ module.exports = {
app: 'desktop',
output: 'desktop-build',
},
protocols: {
name: 'New Expensify',
schemes: ['new-expensify'],
},
};
44 changes: 38 additions & 6 deletions desktop/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ const localizeMenuItems = (browserWindow, systemMenu) => {
};

const mainWindow = (() => {
let deeplinkUrl;
let browserWindow;

const loadURL = __DEV__
? win => win.loadURL(`http://localhost:${port}`)
: serve({directory: `${__dirname}/www`});
Expand All @@ -238,6 +241,19 @@ const mainWindow = (() => {
app.setName('New Expensify');
}

app.on('will-finish-launching', () => {
app.on('open-url', (event, url) => {
event.preventDefault();
const urlObject = new URL(url);
deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}`;

if (browserWindow) {
browserWindow.loadURL(deeplinkUrl);
browserWindow.show();
}
});
});

/*
* Starting from Electron 20, it shall be required to set sandbox option to false explicitly.
* Running a preload script contextBridge.js require access to nodeJS modules from the javascript code.
Expand All @@ -246,7 +262,15 @@ const mainWindow = (() => {
* */
return app.whenReady()
.then(() => {
const browserWindow = new BrowserWindow({
/**
* We only want to register the scheme this way when in dev, since
* when the app is bundled electron-builder will take care of it.
*/
if (__DEV__) {
app.setAsDefaultProtocolClient('new-expensify');
}

browserWindow = new BrowserWindow({
backgroundColor: '#FAFAFA',
width: 1200,
height: 900,
Expand Down Expand Up @@ -455,18 +479,26 @@ const mainWindow = (() => {
})

// After initializing and configuring the browser window, load the compiled JavaScript
.then((browserWindow) => {
loadURL(browserWindow);
return browserWindow;
.then((browserWindowRef) => {
loadURL(browserWindow).then(() => {
if (!deeplinkUrl) {
return;
}

browserWindow.loadURL(deeplinkUrl);
browserWindow.show();
});

return browserWindowRef;
})

// Start checking for JS updates
.then((browserWindow) => {
.then((browserWindowRef) => {
if (__DEV__) {
return;
}

checkForUpdates(electronUpdater(browserWindow));
checkForUpdates(electronUpdater(browserWindowRef));
});
});

Expand Down
5 changes: 5 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ const CONST = {
AU: 'AU',
CA: 'CA',
},
DESKTOP_DEEPLINK_APP_STATE: {
CHECKING: 'checking',
INSTALLED: 'installed',
NOT_INSTALLED: 'not-installed',
},
PLATFORM: {
IOS: 'ios',
ANDROID: 'android',
Expand Down
5 changes: 3 additions & 2 deletions src/Expensify.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import withLocalize, {withLocalizePropTypes} from './components/withLocalize';
import * as User from './libs/actions/User';
import NetworkConnection from './libs/NetworkConnection';
import Navigation from './libs/Navigation/Navigation';
import DeeplinkWrapper from './components/DeeplinkWrapper';

// This lib needs to be imported, but it has nothing to export since all it contains is an Onyx connection
// eslint-disable-next-line no-unused-vars
Expand Down Expand Up @@ -189,7 +190,7 @@ class Expensify extends PureComponent {
}

return (
<>
<DeeplinkWrapper>
{!this.state.isSplashShown && (
<>
<GrowlNotification ref={Growl.growlRef} />
Expand All @@ -213,7 +214,7 @@ class Expensify extends PureComponent {
onReady={this.setNavigationReady}
authenticated={this.isAuthenticated()}
/>
</>
</DeeplinkWrapper>
);
}
}
Expand Down
46 changes: 46 additions & 0 deletions src/components/DeeplinkWrapper/deeplinkRoutes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import ROUTES from '../../ROUTES';

/** @type {Array<object>} Routes regex used for desktop deeplinking */
export default [
{
// /reports/*
pattern: `/${ROUTES.REPORT}($|(//*))`,
},
{
// /settings/*
pattern: `/${ROUTES.SETTINGS}($|(//*))`,
},
Comment on lines +4 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cowboycito

Wondering if can we construct this from the apple-site association file, instead of hard coding here as it may come in handy in the future.

cc: @NikkiWines

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that sounds like a great call @Santhosh-Sellavel. Best to reduce duplication wherever possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried importing that file to use it as a reference to construct the deeplink routes. 2 issues came up: it's outside the src directory and the file doesn't have an extension (which causes for webpack to not know how to load the file). I honestly don't think it's worth to go on that route. Any thoughts? @Santhosh-Sellavel @NikkiWines

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure, I will wait for @NikkiWines

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah looking into it more I can't find a simple route to get this to work. In which case I think this is fine as it is now.

{
// /setpassword/*
pattern: '/setpassword($|(//*))',
},
{
// /details/*
pattern: `/${ROUTES.DETAILS}($|(//*))`,
},
{
// /v/*
pattern: '/v($|(//*))',
},
{
// /bank-account/*
pattern: `/${ROUTES.BANK_ACCOUNT}($|(//*))`,
},
{
// /iou/*
pattern: '/iou($|(//*))',
},
{
// /enable-payments/*
pattern: `/${ROUTES.ENABLE_PAYMENTS}($|(//*))`,
},
{
// /statements/*
pattern: '/statements($|(//*))',
},
{
// /concierge/*
pattern: `/${ROUTES.CONCIERGE}($|(//*))`,
},
];

16 changes: 16 additions & 0 deletions src/components/DeeplinkWrapper/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import PropTypes from 'prop-types';
import {PureComponent} from 'react';

const propTypes = {
/** Children to render. */
children: PropTypes.node.isRequired,
};

class DeeplinkWrapper extends PureComponent {
render() {
return this.props.children;
}
}

DeeplinkWrapper.propTypes = propTypes;
export default DeeplinkWrapper;
158 changes: 158 additions & 0 deletions src/components/DeeplinkWrapper/index.website.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
import _ from 'underscore';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import React, {PureComponent} from 'react';
import deeplinkRoutes from './deeplinkRoutes';
import FullScreenLoadingIndicator from '../FullscreenLoadingIndicator';
import TextLink from '../TextLink';
import * as Illustrations from '../Icon/Illustrations';
import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import Text from '../Text';
import styles from '../../styles/styles';
import CONST from '../../CONST';
import CONFIG from '../../CONFIG';
import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';
import colors from '../../styles/colors';
import * as Browser from '../../libs/Browser';

const propTypes = {
/** Children to render. */
children: PropTypes.node.isRequired,

...withLocalizePropTypes,
};

class DeeplinkWrapper extends PureComponent {
constructor(props) {
super(props);

this.state = {
appInstallationCheckStatus: this.isMacOSWeb() ? CONST.DESKTOP_DEEPLINK_APP_STATE.CHECKING : CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED,
};
}

componentDidMount() {
if (!this.isMacOSWeb()) {
return;
}

let focused = true;

window.addEventListener('blur', () => {
focused = false;
});

NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
setTimeout(() => {
if (!focused) {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED});
} else {
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED});
}
}, 500);

// check if pathname matches with deeplink routes
const matchedRoute = _.find(deeplinkRoutes, (route) => {
const routeRegex = new RegExp(route.pattern);
return routeRegex.test(window.location.pathname);
});

if (matchedRoute) {
this.setState({deeplinkMatch: true});
this.openRouteInDesktopApp();
} else {
this.setState({deeplinkMatch: false});
}
}

openRouteInDesktopApp() {
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL);
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`;
Copy link
Member

Choose a reason for hiding this comment

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

We forgot to pass the query params here which caused a few URLs to break in our App.

Here is the regression issue #15059

cc: @Santhosh-Sellavel


// This check is necessary for Safari, otherwise, if the user
// does NOT have the Expensify desktop app installed, it's gonna
// show an error in the page saying that the address is invalid
Comment on lines +72 to +74
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel Jan 9, 2023

Choose a reason for hiding this comment

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

Can you explain this one?

I think we should open in the desktop app popup only if the Desktop app is available, other we should straight away load the web.

cc: @NikkiWines

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slack with Desktop app installed

Screen.Recording.2023-01-09.at.6.32.56.PM.mov

Slack without Desktop app

Screen.Recording.2023-01-09.at.6.33.23.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that this is Safari only thing. The videos you posted are in Chrome.

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel Jan 9, 2023

Choose a reason for hiding this comment

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

How does it work on safari for slack vs our app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works the same way. Slack doesn't automatically open their webapp like we're doing. They always display the "launching" screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only show the "launching" screen if the user has the app installed, otherwise if will open the webapp automatically. There's only that Safari bug that was previously discussed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I miss understood the comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries

if (CONST.BROWSER.SAFARI === Browser.getBrowser()) {
const iframe = document.createElement('iframe');
iframe.style.display = 'none';
document.body.appendChild(iframe);
iframe.contentWindow.location.href = expensifyDeeplinkUrl;

// Since we're creating an iframe for Safari to handle
// deeplink we need to give this iframe some time for
// it to do what it needs to do. After that we can just
// remove the iframe.
setTimeout(() => {
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
if (!iframe.parentNode) {
return;
}

iframe.parentNode.removeChild(iframe);
}, 100);
} else {
window.location.href = expensifyDeeplinkUrl;
}
}

isMacOSWeb() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we put this in the component instead of the Browser lib?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No specific reasons we just missed out on that Review.

return !Browser.isMobile() && (
typeof navigator === 'object'
&& typeof navigator.userAgent === 'string'
&& /Mac/i.test(navigator.userAgent)
&& !/Electron/i.test(navigator.userAgent)
);
}

render() {
if (this.state.appInstallationCheckStatus === CONST.DESKTOP_DEEPLINK_APP_STATE.CHECKING) {
return <FullScreenLoadingIndicator style={styles.flex1} />;
}

if (
this.state.deeplinkMatch
&& this.state.appInstallationCheckStatus === CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED
) {
return (
<View style={styles.deeplinkWrapperContainer}>
<View style={styles.deeplinkWrapperMessage}>
<View style={styles.mb2}>
<Icon
width={200}
height={164}
src={Illustrations.RocketBlue}
/>
</View>
<Text style={[styles.textHeadline, styles.textXXLarge]}>
{this.props.translate('deeplinkWrapper.launching')}
</Text>
<View style={styles.mt2}>
<Text style={[styles.fontSizeNormal, styles.textAlignCenter]}>
{this.props.translate('deeplinkWrapper.redirectedToDesktopApp')}
{'\n'}
{this.props.translate('deeplinkWrapper.youCanAlso')}
{' '}
<TextLink onPress={() => this.setState({deeplinkMatch: false})}>
{this.props.translate('deeplinkWrapper.openLinkInBrowser')}
</TextLink>
.
</Text>
</View>
</View>
<View style={styles.deeplinkWrapperFooter}>
<Icon
width={154}
height={34}
fill={colors.green}
src={Expensicons.ExpensifyWordmark}
/>
</View>
</View>
);
}

return this.props.children;
}
}

DeeplinkWrapper.propTypes = propTypes;
export default withLocalize(DeeplinkWrapper);
2 changes: 2 additions & 0 deletions src/components/Icon/Expensicons.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import Emoji from '../../../assets/images/emoji.svg';
import Exclamation from '../../../assets/images/exclamation.svg';
import Exit from '../../../assets/images/exit.svg';
import ExpensifyCard from '../../../assets/images/expensifycard.svg';
import ExpensifyWordmark from '../../../assets/images/expensify-wordmark.svg';
import Expand from '../../../assets/images/expand.svg';
import Eye from '../../../assets/images/eye.svg';
import EyeDisabled from '../../../assets/images/eye-disabled.svg';
Expand Down Expand Up @@ -136,6 +137,7 @@ export {
Exclamation,
Exit,
ExpensifyCard,
ExpensifyWordmark,
Expand,
Eye,
EyeDisabled,
Expand Down
2 changes: 2 additions & 0 deletions src/components/Icon/Illustrations.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import MoneyEnvelopeBlue from '../../../assets/images/product-illustrations/mone
import MoneyMousePink from '../../../assets/images/product-illustrations/money-mouse--pink.svg';
import ReceiptsSearchYellow from '../../../assets/images/product-illustrations/receipts-search--yellow.svg';
import ReceiptYellow from '../../../assets/images/product-illustrations/receipt--yellow.svg';
import RocketBlue from '../../../assets/images/product-illustrations/rocket--blue.svg';
import RocketOrange from '../../../assets/images/product-illustrations/rocket--orange.svg';
import TadaYellow from '../../../assets/images/product-illustrations/tada--yellow.svg';
import TadaBlue from '../../../assets/images/product-illustrations/tada--blue.svg';
Expand Down Expand Up @@ -50,6 +51,7 @@ export {
MoneyMousePink,
ReceiptsSearchYellow,
ReceiptYellow,
RocketBlue,
RocketOrange,
TadaYellow,
TadaBlue,
Expand Down
Loading