vndangkhoa/open-design
1
0
Fork 0
mirror of https://github.com/nexu-io/open-design.git synced 2026-06-01 03:14:35 +07:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

fix(#2361): patch both causes that present as blank preview (#2805)

Browse source 
* fix(web): treat external <script src> as needing the sandbox shim (#2361)

Agent-emitted HTML artifacts that read localStorage from an external
boot.js / app.js currently render blank in the preview pane because the
URL-load iframe's sandbox lacks allow-same-origin and htmlNeedsSandboxShim
only scans the HTML string. The "Known limitation" comment already
anticipated this case; #2361 is the reported case justifying the cost.
Conservatively route any HTML with an external <script src=> through the
srcDoc path so injectSandboxShim is in place before the script runs.

* fix(web): stop infinite srcDoc re-activate loop that blanks animated previews (#2361)

The lazy srcDoc transport iframe fires its 'load' event twice for one successful activation: once when the empty transport shell HTML loads, and again when our own document.open()/write()/close() inside the shell finishes. PR #2699 made the onLoad handler unconditionally reset activatedSrcDocTransportHtmlRef.current = null so that switching preview -> source -> preview (which remounts the iframe as a brand new DOM node) would re-activate the new shell. But that reset also fires on the second load of an unchanged frame, which re-triggers activateSrcDocTransport, which re-runs document.open/write/close, which re-fires the load event, ad infinitum. In one local reproduction the dedupe ref was cleared and re-activated 4763 times before the test was stopped.

Each iteration rebuilds the document, which restarts every CSS animation from its 'from' keyframe. Designs that use 'animation-fill-mode: both' with 'from { opacity: 0 }' (very common for editorial hero fades) therefore stay at opacity 0 forever and the preview reads as blank. In React strict mode + HMR (pnpm tools-dev) the symptom is visible high-frequency flashing; in a packaged production build the loop runs cool enough that the user only sees a stable blank — both are the same root cause.

This change keeps PR #2699's remount-after-Source-toggle behavior by tracking which iframe DOM node we last reset for in a new srcDocFrameDedupeResetForRef. The reset runs exactly once per freshly mounted iframe (the first load is the shell HTML) and is skipped on every subsequent load of the same node (those are the document.write loads). Switching source back to preview remounts the iframe as a fresh DOM node, so the reset still happens and PR #2699's regression test still passes; ordinary srcDoc renders no longer enter the infinite loop.

Refs #2361

* chore: re-trigger CI

Upstream's fork-pr-workflow-approval check hit a transient 401 Bad credentials when calling the GitHub API on the previous run; the underlying workflow has nothing to do with the code in this PR. Pushing an empty commit to re-run the workflow chain.

* chore: re-trigger CI (retry transient checkout race)

First re-trigger surfaced a transient race in ci / Build workspaces (actions/checkout failed to fetch refs/remotes/pull/2805/merge with 'could not read Username for https://github.com'). Other concurrent fork PRs' Build workspaces all passed on the same upstream runner, so this is not a token/permission infra issue — likely just a per-PR fetch race after the previous push. Pushing a second empty commit to retry the workflow chain.
This commit is contained in:
张东明 2026-05-24 22:29:36 +08:00 committed by GitHub
parent 0134566ffb
commit 53dfb8808c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 106 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
apps/web/src/components/FileViewer.tsx
View file

@ -3723,6 +3723,13 @@ function HtmlViewer({
const urlPreviewIframeRef = useRef<HTMLIFrameElement | null>(null);
const srcDocPreviewIframeRef = useRef<HTMLIFrameElement | null>(null);
const activatedSrcDocTransportHtmlRef = useRef<string | null>(null);
// Tracks the iframe DOM node whose dedupe ref was last reset by the
// srcDoc onLoad handler. We reset the dedupe exactly once per freshly
// mounted iframe (the first load is the shell HTML), and skip every
// subsequent load on the same node (those are our own
// document.open/write/close inside the shell). See onLoad below for
// the infinite-loop story (issue #2361).
const srcDocFrameDedupeResetForRef = useRef<HTMLIFrameElement | null>(null);
const isActivePreviewIframeSource = useCallback((source: MessageEventSource | null) => {
return !!source && source === iframeRef.current?.contentWindow;
}, []);
@ -6437,15 +6444,40 @@ function HtmlViewer({
onLoad={() => {
const frame = srcDocPreviewIframeRef.current;
if (!useUrlLoadPreview) iframeRef.current = frame;
// Any srcDoc iframe load means we are talking to a
// fresh document shell. Clear the activation dedupe so
// switching preview -> source -> preview cannot strand
// the new shell on the blank transport page.
activatedSrcDocTransportHtmlRef.current = null;
// Belt-and-suspenders for the ready handshake: if the
// postMessage racing the parent's listener registration
// ever loses, the load event still tells us the shell
// script ran to completion.
// Reset the activation dedupe exactly ONCE per
// freshly mounted iframe DOM node, never on the
// subsequent load events that the same node
// emits during normal srcDoc rendering.
//
// The iframe's load event fires twice for one
// successful activation: once when the lazy
// transport shell HTML loads, and again when
// our own document.open/write/close inside the
// shell finishes. PR #2699 reset the dedupe on
// every load so that switching
// preview -> source -> preview (which remounts
// this iframe as a fresh DOM node) would
// re-activate the new shell. But resetting on
// every load also re-activated on the SECOND
// load of a non-remounted frame, which
// re-triggered document.open/write/close, which
// re-fired the load event, ad infinitum. The
// dedupe ref oscillated between null and the
// current srcDoc thousands of times per render
// and each iteration restarted every CSS
// animation from its `from` keyframe. Designs
// using `animation-fill-mode: both` with
// `from { opacity: 0 }` stayed at opacity 0
// forever and the preview read as blank.
// That is issue #2361.
//
// Tracking the last frame we reset for lets us
// keep PR #2699's "remount after Source toggle"
// fix while breaking the loop on plain renders.
if (frame && srcDocFrameDedupeResetForRef.current !== frame) {
srcDocFrameDedupeResetForRef.current = frame;
activatedSrcDocTransportHtmlRef.current = null;
}
if (useLazySrcDocTransport) setSrcDocShellReady(true);
activateSrcDocTransport(frame);
dcViewportRestoreAtRef.current = Date.now();

36
apps/web/src/components/file-viewer-render-mode.ts
View file

@ -117,7 +117,7 @@ export function parseForceInline(search: string | URLSearchParams | null | undef
* serves raw HTML untouched, so artifacts that touch sandbox-blocked Web
* Storage at startup go blank.
*
* Scope is narrow on purpose. This helper detects two reliable signals
* Scope is narrow on purpose. This helper detects three reliable signals
* visible in the *document* source and routes those artifacts back through
* srcDoc by toggling `forceInline`:
*
@ -127,18 +127,24 @@ export function parseForceInline(search: string | URLSearchParams | null | undef
* Storage from `useState` initializers.
* - Direct `localStorage` / `sessionStorage` mentions in the document
* source (covers inline scripts and plain HTML that calls them).
* - Any external `<script src="…">` (including `type="module"`): the
* parent string scan can't see the linked subresource's body, and
* agent-emitted artifacts commonly read Web Storage from an external
* `boot.js` / `app.js` at module eval (issue #2361). Conservatively
* route any external script through srcDoc so the shim is in place
* before that read happens. The alternative — fetching every script
* URL ahead of the iframe and scanning it — would duplicate work the
* browser is about to do and add round trips on every preview load,
* so the heuristic favors a few extra srcDoc-mode previews over those
* additional requests.
*
* Known limitation: a `<script src="./app.js">` (or
* `<script type="module" src="./main.js">`) whose **external** file reads
* Web Storage at module eval is *not* covered — the helper only sees the
* HTML, not the linked subresource, so URL-load is still chosen and the
* SecurityError still throws. Catching that case would require fetching
* and scanning every script reference before deciding the iframe load
* strategy, which duplicates work the browser is about to do and adds
* round trips on every preview load. Leaving that path uncovered until
* there's a reported case that justifies the cost. Workaround for now:
* users can opt the artifact into srcDoc with `?forceInline=1` or by
* toggling Tweaks.
* Remaining known limitation: dynamically injected scripts
* (`document.createElement('script'); s.src = '…'; head.appendChild(s)`)
* are still invisible to this scan because the literal `<script src=…>`
* tag never appears in the source. Such artifacts will still URL-load and
* still throw on Web Storage access at startup. Workaround for now: users
* can opt the artifact into srcDoc with `?forceInline=1` or by toggling
* Tweaks.
*
* Pure string scan — caller passes the same `source` already fetched for
* preview rendering, so this adds no extra I/O. Heuristic by design: false
@ -155,5 +161,11 @@ export function htmlNeedsSandboxShim(source: string): boolean {
// reject hyphenated variants if a real case ever surfaces.
if (/<script\s[^>]*\btype\s*=\s*["']?text\/babel\b/i.test(source)) return true;
if (/\b(?:local|session)Storage\b/.test(source)) return true;
// External `<script ... src=...>` — see issue #2361. `\s[^>]*?` requires at
// least one whitespace after `<script` (so we don't match `<scripts>`-like
// text or self-closing-ish edge cases) and stays non-greedy to keep the
// search bounded to the tag itself. Lazy match avoids spilling into
// unrelated `src=` attributes on later tags in the same document.
if (/<script\s[^>]*?\bsrc\s*=/i.test(source)) return true;
return false;
}

44
apps/web/tests/components/file-viewer-render-mode.test.ts
View file

@ -193,12 +193,14 @@ describe('htmlNeedsSandboxShim', () => {
expect(htmlNeedsSandboxShim('<script type=text/babelish></script>')).toBe(false);
});
it('does not match plain <script> tags or unrelated MIME types', () => {
expect(htmlNeedsSandboxShim('<script src="app.js"></script>')).toBe(false);
expect(htmlNeedsSandboxShim('<script type="module" src="app.js"></script>')).toBe(false);
it('does not match unrelated MIME types or inline-only <script> tags', () => {
// Inline JSON data island — no executable code, no Web Storage access.
expect(htmlNeedsSandboxShim('<script type="application/json">{}</script>')).toBe(false);
// Substring-only matches must not trigger (e.g. text/babel-like custom type).
expect(htmlNeedsSandboxShim('<script type="text/babelish"></script>')).toBe(false);
// A bare inline <script> without src= and without a Web Storage mention
// is left alone (URL-load can render it fine without the shim).
expect(htmlNeedsSandboxShim('<script>console.log("hi")</script>')).toBe(false);
});
it('detects direct localStorage / sessionStorage references in the source', () => {
@ -214,4 +216,40 @@ describe('htmlNeedsSandboxShim', () => {
expect(htmlNeedsSandboxShim('mylocalStorageWrapper')).toBe(false);
expect(htmlNeedsSandboxShim('SuperLocalStorage')).toBe(false);
});
// Issue #2361 — Tweaks and animations problems
// Agent-emitted artifacts commonly read `localStorage` from an *external*
// script (e.g. `<script src="boot.js">` that initializes theme/language).
// The parent string scan can't see the script body, so prior to #2361 the
// helper returned false, the preview took the URL-load path, and the
// sandboxed iframe threw `SecurityError` on first read — leaving the
// artifact blank until the user toggled Tweaks (which forces srcDoc and
// pulls in `injectSandboxShim`). Conservatively route any external script
// through srcDoc so the shim is available from the start.
it('flags any external <script src=> as needing the shim (issue #2361)', () => {
// Plain external script — the original reporter's repro shape.
expect(htmlNeedsSandboxShim('<script src="boot.js"></script>')).toBe(true);
// ES module import.
expect(htmlNeedsSandboxShim('<script type="module" src="main.js"></script>')).toBe(true);
// Attributes between <script and src= (defer / async / nonce / crossorigin).
expect(htmlNeedsSandboxShim('<script defer src="./app.js"></script>')).toBe(true);
expect(htmlNeedsSandboxShim('<script async src="https://cdn.example.com/lib.js"></script>')).toBe(true);
// Single-quoted src.
expect(htmlNeedsSandboxShim("<script src='./bundle.js'></script>")).toBe(true);
// Whitespace around the equals sign.
expect(htmlNeedsSandboxShim('<script src = "./bundle.js"></script>')).toBe(true);
// Unquoted src value (HTML5 permits unquoted attrs).
expect(htmlNeedsSandboxShim('<script src=boot.js></script>')).toBe(true);
// Case-insensitive tag name.
expect(htmlNeedsSandboxShim('<SCRIPT SRC="boot.js"></SCRIPT>')).toBe(true);
});
it('does not match incidental "src=" in non-script contexts (issue #2361 regression)', () => {
// `<img src=>` is not an executable subresource for our purposes.
expect(htmlNeedsSandboxShim('<img src="logo.png">')).toBe(false);
// `<link rel="stylesheet" href=>` similarly does not run JavaScript.
expect(htmlNeedsSandboxShim('<link rel="stylesheet" href="styles.css">')).toBe(false);
// Text content mentioning `script src=` (e.g. a docs page) must not trigger.
expect(htmlNeedsSandboxShim('<p>Use <code>&lt;script src=&quot;app.js&quot;&gt;</code></p>')).toBe(false);
});
});