perf: have the next screenshot ready before the reviewer asks for it

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;

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<string>();
+
+    prefetchImages(["a.png"], alreadyRequested);
+    prefetchImages(["a.png", "b.png"], alreadyRequested);
+
+    expect(requestedSrcs).toEqual([
+      staticService.getImage("a.png"),
+      staticService.getImage("b.png"),
+    ]);
+  });
+});

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<string>();
+  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<string>,
+): void => {
+  names.forEach((name) => {
+    if (alreadyRequested.has(name)) return;
+    alreadyRequested.add(name);
+    const image = new Image();
+    image.src = staticService.getImage(name);
+  });
+};

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<Set<string>>(new Set());
+  React.useEffect(() => {
+    prefetchImages(
+      neighbourImageNames(testRuns, selectedTestRunIndex),
+      prefetched.current,
+    );
+  }, [testRuns, selectedTestRunIndex]);
+
   const navigateById = React.useCallback(
     (id?: string) => {
       if (touched) {