Skip to content

Commit 297fc2e

Browse files
authored
fix: remount <img> on src change in mo.Html to avoid stale paint (#9472)
When mo.Html re-renders with new image URLs, React reconciliation reuses the existing <img> DOM nodes and only updates the src attribute. With flaky CDNs/CORS proxies the new request can stall or fail and the browser keeps the previous image painted, so the user sees stale (or blank) images even though the rendered HTML source is correct. Key <img> elements by src+index in RenderHTML so React unmounts and remounts them when the src changes, mirroring the existing iframe remount workaround.
1 parent 546eafa commit 297fc2e

3 files changed

Lines changed: 176 additions & 3 deletions

File tree

frontend/src/plugins/core/RenderHTML.tsx

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import parse, {
66
type HTMLReactParserOptions,
77
} from "html-react-parser";
88
import React, {
9+
cloneElement,
910
isValidElement,
1011
type JSX,
1112
type ReactNode,
@@ -169,6 +170,35 @@ const addCopyButtonToCodehilite: TransformFn = (
169170
}
170171
};
171172

173+
// Decorator (not a match-and-replace transform): applies a src-based key
174+
// to <img> elements so they remount on src change. Reusing an <img> across
175+
// src changes can leave the previous image painted (e.g. when the new
176+
// request is slow/blocked, served stale by a CDN, or fails CORS), so the
177+
// user sees the old image even though the HTML source is up to date.
178+
//
179+
// Runs unconditionally after the match-and-replace transforms so it still
180+
// applies when an <img> was already wrapped by, say, wrapTooltipTargets.
181+
const keyImagesBySrc: TransformFn = (
182+
reactNode: ReactNode,
183+
domNode: DOMNode,
184+
index: number,
185+
): JSX.Element | undefined => {
186+
if (!(domNode instanceof Element) || domNode.name !== "img") {
187+
return undefined;
188+
}
189+
const src = domNode.attribs?.src;
190+
if (!src || !isValidElement(reactNode)) {
191+
return undefined;
192+
}
193+
// data: URIs are inline — no network fetch — so they can't go stale.
194+
// Skip to avoid bloating the React key with a megabyte base64 payload.
195+
// URI schemes are case-insensitive per RFC 3986.
196+
if (/^data:/i.test(src)) {
197+
return undefined;
198+
}
199+
return cloneElement(reactNode, { key: `${src}-${index}` });
200+
};
201+
172202
// Wrap elements with data-marimo-doc attribute in a DocHoverTarget
173203
const wrapDocHoverTargets: TransformFn = (
174204
reactNode: ReactNode,
@@ -281,6 +311,8 @@ function parseHtml({
281311
...additionalReplacements,
282312
];
283313

314+
// Match-and-replace transforms: the first one that returns a value wins
315+
// (short-circuits the rest).
284316
const transformFunctions: TransformFn[] = [
285317
addCopyButtonToCodehilite,
286318
preserveQueryParamsInAnchorLinks,
@@ -290,6 +322,12 @@ function parseHtml({
290322
removeWrappingHtmlTags,
291323
];
292324

325+
// Decorators: run unconditionally on the result of the transform pipeline
326+
// and may further wrap/clone it. Used for cross-cutting concerns that
327+
// should apply regardless of which (if any) match-and-replace transform
328+
// ran above.
329+
const decoratorFunctions: TransformFn[] = [keyImagesBySrc];
330+
293331
return parse(html, {
294332
replace: (domNode: DOMNode, index: number) => {
295333
for (const renderFunction of renderFunctions) {
@@ -301,13 +339,21 @@ function parseHtml({
301339
return domNode;
302340
},
303341
transform: (reactNode: ReactNode, domNode: DOMNode, index: number) => {
342+
let result: ReactNode = reactNode as JSX.Element;
304343
for (const transformFunction of transformFunctions) {
305-
const transformed = transformFunction(reactNode, domNode, index);
344+
const transformed = transformFunction(result, domNode, index);
306345
if (transformed) {
307-
return transformed;
346+
result = transformed;
347+
break;
348+
}
349+
}
350+
for (const decorate of decoratorFunctions) {
351+
const decorated = decorate(result, domNode, index);
352+
if (decorated) {
353+
result = decorated;
308354
}
309355
}
310-
return reactNode as JSX.Element;
356+
return result as JSX.Element;
311357
},
312358
});
313359
}

frontend/src/plugins/core/__test__/RenderHTML.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,60 @@ describe("parseHtml", () => {
6060
`);
6161
});
6262

63+
test("img has key derived from src so React remounts on src change", () => {
64+
const html = '<img src="https://cdn.example.com/a.png" alt="a">';
65+
const result = parseHtml({ html }) as React.ReactElement;
66+
expect(result.key).toBe("https://cdn.example.com/a.png-0");
67+
});
68+
69+
test("multiple imgs each get distinct keys", () => {
70+
const html =
71+
'<div><img src="https://cdn.example.com/a.png"><img src="https://cdn.example.com/b.png"></div>';
72+
const result = parseHtml({ html }) as React.ReactElement<{
73+
children: React.ReactElement[];
74+
}>;
75+
const children = result.props.children;
76+
expect(children[0].key).toBe("https://cdn.example.com/a.png-0");
77+
expect(children[1].key).toBe("https://cdn.example.com/b.png-1");
78+
});
79+
80+
test("img without src is left alone", () => {
81+
const html = "<img>";
82+
const result = parseHtml({ html }) as React.ReactElement;
83+
expect(result.key).toBeNull();
84+
});
85+
86+
test("img with data: URI is not keyed (inline, no network fetch)", () => {
87+
const longPayload = "A".repeat(10_000);
88+
const html = `<img src="data:image/png;base64,${longPayload}">`;
89+
const result = parseHtml({ html }) as React.ReactElement;
90+
// No remount-on-src needed for inline images, so we leave the key
91+
// unset rather than bloat it with the base64 payload.
92+
expect(result.key).toBeNull();
93+
});
94+
95+
test("img with uppercase DATA: URI is also skipped (scheme is case-insensitive)", () => {
96+
const html = `<img src="DATA:image/png;base64,${"A".repeat(100)}">`;
97+
const result = parseHtml({ html }) as React.ReactElement;
98+
expect(result.key).toBeNull();
99+
});
100+
101+
test("img wrapped by data-tooltip is still keyed by src", () => {
102+
const html =
103+
'<img src="https://cdn.example.com/a.png" data-tooltip="hi" alt="a">';
104+
const result = parseHtml({ html }) as React.ReactElement;
105+
// Outer Tooltip carries the src-based key so it remounts on src change,
106+
// forcing the inner <img> to remount as well.
107+
expect(result.key).toBe("https://cdn.example.com/a.png-0");
108+
});
109+
110+
test("img wrapped by data-marimo-doc is still keyed by src", () => {
111+
const html =
112+
'<img src="https://cdn.example.com/b.png" data-marimo-doc="foo.bar">';
113+
const result = parseHtml({ html }) as React.ReactElement;
114+
expect(result.key).toBe("https://cdn.example.com/b.png-0");
115+
});
116+
63117
test("codehilite with copy button", () => {
64118
const html =
65119
'<div class="codehilite"><pre><code>console.log("Hello");</code></pre></div>';
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# /// script
2+
# requires-python = ">=3.11"
3+
# dependencies = [
4+
# "marimo",
5+
# ]
6+
# ///
7+
# Smoke test for stale <img> rendering when mo.Html re-runs with new src URLs.
8+
#
9+
# Repro: pick a set, run the cell, then change the radio to swap the
10+
# URLs. Each <img> should display the newly selected image. Before the
11+
# RenderHTML key-by-src fix, the prior images stayed painted.
12+
13+
import marimo
14+
15+
__generated_with = "0.23.5"
16+
app = marimo.App()
17+
18+
19+
@app.cell
20+
def _():
21+
import marimo as mo
22+
23+
return (mo,)
24+
25+
26+
@app.cell
27+
def _(mo):
28+
sets = {
29+
"set A (cats)": [
30+
"https://placecats.com/200/200",
31+
"https://placecats.com/201/200",
32+
"https://placecats.com/202/200",
33+
"https://placecats.com/203/200",
34+
"https://placecats.com/204/200",
35+
],
36+
"set B (bears)": [
37+
"https://placebear.com/200/200",
38+
"https://placebear.com/201/200",
39+
"https://placebear.com/202/200",
40+
"https://placebear.com/203/200",
41+
"https://placebear.com/204/200",
42+
],
43+
}
44+
choice = mo.ui.radio(options=list(sets), value="set A (cats)")
45+
choice
46+
return choice, sets
47+
48+
49+
@app.cell
50+
def _(choice, mo, sets):
51+
urls = sets[choice.value]
52+
imgs = "".join(
53+
f'<img src="{u}" width="120" height="120" style="margin:4px"/>'
54+
for u in urls
55+
)
56+
mo.Html(f'<div>{imgs}</div>')
57+
return
58+
59+
60+
@app.cell
61+
def _(mo):
62+
mo.md("""
63+
### What to check
64+
- Toggle the radio between the two sets repeatedly.
65+
- Each `<img>` should swap to the newly selected URL.
66+
- Open devtools and confirm the rendered `<img>` `src` attributes
67+
match the selected set, and that the painted images match too.
68+
""")
69+
return
70+
71+
72+
if __name__ == "__main__":
73+
app.run()

0 commit comments

Comments
 (0)