Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: support devcontainer agents in ui and unify backend#18332

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

Open
mafredri wants to merge24 commits intomain
base:main
Choose a base branch
Loading
frommafredri/feat-agent-devcontainer-injection-6

Conversation

mafredri
Copy link
Member

@mafredrimafredri commentedJun 11, 2025
edited
Loading

This PR adds consolidates two container endpoints on the backend and improves the frontend devcontainer support by showing names and displaying apps as appropriate.

With this change, the frontend now has knowledge of the subagent and we can also display things like port forwards.

The frontend was updated to show dev container labels on the border as well as subagent connection status. The recreation flow was also adjusted a bit to show placeholder app icons when relevant.

Support forapps was also added, although these are still WIP on the backend. And the port forwarding utility was added in since the sub agents now provide the necessary info.

Fixescoder/internal#666

image

@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch fromc9f4ca4 to18e1593CompareJune 13, 2025 17:35
@mafredrimafredri changed the titlefeat: expand devcontainer subagent support in ui and improve backendfeat: support devcontainer agents in ui and unify backendJun 13, 2025
@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch 2 times, most recently fromfc1a236 to414b6f4CompareJune 13, 2025 18:44
@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch from414b6f4 toecfe483CompareJune 13, 2025 19:06
@mafredrimafredri marked this pull request as ready for reviewJune 13, 2025 20:00
@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch from8f12d26 to79e1844CompareJune 13, 2025 20:08
Copy link
Contributor

@DanielleMaywoodDanielleMaywood left a comment

Choose a reason for hiding this comment

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

haven't managed to read all the Go yet but will read it by Monday

@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch from016b6b0 tod3bda05CompareJune 16, 2025 08:26
@mafredrimafredriforce-pushed themafredri/feat-agent-devcontainer-injection-6 branch fromabc9999 tod4f208bCompareJune 16, 2025 08:48
Comment on lines +1255 to +1263
for_,id:=rangesubAgentIDs {
err:=api.subAgentClient.Delete(deleteCtx,id)
iferr!=nil {
api.logger.Error(api.ctx,"delete subagent record during shutdown failed",
slog.Error(err),
slog.F("agent_id",id),
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: should we do this in parallel instead?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

What's the argument for doing it in parallel? My gut feeling is I want to avoid spamming coderd. If this is something we want, we should allow deleting multiple agents in one request instead. // cc@DanielleMaywood

Copy link
Member

@johnstcnjohnstcnJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

Main argument is to reduce time to shutdown. Currently it is worst case maximumlen(subAgentIDs) * deleteCtx timeout. Not a blocker though.

Copy link
MemberAuthor

@mafredrimafredriJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

Gotcha 👍🏻. It's btw just1 * deleteCtx timeout, we give up on deleting the rest if it takes too long.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. That may be another argument for doing multiple at once - I'd pefer it to be all or nothing.

mafredri reacted with thumbs up emoji
}

// From codersdk/workspaceagents.go
export interface WorkspaceAgentDevcontainerAgent {
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresmaJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

This name sounds confusing to me 🤔. From my understanding, we have two agents, WorkspaceAgent, and Devcontainer Agent, is that correct? If yes, why not call DevcontainerAgent to simplify the naming?

I think prefixing the resource names, when not necessary, a bad practice, but I see we do that in a lot of places in coderd, so it is not a blocker, but I would like to bring attention to it.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I do see your point. Technically I suppose "Workspace" in "WorkspaceAgent" is also somehwhat useless. The main idea with this is to follow existing naming conventions but also to symbolize the dependency tree. A devcontainer agent (a workspace agents subagent) cannot exist without a (parent) workspace agent. Hence it is nested in the naming. Another existing type likeWorkspaceAgentContainer also already exists so I don't think it's worth diverging right now without a plan for how to tackle existing names.

consthandleRebuildDevcontainer=async()=>{
setIsRebuilding(true);
setSubAgentRemoved(true);
letrebuildSucceeded=false;
try{
constresponse=awaitfetch(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the FE, we handle requests in two ways: queries or mutations. In this case, you should wrap this request into a mutation.Here is the docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When using a mutation, you can invalidate other queries or update their data to reflect the most recent status, instead of doing it manually, so you wouldn't need the isRebuilding or subAgentRemoved statuses.Here is how you can do it. If you need more examples, you can search in the codebase for "invalidateQueries".

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Left a few comments related to the FE codebase.

@johnstcn
Copy link
Member

I don't have any blocking comments, but deferring final approval to@DanielleMaywood

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma left review comments

@johnstcnjohnstcnjohnstcn left review comments

@DanielleMaywoodDanielleMaywoodAwaiting requested review from DanielleMaywood

Requested changes must be addressed to merge this pull request.

Assignees

@mafredrimafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ConsolidateGET /containers andGET /containers/devcontainers and render devcontainers (instead of containers) in the UI
4 participants
@mafredri@johnstcn@BrunoQuaresma@DanielleMaywood

[8]ページ先頭

©2009-2025 Movatter.jp