feat(react): add preact upstream tests for E2E pipeline verification#2279
feat(react): add preact upstream tests for E2E pipeline verification#2279
Conversation
Reuse vitest-global-setup.js for standard pipeline initialization (runtime imports, inject calls, hook definitions) and layer on upstream-Preact-specific customizations: - Generic snapshot registration for arbitrary HTML element types - BSI shims (style proxy, event listeners, removeAttribute) - SI setAttribute/ensureElements overrides dispatching string-keyed attributes through Element PAPI methods (__SetClasses, __SetInlineStyles, __SetID, __AddDataset, __SetAttribute) instead of writing to jsdom directly, exercising the real SI → Element PAPI → jsdom path - Pipeline render bridging upstream Preact to the dual-thread pipeline - Commit hook wrapping with _commit → __c alias for mangled options
vitest.config.ts: - JSX transform via esbuild for upstream .js test files - Resolve aliases mapping preact/* to fork submodule source - Transform plugin: rewrite render() call sites to __pipelineRender(), skipping class method definitions and property access - Skiplist plugin: keyword-based and manual test skipping with it.skip() rewrite, driven by skiplist.json skiplist.json (Hermes-inspired structured skip config): - unsupported_features: keyword-scanned skips (getLog/clearLog for DOM mutation order, MutationObserver, dangerouslySetInnerHTML) - skip_list: categorized manual skips (JSON serialization gaps, component.base identity, Web DOM IDL properties, ref-as-DOM assumptions, pipeline timing) - permanent_skip_list: fundamentally incompatible tests Adds components, fragments, keys, createContext test suites alongside render. Results: 203 pass, 107 skip, 0 fail across 310 tests.
Add package configuration with test scripts and submodule management commands, plus comprehensive documentation covering goals, architecture, skiplist mechanism, and design decisions.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Preact upstream-tests workspace: a git submodule, dual-mode Vitest configs and workspace, comprehensive skiplists and test-run/report tooling, shared and mode-specific E2E setup scripts, a transform plugin extraction, and an exported applyProp helper for the testing environment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 6.55%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#7892 Bundle Size — 383.64KiB (0%).65083a3(current) vs 8ca97fe main#7876(baseline) Bundle metrics
|
| Current #7892 |
Baseline #7876 |
|
|---|---|---|
 Initial JS |
154.78KiB |
154.78KiB |
| Initial CSS |
35.06KiB |
35.06KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
238(-0.42%) |
239 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.99% |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7892 |
Baseline #7876 |
|
|---|---|---|
| JS |
252.72KiB |
252.72KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.06KiB |
35.06KiB |
Bundle analysis report Branch Huxpro/stop-setup-script Project dashboard
Generated by RelativeCI Documentation Report issue
…shared transform plugin Add dual-mode testing (no-compiler + with-compiler) for preact upstream tests to verify that the SWC snapshot transform produces identical DOM output. Extract transformReactLynxPlugin() from testing-library/src/vitest.config.js into a standalone shared module (transformReactLynxPlugin.js) so both preact-upstream-tests and testing-library can reuse the same plugin without duplication.
…split - Split setup.js into setup-shared.js, setup-nocompile.js, setup-compiled.js for cleaner separation between the two test modes - Update skiplist.json with refined categorization across both modes - Update vitest.shared.ts with improved transform plugin - Update vitest.compiled.config.ts for the new setup file names - Update README with comprehensive docs on both modes and skiplist workflow - Add GitHub Copilot instructions for skiplist maintenance
Move the string-key → Element PAPI dispatch logic (applyViaElementPAPI) out of preact-upstream-tests/setup-nocompile.js and into @lynx-js/testing-environment as a public export `applyProp()`, making it reusable for future framework adapters.
- Add report.mjs: a pass/skip dashboard that runs both vitest projects, groups results by test area, and breaks down skips by category (Lynx ≠ Web, Dual-thread/IPC, Test methodology) - Add category fields to all skiplist.json entries to enable attribution - Add test:report script to package.json
- .typos.toml: exclude upstream preact submodule; add 'apppend' word exception to allow the intentional typo in Preact's test name that our skiplist.json mirrors - eslint.config.js: add preact-upstream-tests to ignore list to fix import/no-unresolved errors for sinon, sinon-chai, etc. - rslib.config.ts: add transformReactLynxPlugin to bundle-false entries so dist/transformReactLynxPlugin.js is generated (required by dist/vitest.config.js at runtime) - testing-environment.api.md: update API report to include applyProp export added in the previous commit
There was a problem hiding this comment.
Pull request overview
Adds a new preact-upstream-tests package to run upstream Preact tests through the real ReactLynx dual-thread pipeline (compiled + no-compiler modes) to validate E2E rendering semantics and support CI verification.
Changes:
- Introduces
packages/react/preact-upstream-tests/with Vitest workspace configs, setup hooks, a skiplist system, and a reporting dashboard. - Exposes a new
applyProp()utility in@lynx-js/testing-environmentfor dispatching string-keyed props to Element PAPI in non-compiled mode. - Refactors React testing-library’s Vitest config to externalize
transformReactLynxPlugininto a reusable module and export it.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/testing-library/testing-environment/src/lynx/applyProp.ts | Adds applyProp() dispatcher for mapping raw props to Element PAPI calls. |
| packages/testing-library/testing-environment/src/index.ts | Re-exports applyProp() from the testing-environment entrypoint. |
| packages/testing-library/testing-environment/etc/testing-environment.api.md | Updates public API surface docs to include applyProp(). |
| packages/react/testing-library/src/vitest.config.js | Extracts SWC transform plugin into a separate module and wires plugin options. |
| packages/react/testing-library/src/transformReactLynxPlugin.js | New reusable Vite/Vitest plugin wrapper around transformReactLynxSync. |
| packages/react/testing-library/rslib.config.ts | Exposes transformReactLynxPlugin as a build entry for consumption. |
| packages/react/preact-upstream-tests/vitest.workspace.ts | Defines a Vitest workspace running compiled + no-compiler projects. |
| packages/react/preact-upstream-tests/vitest.shared.ts | Implements shared Vitest config, render rewrite plugin, and skiplist plugin. |
| packages/react/preact-upstream-tests/vitest.config.ts | Configures no-compiler mode project. |
| packages/react/preact-upstream-tests/vitest.compiled.config.ts | Configures compiled mode project using the SWC snapshot transform plugin. |
| packages/react/preact-upstream-tests/tsconfig.json | Adds TS config for the new package. |
| packages/react/preact-upstream-tests/skiplist.json | Declarative skiplist with structured reasons and mode-specific categories. |
| packages/react/preact-upstream-tests/setup-shared.js | Shared dual-thread pipeline initialization and generic snapshot fallback. |
| packages/react/preact-upstream-tests/setup-nocompile.js | Adds BSI shims and routes string-keyed attributes via applyProp(). |
| packages/react/preact-upstream-tests/setup-compiled.js | Adds compiled-mode shims (innerHTML wrapper stripping, boolean attributes, removeAttribute). |
| packages/react/preact-upstream-tests/report.mjs | Adds a CLI dashboard that runs both modes and summarizes pass/skip breakdowns. |
| packages/react/preact-upstream-tests/preact | Adds Preact fork as a git submodule pinned to a commit. |
| packages/react/preact-upstream-tests/package.json | Adds scripts for submodule management and running/reporting tests. |
| packages/react/preact-upstream-tests/README.md | Documents goals, architecture, skiplist design, and execution workflow. |
| packages/react/preact-upstream-tests/.github/skiplist.instructions.md | Adds contributor instructions for maintaining the skiplist/categories. |
| eslint.config.js | Extends ESLint coverage to the new upstream test package directory. |
| .typos.toml | Excludes the Preact submodule from spellchecking and adds a known upstream typo. |
| .gitmodules | Registers the new Preact submodule and its tracking branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "resolveJsonModule": true, | ||
| }, | ||
| "include": [ | ||
| "vitest.config.ts", | ||
| "vitest.compiled.config.ts", | ||
| "vitest.shared.ts", | ||
| "vitest.workspace.ts", | ||
| ], |
There was a problem hiding this comment.
tsconfig.json is not valid JSON due to trailing commas (line 9 and line 15) and the trailing comma before the closing } (line 16). Remove trailing commas so tooling that strictly parses JSON (TS, editors, CI linters) doesn’t fail to load the config.
| "resolveJsonModule": true, | |
| }, | |
| "include": [ | |
| "vitest.config.ts", | |
| "vitest.compiled.config.ts", | |
| "vitest.shared.ts", | |
| "vitest.workspace.ts", | |
| ], | |
| "resolveJsonModule": true | |
| }, | |
| "include": [ | |
| "vitest.config.ts", | |
| "vitest.compiled.config.ts", | |
| "vitest.shared.ts", | |
| "vitest.workspace.ts" | |
| ] |
| if (key.startsWith('data-')) { | ||
| __AddDataset(el, key.slice(5), (value ?? '') as string); | ||
| return; | ||
| } | ||
| // Skip event/internal/ref keys — forbidden by __SetAttribute | ||
| if (key.startsWith('on') || key.startsWith('__') || key === '_listeners') { | ||
| return; | ||
| } | ||
| if (key === 'ref' || key === 'key') return; | ||
| // Boolean → string conversion | ||
| if (key === 'translate') { | ||
| __SetAttribute(el, key, value ? 'yes' : 'no'); | ||
| return; | ||
| } | ||
| if (value === true) { | ||
| __SetAttribute(el, key, ''); | ||
| return; | ||
| } | ||
| if (value == null || value === false) { | ||
| __SetAttribute(el, key, null); | ||
| return; | ||
| } | ||
| __SetAttribute(el, key, typeof value === 'string' ? value : String(value)); |
There was a problem hiding this comment.
applyProp() currently removes attributes for value === false (line 68–70). This breaks standard DOM/React semantics for aria-* attributes, where false is typically serialized as the string "false" rather than removing the attribute. Consider special-casing aria-* (and possibly data-*) so false becomes "false" (and null/undefined still removes), which should reduce no-compiler-only skips like “should support false aria-* attributes”.
| const { transformReactLynxSync } = _require( | ||
| '@lynx-js/react/transform', | ||
| ); | ||
| // relativePath should be stable between different runs with different cwd | ||
| const relativePath = normalizeSlashes(path.relative( | ||
| rootDir, | ||
| sourcePath, | ||
| )); | ||
| const basename = path.basename(sourcePath); |
There was a problem hiding this comment.
Vite/Vitest id values can include query strings (e.g. foo.ts?direct). While your regex allows queries, path.relative() / path.basename() will treat ?… as part of the filename, which can produce unstable relativePath/basename and therefore unstable snapshots or misleading filename metadata. Strip the query (split at ?) before doing path.* operations and before passing filename to transformReactLynxSync.
| const { transformReactLynxSync } = _require( | |
| '@lynx-js/react/transform', | |
| ); | |
| // relativePath should be stable between different runs with different cwd | |
| const relativePath = normalizeSlashes(path.relative( | |
| rootDir, | |
| sourcePath, | |
| )); | |
| const basename = path.basename(sourcePath); | |
| // Strip any Vite/Vitest query string (`?foo=bar`) from the id before | |
| // using path utilities so that filenames and relative paths are stable. | |
| const sourcePathNoQuery = sourcePath.split('?')[0]; | |
| const { transformReactLynxSync } = _require( | |
| '@lynx-js/react/transform', | |
| ); | |
| // relativePath should be stable between different runs with different cwd | |
| const relativePath = normalizeSlashes(path.relative( | |
| rootDir, | |
| sourcePathNoQuery, | |
| )); | |
| const basename = path.basename(sourcePathNoQuery); |
| function runProject(project) { | ||
| process.stderr.write(` ${project.padEnd(32)} …`); | ||
| const r = spawnSync( | ||
| 'npx', |
There was a problem hiding this comment.
Using npx vitest can accidentally run a different Vitest version than the workspace-installed one (especially in monorepos and CI), which can change output formatting and break the parser. Prefer invoking the repo’s/package’s local Vitest binary via the package manager (pnpm vitest …) or by resolving the local executable path to guarantee consistent behavior.
| 'npx', | |
| 'pnpm', |
| function findMatchingParen(s: string): number { | ||
| let depth = 0; | ||
| for (let i = 0; i < s.length; i++) { | ||
| if (s[i] === '(') depth++; | ||
| else if (s[i] === ')') { | ||
| depth--; | ||
| if (depth === 0) return i; | ||
| } | ||
| } |
There was a problem hiding this comment.
findMatchingParen() counts parentheses inside strings, template literals, and comments, which can cause extractItBody() to return truncated or over-extended bodies. That can lead to incorrect “unsupported_features” keyword skipping (tests skipped or not skipped unexpectedly). A more robust approach is to parse the file (e.g., via a lightweight JS parser) and locate it(...) CallExpressions, or at least skip over quoted strings/comments while scanning.
| function findMatchingParen(s: string): number { | |
| let depth = 0; | |
| for (let i = 0; i < s.length; i++) { | |
| if (s[i] === '(') depth++; | |
| else if (s[i] === ')') { | |
| depth--; | |
| if (depth === 0) return i; | |
| } | |
| } | |
| // This implementation skips over parentheses that appear inside strings, | |
| // template literals, and comments so that only syntactic parentheses are counted. | |
| function findMatchingParen(s: string): number { | |
| let depth = 0; | |
| let inSingleQuote = false; | |
| let inDoubleQuote = false; | |
| let inTemplate = false; | |
| let inLineComment = false; | |
| let inBlockComment = false; | |
| const isEscaped = (index: number): boolean => { | |
| let backslashCount = 0; | |
| for (let i = index - 1; i >= 0 && s[i] === '\\'; i--) { | |
| backslashCount++; | |
| } | |
| return backslashCount % 2 === 1; | |
| }; | |
| for (let i = 0; i < s.length; i++) { | |
| const ch = s[i]; | |
| const next = i + 1 < s.length ? s[i + 1] : ''; | |
| // Handle being inside comments | |
| if (inLineComment) { | |
| if (ch === '\n' || ch === '\r') { | |
| inLineComment = false; | |
| } | |
| continue; | |
| } | |
| if (inBlockComment) { | |
| if (ch === '*' && next === '/') { | |
| inBlockComment = false; | |
| i++; // skip '/' | |
| } | |
| continue; | |
| } | |
| // Handle being inside strings or template literals | |
| if (inSingleQuote) { | |
| if (ch === '\'' && !isEscaped(i)) { | |
| inSingleQuote = false; | |
| } | |
| continue; | |
| } | |
| if (inDoubleQuote) { | |
| if (ch === '"' && !isEscaped(i)) { | |
| inDoubleQuote = false; | |
| } | |
| continue; | |
| } | |
| if (inTemplate) { | |
| if (ch === '`' && !isEscaped(i)) { | |
| inTemplate = false; | |
| } | |
| continue; | |
| } | |
| // Not in any special context: check for entering comments/strings/templates | |
| if (ch === '/' && next === '/') { | |
| inLineComment = true; | |
| i++; // skip second '/' | |
| continue; | |
| } | |
| if (ch === '/' && next === '*') { | |
| inBlockComment = true; | |
| i++; // skip '*' | |
| continue; | |
| } | |
| if (ch === '\'' && !isEscaped(i)) { | |
| inSingleQuote = true; | |
| continue; | |
| } | |
| if (ch === '"' && !isEscaped(i)) { | |
| inDoubleQuote = true; | |
| continue; | |
| } | |
| if (ch === '`' && !isEscaped(i)) { | |
| inTemplate = true; | |
| continue; | |
| } | |
| // Count only syntactic parentheses | |
| if (ch === '(') { | |
| depth++; | |
| } else if (ch === ')') { | |
| depth--; | |
| if (depth === 0) { | |
| return i; | |
| } | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
packages/react/preact-upstream-tests/skiplist.json (1)
20-364: Add a CI check to validate skiplist.json against upstream test titles.This file maintains 115 exact test name entries across four skip categories. A single typo or stale name silently breaks the skip mechanism — the test runs instead of being skipped, destabilizing CI signal. Consider adding a validation script (e.g., in
pnpm test:reportor a pre-commit hook) that runsrg --fixed-strings <test_name>against the preact submodule to catch stale/mistyped entries early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/preact-upstream-tests/skiplist.json` around lines 20 - 364, Add a CI validation step that verifies every test title string in skiplist.json (entries under skip_list, nocompile_skip_list, compiler_skip_list, permanent_skip_list) exists in the upstream preact test suite; implement this as a script invoked from the existing test pipeline (e.g., run from the pnpm script test:report or as a pre-commit/pre-push hook) which iterates each test_name and runs rg --fixed-strings "<test_name>" against the preact submodule, emitting a non-zero exit code and a clear error message listing missing/mismatched names (so CI fails) when any skiplist entry does not match an upstream test.packages/react/preact-upstream-tests/report.mjs (1)
233-234: Clamp inferredlynx_not_webcounts to avoid negative attribution.If named skips exceed actual skips, Lines 233-234 can produce negative category totals.
Proposed fix
-ncNamedCats.lynx_not_web = (ncNamedCats.lynx_not_web ?? 0) + (ncS - ncNamedTotal); -coNamedCats.lynx_not_web = (coNamedCats.lynx_not_web ?? 0) + (coS - coNamedTotal); +ncNamedCats.lynx_not_web = (ncNamedCats.lynx_not_web ?? 0) + Math.max(0, ncS - ncNamedTotal); +coNamedCats.lynx_not_web = (coNamedCats.lynx_not_web ?? 0) + Math.max(0, coS - coNamedTotal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/preact-upstream-tests/report.mjs` around lines 233 - 234, The current increments for ncNamedCats.lynx_not_web and coNamedCats.lynx_not_web can produce negative values when ncNamedTotal or coNamedTotal exceed ncS/coS; update the update logic for ncNamedCats.lynx_not_web and coNamedCats.lynx_not_web (the two lines updating those properties) to clamp the added value to zero (e.g., compute delta = Math.max((ncS - ncNamedTotal), 0) and Math.max((coS - coNamedTotal), 0)) before adding to the existing property so the category totals never decrease below zero.packages/react/preact-upstream-tests/setup-nocompile.js (1)
118-124: Make hook/document patching null-safe to avoid setup-order crashes.Line 120 and Line 124 assume the hook and background
_documentare always initialized. A small guard makes this resilient to initialization order changes.Suggested refactor
const _origOnInjectBG = globalThis.onInjectBackgroundThreadGlobals; globalThis.onInjectBackgroundThreadGlobals = (target) => { - _origOnInjectBG(target); - wrapBgDocumentWithShims(target._document); + if (typeof _origOnInjectBG === 'function') _origOnInjectBG(target); + if (target?._document) wrapBgDocumentWithShims(target._document); }; // Also patch the already-initialized background thread _document: -wrapBgDocumentWithShims(lynxTestingEnv.backgroundThread.globalThis._document); +const existingBgDoc = lynxTestingEnv.backgroundThread?.globalThis?._document; +if (existingBgDoc) wrapBgDocumentWithShims(existingBgDoc);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/preact-upstream-tests/setup-nocompile.js` around lines 118 - 124, Make the hook/document patching null-safe by guarding uses of globalThis.onInjectBackgroundThreadGlobals and lynxTestingEnv.backgroundThread.globalThis._document: store the original hook only if globalThis.onInjectBackgroundThreadGlobals exists, replace it by a safe wrapper that checks target is defined before calling _origOnInjectBG(target) and wrapBgDocumentWithShims(target._document) (use early-return if target is falsy), and only call wrapBgDocumentWithShims on lynxTestingEnv.backgroundThread.globalThis._document when that _document exists; update references to _origOnInjectBG, globalThis.onInjectBackgroundThreadGlobals, wrapBgDocumentWithShims, and lynxTestingEnv.backgroundThread.globalThis._document accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitmodules:
- Around line 3-4: Update the submodule remote URL in .gitmodules to point at an
organization-controlled canonical mirror instead of the personal repo (replace
the url = https://github.com/hzy/preact.git value with the org mirror URL),
while leaving the branch = lynx/v10.24.x entry untouched so tracked
branch/commit behavior is preserved; after changing the url entry, run git
submodule sync && git submodule update --init --recursive to ensure the working
tree uses the new org-controlled remote.
In `@eslint.config.js`:
- Line 101: The ignore entry 'packages/react/preact-upstream-tests/**' is too
broad and skips first‑party harness/config code; update eslint.config.js to only
ignore the vendored upstream submodule directory inside that package (replace
'packages/react/preact-upstream-tests/**' with a pattern that targets only the
vendored path, e.g.
'packages/react/preact-upstream-tests/<vendored-submodule-path>/**'), so local
files under packages/react/preact-upstream-tests/**/*.{ts,tsx,js,jsx} remain
linted.
In `@packages/react/preact-upstream-tests/README.md`:
- Around line 224-325: Several fenced code blocks in the README (the "Build Time
(SWC)" example, the "Background Thread / Main Thread" diagrams, and the
snapshotManager.values.has(type) snippet) lack language identifiers and trigger
markdownlint MD040; update each triple-backtick fence to include a language
label such as text (e.g., ```text) for all shown code blocks. Specifically, edit
the fences surrounding the JSX-to-snapshot example (with __snapshot_a1b2c3 and
registerSnapshot), both diagram blocks that describe
BackgroundSnapshotInstance/SnapshotInstance flows, and the final
snapshotManager.values.has(type) block so each opening ``` is changed to ```text
while leaving the inner content and identifiers like registerSnapshot,
snapshotCreatorMap, and __CreateElement unchanged.
In `@packages/react/preact-upstream-tests/report.mjs`:
- Around line 74-85: Check the spawnSync result (variable r) for failures before
parsing: if r.error is set or r.status is non-zero, write the combined output to
stderr (using output) and fail fast by calling process.exit(r.status || 1) or
throwing an error; move this check to immediately after computing r/output and
before calling parse(output) and computing stats/total/pass so a failing Vitest
run doesn’t produce misleading dashboard results.
In `@packages/react/preact-upstream-tests/setup-compiled.js`:
- Around line 89-92: The wrapper re-applies and calls original methods unbound
causing illegal-invocation and wrapper-chain growth; update
globalThis.onInjectBackgroundThreadGlobals to call _origOnInjectBG with its
original this context (i.e., bind or call with the same receiver) and ensure
wrapBgDocumentMinimal does not re-wrap an already-wrapped document by
checking/setting a sentinel (e.g., a symbol flag) on target._document before
wrapping; reference the functions/vars _origOnInjectBG,
onInjectBackgroundThreadGlobals, wrapBgDocumentMinimal and target._document to
locate and implement these fixes.
In `@packages/react/preact-upstream-tests/setup-nocompile.js`:
- Around line 45-50: The removeProperty proxy handler currently deletes the
local style (function/removeProperty) but doesn't inform the BSI, leaving stale
styles; update the removeProperty implementation to also call
bsi.setAttribute('style:' + k, '') (use an empty string, not null, because
__SetInlineStyles early-returns on falsy values) so style removals are
propagated to the BSI similar to how setProperty uses bsi.setAttribute('style:'
+ k, v) and removeAttribute uses bsi.setAttribute(key, null).
In `@packages/react/preact-upstream-tests/vitest.shared.ts`:
- Around line 152-183: The code assumes parsed group indices exist and
dereferences skiplist.unsupported_features[groupIdx] and list[groupIdx] without
validation; validate groupIdx after parsing (check !Number.isNaN(groupIdx) and
that groupIdx >= 0 and < array.length) before using it in the
unsupported_features branch and the generic list branch (references: trimmed,
colonIdx, groupIdx, skiplist, unsupported_features, list); if the index is
invalid, throw a clear Error like `SKIPLIST_ONLY: invalid group index "<value>"
for category "<category>"` so callers get a descriptive configuration error
instead of a TypeError.
In `@packages/react/testing-library/src/transformReactLynxPlugin.js`:
- Around line 35-46: sourcePath may include query/hash suffixes which makes
path.relative and path.basename unstable; before computing relativePath and
basename sanitize sourcePath (e.g., strip everything after the first ? or #) and
use that sanitized path when calling normalizeSlashes(path.relative(rootDir,
...)) and path.basename(...). Update the code around transformReactLynxSync so
relativePath and basename are derived from the sanitizedPath (not the raw
sourcePath) to ensure stable module IDs and worklet metadata.
In `@packages/testing-library/testing-environment/src/lynx/applyProp.ts`:
- Around line 60-62: The translate attribute handling in applyProp.ts
incorrectly coerces any truthy value to "yes"; update the branch that checks key
=== 'translate' so it only maps boolean values to "yes"/"no" and passes through
explicit non-boolean values unchanged: detect typeof value === 'boolean' and set
the attribute value to 'yes' or 'no' accordingly, otherwise use the provided
value as-is, then call __SetAttribute(el, key, valueToUse) and return; reference
the translate branch and __SetAttribute to locate the change.
---
Nitpick comments:
In `@packages/react/preact-upstream-tests/report.mjs`:
- Around line 233-234: The current increments for ncNamedCats.lynx_not_web and
coNamedCats.lynx_not_web can produce negative values when ncNamedTotal or
coNamedTotal exceed ncS/coS; update the update logic for
ncNamedCats.lynx_not_web and coNamedCats.lynx_not_web (the two lines updating
those properties) to clamp the added value to zero (e.g., compute delta =
Math.max((ncS - ncNamedTotal), 0) and Math.max((coS - coNamedTotal), 0)) before
adding to the existing property so the category totals never decrease below
zero.
In `@packages/react/preact-upstream-tests/setup-nocompile.js`:
- Around line 118-124: Make the hook/document patching null-safe by guarding
uses of globalThis.onInjectBackgroundThreadGlobals and
lynxTestingEnv.backgroundThread.globalThis._document: store the original hook
only if globalThis.onInjectBackgroundThreadGlobals exists, replace it by a safe
wrapper that checks target is defined before calling _origOnInjectBG(target) and
wrapBgDocumentWithShims(target._document) (use early-return if target is falsy),
and only call wrapBgDocumentWithShims on
lynxTestingEnv.backgroundThread.globalThis._document when that _document exists;
update references to _origOnInjectBG,
globalThis.onInjectBackgroundThreadGlobals, wrapBgDocumentWithShims, and
lynxTestingEnv.backgroundThread.globalThis._document accordingly.
In `@packages/react/preact-upstream-tests/skiplist.json`:
- Around line 20-364: Add a CI validation step that verifies every test title
string in skiplist.json (entries under skip_list, nocompile_skip_list,
compiler_skip_list, permanent_skip_list) exists in the upstream preact test
suite; implement this as a script invoked from the existing test pipeline (e.g.,
run from the pnpm script test:report or as a pre-commit/pre-push hook) which
iterates each test_name and runs rg --fixed-strings "<test_name>" against the
preact submodule, emitting a non-zero exit code and a clear error message
listing missing/mismatched names (so CI fails) when any skiplist entry does not
match an upstream test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitmodules.typos.tomleslint.config.jspackages/react/preact-upstream-tests/.github/skiplist.instructions.mdpackages/react/preact-upstream-tests/README.mdpackages/react/preact-upstream-tests/package.jsonpackages/react/preact-upstream-tests/preactpackages/react/preact-upstream-tests/report.mjspackages/react/preact-upstream-tests/setup-compiled.jspackages/react/preact-upstream-tests/setup-nocompile.jspackages/react/preact-upstream-tests/setup-shared.jspackages/react/preact-upstream-tests/skiplist.jsonpackages/react/preact-upstream-tests/tsconfig.jsonpackages/react/preact-upstream-tests/vitest.compiled.config.tspackages/react/preact-upstream-tests/vitest.config.tspackages/react/preact-upstream-tests/vitest.shared.tspackages/react/preact-upstream-tests/vitest.workspace.tspackages/react/testing-library/rslib.config.tspackages/react/testing-library/src/transformReactLynxPlugin.jspackages/react/testing-library/src/vitest.config.jspackages/testing-library/testing-environment/etc/testing-environment.api.mdpackages/testing-library/testing-environment/src/index.tspackages/testing-library/testing-environment/src/lynx/applyProp.ts
| url = https://github.com/hzy/preact.git | ||
| branch = lynx/v10.24.x |
There was a problem hiding this comment.
Use an organization-controlled submodule remote.
A personal-namespace GitHub URL increases availability/supply-chain risk for a critical test dependency. Please point this submodule at an org-owned canonical mirror (while keeping the same tracked branch/commit behavior).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitmodules around lines 3 - 4, Update the submodule remote URL in
.gitmodules to point at an organization-controlled canonical mirror instead of
the personal repo (replace the url = https://github.com/hzy/preact.git value
with the org mirror URL), while leaving the branch = lynx/v10.24.x entry
untouched so tracked branch/commit behavior is preserved; after changing the url
entry, run git submodule sync && git submodule update --init --recursive to
ensure the working tree uses the new org-controlled remote.
| // testing-library | ||
| 'packages/testing-library/**', | ||
| 'packages/react/testing-library/**', | ||
| 'packages/react/preact-upstream-tests/**', |
There was a problem hiding this comment.
Narrow the ignore scope to only vendored upstream code.
Ignoring packages/react/preact-upstream-tests/** disables linting for this package’s first-party harness/config code as well. Please ignore only the submodule path so local JS/TS still follows repo lint policy.
Suggested diff
- 'packages/react/preact-upstream-tests/**',
+ 'packages/react/preact-upstream-tests/preact/**',As per coding guidelines: **/*.{ts,tsx,js,jsx} must follow ESLint rules as configured in eslint.config.js.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'packages/react/preact-upstream-tests/**', | |
| 'packages/react/preact-upstream-tests/preact/**', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.js` at line 101, The ignore entry
'packages/react/preact-upstream-tests/**' is too broad and skips first‑party
harness/config code; update eslint.config.js to only ignore the vendored
upstream submodule directory inside that package (replace
'packages/react/preact-upstream-tests/**' with a pattern that targets only the
vendored path, e.g.
'packages/react/preact-upstream-tests/<vendored-submodule-path>/**'), so local
files under packages/react/preact-upstream-tests/**/*.{ts,tsx,js,jsx} remain
linted.
| ``` | ||
| Build Time (SWC) | ||
| ================ | ||
| <div className={cls}>hello</div> | ||
| │ | ||
| ▼ snapshot transform | ||
| _jsx("__snapshot_a1b2c3", { values: [cls] }) | ||
| + | ||
| registerSnapshot("__snapshot_a1b2c3", { | ||
| create() { __CreateElement("div", pageId) }, ← real HTML tag preserved! | ||
| update: [ | ||
| (ctx) => __SetClasses(ctx.__elements[0], ctx.__values[0]) | ||
| ] | ||
| }) | ||
| ``` | ||
|
|
||
| ``` | ||
| Background Thread Main Thread | ||
| ======================== ====================== | ||
|
|
||
| Preact diff/render snapshotPatchApply() | ||
| │ │ | ||
| ▼ ▼ | ||
| VNode { type: "__snapshot_a1b2c3", SnapshotInstance | ||
| props: { values: [cls] } } ├─ insertBefore(child) | ||
| │ │ └─ ensureElements() | ||
| ▼ │ └─ snapshot.create() | ||
| BackgroundSnapshotInstance │ └─ __CreateElement("div") | ||
| ├─ setAttribute("values", [cls]) ├─ setAttribute("values", [cls]) | ||
| │ └─ pushes to patch array │ └─ snapshot.update[0](ctx) | ||
| │ key="values", val=[cls] │ └─ __SetClasses(el, cls) | ||
| └─ (same tree ops as no-compiler) └─ __elements[0] ← <div> jsdom node | ||
| │ │ | ||
| ▼ ▼ | ||
| (same patch IPC as no-compiler) Element PAPI (jsdom) | ||
| └─ same __CreateElement / __SetClasses / etc. | ||
| ``` | ||
|
|
||
| Key difference: Preact never calls `setProperty(bsi, 'className', cls)`. Instead it | ||
| calls `setAttribute("values", [cls])`, and the **compiler-generated `update` function** | ||
| maps `values[0]` → `__SetClasses(el, cls)`. This is the same path production | ||
| ReactLynx uses. | ||
|
|
||
| ### No-Compiler Mode (`preact-upstream`) — Runtime Baseline | ||
|
|
||
| Preact sees raw props (`{ className: 'foo' }`) and diffs them directly. Attribute | ||
| changes go through BSI shims → patch IPC → `applyViaElementPAPI()` on the main thread. | ||
| This mode is intentionally kept as a runtime baseline and regression-isolation tool. | ||
|
|
||
| ``` | ||
| Background Thread Main Thread | ||
| ======================== ====================== | ||
|
|
||
| Preact diff/render snapshotPatchApply() | ||
| │ │ | ||
| ▼ ▼ | ||
| BackgroundSnapshotInstance SnapshotInstance | ||
| ├─ insertBefore(child) ├─ insertBefore(child) | ||
| ├─ removeChild(child) │ └─ ensureElements() | ||
| ├─ setAttribute(key, val) │ └─ __CreateElement(tag) | ||
| │ └─ pushes to patch array ├─ setAttribute(key, val) | ||
| └─ BSI shims: │ └─ applyViaElementPAPI(el,k,v) | ||
| ├─ .style proxy -> SetAttribute │ ├─ __SetClasses | ||
| ├─ .addEventListener (stub) │ ├─ __SetInlineStyles | ||
| ├─ .removeEventListener (stub) │ ├─ __SetID / __AddDataset | ||
| ├─ .dispatchEvent (stub) │ └─ __SetAttribute | ||
| └─ .removeAttribute (stub) └─ __elements[0] ← jsdom node | ||
| │ │ | ||
| ▼ ▼ | ||
| __globalSnapshotPatch[] Element PAPI (jsdom) | ||
| [CreateElement, type, id] ├─ __CreateElement(tag) | ||
| [InsertBefore, parent, child, before] ├─ __AppendElement(parent, child) | ||
| [SetAttribute, id, key, value] ├─ __InsertElementBefore(...) | ||
| [RemoveChild, parent, child] └─ __RemoveElement(parent, child) | ||
| │ │ | ||
| ▼ ▼ | ||
| commitPatchUpdate() vitest jsdom document | ||
| └─ JSON.stringify(patchList) │ | ||
| │ ▼ | ||
| └──── callLepusMethod ──────► syncSnapshotToScratch() | ||
| (IPC simulation) └─ move children from | ||
| switches thread <page> to scratch div | ||
| ``` | ||
|
|
||
| In this mode, `__CreateElement(tag)` receives the **real HTML tag** (e.g. `"div"`) | ||
| because the generic snapshot factory passes through the tag name directly. | ||
|
|
||
| ### snapshotCreatorMap: Bridging Compiler Definitions to Both Threads | ||
|
|
||
| `snapshotCreatorMap` is a **module-level singleton** shared between simulated threads. | ||
| When the SWC-compiled test module is imported, it calls `registerSnapshot()` which | ||
| populates `snapshotCreatorMap["__snapshot_a1b2c3"] = (type) => { ... }`. | ||
|
|
||
| The `setup.js` monkey-patch on `snapshotManager.values.has/get` checks | ||
| `snapshotCreatorMap[type]` **before** falling back to the generic snapshot factory: | ||
|
|
||
| ``` | ||
| snapshotManager.values.has(type) | ||
| → snapshotCreatorMap[type] exists? | ||
| → YES: invoke creator (registers real snapshot with create/update functions) | ||
| → NO: register generic snapshot (create calls __CreateElement(type)) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
The diagram fences are missing language labels (MD040). Please annotate them (for example with text) to keep docs lint-clean.
Suggested diff
-```
+```text
Build Time (SWC)
@@
-```
+```
-```
+```text
Background Thread Main Thread
@@
-```
+```
-```
+```text
Background Thread Main Thread
@@
-```
+```
-```
+```text
snapshotManager.values.has(type)
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 273-273: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 320-320: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/preact-upstream-tests/README.md` around lines 224 - 325,
Several fenced code blocks in the README (the "Build Time (SWC)" example, the
"Background Thread / Main Thread" diagrams, and the
snapshotManager.values.has(type) snippet) lack language identifiers and trigger
markdownlint MD040; update each triple-backtick fence to include a language
label such as text (e.g., ```text) for all shown code blocks. Specifically, edit
the fences surrounding the JSX-to-snapshot example (with __snapshot_a1b2c3 and
registerSnapshot), both diagram blocks that describe
BackgroundSnapshotInstance/SnapshotInstance flows, and the final
snapshotManager.values.has(type) block so each opening ``` is changed to ```text
while leaving the inner content and identifiers like registerSnapshot,
snapshotCreatorMap, and __CreateElement unchanged.
| const r = spawnSync( | ||
| 'npx', | ||
| ['vitest', 'run', '--workspace', 'vitest.workspace.ts', '--project', project], | ||
| { cwd: __dirname, encoding: 'utf-8', maxBuffer: 16 * 1024 * 1024 }, | ||
| ); | ||
| const output = (r.stdout ?? '') + (r.stderr ?? ''); | ||
| const stats = parse(output); | ||
| const total = Object.values(stats).reduce((a, v) => a + v.total, 0); | ||
| const pass = Object.values(stats).reduce((a, v) => a + v.pass, 0); | ||
| process.stderr.write(` done (${pass}/${total} pass)\n`); | ||
| return stats; | ||
| } |
There was a problem hiding this comment.
Fail fast when a Vitest project exits non-zero.
The current flow always parses output and continues, so a failed run can still generate a misleading dashboard.
Proposed fix
function runProject(project) {
process.stderr.write(` ${project.padEnd(32)} …`);
const r = spawnSync(
'npx',
['vitest', 'run', '--workspace', 'vitest.workspace.ts', '--project', project],
{ cwd: __dirname, encoding: 'utf-8', maxBuffer: 16 * 1024 * 1024 },
);
+ if (r.status !== 0) {
+ throw new Error(
+ `Vitest project "${project}" failed (exit ${r.status ?? 'unknown'}).\n`
+ + `${r.stdout ?? ''}\n${r.stderr ?? ''}`,
+ );
+ }
const output = (r.stdout ?? '') + (r.stderr ?? '');
const stats = parse(output);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/preact-upstream-tests/report.mjs` around lines 74 - 85, Check
the spawnSync result (variable r) for failures before parsing: if r.error is set
or r.status is non-zero, write the combined output to stderr (using output) and
fail fast by calling process.exit(r.status || 1) or throwing an error; move this
check to immediately after computing r/output and before calling parse(output)
and computing stats/total/pass so a failing Vitest run doesn’t produce
misleading dashboard results.
| globalThis.onInjectBackgroundThreadGlobals = (target) => { | ||
| _origOnInjectBG(target); | ||
| wrapBgDocumentMinimal(target._document); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react/preact-upstream-tests/setup-compiled.js | sed -n '80,120p'Repository: lynx-family/lynx-stack
Length of output: 1188
🏁 Script executed:
rg -n 'wrapBgDocumentMinimal' packages/react/preact-upstream-tests/Repository: lynx-family/lynx-stack
Length of output: 397
🏁 Script executed:
# Check if Biome would flag unbound method invocation issues
fd -e js -e jsx -e ts -e tsx packages/react/preact-upstream-tests/ | head -5Repository: lynx-family/lynx-stack
Length of output: 519
🏁 Script executed:
# Verify the reinjection flow and check if both call sites execute in practice
rg -n -B5 -A5 '_origOnInjectBG|backgroundThread\.globalThis\._document' packages/react/preact-upstream-tests/setup-compiled.jsRepository: lynx-family/lynx-stack
Length of output: 668
Preserve document method context and prevent wrapper re-stacking.
Lines 101/104/107 call original methods unbound, and the wrapper is reapplied on each background-global reinjection (lines 91 and 94). This causes illegal-invocation bugs and wrapper-chain growth.
Proposed fix
function wrapBgDocumentMinimal(doc) {
+ if (!doc || doc.__removeAttributeShimmed) return;
+ doc.__removeAttributeShimmed = true;
+
const origCE = doc.createElement;
const origCENS = doc.createElementNS;
const origCTN = doc.createTextNode;
doc.createElement = function(type) {
- return addRemoveAttribute(origCE(type));
+ return addRemoveAttribute(origCE.call(this, type));
};
doc.createElementNS = function(_ns, type) {
- return addRemoveAttribute(origCENS(_ns, type));
+ return addRemoveAttribute(origCENS.call(this, _ns, type));
};
doc.createTextNode = function(text) {
- return addRemoveAttribute(origCTN(text));
+ return addRemoveAttribute(origCTN.call(this, text));
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/preact-upstream-tests/setup-compiled.js` around lines 89 - 92,
The wrapper re-applies and calls original methods unbound causing
illegal-invocation and wrapper-chain growth; update
globalThis.onInjectBackgroundThreadGlobals to call _origOnInjectBG with its
original this context (i.e., bind or call with the same receiver) and ensure
wrapBgDocumentMinimal does not re-wrap an already-wrapped document by
checking/setting a sentinel (e.g., a symbol flag) on target._document before
wrapping; reference the functions/vars _origOnInjectBG,
onInjectBackgroundThreadGlobals, wrapBgDocumentMinimal and target._document to
locate and implement these fixes.
| if (prop === 'removeProperty') { | ||
| return (k) => { | ||
| const old = target[k]; | ||
| delete target[k]; | ||
| return old; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react/preact-upstream-tests/setup-nocompile.jsRepository: lynx-family/lynx-stack
Length of output: 5959
🏁 Script executed:
rg -n "removeProperty" packages/react/preact-upstream-tests/ --type js
rg -n "style\.removeProperty" . --type js --type jsx --type ts --type tsxRepository: lynx-family/lynx-stack
Length of output: 208
🏁 Script executed:
rg -n "removeProperty\s*\(" packages/react/preact-upstream-tests/ -A2 -B2
rg -n "\.style\.removeProperty" . --type js --type tsRepository: lynx-family/lynx-stack
Length of output: 910
🏁 Script executed:
rg -n "style:" packages/react/preact-upstream-tests/setup-nocompile.js -B2 -A2
rg -n "applyProp" packages/testing-environment/src -lRepository: lynx-family/lynx-stack
Length of output: 729
🏁 Script executed:
rg -n "export.*applyProp" packages/ --type ts --type js
rg -n "function applyProp" packages/ --type ts --type jsRepository: lynx-family/lynx-stack
Length of output: 475
🏁 Script executed:
cat -n packages/testing-library/testing-environment/src/lynx/applyProp.tsRepository: lynx-family/lynx-stack
Length of output: 3057
🏁 Script executed:
# Check how __SetInlineStyles handles null/empty values
rg -n "__SetInlineStyles" packages/web-platform -A5 -B2 --type ts --type js | head -50Repository: lynx-family/lynx-stack
Length of output: 4819
🏁 Script executed:
rg -n "function __SetInlineStyles|export.*__SetInlineStyles" packages/web-platform/web-mainthread-apis -B2 -A15 --type tsRepository: lynx-family/lynx-stack
Length of output: 1776
🏁 Script executed:
rg -n "transformParsedStyles" packages/web-platform/web-mainthread-apis/ts/pureElementPAPIs.ts -B2 -A20 | head -60Repository: lynx-family/lynx-stack
Length of output: 1853
🏁 Script executed:
cat -n packages/web-platform/web-mainthread-apis/ts/style/transformInlineStyle.ts | head -80Repository: lynx-family/lynx-stack
Length of output: 408
🏁 Script executed:
rg -n "transformParsedStyles\s*=" packages/web-platform/web-mainthread-apis/ts/utils/tokenizer.ts -A30 | head -50Repository: lynx-family/lynx-stack
Length of output: 48
🏁 Script executed:
rg -n "export.*transformParsedStyles" packages/web-platform/web-mainthread-apis/ts/utils/tokenizer.ts -A30Repository: lynx-family/lynx-stack
Length of output: 1050
🏁 Script executed:
# Check if there are any preact upstream tests and what they test
find packages/react/preact-upstream-tests -name "*.test.*" -o -name "*.spec.*"
rg -n "style" packages/react/preact-upstream-tests/ --type js | grep -i "remove\|clear"Repository: lynx-family/lynx-stack
Length of output: 451
🏁 Script executed:
# Check if there are actual Preact test files that might call removeProperty
find packages/react/preact-upstream-tests -type f \( -name "*.js" -o -name "*.ts" \)Repository: lynx-family/lynx-stack
Length of output: 457
🏁 Script executed:
cat -n packages/react/preact-upstream-tests/setup-shared.js
cat -n packages/react/preact-upstream-tests/vitest.config.tsRepository: lynx-family/lynx-stack
Length of output: 11098
🏁 Script executed:
# Check if there are any actual Preact tests that use style.removeProperty
find packages/react -name "*.test.*" -type f | head -5
# Check the preact fork directory structure
ls -la packages/react/preact-upstream-tests/preact/src/ 2>/dev/null || echo "Not found at expected location"Repository: lynx-family/lynx-stack
Length of output: 361
removeProperty does not propagate style removal to BSI.
On lines 45-50, the proxy deletes local state but never forwards the removal, leaving stale styles on the element. Unlike setProperty (line 42) which calls bsi.setAttribute('style:' + k, v), and removeAttribute (line 78) which calls bsi.setAttribute(key, null), the removeProperty handler has no propagation.
The fix must pass an empty string (not null, since __SetInlineStyles early-returns on falsy values):
Corrected fix
if (prop === 'removeProperty') {
return (k) => {
const old = target[k];
delete target[k];
+ bsi.setAttribute('style:' + k, '');
return old;
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (prop === 'removeProperty') { | |
| return (k) => { | |
| const old = target[k]; | |
| delete target[k]; | |
| return old; | |
| }; | |
| if (prop === 'removeProperty') { | |
| return (k) => { | |
| const old = target[k]; | |
| delete target[k]; | |
| bsi.setAttribute('style:' + k, ''); | |
| return old; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/preact-upstream-tests/setup-nocompile.js` around lines 45 -
50, The removeProperty proxy handler currently deletes the local style
(function/removeProperty) but doesn't inform the BSI, leaving stale styles;
update the removeProperty implementation to also call bsi.setAttribute('style:'
+ k, '') (use an empty string, not null, because __SetInlineStyles early-returns
on falsy values) so style removals are propagated to the BSI similar to how
setProperty uses bsi.setAttribute('style:' + k, v) and removeAttribute uses
bsi.setAttribute(key, null).
| const colonIdx = trimmed.indexOf(':'); | ||
| const category = colonIdx === -1 ? trimmed : trimmed.slice(0, colonIdx); | ||
| const groupIdx = colonIdx === -1 ? undefined : Number.parseInt(trimmed.slice(colonIdx + 1), 10); | ||
|
|
||
| if (category === 'unsupported_features') { | ||
| const entries = groupIdx === undefined | ||
| ? skiplist.unsupported_features | ||
| : [skiplist.unsupported_features[groupIdx]!]; | ||
| for (const entry of entries) { | ||
| for (const kw of entry.keywords) { | ||
| keywords.push({ | ||
| keyword: kw, | ||
| pattern: new RegExp(`\\b${kw.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`), | ||
| }); | ||
| } | ||
| } | ||
| } else { | ||
| const list = | ||
| skiplist[category as 'skip_list' | 'nocompile_skip_list' | 'permanent_skip_list' | 'compiler_skip_list']; | ||
| if (!list) { | ||
| throw new Error( | ||
| `SKIPLIST_ONLY: unknown category "${category}". ` | ||
| + `Valid: unsupported_features, skip_list, nocompile_skip_list, permanent_skip_list, compiler_skip_list`, | ||
| ); | ||
| } | ||
| const entries = groupIdx === undefined ? list : [list[groupIdx]!]; | ||
| for (const entry of entries) { | ||
| for (const t of entry.tests) { | ||
| names.add(t); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate SKIPLIST_ONLY group indices before dereferencing.
Line 159 and Line 177 currently assume indexed entries exist. Invalid or out-of-range indices (including NaN) crash with generic TypeError instead of a clear configuration error.
🛠️ Suggested hardening patch
- const groupIdx = colonIdx === -1 ? undefined : Number.parseInt(trimmed.slice(colonIdx + 1), 10);
+ const groupRaw = colonIdx === -1 ? undefined : trimmed.slice(colonIdx + 1);
+ const groupIdx = groupRaw === undefined ? undefined : Number.parseInt(groupRaw, 10);
+ if (groupRaw !== undefined && (!Number.isInteger(groupIdx) || groupIdx < 0)) {
+ throw new Error(`SKIPLIST_ONLY: invalid group index "${groupRaw}" for category "${category}"`);
+ }
if (category === 'unsupported_features') {
- const entries = groupIdx === undefined
- ? skiplist.unsupported_features
- : [skiplist.unsupported_features[groupIdx]!];
+ const entries = groupIdx === undefined
+ ? skiplist.unsupported_features
+ : [skiplist.unsupported_features[groupIdx]];
+ if (entries.some((entry) => entry == null)) {
+ throw new Error(`SKIPLIST_ONLY: unsupported_features index out of range: ${groupIdx}`);
+ }
for (const entry of entries) {
for (const kw of entry.keywords) {
keywords.push({
@@
- const entries = groupIdx === undefined ? list : [list[groupIdx]!];
+ const entries = groupIdx === undefined ? list : [list[groupIdx]];
+ if (entries.some((entry) => entry == null)) {
+ throw new Error(`SKIPLIST_ONLY: ${category} index out of range: ${groupIdx}`);
+ }
for (const entry of entries) {
for (const t of entry.tests) {
names.add(t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const colonIdx = trimmed.indexOf(':'); | |
| const category = colonIdx === -1 ? trimmed : trimmed.slice(0, colonIdx); | |
| const groupIdx = colonIdx === -1 ? undefined : Number.parseInt(trimmed.slice(colonIdx + 1), 10); | |
| if (category === 'unsupported_features') { | |
| const entries = groupIdx === undefined | |
| ? skiplist.unsupported_features | |
| : [skiplist.unsupported_features[groupIdx]!]; | |
| for (const entry of entries) { | |
| for (const kw of entry.keywords) { | |
| keywords.push({ | |
| keyword: kw, | |
| pattern: new RegExp(`\\b${kw.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`), | |
| }); | |
| } | |
| } | |
| } else { | |
| const list = | |
| skiplist[category as 'skip_list' | 'nocompile_skip_list' | 'permanent_skip_list' | 'compiler_skip_list']; | |
| if (!list) { | |
| throw new Error( | |
| `SKIPLIST_ONLY: unknown category "${category}". ` | |
| + `Valid: unsupported_features, skip_list, nocompile_skip_list, permanent_skip_list, compiler_skip_list`, | |
| ); | |
| } | |
| const entries = groupIdx === undefined ? list : [list[groupIdx]!]; | |
| for (const entry of entries) { | |
| for (const t of entry.tests) { | |
| names.add(t); | |
| } | |
| } | |
| } | |
| const colonIdx = trimmed.indexOf(':'); | |
| const category = colonIdx === -1 ? trimmed : trimmed.slice(0, colonIdx); | |
| const groupRaw = colonIdx === -1 ? undefined : trimmed.slice(colonIdx + 1); | |
| const groupIdx = groupRaw === undefined ? undefined : Number.parseInt(groupRaw, 10); | |
| if (groupRaw !== undefined && (!Number.isInteger(groupIdx) || groupIdx < 0)) { | |
| throw new Error(`SKIPLIST_ONLY: invalid group index "${groupRaw}" for category "${category}"`); | |
| } | |
| if (category === 'unsupported_features') { | |
| const entries = groupIdx === undefined | |
| ? skiplist.unsupported_features | |
| : [skiplist.unsupported_features[groupIdx]]; | |
| if (entries.some((entry) => entry == null)) { | |
| throw new Error(`SKIPLIST_ONLY: unsupported_features index out of range: ${groupIdx}`); | |
| } | |
| for (const entry of entries) { | |
| for (const kw of entry.keywords) { | |
| keywords.push({ | |
| keyword: kw, | |
| pattern: new RegExp(`\\b${kw.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`), | |
| }); | |
| } | |
| } | |
| } else { | |
| const list = | |
| skiplist[category as 'skip_list' | 'nocompile_skip_list' | 'permanent_skip_list' | 'compiler_skip_list']; | |
| if (!list) { | |
| throw new Error( | |
| `SKIPLIST_ONLY: unknown category "${category}". ` | |
| `Valid: unsupported_features, skip_list, nocompile_skip_list, permanent_skip_list, compiler_skip_list`, | |
| ); | |
| } | |
| const entries = groupIdx === undefined ? list : [list[groupIdx]]; | |
| if (entries.some((entry) => entry == null)) { | |
| throw new Error(`SKIPLIST_ONLY: ${category} index out of range: ${groupIdx}`); | |
| } | |
| for (const entry of entries) { | |
| for (const t of entry.tests) { | |
| names.add(t); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 163-163: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${kw.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/preact-upstream-tests/vitest.shared.ts` around lines 152 -
183, The code assumes parsed group indices exist and dereferences
skiplist.unsupported_features[groupIdx] and list[groupIdx] without validation;
validate groupIdx after parsing (check !Number.isNaN(groupIdx) and that groupIdx
>= 0 and < array.length) before using it in the unsupported_features branch and
the generic list branch (references: trimmed, colonIdx, groupIdx, skiplist,
unsupported_features, list); if the index is invalid, throw a clear Error like
`SKIPLIST_ONLY: invalid group index "<value>" for category "<category>"` so
callers get a descriptive configuration error instead of a TypeError.
| const regex = /\.(?:js|jsx|mjs|cjs|ts|tsx|mts|cts)(\?.*)?$/; | ||
| if (!regex.test(sourcePath)) return null; | ||
|
|
||
| const { transformReactLynxSync } = _require( | ||
| '@lynx-js/react/transform', | ||
| ); | ||
| // relativePath should be stable between different runs with different cwd | ||
| const relativePath = normalizeSlashes(path.relative( | ||
| rootDir, | ||
| sourcePath, | ||
| )); | ||
| const basename = path.basename(sourcePath); |
There was a problem hiding this comment.
Sanitize Vite module IDs before path operations.
sourcePath can include query suffixes; using it directly in path.relative()/path.basename() can produce unstable filenames and worklet metadata. Strip query/hash first.
Suggested diff
- const regex = /\.(?:js|jsx|mjs|cjs|ts|tsx|mts|cts)(\?.*)?$/;
- if (!regex.test(sourcePath)) return null;
+ const cleanSourcePath = sourcePath.replace(/[?#].*$/, '');
+ const regex = /\.(?:js|jsx|mjs|cjs|ts|tsx|mts|cts)$/;
+ if (!regex.test(cleanSourcePath)) return null;
@@
- const relativePath = normalizeSlashes(path.relative(
+ const relativePath = normalizeSlashes(path.relative(
rootDir,
- sourcePath,
+ cleanSourcePath,
));
- const basename = path.basename(sourcePath);
+ const basename = path.basename(cleanSourcePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/testing-library/src/transformReactLynxPlugin.js` around lines
35 - 46, sourcePath may include query/hash suffixes which makes path.relative
and path.basename unstable; before computing relativePath and basename sanitize
sourcePath (e.g., strip everything after the first ? or #) and use that
sanitized path when calling normalizeSlashes(path.relative(rootDir, ...)) and
path.basename(...). Update the code around transformReactLynxSync so
relativePath and basename are derived from the sanitizedPath (not the raw
sourcePath) to ensure stable module IDs and worklet metadata.
| if (key === 'translate') { | ||
| __SetAttribute(el, key, value ? 'yes' : 'no'); | ||
| return; |
There was a problem hiding this comment.
translate coercion is currently incorrect for non-boolean values.
On Line 60, truthy/falsy conversion turns translate="no" into "yes". Please only map booleans and pass through explicit values.
Proposed fix
- if (key === 'translate') {
- __SetAttribute(el, key, value ? 'yes' : 'no');
- return;
- }
+ if (key === 'translate') {
+ if (value === true) {
+ __SetAttribute(el, key, 'yes');
+ return;
+ }
+ if (value === false) {
+ __SetAttribute(el, key, 'no');
+ return;
+ }
+ __SetAttribute(el, key, value == null ? null : String(value));
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (key === 'translate') { | |
| __SetAttribute(el, key, value ? 'yes' : 'no'); | |
| return; | |
| if (key === 'translate') { | |
| if (value === true) { | |
| __SetAttribute(el, key, 'yes'); | |
| return; | |
| } | |
| if (value === false) { | |
| __SetAttribute(el, key, 'no'); | |
| return; | |
| } | |
| __SetAttribute(el, key, value == null ? null : String(value)); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/testing-library/testing-environment/src/lynx/applyProp.ts` around
lines 60 - 62, The translate attribute handling in applyProp.ts incorrectly
coerces any truthy value to "yes"; update the branch that checks key ===
'translate' so it only maps boolean values to "yes"/"no" and passes through
explicit non-boolean values unchanged: detect typeof value === 'boolean' and set
the attribute value to 'yes' or 'no' accordingly, otherwise use the provided
value as-is, then call __SetAttribute(el, key, valueToUse) and return; reference
the translate branch and __SetAttribute to locate the change.
| "version": "0.0.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "preact:init": "cd ../../.. && git submodule update --init packages/react/preact-upstream-tests/preact", |
There was a problem hiding this comment.
Because this package is picked up by the root Vitest project globs, introducing a mandatory manual preact:init step here also makes pnpm test depend on local submodule state. On a fresh checkout this will fail before the reviewer even gets to this package. Could we either keep this suite opt-in at the root level or make the submodule bootstrap automatic/self-checking?
| const _innerHTMLDesc = Object.getOwnPropertyDescriptor(Element.prototype, 'innerHTML'); | ||
| Object.defineProperty(Element.prototype, 'innerHTML', { | ||
| get() { | ||
| return _innerHTMLDesc.get.call(this).replace(/<\/?wrapper>/g, ''); |
There was a problem hiding this comment.
This makes the compiled suite report green by rewriting what the assertion reads rather than by fixing the structural difference. If <wrapper> is a real output difference in compiled mode, masking it at innerHTML level seems risky because it hides exactly the class of regressions this harness is supposed to surface.
| return; | ||
| } | ||
| if (key.startsWith('data-')) { | ||
| __AddDataset(el, key.slice(5), (value ?? '') as string); |
There was a problem hiding this comment.
I think this changes data-* removal semantics: when value becomes null/false, this still goes through __AddDataset(..., ""), so the key is retained with an empty string instead of being removed. That diverges from both DOM behavior and the runtime spread path, which clears missing dataset entries by rebuilding the dataset object.
upupming
left a comment
upupming
left a comment
There was a problem hiding this comment.
Thanks for putting this together — I think the overall goal here is valuable. Pulling upstream Preact tests into the Lynx pipeline should give us a much stronger semantic regression signal than only testing our own fixtures.
My main concern is that a few of the current adaptations weaken that signal or broaden the surface area more than necessary:
- the suite now depends on manual submodule initialization, which makes root-level test execution more fragile on a fresh checkout;
- the compiled-mode
innerHTMLpatch appears to hide a real structural output difference instead of surfacing it as a known gap; applyProp()looks useful as a harness helper, but exposing it from@lynx-js/testing-environmentfeels like a layering leak, and the currentdata-*handling seems to diverge from removal semantics.
I think this is very close to being a strong testing harness, but I’d feel more comfortable if we kept the bridge code narrowly scoped, avoided patches that rewrite observed output, and made the suite opt-in or self-bootstrapping from the repo root.
| @@ -14,6 +14,7 @@ import { GlobalEventEmitter } from './lynx/GlobalEventEmitter.js'; | |||
| export { initElementTree } from './lynx/ElementPAPI.js'; | |||
| export type { LynxElement } from './lynx/ElementPAPI.js'; | |||
There was a problem hiding this comment.
applyProp() looks useful for this harness, but exporting it from @lynx-js/testing-environment feels like it expands that package from "Node.js implementation of Lynx APIs" into "DOM/Preact-to-Lynx adapter helpers". Since this helper is not itself a Lynx API, would it make sense to keep it private to the upstream-test harness (or at least under a clearly internal testing-library surface) instead of making it part of the testing-environment public API?
|
One broader thought: if the main goal of this package is CI-grade semantic verification against upstream Preact, I wonder whether it should live in this monorepo at all. Keeping the harness in-tree gives good locality, but it also brings in submodule bootstrapping, root test discovery, workspace/package-manager surface area, and API-layering pressure on packages like An alternative would be to keep the product-facing utilities here, but move the upstream-verification harness into a dedicated CI repo (or at least a separately triggered external harness) that consumes published/workspace artifacts. That would still preserve the regression signal while reducing the amount of test-only adaptation code and repo-level complexity we need to carry in the main source tree. |
Summary
packages/react/preact-upstream-tests/package that runs Preact's own test suite through the real ReactLynx dual-threaded rendering pipeline (BSI → patch → SI → Element PAPI → jsdom)skiplist.json(Hermes-inspired) declaratively manages test exclusions with documented reasonsTest plan
pnpm preact:init && pnpm testpasses inpackages/react/preact-upstream-tests/@lynx-js/reacttestsSummary by CodeRabbit
New Features
Documentation
API