From f3cd541ef03887985e96e37ce73a7d26709b516f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 14:02:34 +0000 Subject: [PATCH] fix(cuesheet): report image problems instead of failing silently A value which is not a link was discarded on blur without any feedback: the text stayed in the field, so it looked like it had been saved. We now mark the field and say what is expected. Paths served by ontime itself (eg. /user/slide.png) are now accepted, they were rejected before. An image which cannot be loaded showed a broken icon with no explanation. This is what a dropbox share link does, since it serves an html page rather than the image, so it is worth naming the problem. We also reserve the space of an image while it is loading, using the aspect ratio of the last time we saw it. Rows are unmounted while out of view, so without it the row collapses and grows again on the way back, shifting the table under the user. Smaller items in the same cell: the lazy loading attribute only added a gate before the request (the row is only mounted when it is already close to the viewport), the image had no alt text, and two expressions could never run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CKXcDrZoQXbJpaXiLi1aff --- .../utils/__tests__/imageDimensions.test.ts | 38 ++++++++ .../src/common/utils/imageDimensions.ts | 47 ++++++++++ .../cuesheet/__tests__/EditableImage.test.ts | 22 +++++ .../EditableImage.module.scss | 13 +++ .../cuesheet-table-elements/EditableImage.tsx | 87 +++++++++++++++---- .../cuesheetColsFactory.tsx | 9 +- 6 files changed, 196 insertions(+), 20 deletions(-) create mode 100644 apps/client/src/common/utils/__tests__/imageDimensions.test.ts create mode 100644 apps/client/src/common/utils/imageDimensions.ts create mode 100644 apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts diff --git a/apps/client/src/common/utils/__tests__/imageDimensions.test.ts b/apps/client/src/common/utils/__tests__/imageDimensions.test.ts new file mode 100644 index 000000000..fa0e3a4d4 --- /dev/null +++ b/apps/client/src/common/utils/__tests__/imageDimensions.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, test } from 'vitest'; + +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('remembers the aspect ratio of a loaded image', () => { + expect(getRememberedAspectRatio('http://ontime.local/image.png')).toBe(null); + + rememberAspectRatio('http://ontime.local/image.png', makeImage(1920, 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); + }); +}); diff --git a/apps/client/src/common/utils/imageDimensions.ts b/apps/client/src/common/utils/imageDimensions.ts new file mode 100644 index 000000000..27519dccf --- /dev/null +++ b/apps/client/src/common/utils/imageDimensions.ts @@ -0,0 +1,47 @@ +/** + * Images in the cuesheet live inside a virtualised table: + * rows are unmounted when they leave the viewport and mounted again when they come back. + * 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 + * 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 */ +const maxSize = 500; + +const aspectRatios = new Map(); + +/** + * @returns the aspect ratio 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; +} + +/** + * Records the aspect ratio of a loaded image + */ +export function rememberAspectRatio(src: string | null | undefined, image: HTMLImageElement) { + if (!src || 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); + + while (aspectRatios.size > maxSize) { + const oldest = aspectRatios.keys().next(); + if (oldest.done) { + return; + } + aspectRatios.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 new file mode 100644 index 000000000..0a2f6ad4f --- /dev/null +++ b/apps/client/src/views/cuesheet/__tests__/EditableImage.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, test } from 'vitest'; + +import { isValidImageSource } from '../cuesheet-table/cuesheet-table-elements/EditableImage'; + +describe('isValidImageSource()', () => { + test('accepts images hosted elsewhere', () => { + expect(isValidImageSource('https://example.com/image.png')).toBe(true); + expect(isValidImageSource('http://example.com/image.png')).toBe(true); + }); + + test('accepts images served by ontime', () => { + expect(isValidImageSource('/user/image.png')).toBe(true); + expect(isValidImageSource('/external/image.png')).toBe(true); + }); + + 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('user/image.png')).toBe(false); + expect(isValidImageSource('some text')).toBe(false); + }); +}); diff --git a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.module.scss b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.module.scss index 194d166b8..c86367533 100644 --- a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.module.scss +++ b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.module.scss @@ -5,6 +5,19 @@ &:not(:read-only):hover::placeholder { opacity: 1; } + + &[data-invalid] { + outline: 1px solid $red-500; + } +} + +/** feedback on a value we cannot use, either rejected or failed to load */ +.message { + display: block; + padding: 0.25rem 0; + color: $red-500; + font-size: calc(1rem - 3px); + line-height: 1.2; } .imageCell { 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 a9bbc00be..bb7a3b28d 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 @@ -1,27 +1,49 @@ -import { memo } from 'react'; +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 style from './EditableImage.module.scss'; interface EditableImageProps { initialValue: string; + fieldLabel: string; readOnly?: boolean; updateValue: (newValue: string) => void; } export default memo(EditableImage); -function EditableImage({ initialValue, readOnly, updateValue }: EditableImageProps) { +/** + * An image is either hosted somewhere else + * or served by ontime itself (eg. a file placed in the user folder) + */ +export function isValidImageSource(value: string): boolean { + return value.startsWith('http://') || value.startsWith('https://') || value.startsWith('/'); +} + +function EditableImage({ initialValue, fieldLabel, readOnly, updateValue }: EditableImageProps) { + const [isRejected, setIsRejected] = useState(false); + /** we keep track of the source itself, so that the state follows the value being shown */ + const [failedSource, setFailedSource] = useState(null); + const [loadedSource, setLoadedSource] = useState(null); + const handleUpdate = (newValue: string) => { - if (newValue === initialValue) { + const value = newValue.trim(); + + if (value === initialValue) { + setIsRejected(false); return; } - if (newValue !== '' && !newValue.startsWith('http')) { + + if (value !== '' && !isValidImageSource(value)) { + setIsRejected(true); return; } - updateValue(newValue); + + setIsRejected(false); + updateValue(value); }; const openInNewTab = () => { @@ -36,22 +58,34 @@ function EditableImage({ initialValue, readOnly, updateValue }: EditableImagePro if (!initialValue) { return ( - handleUpdate(event.currentTarget.value)} - onKeyDown={(event) => { - if (event.key === 'Enter') { - handleUpdate(event.currentTarget.value); - } - }} - defaultValue={initialValue} - /> + <> + setIsRejected(false)} + onBlur={(event) => handleUpdate(event.currentTarget.value)} + onKeyDown={(event) => { + if (event.key === 'Enter') { + handleUpdate(event.currentTarget.value); + } + }} + /> + {isRejected && Use a link (https://...) or a file in ontime (/user/...)} + ); } + /** + * 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. + */ + const knownAspectRatio = getRememberedAspectRatio(initialValue); + const isLoaded = loadedSource === initialValue; + return (
{!readOnly && ( @@ -62,7 +96,22 @@ function EditableImage({ initialValue, readOnly, updateValue }: EditableImagePro
)} - {Boolean(initialValue) && } + {failedSource === initialValue ? ( + Could not load image + ) : ( + {fieldLabel} { + rememberAspectRatio(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} + /> + )} ); } diff --git a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/cuesheetColsFactory.tsx b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/cuesheetColsFactory.tsx index 9c7a8a463..a47a1cc34 100644 --- a/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/cuesheetColsFactory.tsx +++ b/apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/cuesheetColsFactory.tsx @@ -175,7 +175,14 @@ function LazyImage({ row, column, table }: CuesheetCellContext) { const canWrite = column.columnDef.meta?.canWrite; const initialValue = event.custom[column.id]; - return ; + return ( + + ); } function MakeSingleLineField({ row, column, table }: CuesheetCellContext) {