fix(auth): wait to display Q sign-in notification #5632
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. |
hayemaxi
force-pushed
the
signin-notify
branch
2 times, most recently
from
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.
hayemaxi
force-pushed
the
signin-notify
branch
from
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.
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.
Does it emit if there is no connection? That's when we would display the notification
There was a problem hiding this comment.
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.
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.