Skip to content

Commit bc0fa3e

Browse files
committed
fix(media): drop actions on unloadable tiles and reset on src swap
An image tile that could not be shown still offered Download, Copy and Open, handing the host an index to an image it cannot load and opening an empty viewer. Broken tiles now show only host-supplied action children, and viewer items for such images carry no src instead of an empty-string sentinel, so the viewer says there is no preview and offers no download. A tile whose address changed in place kept its failed or loaded flag, so the new address was never tried and skipped its reveal. The flags now reset when the source changes. Citations shown as plain text no longer carry the link icon, the sources list keeps list semantics without bullets, and an image without alt text is named after the prompt's opening words. Adds viewer tests for video, audio, PDF, text and unloadable items, and the first tests for Sources.
1 parent 2bf056a commit bc0fa3e

6 files changed

Lines changed: 350 additions & 33 deletions

File tree

‎src/ui/generated-images.test.tsx‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,80 @@ describe("GeneratedImages", () => {
204204
);
205205
view.unmount();
206206
});
207+
208+
test("an unloadable tile offers no actions and cannot open", () => {
209+
const calls: string[] = [];
210+
const unsafe: PartImage = {
211+
...DONE,
212+
images: [
213+
{
214+
id: "x",
215+
mediaType: "application/pdf",
216+
width: 10,
217+
height: 10,
218+
url: "data:application/pdf;base64,JVBERi0=",
219+
},
220+
],
221+
};
222+
const view = render(
223+
<GeneratedImages
224+
part={unsafe}
225+
onOpen={(index) => calls.push(`open:${index}`)}
226+
onDownload={(index) => calls.push(`download:${index}`)}
227+
onCopy={(index) => calls.push(`copy:${index}`)}
228+
/>,
229+
);
230+
const tile = slots(view.container, "generated-image")[0];
231+
expect(tile?.dataset["state"]).toBe("error");
232+
expect(slots(view.container, "generated-image-actions")).toHaveLength(0);
233+
expect(tile?.querySelectorAll("button")).toHaveLength(0);
234+
act(() => tile?.click());
235+
expect(calls).toEqual([]);
236+
view.unmount();
237+
});
238+
239+
test("a tile that failed tries again when its source changes", () => {
240+
const remote = (url: string): PartImage => ({
241+
...DONE,
242+
images: [{ id: "r", mediaType: "image/png", width: 10, height: 10, url }],
243+
});
244+
const view = render(
245+
<GeneratedImages part={remote("https://images.example.com/1.png")} />,
246+
);
247+
act(() => {
248+
view.container.querySelector("img")?.dispatchEvent(new Event("error"));
249+
});
250+
const tile = () => slots(view.container, "generated-image")[0];
251+
expect(tile()?.dataset["state"]).toBe("error");
252+
view.rerender(
253+
<GeneratedImages part={remote("https://images.example.com/2.png")} />,
254+
);
255+
expect(tile()?.dataset["state"]).toBe("loading");
256+
expect(view.container.querySelector("img")?.getAttribute("src")).toBe(
257+
"https://images.example.com/2.png",
258+
);
259+
view.unmount();
260+
});
261+
262+
test("an image without alt is named after the prompt", () => {
263+
const { alt: _alt, ...plain } = image(1);
264+
const view = render(
265+
<GeneratedImages
266+
part={{ ...DONE, images: [plain] }}
267+
onOpen={() => {}}
268+
onDownload={() => {}}
269+
/>,
270+
);
271+
const labels = [...view.container.querySelectorAll("button")].map(
272+
(button) => button.getAttribute("aria-label"),
273+
);
274+
expect(labels).toEqual([
275+
"Open Cover for the quarterly review deck, image 1",
276+
"Download Cover for the quarterly review deck, image 1",
277+
"Open Cover for the quarterly review deck, image 1 full size",
278+
]);
279+
view.unmount();
280+
});
207281
});
208282

209283
describe("conversationImageSrc", () => {
@@ -240,4 +314,21 @@ describe("generatedImageViewerItems", () => {
240314
expect(items[0]?.detail).toBe("1536 × 1024");
241315
expect(items[1]?.alt).toBe("Cover option 2");
242316
});
317+
318+
test("an image with no safe source becomes an item with no source", () => {
319+
const items = generatedImageViewerItems({
320+
...DONE,
321+
images: [
322+
{
323+
id: "x",
324+
mediaType: "image/png",
325+
width: 10,
326+
height: 10,
327+
url: "javascript:alert(1)",
328+
},
329+
],
330+
});
331+
expect(items).toHaveLength(1);
332+
expect(items[0]?.src).toBe(undefined);
333+
});
243334
});

‎src/ui/generated-images.tsx‎

Lines changed: 63 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ export function conversationImageSrc(
5050
}
5151

5252
/** A file name for one image of a part: the prompt's first words, then which image. */
53-
export function generatedImageFileName(part: PartImage, index: number): string {
53+
function generatedImageFileName(part: PartImage, index: number): string {
5454
const image = part.images[index];
5555
const subtype = image?.mediaType.split("/")[1]?.split(/[;+]/)[0] ?? "";
5656
const extension = subtype === "" ? "png" : subtype.toLowerCase();
@@ -64,21 +64,30 @@ export function generatedImageFileName(part: PartImage, index: number): string {
6464
return `${slug === "" ? "image" : slug}-${index + 1}.${extension}`;
6565
}
6666

67-
/** A part's images as `MediaViewer` items, in the same order as its tiles. */
67+
/**
68+
* A part's images as `MediaViewer` items, in the same order as its tiles. An
69+
* image that cannot be shown safely keeps its place but has no `src`, so the
70+
* viewer offers no preview and no download for it.
71+
*/
6872
export function generatedImageViewerItems(
6973
part: PartImage,
7074
): readonly MediaViewerItem[] {
71-
return part.images.map((image, index) => ({
72-
name: generatedImageFileName(part, index),
73-
mediaType: image.mediaType,
74-
src: conversationImageSrc(image) ?? "",
75-
detail: `${image.width} × ${image.height}`,
76-
...(image.alt === undefined ? {} : { alt: image.alt }),
77-
}));
75+
return part.images.map((image, index) => {
76+
const src = conversationImageSrc(image);
77+
return {
78+
name: generatedImageFileName(part, index),
79+
mediaType: image.mediaType,
80+
...(src === undefined ? {} : { src }),
81+
detail: `${image.width} × ${image.height}`,
82+
...(image.alt === undefined ? {} : { alt: image.alt }),
83+
};
84+
});
7885
}
7986

8087
type GeneratedImagesContextValue = {
8188
readonly seed: number;
89+
/** Names an image that has no `alt`. */
90+
readonly prompt: string;
8291
/** Seen generating in this view: its images are revealed by the grain. */
8392
readonly watched: boolean;
8493
readonly onOpen: ((index: number, opener: HTMLElement) => void) | undefined;
@@ -100,6 +109,8 @@ function useGeneratedImagesContext(part: string): GeneratedImagesContextValue {
100109
type GeneratedImageContextValue = {
101110
readonly index: number;
102111
readonly label: string;
112+
/** It could not be shown, so there is nothing to open, download or copy. */
113+
readonly broken: boolean;
103114
};
104115

105116
const GeneratedImageContext = createContext<GeneratedImageContextValue | null>(
@@ -113,8 +124,19 @@ function seedOf(id: string): number {
113124
return sum;
114125
}
115126

116-
function imageLabel(image: ConversationImage, index: number): string {
117-
return image.alt ?? `Image ${index + 1}`;
127+
/** How many of the prompt's words name an image that has no `alt`. */
128+
const LABEL_WORDS = 6;
129+
130+
function imageLabel(
131+
image: ConversationImage,
132+
index: number,
133+
prompt: string,
134+
): string {
135+
if (image.alt !== undefined) return image.alt;
136+
const opening = prompt.trim().split(/\s+/).slice(0, LABEL_WORDS).join(" ");
137+
return opening === ""
138+
? `Image ${index + 1}`
139+
: `${opening}, image ${index + 1}`;
118140
}
119141

120142
// `part` and `onCopy` are also DOM attributes; here they mean the image part
@@ -191,15 +213,23 @@ export function GeneratedImages({
191213

192214
return (
193215
<GeneratedImagesContext.Provider
194-
value={{ seed, watched, onOpen, onDownload, onCopy }}
216+
value={{
217+
seed,
218+
prompt: part.prompt,
219+
watched,
220+
onOpen,
221+
onDownload,
222+
onCopy,
223+
}}
195224
>
196225
<div
197226
data-slot="generated-images"
198227
data-state={part.state}
199228
className={cn(
200229
"flex max-w-2xl flex-col gap-2",
201-
// One image shows large but never taller than about 520px: a tall
202-
// one narrows instead of becoming a column.
230+
// One image shows large: at most 512px (128 steps) wide, and never
231+
// taller than 520px (130 steps), so a tall one narrows instead of
232+
// becoming a column.
203233
single &&
204234
!failed &&
205235
"max-w-[min(calc(var(--spacing)*128),calc(var(--spacing)*130*var(--generated-images-ratio)))]",
@@ -379,15 +409,24 @@ export function GeneratedImage({
379409
children,
380410
...props
381411
}: GeneratedImageProps) {
382-
const { seed, watched, onOpen } = useGeneratedImagesContext("GeneratedImage");
412+
const { seed, prompt, watched, onOpen } =
413+
useGeneratedImagesContext("GeneratedImage");
383414
const reduce = usePrefersReducedMotion();
384415
const develop = watched && !reduce;
385416
const [developing, setDeveloping] = useState(develop);
386417
const [loaded, setLoaded] = useState(false);
387418
const [failed, setFailed] = useState(false);
388419
const imageRef = useRef<HTMLImageElement>(null);
389420
const src = conversationImageSrc(image);
390-
const label = imageLabel(image, index);
421+
const label = imageLabel(image, index, prompt);
422+
423+
// A new address for the same tile is a new attempt: it loads afresh.
424+
const [attempted, setAttempted] = useState(src);
425+
if (attempted !== src) {
426+
setAttempted(src);
427+
setLoaded(false);
428+
setFailed(false);
429+
}
391430

392431
// An image that finished loading before React listened for `load` (a
393432
// cached one) would otherwise stay under the grain.
@@ -423,7 +462,7 @@ export function GeneratedImage({
423462
);
424463

425464
return (
426-
<GeneratedImageContext.Provider value={{ index, label }}>
465+
<GeneratedImageContext.Provider value={{ index, label, broken }}>
427466
<figure
428467
data-slot="generated-image"
429468
data-state={broken ? "error" : loaded ? "loaded" : "loading"}
@@ -485,7 +524,8 @@ export type GeneratedImageActionsProps = React.ComponentProps<"div">;
485524
* Download, Copy and Open over a `GeneratedImage`'s corner, one for each of
486525
* `GeneratedImages`' `onDownload`, `onCopy` and `onOpen` that is given.
487526
* Shown on hover or focus, and always where there is no hover. Children add
488-
* further buttons after them.
527+
* further buttons after them. An image that could not be shown gets none of
528+
* the three, only the children.
489529
*/
490530
export function GeneratedImageActions({
491531
className,
@@ -499,7 +539,8 @@ export function GeneratedImageActions({
499539
if (tile === null) {
500540
throw new Error("GeneratedImageActions must be inside GeneratedImage.");
501541
}
502-
const { index, label } = tile;
542+
const { index, label, broken } = tile;
543+
if (broken && (children === undefined || children === null)) return null;
503544
return (
504545
<div
505546
data-slot="generated-image-actions"
@@ -510,7 +551,7 @@ export function GeneratedImageActions({
510551
)}
511552
{...props}
512553
>
513-
{onDownload === undefined ? null : (
554+
{broken || onDownload === undefined ? null : (
514555
<button
515556
type="button"
516557
aria-label={`Download ${label}`}
@@ -520,7 +561,7 @@ export function GeneratedImageActions({
520561
<Download aria-hidden />
521562
</button>
522563
)}
523-
{onCopy === undefined ? null : (
564+
{broken || onCopy === undefined ? null : (
524565
<button
525566
type="button"
526567
aria-label={`Copy ${label}`}
@@ -530,7 +571,7 @@ export function GeneratedImageActions({
530571
<Copy aria-hidden />
531572
</button>
532573
)}
533-
{onOpen === undefined ? null : (
574+
{broken || onOpen === undefined ? null : (
534575
<button
535576
type="button"
536577
aria-label={`Open ${label} full size`}

‎src/ui/media-viewer.test.tsx‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ function Harness({ style }: { readonly style?: React.CSSProperties }) {
1717
return <MediaViewer viewer={viewer} style={style} />;
1818
}
1919

20+
function One({ item }: { readonly item: MediaViewerItem }) {
21+
const viewer = useMediaViewer({ items: [item], defaultIndex: 0 });
22+
return <MediaViewer viewer={viewer} />;
23+
}
24+
2025
let root: Root | null = null;
2126
let container: HTMLElement | null = null;
2227

@@ -96,4 +101,93 @@ describe("MediaViewer", () => {
96101
root = null;
97102
expect(page.getAttribute("style") ?? "").not.toContain("overflow");
98103
});
104+
105+
const KINDS: readonly {
106+
readonly item: MediaViewerItem;
107+
readonly slot: string;
108+
readonly download: boolean;
109+
}[] = [
110+
{
111+
item: {
112+
name: "clip.mp4",
113+
mediaType: "video/mp4",
114+
src: "https://media.example.com/clip.mp4",
115+
},
116+
slot: "media-viewer-video",
117+
download: true,
118+
},
119+
{
120+
item: {
121+
name: "memo.mp3",
122+
mediaType: "audio/mpeg",
123+
src: "https://media.example.com/memo.mp3",
124+
},
125+
slot: "media-viewer-audio",
126+
download: true,
127+
},
128+
{
129+
item: {
130+
name: "report.pdf",
131+
mediaType: "application/pdf",
132+
src: "https://media.example.com/report.pdf",
133+
},
134+
slot: "media-viewer-page",
135+
download: true,
136+
},
137+
{
138+
item: {
139+
name: "notes.txt",
140+
mediaType: "text/plain",
141+
src: "https://media.example.com/notes.txt",
142+
text: "First line",
143+
},
144+
slot: "media-viewer-text",
145+
download: true,
146+
},
147+
{
148+
// A data: document would run script in the frame.
149+
item: {
150+
name: "inline.pdf",
151+
mediaType: "application/pdf",
152+
src: "data:application/pdf;base64,JVBERi0=",
153+
},
154+
slot: "media-viewer-empty",
155+
download: false,
156+
},
157+
{
158+
item: {
159+
name: "script.pdf",
160+
mediaType: "application/pdf",
161+
src: "javascript:alert(1)",
162+
},
163+
slot: "media-viewer-empty",
164+
download: false,
165+
},
166+
{
167+
// An image that cannot be loaded, such as an unsafe generated one.
168+
item: { name: "missing.png", mediaType: "image/png" },
169+
slot: "media-viewer-empty",
170+
download: false,
171+
},
172+
];
173+
174+
for (const { item, slot, download } of KINDS) {
175+
test(`${item.name} shows as ${slot}`, () => {
176+
render(<One item={item} />);
177+
const content = dialog()?.querySelector<HTMLElement>(
178+
"[data-slot='media-viewer-figure'] > :first-child",
179+
);
180+
expect(content?.dataset["slot"]).toBe(slot);
181+
if (slot === "media-viewer-text") {
182+
expect(content?.textContent).toBe(item.text);
183+
expect(content?.getAttribute("aria-label")).toBe(item.name);
184+
} else if (slot === "media-viewer-empty") {
185+
expect(content?.textContent).toBe("No preview for this file.");
186+
} else {
187+
expect(content?.getAttribute("src")).toBe(item.src ?? null);
188+
}
189+
const link = dialog()?.querySelector("a[download]");
190+
expect(link === null || link === undefined ? false : true).toBe(download);
191+
});
192+
}
99193
});

0 commit comments

Comments
 (0)