diff --git a/site/jest.polyfills.js b/site/jest.polyfills.js index 0e3653b87c..518cf6e94d 100644 --- a/site/jest.polyfills.js +++ b/site/jest.polyfills.js @@ -29,4 +29,16 @@ Object.defineProperties(globalThis, { FormData: { value: FormData }, Request: { value: Request }, Response: { value: Response }, + matchMedia: { + value: (query) => ({ + matches: false, + media: query, + onchange: null, + addListener: jest.fn(), + removeListener: jest.fn(), + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + }), + }, }); diff --git a/site/jest.setup.ts b/site/jest.setup.ts index 946cf98890..6282295870 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -63,7 +63,7 @@ beforeAll(() => afterEach(() => { cleanup(); server.resetHandlers(); - jest.clearAllMocks(); + jest.resetAllMocks(); }); // Clean up after the tests are finished. diff --git a/site/src/hooks/useClipboard-http.test.ts b/site/src/hooks/useClipboard-http.test.ts deleted file mode 100644 index ca22fc6daa..0000000000 --- a/site/src/hooks/useClipboard-http.test.ts +++ /dev/null @@ -1,15 +0,0 @@ -/** - * This test is for all useClipboard functionality, with the browser context - * set to insecure (HTTP connections). - * - * See useClipboard.test-setup.ts for more info on why this file is set up the - * way that it is. - */ -import { useClipboard } from "./useClipboard"; -import { scheduleClipboardTests } from "./useClipboard.test-setup"; - -describe(useClipboard.name, () => { - describe("HTTP (non-secure) connections", () => { - scheduleClipboardTests({ isHttps: false }); - }); -}); diff --git a/site/src/hooks/useClipboard-https.test.ts b/site/src/hooks/useClipboard-https.test.ts deleted file mode 100644 index e2e3ac264c..0000000000 --- a/site/src/hooks/useClipboard-https.test.ts +++ /dev/null @@ -1,15 +0,0 @@ -/** - * This test is for all useClipboard functionality, with the browser context - * set to secure (HTTPS connections). - * - * See useClipboard.test-setup.ts for more info on why this file is set up the - * way that it is. - */ -import { useClipboard } from "./useClipboard"; -import { scheduleClipboardTests } from "./useClipboard.test-setup"; - -describe(useClipboard.name, () => { - describe("HTTPS (secure/default) connections", () => { - scheduleClipboardTests({ isHttps: true }); - }); -}); diff --git a/site/src/hooks/useClipboard.test-setup.tsx b/site/src/hooks/useClipboard.test.tsx similarity index 93% rename from site/src/hooks/useClipboard.test-setup.tsx rename to site/src/hooks/useClipboard.test.tsx index 7de7ea7143..5ddbed3f8c 100644 --- a/site/src/hooks/useClipboard.test-setup.tsx +++ b/site/src/hooks/useClipboard.test.tsx @@ -1,3 +1,22 @@ +import { act, renderHook } from "@testing-library/react"; +import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar"; +import { ThemeProvider } from "contexts/ThemeProvider"; +import { + type UseClipboardInput, + type UseClipboardResult, + useClipboard, +} from "./useClipboard"; + +describe(useClipboard.name, () => { + describe("HTTP (non-secure) connections", () => { + scheduleClipboardTests({ isHttps: false }); + }); + + describe("HTTPS (secure/default) connections", () => { + scheduleClipboardTests({ isHttps: true }); + }); +}); + /** * @file This is a very weird test setup. * @@ -41,25 +60,6 @@ * order of operations involving closure, but you have no idea why the code * is working, and it's impossible to debug. */ -import { act, renderHook } from "@testing-library/react"; -import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar"; -import { - type UseClipboardInput, - type UseClipboardResult, - useClipboard, -} from "./useClipboard"; - -const initialExecCommand = global.document.execCommand; -beforeAll(() => { - jest.useFakeTimers(); -}); - -afterAll(() => { - jest.restoreAllMocks(); - jest.useRealTimers(); - global.document.execCommand = initialExecCommand; -}); - type MockClipboardEscapeHatches = Readonly<{ getMockText: () => string; setMockText: (newText: string) => void; @@ -124,10 +124,10 @@ function renderUseClipboard(inputs: UseClipboardInput) { { initialProps: inputs, wrapper: ({ children }) => ( - <> - <>{children} + + {children} - + ), }, ); @@ -137,9 +137,10 @@ type ScheduleConfig = Readonly<{ isHttps: boolean }>; export function scheduleClipboardTests({ isHttps }: ScheduleConfig) { const mockClipboardInstance = makeMockClipboard(isHttps); - const originalNavigator = window.navigator; - beforeAll(() => { + + beforeEach(() => { + jest.useFakeTimers(); jest.spyOn(window, "navigator", "get").mockImplementation(() => ({ ...originalNavigator, clipboard: mockClipboardInstance, @@ -170,6 +171,7 @@ export function scheduleClipboardTests({ isHttps }: ScheduleConfig) { }); afterEach(() => { + jest.useRealTimers(); mockClipboardInstance.setMockText(""); mockClipboardInstance.setSimulateFailure(false); }); diff --git a/site/src/pages/TerminalPage/TerminalAlerts.tsx b/site/src/pages/TerminalPage/TerminalAlerts.tsx index 34cea27881..b9ba60279a 100644 --- a/site/src/pages/TerminalPage/TerminalAlerts.tsx +++ b/site/src/pages/TerminalPage/TerminalAlerts.tsx @@ -1,8 +1,66 @@ import Button from "@mui/material/Button"; import Link from "@mui/material/Link"; -import { type FC, useState } from "react"; +import { type FC, useState, useEffect, useRef } from "react"; +import type { WorkspaceAgent } from "api/typesGenerated"; import { Alert, type AlertProps } from "components/Alert/Alert"; import { docs } from "utils/docs"; +import type { ConnectionStatus } from "./types"; + +type TerminalAlertsProps = { + agent: WorkspaceAgent | undefined; + status: ConnectionStatus; + onAlertChange: () => void; +}; + +export const TerminalAlerts = ({ + agent, + status, + onAlertChange, +}: TerminalAlertsProps) => { + const lifecycleState = agent?.lifecycle_state; + const prevLifecycleState = useRef(lifecycleState); + useEffect(() => { + prevLifecycleState.current = lifecycleState; + }, [lifecycleState]); + + // We want to observe the children of the wrapper to detect when the alert + // changes. So the terminal page can resize itself. + // + // Would it be possible to just always call fit() when this component + // re-renders instead of using an observer? + // + // This is a good question and the why this does not work is that the .fit() + // needs to run after the render so in this case, I just think the mutation + // observer is more reliable. I could use some hacky setTimeout inside of + // useEffect to do that, I guess, but I don't think it would be any better. + const wrapperRef = useRef(null); + useEffect(() => { + if (!wrapperRef.current) { + return; + } + const observer = new MutationObserver(onAlertChange); + observer.observe(wrapperRef.current, { childList: true }); + + return () => { + observer.disconnect(); + }; + }, [onAlertChange]); + + return ( +
+ {status === "disconnected" ? ( + + ) : lifecycleState === "start_error" ? ( + + ) : lifecycleState === "starting" ? ( + + ) : lifecycleState === "ready" && + prevLifecycleState.current === "starting" ? ( + + ) : null} +
+ ); +}; export const ErrorScriptAlert: FC = () => { return ( diff --git a/site/src/pages/TerminalPage/TerminalPage.test.tsx b/site/src/pages/TerminalPage/TerminalPage.test.tsx index 05cb4329e4..8b91060247 100644 --- a/site/src/pages/TerminalPage/TerminalPage.test.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.test.tsx @@ -8,27 +8,10 @@ import { MockWorkspace, MockWorkspaceAgent, } from "testHelpers/entities"; -import { - renderWithAuth, - waitForLoaderToBeRemoved, -} from "testHelpers/renderHelpers"; +import { renderWithAuth } from "testHelpers/renderHelpers"; import { server } from "testHelpers/server"; import TerminalPage, { Language } from "./TerminalPage"; -Object.defineProperty(window, "matchMedia", { - writable: true, - value: jest.fn().mockImplementation((query) => ({ - matches: false, - media: query, - onchange: null, - addListener: jest.fn(), // deprecated - removeListener: jest.fn(), // deprecated - addEventListener: jest.fn(), - removeEventListener: jest.fn(), - dispatchEvent: jest.fn(), - })), -}); - const renderTerminal = async ( route = `/${MockUser.username}/${MockWorkspace.name}/terminal`, ) => { @@ -36,7 +19,16 @@ const renderTerminal = async ( route, path: "/:username/:workspace/terminal", }); - await waitForLoaderToBeRemoved(); + await waitFor(() => { + // To avoid 'act' errors during testing, we ensure the component is + // completely rendered without any outstanding state updates. This is + // accomplished by incorporating a 'data-status' attribute into the + // component. We then observe this attribute for any changes, as we cannot + // rely on other screen elements to indicate completion. + const wrapper = + utils.container.querySelector("[data-status]")!; + expect(wrapper.dataset.state).not.toBe("initializing"); + }); return utils; }; @@ -58,11 +50,15 @@ const expectTerminalText = (container: HTMLElement, text: string) => { }; describe("TerminalPage", () => { + afterEach(() => { + WS.clean(); + }); + it("loads the right workspace data", async () => { - const spy = jest + jest .spyOn(API, "getWorkspaceByOwnerAndName") .mockResolvedValue(MockWorkspace); - const ws = new WS( + new WS( `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`, ); await renderTerminal( @@ -75,57 +71,45 @@ describe("TerminalPage", () => { { include_deleted: true }, ); }); - spy.mockRestore(); - ws.close(); }); it("shows an error if fetching workspace fails", async () => { - // Given server.use( http.get("/api/v2/users/:userId/workspace/:workspaceName", () => { return HttpResponse.json({ id: "workspace-id" }, { status: 500 }); }), ); - // When const { container } = await renderTerminal(); - // Then await expectTerminalText(container, Language.workspaceErrorMessagePrefix); }); it("shows an error if the websocket fails", async () => { - // Given server.use( http.get("/api/v2/workspaceagents/:agentId/pty", () => { return HttpResponse.json({}, { status: 500 }); }), ); - // When const { container } = await renderTerminal(); - // Then await expectTerminalText(container, Language.websocketErrorMessagePrefix); }); it("renders data from the backend", async () => { - // Given const ws = new WS( `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`, ); const text = "something to render"; - // When const { container } = await renderTerminal(); - - // Then // Ideally we could use ws.connected but that seems to pause React updates. // For now, wait for the initial resize message instead. await ws.nextMessage; ws.send(text); + await expectTerminalText(container, text); - ws.close(); }); // Ideally we could just pass the correct size in the web socket URL without @@ -134,40 +118,32 @@ describe("TerminalPage", () => { // in the other tests since ws.connected appears to pause React updates. So // for now the initial resize message (and this test) are here to stay. it("resizes on connect", async () => { - // Given const ws = new WS( `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`, ); - // When await renderTerminal(); - // Then const msg = await ws.nextMessage; const req = JSON.parse(new TextDecoder().decode(msg as Uint8Array)); expect(req.height).toBeGreaterThan(0); expect(req.width).toBeGreaterThan(0); - ws.close(); }); it("supports workspace.agent syntax", async () => { - // Given const ws = new WS( `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`, ); const text = "something to render"; - // When const { container } = await renderTerminal( `/some-user/${MockWorkspace.name}.${MockWorkspaceAgent.name}/terminal`, ); - // Then // Ideally we could use ws.connected but that seems to pause React updates. // For now, wait for the initial resize message instead. await ws.nextMessage; ws.send(text); await expectTerminalText(container, text); - ws.close(); }); }); diff --git a/site/src/pages/TerminalPage/TerminalPage.tsx b/site/src/pages/TerminalPage/TerminalPage.tsx index 194db9c289..73ddc1ceeb 100644 --- a/site/src/pages/TerminalPage/TerminalPage.tsx +++ b/site/src/pages/TerminalPage/TerminalPage.tsx @@ -13,7 +13,6 @@ import { WebLinksAddon } from "xterm-addon-web-links"; import { WebglAddon } from "xterm-addon-webgl"; import { deploymentConfig } from "api/queries/deployment"; import { workspaceByOwnerAndName } from "api/queries/workspaces"; -import type { WorkspaceAgent } from "api/typesGenerated"; import { useProxy } from "contexts/ProxyContext"; import { ThemeOverride } from "contexts/ThemeProvider"; import themes from "theme"; @@ -22,12 +21,8 @@ import { pageTitle } from "utils/page"; import { openMaybePortForwardedURL } from "utils/portForward"; import { terminalWebsocketUrl } from "utils/terminal"; import { getMatchingAgentOrFirst } from "utils/workspace"; -import { - DisconnectedAlert, - ErrorScriptAlert, - LoadedScriptsAlert, - LoadingScriptsAlert, -} from "./TerminalAlerts"; +import { TerminalAlerts } from "./TerminalAlerts"; +import type { ConnectionStatus } from "./types"; export const Language = { workspaceErrorMessagePrefix: "Unable to fetch workspace: ", @@ -35,8 +30,6 @@ export const Language = { websocketErrorMessagePrefix: "WebSocket failed: ", }; -type TerminalState = "connected" | "disconnected" | "initializing"; - const TerminalPage: FC = () => { // Maybe one day we'll support a light themed terminal, but terminal coloring // is notably a pain because of assumptions certain programs might make about your @@ -46,10 +39,12 @@ const TerminalPage: FC = () => { const { proxy, proxyLatencies } = useProxy(); const params = useParams() as { username: string; workspace: string }; const username = params.username.replace("@", ""); - const xtermRef = useRef(null); - const [terminal, setTerminal] = useState(null); - const [terminalState, setTerminalState] = - useState("initializing"); + const terminalWrapperRef = useRef(null); + // The terminal is maintained as a state to trigger certain effects when it + // updates. + const [terminal, setTerminal] = useState(); + const [connectionStatus, setConnectionStatus] = + useState("initializing"); const [searchParams] = useSearchParams(); const isDebugging = searchParams.has("debug"); // The reconnection token is a unique token that identifies @@ -93,7 +88,7 @@ const TerminalPage: FC = () => { // Create the terminal! const fitAddonRef = useRef(); useEffect(() => { - if (!xtermRef.current || config.isLoading) { + if (!terminalWrapperRef.current || config.isLoading) { return; } const terminal = new XTerm.Terminal({ @@ -122,7 +117,7 @@ const TerminalPage: FC = () => { }), ); - terminal.open(xtermRef.current); + terminal.open(terminalWrapperRef.current); // We have to fit twice here. It's unknown why, but the first fit will // overflow slightly in some scenarios. Applying a second fit resolves this. @@ -140,7 +135,7 @@ const TerminalPage: FC = () => { window.removeEventListener("resize", listener); terminal.dispose(); }; - }, [theme, renderer, config.isLoading, xtermRef, handleWebLinkRef]); + }, [config.isLoading, renderer, theme.palette.background.default]); // Updates the reconnection token into the URL if necessary. useEffect(() => { @@ -156,7 +151,7 @@ const TerminalPage: FC = () => { replace: true, }, ); - }, [searchParams, navigate, reconnectionToken]); + }, [navigate, reconnectionToken, searchParams]); // Hook up the terminal through a web socket. useEffect(() => { @@ -182,12 +177,14 @@ const TerminalPage: FC = () => { terminal.writeln( Language.workspaceErrorMessagePrefix + workspace.error.message, ); + setConnectionStatus("disconnected"); return; } else if (!workspaceAgent) { terminal.writeln( Language.workspaceAgentErrorMessagePrefix + "no agent found with ID, is the workspace started?", ); + setConnectionStatus("disconnected"); return; } @@ -243,18 +240,18 @@ const TerminalPage: FC = () => { }), ), ); - setTerminalState("connected"); + setConnectionStatus("connected"); }); websocket.addEventListener("error", () => { terminal.options.disableStdin = true; terminal.writeln( Language.websocketErrorMessagePrefix + "socket errored", ); - setTerminalState("disconnected"); + setConnectionStatus("disconnected"); }); websocket.addEventListener("close", () => { terminal.options.disableStdin = true; - setTerminalState("disconnected"); + setConnectionStatus("disconnected"); }); websocket.addEventListener("message", (event) => { if (typeof event.data === "string") { @@ -271,7 +268,7 @@ const TerminalPage: FC = () => { return; // Unmounted while we waited for the async call. } terminal.writeln(Language.websocketErrorMessagePrefix + error.message); - setTerminalState("disconnected"); + setConnectionStatus("disconnected"); }); return () => { @@ -284,8 +281,8 @@ const TerminalPage: FC = () => { proxy.preferredPathAppURL, reconnectionToken, terminal, - workspace.isLoading, workspace.error, + workspace.isLoading, workspaceAgent, ]); @@ -300,15 +297,22 @@ const TerminalPage: FC = () => { : ""} -
+
{ fitAddonRef.current?.fit(); }} /> -
+
{latency && isDebugging && ( @@ -328,62 +332,6 @@ const TerminalPage: FC = () => { ); }; -type TerminalAlertsProps = { - agent: WorkspaceAgent | undefined; - state: TerminalState; - onAlertChange: () => void; -}; - -const TerminalAlerts = ({ - agent, - state, - onAlertChange, -}: TerminalAlertsProps) => { - const lifecycleState = agent?.lifecycle_state; - const prevLifecycleState = useRef(lifecycleState); - useEffect(() => { - prevLifecycleState.current = lifecycleState; - }, [lifecycleState]); - - // We want to observe the children of the wrapper to detect when the alert - // changes. So the terminal page can resize itself. - // - // Would it be possible to just always call fit() when this component - // re-renders instead of using an observer? - // - // This is a good question and the why this does not work is that the .fit() - // needs to run after the render so in this case, I just think the mutation - // observer is more reliable. I could use some hacky setTimeout inside of - // useEffect to do that, I guess, but I don't think it would be any better. - const wrapperRef = useRef(null); - useEffect(() => { - if (!wrapperRef.current) { - return; - } - const observer = new MutationObserver(onAlertChange); - observer.observe(wrapperRef.current, { childList: true }); - - return () => { - observer.disconnect(); - }; - }, [onAlertChange]); - - return ( -
- {state === "disconnected" ? ( - - ) : lifecycleState === "start_error" ? ( - - ) : lifecycleState === "starting" ? ( - - ) : lifecycleState === "ready" && - prevLifecycleState.current === "starting" ? ( - - ) : null} -
- ); -}; - const styles = { terminal: (theme) => ({ width: "100%", diff --git a/site/src/pages/TerminalPage/types.ts b/site/src/pages/TerminalPage/types.ts new file mode 100644 index 0000000000..1a8fe3a68e --- /dev/null +++ b/site/src/pages/TerminalPage/types.ts @@ -0,0 +1 @@ +export type ConnectionStatus = "connected" | "disconnected" | "initializing"; diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index d79d958846..3697e61360 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -22,6 +22,14 @@ export function createTestQueryClient() { // Helps create one query client for each test case, to make sure that tests // are isolated and can't affect each other return new QueryClient({ + logger: { + ...console, + // Some tests are designed to throw errors as part of their functionality. + // To avoid unnecessary noise from these expected errors, the code is + // structured to suppress them. If this suppression becomes problematic, + // the code can be refactored to handle query errors on a per-test basis. + error: () => {}, + }, defaultOptions: { queries: { retry: false,