fix: keep Image error state in React so images recover - #1125
Open
ebeigarts wants to merge 1 commit into
Open
Conversation
Image hid failed loads by writing display:none directly to the DOM node. React never manages that inline style, so the hide survived every later render: once an image errored it stayed invisible for the rest of the session, even after src changed to a working URL. Streaming makes this routine. The <img> mounts while src is still arriving (the parser auto-closes the unterminated string, so a truncated URL is a valid prop), the browser errors on it, and the completed URL then loads fine but stays hidden. Track the failure in state and clear it on load, hiding via an --error class, matching ImageBlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@ebeigarts is attempting to deploy a commit to the thesys-devs Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Imagesometimes renders a valid, fully-loaded image that is invisible.The error path wrote
display: nonestraight onto the DOM node:React does not manage that inline style —
style={styles}is normallyundefined, so React never writes astyleattribute and never has a reason to overwrite it. The hide therefore survives every later render, including a change ofsrc. There is noonLoadto undo it and no retry, so one failure hides that image for the rest of the session.Streaming makes this routine rather than rare. The
<img>mounts whilesrcis still arriving —autoClose()(packages/lang-core/src/parser/statements.ts) terminates the unterminated string so partial input parses, and nothing in the renderer skips nodes flaggedpartial, so a truncated URL is a valid prop and gets rendered. The browser fireserroron it,display: noneis written, and then the completed URL arrives, loads fine, and stays hidden.Observed in the wild as a correct
srcthat returns HTTP 200 sitting next tostyle="display: none;":ImageBlockalready handles this correctly (React state, andonLoadresetshasError); this bringsImagein line with it.Changes
packages/react-ui/src/components/Image/Image.tsx— track the failure in React state (hasError), set it inonError, clear it inonLoad, and apply the hide through anopenui-image--errorclass instead of mutatingnode.style.packages/react-ui/src/components/Image/image.scss— add&--error { visibility: hidden; }, matching the--errormodifiers inimageBlock.scss.Behaviour after the change: the truncated
srcerrors and hides, the completed URL is committed, the browser loads it (neithervisibilitynordisplayblocks the fetch),onLoadfires, and the image becomes visible.Test Plan
Verified the browser semantics this fix relies on, in Chrome 152, logging every
load/errorwith thesrcat dispatch time:error(truncated) →load(full)load(full) onlyerror(404) onlyAn aborted request delivers nothing, matching the spec's abort the image request ("discarding any pending tasks generated by that algorithm"), so an
<img>only ever reports events for its current request and a boolean flag cannot be poisoned by a stale event.Also confirmed the reported URL is healthy (HTTP 200,
image/webp, 34 KB, with and without aReferer), i.e. the hide was self-inflicted rather than a broken asset.No regression test is included:
react-uihas no jsdom or testing-library setup and its existing component tests are SSR-string only, so realload/errorevents cannot be exercised.jsdomis already in the workspace catalog and used byreact-lang, so I can add it and write a test here if you would like that in this PR.Checklist
On the issue link: I did not find an existing issue for this, and I opened this PR before filing one, contrary to the issue-first flow in
CONTRIBUTING.md— happy to open an issue and move the discussion there if you prefer.On backwards compatibility: the hide moves from an inline
display: noneto a class applyingvisibility: hidden. Inside theAspectRatio.Rootwrapper the element already occupies a fixed box, so the two look the same, andvisibilitymatchesImageBlock. Anyone who was targeting the old inline style would be affected, but the newopenui-image--errorclass is a more stable hook. Theconsole.erroron failure is dropped to matchImageBlock; it was also firing on every partial-srcmount, i.e. for images that load fine a moment later.Note for reviewers
This fix stops the symptom but not the cause: the renderer still mounts
partialnodes, so the request for the truncated URL is still made and still fails. Deferring<img>mounting (or debouncingsrc) while a node ispartialwould fix it at the source and help any other URL-valued prop with the same shape. Out of scope here — let me know if you would like an issue for it.