From 94f229c3768b25f812382a542f72886a7e804888 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Mon, 31 Aug 2026 14:22:43 +0300 Subject: [PATCH] perf: have the next screenshot ready before the reviewer asks for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stepping through a build's runs with the arrows left the pane blank under a "Loading..." for a second or more per step: each screenshot is several megapixels, and nothing was fetched until the reviewer had already moved. The dialog now warms the browser cache for the runs on either side of the one on screen — two ahead, one behind, since review moves forward, nearest first so the run the arrow lands on is requested first. Names already asked for are not asked for again. Prefetching only pays if the browser is allowed to keep what it fetched, so nginx now serves the image directory with a year of immutable caching: an image name is unique per upload and the bytes behind it are never rewritten. That needed its own location block, which also stops a missing image falling through to index.html the way the SPA route does — the app's HTML answered under an image's name and then cached for a year would have been a far worse bug than the 404 it now returns. --- nginx/default.conf | 13 +++ src/_helpers/imagePrefetch.helper.test.ts | 118 +++++++++++++++++++++ src/_helpers/imagePrefetch.helper.ts | 62 +++++++++++ src/components/TestDetailsDialog/index.tsx | 15 +++ 4 files changed, 208 insertions(+) create mode 100644 src/_helpers/imagePrefetch.helper.test.ts create mode 100644 src/_helpers/imagePrefetch.helper.ts diff --git a/nginx/default.conf b/nginx/default.conf index 9d010353..58f2bedb 100644 --- a/nginx/default.conf +++ b/nginx/default.conf @@ -5,6 +5,19 @@ server { return 403; } + # Screenshots, baselines and diffs. Their names are unique per upload and the + # bytes behind a name are never rewritten, so the browser may keep them: going + # back to a screen already reviewed then costs no request at all. + # + # This must not fall through to index.html the way the SPA route below does — + # a missing image would be answered with the app's HTML and then cached under + # the image's name for a year. + location /static/imageUploads/ { + root /usr/share/nginx/html; + add_header Cache-Control "public, max-age=31536000, immutable"; + try_files $uri =404; + } + location / { root /usr/share/nginx/html; index index.html index.htm; diff --git a/src/_helpers/imagePrefetch.helper.test.ts b/src/_helpers/imagePrefetch.helper.test.ts new file mode 100644 index 00000000..42191bfc --- /dev/null +++ b/src/_helpers/imagePrefetch.helper.test.ts @@ -0,0 +1,118 @@ +import { run } from "./testRun.fixture"; +import { neighbourImageNames, prefetchImages } from "./imagePrefetch.helper"; +import { staticService } from "../services"; + +const at = (index: number) => + run({ + id: `run-${index}`, + baselineName: `baseline-${index}.png`, + imageName: `image-${index}.png`, + diffName: `diff-${index}.png`, + }); + +const list = (length: number) => + Array.from({ length }, (_unused, index) => at(index)); + +describe("neighbourImageNames", () => { + it("asks for the run ahead before the one behind, since review moves forward", () => { + const names = neighbourImageNames(list(5), 2); + + expect(names.indexOf("image-3.png")).toBeLessThan( + names.indexOf("image-1.png"), + ); + }); + + it("leaves out the run being looked at — it is already loading", () => { + const names = neighbourImageNames(list(5), 2); + + expect(names).not.toContain("image-2.png"); + expect(names).not.toContain("baseline-2.png"); + expect(names).not.toContain("diff-2.png"); + }); + + it("takes all three pictures of a neighbour", () => { + const names = neighbourImageNames(list(3), 0); + + expect(names).toEqual( + expect.arrayContaining(["baseline-1.png", "image-1.png", "diff-1.png"]), + ); + }); + + it("stops at the start of the list", () => { + const names = neighbourImageNames(list(5), 0); + + expect(names).not.toContain("image-4.png"); + expect(names).toContain("image-1.png"); + }); + + it("stops at the end of the list", () => { + const names = neighbourImageNames(list(3), 2); + + expect(names).toEqual( + expect.arrayContaining(["image-1.png", "baseline-1.png"]), + ); + expect(names).toHaveLength(3); + }); + + it("skips a run that has no diff yet", () => { + const runs = [at(0), run({ id: "run-1", diffName: null as never })]; + + expect(neighbourImageNames(runs, 0)).not.toContain(null); + }); + + it("asks for a name shared by two runs only once", () => { + const shared = run({ id: "shared", baselineName: "same-baseline.png" }); + const runs = [at(0), { ...shared, id: "a" }, { ...shared, id: "b" }]; + + const names = neighbourImageNames(runs, 0); + + expect(names.filter((name) => name === "same-baseline.png")).toHaveLength( + 1, + ); + }); + + it("has nothing to fetch when the run is not in the list", () => { + expect(neighbourImageNames(list(3), -1)).toEqual([]); + }); +}); + +describe("prefetchImages", () => { + let requestedSrcs: string[]; + let originalImage: typeof window.Image; + + beforeEach(() => { + requestedSrcs = []; + originalImage = window.Image; + // jsdom will not load anything, but it does record what was asked for + window.Image = class { + set src(value: string) { + requestedSrcs.push(value); + } + } as unknown as typeof window.Image; + }); + + afterEach(() => { + window.Image = originalImage; + }); + + it("requests every name through the same URL the dialog will use", () => { + prefetchImages(["a.png", "b.png"], new Set()); + + expect(requestedSrcs).toEqual([ + staticService.getImage("a.png"), + staticService.getImage("b.png"), + ]); + }); + + it("does not ask twice for a name it already requested", () => { + const alreadyRequested = new Set(); + + prefetchImages(["a.png"], alreadyRequested); + prefetchImages(["a.png", "b.png"], alreadyRequested); + + expect(requestedSrcs).toEqual([ + staticService.getImage("a.png"), + staticService.getImage("b.png"), + ]); + }); +}); diff --git a/src/_helpers/imagePrefetch.helper.ts b/src/_helpers/imagePrefetch.helper.ts new file mode 100644 index 00000000..73e5c681 --- /dev/null +++ b/src/_helpers/imagePrefetch.helper.ts @@ -0,0 +1,62 @@ +import { TestRun } from "../types"; +import { staticService } from "../services"; + +// How far around the reviewed run the dialog warms the browser cache. Review +// moves forward, so it reaches further ahead than behind; each run costs three +// requests, which is why the window is small. +export const PREFETCH_AHEAD = 2; +export const PREFETCH_BEHIND = 1; + +/** + * The pictures of the runs on either side of the one being reviewed, nearest + * first and ahead before behind, so the run the arrow key lands on next is + * requested first. Deduped, and empty names (a run compared to no baseline, or + * not compared yet) are left out. + */ +export const neighbourImageNames = ( + testRuns: TestRun[], + index: number, +): string[] => { + if (index < 0 || index >= testRuns.length) { + return []; + } + + const offsets: number[] = []; + for ( + let step = 1; + step <= Math.max(PREFETCH_AHEAD, PREFETCH_BEHIND); + step++ + ) { + if (step <= PREFETCH_AHEAD) offsets.push(step); + if (step <= PREFETCH_BEHIND) offsets.push(-step); + } + + const names = new Set(); + offsets.forEach((offset) => { + const neighbour = testRuns[index + offset]; + if (!neighbour) return; + [neighbour.baselineName, neighbour.imageName, neighbour.diffName] + .filter((name): name is string => !!name) + .forEach((name) => names.add(name)); + }); + + return [...names]; +}; + +/** + * Warms the browser cache for the given image names, through the very URL the + * dialog will ask for, so arrow navigation shows a picture instead of a + * spinner. `alreadyRequested` is carried by the caller across renders — a + * second request for the same name would be wasted work. + */ +export const prefetchImages = ( + names: string[], + alreadyRequested: Set, +): void => { + names.forEach((name) => { + if (alreadyRequested.has(name)) return; + alreadyRequested.add(name); + const image = new Image(); + image.src = staticService.getImage(name); + }); +}; diff --git a/src/components/TestDetailsDialog/index.tsx b/src/components/TestDetailsDialog/index.tsx index 078a7d64..9037fad6 100644 --- a/src/components/TestDetailsDialog/index.tsx +++ b/src/components/TestDetailsDialog/index.tsx @@ -7,6 +7,10 @@ import { BaseModal } from "../BaseModal"; import TestDetailsModal from "./TestDetailsModal"; import { TestRun } from "../../types"; import { makeStyles } from "@mui/styles"; +import { + neighbourImageNames, + prefetchImages, +} from "../../_helpers/imagePrefetch.helper"; const useStyles = makeStyles(() => ({ modal: { @@ -48,6 +52,17 @@ export const TestDetailsDialog: React.FunctionComponent = () => { [testRuns, selectedTestRun?.id], ); + // Screenshots are megapixels each, so stepping to the next run used to leave + // the pane blank under a "Loading..." while the browser fetched and decoded + // them. Fetch the neighbours' pictures while the reviewer looks at this one. + const prefetched = React.useRef>(new Set()); + React.useEffect(() => { + prefetchImages( + neighbourImageNames(testRuns, selectedTestRunIndex), + prefetched.current, + ); + }, [testRuns, selectedTestRunIndex]); + const navigateById = React.useCallback( (id?: string) => { if (touched) {