refactor(cuesheet): reserve the space an image will actually take

We were remembering the aspect ratio and reserving the full width of the
column, which over-reserves for an image smaller than the column: the row
would settle to a smaller size once the image was shown.

We now remember the size of the image and express the reservation in CSS
as min(100%, width), which is what the image itself resolves to at any
column width. Column sizes are applied as CSS variables and do not
re-render the cells, so the reservation follows a resize on its own.

Also groups the tests around the behaviour they describe.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CKXcDrZoQXbJpaXiLi1aff

apps/client/src/common/utils/__tests__/imageDimensions.test.ts

@@ -1,38 +1,26 @@
-import { describe, expect, test } from 'vitest';
+import { getRememberedDimensions, rememberDimensions } from '../imageDimensions';
-
-import { getRememberedAspectRatio, rememberAspectRatio } from '../imageDimensions';
 
 /** stand-in for a loaded HTMLImageElement */
 function makeImage(naturalWidth: number, naturalHeight: number) {
   return { naturalWidth, naturalHeight } as HTMLImageElement;
 }
 
-describe('image dimensions', () => {
+test('We remember the size of an image, so that we can reserve its space when it comes back', () => {
-  test('remembers the aspect ratio of a loaded image', () => {
+  expect(getRememberedDimensions('http://ontime.local/unseen.png')).toBe(null);
-    expect(getRememberedAspectRatio('http://ontime.local/image.png')).toBe(null);
 
-    rememberAspectRatio('http://ontime.local/image.png', makeImage(1920, 1080));
+  rememberDimensions('http://ontime.local/image.png', makeImage(1920, 1080));
+  expect(getRememberedDimensions('http://ontime.local/image.png')).toMatchObject({ width: 1920, height: 1080 });
 
-    expect(getRememberedAspectRatio('http://ontime.local/image.png')).toBe(1920 / 1080);
+  // an image which failed to load has no size to offer
-  });
+  rememberDimensions('http://ontime.local/broken.png', makeImage(0, 0));
-
+  expect(getRememberedDimensions('http://ontime.local/broken.png')).toBe(null);
-  test('handles images which have not loaded', () => {
+});
-    rememberAspectRatio('http://ontime.local/broken.png', makeImage(0, 0));
+
-    expect(getRememberedAspectRatio('http://ontime.local/broken.png')).toBe(null);
+test('We keep the most recently seen images, older entries are forgotten', () => {
-  });
+  for (let i = 0; i < 600; i++) {
-
+    rememberDimensions(`http://ontime.local/${i}.png`, makeImage(100, 50));
-  test('handles missing values', () => {
+  }
-    expect(getRememberedAspectRatio(undefined)).toBe(null);
+
-    expect(getRememberedAspectRatio('')).toBe(null);
+  expect(getRememberedDimensions('http://ontime.local/0.png')).toBe(null);
-    expect(() => rememberAspectRatio('', makeImage(100, 100))).not.toThrow();
+  expect(getRememberedDimensions('http://ontime.local/599.png')).toMatchObject({ width: 100, height: 50 });
-  });
-
-  test('forgets the least recently used entries', () => {
-    for (let i = 0; i < 600; i++) {
-      rememberAspectRatio(`http://ontime.local/${i}.png`, makeImage(100, 50));
-    }
-
-    expect(getRememberedAspectRatio('http://ontime.local/0.png')).toBe(null);
-    expect(getRememberedAspectRatio('http://ontime.local/599.png')).toBe(2);
-  });
 });

apps/client/src/common/utils/imageDimensions.ts

@@ -4,44 +4,46 @@
  * A re-mounted image has no dimensions until it is available,
  * which makes the row change height and the table shift under the user.
  *
- * We remember the aspect ratio of the images we have already seen
+ * We remember the size of the images we have already seen
  * so that we can reserve the space they will take.
  * This only holds two numbers per image: we leave the image data itself to the browser cache,
  * which knows better than us when memory should be released.
  */
 
-/** how many aspect ratios we remember, this is only a few bytes per entry */
+export interface ImageDimensions {
+  width: number;
+  height: number;
+}
+
+/** how many sizes we remember, this is only a few bytes per entry */
 const maxSize = 500;
 
-const aspectRatios = new Map<string, number>();
+const dimensions = new Map<string, ImageDimensions>();
 
 /**
- * @returns the aspect ratio of a previously loaded image, if we have seen it before
+ * @returns the size of a previously loaded image, if we have seen it before
  */
-export function getRememberedAspectRatio(src: string | null | undefined): number | null {
+export function getRememberedDimensions(src: string): ImageDimensions | null {
-  if (!src) {
+  return dimensions.get(src) ?? null;
-    return null;
-  }
-  return aspectRatios.get(src) ?? null;
 }
 
 /**
- * Records the aspect ratio of a loaded image
+ * Records the size of a loaded image
  */
-export function rememberAspectRatio(src: string | null | undefined, image: HTMLImageElement) {
+export function rememberDimensions(src: string, image: HTMLImageElement) {
-  if (!src || image.naturalHeight === 0) {
+  if (image.naturalHeight === 0) {
     return;
   }
 
   // the map iteration order is our LRU queue, re-adding the entry marks it as recently used
-  aspectRatios.delete(src);
+  dimensions.delete(src);
-  aspectRatios.set(src, image.naturalWidth / image.naturalHeight);
+  dimensions.set(src, { width: image.naturalWidth, height: image.naturalHeight });
 
-  while (aspectRatios.size > maxSize) {
+  while (dimensions.size > maxSize) {
-    const oldest = aspectRatios.keys().next();
+    const oldest = dimensions.keys().next();
     if (oldest.done) {
       return;
     }
-    aspectRatios.delete(oldest.value);
+    dimensions.delete(oldest.value);
   }
 }

apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts

@@ -1,24 +1,18 @@
-import { describe, expect, test } from 'vitest';
-
 import { isValidImageSource } from '../cuesheet-table/cuesheet-table-elements/EditableImage';
 
-describe('isValidImageSource()', () => {
+test('An image is referenced by link, anything else is rejected', () => {
-  test('accepts links to a hosted image', () => {
+  const testCases = [
-    expect(isValidImageSource('https://example.com/image.png')).toBe(true);
+    { value: 'https://example.com/image.png', isValid: true },
-    expect(isValidImageSource('http://example.com/image.png')).toBe(true);
+    { value: 'http://example.com/image.png', isValid: true },
-  });
+    // a file is local to the machine running ontime, it would not resolve for the clients we serve
+    { value: '/user/image.png', isValid: false },
+    { value: 'file:///Users/me/image.png', isValid: false },
+    { value: 'C:\\images\\image.png', isValid: false },
+    // values which do not describe a location we can reach
+    { value: 'www.example.com/image.png', isValid: false },
+    { value: 'https://', isValid: false },
+    { value: 'some text', isValid: false },
+  ];
 
-  test('rejects references to a local file, they would not resolve for our clients', () => {
+  testCases.forEach((t) => expect(isValidImageSource(t.value)).toBe(t.isValid));
-    expect(isValidImageSource('/user/image.png')).toBe(false);
-    expect(isValidImageSource('user/image.png')).toBe(false);
-    expect(isValidImageSource('file:///Users/me/image.png')).toBe(false);
-    expect(isValidImageSource('C:\\images\\image.png')).toBe(false);
-  });
-
-  test('rejects values which would not resolve to an image', () => {
-    expect(isValidImageSource('www.example.com/image.png')).toBe(false);
-    expect(isValidImageSource('example.com/image.png')).toBe(false);
-    expect(isValidImageSource('https://')).toBe(false);
-    expect(isValidImageSource('some text')).toBe(false);
-  });
 });

apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.tsx

@@ -2,7 +2,7 @@ import { memo, useState } from 'react';
 
 import Button from '../../../../common/components/buttons/Button';
 import Input from '../../../../common/components/input/input/Input';
-import { getRememberedAspectRatio, rememberAspectRatio } from '../../../../common/utils/imageDimensions';
+import { getRememberedDimensions, rememberDimensions } from '../../../../common/utils/imageDimensions';
 
 import style from './EditableImage.module.scss';
 
@@ -87,9 +87,17 @@ function EditableImage({ initialValue, fieldLabel, readOnly, updateValue }: Edit
    * The cuesheet is virtualised: rows are unmounted once they leave the viewport.
    * When the row comes back, we reserve the space the image took
    * so that the table does not shift while the browser makes it available.
+   * The reservation is given in CSS so that it follows the column being resized,
+   * the same way the image itself does once it is shown.
    */
-  const knownAspectRatio = getRememberedAspectRatio(initialValue);
+  const knownDimensions = getRememberedDimensions(initialValue);
   const isLoaded = loadedSource === initialValue;
+  const reservedSpace = knownDimensions
+    ? {
+        aspectRatio: knownDimensions.width / knownDimensions.height,
+        width: `min(100%, ${knownDimensions.width}px)`,
+      }
+    : undefined;
 
   return (
     <div className={style.imageCell}>
@@ -109,12 +117,12 @@ function EditableImage({ initialValue, fieldLabel, readOnly, updateValue }: Edit
           alt={fieldLabel}
           className={style.image}
           onLoad={(event) => {
-            rememberAspectRatio(initialValue, event.currentTarget);
+            rememberDimensions(initialValue, event.currentTarget);
             setLoadedSource(initialValue);
           }}
           onError={() => setFailedSource(initialValue)}
           /** until the image is available, we reserve the space it took the last time we saw it */
-          style={!isLoaded && knownAspectRatio !== null ? { aspectRatio: knownAspectRatio, width: '100%' } : undefined}
+          style={isLoaded ? undefined : reservedSpace}
         />
       )}
     </div>