-
Notifications
You must be signed in to change notification settings - Fork 3k
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
clean up the deep link code #17452
clean up the deep link code #17452
Changes from 2 commits
db996cb
cc48aaf
728cc18
f1f19a7
4945eb1
6dc06df
cff9643
3c73416
a129009
cb5f434
1f252d6
5f71937
a737600
c7b323d
c5ee140
d9a3cbc
5e2a6d9
54ee01c
0349229
d858603
65b0f7c
4105086
edbae30
637af52
bb0ddd0
8913f88
03d69bb
bdaf961
8758e31
144abda
9e7f595
fbdbb7d
96be874
ac8ad4e
f9f89da
e465908
92faead
5a66cbc
b0adfd3
3faec2b
4ed6c97
e195537
e31d0bd
7d70e7b
d5937cc
4a69d1e
be41317
57ef6bd
db7a9cb
0c87306
8c7c2c4
b6b672f
1298b0f
2bb7f31
61acb1b
80101fa
6d3bdce
bf37347
3120a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import CONST from '../../CONST'; | |
import CONFIG from '../../CONFIG'; | ||
import * as Browser from '../../libs/Browser'; | ||
import ONYXKEYS from '../../ONYXKEYS'; | ||
import * as Authentication from '../../libs/Authentication'; | ||
import * as Session from '../../libs/actions/Session'; | ||
|
||
const propTypes = { | ||
/** Children to render. */ | ||
|
@@ -21,13 +21,17 @@ const propTypes = { | |
|
||
/** Currently logged-in user authToken */ | ||
authToken: PropTypes.string, | ||
|
||
/** The short-lived auth token for navigating to desktop app */ | ||
shortLivedAuthToken: PropTypes.string, | ||
}), | ||
}; | ||
|
||
const defaultProps = { | ||
session: { | ||
email: '', | ||
authToken: '', | ||
shortLivedAuthToken: '', | ||
}, | ||
}; | ||
|
||
|
@@ -36,53 +40,48 @@ class DeeplinkWrapper extends PureComponent { | |
super(props); | ||
|
||
this.state = { | ||
appInstallationCheckStatus: (this.isMacOSWeb() && CONFIG.ENVIRONMENT !== CONST.ENVIRONMENT.DEV) | ||
? CONST.DESKTOP_DEEPLINK_APP_STATE.CHECKING : CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED, | ||
isShortLivedAuthTokenLoading: this.isMacOSWeb() && CONFIG.ENVIRONMENT !== CONST.ENVIRONMENT.DEV, | ||
}; | ||
this.focused = true; | ||
} | ||
|
||
componentDidMount() { | ||
if (!this.isMacOSWeb() || CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV) { | ||
return; | ||
} | ||
|
||
window.addEventListener('blur', () => { | ||
this.focused = false; | ||
}); | ||
// We need to clear the old short-lived auth token if it exists. | ||
Session.removeShortLivedAuthToken(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really get this. Why don't we just not use it when the component mounts? Why does it need to be "cleared out"? It will get replaced with a new token and we will only ever check it once because of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was based on the above thought. Now thinking back, clearing seems unnecessary. We can also modify the conditions in Index: src/components/DeeplinkWrapper/index.website.js
===================================================================
diff --git a/src/components/DeeplinkWrapper/index.website.js b/src/components/DeeplinkWrapper/index.website.js
--- a/src/components/DeeplinkWrapper/index.website.js (revision a73760026ada268563f485864b1d24fa18aa64ad)
+++ b/src/components/DeeplinkWrapper/index.website.js (date 1682668671513)
@@ -65,7 +65,7 @@
}
componentDidUpdate(prevProps) {
- if (prevProps.session.shortLivedAuthToken || !this.props.session.shortLivedAuthToken) {
+ if (!this.props.session.shortLivedAuthToken || this.props.session.shortLivedAuthToken === prevProps.session.shortLivedAuthToken) {
return;
} |
||
|
||
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL); | ||
const params = new URLSearchParams(); | ||
params.set('exitTo', `${window.location.pathname}${window.location.search}${window.location.hash}`); | ||
if (!this.props.session.authToken) { | ||
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`; | ||
this.openRouteInDesktopApp(expensifyDeeplinkUrl); | ||
this.openRouteInDesktopApp(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be higher up in the order of operations? I'm confused because it looks like we early return here if we don't have an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, If newDot is opened from oldDot, there is no if (!this.props.session.authToken) {
this.openRouteInDesktopApp();
return;
} This condition is only used when the user is indeed not logged in. And in this case, the user is also not logged in when opening the desktop app. |
||
return; | ||
} | ||
Authentication.getShortLivedAuthToken().then((shortLivedAuthToken) => { | ||
params.set('email', this.props.session.email); | ||
params.set('shortLivedAuthToken', `${shortLivedAuthToken}`); | ||
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`; | ||
this.openRouteInDesktopApp(expensifyDeeplinkUrl); | ||
}).catch(() => { | ||
// If the request is successful, we call the updateAppInstallationCheckStatus before the prompt pops up. | ||
// If not, we only need to make sure that the state will be updated. | ||
this.updateAppInstallationCheckStatus(); | ||
}); | ||
Session.getShortLivedAuthToken(); | ||
} | ||
|
||
updateAppInstallationCheckStatus() { | ||
setTimeout(() => { | ||
if (!this.focused) { | ||
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.INSTALLED}); | ||
} else { | ||
this.setState({appInstallationCheckStatus: CONST.DESKTOP_DEEPLINK_APP_STATE.NOT_INSTALLED}); | ||
} | ||
}, 500); | ||
componentDidUpdate(prevProps) { | ||
if (prevProps.session.shortLivedAuthToken || !this.props.session.shortLivedAuthToken) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we not handling the errors we were handling before anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the redirection UI has been removed in PR #16383, there would be no point in updating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remind me what errors we were handling and why they are not necessary anymore? I looked at the linked PR but I'm not seeing anything about errors or error handling (maybe I missed it). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Eh, Error handling was newly added by me in PR #16043. At that time, I just thought that due to the complexity of the network transmission, we could not guarantee that a request would always succeed. And If the request failed (e.g. due to timeouts), we needed to update |
||
return; | ||
} | ||
|
||
// Now that there is a shortLivedAuthToken, the route to the desktop app can be opened. | ||
this.openRouteInDesktopApp(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also please add a code comment here explaining something like "Now that there is a shortLivedAuthToken, the route to the desktop app can be opened"? |
||
} | ||
|
||
openRouteInDesktopApp(expensifyDeeplinkUrl) { | ||
this.updateAppInstallationCheckStatus(); | ||
openRouteInDesktopApp() { | ||
this.setState({ | ||
isShortLivedAuthTokenLoading: false, | ||
}); | ||
|
||
const params = new URLSearchParams(); | ||
params.set('exitTo', `${window.location.pathname}${window.location.search}${window.location.hash}`); | ||
const session = this.props.session; | ||
if (session.email && session.shortLivedAuthToken) { | ||
params.set('email', session.email); | ||
params.set('shortLivedAuthToken', session.shortLivedAuthToken); | ||
} | ||
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL); | ||
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`; | ||
|
||
// This check is necessary for Safari, otherwise, if the user | ||
// does NOT have the Expensify desktop app installed, it's gonna | ||
|
@@ -101,9 +100,8 @@ class DeeplinkWrapper extends PureComponent { | |
if (!iframe.parentNode) { | ||
return; | ||
} | ||
|
||
iframe.parentNode.removeChild(iframe); | ||
}, 100); | ||
}, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we actually need this
What does it mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know the original purpose either, but according to my tests, without this delay, the pop-up window can't appear. |
||
} else { | ||
window.location.href = expensifyDeeplinkUrl; | ||
} | ||
|
@@ -119,10 +117,9 @@ class DeeplinkWrapper extends PureComponent { | |
} | ||
|
||
render() { | ||
if (this.state.appInstallationCheckStatus === CONST.DESKTOP_DEEPLINK_APP_STATE.CHECKING) { | ||
if (this.state.isShortLivedAuthTokenLoading) { | ||
return <FullScreenLoadingIndicator style={styles.flex1} />; | ||
} | ||
|
||
return this.props.children; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,11 +241,6 @@ function signInWithShortLivedAuthToken(email, authToken) { | |
isLoading: true, | ||
}, | ||
}, | ||
{ | ||
onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
key: ONYXKEYS.IS_TOKEN_VALID, | ||
value: true, | ||
}, | ||
]; | ||
|
||
const successData = [ | ||
|
@@ -266,29 +261,13 @@ function signInWithShortLivedAuthToken(email, authToken) { | |
isLoading: false, | ||
}, | ||
}, | ||
{ | ||
onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
key: ONYXKEYS.IS_TOKEN_VALID, | ||
value: false, | ||
}, | ||
]; | ||
|
||
// If the user is signing in with a different account from the current app, should not pass the auto-generated login as it may be tied to the old account. | ||
// scene 1: the user is transitioning to newDot from a different account on oldDot. | ||
// scene 2: the user is transitioning to desktop app from a different account on web app. | ||
const oldPartnerUserID = credentials.login === email ? credentials.autoGeneratedLogin : ''; | ||
// eslint-disable-next-line rulesdir/no-api-side-effects-method | ||
API.makeRequestWithSideEffects( | ||
'SignInWithShortLivedAuthToken', | ||
{authToken, oldPartnerUserID}, | ||
{optimisticData, successData, failureData}, | ||
null, | ||
).then((response) => { | ||
if (response) { | ||
return; | ||
} | ||
Navigation.navigate(); | ||
}); | ||
API.read('SignInWithShortLivedAuthToken', {authToken, oldPartnerUserID, shouldRetry: false}, {optimisticData, successData, failureData}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need the |
||
} | ||
|
||
/** | ||
|
@@ -642,6 +621,23 @@ function authenticatePusher(socketID, channelName, callback) { | |
}); | ||
} | ||
|
||
function getShortLivedAuthToken() { | ||
// TODO: If we have any way to read the shortLivedAuthToken and save it, we can also no longer call makeRequestWithSideEffects. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what "and save it" implies. We are saving it in Onyx, but that isn't enough? Also, TODOs are not allowed in our code. Instead, you should open a GH issue and point to this line of code and then detail what needs to be done. |
||
// eslint-disable-next-line rulesdir/no-api-side-effects-method | ||
API.makeRequestWithSideEffects( | ||
'OpenOldDotLink', {shouldRetry: false}, {}, | ||
).then((response) => { | ||
const {shortLivedAuthToken} = response; | ||
Onyx.merge(ONYXKEYS.SESSION, {shortLivedAuthToken}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. original context: https://github.com/Expensify/Expensify/issues/218742 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it looks like this should just be an |
||
}); | ||
} | ||
|
||
function removeShortLivedAuthToken() { | ||
Onyx.merge(ONYXKEYS.SESSION, { | ||
shortLivedAuthToken: '', | ||
}); | ||
} | ||
|
||
export { | ||
beginSignIn, | ||
updatePasswordAndSignin, | ||
|
@@ -664,4 +660,6 @@ export { | |
reauthenticatePusher, | ||
invalidateCredentials, | ||
invalidateAuthToken, | ||
getShortLivedAuthToken, | ||
removeShortLivedAuthToken, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import withLocalize, {withLocalizePropTypes} from '../components/withLocalize'; | |
import compose from '../libs/compose'; | ||
import TextLink from '../components/TextLink'; | ||
import ONYXKEYS from '../ONYXKEYS'; | ||
import networkPropTypes from '../components/networkPropTypes'; | ||
|
||
const propTypes = { | ||
/** The parameters needed to authenticate with a short lived token are in the URL */ | ||
|
@@ -35,12 +36,23 @@ const propTypes = { | |
|
||
...withLocalizePropTypes, | ||
|
||
/** Whether the short lived auth token is valid */ | ||
isTokenValid: PropTypes.bool, | ||
/** The details about the account that the user is signing in with */ | ||
account: PropTypes.shape({ | ||
/** Whether a sign is loading */ | ||
isLoading: PropTypes.bool, | ||
}), | ||
|
||
/** Props to detect online status */ | ||
network: networkPropTypes, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like everywhere else in the app it uses |
||
}; | ||
|
||
const defaultProps = { | ||
isTokenValid: true, | ||
account: { | ||
isLoading: false, | ||
}, | ||
network: { | ||
isOffline: false, | ||
}, | ||
}; | ||
|
||
class LogInWithShortLivedAuthTokenPage extends Component { | ||
|
@@ -62,7 +74,12 @@ class LogInWithShortLivedAuthTokenPage extends Component { | |
} | ||
|
||
render() { | ||
if (this.props.isTokenValid) { | ||
// redirect the user to the login page if there is a sudden disconnection | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure what the exact edge case we're trying to handle is based on this comment. The user hits this page and in between downloading the JS bundle and executing it they somehow become offline? Can you expand on what exactly we are preventing and why we are doing this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eh, does the desktop app also need to download the js bundle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry I don't think I fully understood the previous comments. I don't think the desktop app needs to download the js bundle because it's an electron app and should be bundled with the app (IIRC).
If they are disconnected then how can they access the page at all 🤔
I think it's fine but the message about the session being expired seems wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that event if the user is disconnected the desktop app would still display the page, only the request
Or we can display an appropriate message for this case. |
||
if (this.props.network.isOffline) { | ||
Navigation.navigate(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be passing something more explicit here? Calling |
||
return; | ||
} | ||
if (this.props.account.isLoading) { | ||
return <FullScreenLoadingIndicator />; | ||
} | ||
return ( | ||
|
@@ -107,6 +124,7 @@ LogInWithShortLivedAuthTokenPage.defaultProps = defaultProps; | |
export default compose( | ||
withLocalize, | ||
withOnyx({ | ||
isTokenValid: {key: ONYXKEYS.IS_TOKEN_VALID}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, but what was it doing before and why are we removing it now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the user clicks |
||
account: {key: ONYXKEYS.ACCOUNT}, | ||
network: {key: ONYXKEYS.NETWORK}, | ||
}), | ||
)(LogInWithShortLivedAuthTokenPage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that the short-lived auth token doesn't exist, then get the new short-lived auto token from the server, and then the pop-up window can be opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the code comment to explain the "why" and not just "what" the code is doing. Why do we need to clear out the previous one before fetching a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgolen , sorry, my english is not good. I'm not sure how to describe it. Since we open the popup window based on whether there is an short-lived auth token. If we don't clear it first, the popup won't open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a perfect explanation 👍 Here is how I would write it: