refactor: with WeakRef in element APIs and WASM bindings to improve m…#2439
Open
PupilTong wants to merge 3 commits intolynx-family:mainfrom
Open
refactor: with WeakRef in element APIs and WASM bindings to improve m…#2439PupilTong wants to merge 3 commits intolynx-family:mainfrom
PupilTong wants to merge 3 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 965bd1f The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors web-core to use WeakRef for DOM references across Rust and TypeScript, updates WASM exports/signatures (adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ 1 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs (1)
75-160:⚠️ Potential issue | 🟠 MajorAdd JS-side coverage for the WeakRef lifecycle and
gc()cleanup.This changes
src/main_threadbehavior across the JS/WASM boundary, but the PR does not include the correspondingelement-apis.spec.tscoverage for lookup/deref and stale-entry cleanup. Please add tests for the newget_dom_by_unique_id()?.deref()flow and forgc()clearing dead handles, preferably with theWASMJSBinding.tsmock.As per coding guidelines,
packages/web-platform/web-core/src/main_thread/**/*.rs: "When modifyingsrc/main_thread, ALWAYS add corresponding tests intests/element-apis.spec.tsto verify the JS-side behavior"; based on learnings, useWASMJSBinding.tsto mock the Wasm binding in those tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs` around lines 75 - 160, Add JS-side tests in tests/element-apis.spec.ts to cover the new WeakRef lifecycle and gc() cleanup: write tests that call the wasm-exposed paths which use create_element_common to register elements, assert get_dom_by_unique_id()?.deref() returns the mocked HtmlElement when alive (use WASMJSBinding.ts to mock the Wasm binding and supply js_sys.WeakRef-like behavior), then simulate the element going out of scope / being collected and call gc() via the wasm binding and assert the weak ref is gone and the unique_id entry in the JS-visible maps is cleared; reference the exported methods that drive these flows (create_element_common behavior leading to get_dom_by_unique_id and gc) so the tests exercise the lookup/deref and stale-entry cleanup code paths.
🧹 Nitpick comments (2)
.changeset/refactor-weakref-webcore.md (1)
5-5: Consider improving the description grammar.The phrase "with WeakRef" is grammatically awkward. Consider rephrasing to:
- "refactor: use WeakRef in element APIs and WASM bindings to improve memory management"
- "refactor: introduce WeakRef in element APIs and WASM bindings to improve memory management"
✏️ Suggested wording improvement
-refactor: with WeakRef in element APIs and WASM bindings to improve memory management. +refactor: use WeakRef in element APIs and WASM bindings to improve memory management🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/refactor-weakref-webcore.md at line 5, Update the changeset title phrase "refactor: with WeakRef in element APIs and WASM bindings to improve memory management" to a grammatically correct form; edit the string in .changeset/refactor-weakref-webcore.md replacing the awkward "with WeakRef" with one of the suggested alternatives such as "refactor: use WeakRef in element APIs and WASM bindings to improve memory management" or "refactor: introduce WeakRef in element APIs and WASM bindings to improve memory management" so the commit message reads clearly and consistently.packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs (1)
60-72: Silently ignoring Result may hide binding errors.The
let _ = ...pattern explicitly discards errors fromenable_element_eventanddisable_element_event. While this prevents panics, it could hide issues when the WeakRef has been garbage collected or when there are binding failures.Consider logging these errors at debug level for troubleshooting, or at minimum documenting why errors are intentionally ignored here.
♻️ Optional: Log errors at debug level
if should_enable { if let Some(element) = self.unique_id_to_dom_map.get(&unique_id) { - let _ = self + if let Err(e) = self .mts_binding - .enable_element_event(element, event_name_str); + .enable_element_event(element, event_name_str) { + // WeakRef may have been collected - this is expected during cleanup + web_sys::console::debug_1(&e); + } } } else if should_disable { if let Some(element) = self.unique_id_to_dom_map.get(&unique_id) { - let _ = self + if let Err(e) = self .mts_binding - .disable_element_event(element, event_name_str); + .disable_element_event(element, event_name_str) { + web_sys::console::debug_1(&e); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs` around lines 60 - 72, The code is discarding the Result from calls to mts_binding.enable_element_event and disable_element_event, hiding binding errors; update the branches where self.unique_id_to_dom_map.get(&unique_id) is Some(...) so that the returned Result is inspected and any Err is logged (e.g., via tracing::debug! or the crate's logger) with context including unique_id and event_name_str; ensure you call logging in both the enable path (mts_binding.enable_element_event) and disable path (mts_binding.disable_element_event) so failures (like a dropped WeakRef or binding error) are visible while preserving current control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs`:
- Around line 117-119: The get_dom_by_unique_id() Rust binding now returns a
js_sys::WeakRef, but createElementAPI.ts still treats it as an HTMLElement;
update the two callers to dereference the weak handle before using it: at the
call that passes the value to markExposureRelatedElementByUniqueId(), call
.deref() and validate non-null before passing, and at the call that uses
element.insertBefore(), deref the WeakRef to a Node and check for null;
alternatively replace direct get_dom_by_unique_id() usage with the existing
getElementByUniqueId() wrapper in WASMJSBinding.ts which already performs the
dereference and null-check.
---
Outside diff comments:
In
`@packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs`:
- Around line 75-160: Add JS-side tests in tests/element-apis.spec.ts to cover
the new WeakRef lifecycle and gc() cleanup: write tests that call the
wasm-exposed paths which use create_element_common to register elements, assert
get_dom_by_unique_id()?.deref() returns the mocked HtmlElement when alive (use
WASMJSBinding.ts to mock the Wasm binding and supply js_sys.WeakRef-like
behavior), then simulate the element going out of scope / being collected and
call gc() via the wasm binding and assert the weak ref is gone and the unique_id
entry in the JS-visible maps is cleared; reference the exported methods that
drive these flows (create_element_common behavior leading to
get_dom_by_unique_id and gc) so the tests exercise the lookup/deref and
stale-entry cleanup code paths.
---
Nitpick comments:
In @.changeset/refactor-weakref-webcore.md:
- Line 5: Update the changeset title phrase "refactor: with WeakRef in element
APIs and WASM bindings to improve memory management" to a grammatically correct
form; edit the string in .changeset/refactor-weakref-webcore.md replacing the
awkward "with WeakRef" with one of the suggested alternatives such as "refactor:
use WeakRef in element APIs and WASM bindings to improve memory management" or
"refactor: introduce WeakRef in element APIs and WASM bindings to improve memory
management" so the commit message reads clearly and consistently.
In
`@packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs`:
- Around line 60-72: The code is discarding the Result from calls to
mts_binding.enable_element_event and disable_element_event, hiding binding
errors; update the branches where self.unique_id_to_dom_map.get(&unique_id) is
Some(...) so that the returned Result is inspected and any Err is logged (e.g.,
via tracing::debug! or the crate's logger) with context including unique_id and
event_name_str; ensure you call logging in both the enable path
(mts_binding.enable_element_event) and disable path
(mts_binding.disable_element_event) so failures (like a dropped WeakRef or
binding error) are visible while preserving current control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5aa3ad09-ee11-4f06-98ab-ee5c996f0012
📒 Files selected for processing (13)
.changeset/refactor-weakref-webcore.mdpackages/web-platform/web-core/binary/client/client.d.tspackages/web-platform/web-core/binary/client/client_bg.wasm.d.tspackages/web-platform/web-core/binary/client_legacy/client.d.tspackages/web-platform/web-core/binary/client_legacy/client_bg.wasm.d.tspackages/web-platform/web-core/src/js_binding/mts_js_binding.rspackages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rspackages/web-platform/web-core/src/main_thread/client/element_apis/style_apis.rspackages/web-platform/web-core/src/main_thread/client/main_thread_context.rspackages/web-platform/web-core/tests/element-apis.spec.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/WASMJSBinding.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.tspackages/web-platform/web-core/ts/types/IMtsBinding.ts
packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs
Show resolved
Hide resolved
Merging this PR will degrade performance by 14.57%
Performance Changes
Comparing 
Footnotes
|
React External#301 Bundle Size — 590.13KiB (0%).965bd1f(current) vs 5151fcf main#294(baseline) Bundle metrics
|
| Current #301 |
Baseline #294 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch PupilTong:p/hw/use-weak-ref-dom-... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8757 Bundle Size — 748.66KiB (+0.7%).965bd1f(current) vs 5151fcf main#8751(baseline) Bundle metrics
|
| Current #8757 |
Baseline #8751 |
|
|---|---|---|
 Initial JS |
44.27KiB(+1.04%) |
43.82KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
 Cache Invalidation |
48.89% |
28.41% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
149 |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
35.02%(+0.17%) |
34.96% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 2 changes 2 regressions
| Current #8757 |
Baseline #8751 |
|
|---|---|---|
| Other |
401.63KiB (+0.87%) |
398.15KiB |
| JS |
344.87KiB (+0.5%) |
343.13KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch PupilTong:p/hw/use-weak-ref-dom-... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#316 Bundle Size — 206.11KiB (0%).965bd1f(current) vs 5151fcf main#309(baseline) Bundle metrics
|
| Current #316 |
Baseline #309 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.76% |
45.76% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #316 |
Baseline #309 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
94.88KiB |
94.88KiB |
Bundle analysis report Branch PupilTong:p/hw/use-weak-ref-dom-... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7183 Bundle Size — 236.82KiB (0%).965bd1f(current) vs 5151fcf main#7176(baseline) Bundle metrics
|
| Current #7183 |
Baseline #7176 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
46.1% |
46.1% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7183 |
Baseline #7176 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.06KiB |
91.06KiB |
Bundle analysis report Branch PupilTong:p/hw/use-weak-ref-dom-... Project dashboard
Generated by RelativeCI Documentation Report issue
Sherry-hue
previously approved these changes
Apr 10, 2026
PupilTong
force-pushed
the
p/hw/use-weak-ref-dom-handle
branch
from
8c57025 to
965bd1f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web-platform/web-core/src/js_binding/mts_js_binding.rs`:
- Around line 39-72: The new wasm_bindgen methods (enable_element_event,
disable_element_event, get_class_name_list, set_attribute, remove_attribute) now
return Result<..., JsValue> but callers like add_cross_thread_event in
event_apis.rs and set_css_id in style_apis.rs currently discard results with let
_ = ..., causing JS/DOM errors to be swallowed; update those call sites to
handle the Result by either propagating the error (use the ? operator or convert
JsValue into the function's error type and return Err) in functions that already
return Result, or at minimum log the Err (convert JsValue to JsError/Debug)
before continuing, ensuring you address every occurrence where
enable_element_event/disable_element_event/get_class_name_list/set_attribute/remove_attribute
are discarded rather than handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fa4ff99-96d2-4c27-bd49-71c35b522f8b
📒 Files selected for processing (14)
.changeset/refactor-weakref-webcore.mdpackages/web-platform/web-core/binary/client/client.d.tspackages/web-platform/web-core/binary/client/client_bg.wasm.d.tspackages/web-platform/web-core/binary/client_legacy/client.d.tspackages/web-platform/web-core/binary/client_legacy/client_bg.wasm.d.tspackages/web-platform/web-core/src/js_binding/mts_js_binding.rspackages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rspackages/web-platform/web-core/src/main_thread/client/element_apis/style_apis.rspackages/web-platform/web-core/src/main_thread/client/main_thread_context.rspackages/web-platform/web-core/tests/element-apis.spec.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/WASMJSBinding.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.tspackages/web-platform/web-core/ts/client/wasm.tspackages/web-platform/web-core/ts/types/IMtsBinding.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/refactor-weakref-webcore.md
- packages/web-platform/web-core/tests/element-apis.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
- packages/web-platform/web-core/ts/client/wasm.ts
- packages/web-platform/web-core/src/main_thread/client/element_apis/style_apis.rs
- packages/web-platform/web-core/binary/client_legacy/client_bg.wasm.d.ts
- packages/web-platform/web-core/binary/client/client_bg.wasm.d.ts
- packages/web-platform/web-core/binary/client_legacy/client.d.ts
PupilTong
force-pushed
the
p/hw/use-weak-ref-dom-handle
branch
from
c6f2a9e to
965bd1f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…emory management.
Summary by CodeRabbit
Refactor
New Features
Tests
Checklist