1430 image loading blocking p5 javascript projects - #1609
Conversation
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Fixes a preview/runtime mismatch where JavaScript projects (notably p5 sketches) reference project media by filename (e.g. cat.png), which the preview iframe cannot resolve, causing sketches to hang during load.
Changes:
- Extend HTML preview media substitution to rewrite media filenames inside JS (in addition to CSS) before creating blob URLs for the iframe.
- Add a regression test ensuring JavaScript media references are rewritten to their resolved media URLs.
- Add a
draw-a-catsample project (and link) to reproduce the original p5 image-loading issue.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web-component.html | Adds a sample-project link to load draw-a-cat in the local web-component harness. |
| src/projects/draw-a-cat.json | Introduces a p5-based repro project that loads cat.png via loadImage("cat.png"). |
| src/components/Editor/Runners/HtmlRunner/HtmlRenderer.test.jsx | Adds a test that verifies JS blob contents have media filenames substituted with their resolved URLs. |
| src/components/Editor/Runners/HtmlRunner/HtmlRenderer.jsx | Implements filename→URL substitution for JS (and hardens replacement behavior for URLs containing $ sequences). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
| const escapeForRegExp = (string) => | ||
| string.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
|
|
||
| const substituteProjectMedia = (projectFile, projectMedia) => { | ||
| let updatedProjectFile = { ...projectFile }; | ||
| if (projectFile.extension === "css") { | ||
| projectMedia.forEach((media_file) => { | ||
| const find = new RegExp(`['"]${media_file.filename}['"]`, "g"); // prevent substring matches | ||
| const replace = `"${media_file.url}"`; | ||
| if (mediaSubstitutedExtensions.includes(projectFile.extension)) { | ||
| projectMedia.forEach((mediaFile) => { | ||
| const find = new RegExp( | ||
| `['"]${escapeForRegExp(mediaFile.filename)}['"]`, | ||
| "g", | ||
| ); // prevent substring matches | ||
| updatedProjectFile.content = updatedProjectFile.content.replaceAll( | ||
| find, | ||
| replace, | ||
| () => `"${mediaFile.url}"`, // callback, so $-sequences in the URL are not expanded | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I was thinking about ways to simplify this, I since escapeForRegExp looks hard to understand and might be easy to get wrong.
One way might be avoid the need to build a regex by calling replaceAll with the different strings:
updatedProjectFile.content.replaceAll(
`"${mediaFile.filename"}"`,
replace,
() => `"${mediaFile.url}"`, // callback, so $-sequences in the URL are not expanded
).replaceAll(
`'${mediaFile.filename"}'`,
replace,
() => `"${mediaFile.url}"`, // callback, so $-sequences in the URL are not expanded
)Slightly less efficient, but avoids needing to construct a regular expression.
If you don't think that would work, we could keep the regexp, but it would be good to have a least another unit test that covers the need to escape it.
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Nice one of fixing this bug, I think your the third person to look at it and the first to make progress 😀
I've added a suggestion on how to simplify the code by avoiding the need for escaping regexes.
@zetter-rpf Thanks the review, made a change to use |
Nice one, I think that's a lot simpler |
closes #1430