-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[github] fix register app #4093
Conversation
pathname: '/authorize', | ||
search: `returnTo=${returnTo}&host=github.com&scopes=repo` | ||
}).toString(); | ||
window.open(url, "gitpod-login"); |
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 is left-over. the window.open
call is centralized in openAuthorizeWindow
.
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.
Thanks for the quick fix!
Code looks good to me, and I guess there is no convenient way to test it apart from gitpod-staging.com
@@ -16,13 +16,6 @@ async function registerApp(installationId: string, setModal: (modal: 'done' | st | |||
try { | |||
await getGitpodService().server.registerGithubApp(installationId); |
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.
Thought: If possible, every window.open()
call should always happen in the exact same event loop cycle as the click
event.
Otherwise the window.open()
will most likely be blocked by the browser's pop-up blocker. This means, no await
, no callbacks, etc.
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.
Can we change the order?
c422aaa
to
6ff039b
Compare
@@ -56,12 +56,8 @@ async function openAuthorizeWindow(params: OpenAuthorizeWindowParams) { | |||
search: `returnTo=${encodeURIComponent(returnTo)}&host=${host}&override=true&scopes=${(scopes || []).join(',')}` | |||
}).toString(); | |||
|
|||
const newWindow = window.open(url, "gitpod-auth-window"); | |||
if (!newWindow) { |
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.
reverting this to get to square one, where the untracked new window was opened just fine.
let's think this through one more time, next time 🙏🏻
fixes #4092