Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pass user session information when opening the electron app from web platform #16043
Pass user session information when opening the electron app from web platform #16043
Changes from all commits
eb90123
21781a5
1b894bd
5bf275b
6abae93
19907b1
03e359c
c34eaff
62ff96e
da8cf9c
8f87e06
a249bb7
3035261
42222cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 add a code comment to explain what this is used for, or update the name of the property to
this.isBrowserWindowFocused
.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.
App/src/components/DeeplinkWrapper/index.website.js
Line 39 in ae5ecb1
It comes from the original
focus
variable. I think we can safely remove it now, after the redirection screen is removed, I think we don't need it anymore.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.
Promise chains should be EXPLICITLY avoided in code. We did a ton of work to remove all the API chaining that we should not see these reintroduced back into the code (and if
Authentication
would have been an action file as opposed to a lib file, ESLint would have thrown an error).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.
Thank you for pointing this out. It has been removed.
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 is there a
setTimeout()
here? I see this is moving some of the original code, but we should NEVER EVER have asetTimeout()
in the code without a thorough code comment explaining it's necessity.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.
Yeah. I guess it was originally intended to check the focus with a delay. But it's not needed now since the redirection screen has been removed.
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.
It appears to me that these values aren't used anywhere? The only thing I see looking at this value is in the render method and it's only looking for the value of
CHECKING
.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.
Yeah, it's not needed now since the redirection screen has been removed, and we can replace
appInstallationCheckStatus
withisShortLivedAuthTokenLoading
, the latter is clearer.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 is
Network.post()
used here? This is considered a deprecated method (and sorry if it was never marked as such, but we've been trying to remove this for the past year).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 also used to think that using the onyx to retrieve short live token would be a little weird, but based on your discussions, this should no longer be an issue. And if we can create the
BeginDeepLinkRedirect
new command, then we can also no longer callmakeRequestWithSideEffects
.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.
What does this failure mean? It looks like it sets a state property in the deeplink component that either indicates that the desktop app is or isn't installed (yet, the state property is never checked for that).
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.
Yep, it's no longer needed either. We can remove it after we replace the
appInstallationCheckStatus
withisShortLivedAuthTokenLoading
.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.
This looks like it's only used for showing a loading indicator in
LogInWithShortLivedAuthTokenPage
and has nothing to do with a token being valid or invalid. Please change the name toIS_TOKEN_LOADING
. However, this is alreadyONYXKEYS.ACCOUNT.isLoading
so why is there a need for both?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 saw
ONYXKEYS.ACCOUNT.isLoading
being used in the login form, not sure if it can be used in other scenarios.And replace
IS_TOKEN_VALID
withONYXKEYS.ACCOUNT.isLoading
in the new followup PR.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.
It seems like this might have something to do with one of the above scenarios, but it is not clear what. Can you please add a code comment to explain why this is here and why there is no way that this can be done optimistically before the response comes?
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.
For example, if a request fails due to a sudden disconnection, we need to redirect the user to the login page instead of leaving them stuck on the transition page. I tried using
network.isOffline
to handle this situation in the new PR and left a todo comment for review.