Skip to content

fix(site): fix resource selection when workspace has no prev resources #11594

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.

Sign up for GitHub

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 1 commit into from
Jan 12, 2024
+66 −47
Merged

fix(site): fix resource selection when workspace has no prev resources #11594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions site/src/pages/WorkspacePage/Workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { SidebarIconButton } from "components/FullPageLayout/Sidebar";
import HubOutlined from "@mui/icons-material/HubOutlined";
import { ResourcesSidebar } from "./ResourcesSidebar";
import { ResourceCard } from "components/Resources/ResourceCard";
import { useResourcesNav } from "./useResourcesNav";
import { resourceOptionValue, useResourcesNav } from "./useResourcesNav";
import { MemoizedInlineMarkdown } from "components/Markdown/Markdown";

export type WorkspaceError =
Expand Down Expand Up @@ -163,6 +163,9 @@ export const Workspace: FC<WorkspaceProps> = ({
(a, b) => countAgents(b) - countAgents(a),
);
const resourcesNav = useResourcesNav(resources);
const selectedResource = resources.find(
(r) => resourceOptionValue(r) === resourcesNav.value,
);

return (
<div
Expand Down Expand Up @@ -377,9 +380,9 @@ export const Workspace: FC<WorkspaceProps> = ({

{buildLogs}

{resourcesNav.selected && (
{selectedResource && (
<ResourceCard
resource={resourcesNav.selected}
resource={selectedResource}
agentRow={(agent) => (
<AgentRow
key={agent.id}
Expand Down
76 changes: 47 additions & 29 deletions site/src/pages/WorkspacePage/useResourcesNav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { renderHook } from "@testing-library/react";
import { resourceOptionId, useResourcesNav } from "./useResourcesNav";
import { resourceOptionValue, useResourcesNav } from "./useResourcesNav";
import { WorkspaceResource } from "api/typesGenerated";
import { MockWorkspaceResource } from "testHelpers/entities";
import { RouterProvider, createMemoryRouter } from "react-router-dom";
Expand All @@ -20,27 +20,9 @@ describe("useResourcesNav", () => {
/>
),
});
expect(result.current.selected?.id).toBe(MockWorkspaceResource.id);
});

it("selects the first resource if it has agents and selected resource is not find", async () => {
const resources: WorkspaceResource[] = [
MockWorkspaceResource,
{
...MockWorkspaceResource,
agents: [],
},
];
const { result } = renderHook(() => useResourcesNav(resources), {
wrapper: ({ children }) => (
<RouterProvider
router={createMemoryRouter([{ path: "/", element: children }], {
initialEntries: ["/?resources=not_found_resource_id"],
})}
/>
),
});
expect(result.current.selected?.id).toBe(MockWorkspaceResource.id);
expect(result.current.value).toBe(
resourceOptionValue(MockWorkspaceResource),
);
});

it("selects the resource passed in the URL", () => {
Expand All @@ -66,12 +48,14 @@ describe("useResourcesNav", () => {
wrapper: ({ children }) => (
<RouterProvider
router={createMemoryRouter([{ path: "/", element: children }], {
initialEntries: [`/?resources=${resourceOptionId(resources[1])}`],
initialEntries: [
`/?resources=${resourceOptionValue(resources[1])}`,
],
})}
/>
),
});
expect(result.current.selected?.id).toBe(resources[1].id);
expect(result.current.value).toBe(resourceOptionValue(resources[1]));
});

it("selects a resource when resources are updated", () => {
Expand Down Expand Up @@ -104,7 +88,7 @@ describe("useResourcesNav", () => {
initialProps: { resources: startedResources },
},
);
expect(result.current.selected?.id).toBe(startedResources[0].id);
expect(result.current.value).toBe(resourceOptionValue(startedResources[0]));

// When a workspace is stopped, there are no resources with agents, so we
// need to retain the currently selected resource. This ensures consistency
Expand All @@ -121,12 +105,46 @@ describe("useResourcesNav", () => {
},
];
rerender({ resources: stoppedResources });
expect(result.current.selectedValue).toBe(
resourceOptionId(startedResources[0]),
);
expect(result.current.value).toBe(resourceOptionValue(startedResources[0]));

// When a workspace is started again a resource is selected
rerender({ resources: startedResources });
expect(result.current.selected?.id).toBe(startedResources[0].id);
expect(result.current.value).toBe(resourceOptionValue(startedResources[0]));
});

// This happens when a new workspace is created and there are no resources
it("selects a resource when resources are not defined previously", () => {
const startingResources: WorkspaceResource[] = [];
const { result, rerender } = renderHook(
({ resources }) => useResourcesNav(resources),
{
wrapper: ({ children }) => (
<RouterProvider
router={createMemoryRouter([{ path: "/", element: children }])}
/>
),
initialProps: { resources: startingResources },
},
);
const startedResources: WorkspaceResource[] = [
{
...MockWorkspaceResource,
type: "docker_container",
name: "coder_python",
},
{
...MockWorkspaceResource,
type: "docker_container",
name: "coder_java",
},
{
...MockWorkspaceResource,
type: "docker_image",
name: "coder_image_python",
agents: [],
},
];
rerender({ resources: startedResources });
expect(result.current.value).toBe(resourceOptionValue(startedResources[0]));
});
});
28 changes: 13 additions & 15 deletions site/src/pages/WorkspacePage/useResourcesNav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useTab } from "hooks";
import { useEffectEvent } from "hooks/hookPolyfills";
import { useCallback, useEffect } from "react";

export const resourceOptionId = (resource: WorkspaceResource) => {
export const resourceOptionValue = (resource: WorkspaceResource) => {
return `${resource.type}_${resource.name}`;
};

Expand All @@ -18,38 +18,36 @@ export const useResourcesNav = (resources: WorkspaceResource[]) => {

const isSelected = useCallback(
(resource: WorkspaceResource) => {
return resourceOptionId(resource) === resourcesNav.value;
return resourceOptionValue(resource) === resourcesNav.value;
},
[resourcesNav.value],
);

const selectedResource = resources.find(isSelected);
const onSelectedResourceChange = useEffectEvent(
(previousResource?: WorkspaceResource) => {
const onResourceChanges = useEffectEvent(
(resources?: WorkspaceResource[]) => {
const hasSelectedResource = resourcesNav.value !== "";
const hasResources = resources && resources.length > 0;
const hasResourcesWithAgents =
resources.length > 0 &&
resources[0].agents &&
resources[0].agents.length > 0;
if (!previousResource && hasResourcesWithAgents) {
resourcesNav.set(resourceOptionId(resources[0]));
hasResources && resources[0].agents && resources[0].agents.length > 0;
if (!hasSelectedResource && hasResourcesWithAgents) {
resourcesNav.set(resourceOptionValue(resources[0]));
}
},
);
useEffect(() => {
onSelectedResourceChange(selectedResource);
}, [onSelectedResourceChange, selectedResource]);
onResourceChanges(resources);
}, [onResourceChanges, resources]);
Copy link
Member

Choose a reason for hiding this comment

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

In practice, this might make the effect run a little more often, but I think the logic for the main function is set up good, so the change shouldn't really be noticeable


const select = useCallback(
(resource: WorkspaceResource) => {
resourcesNav.set(resourceOptionId(resource));
resourcesNav.set(resourceOptionValue(resource));
},
[resourcesNav],
);

return {
isSelected,
select,
selected: selectedResource,
selectedValue: resourcesNav.value,
value: resourcesNav.value,
};
};