From e28b06efed0709cd74c0c24fd72beb44379e664b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 10:52:39 +0000 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01CKXcDrZoQXbJpaXiLi1aff --- .../utils/__tests__/imageDimensions.test.ts | 46 +++++++------------ .../src/common/utils/imageDimensions.ts | 36 ++++++++------- .../cuesheet/__tests__/EditableImage.test.ts | 34 ++++++-------- .../cuesheet-table-elements/EditableImage.tsx | 16 +++++-- 4 files changed, 62 insertions(+), 70 deletions(-) diff --git a/apps/client/src/common/utils/__tests__/imageDimensions.test.ts b/apps/client/src/common/utils/__tests__/imageDimensions.test.ts index fa0e3a4d4..dd7cb6689 100644 --- a/apps/client/src/common/utils/__tests__/imageDimensions.test.ts +++ b/apps/client/src/common/utils/__tests__/imageDimensions.test.ts @@ -1,38 +1,26 @@ -import { describe, expect, test } from 'vitest'; - -import { getRememberedAspectRatio, rememberAspectRatio } from '../imageDimensions'; +import { getRememberedDimensions, rememberDimensions } from '../imageDimensions'; /** stand-in for a loaded HTMLImageElement */ function makeImage(naturalWidth: number, naturalHeight: number) { return { naturalWidth, naturalHeight } as HTMLImageElement; } -describe('image dimensions', () => { - test('remembers the aspect ratio of a loaded image', () => { - expect(getRememberedAspectRatio('http://ontime.local/image.png')).toBe(null); +test('We remember the size of an image, so that we can reserve its space when it comes back', () => { + expect(getRememberedDimensions('http://ontime.local/unseen.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); - }); - - 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('handles missing values', () => { - expect(getRememberedAspectRatio(undefined)).toBe(null); - expect(getRememberedAspectRatio('')).toBe(null); - expect(() => rememberAspectRatio('', makeImage(100, 100))).not.toThrow(); - }); - - 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); - }); + // 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('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)); + } + + expect(getRememberedDimensions('http://ontime.local/0.png')).toBe(null); + expect(getRememberedDimensions('http://ontime.local/599.png')).toMatchObject({ width: 100, height: 50 }); }); diff --git a/apps/client/src/common/utils/imageDimensions.ts b/apps/client/src/common/utils/imageDimensions.ts index 27519dccf..631f75cd3 100644 --- a/apps/client/src/common/utils/imageDimensions.ts +++ b/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(); +const dimensions = new Map(); /** - * @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 { - if (!src) { - return null; - } - return aspectRatios.get(src) ?? null; +export function getRememberedDimensions(src: string): ImageDimensions | null { + return dimensions.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) { - if (!src || image.naturalHeight === 0) { +export function rememberDimensions(src: string, image: HTMLImageElement) { + 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); - aspectRatios.set(src, image.naturalWidth / image.naturalHeight); + dimensions.delete(src); + dimensions.set(src, { width: image.naturalWidth, height: image.naturalHeight }); - while (aspectRatios.size > maxSize) { - const oldest = aspectRatios.keys().next(); + while (dimensions.size > maxSize) { + const oldest = dimensions.keys().next(); if (oldest.done) { return; } - aspectRatios.delete(oldest.value); + dimensions.delete(oldest.value); } } diff --git a/apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts b/apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts index 7b971dd5f..d451bf925 100644 --- a/apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts +++ b/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('accepts links to a hosted image', () => { - expect(isValidImageSource('https://example.com/image.png')).toBe(true); - expect(isValidImageSource('http://example.com/image.png')).toBe(true); - }); +test('An image is referenced by link, anything else is rejected', () => { + const testCases = [ + { value: 'https://example.com/image.png', isValid: 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', () => { - 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); - }); + testCases.forEach((t) => expect(isValidImageSource(t.value)).toBe(t.isValid)); }); diff --git a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.tsx b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.tsx index fe258fe0e..abeee3679 100644 --- a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.tsx +++ b/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 (
@@ -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} /> )}