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: display builtin apps on workspaces table#17695

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

Merged
BrunoQuaresma merged 4 commits intomainfrombq/refact-actions
May 7, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

Related to#17311

Screenshot 2025-05-06 at 16 20 40

@BrunoQuaresmaBrunoQuaresma requested a review froma teamMay 6, 2025 19:22
@BrunoQuaresmaBrunoQuaresma self-assigned thisMay 6, 2025
@BrunoQuaresmaBrunoQuaresma requested review frombrettkolodny and removed request fora teamMay 6, 2025 19:22
export const openAppInNewWindow = (name: string, href: string) => {
window.open(
href,
`${name} - ${generateRandomString(12)}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

What does setting the target like this do? TheMDN docs make it out like this is only useful if you wanted to target the new context with an anchor or form. More over it actually says that whitespace isn't allowed. Would one of the built intarget keywords be better for this?

BrunoQuaresma reacted with thumbs up emoji
Comment on lines -560 to -571
{abilities.actions.includes("stop") && (
<PrimaryAction
onClick={() => {
stopWorkspaceMutation.mutate({});
}}
isLoading={stopWorkspaceMutation.isLoading}
label="Stop workspace"
>
<SquareIcon />
</PrimaryAction>
)}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not want users to be able to stop workspaces from the table view anymore? I see in the designs there is a dropdown menu that will have this. Is that going to be a fast follow to this PR?

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yes, the "more options" menu will be implemented in a different PR.

brettkolodny reacted with thumbs up emoji
Comment on lines 122 to 130
consthref =getVSCodeHref("vscode",{
owner: userName,
workspace: workspaceName,
url: location.origin,
token: key,
openRecent: "true",
agent: agentName,
folder: folderPath,
});
if (agentName) {
query.set("agent", agentName);
}
if (folderPath) {
query.set("folder", folderPath);
}

location.href =`vscode://coder.coder-remote/open?${query.toString()}`;
location.href =href;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

location.href=getVSCodeHref("vscode",{owner:userName,workspace:workspaceName,token:key,agent:agentName,folder:folderPath,});

BrunoQuaresma reacted with thumbs up emoji
Comment on lines 645 to 655
const resource = workspace.latest_build.resources
.filter((r) => !r.hide)
.at(0);
if (!resource) {
return null;
}

const agent = resource.agents?.at(0);
if (!agent) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These two could be combined into one variable and one check

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yeap, but I think this way the code make it more explicit that we are looking for the first non-hide resource and after, the first agent. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I don't think either are particularly readable 😅 . I think my preference would be the one variable with a comment above explaining. I think a comment would be especially if it includes this info from your comment below:

Coder is pretty flexible and allows an enormous variety of use cases, such as having multiple resources with many agents, but they are not common. From my experience dealing with customers, the most common scenario is to have one single compute resource with one single agent containing all the apps, but Idk this in numbers. My idea is to test this as it is, getting the apps for the first resource, and first agent - they are sorted to return the compute resource first - and see what customers and ourselves, using dogfood, think about that.

BrunoQuaresma reacted with thumbs up emoji
};

const WorkspaceApps: FC<WorkspaceAppsProps> = ({ workspace }) => {
const buttons: ReactNode[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you want to use this variable I think it'd be better to have it closer to where it's actually used

BrunoQuaresma reacted with thumbs up emoji
return null;
}

const agent = resource.agents?.at(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably out of the scope of this PR but can a workspace have more than one resource that can then also have more one agent? Do we need to check all the resources and all the agents? Right now we're only checking if the first resource's first agent has one these apps, do we need to check if all of them do? Something like:

constagents=workspace.latest_build.resources.filter((r)=>!r.hide).flatMap((resource)=>resource.agents??[]);if(agents.length===0){returnnull;}constdisplayApps=newSet(agents.flatMap((agent)=>agent.display_apps))...

But if this is the case a lot of the other code would need to be re-thought too

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Probably out of the scope of this PR but can a workspace have more than one resource that can then also have more one agent?

Yes.

Do we need to check all the resources and all the agents?

Nops.

Right now we're only checking if the first resource's first agent has one these apps, do we need to check if all of them do?

Not right now.

For each agent we have two type of apps, built in apps such as VSCode, VSCode insiders, and terminal, and we also have the apps defined by the user (it is coming in a next PR). If we want to show the apps from all the agents, we would need to include all of them, which could make the UI a bit confusing like having two or three terminal buttons.

Coder is pretty flexible and allows an enormous variety of use cases, such as having multiple resources with many agents, but they are not common. From my experience dealing with customers, the most common scenario is to have one single compute resource with one single agent containing all the apps, but Idk this in numbers. My idea is to test this as it is, getting the apps for the first resource, and first agent - they are sorted to return the compute resource first - and see what customers and ourselves, using dogfood, think about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood! Left a comment above that I think this would be useful info to document into the code

Copy link
Contributor

@brettkolodnybrettkolodny left a comment

Choose a reason for hiding this comment

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

A couple of nits/thoughts left but up to you. Otherwise LGTM!

BrunoQuaresma reacted with heart emoji
@BrunoQuaresmaBrunoQuaresma merged commit6ac1bd8 intomainMay 7, 2025
29 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/refact-actions branchMay 7, 2025 17:26
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 7, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@brettkolodnybrettkolodnybrettkolodny approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@BrunoQuaresma@brettkolodny

[8]ページ先頭

©2009-2025 Movatter.jp