From 1adeb3621a453b23c67b501e9b37099eb2569fb9 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 30 Sep 2024 18:10:47 +0000 Subject: [PATCH 1/6] fix(site): fix build logs scrolling on safari --- .../WorkspaceBuildPageView.tsx | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index c89ae6ad8fa59..4c2bae7170df2 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -24,7 +24,7 @@ import { WorkspaceBuildDataSkeleton, } from "modules/workspaces/WorkspaceBuildData/WorkspaceBuildData"; import { WorkspaceBuildLogs } from "modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs"; -import type { FC } from "react"; +import { useLayoutEffect, useRef, type FC } from "react"; import { Link } from "react-router-dom"; import { displayWorkspaceBuildDuration } from "utils/workspace"; import { Sidebar, SidebarCaption, SidebarItem } from "./Sidebar"; @@ -57,6 +57,34 @@ export const WorkspaceBuildPageView: FC = ({ defaultValue: "build", }); + // TODO: Use only CSS to set the height of the content. + // Note: On Safari, when content is rendered inside a flex container and needs + // to scroll, the parent container must have a height set. Achieving this may + // require significant refactoring of the layout components where we currently + // use height and min-height set to 100%. + // Issue: https://github.com/coder/coder/issues/9687 + // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari + const contentRef = useRef(null); + useLayoutEffect(() => { + const contentEl = contentRef.current; + if (!contentEl) { + return; + } + + const resizeObserver = new ResizeObserver(() => { + const parentEl = contentEl.parentElement; + if (!parentEl) { + return; + } + contentEl.style.setProperty("height", `${parentEl.clientHeight}px`); + }); + resizeObserver.observe(document.body); + + return () => { + resizeObserver.disconnect(); + }; + }, []); + if (!build) { return ; } @@ -144,7 +172,10 @@ export const WorkspaceBuildPageView: FC = ({ ))} -
+
From c81a42b0de007f1bf8b4056a008bf4d7b54b2483 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 1 Oct 2024 14:02:03 +0000 Subject: [PATCH 2/6] Extract scroll area to encapsulate effect and state changes --- .../WorkspaceBuildPageView.tsx | 74 ++++++++++--------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 4c2bae7170df2..369c88a2c238d 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -24,7 +24,7 @@ import { WorkspaceBuildDataSkeleton, } from "modules/workspaces/WorkspaceBuildData/WorkspaceBuildData"; import { WorkspaceBuildLogs } from "modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs"; -import { useLayoutEffect, useRef, type FC } from "react"; +import { HTMLProps, useLayoutEffect, useRef, type FC } from "react"; import { Link } from "react-router-dom"; import { displayWorkspaceBuildDuration } from "utils/workspace"; import { Sidebar, SidebarCaption, SidebarItem } from "./Sidebar"; @@ -57,34 +57,6 @@ export const WorkspaceBuildPageView: FC = ({ defaultValue: "build", }); - // TODO: Use only CSS to set the height of the content. - // Note: On Safari, when content is rendered inside a flex container and needs - // to scroll, the parent container must have a height set. Achieving this may - // require significant refactoring of the layout components where we currently - // use height and min-height set to 100%. - // Issue: https://github.com/coder/coder/issues/9687 - // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari - const contentRef = useRef(null); - useLayoutEffect(() => { - const contentEl = contentRef.current; - if (!contentEl) { - return; - } - - const resizeObserver = new ResizeObserver(() => { - const parentEl = contentEl.parentElement; - if (!parentEl) { - return; - } - contentEl.style.setProperty("height", `${parentEl.clientHeight}px`); - }); - resizeObserver.observe(document.body); - - return () => { - resizeObserver.disconnect(); - }; - }, []); - if (!build) { return ; } @@ -172,10 +144,7 @@ export const WorkspaceBuildPageView: FC = ({ ))} -
+ @@ -228,12 +197,49 @@ export const WorkspaceBuildPageView: FC = ({ agent={selectedAgent!} /> )} -
+
); }; +const ScrollArea: FC> = () => { + // TODO: Use only CSS to set the height of the content. + // Note: On Safari, when content is rendered inside a flex container and needs + // to scroll, the parent container must have a height set. Achieving this may + // require significant refactoring of the layout components where we currently + // use height and min-height set to 100%. + // Issue: https://github.com/coder/coder/issues/9687 + // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari + const contentRef = useRef(null); + useLayoutEffect(() => { + const contentEl = contentRef.current; + if (!contentEl) { + return; + } + + const resizeObserver = new ResizeObserver(() => { + const parentEl = contentEl.parentElement; + if (!parentEl) { + return; + } + contentEl.style.setProperty("height", `${parentEl.clientHeight}px`); + }); + resizeObserver.observe(document.body); + + return () => { + resizeObserver.disconnect(); + }; + }, []); + + return ( +
+ ); +}; + const BuildLogsContent: FC<{ logs?: ProvisionerJobLog[] }> = ({ logs }) => { if (!logs) { return ; From 170bc1c2a4e780313c2a3ac2af1031b39ddb31d0 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 2 Oct 2024 16:08:38 +0000 Subject: [PATCH 3/6] Use state to set height --- .../WorkspaceBuildPageView.tsx | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 369c88a2c238d..c38d0b0515324 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -24,7 +24,14 @@ import { WorkspaceBuildDataSkeleton, } from "modules/workspaces/WorkspaceBuildData/WorkspaceBuildData"; import { WorkspaceBuildLogs } from "modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs"; -import { HTMLProps, useLayoutEffect, useRef, type FC } from "react"; +import { + type CSSProperties, + type FC, + type HTMLProps, + useLayoutEffect, + useRef, + useState, +} from "react"; import { Link } from "react-router-dom"; import { displayWorkspaceBuildDuration } from "utils/workspace"; import { Sidebar, SidebarCaption, SidebarItem } from "./Sidebar"; @@ -212,6 +219,7 @@ const ScrollArea: FC> = () => { // Issue: https://github.com/coder/coder/issues/9687 // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari const contentRef = useRef(null); + const [height, setHeight] = useState("100%"); useLayoutEffect(() => { const contentEl = contentRef.current; if (!contentEl) { @@ -223,7 +231,7 @@ const ScrollArea: FC> = () => { if (!parentEl) { return; } - contentEl.style.setProperty("height", `${parentEl.clientHeight}px`); + setHeight(parentEl.clientHeight); }); resizeObserver.observe(document.body); @@ -233,10 +241,7 @@ const ScrollArea: FC> = () => { }, []); return ( -
+
); }; From bbb93bb0a41db9f6369b1e719631df72c01c55a9 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 3 Oct 2024 14:31:30 +0000 Subject: [PATCH 4/6] Use height instead of width --- site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index c38d0b0515324..37d2318d6506a 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -219,7 +219,7 @@ const ScrollArea: FC> = () => { // Issue: https://github.com/coder/coder/issues/9687 // Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari const contentRef = useRef(null); - const [height, setHeight] = useState("100%"); + const [height, setHeight] = useState("100%"); useLayoutEffect(() => { const contentEl = contentRef.current; if (!contentEl) { From 7a5f53effc8d4c79c2773f7cd13e847e7909e594 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 3 Oct 2024 14:53:41 +0000 Subject: [PATCH 5/6] Try to fix test --- .../pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 37d2318d6506a..b47c3b09e5165 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -221,6 +221,14 @@ const ScrollArea: FC> = () => { const contentRef = useRef(null); const [height, setHeight] = useState("100%"); useLayoutEffect(() => { + // TODO: Mock ResizeObserver in the test environment. This is a temporary + // workaround because the recommended way to mock ResizeObserver, as + // described in the following reference, did not work: + // https://github.com/ZeeCoder/use-resize-observer/issues/40#issuecomment-991256805 + if (window.ResizeObserver === undefined) { + return; + } + const contentEl = contentRef.current; if (!contentEl) { return; From 01e66ed1afb37da966f51dc210d7a460aa45d501 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 3 Oct 2024 16:09:06 +0000 Subject: [PATCH 6/6] Fix tests --- site/jest.setup.ts | 70 ++++++++++--------- site/package.json | 1 + site/pnpm-lock.yaml | 8 +++ .../WorkspaceBuildPage.test.tsx | 4 +- .../WorkspaceBuildPageView.tsx | 16 ++--- 5 files changed, 54 insertions(+), 45 deletions(-) diff --git a/site/jest.setup.ts b/site/jest.setup.ts index 40bb92fa44965..7a06ebba2592f 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -11,30 +11,30 @@ import { server } from "testHelpers/server"; // This would fail unit testing, or at least make it very slow with // actual network requests. So just globally mock this hook. jest.mock("contexts/useProxyLatency", () => ({ - useProxyLatency: (proxies?: Region[]) => { - // Must use `useMemo` here to avoid infinite loop. - // Mocking the hook with a hook. - const proxyLatencies = useMemo(() => { - if (!proxies) { - return {} as Record; - } - return proxies.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]); + useProxyLatency: (proxies?: Region[]) => { + // Must use `useMemo` here to avoid infinite loop. + // Mocking the hook with a hook. + const proxyLatencies = useMemo(() => { + if (!proxies) { + return {} as Record; + } + return proxies.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 { proxyLatencies, refetch: jest.fn() }; - }, + return { proxyLatencies, refetch: jest.fn() }; + }, })); global.scrollTo = jest.fn(); @@ -43,28 +43,30 @@ window.HTMLElement.prototype.scrollIntoView = jest.fn(); window.open = jest.fn(); navigator.sendBeacon = jest.fn(); +global.ResizeObserver = require("resize-observer-polyfill"); + // Polyfill the getRandomValues that is used on utils/random.ts Object.defineProperty(global.self, "crypto", { - value: { - getRandomValues: function (buffer: Buffer) { - return crypto.randomFillSync(buffer); - }, - }, + value: { + getRandomValues: function (buffer: Buffer) { + return crypto.randomFillSync(buffer); + }, + }, }); // Establish API mocking before all tests through MSW. beforeAll(() => - server.listen({ - onUnhandledRequest: "warn", - }), + server.listen({ + onUnhandledRequest: "warn", + }), ); // Reset any request handlers that we may add during the tests, // so they don't affect other tests. afterEach(() => { - cleanup(); - server.resetHandlers(); - jest.resetAllMocks(); + cleanup(); + server.resetHandlers(); + jest.resetAllMocks(); }); // Clean up after the tests are finished. diff --git a/site/package.json b/site/package.json index 62685e5e068c2..69fe9082670f8 100644 --- a/site/package.json +++ b/site/package.json @@ -90,6 +90,7 @@ "react-virtualized-auto-sizer": "1.0.24", "react-window": "1.8.10", "remark-gfm": "4.0.0", + "resize-observer-polyfill": "1.5.1", "rollup-plugin-visualizer": "5.12.0", "semver": "7.6.2", "tzdata": "1.0.40", diff --git a/site/pnpm-lock.yaml b/site/pnpm-lock.yaml index 6004c925c22c7..ff85ed26e16ed 100644 --- a/site/pnpm-lock.yaml +++ b/site/pnpm-lock.yaml @@ -183,6 +183,9 @@ importers: remark-gfm: specifier: 4.0.0 version: 4.0.0 + resize-observer-polyfill: + specifier: 1.5.1 + version: 1.5.1 rollup-plugin-visualizer: specifier: 5.12.0 version: 5.12.0(rollup@4.20.0) @@ -6079,6 +6082,9 @@ packages: requires-port@1.0.0: resolution: {integrity: sha512-KigOCHcocU3XODJxsu8i/j8T9tzT4adHiecwORRQ0ZZFcp7ahwXuRU1m+yuO90C5ZUyGeGfocHDI14M3L3yDAQ==} + resize-observer-polyfill@1.5.1: + resolution: {integrity: sha512-LwZrotdHOo12nQuZlHEmtuXdqGoOD0OhaxopaNFxWzInpEgaLWoVuAMbTzixuosCx2nEG58ngzW3vxdWoxIgdg==} + resolve-cwd@3.0.0: resolution: {integrity: sha512-OrZaX2Mb+rJCpH/6CpSqt9xFVpN++x01XnN2ie9g6P5/3xelLAkXWVADpdz1IHD/KFfEXyE6V0U01OQ3UO2rEg==} engines: {node: '>=8'} @@ -14062,6 +14068,8 @@ snapshots: requires-port@1.0.0: {} + resize-observer-polyfill@1.5.1: {} + resolve-cwd@3.0.0: dependencies: resolve-from: 5.0.0 diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPage.test.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPage.test.tsx index d4108484e1a6a..5e1fe45629c45 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPage.test.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPage.test.tsx @@ -65,7 +65,9 @@ describe("WorkspaceBuildPage", () => { test("shows selected agent logs", async () => { const server = new WS( - `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/logs?follow&after=0`, + `ws://localhost/api/v2/workspaceagents/${ + MockWorkspaceAgent.id + }/logs?follow&after=0`, ); renderWithAuth(, { route: `/@${MockWorkspace.owner_name}/${MockWorkspace.name}/builds/${MockWorkspace.latest_build.build_number}?${LOGS_TAB_KEY}=${MockWorkspaceAgent.id}`, diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index b47c3b09e5165..0a51fa0e2b585 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -210,7 +210,7 @@ export const WorkspaceBuildPageView: FC = ({ ); }; -const ScrollArea: FC> = () => { +const ScrollArea: FC> = (props) => { // TODO: Use only CSS to set the height of the content. // Note: On Safari, when content is rendered inside a flex container and needs // to scroll, the parent container must have a height set. Achieving this may @@ -221,14 +221,6 @@ const ScrollArea: FC> = () => { const contentRef = useRef(null); const [height, setHeight] = useState("100%"); useLayoutEffect(() => { - // TODO: Mock ResizeObserver in the test environment. This is a temporary - // workaround because the recommended way to mock ResizeObserver, as - // described in the following reference, did not work: - // https://github.com/ZeeCoder/use-resize-observer/issues/40#issuecomment-991256805 - if (window.ResizeObserver === undefined) { - return; - } - const contentEl = contentRef.current; if (!contentEl) { return; @@ -249,7 +241,11 @@ const ScrollArea: FC> = () => { }, []); return ( -
+
); };