feat: allow magic string to generate session token for external apps #9878
Conversation
| if (app.external && !app.url.startsWith("http")) { | ||
| // If the protocol is external the browser does not | ||
| // redirect the user from the page. | ||
| window.location.href = href |
There was a problem hiding this comment.
I'd say we should just not prevent default and let the browser do it's thing
There was a problem hiding this comment.
also I feel like this should be an || not a && right? if it was labeled as external by the template, or it's not an http/https link, then we cannot open it as a new window
There was a problem hiding this comment.
The browser does the redirect but leaves an empty window artifact.
There was a problem hiding this comment.
An app can be external and not HTTP(s), in which case we'd want to open it in a new window I believe.
There was a problem hiding this comment.
Yes that's the current behavior. We are just adding an exception for non http ones.
Having a window or tab is another debate. But let's leave that for now.
There was a problem hiding this comment.
The browser does the redirect but leaves an empty window artifact.
oh because of the _blank, right? we could make that value depend on app.external to fix that.
There was a problem hiding this comment.
An app can be external and not HTTP(s), in which case we'd want to open it in a new window I believe.
but the new window will still be a browser, only capable of opening http and https links. I'd expect that anything that's not http should not be opened in a new window
There was a problem hiding this comment.
I think if it's not labeled as external then we'd technically want to proxy it through the workspace, which I agree is weird, but don't wanna have a shot at breaking anyone right now before launch.
| getApiKey() | ||
| .then((key) => { | ||
| const url = href.replaceAll( | ||
| magicTokenString, | ||
| key.key, | ||
| ); | ||
| window.location.href = url; | ||
| setFetchingSessionToken(false); | ||
| }) | ||
| .catch((ex) => { | ||
| console.error(ex); | ||
| setFetchingSessionToken(false); | ||
| }); |
There was a problem hiding this comment.
you could just make this event handler async and then await these
There was a problem hiding this comment.
I suppose that's the same as console.error really, will change
| ); | ||
| // This is an external URI like "vscode://", so | ||
| // it needs to be opened with the browser protocol handler. | ||
| if (app.external && !app.url.startsWith("http")) { |
There was a problem hiding this comment.
I'm still pretty convinced this should be an ||
There was a problem hiding this comment.
If app.external and the URL is https://my-internal-jira.com we wouldn't want to replace window.location.href, otherwise it would redirect the user from the dashboard.
There was a problem hiding this comment.
@aslilac lmk async if I'm thinking of this wrong, going to merge since it's blocking some registry work, but I'm happy to change it!
There was a problem hiding this comment.
OH. I think I misunderstood what external meant here. It's probably fine.
This will enable modules to replace built-in apps like VS Code Desktop by having long-lived user-scoped tokens dynamically.