feat: improve web-core-wasm#2148
Conversation
🦋 Changeset detectedLatest commit: 59a4a5b 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 |
📝 WalkthroughWalkthroughWASM/TS and Rust refactor in web-core-wasm: migration from bincode→rkyv, new TemplateManager/StyleManager, typed CSSProperty and string-based style transformer, many wasm binding renames, removal of element-template encode/TS surfaces, and corresponding JS/TS integration updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/rust.yml (1)
51-56: Fix missing wasm32-unknown-unknown target and coverage clobbering in Rust workflow.The workflow will fail when executing the second
cargo llvm-covcommand because thewasm32-unknown-unknowntarget is not installed by the rustup action. Additionally, both coverage runs output to the samelcov.info, causing the native coverage to be overwritten. Install the wasm target in the rustup action and use separate output paths for each coverage report:🧩 Required changes
In
.github/actions/rustup/action.yml(lines 60-70):- name: Install shell: bash env: toolchain: ${{ steps.get-toolchain.outputs.toolchain }} run: | rustup toolchain install "$toolchain" \ -c rustc \ -c cargo \ -c rust-std \ -c clippy \ -c rustfmt + -t wasm32-unknown-unknownIn
.github/workflows/rust.yml(lines 51-56):cargo llvm-cov nextest --all-features --profile ci --config-file .cargo/nextest.toml --lcov --output-path lcov.info --release - cargo llvm-cov nextest --all-features --profile ci --config-file .cargo/nextest.toml --lcov --output-path lcov.info --release --target wasm32-unknown-unknown + cargo llvm-cov nextest --all-features --profile ci --config-file .cargo/nextest.toml --lcov --output-path lcov.wasm.info --release --target wasm32-unknown-unknown - name: Upload coverage reports to Codecov uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5 with: token: ${{ secrets.CODECOV_TOKEN }} + files: lcov.info,lcov.wasm.infopackages/web-platform/web-core-wasm/ts/client/decodeWorker/cssLoader.ts (1)
100-121: Unreachable code path inparseAndPushSelector.Lines 116-119 check if
s.startsWith('[')inside theelsebranch, but this condition was already handled at line 105. The code inside lines 116-119 is unreachable.♻️ Suggested fix
} else { // Type selector - // It comes as [lynx-tag="div"] usually. - let content = s; - if (s.startsWith('[') && s.endsWith(']')) { - content = s.substring(1, s.length - 1); - } - selector.push_one_selector_section('AttributeSelector', content); + selector.push_one_selector_section('TypeSelector', s); }packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs (1)
88-133: Add cleanup logic forunique_id_to_dom_mapto prevent memory leaks from detached nodes.The map stores cloned
HtmlElementreferences indefinitely with no removal mechanism. Detached DOM nodes remain retained in the map, causing memory leaks. The commented-outgc()function (lines 165–170) addressesunique_id_to_element_mapbut is incomplete and doesn't clean upunique_id_to_dom_map. Either implement the fullgc()method to clean both maps based onis_connected()status, or add explicit removal when elements are detached.
🤖 Fix all issues with AI agents
In `@packages/web-platform/web-core-wasm/.gitignore`:
- Line 4: Replace the invalid gitignore entry `binary/**/**_debug**` with a
consistent, valid pattern that matches debug artifacts in any binary directory;
update it to `**/binary/**/*_debug*` so it uses standard gitignore wildcards and
follows the existing `**/binary/` convention used in the file.
In `@packages/web-platform/web-core-wasm/AGENTS.md`:
- Line 35: Update the description for the utils entry in AGENTS.md to use the
hyphenated form "general-purpose utilities" (replace the current "**`utils`**:
General purpose utilities." with "**`utils`**: General-purpose utilities.") so
the phrase follows standard usage; ensure only the punctuation/spacing is
changed and other wording remains identical.
- Line 48: The phrase "CSS OG style updates" in AGENTS.md is ambiguous; update
the line describing src/main_thread/style_manager.rs to spell out or rephrase
"OG" (e.g., "CSS Open Graph (OG) style updates" if OG means Open Graph, or "CSS
original (OG) style updates" / "CSS legacy style updates" as appropriate), so
the intent is clear to readers; modify the description accordingly to use the
chosen clarified term and optionally add a short parenthetical explaining what
"OG" stands for.
- Line 33: In AGENTS.md update the typo in the description of css_property.rs:
change the word "homogenously" to "homogeneously" in the sentence referencing
css_property.rs, the CSSProperty enum, and the ParsedDeclaration struct so the
documentation uses the correct spelling.
- Line 47: The sentence fragment in AGENTS.md for
`ts/client/mainthread/TemplateManager.ts` should be joined into a single
sentence; change "Manages template loading and processing. a dedicated
`decode.worker.js` to parse binary templates off-main-thread." to a single
capitalized sentence such as "Manages template loading and processing and
includes a dedicated `decode.worker.js` to parse binary templates
off-main-thread," referencing `TemplateManager.ts` and `decode.worker.js`.
In `@packages/web-platform/web-core-wasm/src/js_binding/mts_js_binding.rs`:
- Around line 39-57: Add the missing getClassList signature to the
RustMainthreadContextBinding TypeScript interface and add a unit test mirroring
the existing enableElementEvent/disableElementEvent spies: update the
RustMainthreadContextBinding interface to include getClassList(element:
HTMLElement): string[] (matching the Rust extern pub fn get_class_name_list and
the implementation in WASMJSBinding.ts), and add a test in
tests/element-apis.spec.ts that spies on the binding’s getClassList call for an
element (similar to the existing spies around the enable/disable tests)
asserting it was invoked with the correct element and returns the expected class
list.
In
`@packages/web-platform/web-core-wasm/src/main_thread/element_apis/event_apis.rs`:
- Around line 16-21: Add unit tests in element-apis.spec.ts that cover the
JS-side behavior missing from the existing cross-thread test: (1) test
add_run_worklet_event including its enable/disable guards by registering a
worklet handler via add_run_worklet_event and asserting that enabling/disabling
follows the same whitelist optimization pattern used by the existing "should
optimize..." test; (2) test get_events and the EventInfo struct by calling
get_events after registering events and asserting returned EventInfo entries
contain expected event_name, event_type and event_handler (use wasm-bindgen
getters event_name/event_type/event_handler to inspect values); and (3) test
common_event_handler dispatch logic by invoking common_event_handler with a mock
event and verifying the correct JS handler from EventInfo is invoked. Reuse the
setup and assertion patterns from the existing element-apis.spec.ts test to keep
tests consistent with cross-thread checks.
In `@packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs`:
- Around line 63-79: The push_style_sheet method currently ignores the Result
from StyleManager::push_style_sheet, which can hide DOM append failures; change
push_style_sheet in main_thread_context.rs to handle the Result from
self.style_manager.push_style_sheet(style_info, if is_entry_template { None }
else { Some(entry_name) }) by either returning a Result from the wasm_bindgen
pub fn (propagate the error) or at minimum logging the error via an available
logger before dropping it; update the function signature (and any callers) to
return Result<(), JsValue> or similar if propagating, or call e.g.
process_logger.error / console::error with the error from
StyleManager::push_style_sheet to ensure failures aren’t silently swallowed.
In `@packages/web-platform/web-core-wasm/src/main_thread/style_manager.rs`:
- Around line 139-143: The code silently drops new_font_face_element when
self.root_node.parent_element() is None; change this to return an error (or at
minimum log a warning) instead of silently continuing: when
self.root_node.parent_element() yields None, return Err(JsError::new("Failed to
append `@font-face`: root node has no parent element")) (or call the project
logger with a clear warning) and ensure you do not keep the associated stored
resource if appending fails; update the branch around
self.root_node.parent_element(), append_child(&new_font_face_element), and any
code that stores the resource so failures abort storing.
In `@packages/web-platform/web-core-wasm/src/style_transformer/inline_style.rs`:
- Line 36: Fix the typo in the comment inside inline_style.rs: change "valule"
to "value" so the comment reads "the even value of source should be processed by
hyphenate_style_name"; locate the comment near the hyphenate_style_name
reference to update it accordingly.
- Around line 30-49: The function transform_inline_style_key_value_vec silently
drops a trailing key when given an odd-length source; change it to either assert
the input length is even (debug/assertion) or iterate in explicit pairs (e.g.,
use chunks_exact(2)) so every key has a value; update the loop that currently
fills key and calls hyphenate_style_name and transformer.on_declaration_parsed
to use paired iteration (or add a debug_assert!(source.len() % 2 == 0)) and
ensure you still use InlineStyleGenerator and StyleTransformer::new with the
same call sites.
In `@packages/web-platform/web-core-wasm/src/template/mod.rs`:
- Around line 7-20: Update the Web Core WASM architecture documentation to
reflect the new client-only TemplateManager export and module: document that a
client-only module template_manager exists (symbol: TemplateManager) and is
conditionally compiled with #[cfg(feature = "client")], describe how
TemplateManager alters the public template surface compared to the existing
template_sections module (symbols: template_sections, RawElementTemplate,
RawStyleInfo), and note that TemplateManager is only available when the "client"
feature is enabled so consumers must enable that feature to access the manager
API.
In `@packages/web-platform/web-core-wasm/src/template/template_manager.rs`:
- Around line 25-26: There are two identical #[wasm_bindgen] attributes in a
row; remove one of the duplicate #[wasm_bindgen] annotations so only a single
attribute remains (e.g., on the item annotated in template_manager.rs such as
the struct/impl/function that follows the attributes) to eliminate the redundant
attribute and keep correct wasm_bindgen metadata.
In
`@packages/web-platform/web-core-wasm/src/template/template_sections/style_info/css_property.rs`:
- Around line 581-602: ParsedDeclaration currently always leaves
is_important=false because new() calls tokenize::tokenize(&property_value, &mut
self_entity) without detecting "!important" and on_token() just appends tokens;
fix by detecting and setting the flag before tokenization (or by handling it in
on_token). For a minimal change: in ParsedDeclaration::new(), scan
property_value for a trailing "!important" (case-insensitive, allowing
surrounding whitespace), if found set self_entity.is_important = true and strip
the "!important" suffix from the string passed to tokenize::tokenize;
alternatively implement detection inside tokenize::Parser::on_token (check
token_value == "!important" and set self.is_important instead of pushing it into
value_token_list). Ensure the transformer’s use of
parsed_declaration.is_important now reflects the original declaration.
In
`@packages/web-platform/web-core-wasm/src/template/template_sections/style_info/mod.rs`:
- Around line 7-21: Documentation update required: the template/style_info
module added new submodules (css_property, decoded_style_data,
flattened_style_info, raw_style_info, style_info_decoder) plus client-only
style_sheet_resource and exported StyleSheetResource, and introduced new type
aliases CssOgClassSelectorNameToDeclarationsMap and
CssOgCssIdToClassSelectorNameToDeclarationsMap; update the Web Core WASM
architecture docs to list these new modules, describe their responsibilities
(e.g., raw_style_info -> RawStyleInfo, decoding/flattening/decoded data flow via
style_info_decoder, CSS property handling in css_property), and note the
client-only style_sheet_resource export and its API surface (StyleSheetResource)
so the architecture diagram and module surface reflect the new structure.
In
`@packages/web-platform/web-core-wasm/src/template/template_sections/style_info/style_info_decoder.rs`:
- Around line 922-957: The test shows that custom CSS properties (e.g.,
"--color") lose their name through the rkyv encode/decode path; confirm whether
this is intentional and either preserve unknown_name during serialization or
document the behavior. Inspect RawStyleInfo (and its rkyv implementation) and
the StyleInfoDecoder::new path to ensure the unknown_name field (or equivalent)
is included in the archived schema and copied through
encode()/from_bytes_unchecked, or if keeping current behavior, add clear
docs/tests explaining that only transformed variables produced by
query_transform_rules() (see RENAME_RULE/REPLACE_RULE and generated "--lynx-*"
names) survive encode/decode while arbitrary custom properties are dropped.
Ensure tests reflect the chosen behavior (re-enable or update the commented
assertion or add a new assertion/documentation).
In `@packages/web-platform/web-core-wasm/ts/client/decodeWorker/decode.worker.ts`:
- Around line 351-353: The current transformation of config using
Object.fromEntries with .map calling value.toString() can throw when a value is
null/undefined; update the logic around the config variable (the
Object.entries(config) -> Object.fromEntries(...) block) to defensively handle
null/undefined by either filtering them out (filter(([, v]) => v != null)) or
converting via String(value) / defaulting (e.g., value ?? '') before mapping,
ensuring you modify the Object.entries(...).map(...) pipeline that produces the
new config to avoid calling value.toString() on null/undefined.
In `@packages/web-platform/web-core-wasm/ts/types/IElementPAPI.ts`:
- Around line 325-327: The ElementPAPIs interface currently has the
__ElementFromBinary member commented out, causing a type mismatch with the
implemented/exported API; restore the interface member by uncommenting and
declaring __ElementFromBinary: ElementFromBinaryPAPI on the ElementPAPIs
interface so it matches the implementation used by createMainThreadGlobalThis
and referenced in main-thread-apis.test.ts (or, if you intentionally deprecate
it, remove the corresponding implementation and tests instead—prefer restoring
the interface to match existing code and tests).
🧹 Nitpick comments (15)
.cargo/config.toml (1)
1-5: Scopewasm_jscfg to the wasm target to avoid impacting native builds.
[build]rustflags apply to all targets; if anycfg(wasm_js)blocks aren’t also gated bytarget_arch = "wasm32", native builds could accidentally compile wasm-only paths. Consider moving the flag under the wasm32 target stanza.♻️ Proposed refactor
-[build] -rustflags = ["--cfg", "wasm_js"] - [target.wasm32-unknown-unknown] +rustflags = ["--cfg", "wasm_js"] runner = "wasm-bindgen-test-runner"packages/web-platform/web-core-wasm/Cargo.toml (1)
22-23: Alignwasm-bindgen-testwith workspace/wasm-bindgenversions to prevent mismatches.If the workspace already pins
wasm-bindgen-test, preferworkspace = true; otherwise verify0.3.58matches yourwasm-bindgenmajor/minor.♻️ Optional alignment (if workspace provides it)
-wasm-bindgen-test = "0.3.58" +wasm-bindgen-test = { workspace = true }packages/web-platform/web-core-wasm/ts/types/IMtsBinding.ts (1)
35-38: Method name is misleading after parameter change.The method
markExposureRelatedElementByUniqueIdno longer accepts auniqueIdbut anHTMLElement. Consider renaming tomarkExposureRelatedElementfor clarity and consistency with the new element-based API design.Suggested rename
- markExposureRelatedElementByUniqueId( + markExposureRelatedElement( element: HTMLElement, toEnable: boolean, ): void;packages/web-platform/web-core-wasm/src/utils/hyphenate_style_name.rs (1)
12-25: Consider capacity calculation and leading uppercase behavior.Two observations:
Capacity underestimate:
String::with_capacity(name.len())doesn't account for inserted hyphens. For "backgroundColor", the result needs 16 bytes but only 15 are pre-allocated. This causes a reallocation—minor but avoidable.Leading uppercase: "BackgroundColor" becomes "-background-color". Verify this is acceptable for your use case (typically CSS properties are camelCase, not PascalCase, so this may be fine).
Optional: Improve capacity estimation
pub fn hyphenate_style_name(name: &str) -> String { - let mut result = String::with_capacity(name.len()); + // Estimate capacity: original length + number of uppercase letters + let extra = name.chars().filter(|c| c.is_uppercase()).count(); + let mut result = String::with_capacity(name.len() + extra); for c in name.chars() {packages/web-platform/web-core-wasm/ts/client/decodeWorker/cssLoader.ts (1)
26-42: Dead code block: Imports handling is documented but not implemented.This entire block is comments explaining why imports aren't handled, but it never actually processes
info.imports. If imports support isn't planned, consider removing this commented explanation to reduce noise. If it is planned, consider adding a TODO with a tracking issue.packages/web-platform/web-core-wasm/tests/encode.bench.spec.ts (1)
85-117: Consider adding a small CSS roundtrip and encoding benchmarks.The current benchmarks cover decode and generate well. Consider adding:
- An encoding benchmark (
encodeCSSperformance)- Small/Large CSS roundtrip benchmarks for completeness
This would provide a more comprehensive view of the encode/decode pipeline performance.
packages/web-platform/web-core-wasm/src/main_thread/element_apis/element_data.rs (1)
26-29: Remove or track commented-out exposure-related code.Large blocks of commented code (
exposure_id_assignedfield,clone_node,should_enable_exposure_event) reduce readability. If this functionality is being deprecated, remove the comments. If it's planned for future restoration, add a TODO with a tracking issue reference.♻️ Suggested approach
Either remove the commented code entirely:
pub struct LynxElementData { pub(crate) parent_component_unique_id: usize, pub(crate) css_id: i32, pub(crate) component_id: Option<String>, pub(crate) dataset: Option<js_sys::Object>, pub(crate) component_config: Option<js_sys::Object>, pub(crate) event_handlers_map: Option<FnvHashMap<String, EventHandler>>, - // /** - // * Whether the exposure-id attribute has been assigned - // */ - // pub(crate) exposure_id_assigned: bool, }Or add a tracking TODO if restoration is planned:
+ // TODO(`#ISSUE_NUMBER`): Restore exposure_id_assigned tracking after refactor // pub(crate) exposure_id_assigned: bool,Also applies to: 45-45, 49-79
packages/web-platform/web-core-wasm/src/template/template_manager.rs (1)
42-45: Prefer&strover&Stringfor function parameters.Using
&Stringis not idiomatic Rust. Accept&strinstead, which is more flexible and allows callers to pass string slices directly without requiring aStringallocation.♻️ Suggested refactor
pub(crate) fn get_style_info_by_name( &self, - template_name: &String, + template_name: &str, ) -> Option<Rc<StyleSheetResource>> { self.style_info_map.get(template_name).cloned() }packages/web-platform/web-core-wasm/src/template/template_sections/style_info/style_sheet_resource.rs (1)
48-52: Parameter nameclass_nameshould be plural since it's a collection.The parameter accepts multiple class names but is named singularly.
♻️ Suggested rename
pub(crate) fn query_css_og_declarations_by_css_id( &self, css_id: i32, - class_name: Vec<String>, + class_names: Vec<String>, ) -> String { let mut result = String::new(); if let Some(map) = &self.css_og_css_id_to_class_selector_name_to_declarations_map { if let Some(class_selector_map) = map.get(&css_id) { - for class_name in class_name.iter() { + for class_name in class_names.iter() {packages/web-platform/web-core-wasm/tests/encode.spec.ts (1)
356-368: Stale commented code references removed API.This commented-out test references
DecodedStyle.webWorkerDecodewhich no longer exists. Either update it to use the new API or remove it entirely.♻️ Options
Option 1: Remove the stale code
- // test('cssog basic', () => { const cssMap = { - // '0': CSS.parse(` - // .parent{ - // background-color: green; - // } - // `).root, - // }; - // const buffer = encodeCSS(cssMap); - // const decodedString = get_style_content( - // DecodedStyle.webWorkerDecode(buffer, true, undefined), - // ); - // expect(decodedString.trim()).toBe(''); - // })Option 2: Update to new API if test is needed
test('cssog basic', () => { const cssMap = { '0': CSS.parse(` .parent{ background-color: green; } `).root, }; const buffer = encodeCSS(cssMap); const decodedString = get_style_content( decode_style_info(buffer, undefined, true), ); expect(decodedString.trim()).toBe(''); });packages/web-platform/web-core-wasm/src/main_thread/style_manager.rs (1)
39-39: Extract magic string"__Card__"to a constant.The default entry name
"__Card__"is duplicated on lines 39 and 109. Extract this to a module-level constant for consistency and easier maintenance.♻️ Proposed fix
+const DEFAULT_ENTRY_NAME: &str = "__Card__"; + pub struct StyleManager { // ... } impl StyleManager { // ... pub fn update_css_og_style( // ... ) -> Result<(), JsError> { - let entry_name = entry_name.as_deref().unwrap_or("__Card__"); + let entry_name = entry_name.as_deref().unwrap_or(DEFAULT_ENTRY_NAME); // ... } pub fn push_style_sheet( // ... ) -> Result<(), JsError> { - let entry_key = entry_name.clone().unwrap_or_else(|| "__Card__".to_string()); + let entry_key = entry_name.clone().unwrap_or_else(|| DEFAULT_ENTRY_NAME.to_string()); // ... } }Also applies to: 109-109
packages/web-platform/web-core-wasm/src/constants.rs (1)
14-23: Consider removing the commented-out constants.Leaving deprecated public constants as comments can drift and confuse API consumers; prefer deleting or documenting in a changelog.
♻️ Proposed cleanup
-// #[cfg(feature = "client")] -// pub const LYNX_TEMPLATE_MEMBER_ID_ATTRIBUTE: &str = "l-t-e-id"; -// #[cfg(feature = "client")] -// pub const APPEAR_EVENT_NAME: &str = "appear"; -// #[cfg(feature = "client")] -// pub const DISAPPEAR_EVENT_NAME: &str = "disappear"; -// #[cfg(feature = "client")] -// pub const LYNX_EXPOSURE_ID_ATTRIBUTE: &str = "exposure-id"; // if this attribute is present, the exposure event is enabled -// #[cfg(feature = "client")] -// pub const LYNX_TIMING_FLAG_ATTRIBUTE: &str = "__lynx_timing_flag"; // if this attribute is present, we should collect timing flags on creating and send it on calling __flushElementTreepackages/web-platform/web-core-wasm/src/style_transformer/transformer.rs (1)
403-409: TODO: escaped property names.If support for escaped identifiers is planned, consider opening a tracking issue; I can help implement the parser changes if you want.
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/css_property.rs (2)
463-471: Consider using a safer alternative totransmutefor enum conversion.The
unsafe { std::mem::transmute }is fragile because it assumes enum variants remain contiguous. If a variant is ever removed or gaps are introduced, this will produce undefined behavior silently.Consider using a
matchstatement or thenum_enumcrate for safer conversions. Alternatively, add a compile-time assertion to verify contiguity.♻️ Suggested safer implementation using match (alternative: use num_enum crate)
// Alternative: Add a compile-time check to ensure safety impl CSSPropertyEnum { pub fn from_id(id: usize) -> Self { // Compile-time assertion that the enum is contiguous const _: () = assert!(CSSPropertyEnum::OffsetDistance as u32 == 215); if id <= CSSPropertyEnum::OffsetDistance as usize { // SAFETY: We've verified id is in range and enum is #[repr(u32)] with contiguous values 0..=215 unsafe { std::mem::transmute(id as u32) } } else { CSSPropertyEnum::Unknown } } }
545-549: Hash collisions for unknown properties may affect HashMap performance.The
Hashimplementation only hashesid, so all unknown CSS properties (e.g., "custom-a", "custom-b") will have identical hashes. While the tests indicate this is intentional, consider documenting this trade-off with an inline comment explaining why this design choice was made and when it's acceptable.📝 Add documentation comment
impl std::hash::Hash for CSSProperty { + // NOTE: Hash is based solely on `id` for performance. All unknown properties + // will hash to the same value, which is acceptable because CSSProperty equality + // still differentiates them via `unknown_name`. Avoid using CSSProperty as a + // HashMap key when many unknown properties are expected. fn hash<H: std::hash::Hasher>(&self, state: &mut H) { self.id.hash(state); } }
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/css_property.rs
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/mod.rs
Show resolved
Hide resolved
...s/web-platform/web-core-wasm/src/template/template_sections/style_info/style_info_decoder.rs
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/ts/client/decodeWorker/decode.worker.ts
Show resolved
Hide resolved
…date element exposure event logic.
PupilTong
force-pushed
the
p/hw/rust-refactor-2
branch
from
4e86bec to
fcddeb2
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging this PR will degrade performance by 9.29%Comparing Summary
Performance Changes
Footnotes
|
Web Explorer#7419 Bundle Size — 383.56KiB (0%).59a4a5b(current) vs b020525 main#7416(baseline) Bundle metrics
|
| Current #7419 |
Baseline #7416 |
|
|---|---|---|
 Initial JS |
154.71KiB |
154.71KiB |
| Initial CSS |
35.05KiB |
35.05KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
238(+0.42%) |
237 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.99% |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7419 |
Baseline #7416 |
|
|---|---|---|
| JS |
252.66KiB |
252.66KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.05KiB |
35.05KiB |
Bundle analysis report Branch PupilTong:p/hw/rust-refactor-2 Project dashboard
Generated by RelativeCI Documentation Report issue
…age, and refine Rust type casting.
…PI definitions, and refined the debug binary ignore pattern in `.gitignore`.
PupilTong
force-pushed
the
p/hw/rust-refactor-2
branch
from
7beaa34 to
693b19e
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)
packages/web-platform/web-core-wasm/src/main_thread/element_apis/element_data.rs (1)
10-46: Add tests for the newadd_run_worklet_eventpath and verify exposure event handling.The changes add
framework_run_worklet_identifierhandling toEventHandler, buttests/element-apis.spec.tshas no test coverage for theadd_run_worklet_eventfunction or the different code paths it takes whenframeworkCrossThreadIdentifieris null versus an object. Additionally, the removal ofexposure_id_assignedlacks corresponding test verification.The existing event enable/disable test only validates the
add_cross_thread_eventpath. Add tests to cover:
add_run_worklet_eventregistration with handler identifiers- Event enable/disable behavior for reactive events in the run_worklet path
- Exposure event handling (uiappear/uidisappear) from the removed field
packages/web-platform/web-core-wasm/ts/client/mainthread/TemplateManager.ts (1)
184-210: Handleadd_style_infofailures to avoid hanging template loads.If
templateManagerWasm.add_style_info(...)throws (bad buffer or wasm init issue), the async handler rejects but the caller doesn’t await it, leaving#pendingResolvesunresolved and the view stuck. Wrap this branch in atry/catchand reject/cleanup on failure.🛠️ Proposed fix
case TemplateSectionLabel.StyleInfo: { - templateManagerWasm!.add_style_info( - url, - new Uint8Array(data as ArrayBuffer), - document, - ); - instance.onStyleInfoReady(url); + try { + if (!templateManagerWasm) { + throw new Error('TemplateManager wasm not initialized'); + } + templateManagerWasm.add_style_info( + url, + new Uint8Array(data as ArrayBuffer), + document, + ); + instance.onStyleInfoReady(url); + } catch (error) { + console.error(`Error applying style info ${url}:`, error); + this.#cleanup(url); + this.#removeTemplate(url); + this.#rejectPromise( + url, + error instanceof Error ? error : new Error(String(error)), + ); + } break; }packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createElementAPI.ts (1)
286-309: Remove redundant null check and add type safety to style value processing.The code has a dead code path (line 302's redundant
else if (!value)check). More importantly, while the TypeScript signature guaranteesRecord<string, string>for object values, the.map((item) => item.toString())approach is fragile. Consider using a more explicit transformation that handles the flattened array safely:Suggested fix
- } else if (!value) { - element.removeAttribute('style'); - } else { + } else { get_inline_styles_in_key_value_vec( element, - Object.entries(value).flat().map((item) => item.toString()), + Object.entries(value).flat() as string[], ); }The type cast is safe here because TypeScript guarantees the object values are strings (Record<string, string>), making the cast explicit without needing runtime guards.
🤖 Fix all issues with AI agents
In `@packages/web-platform/web-core-wasm/Cargo.toml`:
- Around line 22-23: The dev-dependency wasm-bindgen-test is pinned to "0.3.58"
while wasm-bindgen in this crate uses workspace dependency management; to align
them, either move wasm-bindgen-test into the root workspace Cargo.toml under
[dev-dependencies] (so the crate uses wasm-bindgen-test with workspace = true
like wasm-bindgen) or add a short justification comment in this crate's
Cargo.toml explaining why wasm-bindgen-test must be pinned here; update
references to the wasm-bindgen-test entry so they match whichever approach you
choose.
- Line 13: Add rkyv to the workspace dependencies with the "derive" feature
enabled and update the crate's local dependency to use the workspace entry: in
the workspace root Cargo.toml add a workspace dependency for rkyv at version 0.7
with features including "derive", then in the web-core-wasm Cargo.toml replace
the direct rkyv version entry with a workspace-based dependency (i.e., point
rkyv to the workspace) so the derive macros used by #[derive(Archive, Serialize,
Deserialize)] are available and versioning is centralized.
In
`@packages/web-platform/web-core-wasm/src/main_thread/element_apis/event_apis.rs`:
- Around line 61-73: Replace the silent `if let Some(element) =
self.unique_id_to_dom_map.get(&unique_id)` branches with a lookup that uses
`expect_throw("El")` so missing DOM elements fail loudly; specifically, in the
event enable/disable paths call
`self.unique_id_to_dom_map.get(&unique_id).expect_throw("El")` and pass that
`element` into `self.mts_binding.enable_element_event(element, event_name_str)`
and `self.mts_binding.disable_element_event(element, event_name_str)`
respectively (this matches the pattern used in style_apis.rs and enforces the
invariant with `unique_id_to_element_map`).
In `@packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs`:
- Around line 124-127: The map unique_id_to_dom_map currently holds strong
references to HtmlElement and never removes entries, causing memory growth;
implement a cleanup: either add a public removal method (e.g., remove_unique_id
or drop_element_by_id) that removes the entry from both unique_id_to_dom_map and
unique_id_to_element_map when an element is disposed, or implement and call the
commented gc() function to iterate entries, calling is_connected() on each
HtmlElement and removing absent ones from both maps; update places that detach
elements to call the removal API or schedule gc() appropriately so detached
nodes are swept.
In `@packages/web-platform/web-core-wasm/src/main_thread/style_manager.rs`:
- Around line 47-63: The code silently ignores when HtmlStyleElement::sheet()
returns None, leaving css_og_style_sheet unset; change this to return or surface
an explicit error instead of a no-op: after obtaining sheet via let sheet =
css_og_style_element.unchecked_into::<HtmlStyleElement>().sheet(); check if
sheet.is_none() and return an Err(JsError::new("Failed to get stylesheet from
created <style> element")) (or map the Option to a Result with a descriptive
message) before assigning self.css_og_style_sheet, referencing the
css_og_style_sheet field and the css_og_style_element / HtmlStyleElement sheet()
call so callers can handle the failure instead of it being silent.
In `@packages/web-platform/web-core-wasm/src/utils/hyphenate_style_name.rs`:
- Around line 12-21: The current hyphenate_style_name function mishandles MS
vendor-prefixed names (e.g., "msTransform" -> "ms-transform" instead of
"-ms-transform"); update hyphenate_style_name to special-case names that start
with "ms" followed by an uppercase letter: prepend a '-' to result, then append
'm' and 's' (lowercase) and continue the existing per-char logic for the
remainder so "msTransform" becomes "-ms-transform"; add a unit test asserting
hyphenate_style_name("msTransform") == "-ms-transform" to cover this defensive
behavior.
In `@packages/web-platform/web-core-wasm/src/utils/mod.rs`:
- Line 7: Add the new utils module hyphenate_style_name to the Web Core WASM
architecture documentation so ownership and module responsibilities remain
traceable; locate the utils module declaration (pub mod hyphenate_style_name) in
mod.rs and update the architecture docs to list the new module, its purpose,
public API surface, and the owning team or maintainer contact, and include any
relevant build/wasm integration notes so the docs reflect the current code
structure.
In
`@packages/web-platform/web-core-wasm/ts/client/mainthread/LynxViewInstance.ts`:
- Around line 146-158: The StyleInfo handler can run before wasmContext is
initialized because `#handleMessage` calls the async `#handleSection` without
awaiting it, so onStyleInfoReady may call push_style_sheet while wasmContext is
undefined and still call styleReadyResolve and set parentDom.style.display; to
fix, either make `#handleMessage` await this.#handleSection(...) so sections are
processed sequentially, or change onStyleInfoReady to defer applying styles
until mtsWasmBinding.wasmContext exists (e.g., store the pending style info and
call push_style_sheet once wasmContext is set or await a wasmContext-initialized
promise), ensuring push_style_sheet is only invoked when wasmContext is
initialized and delaying styleReadyResolve/display change until after the push
completes.
🧹 Nitpick comments (8)
packages/web-platform/web-core-wasm/src/constants.rs (1)
14-23: Remove commented-out code instead of keeping it.Commented-out code adds noise and reduces readability. Since this is a refactoring PR, these constants should be fully removed rather than kept as comments. Version control already preserves the history if needed.
♻️ Suggested removal
#[cfg(feature = "client")] pub const LYNX_UNIQUE_ID_ATTRIBUTE: &str = "l-uid"; -// #[cfg(feature = "client")] -// pub const LYNX_TEMPLATE_MEMBER_ID_ATTRIBUTE: &str = "l-t-e-id"; -// #[cfg(feature = "client")] -// pub const APPEAR_EVENT_NAME: &str = "appear"; -// #[cfg(feature = "client")] -// pub const DISAPPEAR_EVENT_NAME: &str = "disappear"; -// #[cfg(feature = "client")] -// pub const LYNX_EXPOSURE_ID_ATTRIBUTE: &str = "exposure-id"; // if this attribute is present, the exposure event is enabled -// #[cfg(feature = "client")] -// pub const LYNX_TIMING_FLAG_ATTRIBUTE: &str = "__lynx_timing_flag"; // if this attribute is present, we should collect timing flags on creating and send it on calling __flushElementTree + lazy_static::lazy_static! {packages/web-platform/web-core-wasm/ts/encode/webEncoder.ts (1)
58-70: AlignTasmJSONInfowith removed Element Templates serialization.
encodeno longer writes Element Templates, but the field is still required onTasmJSONInfo. Consider removing it or at least marking it optional so callers don’t assume it’s consumed.♻️ Suggested tweak
export type TasmJSONInfo = { styleInfo: Record<string, CSS.LynxStyleNode[]>; manifest: Record<string, string>; cardType: string; appType: string; pageConfig: Record<string, unknown>; lepusCode: Record<string, string>; customSections: Record<string, { type?: 'lazy'; content: string | Record<string, unknown>; }>; - elementTemplates: Record<string, ElementTemplateData>; + elementTemplates?: Record<string, ElementTemplateData>; };packages/web-platform/web-core-wasm/src/main_thread/element_apis/element_data.rs (1)
49-79: Consider removing the commented-out exposure/clone blocks.If these paths are truly retired, deleting the commented code will keep the file easier to scan and avoid confusion about current behavior.
🧹 Possible cleanup
- // pub(crate) fn clone_node(&self, parent_component_unique_id: usize, css_id: i32) -> Self { - // LynxElementData { - // parent_component_unique_id, - // css_id, - // dataset: self - // .dataset - // .as_ref() - // .map(|dataset| js_sys::Object::assign(&js_sys::Object::default(), dataset)), - // component_config: self.component_config.as_ref().map(|component_config| { - // js_sys::Object::assign(&js_sys::Object::default(), component_config) - // }), - // component_id: self.component_id.clone(), - // event_handlers_map: self.event_handlers_map.clone(), - // exposure_id_assigned: self.exposure_id_assigned, - // } - // } - - // /** - // * There are two conditions to enable exposure/disexposure(InsectionObserver) detection: - // * 1. an element has exposure-id attribute - // * 2. an element has 'appear'/'disappear' event listener added - // */ - // pub(crate) fn should_enable_exposure_event(&self) -> bool { - // self.exposure_id_assigned || { - // if let Some(event_handlers_map) = &self.event_handlers_map { - // event_handlers_map.contains_key("appear") || event_handlers_map.contains_key("disappear") - // } else { - // false - // } - // } - // }packages/web-platform/web-core-wasm/src/template/template_sections/style_info/decoded_style_data.rs (1)
33-42: Document unsafe usage and consider validation for untrusted inputs.
rkyv::from_bytes_uncheckedskips validation and can cause undefined behavior if the input is malformed. While this may be acceptable for performance-critical paths with trusted data, consider:
- Adding a doc comment explaining why unchecked deserialization is safe in this context (e.g., data always comes from trusted internal serialization).
- For any code paths where the buffer could originate from untrusted sources, use
rkyv::from_byteswith validation instead.📝 Suggested documentation
impl TryFrom<js_sys::Uint8Array> for DecodedStyleData { type Error = wasm_bindgen::JsError; fn try_from(buffer: js_sys::Uint8Array) -> Result<DecodedStyleData, Self::Error> { let buf = buffer.to_vec(); + // SAFETY: The buffer is expected to come from internal serialization via rkyv::to_bytes. + // If this data could originate from untrusted sources, use rkyv::from_bytes with validation. let data = unsafe { rkyv::from_bytes_unchecked::<DecodedStyleData>(&buf) }.map_err(|e| { wasm_bindgen::JsError::new(&format!("Failed to decode DecodedStyleData: {e:?}")) })?; Ok(data) } }packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/WASMJSBinding.ts (1)
62-66: Consider type assertion for DOMTokenList iteration.The spread operator on
classListworks becauseDOMTokenListis iterable, but the type castas unknown as string[]is awkward.♻️ Cleaner alternative
getClassList( element: HTMLElement, ): string[] { - return [...(element.classList as unknown as string[])]; + return Array.from(element.classList); }packages/web-platform/web-core-wasm/src/main_thread/mod.rs (1)
12-17: Update module docs to mentionstyle_manager.
Lines 12-17: The module-level “Key components” list now omits the new publicstyle_manager; add a bullet for clarity.📝 Doc-only tweak
/// Key components: /// - `main_thread_context`: Defines `MainThreadWasmContext`, the central struct holding the application state. /// - `element_apis`: Contains APIs for manipulating elements, handling events, and managing styles. +/// - `style_manager`: Provides `StyleManager` for stylesheet management on the main thread. pub(crate) mod element_apis; pub(crate) mod main_thread_context; pub mod style_manager;packages/web-platform/web-core-wasm/benches/wasm_bench.rs (1)
69-76: Add safety comment for unsafe block.The
rkyv::from_bytes_uncheckedcall is unsafe and skips validation. While this is acceptable in controlled benchmark code where the input is known-good, adding a// SAFETY:comment documenting why this is safe would improve maintainability.📝 Suggested documentation
#[cfg(feature = "encode")] fn decode_style_info_bytes(bytes: &js_sys::Uint8Array) { let mut buf = vec![0u8; bytes.length() as usize]; bytes.copy_to(&mut buf); + // SAFETY: `bytes` was produced by `build_style_info_bytes()` using + // `RawStyleInfo::encode()`, guaranteeing valid rkyv archive format. let raw = unsafe { rkyv::from_bytes_unchecked::<RawStyleInfo>(&buf) } .expect("RawStyleInfo decode should succeed"); let _ = StyleInfoDecoder::new(raw, None, true).expect("StyleInfoDecoder should succeed"); }packages/web-platform/web-core-wasm/src/template/template_manager.rs (1)
12-23: Consider updating Web Core WASM architecture docs for the new TemplateManager module.Based on learnings: Keep the Web Core WASM architecture documentation updated if you add new modules or change the architecture.
packages/web-platform/web-core-wasm/src/main_thread/element_apis/event_apis.rs
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/ts/client/mainthread/LynxViewInstance.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/web-platform/web-core-wasm/binary/client/client.d.ts`:
- Around line 156-176: The doc comment above add_inline_style_raw_string_key is
inaccurate: update the block comment to state that the key and value are strings
(value may be optional/null) and remove references to numeric keys/values so it
matches the function signature add_inline_style_raw_string_key(dom: HTMLElement,
key: string, value?: string | null): void; ensure wording clarifies value may be
undefined/null and that the key is a string.
In `@packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs`:
- Around line 63-82: The tests are missing JS-side coverage for the new
wasm-bindgen APIs push_style_sheet and get_dom_by_unique_id; add tests in
tests/element-apis.spec.ts that import the built wasm module, instantiate or
obtain the main-thread context, call push_style_sheet(templateManager,
entryName, isEntryTemplate) and assert it returns successfully and that the
stylesheet was injected/applied (e.g., by checking document.styleSheets or
computed styles), and create an element with the unique id then call
get_dom_by_unique_id(uniqueId) and assert it returns the correct DOM node;
reference the exported functions push_style_sheet and get_dom_by_unique_id and
use TemplateManager or stubs to supply style info/entryName as needed.
In
`@packages/web-platform/web-core-wasm/src/template/template_sections/style_info/raw_style_info.rs`:
- Around line 28-41: The Rule struct's recursive field currently uses an
unsupported #[archive(boxed)] attribute on pub(super) nested_rules: Vec<Rule>;
remove the #[archive(boxed)] attribute and change the field type to pub(super)
nested_rules: Vec<Box<Rule>> so rkyv can handle the recursive type; keep the
existing #[omit_bounds] and archive(bound(...)) attributes on the Rule derive
and ensure no other archive-specific attributes remain on nested_rules.
🧹 Nitpick comments (1)
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/raw_style_info.rs (1)
136-142: Consider increasing initial scratch space forrkyv::to_bytes.The fixed scratch space of 1024 bytes may be undersized for larger stylesheets with many rules. While rkyv will reallocate as needed, a more generous initial allocation (e.g., 4096 or 8192) could reduce reallocation overhead for typical payloads.
♻️ Optional: Increase initial scratch space
- let serialized = rkyv::to_bytes::<_, 1024>(self) + let serialized = rkyv::to_bytes::<_, 4096>(self)
packages/web-platform/web-core-wasm/src/main_thread/main_thread_context.rs
Show resolved
Hide resolved
packages/web-platform/web-core-wasm/src/template/template_sections/style_info/raw_style_info.rs
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist