Merge pull request #1295 from PicPeak/fix/post-merge-followups

fix(gallery): image-loading follow-ups — pre-load band, decode release, sanitizer dedup
2026-09-05 23:36:45 +02:00
parent 1f316ef91c 1151e96144
commit 905fc595e3
8 changed files with 557 additions and 45 deletions
@@ -78,14 +78,16 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
  const imageRef = useRef<HTMLImageElement | null>(null);

  // Draw image to canvas when canvas rendering is enabled
+  // Returns whether the pixels actually made it onto the canvas, so the
+  // caller knows if the source image is still needed (#1287).
  const drawToCanvas = useCallback(() => {
-    if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return;
+    if (!useCanvasRendering || !canvasRef.current || !imageRef.current) return false;

    const canvas = canvasRef.current;
    const img = imageRef.current;
    const ctx = canvas.getContext('2d');

-    if (!ctx || !img.complete || img.naturalWidth === 0) return;
+    if (!ctx || !img.complete || img.naturalWidth === 0) return false;

    // Set canvas dimensions to match image
    canvas.width = img.naturalWidth;
@@ -95,6 +97,7 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
    ctx.drawImage(img, 0, 0);

    setCanvasReady(true);
+    return true;
  }, [useCanvasRendering]);

  useEffect(() => {
@@ -251,7 +254,26 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({

    img.onload = () => {
      imageRef.current = img;
-      drawToCanvas();
+      const drawn = drawToCanvas();
+      // Once drawImage has copied the pixels into the canvas the source
+      // decode is dead weight, so drop it here rather than at unmount. The
+      // grid is not virtualised — a 546-photo event mounts 546 of these and
+      // none of them unmount while the gallery is open — so a cleanup-only
+      // release never actually runs for the case it was meant to fix
+      // (#1287). Nothing redraws from `imageRef` afterwards: drawToCanvas
+      // has this one caller.
+      if (drawn) {
+        // Handlers off BEFORE the src goes. Measured in Chromium and WebKit:
+        // neither fires `error` when the attribute is removed after a
+        // successful load, so this is not fixing an observed bug — but if any
+        // engine ever did, `onerror` would set canvasFailed, swap the canvas
+        // for a plain <img>, and decode the image a second time, which is the
+        // exact opposite of what this release is for. The ordering is free.
+        img.onload = null;
+        img.onerror = null;
+        imageRef.current = null;
+        img.removeAttribute('src');
+      }
      onLoad?.();
    };

@@ -266,6 +288,17 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
    return () => {
      img.onload = null;
      img.onerror = null;
+      // Fallback release for the paths the onload handler above cannot
+      // cover: the draw failed, or the source changed / the component
+      // unmounted before onload ever fired. `imageRef` is what drawToCanvas
+      // reads and it was never cleared, so a detached Image — and the decode
+      // behind it — stayed pinned by a live JS reference. A decoded <img> in
+      // the document is evictable under memory pressure; one held by a ref
+      // is not.
+      if (imageRef.current === img) {
+        imageRef.current = null;
+      }
+      img.removeAttribute('src');
    };
  }, [imageSrc, useCanvasRendering, drawToCanvas, onLoad]);

@@ -0,0 +1,135 @@
+/**
+ * Canvas-mode memory release (#1287).
+ *
+ * In canvas mode the component keeps a detached `Image` in `imageRef` so
+ * `drawToCanvas` can read it. The effect cleanup nulled `onload`/`onerror`
+ * but never cleared that ref, so the Image — and the decoded bitmap behind
+ * it — stayed pinned by a live JS reference for the component's lifetime.
+ *
+ * That is not academic at gallery scale. The photo grid is NOT virtualised:
+ * a 546-photo event mounts 546 of these and none ever unmount, so nothing was
+ * ever released. A decoded <img> in the document is evictable under memory
+ * pressure; one held by a ref is not.
+ */
+import { render, waitFor } from '@testing-library/react';
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+
+vi.mock('../../../utils/galleryAuthStorage', () => ({
+  getActiveGallerySlug: () => 'demo',
+  getGalleryToken: () => 'token',
+  inferGallerySlugFromLocation: () => 'demo',
+  resolveSlugFromRequestUrl: () => 'demo',
+}));
+vi.mock('../../../utils/url', () => ({ buildResourceUrl: (u: string) => `http://localhost${u}` }));
+
+import { AuthenticatedImage } from '../AuthenticatedImage';
+
+/** Every Image the component constructs, so the test can inspect them. */
+const created: HTMLImageElement[] = [];
+let createObjectURL: ReturnType<typeof vi.fn>;
+let revokeObjectURL: ReturnType<typeof vi.fn>;
+
+beforeEach(() => {
+  created.length = 0;
+  vi.stubGlobal('fetch', vi.fn(async () => ({
+    ok: true,
+    blob: async () => new Blob(['x'], { type: 'image/png' }),
+  })));
+  // Patch the two methods rather than replacing URL — spreading the
+  // constructor loses its prototype and breaks every `new URL(...)`.
+  // Unique per call: the canvas effect keys off `imageSrc`, so a constant
+  // URL would make a src change look like no change at all.
+  let n = 0;
+  createObjectURL = vi.fn(() => `blob:mock-url-${++n}`);
+  revokeObjectURL = vi.fn();
+  URL.createObjectURL = createObjectURL as unknown as typeof URL.createObjectURL;
+  URL.revokeObjectURL = revokeObjectURL as unknown as typeof URL.revokeObjectURL;
+
+  const RealImage = globalThis.Image;
+  vi.stubGlobal('Image', class extends RealImage {
+    constructor() {
+      super();
+      created.push(this as unknown as HTMLImageElement);
+      // jsdom leaves these at 0/false for a blob: src, which makes
+      // drawToCanvas bail before it draws. Present a decoded image so the
+      // draw path is reachable.
+      Object.defineProperty(this, 'complete', { get: () => true });
+      Object.defineProperty(this, 'naturalWidth', { get: () => 10 });
+      Object.defineProperty(this, 'naturalHeight', { get: () => 10 });
+      // jsdom never fires load for a blob: src, so drive it manually.
+      setTimeout(() => this.onload?.(new Event('load')), 0);
+    }
+  });
+});
+
+afterEach(() => vi.unstubAllGlobals());
+
+describe('AuthenticatedImage canvas mode', () => {
+  it('releases the decoded image as soon as it is drawn, without waiting for unmount', async () => {
+    // The case this whole change exists for. Every other test here asserts
+    // release on unmount or src change — neither of which happens to a grid
+    // tile, because the grid is not virtualised and the tiles stay mounted
+    // for as long as the gallery is open. Once drawImage has copied the
+    // pixels the source decode is dead weight and must go immediately.
+    const ctx = { drawImage: vi.fn() };
+    const getContext = vi
+      .spyOn(HTMLCanvasElement.prototype, 'getContext')
+      .mockReturnValue(ctx as unknown as CanvasRenderingContext2D);
+
+    render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+
+    await waitFor(() => expect(created.length).toBeGreaterThan(0));
+    const img = created[0];
+
+    await waitFor(() => expect(ctx.drawImage).toHaveBeenCalled());
+    // Still mounted, still the same src — and already released.
+    await waitFor(() => expect(img.getAttribute('src')).toBeNull());
+
+    getContext.mockRestore();
+  });
+
+  it('releases the decoded image on unmount', async () => {
+    const { unmount } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+
+    await waitFor(() => expect(created.length).toBeGreaterThan(0));
+    const img = created[0];
+
+    unmount();
+
+    // The src is dropped so the browser can reclaim the decode without
+    // waiting for GC, and the handlers are detached.
+    expect(img.getAttribute('src')).toBeNull();
+    expect(img.onload).toBeNull();
+    expect(img.onerror).toBeNull();
+  });
+
+  it('revokes the blob URL on unmount', async () => {
+    const { unmount } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+
+    await waitFor(() => expect(created.length).toBeGreaterThan(0));
+    unmount();
+
+    expect(revokeObjectURL).toHaveBeenCalledWith('blob:mock-url-1');
+  });
+
+  it('releases the previous image when the src changes', async () => {
+    // A recycled tile (a layout reusing a component instance for a different
+    // photo) must not accumulate one pinned decode per photo it has shown.
+    const { rerender } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+    await waitFor(() => expect(created.length).toBe(1));
+    const first = created[0];
+
+    rerender(<AuthenticatedImage src="/api/gallery/demo/thumbnail/2" alt="t" useCanvasRendering />);
+    await waitFor(() => expect(created.length).toBe(2));
+
+    expect(first.getAttribute('src')).toBeNull();
+  });
+});
@@ -83,6 +83,24 @@ const GridPhoto: React.FC<GridPhotoProps> = ({
      onToggleSelect={onToggleSelect}
      className={`photo-card relative group cursor-pointer aspect-square ${animationClass}`}
      lazy
+      /*
+       * Pre-load band (#1287). Grid was the only lazy layout passing no
+       * `inViewRootMargin`, so PhotoCard ran the observer at the
+       * IntersectionObserver default of 0px with threshold 0.1 — a tile could
+       * not begin loading until a tenth of it was already on screen. The
+       * gallery owner's description of the symptom is that exact shape:
+       * spinning the wheel outran loading by ~50 images, then it caught up.
+       *
+       * Viewport-relative rather than a fixed 100px like Justified: a phone
+       * and a 4K desktop scroll past very different amounts of grid per
+       * gesture, and a band tuned to one is wrong for the other.
+       *
+       * `%`, not `vh` — rootMargin only accepts px and percentages, and an
+       * IntersectionObserver constructed with a vh value throws. A percentage
+       * resolves against the root's own box, so 100% is one viewport height
+       * of lead in each direction, which is what vh would have meant.
+       */
+      inViewRootMargin="100% 0px"
      fadeInWhenVisible={animationType === 'fade'}
      skeletonClassName="skeleton aspect-square w-full rounded-lg"
      imageProps={{
@@ -0,0 +1,59 @@
+/**
+ * Grid's lazy-loading pre-load band (#1287).
+ *
+ * Grid was the only layout passing `lazy` without an `inViewRootMargin`, so
+ * PhotoCard ran its observer at the IntersectionObserver default of `0px`
+ * with `threshold: 0.1` — a tile could not begin loading until a tenth of it
+ * was already on screen. The gallery owner described exactly that: spinning
+ * the scroll wheel outran loading by ~50 images before it caught up.
+ *
+ * The unit matters as much as the value. `rootMargin` accepts only px and
+ * percentages; an IntersectionObserver constructed with a `vh` value throws
+ * SyntaxError, which would have broken every Grid gallery outright. Verified
+ * in Chrome:
+ *
+ *   '100% 0px'  → accepted
+ *   '100px 0px' → accepted
+ *   '100vh 0px' → SyntaxError: rootMargin must be specified in pixels or percent
+ *
+ * jsdom has no IntersectionObserver, so this asserts against the source
+ * rather than constructing one.
+ */
+import { describe, it, expect } from 'vitest';
+import { readFileSync } from 'fs';
+import { resolve } from 'path';
+
+const layouts = resolve(__dirname, '..');
+const read = (f: string) => readFileSync(resolve(layouts, f), 'utf8');
+
+/** Only px and % are legal rootMargin units. */
+const LEGAL_ROOT_MARGIN = /^(-?\d+(px|%)|0)(\s+(-?\d+(px|%)|0)){0,3}$/;
+
+describe('grid lazy pre-load band', () => {
+  it('Grid passes an inViewRootMargin', () => {
+    expect(read('GridGalleryLayout.tsx')).toMatch(/inViewRootMargin=/);
+  });
+
+  it('every inViewRootMargin in every layout uses a legal unit', () => {
+    // A vh value throws at IntersectionObserver construction and takes the
+    // whole gallery down with it, so this guards the unit, not just presence.
+    for (const file of ['GridGalleryLayout.tsx', 'JustifiedGalleryLayout.tsx']) {
+      const src = read(file);
+      for (const [, value] of src.matchAll(/inViewRootMargin="([^"]+)"/g)) {
+        expect(value, `${file}: "${value}"`).toMatch(LEGAL_ROOT_MARGIN);
+      }
+    }
+  });
+
+  it('every layout that lazy-renders also declares a pre-load band', () => {
+    // The defect was Grid being lazy with no margin. Any future layout that
+    // opts into `lazy` and forgets the margin reintroduces it.
+    for (const file of ['GridGalleryLayout.tsx', 'JustifiedGalleryLayout.tsx']) {
+      const src = read(file);
+      const isLazy = /^\s*lazy\s*$/m.test(src) || /\slazy=\{?true/.test(src);
+      if (!isLazy) continue;
+      expect(src, `${file} is lazy but declares no inViewRootMargin`)
+        .toMatch(/inViewRootMargin=/);
+    }
+  });
+});