- Notifications
You must be signed in to change notification settings - Fork905
chore: extract app access logic for reuse#17724
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kylecarbs left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Edit: This was AI-generated. Disregard!
| let href = `${path}/@${workspace.owner_name}/${workspace.name}.${ | ||
| agent.name | ||
| }/apps/${encodeURIComponent(app.slug)}/`; | ||
| if (app.command) { | ||
| // Terminal links are relative. The terminal page knows how | ||
| // to select the correct workspace proxy for the websocket | ||
| // connection. | ||
| href = `/@${workspace.owner_name}/${workspace.name}.${ | ||
| agent.name | ||
| }/terminal?command=${encodeURIComponent(app.command)}`; | ||
| } | ||
| if (host && app.subdomain && app.subdomain_name) { | ||
| const baseUrl = `${window.location.protocol}//${host.replace(/\*/g, app.subdomain_name)}`; | ||
| const url = new URL(baseUrl); | ||
| url.pathname = "/"; | ||
| href = url.toString(); | ||
| } | ||
| return href; |
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 know this was already like this, so no need to change, but it does seem like early returns could make more sense than alet, since we are not ever combining anything, just returning three separate hrefs.
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.
Going to refactor this in a next PR.
| if (isExternalApp(app)) { | ||
| return needsSessionToken(app) | ||
| ? app.url.replaceAll(SESSION_TOKEN_PLACEHOLDER, token ?? "") | ||
| : app.url; | ||
| } |
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.
Not a big deal so feel free to ignore but do we need to make all these checks again? It looks like we already checkisExternalApp(app) && needsSessionToken(app) inuseAppLink (which seems to be the only place we usegetAppHref) and will pass an empty token if the query is disabled.
IMO it would make more sense to dogetAppHref(...).replace(SESSION_TOKEN_PLACEHOLDER, apiKeyResponse?.key ?? "") inuseAppLink. Although if we are removing this later I suppose it does not matter anyway.
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.
Exactly, but I will take a second look into this.
| const { proxy } = useProxy(); | ||
| const { data: apiKeyResponse } = useQuery({ | ||
| ...apiKey(), | ||
| enabled: isExternalApp(app) && needsSessionToken(app), |
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.
Could beenabled: needsSessionToken(app), sinceneedsSessionToken already checksisExternalApp.
| host: proxy.preferredWildcardHostname, | ||
| }); | ||
| const onClick = (e: React.MouseEvent) => { |
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.
Very cool, I like that this just lets the native click through when we can now.
| @@ -49,6 +61,73 @@ export const getTerminalHref = ({ | |||
| }/terminal?${params}`; | |||
| }; | |||
| export const openAppInNewWindow = (name: string,href: string) => { | |||
| export const openAppInNewWindow = (href: string) => { | |||
| window.open(href, "_blank", "width=900,height=600"); | |||
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 looks like forslim-window apps we were adding the name before, now that we callopenAppInNewWindow for those, should we add the name?window.open(href, name, "width=900,height=600");
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.
The second arg was never used to be a name thohttps://developer.mozilla.org/en-US/docs/Web/API/Window/open but the target.
code-asherMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
True, but that link says that the target can be:
A string, without whitespace, specifying thename
And the link to name says:
The name of the window
| const href = getAppHref(app, { | ||
| agent, | ||
| workspace, | ||
| token: apiKeyResponse?.key ?? "", |
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.
Could betoken: apiKeyResponse?.key since it accepts undefined.
| app: WorkspaceApp, | ||
| { agent, workspace }: UseAppLinkParams, | ||
| ) => { | ||
| const label = app.display_name ?? app.slug; |
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 wasapp.display_name || app.slug before, just to be sure, couldapp.display_name ever be an empty string?
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.
Yes, it can be empty. I fixed that pretty recently in the coderdsdk api response.
f897981 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
We are starting to add app links in many places in the UI, and to make it consistent, this PR extracts the most core logic into the modules/apps for reuse.
Related to#17311