From 1222a24dbbd96a57bde28f98e58cdcf4d5695afb Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 11 Jan 2024 17:49:08 +0000 Subject: [PATCH 1/4] fix(site): fix resource selection when workspace resources change --- .../pages/WorkspacePage/ResourcesSidebar.tsx | 10 +- site/src/pages/WorkspacePage/Workspace.tsx | 19 +-- .../WorkspacePage/useResourcesNav.test.tsx | 125 ++++++++++++++++++ .../pages/WorkspacePage/useResourcesNav.ts | 44 ++++++ 4 files changed, 180 insertions(+), 18 deletions(-) create mode 100644 site/src/pages/WorkspacePage/useResourcesNav.test.tsx create mode 100644 site/src/pages/WorkspacePage/useResourcesNav.ts diff --git a/site/src/pages/WorkspacePage/ResourcesSidebar.tsx b/site/src/pages/WorkspacePage/ResourcesSidebar.tsx index f6354a6bb9ed9..4cf6d9c2ac971 100644 --- a/site/src/pages/WorkspacePage/ResourcesSidebar.tsx +++ b/site/src/pages/WorkspacePage/ResourcesSidebar.tsx @@ -12,13 +12,13 @@ import { getResourceIconPath } from "utils/workspace"; type ResourcesSidebarProps = { failed: boolean; resources: WorkspaceResource[]; - onChange: (resourceId: string) => void; - selected: string; + onChange: (resource: WorkspaceResource) => void; + isSelected: (resource: WorkspaceResource) => boolean; }; export const ResourcesSidebar = (props: ResourcesSidebarProps) => { const theme = useTheme(); - const { failed, onChange, selected, resources } = props; + const { failed, onChange, isSelected, resources } = props; return ( @@ -46,8 +46,8 @@ export const ResourcesSidebar = (props: ResourcesSidebarProps) => { ))} {resources.map((r) => ( onChange(r.id)} - isActive={r.id === selected} + onClick={() => onChange(r)} + isActive={isSelected(r)} key={r.id} css={styles.root} > diff --git a/site/src/pages/WorkspacePage/Workspace.tsx b/site/src/pages/WorkspacePage/Workspace.tsx index e19f485aa903a..af89717042f31 100644 --- a/site/src/pages/WorkspacePage/Workspace.tsx +++ b/site/src/pages/WorkspacePage/Workspace.tsx @@ -26,6 +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"; export type WorkspaceError = | "getBuildsError" @@ -155,18 +156,10 @@ export const Workspace: FC = ({ } }; - const selectedResourceId = useTab("resources", ""); const resources = [...workspace.latest_build.resources].sort( (a, b) => countAgents(b) - countAgents(a), ); - const selectedResource = workspace.latest_build.resources.find( - (r) => r.id === selectedResourceId.value, - ); - useEffect(() => { - if (resources.length > 0 && selectedResourceId.value === "") { - selectedResourceId.set(resources[0].id); - } - }, [resources, selectedResourceId]); + const resourcesNav = useResourcesNav(resources); return (
= ({ )} {sidebarOption.value === "history" && ( @@ -374,9 +367,9 @@ export const Workspace: FC = ({ {buildLogs} - {selectedResource && ( + {resourcesNav.selected && ( ( { + it("selects the first resource if it has agents and no resource is selected", () => { + const resources: WorkspaceResource[] = [ + MockWorkspaceResource, + { + ...MockWorkspaceResource, + agents: [], + }, + ]; + const { result } = renderHook(() => useResourcesNav(resources), { + wrapper: ({ children }) => ( + + ), + }); + 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 }) => ( + + ), + }); + expect(result.current.selected?.id).toBe(MockWorkspaceResource.id); + }); + + it("selects the resource passed in the URL", () => { + const resources: WorkspaceResource[] = [ + { + ...MockWorkspaceResource, + type: "docker_container", + name: "coder_python", + }, + { + ...MockWorkspaceResource, + type: "docker_container", + name: "coder_java", + }, + { + ...MockWorkspaceResource, + type: "docker_image", + name: "coder_image_python", + agents: [], + }, + ]; + const { result } = renderHook(() => useResourcesNav(resources), { + wrapper: ({ children }) => ( + + ), + }); + expect(result.current.selected?.id).toBe(resources[1].id); + }); + + it("selects a resource when resources are updated", () => { + 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: [], + }, + ]; + const { result, rerender } = renderHook( + ({ resources }) => useResourcesNav(resources), + { + wrapper: ({ children }) => ( + + ), + initialProps: { resources: startedResources }, + }, + ); + expect(result.current.selected?.id).toBe(startedResources[0].id); + + // When a workspace is stopped, there is no resource with agents + const stoppedResources: WorkspaceResource[] = [ + { + ...MockWorkspaceResource, + type: "docker_image", + name: "coder_image_python", + agents: [], + }, + ]; + rerender({ resources: stoppedResources }); + expect(result.current.selected).toBe(undefined); + + // When a workspace is started again a resource is selected + rerender({ resources: startedResources }); + expect(result.current.selected?.id).toBe(startedResources[0].id); + }); +}); diff --git a/site/src/pages/WorkspacePage/useResourcesNav.ts b/site/src/pages/WorkspacePage/useResourcesNav.ts new file mode 100644 index 0000000000000..f7b9d5530177a --- /dev/null +++ b/site/src/pages/WorkspacePage/useResourcesNav.ts @@ -0,0 +1,44 @@ +import { WorkspaceResource } from "api/typesGenerated"; +import { useTab } from "hooks"; +import { useCallback, useEffect } from "react"; + +export const resourceOptionId = (resource: WorkspaceResource) => { + return `${resource.type}_${resource.name}`; +}; + +export const useResourcesNav = (resources: WorkspaceResource[]) => { + const resourcesNav = useTab("resources", ""); + const selectedResource = resources.find( + (r) => resourceOptionId(r) === resourcesNav.value, + ); + + useEffect(() => { + const hasResourcesWithAgents = + resources.length > 0 && + resources[0].agents && + resources[0].agents.length > 0; + if (!selectedResource && hasResourcesWithAgents) { + resourcesNav.set(resourceOptionId(resources[0])); + } + }, [resources, selectedResource, resourcesNav]); + + const select = useCallback( + (resource: WorkspaceResource) => { + resourcesNav.set(resourceOptionId(resource)); + }, + [resourcesNav], + ); + + const isSelected = useCallback( + (resource: WorkspaceResource) => { + return resourceOptionId(resource) === resourcesNav.value; + }, + [resourcesNav.value], + ); + + return { + isSelected, + select, + selected: selectedResource, + }; +}; From 8c6e3341936504dc39f8e47af04ec7b65130550d Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 12 Jan 2024 12:36:33 +0000 Subject: [PATCH 2/4] Apply improvements from PRs --- .../pages/WorkspacePage/useResourcesNav.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/site/src/pages/WorkspacePage/useResourcesNav.ts b/site/src/pages/WorkspacePage/useResourcesNav.ts index f7b9d5530177a..511f82ccf9949 100644 --- a/site/src/pages/WorkspacePage/useResourcesNav.ts +++ b/site/src/pages/WorkspacePage/useResourcesNav.ts @@ -1,26 +1,37 @@ import { WorkspaceResource } from "api/typesGenerated"; import { useTab } from "hooks"; -import { useCallback, useEffect } from "react"; +import { useCallback, useEffect, useMemo } from "react"; export const resourceOptionId = (resource: WorkspaceResource) => { return `${resource.type}_${resource.name}`; }; export const useResourcesNav = (resources: WorkspaceResource[]) => { - const resourcesNav = useTab("resources", ""); - const selectedResource = resources.find( - (r) => resourceOptionId(r) === resourcesNav.value, + const firstResource = useMemo( + () => (resources.length > 0 ? resources[0] : undefined), + [resources], + ); + const resourcesNav = useTab( + "resources", + firstResource ? resourceOptionId(firstResource) : "", + ); + + const isSelected = useCallback( + (resource: WorkspaceResource) => { + return resourceOptionId(resource) === resourcesNav.value; + }, + [resourcesNav.value], ); + const selectedResource = resources.find(isSelected); + useEffect(() => { const hasResourcesWithAgents = - resources.length > 0 && - resources[0].agents && - resources[0].agents.length > 0; + firstResource && firstResource.agents && firstResource.agents.length > 0; if (!selectedResource && hasResourcesWithAgents) { - resourcesNav.set(resourceOptionId(resources[0])); + resourcesNav.set(resourceOptionId(firstResource)); } - }, [resources, selectedResource, resourcesNav]); + }, [firstResource, resourcesNav, selectedResource]); const select = useCallback( (resource: WorkspaceResource) => { @@ -29,13 +40,6 @@ export const useResourcesNav = (resources: WorkspaceResource[]) => { [resourcesNav], ); - const isSelected = useCallback( - (resource: WorkspaceResource) => { - return resourceOptionId(resource) === resourcesNav.value; - }, - [resourcesNav.value], - ); - return { isSelected, select, From 1d2e8f376215615cbef62ae746588ff6a83e05f1 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 12 Jan 2024 12:44:18 +0000 Subject: [PATCH 3/4] Use effect event --- .../pages/WorkspacePage/useResourcesNav.ts | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/site/src/pages/WorkspacePage/useResourcesNav.ts b/site/src/pages/WorkspacePage/useResourcesNav.ts index 511f82ccf9949..fea70f741a4e4 100644 --- a/site/src/pages/WorkspacePage/useResourcesNav.ts +++ b/site/src/pages/WorkspacePage/useResourcesNav.ts @@ -1,5 +1,6 @@ import { WorkspaceResource } from "api/typesGenerated"; import { useTab } from "hooks"; +import { useEffectEvent } from "hooks/hookPolyfills"; import { useCallback, useEffect, useMemo } from "react"; export const resourceOptionId = (resource: WorkspaceResource) => { @@ -24,14 +25,20 @@ export const useResourcesNav = (resources: WorkspaceResource[]) => { ); const selectedResource = resources.find(isSelected); - + const onSelectedResourceChange = useEffectEvent( + (previousResource?: WorkspaceResource) => { + const hasResourcesWithAgents = + firstResource && + firstResource.agents && + firstResource.agents.length > 0; + if (!previousResource && hasResourcesWithAgents) { + resourcesNav.set(resourceOptionId(firstResource)); + } + }, + ); useEffect(() => { - const hasResourcesWithAgents = - firstResource && firstResource.agents && firstResource.agents.length > 0; - if (!selectedResource && hasResourcesWithAgents) { - resourcesNav.set(resourceOptionId(firstResource)); - } - }, [firstResource, resourcesNav, selectedResource]); + onSelectedResourceChange(selectedResource); + }, [onSelectedResourceChange, selectedResource]); const select = useCallback( (resource: WorkspaceResource) => { From ec2282e29b5ea14f75991ed2e53afb50d583e12e Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 12 Jan 2024 13:00:13 +0000 Subject: [PATCH 4/4] Add more comments about the solution --- .../WorkspacePage/useResourcesNav.test.tsx | 11 ++++++-- .../pages/WorkspacePage/useResourcesNav.ts | 26 +++++++++---------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/site/src/pages/WorkspacePage/useResourcesNav.test.tsx b/site/src/pages/WorkspacePage/useResourcesNav.test.tsx index 233493dd99185..d403ea1b4c3d8 100644 --- a/site/src/pages/WorkspacePage/useResourcesNav.test.tsx +++ b/site/src/pages/WorkspacePage/useResourcesNav.test.tsx @@ -106,7 +106,12 @@ describe("useResourcesNav", () => { ); expect(result.current.selected?.id).toBe(startedResources[0].id); - // When a workspace is stopped, there is no resource with agents + // When a workspace is stopped, there are no resources with agents, so we + // need to retain the currently selected resource. This ensures consistency + // when handling workspace updates that involve a sequence of stopping and + // starting. By preserving the resource selection, we maintain the desired + // configuration and prevent unintended changes during the stop-and-start + // process. const stoppedResources: WorkspaceResource[] = [ { ...MockWorkspaceResource, @@ -116,7 +121,9 @@ describe("useResourcesNav", () => { }, ]; rerender({ resources: stoppedResources }); - expect(result.current.selected).toBe(undefined); + expect(result.current.selectedValue).toBe( + resourceOptionId(startedResources[0]), + ); // When a workspace is started again a resource is selected rerender({ resources: startedResources }); diff --git a/site/src/pages/WorkspacePage/useResourcesNav.ts b/site/src/pages/WorkspacePage/useResourcesNav.ts index fea70f741a4e4..7774d1073240c 100644 --- a/site/src/pages/WorkspacePage/useResourcesNav.ts +++ b/site/src/pages/WorkspacePage/useResourcesNav.ts @@ -1,21 +1,20 @@ import { WorkspaceResource } from "api/typesGenerated"; import { useTab } from "hooks"; import { useEffectEvent } from "hooks/hookPolyfills"; -import { useCallback, useEffect, useMemo } from "react"; +import { useCallback, useEffect } from "react"; export const resourceOptionId = (resource: WorkspaceResource) => { return `${resource.type}_${resource.name}`; }; +// TODO: This currently serves as a temporary workaround for synchronizing the +// resources tab during workspace transitions. The optimal resolution involves +// eliminating the sync and updating the URL within the workspace data update +// event in the WebSocket "onData" event. However, this requires substantial +// refactoring. Consider revisiting this solution in the future for a more +// robust implementation. export const useResourcesNav = (resources: WorkspaceResource[]) => { - const firstResource = useMemo( - () => (resources.length > 0 ? resources[0] : undefined), - [resources], - ); - const resourcesNav = useTab( - "resources", - firstResource ? resourceOptionId(firstResource) : "", - ); + const resourcesNav = useTab("resources", ""); const isSelected = useCallback( (resource: WorkspaceResource) => { @@ -28,11 +27,11 @@ export const useResourcesNav = (resources: WorkspaceResource[]) => { const onSelectedResourceChange = useEffectEvent( (previousResource?: WorkspaceResource) => { const hasResourcesWithAgents = - firstResource && - firstResource.agents && - firstResource.agents.length > 0; + resources.length > 0 && + resources[0].agents && + resources[0].agents.length > 0; if (!previousResource && hasResourcesWithAgents) { - resourcesNav.set(resourceOptionId(firstResource)); + resourcesNav.set(resourceOptionId(resources[0])); } }, ); @@ -51,5 +50,6 @@ export const useResourcesNav = (resources: WorkspaceResource[]) => { isSelected, select, selected: selectedResource, + selectedValue: resourcesNav.value, }; };