Просмотр исходного кода

Show the printer card thumbnail again after navigating back to the page (#2826)

The cover URL is cache-busted on the print name, which does not change
while a print runs. Leaving the printers page and returning therefore
re-mounts with a byte-identical src, which the browser serves from its
in-memory cache with no network request -- which is why the reporter's
network panel was empty while the placeholder sat there.

`loaded` could only ever be set by onLoad, and a mount effect reset it to
false unconditionally. For a cache hit those are two racing tasks with no
ordering between them: when the load event won, the effect undid it, and
nothing put it right afterwards because the URL does not change again for
the rest of the print. Read the element's own complete/naturalWidth in
that effect instead of assuming nothing has loaded. That settles it
whichever task wins, and also covers the variant the reporter proposed,
where the handler is not live in time.

Being a race explains why it reproduced 100% for the reporter and not at
all here. Only the printer card was affected; archive thumbnails build a
fresh URL every mount, so they always hit the network.

CoverImage is exported so the regression tests can mount it directly.
maziggy 3 недель назад
Родитель
Сommit
71d506be0c

Разница между файлами не показана из-за своего большого размера
+ 1 - 0
CHANGELOG.md


+ 193 - 0
frontend/src/__tests__/components/CoverImageCachedMount.test.tsx

@@ -0,0 +1,193 @@
+/**
+ * The printer-card thumbnail has to survive an image the browser already has (#2826).
+ *
+ * The URL is cache-busted on the print *name*, which does not change while a
+ * print runs. So navigating away from the printers page and back re-mounts
+ * with a byte-identical `src`, which the browser serves from its in-memory
+ * cache -- no network request at all, which is why the reporter's Network
+ * panel was empty while the thumbnail sat on the placeholder.
+ *
+ * `loaded` used to be settable only by `onLoad`, while a mount effect reset it
+ * to false unconditionally. For a cache hit those two are racing tasks with no
+ * ordering between them, and when `load` won, the effect undid it -- and never
+ * ran again, because the URL does not change again during the print. That is
+ * why it reproduced 100% for the reporter and not at all on the maintainer's
+ * machine.
+ *
+ * jsdom never loads images or fires `load`, so the race itself cannot be
+ * staged here. What these tests pin is the invariant that makes the race
+ * unwinnable either way: the component must read the element's own state
+ * instead of assuming nothing has loaded yet.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { render, waitFor } from '@testing-library/react';
+import { CoverImage } from '../../pages/PrintersPage';
+
+vi.mock('../../hooks/useCameraStreamToken', async () => {
+  const actual = await vi.importActual<Record<string, unknown>>('../../hooks/useCameraStreamToken');
+  return { ...actual, withStreamToken: (u: string) => u };
+});
+
+/** Present an <img> the way a memory-cache hit does: already complete. */
+function stubAlreadyComplete(naturalWidth = 640) {
+  Object.defineProperty(HTMLImageElement.prototype, 'complete', {
+    get: () => true,
+    configurable: true,
+  });
+  Object.defineProperty(HTMLImageElement.prototype, 'naturalWidth', {
+    get: () => naturalWidth,
+    configurable: true,
+  });
+}
+
+function restoreImg() {
+  // @ts-expect-error removing the test-only prototype overrides
+  delete HTMLImageElement.prototype.complete;
+  // @ts-expect-error removing the test-only prototype overrides
+  delete HTMLImageElement.prototype.naturalWidth;
+}
+
+const URL_ = '/api/v1/printers/1/cover';
+
+describe('CoverImage with an image the browser already has', () => {
+  afterEach(restoreImg);
+
+  it('shows it instead of the placeholder', async () => {
+    stubAlreadyComplete();
+
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    await waitFor(() => {
+      expect(container.querySelector('img')!.className).toContain('block');
+    });
+    expect(container.querySelector('img')!.className).not.toContain('hidden');
+  });
+
+  it('treats it as loaded across a remount with the same print', async () => {
+    stubAlreadyComplete();
+
+    // First visit: warms the cache in a real browser.
+    const first = render(<CoverImage url={URL_} printName="Benchy" />);
+    await waitFor(() => expect(first.container.querySelector('img')!.className).toContain('block'));
+    first.unmount();
+
+    // Navigating back. Same print name means a byte-identical URL, so nothing
+    // is fetched -- this is the mount that used to come back blank.
+    const second = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    await waitFor(() => {
+      expect(second.container.querySelector('img')!.className).toContain('block');
+    });
+  });
+
+  it('makes it clickable, not just visible', async () => {
+    // `loaded` also gates the click-to-enlarge overlay, so a stuck `false`
+    // left the thumbnail inert as well as invisible.
+    stubAlreadyComplete();
+
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    await waitFor(() => {
+      expect(container.querySelector('div')!.className).toContain('cursor-pointer');
+    });
+  });
+});
+
+describe('CoverImage when nothing is cached', () => {
+  beforeEach(restoreImg);
+
+  it('waits behind the placeholder until the image arrives', () => {
+    // jsdom leaves `complete` false and never fires `load`, which is exactly
+    // the cold-cache state: the placeholder is correct until it resolves.
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    expect(container.querySelector('img')!.className).toContain('hidden');
+  });
+
+  it('still reveals the image when onLoad fires', async () => {
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+    const img = container.querySelector('img')!;
+
+    img.dispatchEvent(new Event('load'));
+
+    await waitFor(() => expect(img.className).toContain('block'));
+  });
+
+  it('falls back to the placeholder when the image fails', async () => {
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    container.querySelector('img')!.dispatchEvent(new Event('error'));
+
+    await waitFor(() => expect(container.querySelector('img')).toBeNull());
+  });
+
+  it('shows the placeholder when there is no cover at all', () => {
+    const { container } = render(<CoverImage url={null} printName="Benchy" />);
+
+    expect(container.querySelector('img')).toBeNull();
+  });
+});
+
+describe('CoverImage when the print changes', () => {
+  afterEach(restoreImg);
+
+  it('re-evaluates rather than carrying the previous print forward', async () => {
+    // The reset-on-change behaviour the effect was added for in the first
+    // place still has to hold: a new print name is a new URL, and a fresh
+    // element that is not yet complete must go back behind the placeholder.
+    stubAlreadyComplete();
+    const { container, rerender } = render(<CoverImage url={URL_} printName="Benchy" />);
+    await waitFor(() => expect(container.querySelector('img')!.className).toContain('block'));
+
+    restoreImg();
+    rerender(<CoverImage url={URL_} printName="Something Else" />);
+
+    await waitFor(() => {
+      expect(container.querySelector('img')!.className).toContain('hidden');
+    });
+  });
+
+  it('keeps the cache-buster tied to the print name', () => {
+    stubAlreadyComplete();
+    const { container, rerender } = render(<CoverImage url={URL_} printName="Benchy" />);
+    const first = container.querySelector('img')!.getAttribute('src');
+
+    rerender(<CoverImage url={URL_} printName="Other" />);
+    const second = container.querySelector('img')!.getAttribute('src');
+
+    expect(first).toContain('v=Benchy');
+    expect(second).toContain('v=Other');
+    expect(first).not.toEqual(second);
+  });
+
+  it('reuses the URL for the same print, which is what makes the cache hit', () => {
+    // Pinning the precondition, not an incidental detail: if this ever became
+    // unique per mount the bug would vanish and so would the caching, and the
+    // tests above would silently stop covering anything.
+    stubAlreadyComplete();
+    const a = render(<CoverImage url={URL_} printName="Benchy" />);
+    const first = a.container.querySelector('img')!.getAttribute('src');
+    a.unmount();
+
+    const b = render(<CoverImage url={URL_} printName="Benchy" />);
+    const second = b.container.querySelector('img')!.getAttribute('src');
+
+    expect(second).toEqual(first);
+  });
+});
+
+describe('CoverImage with a broken cached image', () => {
+  afterEach(restoreImg);
+
+  it('does not treat a zero-width complete image as loaded', async () => {
+    // `complete` is also true for an image that failed. Width is what
+    // separates "decoded and ready" from "finished, with nothing to show".
+    stubAlreadyComplete(0);
+
+    const { container } = render(<CoverImage url={URL_} printName="Benchy" />);
+
+    await waitFor(() => expect(container.querySelector('img')).not.toBeNull());
+    expect(container.querySelector('img')!.className).toContain('hidden');
+  });
+});

+ 29 - 3
frontend/src/pages/PrintersPage.tsx

@@ -943,7 +943,7 @@ function getEmptySlotKind(tray: { tray_type?: string | null; state?: number | nu
 const DRY_START_CONFIRM_MS = 30_000;
 
 
-function CoverImage({
+export function CoverImage({
   url,
   printName,
   className = 'w-20 h-20',
@@ -958,6 +958,7 @@ function CoverImage({
   const [loaded, setLoaded] = useState(false);
   const [error, setError] = useState(false);
   const [showOverlay, setShowOverlay] = useState(false);
+  const imgRef = useRef<HTMLImageElement>(null);
 
   // Cache-bust the image URL when the print name changes so the browser
   // fetches the new cover instead of serving the stale cached image.
@@ -967,10 +968,34 @@ function CoverImage({
     return withStreamToken(`${url}${sep}v=${encodeURIComponent(printName || Date.now().toString())}`);
   }, [url, printName]);
 
-  // Reset loaded/error state when the image URL changes
+  // Re-evaluate load state when the image URL changes, and ask the element
+  // whether it is already showing something rather than assuming it is not.
+  //
+  // `onLoad` used to be the only thing that could set `loaded`, and this
+  // effect reset it to false unconditionally. That is right when the URL
+  // really changes, but it also runs on mount — and the two are not the same
+  // situation (#2826). The URL is cache-busted on the print *name*, which is
+  // constant for the duration of a print, so navigating away from the
+  // printers page and back re-mounts with a byte-identical src that the
+  // browser serves from its in-memory cache. The `load` event for a cache hit
+  // and React's passive-effect flush are both plain tasks with no ordering
+  // between them, so when `load` won, this effect ran afterwards and undid
+  // it: `loaded` stayed false, the img stayed `hidden`, and the placeholder
+  // sat there until a full reload. Nothing recovered it, because the URL
+  // never changes again during the print and so this effect never re-runs.
+  //
+  // Being a race, it reproduced every time for #2826's reporter and not at
+  // all here, which is also why the network panel showed no request: there
+  // was no request to show.
+  //
+  // Asking `complete && naturalWidth > 0` settles it without needing to know
+  // which of the two ran first. It is also correct for the case the reporter
+  // proposed (a `load` that fires before React's handler is live), so the
+  // fix stands whichever mechanism is really at work.
   useEffect(() => {
-    setLoaded(false);
     setError(false);
+    const el = imgRef.current;
+    setLoaded(Boolean(el?.complete && el.naturalWidth > 0));
   }, [cacheBustedUrl]);
 
   return (
@@ -982,6 +1007,7 @@ function CoverImage({
         {cacheBustedUrl && !error ? (
           <>
             <img
+              ref={imgRef}
               src={cacheBustedUrl}
               alt={t('printers.printPreview')}
               className={`w-full h-full object-cover ${loaded ? 'block' : 'hidden'}`}

Разница между файлами не показана из-за своего большого размера
+ 0 - 0
static/assets/index-Cin_PtM9.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-CwdtRDU5.js"></script>
+    <script type="module" crossorigin src="/assets/index-Cin_PtM9.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-1Ya6fAmN.css">
   </head>
   <body>

Некоторые файлы не были показаны из-за большого количества измененных файлов