feat: refactor web mainthread implementation in rust#1937
feat: refactor web mainthread implementation in rust#1937PupilTong merged 6 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: fc8f4fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
📝 WalkthroughWalkthroughAdds extensive WASM-backed style encoding and JS bindings, JSON-artifact streaming and a CSS→WASM loader, expands element-event/exposure APIs and LynxView surfaces, introduces new encode/test bindings and e2e CI/job additions, and updates related tests, timing instrumentation, and templates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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! |
This is a part of #1937 <!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Standardized internal element type handling to align with native web APIs. * Updated API signatures to use standard HTML element types, improving compatibility with web standards. * Reorganized element-related APIs under a unified interface structure for clearer organization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Checklist <!--- Check and mark with an "x" --> - [ ] 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).
This is a part of #1937 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated TypeScript project configuration paths for the web mainthread APIs package. * Added build system configuration to enable faster incremental builds and improve build pipeline orchestration. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
refactor: move rust source files into web-mainthread-apis dir This is a part of #1937 * also updated the timing for loading the wasm chunk <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Reorganized workspace packages and updated membership; removed an obsolete style-transformer package and related manifests. * Updated wasm-bindgen to 0.2.105 and added binary build outputs; removed a dependency entry from changeset config. * **New Features** * Added a dedicated WebAssembly build workflow and automatic WASM initialization for the new mainthread APIs package. * **Documentation** * Removed legacy package docs and changelog entries. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
from
1b1a2c9 to
c491529
Compare
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
8 times, most recently
from
6d79647 to
6db7f0a
Compare
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
3 times, most recently
from
7dd2fcc to
f4b9e3a
Compare
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
2 times, most recently
from
e910fb2 to
d2dd84f
Compare
this is a part of lynx-family#1937
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
from
d2dd84f to
50a6a3e
Compare
this is a part of #1937 <!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive test suite for web-worker RPC functionality, covering arithmetic operations, error handling, lazy handlers, and data transfer capabilities across worker threads * **Chores** * Added Vitest configuration and npm test script to the web-worker-rpc package for testing infrastructure * Enhanced RPC implementation with additional logging for synchronous message replies <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Checklist <!--- Check and mark with an "x" --> - [ ] 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). --------- Signed-off-by: Haoyang Wang <12288479+PupilTong@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
7 times, most recently
from
3ef9837 to
526c31c
Compare
PupilTong
changed the title
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
from
0c0dc28 to
521e20a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/web-platform/web-core-wasm-e2e/rsbuild.config.ts (1)
4-4: Unused variable__filename.The
__filenamevariable is defined but never used in this file. This appears to be leftover from the removednode:pathimport.Suggested fix
import { defineConfig } from '@rsbuild/core'; -import { fileURLToPath } from 'node:url'; - -const __filename = fileURLToPath(import.meta.url); + const port = process.env.PORT ?? 3080;packages/web-platform/web-core-wasm-e2e/tests/reactlynx/basic-element-list-estimated-main-axis-size-px/index.jsx (1)
4-4: Unused imports.
useEffectis imported but not used in the component. Therefis also created but only attached to the list element without being utilized further.Suggested fix
-import { root, useEffect, useRef } from '@lynx-js/react'; +import { root } from '@lynx-js/react'; import './index.css'; function App() { - const ref = useRef(null); - return ( <view class='page'> <list list-type='single' - ref={ref} >If the
refis intended for future use or was needed in prior test logic, please disregard.packages/web-platform/web-core-wasm-e2e/tests/reactlynx.spec.ts (1)
90-105: Fix reload lifecycle:lynxDisposedAttributefilter logic is backwards, causing disposed page elements to persist.During reload,
disconnectedCallback()setslynxDisposedAttribute='l-disposed'(empty string) on the old page element, then clears shadowRoot. However, the filter in__FlushElementTreecheckspage.getAttribute(lynxDisposedAttribute) !== '', which means disposed elements (where the attribute equals'') are included rather than excluded. This likely allows the old element to coexist with the new one, resulting in 2[part="page"]nodes.The filter should either:
- Check that the attribute is not set at all (use
=== nullor check withhasAttribute)- Or set the attribute to a truthy value (e.g.,
'true') and check accordinglyThis is a genuine component lifecycle bug, not a timing issue—adding sleeps masks the underlying disposal problem.
🤖 Fix all issues with AI agents
In @packages/web-platform/playwright-fixtures/src/coverage-fixture.ts:
- Around line 39-52: The code uses a non-null assertion on the result of the
.find() when building sourceFilePath and then passes it to v8ToIstanbul, which
will produce a cryptic runtime error if no candidate exists; change the logic in
the block that computes sourceFilePath (the array .find(...) and subsequent
v8ToIstanbul call) to explicitly check whether the find returned a value and if
not throw a clear error (including testInfo.file and the attempted candidate
list) or handle the fallback, then only call v8ToIstanbul when sourceFilePath is
defined.
In @packages/web-platform/web-core-wasm-e2e/shell-project/index.ts:
- Around line 196-201: The code can create/modify the same lynx-view twice:
first via the casename branch that calls lynxViewTests(...) and then again in
the resourceName block which re-queries document.querySelector('lynx-view') and
calls setAttribute('url', ...), potentially overwriting prior setup; fix by
making the branches mutually exclusive or reusing the existing lynxView instance
from the casename flow — e.g., change the second check to else if (resourceName)
or only call document.querySelector('lynx-view') if lynxView is undefined, and
ensure you call lynxViewTests(...) once and then setAttribute('url',
`/resources/${resourceName}`) on that same lynxView rather than
re-creating/overwriting it.
In @packages/web-platform/web-core-wasm/tests/element-apis.spec.ts:
- Around line 1351-1358: The test comments refer to "scroll listener" but the
code is adding an 'input' event via mtsGlobalThis.__AddEvent; update the comment
text to say "input listener" to match the code, or if the intent was to test
scroll behavior, change the event strings in the calls to
mtsGlobalThis.__AddEvent from 'input' to 'scroll' and adjust the expect calls
that assert enableSpy to use 'scroll'; references: mtsGlobalThis.__AddEvent,
'input'/'scroll', and enableSpy.
In @packages/web-platform/web-core-wasm/ts/client/decodeWorker/cssLoader.ts:
- Around line 113-121: The inner check for s.startsWith('[') && s.endsWith(']')
is dead because attribute selectors are already handled earlier, so remove that
unreachable branch and simply set content = s (no substring) before calling
selector.push_one_selector_section('AttributeSelector', content); ensure you
only keep the surrounding else that handles the "Type selector" case and
eliminate the inner startsWith/endsWith block and its substring call to avoid
unnecessary/unreachable code.
In @packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts:
- Around line 251-253: The call to
this.#rpc.invoke(BackgroundThreadStartEndpoint, []) can reject and currently
only wires the success path to this.#btsReadyResolver, leaving the private
promise (#btsReady) unresolved and causing [Symbol.asyncDispose]() to hang;
modify the invocation to handle rejection as well—attach a .catch that logs the
error and calls the same resolver (or a dedicated rejection resolver) so that
#btsReady always settles, and ensure the rejection path uses the same
identifiers (this.#rpc.invoke, BackgroundThreadStartEndpoint,
this.#btsReadyResolver) so disposal can proceed and unhandled promise rejections
are avoided.
- Around line 285-287: The async dispose currently awaits this.#btsReady
unconditionally which hangs if startBTS() was never called; update
[Symbol.asyncDispose]() to first check the boolean flag this.#btsStarted and
only await this.#btsReady and call this.#rpc.invoke(disposeEndpoint, []) when
#btsStarted is true, otherwise just return immediately to avoid the deadlock.
In
@packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/WASMJSBinding.ts:
- Around line 215-229: The test currently only spies on
mtsBinding.enableElementEvent/disableElementEvent; update it to assert the full
call chain and side effects by creating a mock element with enableEvent and
disableEvent implementations and verifying that calling
enableElementEvent(uniqueId, eventName) calls element.enableEvent (and similarly
for disable), increments the element's event listener count, and invokes any
registered event-status callbacks; locate the element via getElementByUniqueId
used by enableElementEvent/disableElementEvent and mirror the assertion pattern
from packages/web-platform/web-elements/tests/component-event.spec.ts to check
listener count changes and event status handler invocations (use
LynxEventNameToW3cCommon mapping when computing expected eventName).
In @packages/web-platform/web-core-wasm/ts/client/mainthread/TemplateManager.ts:
- Around line 176-180: The code resolves the #load promise inside
lynxViewInstancePromise.then but doesn't handle exceptions from
instance.backgroundThread.markTiming, which can leave the outer promise pending;
wrap the calls in the then callback with try/catch (or chain a .catch on the
promise) so that if markTiming (or load_template_start) throws you call
this.#rejectPromise(error) (and optionally log the error), and on success still
call this.#resolvePromise(url); ensure every execution path either resolves or
rejects the pending promise created by #load.
🧹 Nitpick comments (20)
packages/web-platform/web-core-wasm/tests/StyleManager.spec.ts (2)
19-22: Missing cleanup: appended elements should be removed after each test.The
rootNodeis appended todocument.bodyinbeforeEach, but there's no correspondingafterEachto remove it. This can cause test pollution and DOM bloat across tests.♻️ Suggested fix
+import { describe, it, expect, beforeEach, afterEach, vi, beforeAll } from 'vitest'; ... beforeEach(() => { rootNode = document.createElement('div'); document.body.appendChild(rootNode); }); + afterEach(() => { + rootNode.remove(); + });
113-159: Good test coverage for css-id isolation.The test correctly verifies that styles from non-imported css-ids are excluded. The setup with css-id 123 importing only css-id 1 via
@import "1"and then asserting that onlygreen(from css-id 1) appears whileyellow(from css-id 2) is excluded is well-structured.Minor note: Line 156 (
expect(rule.style.backgroundColor).not.toBe('yellow')) is redundant since line 154 already asserts it equals'green'. Consider keeping only line 157 which is the more meaningful check thatyellowdoesn't appear anywhere in the rule.♻️ Optional cleanup
expect(sheet.cssRules.length).toBe(1); const rule = sheet.cssRules[0] as CSSStyleRule; // Should contain green expect(rule.style.backgroundColor).toBe('green'); // Should NOT contain yellow - expect(rule.style.backgroundColor).not.toBe('yellow'); expect(rule.cssText).not.toContain('yellow');packages/web-platform/web-core-wasm/tests/encode.spec.ts (1)
365-391: Consider usingexpect().fail()instead of throwing.The test logic is correct. However, using
throw new Error()for assertion failures is unconventional in vitest. Consider usingexpect.fail()for consistency with the testing framework.Suggested improvement
if (label === TemplateSectionLabel.ElementTemplates) { - throw new Error('ElementTemplates section should not be present'); + expect.fail('ElementTemplates section should not be present'); }packages/web-platform/web-core-wasm/ts/client/mainthread/TemplateManager.ts (2)
76-83: Thefetch_starttiming mark is recorded asynchronously, potentially after the actual fetch begins.The
currentTimeis captured synchronously, butmarkTimingis called inside.then(), which executes afterlynxViewInstancePromiseresolves. If the instance resolution takes time, the fetch may already be in progress when this mark is recorded.Consider capturing the instance synchronously if available, or documenting that this timing represents "instance ready + fetch start" rather than the exact fetch initiation.
51-64: Consider the semantics ofdecode_startin the cache-hit path.On line 56,
decode_startis marked when returning a cached template, but no actual decoding occurs in this path. This could be confusing for timing analysis. Consider using a different marker likecache_hitor documenting this behavior.packages/web-platform/web-core-wasm/src/template/template_sections/style_info/decoded_style_info.rs (1)
296-343: Consider extracting the duplicated nested-loop logic.Both
push_transform_kids_styleandpush_transformed_stylecontain nearly identical nested loops for populating thecss_og_css_id_to_class_selector_name_to_declarations_map. Extracting this into a helper method would reduce duplication.♻️ Proposed helper extraction
+ fn push_declaration_to_css_og_map(&mut self, declaration: &ParsedDeclaration) { + if let (Some(map), Some(css_ids), Some(names)) = ( + self.css_og_css_id_to_class_selector_name_to_declarations_map.as_mut(), + self.css_og_current_processing_css_ids.as_ref(), + self.css_og_current_processing_class_selector_names.as_ref(), + ) { + for css_id in css_ids.iter() { + let class_selector_map = map.entry(*css_id).or_default(); + for class_selector_name in names.iter() { + let string_buf = class_selector_map.entry(class_selector_name.clone()).or_default(); + declaration.generate_to_string_buf(string_buf); + } + } + } + }Then use
self.push_declaration_to_css_og_map(&declaration);in both trait methods.packages/web-platform/web-core-wasm/ts/client/webElementsDynamicLoader.ts (1)
84-93: Consider extracting the element count to avoid magic number coupling.The hardcoded upper bound
8in the loop couplesloadAllWebElementsto theswitchstatement implementation. If new element cases are added (e.g., case 9), this function won't load them unless manually updated.♻️ Suggested improvement
+const MAX_WEB_ELEMENT_ID = 8; + export function loadAllWebElements(): Promise<void> { const promises: Promise<unknown>[] = []; - for (let i = 0; i <= 8; i++) { + for (let i = 0; i <= MAX_WEB_ELEMENT_ID; i++) { const p = loadWebElement(i); if (p) { promises.push(p); } } return Promise.all(promises) as Promise<unknown> as Promise<void>; }Alternatively, consider deriving the count from the switch cases or using an array/map of loaders to eliminate the coupling entirely.
packages/web-platform/web-core-wasm/tests/element-apis.spec.ts (1)
1361-1391: Consider simplifying verbose inline comments.These comments extensively document the Rust implementation internals. While useful during development, they may become stale if the implementation changes and add noise to the test file. Consider condensing to a brief explanation of the test's intent.
♻️ Suggested simplification
- // 3. Remove first scroll listener (by setting null/undefined or however removal works) - // __AddEvent doesn't explicitly support removal in the type signature shown in createElementAPI ?? - // Actually createElementAPI.__AddEvent just calls __wasm_add_event_bts. - // To remove, we usually pass null/undefined as identifier? - // Looking at createElementAPI.ts: if frameworkCrossThreadIdentifier == null, it calls with undefined. - // But how to remove? - // In Rust: replace_framework_cross_thread_event_handler takes Option<String>. - // If we call __AddEvent with null identifier? - - // Let's check createElementAPI.ts again. - // if (frameworkCrossThreadIdentifier == null) { wasmContext.__wasm_add_event_bts(..., undefined); ... } - // So passing null/undefined removes it? - // Wait, the Rust side: `replace_framework_cross_thread_event_handler` puts the new identifier. - // If new identifier is None (from undefined), it removes. - // But we need to target the *specific* event name. - - // Warning: `AddEventPAPI` signature usually implies adding. - // "replace_framework_cross_thread_event_handler" suggests we REPLACE the handler for (event_name, event_type). - // So there is only ONE "bindEvent" handler for "lynxscroll" at a time? - // Use `event_apis.rs`: `framework_cross_thread_identifier: FnvHashMap<String, String>` where key is like "bind", "capture-bind". - // So YES, for a given (event_name, type="bindEvent"), there is only ONE handler identifier. - - // So my test step 2 "Add second scroll listener" actually REPLACES the first one. - // Rust logic: - // Old: Some("handler1"), New: Some("handler2"). - // match (Some, Some) => nothing. - // Correct. - - // 4. Remove listener. - // Call __AddEvent with null identifier. + // 3. Remove listener by passing null identifier (replaces existing handler with None) mtsGlobalThis.__AddEvent(element, 'bindEvent', 'input', null as any); - - // Rust logic: - // Old: Some("handler2"), New: None. - // match (Some, None) => should_disable = true.packages/web-platform/web-core-wasm/tests/template-manager.spec.ts (1)
328-335: Consider strengthening assertions for callback arguments.The test verifies that
onStyleInfoReadyandonMTSScriptsLoadedare called but doesn't assert on the actual arguments passed. Consider usingtoHaveBeenCalledWith()to verify the data is correctly decoded and passed.♻️ Example improvement
// Verify style info - expect(mockLynxViewInstance.onStyleInfoReady).toHaveBeenCalled(); + expect(mockLynxViewInstance.onStyleInfoReady).toHaveBeenCalledWith( + expect.objectContaining(jsonContent.styleInfo), + ); // Verify script decoding (LepusCode) - // The worker sends section: LepusCode with a blob URL map. - // TemplateManager handles this but doesn't expose the blob map directly easily in tests unless we mock the handler side effect or inspect mockLynxViewInstance. - // TemplateManager calls lynxViewInstance.onMTSScriptsLoaded(url, data). - expect(mockLynxViewInstance.onMTSScriptsLoaded).toHaveBeenCalled(); + expect(mockLynxViewInstance.onMTSScriptsLoaded).toHaveBeenCalledWith( + expect.any(String), // template URL + expect.any(Object), // blob URL map or similar + );packages/web-platform/web-core-wasm-e2e/tests/reactlynx/basic-element-list-basic-size/index.jsx (1)
22-51: Consider reducing repetition with array mapping.The 10
list-itemelements are manually duplicated. While acceptable for test readability, you could reduce maintenance burden using a map.♻️ Optional refactor using array
<list list-type='single' bindscroll={handleScroll} bindscrollend={handleScrollEnd} > - <list-item id='1'> - <view class='item' style={{ '--item-index': 1 }}></view> - </list-item> - <list-item id='2'> - <view class='item' style={{ '--item-index': 2 }}></view> - </list-item> - <list-item id='3'> - <view class='item' style={{ '--item-index': 3 }}></view> - </list-item> - <list-item id='4'> - <view class='item' style={{ '--item-index': 4 }}></view> - </list-item> - <list-item id='5'> - <view class='item' style={{ '--item-index': 5 }}></view> - </list-item> - <list-item id='6'> - <view class='item' style={{ '--item-index': 6 }}></view> - </list-item> - <list-item id='7'> - <view class='item' style={{ '--item-index': 7 }}></view> - </list-item> - <list-item id='8'> - <view class='item' style={{ '--item-index': 8 }}></view> - </list-item> - <list-item id='9'> - <view class='item' style={{ '--item-index': 9 }}></view> - </list-item> - <list-item id='10'> - <view class='item' style={{ '--item-index': 10 }}></view> - </list-item> + {Array.from({ length: 10 }, (_, i) => ( + <list-item id={String(i + 1)} key={i + 1}> + <view class='item' style={{ '--item-index': i + 1 }}></view> + </list-item> + ))} </list>packages/web-platform/web-core-wasm/src/main_thread/element_apis/event_apis.rs (1)
72-116: Consider extracting shared enable/disable logic into a helper.The
add_cross_thread_eventandadd_run_worklet_eventmethods share nearly identical allowlist-based enable/disable logic. A helper function could reduce duplication while maintaining clarity.♻️ Possible refactor pattern
fn should_toggle_element_event( old_handler: &Option<impl Sized>, new_handler: &Option<impl Sized>, ) -> (bool, bool) { match (old_handler, new_handler) { (None, Some(_)) => (true, false), // should_enable (Some(_), None) => (false, true), // should_disable _ => (false, false), } }packages/web-platform/web-core-wasm/ts/client/decodeWorker/decode.worker.ts (2)
391-394: Minor inconsistency in Blob MIME type.The JSON path uses
'text/javascript;'while the binary path (lines 291-293, 322-323) uses'text/javascript; charset=utf-8'. Consider aligning them for consistency.♻️ Suggested fix
const blob = new Blob([prefix, code, suffix], { - type: 'text/javascript;', + type: 'text/javascript; charset=utf-8', });
410-414: Same MIME type inconsistency in Manifest blob.♻️ Suggested fix
const blob = new Blob([code], { - type: 'text/javascript;', + type: 'text/javascript; charset=utf-8', });packages/web-platform/web-core-wasm/ts/client/decodeWorker/cssLoader.ts (1)
26-42: Clean up development comments before merging.These inline development notes (lines 27-42) read like debugging thoughts and TODO considerations. Consider condensing to a concise comment explaining the current behavior.
♻️ Suggested cleanup
// Handle imports if (info.imports) { - // imports in StyleInfo are filenames/hrefs, but RawStyleInfo expects cssIds. - // Wait, genStyleInfo output imports as string hrefs? - // RawStyleInfo: imports: Vec<i32> - // It seems genStyleInfo.ts produces imports array of strings, but RawStyleInfo needs integers. - // If the JSON only contains strings, we might have a problem mapping them back to IDs unless we have a map. - // However, WebEncodePlugin usually handles mapping. - // Let's check genStyleInfo again. - // node.type === 'ImportRule' => imports.push(node.href). - // If imports are paths, we can't easily convert to ID without extra info. - // BUT, current usage of imports in RawStyleInfo is strictly ID-based. - // If the input JSON has hrefs, we might skip imports or error. - // For now, I will omit imports if they are strings, or try to parse if they look like IDs. - // Actually, in the ecosystem, imports might not be fully supported in JSON mode yet or resolved differently. - // I will log or ignore for now, focusing on Rules. + // CSS imports are currently not supported in JSON mode. + // RawStyleInfo expects cssIds (integers), but genStyleInfo produces href strings. + // TODO: Add import support when ID mapping is available. }packages/web-platform/web-core-wasm-e2e/tests/reactlynx.spec.ts (2)
1651-1666: The added waits stabilize console collection, but consider replacing with a state-based wait.
E.g., wait for a specific UI mutation / text / event count instead of fixed 200ms/300ms.[sort_optional_refactor]
4598-4606:basic-element-list-scroll-to-position: add a deterministic “post-click settled” check before screenshot.
Right now it clicks then immediately snapshots; if the scroll is async, this can become flaky.packages/web-platform/web-core-wasm/ts/client/mainthread/LynxView.ts (2)
379-386:injectStyleRules:insertRule()can throw; also concatenating adoptedStyleSheets can duplicate on repeated renders.
If you keep the current approach, consider tracking and replacing a single “injected rules sheet” rather than concatenating indefinitely.
354-357: ClearingshadowRoot.innerHTML = ''is not an XSS issue here (no user input), but it can hide lifecycle ordering problems.
Prefer centralizing cleanup in a single awaited path (see dispose suggestion above).packages/web-platform/web-core-wasm/binary/client/client.d.ts (2)
103-135: Consider union types for improved type safety.The
rule_typeparameter accepts a plainstring, but according to the documentation, only specific values are valid:"StyleRule","FontFaceRule","KeyframesRule". If the Rust source can expose this as an enum that wasm-bindgen translates to a TypeScript union type, it would provide better compile-time safety for consumers.// Ideal type definition type RuleType = "StyleRule" | "FontFaceRule" | "KeyframesRule"; constructor(rule_type: RuleType);
157-169: Same type safety consideration applies to selector_type.Similar to
rule_type, theselector_typeparameter could benefit from a union type (e.g.,"ClassSelector" | "IdSelector" | ...) if the Rust source can be updated to use an enum.
packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/WASMJSBinding.ts
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/ts/client/mainthread/TemplateManager.ts
Show resolved
Hide resolved
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
from
521e20a to
2cb2481
Compare
PupilTong
force-pushed
the
p/hw/rust-refactor
branch
from
2cb2481 to
f8c6f11
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/test.yml (1)
284-298: Newweb-core-wasm-e2ejob is not included in thedonejob's needs array.The
donejob gates the final status check but doesn't depend onweb-core-wasm-e2e. This means failures in the new e2e tests won't block PR merging, which may not be the intended behavior.Suggested fix
done: needs: - benchmark - code-style-check - eslint - playwright-linux - playwright-web-elements - test-api - test-publish - test-react - test-rust - test-type - test-typos - test-vitest - website + - web-core-wasm-e2epackages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts (1)
285-310: Address the documented potential deadlock before merge.The TODO comment correctly identifies that if
startBTS()was never called,#btsReadywill never resolve, causing[Symbol.asyncDispose]()to hang indefinitely.Proposed fix using #btsStarted guard
async [Symbol.asyncDispose](): Promise<void> { - await this.#btsReady; - await this.#rpc.invoke(disposeEndpoint, []); + if (this.#btsStarted) { + await this.#btsReady; + await this.#rpc.invoke(disposeEndpoint, []); + } if (this.#lynxGroupId !== undefined) {packages/web-platform/web-core-wasm/ts/client/mainthread/LynxView.ts (1)
402-420: Critical:lynxGroupIdis used before declaration (Temporal Dead Zone).On line 410,
lynxGroupIdis passed toLynxViewInstanceconstructor, but it's not declared until line 418. This will cause aReferenceErrorat runtime due to JavaScript's Temporal Dead Zone.Proposed fix: move declaration before usage
queueMicrotask(async () => { if (this.injectStyleRules && this.injectStyleRules.length > 0) { const styleSheet = new CSSStyleSheet(); for (const rule of this.injectStyleRules) { styleSheet.insertRule(rule); } this.shadowRoot!.adoptedStyleSheets = this.shadowRoot! .adoptedStyleSheets.concat(styleSheet); } const mtsRealm = await mtsRealmPromise; + const lynxGroupId = this.lynxGroupId; if (this.#url) { const lynxViewInstance = import( /* webpackChunkName: "web-core-main-chunk" */ /* webpackFetchPriority: "high" */ './LynxViewInstance.js' ).then(({ LynxViewInstance }) => { return new LynxViewInstance( this, this.initData, this.globalProps, this.#url!, this.shadowRoot!, mtsRealm, lynxGroupId, this.nativeModulesMap, this.napiModulesMap, this.#initI18nResources, ); }); templateManager.fetchBundle(this.#url, lynxViewInstance); - const lynxGroupId = this.lynxGroupId; this.#instance = await lynxViewInstance; this.#rendering = false; } });
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 137-142: The job name "Playwright ${{ matrix.render }} (${{
matrix.shard }}/3)" and the --shard option are inconsistent with the matrix
definition (matrix.shard: [1, 2]); either update the matrix.shard values to
match the intended total shards (e.g., [1,2,3,4] if you want 4 shards) and keep
the job name and --shard as /4, or change the job name and --shard suffix to /2
(and ensure matrix.shard enumerates 1..2) so all three places (the job name
string, the --shard option, and matrix.shard values) reflect the same total
shard count.
In @packages/web-platform/playwright-fixtures/src/coverage-fixture.ts:
- Around line 39-52: The code uses .find(... )! to set sourceFilePath and then
passes it to v8ToIstanbul which can receive undefined; replace the non-null
assertion with a guarded check: after computing the found path (sourceFilePath),
if it's undefined throw or log a clear, descriptive error (including
testInfo.file and the candidate paths) so the failure is explicit, otherwise
pass the validated sourceFilePath into v8ToIstanbul; reference the
sourceFilePath variable and the v8ToIstanbul(...) call to locate where to add
the guard.
In @packages/web-platform/web-core-wasm-e2e/shell-project/index.ts:
- Around line 113-116: Replace the document.querySelector call with the
already-available closure variable lynxView to avoid selecting the wrong element
when multiple <lynx-view> instances exist; in the branch where name ===
'bindEvent' && moduleName === 'event_method', attach the click listener to
lynxView and call dispatchNapiModules('lynx-view') from that listener so the
event is bound to the intended element.
- Around line 196-201: The current block unconditionally sets the LynxView URL
when resourceName exists, which overwrites a prior URL set for casename and also
runs after the earlier logged error when casename is missing; change the control
flow so the resourceName branch is mutually exclusive with the casename branch —
e.g., replace the standalone if (resourceName) with an else if (resourceName) or
explicitly check that casename is not set (e.g., if (resourceName &&
!casename)), and ensure you do not proceed to call lynxViewTests / setAttribute
when an error was previously logged; locate the logic around casename,
resourceName, lynxViewTests and LynxViewElement and modify the conditional
accordingly.
In @packages/web-platform/web-core-wasm/src/js_binding/mts_js_binding.rs:
- Around line 51-64: The TypeScript interface IMtsBinding is missing
declarations for the new wasm bindings; add method signatures
enableElementEvent(uniqueId: number, eventName: string): void and
disableElementEvent(uniqueId: number, eventName: string): void to the
RustMainthreadContextBinding interface (IMtsBinding.ts) so the interface matches
the wasm_bindgen exports and existing implementations/tests (WASMJSBinding.ts,
element-apis.spec.ts).
In
@packages/web-platform/web-core-wasm/src/template/template_sections/style_info/decoded_style_info.rs:
- Around line 131-138: The documentation comment describing the ::placeholder
transformation is wrong; update the doc so it states the code transforms
::placeholder into ::part(input)::placeholder (matching the implementation in
the selector mutation that inserts OneSimpleSelector with
OneSimpleSelectorType::PseudoElementSelector and value "part(input)"), and add a
note clarifying that the shadow DOM actually exposes separate parts like
part="placeholder-top" and part="placeholder-bot" rather than a true
::placeholder pseudo-element on the input part—also add a brief verification
note to confirm this behavior against the shadow DOM structure used by the
component.
In @packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts:
- Around line 105-112: The current truthy check for
process.env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE'] causes values like "false"
to enable the experimental path; change the check in the isExperimentalWebBinary
assignment to an explicit boolean comparison (e.g., normalize and compare to
'true' or check against allowed truthy tokens like '1'/'true') so only intended
values trigger the branch; update the code around isExperimentalWebBinary and
the subsequent import of '@lynx-js/web-core-wasm/encode' and return block to use
the new boolean variable.
- Around line 89-104: tasmJSONInfo is currently declared as Record<string,
unknown> and uses a non-null assertion on encodeOptions.lepusCode.root; instead
declare tasmJSONInfo with the TasmJSONInfo type and ensure
encodeOptions.lepusCode.root is validated before use: check
encodeOptions.lepusCode.root is defined (or provide a safe default or throw a
clear error) and then assign values so the compiler can verify types without
using the ! operator; update the construction of tasmJSONInfo (and any
downstream casts) to rely on the TasmJSONInfo type rather than Record<string,
unknown>.
🧹 Nitpick comments (12)
packages/web-platform/web-core-wasm-e2e/rsbuild.config.ts (1)
2-4: Unused variable and import.
__filenameis defined but never used in this configuration. ThefileURLToPathimport exists solely to create it. Consider removing both to complete the cleanup (similar to the removednode:pathimport).🧹 Suggested cleanup
import { defineConfig } from '@rsbuild/core'; -import { fileURLToPath } from 'node:url'; - -const __filename = fileURLToPath(import.meta.url); const port = process.env.PORT ?? 3080;.github/workflows/workflow-test.yml (1)
101-101: Minor formatting inconsistency.Missing space before the closing braces in the template expression.
Suggested fix
- path: ${{ inputs.web-report-path}} + path: ${{ inputs.web-report-path }}packages/web-platform/web-core-wasm/README.md (1)
1-1: Expand README with essential documentation and clarify package visibility.The current one-line warning is insufficient for an internal experimental package. AGENTS.md exists with architecture documentation but is not referenced. Additionally, the
privatefield in package.json is not set totrue, meaning the package is publicly publishable by default—contradicting the "do not use" warning.Recommendations:
- Link to architecture documentation: Reference the existing AGENTS.md to provide architectural context about the WASM implementation
- Add package purpose: Briefly explain this is a WASM-backed implementation of web mainthread APIs for A/B testing
- Clarify relationship: Document how this relates to and may replace @lynx-js/web-core
- Usage guidance: Explain who should use it (internal testing only) and provide basic setup instructions if applicable
- Fix package visibility: Either set
"private": truein package.json to prevent accidental publication, or provide comprehensive documentation if the package is intended for internal distributionComparable packages (web-core and web-elements) provide 37-42 lines of documentation with usage examples and feature details. This README should follow a similar pattern.
packages/web-platform/web-core-wasm/tests/template-manager.spec.ts (1)
260-336: LGTM — consider strengthening assertions on callback arguments.The test correctly validates JSON format loading with appropriate streaming setup and callback verification. The distinction between JSON encoding (TextEncoder) vs binary encoding (encode function) is properly handled.
Consider verifying the actual arguments passed to
onMTSScriptsLoadedrather than just checking it was called, which would catch regressions in the data transformation:// Verify script decoding (LepusCode) - // The worker sends section: LepusCode with a blob URL map. - // TemplateManager handles this but doesn't expose the blob map directly easily in tests unless we mock the handler side effect or inspect mockLynxViewInstance. - // TemplateManager calls lynxViewInstance.onMTSScriptsLoaded(url, data). - expect(mockLynxViewInstance.onMTSScriptsLoaded).toHaveBeenCalled(); + expect(mockLynxViewInstance.onMTSScriptsLoaded).toHaveBeenCalledWith( + templateUrl, + expect.any(Object), // or more specific shape if known + );packages/web-platform/web-core-wasm/src/template/template_sections/style_info/raw_style_info.rs (2)
110-155: Consider potential panic inencodemethod ifstyle_contentis very large.The
encodemethod clonesselfon line 150 before encoding. For very large style sheets, this could cause memory pressure. Additionally, the mutation ofself.style_content_str_size_hintafter cloning means the original struct is updated with the hint, which is correct for subsequent uses.However, the pattern of mutating
&mut selffor what is conceptually a read operation (encoding) is slightly unusual. Consider whetherencodeshould take&selfand handle the size hint differently, or document this behavior.
199-225: Consider pre-allocating token capacity inpush_declaration.The method pushes 4 tokens (property name, colon, value tokens, semicolon) plus the tokenized value. For better performance, consider pre-allocating capacity:
// Rough estimate: 4 fixed tokens + estimated value tokens self.declaration_block.tokens.reserve(4 + value.len() / 4);This is a minor optimization that could help reduce reallocations for rules with many declarations.
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/mod.rs (1)
103-115: Parameter order inconsistency withdecode_into.The parameter order differs between the two methods:
decode_into(buffer, entry_name, config_enable_css_selector)encode_from_raw_style_info(raw_style_info, config_enable_css_selector, entry_name)For API consistency and developer experience, consider aligning the parameter order. The
decode_intostyle (entry_namebeforeconfig_enable_css_selector) seems more intuitive since optional parameters typically come last.♻️ Suggested fix
#[wasm_bindgen] pub fn encode_from_raw_style_info( raw_style_info: RawStyleInfo, - config_enable_css_selector: bool, entry_name: Option<String>, + config_enable_css_selector: bool, ) -> Result<js_sys::Uint8Array, wasm_bindgen::JsError> { let decode_data: DecodedStyleData = - StyleInfoDecoder::new(raw_style_info, entry_name, config_enable_css_selector)?.into(); + StyleInfoDecoder::new(raw_style_info, entry_name, config_enable_css_selector)?.into(); let data = &bincode::encode_to_vec(&decode_data, bincode::config::standard()) .map_err(|e| wasm_bindgen::JsError::new(&format!("Failed to encode to Uint8Array: {e}",)))?; Ok(js_sys::Uint8Array::from(data.as_slice())) }packages/web-platform/web-core-wasm/src/template/template_sections/style_info/decoded_style_info.rs (1)
65-72: Verify that discardingcss_idin the iterator is intentional.The pattern
for (_, style_sheet)explicitly discards thecss_idkey. Given thatimported_byis cloned from the style_sheet (line 70), this appears intentional. However, consider adding a brief comment explaining why the key is not needed here to aid future maintainability.packages/web-platform/web-core-wasm/ts/client/mainthread/LynxViewInstance.ts (1)
179-197: Good conditional loading and timing instrumentation.The logic correctly loads all web elements when
elementTemplatesare absent, with proper timing markers around data processing.Minor: The optional chaining on line 195 (
processData?.) is redundant since line 194 already guards with a truthy check.Remove redundant optional chaining
- const processedData = this.mainThreadGlobalThis.processData - ? this.mainThreadGlobalThis.processData?.(this.initData) + const processedData = this.mainThreadGlobalThis.processData + ? this.mainThreadGlobalThis.processData(this.initData) : this.initData;packages/web-platform/web-core-wasm/ts/client/mainthread/LynxView.ts (1)
345-367: The documented race condition should be addressed.The TODO correctly identifies that
disconnectedCallback()doesn't await async disposal, allowing concurrent instance mutations. Consider creating a follow-up issue to track this fix.The static analysis hint about
innerHTML = ''is a false positive—assigning an empty string is safe and doesn't involve user input.Do you want me to open a new issue to track the disposal race condition fix?
packages/web-platform/web-core-wasm/src/main_thread/element_apis/event_apis.rs (1)
81-115: Consider extracting shared enable/disable logic.The
add_cross_thread_eventandadd_run_worklet_eventfunctions have nearly identical enable/disable logic. Consider extracting this into a helper method to reduce duplication.Potential helper extraction
fn determine_enable_disable_action( old_handler: &Option<impl Sized>, new_handler: &Option<impl Sized>, ) -> (bool, bool) { match (old_handler, new_handler) { (None, Some(_)) => (true, false), // should_enable, should_disable (Some(_), None) => (false, true), _ => (false, false), } }This would consolidate the match logic and make future changes easier.
packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/WASMJSBinding.ts (1)
160-162: Remove dead code.Line 162 contains a standalone
bubblePath;statement that has no effect. This appears to be leftover debug code.🧹 Suggested cleanup
let bubblePath: Uint32Array = new Uint32Array(32); let bubblePathLength = 0; - bubblePath; let currentTarget = target as
...s/web-platform/web-core-wasm/src/template/template_sections/style_info/decoded_style_info.rs
Show resolved
Hide resolved
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 132-155: The workflow is missing the new job web-core-wasm-e2e
from the done job's needs list so failures there won't block merges; update the
done job to include "web-core-wasm-e2e" in its needs array (alongside other
Playwright jobs) so the done job depends on it, and optionally add a post-test
coverage step for the web-core-wasm-e2e job by running pnpm --filter
@lynx-js/web-core-wasm-e2e run coverage:ci after its test run to match the other
Playwright jobs.
In @packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts:
- Around line 251-253: The call to
this.#rpc.invoke(BackgroundThreadStartEndpoint, []) can reject and leave
this.#btsReady unresolved, causing [Symbol.asyncDispose]() to hang; update the
invocation chain to always call this.#btsReadyResolver in a .finally() handler
(e.g., this.#rpc.invoke(...).then(this.#btsReadyResolver).catch(() => {/*
optional log */}).finally(this.#btsReadyResolver)) so the resolver runs whether
the RPC succeeds or fails, ensuring this.#btsReady is settled and async disposal
cannot deadlock.
🧹 Nitpick comments (1)
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts (1)
285-294: Address the TODO to prevent potential deadlock.The TODO correctly identifies the deadlock risk. Since the
#btsStartedflag already exists, the guard is straightforward to implement. This should be resolved before merging to avoid hanging disposal in edge cases.🔧 Proposed fix using existing #btsStarted flag
async [Symbol.asyncDispose](): Promise<void> { - await this.#btsReady; - /* - * TODO: - * Potential deadlock if startBTS() was never called. - * If [Symbol.asyncDispose]() is invoked on a BackgroundThread instance where startBTS() was never called, - * #btsReady will never resolve, causing the disposal to hang indefinitely. - * Consider guarding with the existing #btsStarted flag. - */ - await this.#rpc.invoke(disposeEndpoint, []); + if (this.#btsStarted) { + await this.#btsReady; + await this.#rpc.invoke(disposeEndpoint, []); + } if (this.#lynxGroupId !== undefined) {Would you like me to open an issue to track this if you prefer to address it in a follow-up PR?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.ymlpackages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: When modifying Rust core logic in src/main_thread, ALWAYS add corresponding tests in tests/element-apis.spec.ts to verify the JS-side behavior
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: Avoid main-thread blocking concepts; prefer Wasm for heavy compute in the high-performance core
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: Keep the Web Core WASM architecture documentation updated if you add new modules or change the architecture
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 1770
File: packages/web-platform/web-mainthread-apis/src/utils/processStyleInfo.ts:316-318
Timestamp: 2025-09-18T08:12:56.802Z
Learning: In packages/web-platform/web-mainthread-apis/src/utils/processStyleInfo.ts, the current implementation uses cardStyleElement.textContent += for lazy component styles. While this could theoretically invalidate rule indices by reparsing the stylesheet, Sherry-hue indicated that UIDs don't repeat for the same element, making this approach acceptable for now. A future optimization to use separate style elements per entry was discussed but deferred to a separate PR to keep the current lazy bundle PR focused.
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/src/**/*.css : Use custom CSS properties to control linear layout behavior: `--lynx-display: linear`, `--lynx-linear-orientation`, `--lynx-linear-weight`, `--lynx-linear-weight-sum`, and `--lynx-linear-weight-basis`
📚 Learning: 2025-08-29T16:57:19.221Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1235
File: packages/web-platform/web-mainthread-apis/src/crossThreadHandlers/createQueryComponent.ts:24-26
Timestamp: 2025-08-29T16:57:19.221Z
Learning: In the Lynx RPC system, when createRpcEndpoint has hasReturn=true (third parameter), even if the return type is void, it returns a Promise<void> that resolves when the work on the background thread is completed. This allows proper awaiting of cross-thread operations.
Applied to files:
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
📚 Learning: 2025-09-25T14:03:25.576Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1834
File: packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createChunkLoading.ts:162-171
Timestamp: 2025-09-25T14:03:25.576Z
Learning: In the lynx-stack codebase, for loadScriptAsync implementations in createChunkLoading.ts, unhandled promise rejections from readScriptAsync are intentionally not caught - the caller is expected to handle errors rather than the loadScriptAsync method itself invoking the callback with error messages.
Applied to files:
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
📚 Learning: 2025-08-29T17:32:08.078Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1235
File: packages/web-platform/web-constants/src/endpoints.ts:248-261
Timestamp: 2025-08-29T17:32:08.078Z
Learning: In the Lynx RPC system, Promise<void> with hasReturn=true is meaningful for cross-thread operations as it allows callers to await completion of background work, even when no data is returned. The Promise<void> serves as a completion signal rather than a data carrier.
Applied to files:
packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts
📚 Learning: 2025-12-26T05:10:01.608Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.608Z
Learning: Applies to packages/web-platform/web-tests/**/*.{ts,tsx,js} : Use Playwright for E2E tests in packages/web-platform/web-tests/
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-09-10T10:27:32.903Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1706
File: .github/workflows/test.yml:0-0
Timestamp: 2025-09-10T10:27:32.903Z
Learning: MULTI_THREAD×SSR combination is NYI (Not Yet Implemented) in the Playwright test configuration, which is why it's excluded from the test matrix in .github/workflows/test.yml.
Applied to files:
.github/workflows/test.yml
📚 Learning: 2026-01-04T11:17:24.013Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: Keep the Web Core WASM architecture documentation updated if you add new modules or change the architecture
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-12-29T11:26:09.502Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/tests/**/*.spec.ts : Use Playwright for all E2E and functional tests with standard assertions like `expect(locator).toBeVisible()`, `expect(locator).toHaveCSS()`, and `diffScreenShot` for screenshot comparisons
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 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:
.github/workflows/test.yml
📚 Learning: 2025-09-10T10:24:40.137Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1706
File: .github/workflows/test.yml:109-109
Timestamp: 2025-09-10T10:24:40.137Z
Learning: In Playwright, when fullyParallel: true is set, the --shard option distributes individual tests across shards rather than splitting by files. This provides better load balancing compared to the default file-level sharding when fullyParallel is false.
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-12-29T11:26:09.502Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/src/**/*.css : Use custom CSS properties to control linear layout behavior: `--lynx-display: linear`, `--lynx-linear-orientation`, `--lynx-linear-weight`, `--lynx-linear-weight-sum`, and `--lynx-linear-weight-basis`
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-12-29T11:26:09.502Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/tests/**/*.spec.ts : When a component makes external requests (e.g., via `iframe` or `fetch`), mock them using `page.route` in Playwright tests to ensure tests are hermetic and fast
Applied to files:
.github/workflows/test.yml
⏰ 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). (6)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / clippy
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: code-style-check
- GitHub Check: zizmor
🔇 Additional comments (5)
.github/workflows/test.yml (1)
95-95: LGTM!The
web-report-pathaddition correctly specifies the report directory for Playwright web-elements tests.packages/web-platform/web-core-wasm/ts/client/mainthread/Background.ts (4)
89-92: LGTM!The readiness mechanism using a promise with an externally-stored resolver is a clean pattern for coordinating async lifecycle events. The definite assignment assertion is appropriate since the resolver is assigned in the constructor.
108-110: LGTM!Standard deferred promise pattern correctly implemented.
199-208: LGTM!Using the static
'app-service.js'string for error reporting aligns with the BTS context where errors originate.
256-271: LGTM!The updated timestamp calculation using
performance.now() + performance.timeOriginproduces an absolute epoch-based timestamp, which is appropriate for cross-context timing correlation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts:
- Around line 105-106: Add integration tests that exercise both encoding
branches: call the WebEncodePlugin (or the function that runs its encoding
logic) twice—once with process.env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE']
unset/false to exercise the default JSON path, and once with it set to a truthy
value to exercise the binary path. In the binary-case test, either allow the
real @lynx-js/web-core-wasm/encode to run or mock/stub it and assert it was
invoked; in both tests assert the plugin output/emit behavior and that outputs
are equivalent or meet expected shape/contents. Ensure tests set and then
restore/clear process.env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE'] to avoid
cross-test pollution and include any necessary setup/teardown to load the plugin
class WebEncodePlugin in each scenario.
🧹 Nitpick comments (2)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (2)
89-104: Consider stronger typing and defensive handling forlepusCode.root.Two concerns:
Type safety: The variable is typed as
Record<string, unknown>but later cast toTasmJSONInfo. Since you've imported theTasmJSONInfotype, consider typing the object directly or building it with proper type constraints to catch mismatches at compile time.Non-null assertion on line 100: The
!assertion onencodeOptions.lepusCode.rootwill throw at runtime ifrootis undefined. Consider adding a defensive check or a meaningful error message.♻️ Suggested improvement
- }, async ({ encodeOptions }) => { - const tasmJSONInfo: Record<string, unknown> = { + }, async ({ encodeOptions }) => { + const lepusRoot = encodeOptions.lepusCode.root; + if (!lepusRoot) { + throw new Error('lepusCode.root is required for encoding'); + } + const tasmJSONInfo: TasmJSONInfo = { styleInfo: (encodeOptions['css'] as { cssMap: Record<string, LynxStyleNode[]>; }).cssMap, - manifest: encodeOptions.manifest as Record<string, string>, - cardType: encodeOptions['cardType'] as string, - appType: encodeOptions['appType'] as string, - pageConfig: encodeOptions['pageConfig'] as Record<string, unknown>, + manifest: encodeOptions.manifest, + cardType: encodeOptions['cardType'], + appType: encodeOptions['appType'], + pageConfig: encodeOptions['pageConfig'], lepusCode: { // flatten the lepusCode to a single object ...encodeOptions.lepusCode.lepusChunk, - root: encodeOptions.lepusCode.root!, + root: lepusRoot, }, customSections: encodeOptions.customSections ?? {}, elementTemplates: encodeOptions['elementTemplates'] ?? {}, };Note: This requires improving the typing of
encodeOptionsupstream to avoid the casts, which may be out of scope for this PR.
107-119: Clarify the error message for failed import.The catch handler throws an error with message
"FLAG EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE IS INTERNAL USED ONLY", but this is triggered when the dynamic import fails. The message doesn't indicate the actual problem (missing package) and conflates "import failed" with "internal use warning".♻️ Suggested improvement
if (isExperimentalWebBinary) { const { encode } = await import('@lynx-js/web-core-wasm/encode') .catch( - () => { + (err) => { throw new Error( - `FLAG EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE IS INTERNAL USED ONLY`, + `EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE requires @lynx-js/web-core-wasm to be installed. ` + + `This flag is for internal use only. Original error: ${err.message}`, ); }, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: When modifying Rust core logic in src/main_thread, ALWAYS add corresponding tests in tests/element-apis.spec.ts to verify the JS-side behavior
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: Avoid main-thread blocking concepts; prefer Wasm for heavy compute in the high-performance core
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Use extensive CSS variables (e.g., `--lynx-display`) to drive layout and style from the Lynx engine
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: Keep the Web Core WASM architecture documentation updated if you add new modules or change the architecture
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/src/**/*.css : Use custom CSS properties to control linear layout behavior: `--lynx-display: linear`, `--lynx-linear-orientation`, `--lynx-linear-weight`, `--lynx-linear-weight-sum`, and `--lynx-linear-weight-basis`
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 1770
File: packages/web-platform/web-mainthread-apis/src/utils/processStyleInfo.ts:316-318
Timestamp: 2025-09-18T08:12:56.802Z
Learning: In packages/web-platform/web-mainthread-apis/src/utils/processStyleInfo.ts, the current implementation uses cardStyleElement.textContent += for lazy component styles. While this could theoretically invalidate rule indices by reparsing the stylesheet, Sherry-hue indicated that UIDs don't repeat for the same element, making this approach acceptable for now. A future optimization to use separate style elements per entry was discussed but deferred to a separate PR to keep the current lazy bundle PR focused.
📚 Learning: 2025-12-29T11:26:09.502Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-elements/AGENTS.md:0-0
Timestamp: 2025-12-29T11:26:09.502Z
Learning: Applies to packages/web-platform/web-elements/src/elements/all.ts : Export new components in `src/elements/all.ts` and add export configuration to `package.json` under `exports` for both types and default
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
📚 Learning: 2025-08-13T11:46:43.737Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1523
File: vitest.config.ts:5-6
Timestamp: 2025-08-13T11:46:43.737Z
Learning: In the lynx-stack codebase, default imports are consistently used for Node.js built-in modules (e.g., `import os from 'node:os'`, `import fs from 'node:fs'`). The TypeScript configuration supports esModuleInterop and allowSyntheticDefaultImports, making default imports the preferred pattern over namespace imports for Node.js built-ins.
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
📚 Learning: 2025-12-26T05:10:01.608Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.608Z
Learning: Applies to packages/react/components/**/*.{ts,tsx} : Optimize component library in packages/react/components/ using ReactLynx syntax
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
📚 Learning: 2025-08-27T08:10:09.932Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1612
File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13
Timestamp: 2025-08-27T08:10:09.932Z
Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.
Applied to files:
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
🧬 Code graph analysis (1)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (2)
packages/web-platform/web-core-wasm/ts/encode/webEncoder.ts (2)
encode(73-172)TasmJSONInfo(59-71)packages/webpack/template-webpack-plugin/src/web/genStyleInfo.ts (1)
genStyleInfo(8-136)
⏰ 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 (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / clippy
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (3)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (3)
7-8: LGTM!Type-only import is appropriate here.
85-88: LGTM!The change from
taptotapPromisewith an async handler is the correct approach for supporting the dynamic import path.
120-136: LGTM!The JSON encoding fallback path correctly transforms
styleInfoviagenStyleInfobefore serialization, maintaining backward compatibility with the non-binary template format.
3 participants
Note: the package
@lynx-js/web-core-wasmis a temporary package for A/B testing. We will replace the@lynx-js/web-core by @lynx-js/web-core-wasmonce it's performance improvement has been confirmed.Summary by CodeRabbit
New Features
Bug Fixes
Tests / CI
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist