Repository navigation
fix(ui): prevent automatic remote Markdown images - #909
Conversation
|
nice |
 [Medium impact] The changes affect report previews and the library picker in the UI. No blocking behavior defect was established, but the documentation and comment must meet repository requirements before merging. Findings
Fix with agent prompt### Issue 1
ui/src/components/LibraryPicker.tsx:264
Removing the thumbnails makes `arc/storage.md` falsely say the library picker loads images from alphaXiv’s CDN. The repository guide requires fixing essay statements that a change makes false. Update that statement before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
### Issue 2
ui/tests/htmlPreviewAssets.test.mjs:56
This assertion checks only that the mock recorded a policy node. The mock’s `outerHTML` is empty, so the test never checks the returned HTML or whether a browser blocks image requests. It can pass even if image blocking breaks.
Add a repeatable browser test for remote images, CSS backgrounds, script-created images, and an allowed local image, while confirming scripts still run.
### Issue 3
ui/src/components/HtmlPreview.tsx:92-93
The new comment spans two lines, but the repository guide requires comments to stay on one line. Shorten it to one line explaining why images are restricted while scripts remain enabled. This repository requirement must be met before merging.
```suggestion
// Block network images while keeping inlined project images and report scripts enabled.
```
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe PR adds an image-only policy to HTML report previews and replaces remote library thumbnails with the existing file icon.
Reviews (1) · Last reviewed commit: "fix(ui): prevent automatic remote image ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
| const resolved = []; | ||
| await exports.assembleDocument("", (src) => { resolved.push(src); return `/root/${src}`; }, new AbortController().signal); | ||
| assert.ok(doc.headNodes.some((node) => node.attributes["http-equiv"] === "Content-Security-Policy" && node.attributes.content === "img-src data: blob:")); |
There was a problem hiding this comment.
This assertion checks only that the mock recorded a policy node. The mock’s outerHTML is empty, so the test never checks the returned HTML or whether a browser blocks image requests. It can pass even if image blocking breaks.
Add a repeatable browser test for remote images, CSS backgrounds, script-created images, and an allowed local image, while confirming scripts still run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/tests/htmlPreviewAssets.test.mjs
Line: 56
Comment:
**Image blocking lacks browser coverage**
This assertion checks only that the mock recorded a policy node. The mock’s `outerHTML` is empty, so the test never checks the returned HTML or whether a browser blocks image requests. It can pass even if image blocking breaks.
Add a repeatable browser test for remote images, CSS backgrounds, script-created images, and an allowed local image, while confirming scripts still run.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| // Project images are inlined below. Prevent automatic network image loads, | ||
| // including srcset and CSS backgrounds, without disabling report scripts. |
There was a problem hiding this comment.
The new comment spans two lines, but the repository guide requires comments to stay on one line. Shorten it to one line explaining why images are restricted while scripts remain enabled. This repository requirement must be met before merging.
| // Project images are inlined below. Prevent automatic network image loads, | |
| // including srcset and CSS backgrounds, without disabling report scripts. | |
| // Block network images while keeping inlined project images and report scripts enabled. |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/HtmlPreview.tsx
Line: 92-93
Comment:
**Comment breaks the format rule**
The new comment spans two lines, but the repository guide requires comments to stay on one line. Shorten it to one line explaining why images are restricted while scripts remain enabled. This repository requirement must be met before merging.
```suggestion
// Block network images while keeping inlined project images and report scripts enabled.
```
**Context Used:** AGENTS.md ([source](https://github.com/alphaxiv/openresearch/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
the two changes here are right for what they cover: the blocking, remaining places that still load external images or media:
what to do with a blocked external image in markdown is a taste call. dropping it silently hides that something was there; rendering the alt text or a plain link keeps it visible. non-blocking: Clover's concern is that more restrictions could break existing html reports. reports that embed remote images or media are expected to break under this policy, so that isn't a reason to hold back. the one case worth confirming is local files: a report referencing a local image that the inliner skips (over the 4 MB budget, 200 file cap, or |
29d1f26 to
2b8ac2a
Compare
|
thanks for covering the markdown and media gaps, the link fallback with alt text is what we wanted. the review turned up one change of direction and a conflict. blocking: drop the html preview restriction. markdown images are passive, so a remote blocking: the branch conflicts with main in once those are in and checks are green, this is good to merge. |
Assisted-by: gpt-6.1-terra
164cf54 to
360ab31
Compare
dajiaohuang
changed the title
Chat and Artifacts Markdown now share one rehype plugin for remote images, so a badge inside a link renders as its text instead of a nested anchor, and remote images no longer form an image carousel of links. The happy-dom test runs in `pnpm test`, replacing the Playwright test CI never ran, and the now-unread `versionId` and `coverBlobId` paper fields go. Assisted-by: claude-opus-5.5 Assisted-by: gpt-6.1-sol
|
we got this. |
Closes #908
Chat and Artifacts Markdown loaded remote images on render, and the library picker loaded remote cover thumbnails. A remote Markdown image now renders as a link labelled with its alt text or URL (plain text when it already sits inside a link), local images still render, and the picker shows its file icon. HTML report previews keep loading remote assets: a report is code the user chose to open.
Proved with
ui/tests/markdownImages.test.mjsinpnpm test(fails with the plugin disabled; full suite 698/698), typecheck and lints, and a browser render of both surfaces showing no remote requests or nested links.Reviewed with sol-loop: 1 round with gpt-6.1-sol. Sol verdict: approved
Assisted-by: claude-opus-5.5
Assisted-by: gpt-6.1-sol