- Notifications
You must be signed in to change notification settings - Fork909
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 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
| /> | ||
| </header> | ||
| <h4 className="m-0 text-xl font-semibold">Forwarded ports</h4> |
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.
What do you think about removing this?
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 personally like that. I would think on removing it later on when we have more things to display on the screen.
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.
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", |
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.
@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 🤔
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 think it's always going to be the same protocol as the Coder deployment?
If not, we can get that fromportAttributes:https://containers.dev/implementors/json_reference/#port-attributes
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 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Cian Johnston <cian@coder.com>
@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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| <section> | ||
| {containers.map((container) => { | ||
| return ( | ||
| <AgentDevcontainerCard | ||
| key={container.id} | ||
| container={container} | ||
| workspace={workspace} | ||
| host={proxy.preferredWildcardHostname || window.location.host} | ||
| agentName={agent.name} | ||
| /> | ||
| ); | ||
| })} | ||
| </section> |
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.
Should we skip showing a card for a stopped devcontainer? Or continue showing it but indicate it's stopped?
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 put some extra thoughts on this and I think we could just hide it for now since it would require design changes.
johnstcn commentedMar 4, 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.
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. |
@johnstcn I think what we have now is good to go and test on dev.coder.com 👍 |
| component={AgentButton} | ||
| underline="none" | ||
| startIcon={<ExternalLinkIcon className="size-icon-sm" />} | ||
| href={portForwardURL( |
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 think we should only showportForwardURL if wildcard hostname is set. Otherwise you get a URL likehttp://localhost:12345 which won't work.
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.
Fixed ✅
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@BrunoQuaresma !
861c4b1 intomainUh oh!
There was an error while loading.Please reload this page.
Related to#16422