From 159538c3741b4952ba213692a3b429da07483305 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 May 2023 10:44:11 -0500 Subject: [PATCH 01/13] WIP: Proxy auto select and user selection state Lotta comments --- .../components/AppLink/AppLink.stories.tsx | 3 + .../components/Resources/AgentRow.stories.tsx | 3 + .../Resources/ResourceCard.stories.tsx | 6 + .../Workspace/Workspace.stories.tsx | 3 + site/src/contexts/ProxyContext.test.ts | 62 ------ site/src/contexts/ProxyContext.test.tsx | 192 ++++++++++++++++++ site/src/contexts/ProxyContext.tsx | 81 ++++++-- .../pages/TerminalPage/TerminalPage.test.tsx | 1 + site/src/testHelpers/localstorage.ts | 22 ++ 9 files changed, 295 insertions(+), 78 deletions(-) delete mode 100644 site/src/contexts/ProxyContext.test.ts create mode 100644 site/src/contexts/ProxyContext.test.tsx create mode 100644 site/src/testHelpers/localstorage.ts diff --git a/site/src/components/AppLink/AppLink.stories.tsx b/site/src/components/AppLink/AppLink.stories.tsx index d963637902fd5..c478dbf973a77 100644 --- a/site/src/components/AppLink/AppLink.stories.tsx +++ b/site/src/components/AppLink/AppLink.stories.tsx @@ -26,6 +26,9 @@ const Template: Story = (args) => ( setProxy: () => { return }, + clearProxy: () => { + return + }, }} > diff --git a/site/src/components/Resources/AgentRow.stories.tsx b/site/src/components/Resources/AgentRow.stories.tsx index 9b9d63d5b90ca..eac06ec1231bf 100644 --- a/site/src/components/Resources/AgentRow.stories.tsx +++ b/site/src/components/Resources/AgentRow.stories.tsx @@ -65,6 +65,9 @@ const TemplateFC = ( setProxy: () => { return }, + clearProxy: () => { + return + }, }} > diff --git a/site/src/components/Resources/ResourceCard.stories.tsx b/site/src/components/Resources/ResourceCard.stories.tsx index afba475fa24ee..cdc1f028772db 100644 --- a/site/src/components/Resources/ResourceCard.stories.tsx +++ b/site/src/components/Resources/ResourceCard.stories.tsx @@ -30,6 +30,9 @@ Example.args = { setProxy: () => { return }, + clearProxy: () => { + return + }, }} > { return }, + clearProxy: () => { + return + }, }} > = (args) => ( setProxy: () => { return }, + clearProxy: () => { + return + }, }} > diff --git a/site/src/contexts/ProxyContext.test.ts b/site/src/contexts/ProxyContext.test.ts deleted file mode 100644 index a94ebbec22081..0000000000000 --- a/site/src/contexts/ProxyContext.test.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { - MockPrimaryWorkspaceProxy, - MockWorkspaceProxies, - MockHealthyWildWorkspaceProxy, - MockUnhealthyWildWorkspaceProxy, -} from "testHelpers/entities" -import { getPreferredProxy } from "./ProxyContext" - -describe("ProxyContextGetURLs", () => { - it.each([ - ["empty", [], undefined, "", ""], - // Primary has no path app URL. Uses relative links - [ - "primary", - [MockPrimaryWorkspaceProxy], - MockPrimaryWorkspaceProxy, - "", - MockPrimaryWorkspaceProxy.wildcard_hostname, - ], - [ - "regions selected", - MockWorkspaceProxies, - MockHealthyWildWorkspaceProxy, - MockHealthyWildWorkspaceProxy.path_app_url, - MockHealthyWildWorkspaceProxy.wildcard_hostname, - ], - // Primary is the default if none selected - [ - "no selected", - [MockPrimaryWorkspaceProxy], - undefined, - "", - MockPrimaryWorkspaceProxy.wildcard_hostname, - ], - [ - "regions no select primary default", - MockWorkspaceProxies, - undefined, - "", - MockPrimaryWorkspaceProxy.wildcard_hostname, - ], - // Primary is the default if the selected is unhealthy - [ - "unhealthy selection", - MockWorkspaceProxies, - MockUnhealthyWildWorkspaceProxy, - "", - MockPrimaryWorkspaceProxy.wildcard_hostname, - ], - // This should never happen, when there is no primary - ["no primary", [MockHealthyWildWorkspaceProxy], undefined, "", ""], - ])( - `%p`, - (_, regions, selected, preferredPathAppURL, preferredWildcardHostname) => { - const preferred = getPreferredProxy(regions, selected) - expect(preferred.preferredPathAppURL).toBe(preferredPathAppURL) - expect(preferred.preferredWildcardHostname).toBe( - preferredWildcardHostname, - ) - }, - ) -}) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx new file mode 100644 index 0000000000000..82fe6a062a696 --- /dev/null +++ b/site/src/contexts/ProxyContext.test.tsx @@ -0,0 +1,192 @@ +import { + MockPrimaryWorkspaceProxy, + MockWorkspaceProxies, + MockHealthyWildWorkspaceProxy, + MockUnhealthyWildWorkspaceProxy, +} from "testHelpers/entities" +import { + getPreferredProxy, + ProxyProvider, + saveUserSelectedProxy, + useProxy, +} from "./ProxyContext" +import * as ProxyContextModule from "./ProxyContext" +import { + renderWithAuth, + waitForLoaderToBeRemoved, +} from "testHelpers/renderHelpers" +import { screen } from "@testing-library/react" +import { server } from "testHelpers/server" +import "testHelpers/localstorage" +import { rest } from "msw" +import { Region } from "api/typesGenerated" + +describe("ProxyContextGetURLs", () => { + it.each([ + ["empty", [], undefined, "", ""], + // Primary has no path app URL. Uses relative links + [ + "primary", + [MockPrimaryWorkspaceProxy], + MockPrimaryWorkspaceProxy, + "", + MockPrimaryWorkspaceProxy.wildcard_hostname, + ], + [ + "regions selected", + MockWorkspaceProxies, + MockHealthyWildWorkspaceProxy, + MockHealthyWildWorkspaceProxy.path_app_url, + MockHealthyWildWorkspaceProxy.wildcard_hostname, + ], + // Primary is the default if none selected + [ + "no selected", + [MockPrimaryWorkspaceProxy], + undefined, + "", + MockPrimaryWorkspaceProxy.wildcard_hostname, + ], + [ + "regions no select primary default", + MockWorkspaceProxies, + undefined, + "", + MockPrimaryWorkspaceProxy.wildcard_hostname, + ], + // Primary is the default if the selected is unhealthy + [ + "unhealthy selection", + MockWorkspaceProxies, + MockUnhealthyWildWorkspaceProxy, + "", + MockPrimaryWorkspaceProxy.wildcard_hostname, + ], + // This should never happen, when there is no primary + ["no primary", [MockHealthyWildWorkspaceProxy], undefined, "", ""], + ])( + `%p`, + (_, regions, selected, preferredPathAppURL, preferredWildcardHostname) => { + const preferred = getPreferredProxy(regions, selected) + expect(preferred.preferredPathAppURL).toBe(preferredPathAppURL) + expect(preferred.preferredWildcardHostname).toBe( + preferredWildcardHostname, + ) + }, + ) +}) + +// interface ProxySelectTest { +// name: string +// actions: () +// } + +const TestingComponent = () => { + return renderWithAuth( + + + , + { + route: `/proxies`, + path: "/proxies", + }, + ) +} + +// TestingScreen just mounts some components that we can check in the unit test. +const TestingScreen = () => { + const { proxy, isFetched, isLoading } = useProxy() + return ( + <> +
+
+
+ + ) +} + +interface ProxyContextSelectionTest { + expSelectedProxyID: string + regions: Region[] + storageProxy: Region | undefined +} + +describe("ProxyContextSelection", () => { + beforeEach(() => { + window.localStorage.clear() + }) + + it.each([ + [ + "empty", + { + expSelectedProxyID: "", + regions: [], + storageProxy: undefined, + }, + ], + [ + "regions_no_selection", + { + expSelectedProxyID: MockPrimaryWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: undefined, + }, + ], + [ + "regions_selected_unhealthy", + { + expSelectedProxyID: MockPrimaryWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: MockUnhealthyWildWorkspaceProxy, + }, + ], + ] as [string, ProxyContextSelectionTest][])( + `%s`, + async (_, { expSelectedProxyID, regions, storageProxy }) => { + // Initial selection if present + if (storageProxy) { + saveUserSelectedProxy(storageProxy) + } + + // Mock the API response + server.use( + rest.get("/api/v2/regions", async (req, res, ctx) => { + return res( + ctx.status(200), + ctx.json({ + regions: regions, + }), + ) + }), + ) + + TestingComponent() + await waitForLoaderToBeRemoved() + + await screen.findByTestId("isFetched").then((x) => { + expect(x.title).toBe("true") + }) + await screen.findByTestId("isLoading").then((x) => { + expect(x.title).toBe("false") + }) + await screen.findByTestId("preferredProxy").then((x) => { + expect(x.title).toBe(expSelectedProxyID) + }) + + // const { proxy, proxies, isFetched, isLoading, proxyLatencies } = useProxy() + // expect(isLoading).toBe(false) + // expect(isFetched).toBe(true) + + // expect(x).toBe(2) + // const preferred = getPreferredProxy(regions, selected) + // expect(preferred.preferredPathAppURL).toBe(preferredPathAppURL) + // expect(preferred.preferredWildcardHostname).toBe( + // preferredWildcardHostname, + // ) + }, + ) +}) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 66390164809a6..8dd9aa3120c46 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -11,16 +11,39 @@ import { } from "react" import { ProxyLatencyReport, useProxyLatency } from "./useProxyLatency" -interface ProxyContextValue { +export interface ProxyContextValue { + // proxy is **always** the workspace proxy that should be used. + // The 'proxy.selectedProxy' field is the proxy being used and comes from either: + // 1. The user manually selected this proxy. (saved to local storage) + // 2. The default proxy auto selected because: + // a. The user has not selected a proxy. + // b. The user's selected proxy is not in the list of proxies. + // c. The user's selected proxy is not healthy. + // 3. undefined if there are no proxies. + // + // The values 'proxy.preferredPathAppURL' and 'proxy.preferredWildcardHostname' can + // always be used even if 'proxy.selectedProxy' is undefined. These values are sourced from + // the 'selectedProxy', but default to relative paths if the 'selectedProxy' is undefined. proxy: PreferredProxy + + // proxies is the list of proxies returned by coderd. This is fetched async. + // isFetched, isLoading, and error are used to track the state of the async call. proxies?: Region[] - proxyLatencies?: Record - // isfetched is true when the proxy api call is complete. + // isFetched is true when the 'proxies' api call is complete. isFetched: boolean - // isLoading is true if the proxy is in the process of being fetched. isLoading: boolean error?: Error | unknown + // proxyLatencies is a map of proxy id to latency report. If the proxyLatencies[proxy.id] is undefined + // then the latency has not been fetched yet. Calculations happen async for each proxy in the list. + // Refer to the returned report for a given proxy for more information. + proxyLatencies: Record + // setProxy is a function that sets the user's selected proxy. This function should + // only be called if the user is manually selecting a proxy. This value is stored in local + // storage and will persist across reloads and tabs. setProxy: (selectedProxy: Region) => void + // clearProxy is a function that clears the user's selected proxy. + // If no proxy is selected, then the default proxy will be used. + clearProxy: () => void } interface PreferredProxy { @@ -45,15 +68,23 @@ export const ProxyContext = createContext( */ export const ProxyProvider: FC = ({ children }) => { // Try to load the preferred proxy from local storage. - let savedProxy = loadPreferredProxy() + const savedProxy = loadUserSelectedProxy() + // As the proxies are being loaded, default to using the saved proxy. + // If the saved proxy is not valid when the async fetch happens, the + // selectedProxy will be updated accordingly. + let defaultPreferredProxy: PreferredProxy = { + selectedProxy: savedProxy, + preferredPathAppURL: savedProxy?.path_app_url.replace(/\/$/, "") || "", + preferredWildcardHostname: savedProxy?.wildcard_hostname || "", + } if (!savedProxy) { // If no preferred proxy is saved, then default to using relative paths // and no subdomain support until the proxies are properly loaded. // This is the same as a user not selecting any proxy. - savedProxy = getPreferredProxy([]) + defaultPreferredProxy = getPreferredProxy([]) } - const [proxy, setProxy] = useState(savedProxy) + const [proxy, setProxy] = useState(defaultPreferredProxy) const dashboard = useDashboard() const experimentEnabled = dashboard?.experiments.includes("moons") @@ -68,13 +99,13 @@ export const ProxyProvider: FC = ({ children }) => { queryFn: getWorkspaceProxies, // This onSuccess ensures the local storage is synchronized with the // proxies returned by coderd. If the selected proxy is not in the list, - // then the user selection is removed. + // then the user selection is ignored. onSuccess: (resp) => { setAndSaveProxy(proxy.selectedProxy, resp.regions) }, }) - // Everytime we get a new proxiesResponse, update the latency check + // Every time we get a new proxiesResponse, update the latency check // to each workspace proxy. const proxyLatencies = useProxyLatency(proxiesResp) @@ -90,14 +121,27 @@ export const ProxyProvider: FC = ({ children }) => { "proxies are not yet loaded, so selecting a proxy makes no sense. How did you get here?", ) } + + if (selectedProxy) { + // Save to local storage to persist the user's preference across reloads + // and other tabs. We always save this, even if the selection is "bad". + saveUserSelectedProxy(selectedProxy) + } + + // The preferred proxy attempts to use the user's selection if it is valid. const preferred = getPreferredProxy(proxies, selectedProxy) - // Save to local storage to persist the user's preference across reloads - // and other tabs. - savePreferredProxy(preferred) // Set the state for the current context. setProxy(preferred) } + const clearProxy = () => { + // Clear the user's selection from local storage. + clearUserSelectedProxy() + // Set the state for the current context. + // If we pass no values, then the default proxy will be used. + setAndSaveProxy() + } + return ( = ({ children }) => { // A function that takes the new proxies and selected proxy and updates // the state with the appropriate urls. setProxy: setAndSaveProxy, + clearProxy: clearProxy, }} > {children} @@ -183,12 +228,16 @@ export const getPreferredProxy = ( // Local storage functions -export const savePreferredProxy = (saved: PreferredProxy): void => { - window.localStorage.setItem("preferred-proxy", JSON.stringify(saved)) +export const clearUserSelectedProxy = (): void => { + window.localStorage.removeItem("user-selected-proxy") +} + +export const saveUserSelectedProxy = (saved: Region): void => { + window.localStorage.setItem("user-selected-proxy", JSON.stringify(saved)) } -const loadPreferredProxy = (): PreferredProxy | undefined => { - const str = localStorage.getItem("preferred-proxy") +export const loadUserSelectedProxy = (): Region | undefined => { + const str = localStorage.getItem("user-selected-proxy") if (!str) { return undefined } diff --git a/site/src/pages/TerminalPage/TerminalPage.test.tsx b/site/src/pages/TerminalPage/TerminalPage.test.tsx index 1ca67ae146344..52046826bd7d7 100644 --- a/site/src/pages/TerminalPage/TerminalPage.test.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.test.tsx @@ -54,6 +54,7 @@ const renderTerminal = () => { isFetched: true, isLoading: false, setProxy: jest.fn(), + clearProxy: jest.fn(), }} > diff --git a/site/src/testHelpers/localstorage.ts b/site/src/testHelpers/localstorage.ts new file mode 100644 index 0000000000000..9b645c7e0f0f7 --- /dev/null +++ b/site/src/testHelpers/localstorage.ts @@ -0,0 +1,22 @@ +export const localStorageMock = () => { + const store = {} as Record; + + return { + getItem: (key: string):string =>{ + return store[key]; + }, + setItem: (key:string, value:string)=> { + store[key] = value; + }, + clear: ()=> { + Object.keys(store).forEach((key) => { + delete store[key]; + }); + }, + removeItem: (key:string) => { + delete store[key]; + } + } +} + +Object.defineProperty(window, "localStorage", { value: localStorageMock }); From 13059311bcdcde1c8405f1e52a679e15c2931865 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 09:24:07 -0500 Subject: [PATCH 02/13] chore: Auto select based on latency --- site/src/contexts/ProxyContext.test.tsx | 229 +++++++++++++++++++++--- site/src/contexts/ProxyContext.tsx | 131 ++++++++++---- site/src/testHelpers/entities.ts | 2 +- site/src/testHelpers/localstorage.ts | 2 +- 4 files changed, 297 insertions(+), 67 deletions(-) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx index 82fe6a062a696..504e5e930474d 100644 --- a/site/src/contexts/ProxyContext.test.tsx +++ b/site/src/contexts/ProxyContext.test.tsx @@ -10,24 +10,44 @@ import { saveUserSelectedProxy, useProxy, } from "./ProxyContext" -import * as ProxyContextModule from "./ProxyContext" +import * as ProxyLatency from "./useProxyLatency" import { renderWithAuth, waitForLoaderToBeRemoved, } from "testHelpers/renderHelpers" import { screen } from "@testing-library/react" import { server } from "testHelpers/server" -import "testHelpers/localstorage" import { rest } from "msw" import { Region } from "api/typesGenerated" +import "testHelpers/localstorage" +import userEvent from "@testing-library/user-event" + +// Mock useProxyLatency to use a hard-coded latency. 'jest.mock' must be called +// here and not inside a unit test. +jest.mock("contexts/useProxyLatency", () => ({ + useProxyLatency: () => { + return hardCodedLatencies + }, +})) + +let hardCodedLatencies: Record = {} + +const fakeLatency = (ms: number): ProxyLatency.ProxyLatencyReport => { + return { + latencyMS: ms, + accurate: true, + at: new Date(), + } +} describe("ProxyContextGetURLs", () => { it.each([ - ["empty", [], undefined, "", ""], + ["empty", [], {}, undefined, "", ""], // Primary has no path app URL. Uses relative links [ "primary", [MockPrimaryWorkspaceProxy], + {}, MockPrimaryWorkspaceProxy, "", MockPrimaryWorkspaceProxy.wildcard_hostname, @@ -35,6 +55,7 @@ describe("ProxyContextGetURLs", () => { [ "regions selected", MockWorkspaceProxies, + {}, MockHealthyWildWorkspaceProxy, MockHealthyWildWorkspaceProxy.path_app_url, MockHealthyWildWorkspaceProxy.wildcard_hostname, @@ -43,6 +64,7 @@ describe("ProxyContextGetURLs", () => { [ "no selected", [MockPrimaryWorkspaceProxy], + {}, undefined, "", MockPrimaryWorkspaceProxy.wildcard_hostname, @@ -50,6 +72,7 @@ describe("ProxyContextGetURLs", () => { [ "regions no select primary default", MockWorkspaceProxies, + {}, undefined, "", MockPrimaryWorkspaceProxy.wildcard_hostname, @@ -58,16 +81,40 @@ describe("ProxyContextGetURLs", () => { [ "unhealthy selection", MockWorkspaceProxies, + {}, MockUnhealthyWildWorkspaceProxy, "", MockPrimaryWorkspaceProxy.wildcard_hostname, ], // This should never happen, when there is no primary - ["no primary", [MockHealthyWildWorkspaceProxy], undefined, "", ""], + ["no primary", [MockHealthyWildWorkspaceProxy], {}, undefined, "", ""], + // Latency behavior + [ + "best latency", + MockWorkspaceProxies, + { + [MockPrimaryWorkspaceProxy.id]: fakeLatency(100), + [MockHealthyWildWorkspaceProxy.id]: fakeLatency(50), + // This should be ignored because it's unhealthy + [MockUnhealthyWildWorkspaceProxy.id]: fakeLatency(25), + // This should be ignored because it is not in the list. + ["not a proxy"]: fakeLatency(10), + }, + undefined, + MockHealthyWildWorkspaceProxy.path_app_url, + MockHealthyWildWorkspaceProxy.wildcard_hostname, + ], ])( `%p`, - (_, regions, selected, preferredPathAppURL, preferredWildcardHostname) => { - const preferred = getPreferredProxy(regions, selected) + ( + _, + regions, + latencies, + selected, + preferredPathAppURL, + preferredWildcardHostname, + ) => { + const preferred = getPreferredProxy(regions, selected, latencies) expect(preferred.preferredPathAppURL).toBe(preferredPathAppURL) expect(preferred.preferredWildcardHostname).toBe( preferredWildcardHostname, @@ -76,11 +123,6 @@ describe("ProxyContextGetURLs", () => { ) }) -// interface ProxySelectTest { -// name: string -// actions: () -// } - const TestingComponent = () => { return renderWithAuth( @@ -95,7 +137,8 @@ const TestingComponent = () => { // TestingScreen just mounts some components that we can check in the unit test. const TestingScreen = () => { - const { proxy, isFetched, isLoading } = useProxy() + const { proxy, userProxy, isFetched, isLoading, clearProxy, setProxy } = + useProxy() return ( <>
@@ -104,34 +147,71 @@ const TestingScreen = () => { data-testid="preferredProxy" title={proxy.selectedProxy && proxy.selectedProxy.id} > +
+ +
+ ) } interface ProxyContextSelectionTest { - expSelectedProxyID: string regions: Region[] storageProxy: Region | undefined + latencies?: Record + afterLoad?: (user: typeof userEvent) => Promise + + expProxyID: string + expUserProxyID?: string } describe("ProxyContextSelection", () => { beforeEach(() => { + // Object.defineProperty(window, "localStorage", { value: localStorageMock }) window.localStorage.clear() }) + // A way to simulate a user clearing the proxy selection. + const clearProxyAction = async (user: typeof userEvent): Promise => { + const clearProxyButton = screen.getByTestId("clearProxy") + await user.click(clearProxyButton) + } + + const userSelectProxy = ( + proxy: Region, + ): ((user: typeof userEvent) => Promise) => { + return async (user): Promise => { + const selectData = screen.getByTestId("userSelectProxyData") + selectData.innerText = JSON.stringify(proxy) + + const selectProxyButton = screen.getByTestId("userSelectProxy") + await user.click(selectProxyButton) + } + } + it.each([ + // Not latency behavior [ "empty", { - expSelectedProxyID: "", + expProxyID: "", regions: [], storageProxy: undefined, + latencies: {}, }, ], [ "regions_no_selection", { - expSelectedProxyID: MockPrimaryWorkspaceProxy.id, + expProxyID: MockPrimaryWorkspaceProxy.id, regions: MockWorkspaceProxies, storageProxy: undefined, }, @@ -139,14 +219,110 @@ describe("ProxyContextSelection", () => { [ "regions_selected_unhealthy", { - expSelectedProxyID: MockPrimaryWorkspaceProxy.id, + expProxyID: MockPrimaryWorkspaceProxy.id, regions: MockWorkspaceProxies, storageProxy: MockUnhealthyWildWorkspaceProxy, + expUserProxyID: MockUnhealthyWildWorkspaceProxy.id, + }, + ], + [ + "regions_selected_healthy", + { + expProxyID: MockHealthyWildWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: MockHealthyWildWorkspaceProxy, + expUserProxyID: MockHealthyWildWorkspaceProxy.id, + }, + ], + [ + "regions_selected_clear", + { + expProxyID: MockPrimaryWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: MockHealthyWildWorkspaceProxy, + afterLoad: clearProxyAction, + expUserProxyID: undefined, + }, + ], + [ + "regions_make_selection", + { + expProxyID: MockHealthyWildWorkspaceProxy.id, + regions: MockWorkspaceProxies, + afterLoad: userSelectProxy(MockHealthyWildWorkspaceProxy), + expUserProxyID: MockHealthyWildWorkspaceProxy.id, + }, + ], + // Latency behavior + [ + "regions_default_low_latency", + { + expProxyID: MockHealthyWildWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: undefined, + latencies: { + [MockPrimaryWorkspaceProxy.id]: fakeLatency(100), + [MockHealthyWildWorkspaceProxy.id]: fakeLatency(50), + // This is a trick. It's unhealthy so should be ignored. + [MockUnhealthyWildWorkspaceProxy.id]: fakeLatency(25), + }, + }, + ], + [ + // User intentionally selected a high latency proxy. + "regions_select_high_latency", + { + expProxyID: MockHealthyWildWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: undefined, + afterLoad: userSelectProxy(MockHealthyWildWorkspaceProxy), + expUserProxyID: MockHealthyWildWorkspaceProxy.id, + latencies: { + [MockHealthyWildWorkspaceProxy.id]: fakeLatency(500), + [MockPrimaryWorkspaceProxy.id]: fakeLatency(100), + // This is a trick. It's unhealthy so should be ignored. + [MockUnhealthyWildWorkspaceProxy.id]: fakeLatency(25), + }, + }, + ], + [ + // Low latency proxy is selected, but it is unhealthy + "regions_select_unhealthy_low_latency", + { + expProxyID: MockPrimaryWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: MockUnhealthyWildWorkspaceProxy, + expUserProxyID: MockUnhealthyWildWorkspaceProxy.id, + latencies: { + [MockHealthyWildWorkspaceProxy.id]: fakeLatency(500), + [MockPrimaryWorkspaceProxy.id]: fakeLatency(100), + // This is a trick. It's unhealthy so should be ignored. + [MockUnhealthyWildWorkspaceProxy.id]: fakeLatency(25), + }, }, ], ] as [string, ProxyContextSelectionTest][])( `%s`, - async (_, { expSelectedProxyID, regions, storageProxy }) => { + async ( + _, + { + expUserProxyID, + expProxyID: expSelectedProxyID, + regions, + storageProxy, + latencies = {}, + afterLoad, + }, + ) => { + // Mock the latencies + hardCodedLatencies = latencies + + // jest.mock("contexts/useProxyLatency", () => ({ + // useProxyLatency: () => { + // return latencies + // }, + // })) + // Initial selection if present if (storageProxy) { saveUserSelectedProxy(storageProxy) @@ -167,6 +343,11 @@ describe("ProxyContextSelection", () => { TestingComponent() await waitForLoaderToBeRemoved() + const user = userEvent.setup() + if (afterLoad) { + await afterLoad(user) + } + await screen.findByTestId("isFetched").then((x) => { expect(x.title).toBe("true") }) @@ -176,17 +357,9 @@ describe("ProxyContextSelection", () => { await screen.findByTestId("preferredProxy").then((x) => { expect(x.title).toBe(expSelectedProxyID) }) - - // const { proxy, proxies, isFetched, isLoading, proxyLatencies } = useProxy() - // expect(isLoading).toBe(false) - // expect(isFetched).toBe(true) - - // expect(x).toBe(2) - // const preferred = getPreferredProxy(regions, selected) - // expect(preferred.preferredPathAppURL).toBe(preferredPathAppURL) - // expect(preferred.preferredWildcardHostname).toBe( - // preferredWildcardHostname, - // ) + await screen.findByTestId("userProxy").then((x) => { + expect(x.title).toBe(expUserProxyID || "") + }) }, ) }) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 8dd9aa3120c46..8f5d69118eca3 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -6,7 +6,9 @@ import { createContext, FC, PropsWithChildren, + useCallback, useContext, + useEffect, useState, } from "react" import { ProxyLatencyReport, useProxyLatency } from "./useProxyLatency" @@ -26,6 +28,15 @@ export interface ProxyContextValue { // the 'selectedProxy', but default to relative paths if the 'selectedProxy' is undefined. proxy: PreferredProxy + // userProxy is always the proxy the user has selected. This value comes from local storage. + // The value `proxy` should always be used instead of `userProxy`. `userProxy` is only exposed + // so the caller can determine if the proxy being used is the user's selected proxy, or if it + // was auto selected based on some other criteria. + // + // if(proxy.selectedProxy.id === userProxy.id) { /* user selected proxy */ } + // else { /* proxy was auto selected */ } + userProxy?: Region + // proxies is the list of proxies returned by coderd. This is fetched async. // isFetched, isLoading, and error are used to track the state of the async call. proxies?: Region[] @@ -47,7 +58,8 @@ export interface ProxyContextValue { } interface PreferredProxy { - // selectedProxy is the proxy the user has selected. + // selectedProxy is the preferred proxy being used. It is provided for + // getting the fields such as "display_name" and "id" // Do not use the fields 'path_app_url' or 'wildcard_hostname' from this // object. Use the preferred fields. selectedProxy: Region | undefined @@ -68,7 +80,10 @@ export const ProxyContext = createContext( */ export const ProxyProvider: FC = ({ children }) => { // Try to load the preferred proxy from local storage. - const savedProxy = loadUserSelectedProxy() + const [savedProxy, setUserProxy] = useState( + loadUserSelectedProxy(), + ) + // As the proxies are being loaded, default to using the saved proxy. // If the saved proxy is not valid when the async fetch happens, the // selectedProxy will be updated accordingly. @@ -101,7 +116,8 @@ export const ProxyProvider: FC = ({ children }) => { // proxies returned by coderd. If the selected proxy is not in the list, // then the user selection is ignored. onSuccess: (resp) => { - setAndSaveProxy(proxy.selectedProxy, resp.regions) + // Always pass in the user's choice. + setAndSaveProxy(savedProxy, resp.regions) }, }) @@ -109,42 +125,44 @@ export const ProxyProvider: FC = ({ children }) => { // to each workspace proxy. const proxyLatencies = useProxyLatency(proxiesResp) - const setAndSaveProxy = ( - selectedProxy?: Region, - // By default the proxies come from the api call above. - // Allow the caller to override this if they have a more up - // to date list of proxies. - proxies: Region[] = proxiesResp?.regions || [], - ) => { - if (!proxies) { - throw new Error( - "proxies are not yet loaded, so selecting a proxy makes no sense. How did you get here?", - ) - } - - if (selectedProxy) { - // Save to local storage to persist the user's preference across reloads - // and other tabs. We always save this, even if the selection is "bad". - saveUserSelectedProxy(selectedProxy) - } + // If the proxies change or latencies change, we need to update the + // callback function. + const setAndSaveProxy = useCallback( + ( + selectedProxy?: Region, + // By default the proxies come from the api call above. + // Allow the caller to override this if they have a more up + // to date list of proxies. + proxies: Region[] = proxiesResp?.regions || [], + ) => { + if (!proxies) { + throw new Error( + "proxies are not yet loaded, so selecting a proxy makes no sense. How did you get here?", + ) + } - // The preferred proxy attempts to use the user's selection if it is valid. - const preferred = getPreferredProxy(proxies, selectedProxy) - // Set the state for the current context. - setProxy(preferred) - } + // The preferred proxy attempts to use the user's selection if it is valid. + const preferred = getPreferredProxy( + proxies, + selectedProxy, + proxyLatencies, + ) + // Set the state for the current context. + setProxy(preferred) + }, + [proxiesResp, proxyLatencies], + ) - const clearProxy = () => { - // Clear the user's selection from local storage. - clearUserSelectedProxy() - // Set the state for the current context. - // If we pass no values, then the default proxy will be used. - setAndSaveProxy() - } + // This useEffect ensures the proxy to be used is updated whenever the state changes. + // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy. + useEffect(() => { + setAndSaveProxy(savedProxy) + }, [savedProxy, proxiesResp, proxyLatencies, setAndSaveProxy]) return ( = ({ children }) => { isLoading: proxiesLoading, isFetched: proxiesFetched, error: proxiesError, - // A function that takes the new proxies and selected proxy and updates - // the state with the appropriate urls. - setProxy: setAndSaveProxy, - clearProxy: clearProxy, + + // These functions are exposed to allow the user to select a proxy. + setProxy: (proxy: Region) => { + // Save to local storage to persist the user's preference across reloads + saveUserSelectedProxy(proxy) + // Set the state for the current context. + setUserProxy(proxy) + }, + clearProxy: () => { + // Clear the user's selection from local storage. + clearUserSelectedProxy() + setUserProxy(undefined) + }, }} > {children} @@ -185,10 +212,13 @@ export const useProxy = (): ProxyContextValue => { * * @param proxies Is the list of proxies returned by coderd. If this is empty, default behavior is used. * @param selectedProxy Is the proxy the user has selected. If this is undefined, default behavior is used. + * @param latencies If provided, this is used to determine the best proxy to default to. + * If not, `primary` is always the best default. */ export const getPreferredProxy = ( proxies: Region[], selectedProxy?: Region, + latencies?: Record, ): PreferredProxy => { // By default we set the path app to relative and disable wildcard hostnames. // We will set these values if we find a proxy we can use that supports them. @@ -203,7 +233,34 @@ export const getPreferredProxy = ( // If no proxy is selected, or the selected proxy is unhealthy default to the primary proxy. if (!selectedProxy || !selectedProxy.healthy) { + // By default, use the primary proxy. selectedProxy = proxies.find((proxy) => proxy.name === "primary") + // If we have latencies, then attempt to use the best proxy by latency instead. + if (latencies) { + const proxyMap = proxies.reduce((acc, proxy) => { + acc[proxy.id] = proxy + return acc + }, {} as Record) + + const best = Object.keys(latencies) + .map((proxyId) => { + return { + id: proxyId, + ...latencies[proxyId], + } + }) + // If the proxy is not in our list, or it is unhealthy, ignore it. + .filter((latency) => proxyMap[latency.id]?.healthy) + .sort((a, b) => a.latencyMS - b.latencyMS) + .at(0) + + // Found a new best, use it! + if (best) { + const bestProxy = proxies.find((proxy) => proxy.id === best.id) + // Default to w/e it was before + selectedProxy = bestProxy || selectedProxy + } + } } // Only use healthy proxies. diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 1d939f28bdba9..5dbd482990e1f 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1374,7 +1374,7 @@ export const MockEntitlementsWithScheduling: TypesGen.Entitlements = { }), } -export const MockExperiments: TypesGen.Experiment[] = ["workspace_actions"] +export const MockExperiments: TypesGen.Experiment[] = ["workspace_actions", "moons"] export const MockAuditLog: TypesGen.AuditLog = { id: "fbd2116a-8961-4954-87ae-e4575bd29ce0", diff --git a/site/src/testHelpers/localstorage.ts b/site/src/testHelpers/localstorage.ts index 9b645c7e0f0f7..0150b40969b03 100644 --- a/site/src/testHelpers/localstorage.ts +++ b/site/src/testHelpers/localstorage.ts @@ -19,4 +19,4 @@ export const localStorageMock = () => { } } -Object.defineProperty(window, "localStorage", { value: localStorageMock }); +Object.defineProperty(window, "localStorage", { value: localStorageMock() }); From d1c9f3fa79bca20bbf42e157b55076b013a87d48 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 09:32:34 -0500 Subject: [PATCH 03/13] Add extra test for unknown latencies --- site/src/contexts/ProxyContext.test.tsx | 35 ++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx index 504e5e930474d..d412a4b86cd9a 100644 --- a/site/src/contexts/ProxyContext.test.tsx +++ b/site/src/contexts/ProxyContext.test.tsx @@ -32,6 +32,7 @@ jest.mock("contexts/useProxyLatency", () => ({ let hardCodedLatencies: Record = {} +// fakeLatency is a helper function to make a Latency report from just a number. const fakeLatency = (ms: number): ProxyLatency.ProxyLatencyReport => { return { latencyMS: ms, @@ -164,12 +165,23 @@ const TestingScreen = () => { } interface ProxyContextSelectionTest { + // Regions is the list of regions to return via the "api" response. regions: Region[] + // storageProxy should be the proxy stored in local storage before the + // component is mounted and context is loaded. This simulates opening a + // new window with a selection saved from before. storageProxy: Region | undefined + // latencies is the hard coded latencies to return. If empty, no latencies + // are returned. latencies?: Record + // afterLoad are actions to take after loading the component, but before + // assertions. This is useful for simulating user actions. afterLoad?: (user: typeof userEvent) => Promise + // Assert these values. + // expProxyID is the proxyID returned to be used. expProxyID: string + // expUserProxyID is the user's stored selection. expUserProxyID?: string } @@ -301,6 +313,23 @@ describe("ProxyContextSelection", () => { }, }, ], + [ + // Excess proxies we do not have are low latency. + // This will probably never happen in production. + "unknown_regions_low_latency", + { + // Default to primary since we have unknowns + expProxyID: MockPrimaryWorkspaceProxy.id, + regions: MockWorkspaceProxies, + storageProxy: MockUnhealthyWildWorkspaceProxy, + expUserProxyID: MockUnhealthyWildWorkspaceProxy.id, + latencies: { + ["some"]: fakeLatency(500), + ["random"]: fakeLatency(100), + ["ids"]: fakeLatency(25), + }, + }, + ], ] as [string, ProxyContextSelectionTest][])( `%s`, async ( @@ -317,12 +346,6 @@ describe("ProxyContextSelection", () => { // Mock the latencies hardCodedLatencies = latencies - // jest.mock("contexts/useProxyLatency", () => ({ - // useProxyLatency: () => { - // return latencies - // }, - // })) - // Initial selection if present if (storageProxy) { saveUserSelectedProxy(storageProxy) From c739f245f36b5127bc2d3c4a876f1c73b2cc1d88 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 09:37:56 -0500 Subject: [PATCH 04/13] Move arg inside the funcs --- site/src/contexts/ProxyContext.test.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx index d412a4b86cd9a..ed71def26b5ed 100644 --- a/site/src/contexts/ProxyContext.test.tsx +++ b/site/src/contexts/ProxyContext.test.tsx @@ -21,6 +21,7 @@ import { rest } from "msw" import { Region } from "api/typesGenerated" import "testHelpers/localstorage" import userEvent from "@testing-library/user-event" +import UserEvent from "@testing-library/user-event" // Mock useProxyLatency to use a hard-coded latency. 'jest.mock' must be called // here and not inside a unit test. @@ -176,7 +177,7 @@ interface ProxyContextSelectionTest { latencies?: Record // afterLoad are actions to take after loading the component, but before // assertions. This is useful for simulating user actions. - afterLoad?: (user: typeof userEvent) => Promise + afterLoad?: () => Promise // Assert these values. // expProxyID is the proxyID returned to be used. @@ -192,15 +193,15 @@ describe("ProxyContextSelection", () => { }) // A way to simulate a user clearing the proxy selection. - const clearProxyAction = async (user: typeof userEvent): Promise => { + const clearProxyAction = async (): Promise => { + const user = userEvent.setup() const clearProxyButton = screen.getByTestId("clearProxy") await user.click(clearProxyButton) } - const userSelectProxy = ( - proxy: Region, - ): ((user: typeof userEvent) => Promise) => { - return async (user): Promise => { + const userSelectProxy = (proxy: Region): (() => Promise) => { + return async (): Promise => { + const user = userEvent.setup() const selectData = screen.getByTestId("userSelectProxyData") selectData.innerText = JSON.stringify(proxy) @@ -366,9 +367,8 @@ describe("ProxyContextSelection", () => { TestingComponent() await waitForLoaderToBeRemoved() - const user = userEvent.setup() if (afterLoad) { - await afterLoad(user) + await afterLoad() } await screen.findByTestId("isFetched").then((x) => { From ae2677736518a83e7f8b590dfd387a19f355e23b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 09:45:42 -0500 Subject: [PATCH 05/13] Fmt --- site/src/testHelpers/entities.ts | 5 ++++- site/src/testHelpers/localstorage.ts | 24 ++++++++++++------------ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 5dbd482990e1f..cbd69a2baabb5 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1374,7 +1374,10 @@ export const MockEntitlementsWithScheduling: TypesGen.Entitlements = { }), } -export const MockExperiments: TypesGen.Experiment[] = ["workspace_actions", "moons"] +export const MockExperiments: TypesGen.Experiment[] = [ + "workspace_actions", + "moons", +] export const MockAuditLog: TypesGen.AuditLog = { id: "fbd2116a-8961-4954-87ae-e4575bd29ce0", diff --git a/site/src/testHelpers/localstorage.ts b/site/src/testHelpers/localstorage.ts index 0150b40969b03..331a311323a36 100644 --- a/site/src/testHelpers/localstorage.ts +++ b/site/src/testHelpers/localstorage.ts @@ -1,22 +1,22 @@ export const localStorageMock = () => { - const store = {} as Record; + const store = {} as Record return { - getItem: (key: string):string =>{ - return store[key]; + getItem: (key: string): string => { + return store[key] }, - setItem: (key:string, value:string)=> { - store[key] = value; + setItem: (key: string, value: string) => { + store[key] = value }, - clear: ()=> { + clear: () => { Object.keys(store).forEach((key) => { - delete store[key]; - }); + delete store[key] + }) + }, + removeItem: (key: string) => { + delete store[key] }, - removeItem: (key:string) => { - delete store[key]; - } } } -Object.defineProperty(window, "localStorage", { value: localStorageMock() }); +Object.defineProperty(window, "localStorage", { value: localStorageMock() }) From 523e492e4611dcd17404792d30dc5d92d4a465da Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 10:17:06 -0500 Subject: [PATCH 06/13] Duplicate import --- site/src/contexts/ProxyContext.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx index ed71def26b5ed..84242821cc9cf 100644 --- a/site/src/contexts/ProxyContext.test.tsx +++ b/site/src/contexts/ProxyContext.test.tsx @@ -21,7 +21,6 @@ import { rest } from "msw" import { Region } from "api/typesGenerated" import "testHelpers/localstorage" import userEvent from "@testing-library/user-event" -import UserEvent from "@testing-library/user-event" // Mock useProxyLatency to use a hard-coded latency. 'jest.mock' must be called // here and not inside a unit test. From 877c0c900cfc0dd0d5cd6f88e348b6a66c249f1d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 13:41:30 -0500 Subject: [PATCH 07/13] PR feedback --- site/src/contexts/ProxyContext.test.tsx | 3 +- site/src/contexts/ProxyContext.tsx | 48 +++++++++---------- .../pages/TerminalPage/TerminalPage.test.tsx | 2 +- .../WorkspaceProxyPage/WorkspaceProxyPage.tsx | 2 +- 4 files changed, 27 insertions(+), 28 deletions(-) diff --git a/site/src/contexts/ProxyContext.test.tsx b/site/src/contexts/ProxyContext.test.tsx index 84242821cc9cf..f7b513699b3e0 100644 --- a/site/src/contexts/ProxyContext.test.tsx +++ b/site/src/contexts/ProxyContext.test.tsx @@ -146,7 +146,7 @@ const TestingScreen = () => {
@@ -187,7 +187,6 @@ interface ProxyContextSelectionTest { describe("ProxyContextSelection", () => { beforeEach(() => { - // Object.defineProperty(window, "localStorage", { value: localStorageMock }) window.localStorage.clear() }) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 8f5d69118eca3..7bf4fb489333f 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -58,11 +58,11 @@ export interface ProxyContextValue { } interface PreferredProxy { - // selectedProxy is the preferred proxy being used. It is provided for + // proxy is the proxy being used. It is provided for // getting the fields such as "display_name" and "id" // Do not use the fields 'path_app_url' or 'wildcard_hostname' from this // object. Use the preferred fields. - selectedProxy: Region | undefined + proxy: Region | undefined // PreferredPathAppURL is the URL of the proxy or it is the empty string // to indicate using relative paths. To add a path to this: // PreferredPathAppURL + "/path/to/app" @@ -86,12 +86,9 @@ export const ProxyProvider: FC = ({ children }) => { // As the proxies are being loaded, default to using the saved proxy. // If the saved proxy is not valid when the async fetch happens, the - // selectedProxy will be updated accordingly. - let defaultPreferredProxy: PreferredProxy = { - selectedProxy: savedProxy, - preferredPathAppURL: savedProxy?.path_app_url.replace(/\/$/, "") || "", - preferredWildcardHostname: savedProxy?.wildcard_hostname || "", - } + // proxy will be updated accordingly. + let defaultPreferredProxy = computeUsableURLS(savedProxy) + if (!savedProxy) { // If no preferred proxy is saved, then default to using relative paths // and no subdomain support until the proxies are properly loaded. @@ -206,12 +203,12 @@ export const useProxy = (): ProxyContextValue => { } /** - * getURLs is a helper function to calculate the urls to use for a given proxy configuration. By default, it is + * getPreferredProxy is a helper function to calculate the urls to use for a given proxy configuration. By default, it is * assumed no proxy is configured and relative paths should be used. * Exported for testing. * * @param proxies Is the list of proxies returned by coderd. If this is empty, default behavior is used. - * @param selectedProxy Is the proxy the user has selected. If this is undefined, default behavior is used. + * @param selectedProxy Is the proxy saved in local storage. If this is undefined, default behavior is used. * @param latencies If provided, this is used to determine the best proxy to default to. * If not, `primary` is always the best default. */ @@ -220,11 +217,6 @@ export const getPreferredProxy = ( selectedProxy?: Region, latencies?: Record, ): PreferredProxy => { - // By default we set the path app to relative and disable wildcard hostnames. - // We will set these values if we find a proxy we can use that supports them. - let pathAppURL = "" - let wildcardHostname = "" - // If a proxy is selected, make sure it is in the list of proxies. If it is not // we should default to the primary. selectedProxy = proxies.find( @@ -263,23 +255,31 @@ export const getPreferredProxy = ( } } - // Only use healthy proxies. - if (selectedProxy && selectedProxy.healthy) { + return computeUsableURLS(selectedProxy) +} + +const computeUsableURLS = (proxy?: Region): PreferredProxy => { + if (!proxy) { // By default use relative links for the primary proxy. // This is the default, and we should not change it. - if (selectedProxy.name !== "primary") { - pathAppURL = selectedProxy.path_app_url + return { + proxy: undefined, + preferredPathAppURL: "", + preferredWildcardHostname: "", } - wildcardHostname = selectedProxy.wildcard_hostname } - // TODO: @emyrk Should we notify the user if they had an unhealthy proxy selected? + let pathAppURL = proxy?.path_app_url.replace(/\/$/, "") + // Primary proxy uses relative paths. It's the only exception. + if (proxy.name === "primary") { + pathAppURL = "" + } return { - selectedProxy: selectedProxy, + proxy: proxy, // Trim trailing slashes to be consistent - preferredPathAppURL: pathAppURL.replace(/\/$/, ""), - preferredWildcardHostname: wildcardHostname, + preferredPathAppURL: pathAppURL, + preferredWildcardHostname: proxy.wildcard_hostname, } } diff --git a/site/src/pages/TerminalPage/TerminalPage.test.tsx b/site/src/pages/TerminalPage/TerminalPage.test.tsx index 52046826bd7d7..e12363506d504 100644 --- a/site/src/pages/TerminalPage/TerminalPage.test.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.test.tsx @@ -46,7 +46,7 @@ const renderTerminal = () => { value={{ proxyLatencies: MockProxyLatencies, proxy: { - selectedProxy: MockPrimaryWorkspaceProxy, + proxy: MockPrimaryWorkspaceProxy, preferredPathAppURL: "", preferredWildcardHostname: "", }, diff --git a/site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx b/site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx index 30a549eea1391..1a6a26db31313 100644 --- a/site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx +++ b/site/src/pages/UserSettingsPage/WorkspaceProxyPage/WorkspaceProxyPage.tsx @@ -36,7 +36,7 @@ export const WorkspaceProxyPage: FC> = () => { isLoading={proxiesLoading} hasLoaded={proxiesFetched} getWorkspaceProxiesError={proxiesError} - preferredProxy={proxy.selectedProxy} + preferredProxy={proxy.proxy} onSelect={(proxy) => { if (!proxy.healthy) { displayError("Please select a healthy workspace proxy.") From 5c534fa260a270da8a56edcf9512199bf9e40e01 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 May 2023 17:46:43 -0500 Subject: [PATCH 08/13] Refactor --- site/src/contexts/ProxyContext.tsx | 90 ++++++++++-------------------- 1 file changed, 30 insertions(+), 60 deletions(-) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 7bf4fb489333f..deafee0f19b74 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -32,9 +32,6 @@ export interface ProxyContextValue { // The value `proxy` should always be used instead of `userProxy`. `userProxy` is only exposed // so the caller can determine if the proxy being used is the user's selected proxy, or if it // was auto selected based on some other criteria. - // - // if(proxy.selectedProxy.id === userProxy.id) { /* user selected proxy */ } - // else { /* proxy was auto selected */ } userProxy?: Region // proxies is the list of proxies returned by coderd. This is fetched async. @@ -79,27 +76,18 @@ export const ProxyContext = createContext( * ProxyProvider interacts with local storage to indicate the preferred workspace proxy. */ export const ProxyProvider: FC = ({ children }) => { - // Try to load the preferred proxy from local storage. - const [savedProxy, setUserProxy] = useState( - loadUserSelectedProxy(), - ) - - // As the proxies are being loaded, default to using the saved proxy. - // If the saved proxy is not valid when the async fetch happens, the - // proxy will be updated accordingly. - let defaultPreferredProxy = computeUsableURLS(savedProxy) + const dashboard = useDashboard() + const experimentEnabled = dashboard?.experiments.includes("moons") - if (!savedProxy) { - // If no preferred proxy is saved, then default to using relative paths - // and no subdomain support until the proxies are properly loaded. - // This is the same as a user not selecting any proxy. - defaultPreferredProxy = getPreferredProxy([]) - } + // Using a useState so the caller always has the latest user saved + // proxy. + const [userSavedProxy, setUserSavedProxy] = useState(loadUserSelectedProxy()) - const [proxy, setProxy] = useState(defaultPreferredProxy) + // Load the initial state from local storage. + const [proxy, setProxy] = useState( + computeUsableURLS(userSavedProxy), + ) - const dashboard = useDashboard() - const experimentEnabled = dashboard?.experiments.includes("moons") const queryKey = ["get-proxies"] const { data: proxiesResp, @@ -109,57 +97,39 @@ export const ProxyProvider: FC = ({ children }) => { } = useQuery({ queryKey, queryFn: getWorkspaceProxies, - // This onSuccess ensures the local storage is synchronized with the - // proxies returned by coderd. If the selected proxy is not in the list, - // then the user selection is ignored. - onSuccess: (resp) => { - // Always pass in the user's choice. - setAndSaveProxy(savedProxy, resp.regions) - }, }) // Every time we get a new proxiesResponse, update the latency check // to each workspace proxy. const proxyLatencies = useProxyLatency(proxiesResp) - // If the proxies change or latencies change, we need to update the - // callback function. - const setAndSaveProxy = useCallback( - ( - selectedProxy?: Region, - // By default the proxies come from the api call above. - // Allow the caller to override this if they have a more up - // to date list of proxies. - proxies: Region[] = proxiesResp?.regions || [], - ) => { - if (!proxies) { - throw new Error( - "proxies are not yet loaded, so selecting a proxy makes no sense. How did you get here?", - ) - } - - // The preferred proxy attempts to use the user's selection if it is valid. - const preferred = getPreferredProxy( - proxies, - selectedProxy, + // updateProxy is a helper function that when called will + // update the proxy being used. + const updateProxy = useCallback(() => { + // Update the saved user proxy for the caller. + setUserSavedProxy(loadUserSelectedProxy()) + setProxy( + getPreferredProxy( + proxiesResp?.regions ?? [], + // For some reason if I use 'userSavedProxy' here, + // the tests fail. It does not load "undefined" after a + // clear, but takes the previous value. + loadUserSelectedProxy(), proxyLatencies, - ) - // Set the state for the current context. - setProxy(preferred) - }, - [proxiesResp, proxyLatencies], - ) + ), + ) + }, [userSavedProxy, proxiesResp, proxyLatencies]) // This useEffect ensures the proxy to be used is updated whenever the state changes. // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy. useEffect(() => { - setAndSaveProxy(savedProxy) - }, [savedProxy, proxiesResp, proxyLatencies, setAndSaveProxy]) + updateProxy() + }, [proxiesResp, proxyLatencies]) return ( = ({ children }) => { setProxy: (proxy: Region) => { // Save to local storage to persist the user's preference across reloads saveUserSelectedProxy(proxy) - // Set the state for the current context. - setUserProxy(proxy) + // Update the selected proxy + updateProxy() }, clearProxy: () => { // Clear the user's selection from local storage. clearUserSelectedProxy() - setUserProxy(undefined) + updateProxy() }, }} > From a3703e131438c100664d2bfb861dca41424f5352 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 18 May 2023 10:40:29 +0200 Subject: [PATCH 09/13] Fix useEffect dep --- site/src/contexts/ProxyContext.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index deafee0f19b74..240dc74d90a80 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -118,7 +118,7 @@ export const ProxyProvider: FC = ({ children }) => { proxyLatencies, ), ) - }, [userSavedProxy, proxiesResp, proxyLatencies]) + }, [proxiesResp, proxyLatencies]) // This useEffect ensures the proxy to be used is updated whenever the state changes. // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy. From a358258afdc7acd0ba54681ab8859680d81849c1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 18 May 2023 11:38:30 +0200 Subject: [PATCH 10/13] Fix deps comment --- site/src/contexts/ProxyContext.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 240dc74d90a80..32ee572430968 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -124,6 +124,7 @@ export const ProxyProvider: FC = ({ children }) => { // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy. useEffect(() => { updateProxy() + // eslint-disable-next-line react-hooks/exhaustive-deps -- Only update if the source data changes }, [proxiesResp, proxyLatencies]) return ( From ec860c31707a2472dfb1a2dc460fd062cba588c1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 22 May 2023 14:33:12 +0200 Subject: [PATCH 11/13] Remove comment --- site/src/contexts/ProxyContext.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 32ee572430968..32d3e3ae11d63 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -111,9 +111,6 @@ export const ProxyProvider: FC = ({ children }) => { setProxy( getPreferredProxy( proxiesResp?.regions ?? [], - // For some reason if I use 'userSavedProxy' here, - // the tests fail. It does not load "undefined" after a - // clear, but takes the previous value. loadUserSelectedProxy(), proxyLatencies, ), From ebb6a7617a5da0a3b8cbcc1c1b6d4d535903c8b7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 22 May 2023 16:21:45 +0200 Subject: [PATCH 12/13] Mock the hook with a hook --- site/jest.setup.ts | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/site/jest.setup.ts b/site/jest.setup.ts index 24e3ae46a7eda..237e968ab0664 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -8,6 +8,7 @@ import { Blob } from "buffer" import jestFetchMock from "jest-fetch-mock" import { ProxyLatencyReport } from "contexts/useProxyLatency" import { RegionsResponse } from "api/typesGenerated" +import { useMemo, useState } from "react" jestFetchMock.enableMocks() @@ -16,20 +17,25 @@ jestFetchMock.enableMocks() // actual network requests. So just globally mock this hook. jest.mock("contexts/useProxyLatency", () => ({ useProxyLatency: (proxies?: RegionsResponse) => { - if (!proxies) { - return {} as Record - } - - return proxies.regions.reduce((acc, proxy) => { - acc[proxy.id] = { - accurate: true, - // Return a constant latency of 8ms. - // If you make this random it could break stories. - latencyMS: 8, - at: new Date(), + // Must use `useMemo` here to avoid infinite loop. + // Mocking the hook with a hook. + const latencies = useMemo(() => { + if (!proxies) { + return {} as Record } - return acc - }, {} as Record) + return proxies.regions.reduce((acc, proxy) => { + acc[proxy.id] = { + accurate: true, + // Return a constant latency of 8ms. + // If you make this random it could break stories. + latencyMS: 8, + at: new Date(), + } + return acc + }, {} as Record) + }, [proxies]) + + return latencies }, })) From 549093e21ba2a865f7476b4301fb7adde9af145b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 22 May 2023 16:30:07 +0200 Subject: [PATCH 13/13] Remove unused dep --- site/jest.setup.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/jest.setup.ts b/site/jest.setup.ts index 237e968ab0664..86a6108f9661e 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -8,7 +8,7 @@ import { Blob } from "buffer" import jestFetchMock from "jest-fetch-mock" import { ProxyLatencyReport } from "contexts/useProxyLatency" import { RegionsResponse } from "api/typesGenerated" -import { useMemo, useState } from "react" +import { useMemo } from "react" jestFetchMock.enableMocks()