Skip to content

fix: add sandbox attribute to iframe for enhanced security#1709

Merged
PupilTong merged 3 commits intolynx-family:mainfrom
PupilTong:p/hw/iframe-sandbox
Sep 16, 2025
Merged

fix: add sandbox attribute to iframe for enhanced security#1709
PupilTong merged 3 commits intolynx-family:mainfrom
PupilTong:p/hw/iframe-sandbox

Conversation

@PupilTong
Copy link
Copy Markdown
Collaborator

@PupilTong PupilTong commented Sep 10, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Embedded iframes now use stricter sandboxing and adjusted loading for improved security and stability.
    • Updated mobile capability meta to better reflect mobile web behavior.
  • Performance

    • Template loading deduplicates concurrent requests to avoid duplicate fetches and improve reliability.
  • Tests

    • Added per‑browser skip logic for WebKit on several browser tests to reduce flaky failures.
  • Chores

    • Published a patch release and updated release metadata.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).
  • Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 10, 2025

🦋 Changeset detected

Latest commit: abdd817

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@lynx-js/web-core Patch
@lynx-js/web-core-server Patch
@lynx-js/web-constants Patch
@lynx-js/web-mainthread-apis Patch
@lynx-js/web-worker-rpc Patch
@lynx-js/web-worker-runtime Patch
@lynx-js/web-style-transformer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 10, 2025

📝 Walkthrough

Walkthrough

Adds two changeset entries; switches the UI iframe to use srcdoc with a restricted sandbox and adjusted script loading; changes template caching to a Map that caches in-flight Promises for deduplication; and adds WebKit skips to three Playwright tests. No public API signatures changed.

Changes

Cohort / File(s) Summary
Changesets / Release metadata
.changeset/rich-candles-think.md, .changeset/rude-owls-cheer.md
Adds two changeset entries bumping @lynx-js/web-core (patch) documenting iframe sandbox/security and template-loader deduplication; no code changes in the changeset files.
UI thread — iframe & script loading
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts
Replaces iframe.src with iframe.srcdoc, sets iframe.sandbox = 'allow-same-origin allow-scripts' and iframe.loading = 'eager'; removes explicit iframe readiness wait; pre-creates and appends <script> into iframe document (fetchPriority='high', defer=true, async=false), sets src after attaching handlers, resolves with iframeWindow.module.exports and clears iframeWindow.module; loadScriptSync adjusted similarly.
Template loader — caching & dedupe
packages/web-platform/web-core/src/utils/loadTemplate.ts
Replaces plain-object cache with `Map<string, Promise
Tests — Playwright per-browser skips
packages/web-platform/web-tests/tests/react.spec.ts
Adds browserName parameter to three tests and inserts test.skip(browserName === 'webkit', 'playwright issue, tested locally') to avoid WebKit flakiness; test logic otherwise unchanged.
Test config — meta tag tweak
packages/web-platform/web-tests/rspack.config.js
Changes the first HtmlRspackPlugin meta entry from 'apple-mobile-web-app-capable': 'yes' to 'mobile-web-app-capable': 'yes'.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • Sherry-hue
  • colinaaa

Poem

I nibble bytes beneath the moonlight,
I tuck a srcdoc snug and tight.
Sandbox gates keep mischief thin,
Templates queue — no double spin. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: add sandbox attribute to iframe for enhanced security" is concise and directly matches the primary change in the PR; the diffs show iframe.sandbox being set (packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts) and related iframe adjustments, so the title accurately summarizes the main change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67ee3e6 and 4f8610b.

📒 Files selected for processing (2)
  • .changeset/rich-candles-think.md (1 hunks)
  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md

📄 CodeRabbit inference engine (AGENTS.md)

For contributions, always generate a changeset and commit the resulting markdown file(s)

Files:

  • .changeset/rich-candles-think.md
🧠 Learnings (2)
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.

Applied to files:

  • .changeset/rich-candles-think.md
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.645Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.

Applied to files:

  • .changeset/rich-candles-think.md
🔇 Additional comments (1)
.changeset/rich-candles-think.md (1)

1-6: Changeset looks correct for a user-facing security fix.

Patch bump and summary are appropriate.

Comment thread packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts Outdated
@relativeci
Copy link
Copy Markdown

relativeci Bot commented Sep 10, 2025

React Example

#5248 Bundle Size — 237.52KiB (0%).

abdd817(current) vs 93d707b main#5243(baseline)

Bundle metrics  no changes
                 Current
#5248
     Baseline
#5243
No change  Initial JS 0B 0B
No change  Initial CSS 0B 0B
No change  Cache Invalidation 0% 0%
No change  Chunks 0 0
No change  Assets 4 4
No change  Modules 162 162
No change  Duplicate Modules 65 65
No change  Duplicate Code 46.71% 46.71%
No change  Packages 2 2
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#5248
     Baseline
#5243
No change  IMG 145.76KiB 145.76KiB
No change  Other 91.76KiB 91.76KiB

Bundle analysis reportBranch PupilTong:p/hw/iframe-sandboxProject dashboard


Generated by RelativeCIDocumentationReport issue

@relativeci
Copy link
Copy Markdown

relativeci Bot commented Sep 10, 2025

Web Explorer

#5241 Bundle Size — 365.07KiB (+0.07%).

abdd817(current) vs 93d707b main#5236(baseline)

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#5241
     Baseline
#5236
Regression  Initial JS 145.65KiB(+0.16%) 145.41KiB
No change  Initial CSS 31.89KiB 31.89KiB
Change  Cache Invalidation 39.86% 6.72%
No change  Chunks 8 8
No change  Assets 8 8
Change  Modules 218(-0.46%) 219
No change  Duplicate Modules 16 16
No change  Duplicate Code 3.37% 3.37%
No change  Packages 4 4
No change  Duplicate Packages 0 0
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#5241
     Baseline
#5236
Regression  JS 239.16KiB (+0.1%) 238.92KiB
No change  Other 94.02KiB 94.02KiB
No change  CSS 31.89KiB 31.89KiB

Bundle analysis reportBranch PupilTong:p/hw/iframe-sandboxProject dashboard


Generated by RelativeCIDocumentationReport issue

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Sep 10, 2025

CodSpeed Performance Report

Merging #1709 will improve performances by 6.63%

Comparing PupilTong:p/hw/iframe-sandbox (abdd817) with main (93d707b)

🎉 Hooray! codspeed-cpp just leveled up to 1.2.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 1 improvement
✅ 55 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
transform 1000 effects 32.5 ms 30.5 ms +6.63%

Sherry-hue
Sherry-hue previously approved these changes Sep 11, 2025
@PupilTong PupilTong force-pushed the p/hw/iframe-sandbox branch 2 times, most recently from f249085 to 1a9f00e Compare September 15, 2025 14:11
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

188-203: Bug: pending updates are never queued; calls can be dropped before start().

mtsGlobalThis is always truthy after realm creation, so the else branch never runs. If updatePage isn’t ready yet, calls get silently ignored via optional chaining. Queue based on updatePage presence:

-    if (mtsGlobalThis) {
-      mtsGlobalThis.updatePage?.(...args);
-    } else {
-      // Cache the call if mtsGlobalThis is not yet initialized
-      pendingUpdateCalls.push(args);
-    }
+    if (mtsGlobalThis.updatePage) {
+      mtsGlobalThis.updatePage(...args);
+    } else {
+      // Cache until updatePage is available
+      pendingUpdateCalls.push(args);
+    }

Optionally, update the comment near Lines 188-193 to reflect that we flush calls queued while updatePage was undefined.

♻️ Duplicate comments (1)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

44-46: Fix: sandbox assignment is a TS type error; use DOMTokenList or setAttribute.

iframe.sandbox is a DOMTokenList (read‑only for strings). Assigning a string won’t type‑check and may not behave as intended. Also ensure sandbox is set before navigation. Apply:

-  iframe.srcdoc = '<!DOCTYPE html><html><head></head><body></body></html>';
-  iframe.sandbox = 'allow-same-origin allow-scripts'; // Restrict capabilities for security
+  // Set sandbox tokens via attribute to satisfy TS and browsers.
+  iframe.setAttribute('sandbox', 'allow-same-origin allow-scripts');
+  iframe.srcdoc = '<!DOCTYPE html><html><head></head><body></body></html>';
🧹 Nitpick comments (3)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

50-54: Use attribute for fetch priority and drop defer (ignored on dynamic scripts).

HTMLScriptElement.fetchPriority may not be in your lib.dom types; using the attribute is safer. defer has no effect on dynamically inserted scripts; async = false already preserves insertion order.

-    const script = iframeDocument.createElement('script');
-    script.fetchPriority = 'high';
-    script.defer = true;
-    script.async = false;
+    const script = iframeDocument.createElement('script');
+    script.setAttribute('fetchpriority', 'high'); // TS-safe; no-op if unsupported
+    script.async = false; // preserve insertion-order execution
packages/web-platform/web-core/src/utils/loadTemplate.ts (2)

23-27: Timing metric fires too early on cache hits (and for in‑flight Promises).

load_template_end triggers before the template is actually available when the cache holds a Promise, skewing metrics. Fire the end marker when the returned value (Promise or value) settles:

-    const cachedTemplate = templateCache.get(url);
-    if (cachedTemplate) {
-      markTimingInternal('load_template_end');
-      return cachedTemplate;
+    const cachedTemplate = templateCache.get(url);
+    if (cachedTemplate) {
+      return Promise.resolve(cachedTemplate).finally(() =>
+        markTimingInternal('load_template_end'),
+      );

28-52: Deduplicate loads and mark timing on settle; replace cached Promise with resolved value.

Store the Promise on miss, but swap to the resolved template to reduce retained closures and stack traces. Also move the end marker to .finally() to capture full load/decoding time.

-      const promise = new Promise<LynxTemplate>(async (resolve, reject) => {
+      const promise = new Promise<LynxTemplate>(async (resolve, reject) => {
         try {
           const template = customTemplateLoader
             ? await customTemplateLoader(url)
             : (await (await fetch(url, {
               method: 'GET',
             })).json()) as LynxTemplate;
           const decodedTemplate = await generateTemplate(
             template,
             createJsModuleUrl,
           );
           resolve(decodedTemplate);
         } catch (e) {
           templateCache.delete(url);
           reject(e);
         }
       });
-      templateCache.set(url, promise);
+      templateCache.set(url, promise);
+      // Replace in-flight entry with resolved value to reduce memory/stack retention.
+      promise.then((t) => {
+        templateCache.set(url, t);
+      }).catch(() => {
+        /* noop: deletion already handled above */
+      });
       /**
        * This will cause a memory leak, which is expected.
        * We cannot ensure that the `URL.createObjectURL` created url will never be used, therefore here we keep it for the entire lifetime of this page.
        */
-      markTimingInternal('load_template_end');
-      return promise;
+      return promise.finally(() => markTimingInternal('load_template_end'));
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a9f00e and b27db7b.

📒 Files selected for processing (2)
  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2 hunks)
  • packages/web-platform/web-core/src/utils/loadTemplate.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/web-platform/web-core/src/utils/loadTemplate.ts (1)
packages/web-platform/web-constants/src/utils/generateTemplate.ts (1)
  • generateTemplate (122-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: build / Build (Windows)
  • GitHub Check: build / Build (Ubuntu)
  • GitHub Check: test-rust / Test (Ubuntu)
  • GitHub Check: zizmor
🔇 Additional comments (1)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

41-49: Confirm iframe readiness before first script injection.

Given srcdoc is set and the node is appended immediately, head should exist; however, to avoid edge‑case races across browsers, consider guarding with iframeDocument.readyState === 'complete' || 'interactive' or awaiting a one‑time load before first loadScript() call.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

46-47: Type error: assigning a string to iframe.sandbox breaks TS; set tokens via attribute or DOMTokenList.

Use setAttribute or sandbox.add to satisfy types and keep same-origin access.

-  iframe.sandbox = 'allow-same-origin allow-scripts'; // Restrict capabilities for security
+  // Restrict capabilities while preserving required same-origin script execution
+  iframe.setAttribute('sandbox', 'allow-same-origin allow-scripts');
🧹 Nitpick comments (4)
packages/web-platform/web-tests/tests/react.spec.ts (2)

79-81: WebKit-only skip is fine; consider tracking the upstream issue.

Add a reference (issue link/TODO) so the skip doesn’t become permanent.


178-198: Drop the unused testInfo parameter.

The second argument is unused; simplify the signature.

-    test('basic-globalProps-reload', async ({ page, browserName }, {}) => {
+    test('basic-globalProps-reload', async ({ page, browserName }) => {
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

75-83: Create the inline script in the iframe document to avoid cross-document adoption.

Also consider removing sync XHR soon; it’s deprecated and can be blocked.

-      const script = document.createElement('script');
+      const script = iframe.contentDocument!.createElement('script');
packages/web-platform/web-core/src/utils/loadTemplate.ts (1)

8-9: Simplify cache to a single Promise type.

You only ever store Promises; narrow the type.

-const templateCache: Map<string, Promise<LynxTemplate> | LynxTemplate> =
-  new Map();
+const templateCache: Map<string, Promise<LynxTemplate>> = new Map();
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b27db7b and 82fe6b7.

📒 Files selected for processing (5)
  • .changeset/rich-candles-think.md (1 hunks)
  • .changeset/rude-owls-cheer.md (1 hunks)
  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2 hunks)
  • packages/web-platform/web-core/src/utils/loadTemplate.ts (2 hunks)
  • packages/web-platform/web-tests/tests/react.spec.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .changeset/rude-owls-cheer.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • .changeset/rich-candles-think.md
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Place Playwright E2E tests in the web platform’s web-tests suite

Files:

  • packages/web-platform/web-tests/tests/react.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:38:10.450Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.450Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite

Applied to files:

  • packages/web-platform/web-tests/tests/react.spec.ts
🧬 Code graph analysis (2)
packages/web-platform/web-tests/tests/react.spec.ts (1)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
  • test (11-60)
packages/web-platform/web-core/src/utils/loadTemplate.ts (1)
packages/web-platform/web-constants/src/utils/generateTemplate.ts (1)
  • generateTemplate (122-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build / Build (Windows)
  • GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (1)
packages/web-platform/web-tests/tests/react.spec.ts (1)

94-96: LGTM: per-browser skip added appropriately.

No functional change beyond the skip and title handling.

Comment thread packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts
Comment thread packages/web-platform/web-core/src/utils/loadTemplate.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2)

46-46: Fix: assigning a string to iframe.sandbox is a TS error (DOMTokenList).

Use attribute or DOMTokenList.add; keep same-origin + scripts.

-  iframe.sandbox = 'allow-same-origin allow-scripts'; // Restrict capabilities for security
+  // Restrict capabilities; keep same-origin so we can inject/read module exports.
+  iframe.setAttribute('sandbox', 'allow-same-origin allow-scripts');
+  // Alternatively:
+  // iframe.sandbox.add('allow-same-origin', 'allow-scripts');

50-55: Harden script injection: wait for iframe doc/head; avoid non-standard typing; drop no-op defer.

  • Guard for iframe document readiness (WebKit race-prone).
  • Use setAttribute for fetchpriority to avoid TS lib.dom gaps.
  • defer is ignored for dynamically inserted scripts; keep async=false for order.
-  const loadScript: (url: string) => Promise<unknown> = async (url) => {
-    const script = iframe.contentDocument!.createElement('script');
-    script.fetchPriority = 'high';
-    script.defer = true;
-    script.async = false;
-    iframe.contentDocument!.head.appendChild(script);
+  const loadScript: (url: string) => Promise<unknown> = async (url) => {
+    if (!iframe.contentDocument || !iframe.contentDocument.head) {
+      await new Promise<void>((resolve) =>
+        iframe.addEventListener('load', () => resolve(), { once: true }),
+      );
+    }
+    const doc = iframe.contentDocument!;
+    const script = doc.createElement('script');
+    // Hint priority without depending on TS typing support
+    script.setAttribute('fetchpriority', 'high');
+    // For dynamic scripts, defer is ignored; maintain order via async = false
+    script.async = false;
+    doc.head.appendChild(script);
🧹 Nitpick comments (2)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2)

47-47: Nit: loading="eager" is the default and has no effect here.

Consider dropping for clarity.


56-68: Tidy Promise executor and clean up script node on resolve/reject.

Avoid async executor; remove script after load; reset module on error too.

-    return new Promise(async (resolve, reject) => {
+    return new Promise((resolve, reject) => {
       script.onload = () => {
         const ret = iframeWindow?.module?.exports;
         // @ts-expect-error
         iframeWindow.module = { exports: undefined };
+        script.remove();
         resolve(ret);
       };
-      script.onerror = (err) =>
-        reject(new Error(`Failed to load script: ${url}`, { cause: err }));
+      script.onerror = (err) => {
+        // @ts-expect-error
+        iframeWindow.module = { exports: undefined };
+        script.remove();
+        reject(new Error(`Failed to load script: ${url}`, { cause: err }));
+      };
       // @ts-expect-error
       iframeWindow.module = { exports: undefined };
       script.src = url;
     });
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82fe6b7 and 64a62d7.

📒 Files selected for processing (1)
  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:37:38.587Z
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1589
File: packages/web-platform/web-worker-runtime/src/mainThread/startMainThread.ts:36-49
Timestamp: 2025-08-27T11:37:38.587Z
Learning: In web worker script loading (loadScript function), the pattern of calling fetch() before importScripts() is intentional for caching benefits, not redundant networking. The fetch() ensures HTTP caching is utilized before the synchronous importScripts() call.

Applied to files:

  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build / Build (Windows)
  • GitHub Check: build / Build (Ubuntu)
  • GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (1)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (1)

44-45: LGTM: switching to srcdoc with a minimal document is appropriate.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2)

44-48: Fix TS type error: assign sandbox via attribute or DOMTokenList, not a string.

iframe.sandbox is a DOMTokenList; assigning a string won’t type-check. Include allow-same-origin (already present) to keep DOM access.

-  iframe.srcdoc =
-    '<!DOCTYPE html><html><head></head><body style="display:none"></body></html>';
-  iframe.sandbox = 'allow-same-origin allow-scripts'; // Restrict capabilities for security
+  iframe.setAttribute('sandbox', 'allow-same-origin allow-scripts'); // Restrict capabilities while preserving DOM access
+  iframe.srcdoc =
+    '<!DOCTYPE html><html><head></head><body style="display:none"></body></html>';
   iframe.loading = 'eager';

50-63: Harden script loading: avoid non-standard property, don’t clobber onload, ensure head readiness, and clean up inserted nodes.

  • Prefer setAttribute('fetchpriority','high') over script.fetchPriority (not widely typed).
  • Use addEventListener('load', …, { once: true }) instead of overwriting onload.
  • Append the script after setting handlers and src.
  • Remove the script after it runs to avoid DOM buildup.
-  const loadScript: (url: string) => Promise<unknown> = async (url) => {
-    const script = iframe.contentDocument!.createElement('script');
-    script.fetchPriority = 'high';
-    script.defer = true;
-    script.async = false;
-    if (!iframe.contentDocument!.head) {
-      await new Promise<void>((resolve) => {
-        iframe.onload = () => resolve();
-        // In case iframe is already loaded, wait a macro task
-        setTimeout(() => resolve(), 0);
-      });
-    }
-    iframe.contentDocument!.head.appendChild(script);
-    return new Promise(async (resolve, reject) => {
+  const loadScript: (url: string) => Promise<unknown> = async (url) => {
+    const doc = iframe.contentDocument!;
+    if (!doc.head) {
+      await new Promise<void>((resolve) => {
+        iframe.addEventListener('load', () => resolve(), { once: true });
+      });
+    }
+    const script = doc.createElement('script');
+    // Priority hint without relying on untyped properties
+    script.setAttribute('fetchpriority', 'high');
+    // For dynamically inserted classic scripts, defer is ignored; force ordered execution:
+    script.async = false;
+    return new Promise((resolve, reject) => {
       script.onload = () => {
         const ret = iframeWindow?.module?.exports;
         // @ts-expect-error
         iframeWindow.module = { exports: undefined };
+        script.remove();
         resolve(ret);
       };
-      script.onerror = (err) =>
-        reject(new Error(`Failed to load script: ${url}`, { cause: err }));
+      script.onerror = (err) => {
+        script.remove();
+        reject(new Error(`Failed to load script: ${url}`, { cause: err }));
+      };
       // @ts-expect-error
       iframeWindow.module = { exports: undefined };
-      script.src = url;
+      script.src = url;
+      doc.head!.appendChild(script);
     });
   };

Also applies to: 74-75

🧹 Nitpick comments (1)
packages/web-platform/web-tests/rspack.config.js (1)

49-57: Keep both Android and iOS PWA meta; don’t drop the Apple variant.

Switching to only mobile-web-app-capable loses iOS behavior. Include both for consistency with the other pages in this file.

       meta: {
         viewport:
           'width=device-width,initial-scale=1.0,maximum-scale=1.0,user-scalable=no',
-        'mobile-web-app-capable': 'yes',
+        // Support both Android Chrome and iOS Safari
+        'mobile-web-app-capable': 'yes',
+        'apple-mobile-web-app-capable': 'yes',
         'apple-mobile-web-app-status-bar-style': 'default',
         'screen-orientation': 'portrait',
         'format-detection': 'telephone=no',
         'x5-orientation': 'portrait',
       },

Also mirror this pair in any other pages that should behave as standalone web apps.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64a62d7 and abdd817.

📒 Files selected for processing (2)
  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts (2 hunks)
  • packages/web-platform/web-tests/rspack.config.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Place Playwright E2E tests in the web platform’s web-tests suite

Files:

  • packages/web-platform/web-tests/rspack.config.js
🧠 Learnings (1)
📚 Learning: 2025-08-27T11:37:38.587Z
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1589
File: packages/web-platform/web-worker-runtime/src/mainThread/startMainThread.ts:36-49
Timestamp: 2025-08-27T11:37:38.587Z
Learning: In web worker script loading (loadScript function), the pattern of calling fetch() before importScripts() is intentional for caching benefits, not redundant networking. The fetch() ensures HTTP caching is utilized before the synchronous importScripts() call.

Applied to files:

  • packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts

@PupilTong PupilTong enabled auto-merge (squash) September 16, 2025 11:51
@PupilTong PupilTong merged commit 16cdd21 into lynx-family:main Sep 16, 2025
50 checks passed
colinaaa pushed a commit that referenced this pull request Sep 21, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @lynx-js/react@0.114.0

### Minor Changes

- Partially fix the "cannot read property 'update' of undefined" error.
([#1771](#1771))

This error happens when rendering a JSX expression in a
[background-only](https://lynxjs.org/react/thinking-in-reactlynx.html)
context.

See
[#894](#894)
for more details.

### Patch Changes

- Reduce extra snapshot when children are pure text
([#1562](#1562))

- feat: Support `SelectorQuery` `animation` APIs
([#1768](#1768))

- Fix spread props inside list-item caused redundant snapshot patch
([#1760](#1760))

- fix: `ref is not initialized` error on template reload
([#1757](#1757))

## @lynx-js/react-rsbuild-plugin@0.11.0

### Minor Changes

- **BREAKING CHANGE:** Remove the `enableParallelElement` and
`pipelineSchedulerConfig` options.
([#1705](#1705))

Since the thread element resolution is still in experimental stage and
may have stability risks, it will be disabled by default after this
change.

- **BREAKING CHANGE**: Remove the `enableICU` option.
([#1800](#1800))

### Patch Changes

- Be compat with `@lynx-js/react` v0.114.0
([#1781](#1781))

- Updated dependencies
\[[`24100ab`](24100ab),
[`24100ab`](24100ab),
[`d0ef559`](d0ef559)]:
    -   @lynx-js/template-webpack-plugin@0.9.0
    -   @lynx-js/react-webpack-plugin@0.7.1
    -   @lynx-js/css-extract-webpack-plugin@0.6.3
    -   @lynx-js/react-alias-rsbuild-plugin@0.11.0
    -   @lynx-js/use-sync-external-store@1.5.0
    -   @lynx-js/react-refresh-webpack-plugin@0.3.4

## @lynx-js/tailwind-preset@0.4.0

### Minor Changes

- Enable `lynxUIPlugins` (incl. `uiVariants`) by default. Fills the gap
left by missing pseudo- and data-attribute selectors in Lynx, offering
state and structural variants out of the box.
([#1774](#1774))

    Opt-out globally or per plugin:

    ```ts
    // disable all UI plugins
    createLynxPreset({ lynxUIPlugins: false });
    // or disable one plugin
    createLynxPreset({ lynxUIPlugins: { uiVariants: false } });
    ```

### Patch Changes

- Fixed transform-related CSS variables previously defined on `:root`;
they are now reset on `*` to prevent inheritance between parent and
child elements.
([#1773](#1773))

## @lynx-js/web-core@0.17.0

### Minor Changes

- break(web): temporary remove support for chunk split
([#1739](#1739))

Since the global variables cannot be accessed in the splited chunk, we
temporary remove supporting for chunk spliting

Developers could easily remove the chunk Split settings in Rspeedy for
migration

        import { defineConfig } from '@lynx-js/rspeedy'

        export default defineConfig({
          performance: {
            chunkSplit: {
              strategy: 'all-in-one',
            },
          },
        })

### Patch Changes

- fix: lazy component load error
([#1794](#1794))

Some special version template may have chunk loading error. We fixed it.

- fix: avoid duplicate style transformation
([#1748](#1748))

    After this commit, we use DAG methods to handle the styleInfos

- fix: add sandbox attribute to iframe for enhanced security
([#1709](#1709))

- fix: the default template loader won't fetch twice for one url
([#1709](#1709))

- Updated dependencies
\[[`721635d`](721635d),
[`93d707b`](93d707b),
[`d150ed4`](d150ed4)]:
    -   @lynx-js/web-mainthread-apis@0.17.0
    -   @lynx-js/web-constants@0.17.0
    -   @lynx-js/web-worker-runtime@0.17.0
    -   @lynx-js/web-worker-rpc@0.17.0

## @lynx-js/template-webpack-plugin@0.9.0

### Minor Changes

- **BREAKING CHANGE:** Remove the `enableParallelElement` and
`pipelineSchedulerConfig` options.
([#1705](#1705))

Since the thread element resolution is still in experimental stage and
may have stability risks, it will be disabled by default after this
change.

- **BREAKING CHANGE**: Remove the `enableICU` option.
([#1800](#1800))

## @lynx-js/rspeedy@0.11.3

### Patch Changes

- Use `output.chunkLoading: 'lynx'` for `environments.web`.
([#1737](#1737))

- Support `resolve.extensions`
([#1759](#1759))

- Set the default value of `output.cssModules.localIdentName` to
`[local]-[hash:base64:6]`.
([#1783](#1783))

## @lynx-js/web-constants@0.17.0

### Patch Changes

- fix: avoid duplicate style transformation
([#1748](#1748))

    After this commit, we use DAG methods to handle the styleInfos

-   Updated dependencies \[]:
    -   @lynx-js/web-worker-rpc@0.17.0

## @lynx-js/web-elements@0.8.7

### Patch Changes

- The img within svg does not inherit the position
([#1769](#1769))

-   Updated dependencies \[]:
    -   @lynx-js/web-elements-template@0.8.7

## @lynx-js/web-mainthread-apis@0.17.0

### Patch Changes

- fix: \_\_QueryComponentImpl in mts should execute only once for same
url ([#1763](#1763))

- fix: avoid duplicate style transformation
([#1748](#1748))

    After this commit, we use DAG methods to handle the styleInfos

- feat: support lazy bundle with CSSOG(`enableCSSSelector: false`).
([#1770](#1770))

- Updated dependencies
\[[`93d707b`](93d707b)]:
    -   @lynx-js/web-constants@0.17.0
    -   @lynx-js/web-style-transformer@0.17.0

## @lynx-js/web-worker-runtime@0.17.0

### Patch Changes

- Updated dependencies
\[[`721635d`](721635d),
[`93d707b`](93d707b),
[`d150ed4`](d150ed4)]:
    -   @lynx-js/web-mainthread-apis@0.17.0
    -   @lynx-js/web-constants@0.17.0
    -   @lynx-js/web-worker-rpc@0.17.0

## @lynx-js/css-extract-webpack-plugin@0.6.3

### Patch Changes

- Supports `@lynx-js/template-webpack-plugin` 0.9.0.
([#1705](#1705))

## @lynx-js/react-webpack-plugin@0.7.1

### Patch Changes

- Supports `@lynx-js/template-webpack-plugin` 0.9.0.
([#1705](#1705))

## create-rspeedy@0.11.3



## @lynx-js/react-alias-rsbuild-plugin@0.11.0



## upgrade-rspeedy@0.11.3



## @lynx-js/web-core-server@0.17.0



## @lynx-js/web-elements-template@0.8.7



## @lynx-js/web-style-transformer@0.17.0



## @lynx-js/web-worker-rpc@0.17.0

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants