Skip to content

feat: add devcontainer in the UI #16800

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

Merged
merged 10 commits into from
Mar 4, 2025
Merged

feat: add devcontainer in the UI #16800

merged 10 commits into from
Mar 4, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

image

Related to #16422

@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 4, 2025 13:53
@BrunoQuaresma BrunoQuaresma self-assigned this Mar 4, 2025
/>
</header>

<h4 className="m-0 text-xl font-semibold">Forwarded ports</h4>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about removing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like that. I would think on removing it later on when we have more things to display on the screen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be a little smaller? This is just a nit though, and I'm totally fine with leaving it as-is.

agentName,
workspace.name,
workspace.owner_name,
location.protocol === "https" ? "https" : "http",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnstcn should we return the port protocol in the response? Here, I'm just using whatever the user is currently using in the UI but maybe, some apps use HTTP instead of HTTPS... I'm not sure if it makes sense tho 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's always going to be the same protocol as the Coder deployment?

If not, we can get that from portAttributes: https://containers.dev/implementors/json_reference/#port-attributes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's always going to be the same protocol as the Coder deployment?

I'm not sure but would be safest to rely in the devcontainer config.

Co-authored-by: Cian Johnston <cian@coder.com>
@BrunoQuaresma BrunoQuaresma changed the title feat: display devcontainer in the UI feat: add devcontainer in the UI Mar 4, 2025
@BrunoQuaresma
Copy link
Collaborator Author

@johnstcn I would really appreciate if you could QA those changes locally and see if they are working nicely. My main concerns are the terminal and "port forwarded" links.

@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 4, 2025 14:01
@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 4, 2025 14:52
Comment on lines 285 to 297
<section>
{containers.map((container) => {
return (
<AgentDevcontainerCard
key={container.id}
container={container}
workspace={workspace}
host={proxy.preferredWildcardHostname || window.location.host}
agentName={agent.name}
/>
);
})}
</section>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we skip showing a card for a stopped devcontainer? Or continue showing it but indicate it's stopped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put some extra thoughts on this and I think we could just hide it for now since it would require design changes.

@johnstcn
Copy link
Member

johnstcn commented Mar 4, 2025

@johnstcn I would really appreciate if you could QA those changes locally and see if they are working nicely. My main concerns are the terminal and "port forwarded" links.

Web terminal link is working fine locally! There should be no changes in behaviour required for the "forwarded port" links. They work similarly to the "listening ports" feature.

@BrunoQuaresma
Copy link
Collaborator Author

@johnstcn I think what we have now is good to go and test on dev.coder.com 👍

@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 4, 2025 17:00
component={AgentButton}
underline="none"
startIcon={<ExternalLinkIcon className="size-icon-sm" />}
href={portForwardURL(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only show portForwardURL if wildcard hostname is set. Otherwise you get a URL like http://localhost:12345 which won't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed ✅

@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 4, 2025 17:19
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BrunoQuaresma !

@BrunoQuaresma BrunoQuaresma merged commit 861c4b1 into main Mar 4, 2025
30 checks passed
@BrunoQuaresma BrunoQuaresma deleted the bq/devcontainer branch March 4, 2025 17:29
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants