-
Notifications
You must be signed in to change notification settings - Fork 424
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
fix(auth): wait to display Q sign-in notification #5632
base: master
Are you sure you want to change the base?
Conversation
This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required. |
f4cd1bb
to
d387b10
Compare
(Super low risk and effort fix, but may not address the root issue due to difficulties to repro) Problem: The sign-in notification is queued to display immediately after startup. Some users are reporting that it displays even if they are signed in. This may be due to the tryAutoConnection routine not having a chance to complete because of its async nature. Solution: Put a delay on displaying the notification to users to potentially mitigate this issue.
d387b10
to
d2bf537
Compare
|
||
// Display notification to sign if the user hasn't yet. | ||
// Abitrary wait time to give the auth restore routine time to initialize. | ||
globals.clock.setTimeout(() => void setupAuthNotification(), 5000) |
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.
AuthUtil emits an onDidChangeActiveConnection event, is it possible we can wait for that to trigger and then we call setupAuthNotification?
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.
Does it emit if there is no connection? That's when we would display the notification
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.
Oh true. Would it make sense to emit an event then when the restore logic is done? I'd think we'd want to avoid a using arbitrary timeout in general.
I see Justin's comment below and if we cannot add it to the tryAutoConnect due to the calls being disconnected or something, the event emitter method may be useful.
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.
replied to Justin; relevant to this chain.
Can we make the "sign in notification" the responsibility of |
This method is resilient against other (un)expected auth behavior, not just |
(Super low risk and effort fix, but may not address the root issue due to difficulties to repro)
Problem
The sign-in notification is queued to display immediately after startup. Some users are reporting that it displays even if they are signed in. This may be due to the tryAutoConnection routine not having a chance to complete because of its async nature.
Solution
Put a delay on displaying the notification to users to potentially mitigate this issue.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.