feat: implement global-bind event support for decoupled cross-element…#2438
Merged
PupilTong merged 8 commits intolynx-family:mainfrom Apr 10, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 0601e37 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:
📝 WalkthroughWalkthroughAdds documentation, WASM binding declarations, Rust runtime bookkeeping and dispatch, and tests to implement a new 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: 2
🧹 Nitpick comments (2)
packages/web-platform/web-core-e2e/tests/reactlynx/basic-global-bind/index.jsx (1)
31-31: Prefer functional state update for toggle safety.Use a functional setter to avoid stale state reads during rapid event bursts.
♻️ Suggested change
- global-bindTap={() => setColor(color === 'pink' ? 'green' : 'pink')} + global-bindTap={() => + setColor((prev) => (prev === 'pink' ? 'green' : 'pink')) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core-e2e/tests/reactlynx/basic-global-bind/index.jsx` at line 31, The current inline handler on global-bindTap reads the outer color variable directly, which can produce stale reads under rapid events; change the handler to call the setColor updater with a functional setter that receives the previous color (e.g., prevColor) and returns the toggled value to ensure safe toggling; update the attribute that uses setColor and color (global-bindTap) to use this functional updater form instead.packages/web-platform/web-core/tests/element-apis.spec.ts (1)
1464-1511: Add a non-bubbling negative assertion for global-bind.The test currently validates bubbling + unregister paths only. Add one
bubbles: falsedispatch check to lock in the “bubble-required” contract.✅ Suggested test addition
rootDom.querySelector('#global_bind_target')?.dispatchEvent( new window.Event('click', { bubbles: true }), ); expect(publishSpy).toHaveBeenCalledTimes(1); @@ undefined, ); + + publishSpy.mockClear(); + rootDom.querySelector('#global_bind_target')?.dispatchEvent( + new window.Event('click', { bubbles: false }), + ); + expect(publishSpy).toHaveBeenCalledTimes(0); // Unregister global-bind mtsGlobalThis.__AddEvent(element2, 'global-bind', 'tap', null as any);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core/tests/element-apis.spec.ts` around lines 1464 - 1511, Add a non-bubbling negative assertion inside the "should handle global-bind events" test: after registering the global-bind on element2 (via mtsGlobalThis.__AddEvent) and after creating publishSpy, dispatch a click event on the target element (found via rootDom.querySelector('#global_bind_target')) with bubbles set to false, and assert that mtsBinding.publishEvent (publishSpy) was not called; keep the existing bubbling dispatch and unregister assertions unchanged. This ensures the global-bind requires bubbling to trigger.
🤖 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/element_apis/event_apis.rs`:
- Around line 38-50: The removal logic in the event handler branch
unconditionally removes unique_id from self.global_bind_events based solely on
the current API call; instead, after calling replace_framework_*_event_handler
(the framework-side registration helpers) re-check both handler registries
(i.e., whether any global-bind handler flavor remains for that element) before
removing unique_id from self.global_bind_events so a listener that still has the
other flavor stays in the dispatch index used by dispatch_global_bind_event;
update the removal branch that uses event_handler_identifier and the symmetric
branch (lines ~101-111) to query both registries and only remove when neither
flavor is present, and add a regression test in tests/element-apis.spec.ts
verifying independent unregister of each global-bind flavor and that
dispatch_global_bind_event still delivers to a remaining handler.
- Around line 354-404: The loop holds a borrow of self.global_bind_events via
global_bind_ids while calling back into JS (self.mts_binding.publish_event /
publish_mts_event), which allows re-entrant mutation and UB; fix by snapshotting
the IDs into a Vec<usize> before the loop so the borrow is released (i.e.,
replace iterating over global_bind_ids directly with let ids: Vec<usize> =
self.global_bind_events.get(&event_name_lowercase).cloned().unwrap_or_default().into_iter().collect()
or similar), then iterate over that Vec and use get_element_data_by_unique_id,
get_framework_cross_thread_event_handler, publish_event and publish_mts_event as
before.
---
Nitpick comments:
In
`@packages/web-platform/web-core-e2e/tests/reactlynx/basic-global-bind/index.jsx`:
- Line 31: The current inline handler on global-bindTap reads the outer color
variable directly, which can produce stale reads under rapid events; change the
handler to call the setColor updater with a functional setter that receives the
previous color (e.g., prevColor) and returns the toggled value to ensure safe
toggling; update the attribute that uses setColor and color (global-bindTap) to
use this functional updater form instead.
In `@packages/web-platform/web-core/tests/element-apis.spec.ts`:
- Around line 1464-1511: Add a non-bubbling negative assertion inside the
"should handle global-bind events" test: after registering the global-bind on
element2 (via mtsGlobalThis.__AddEvent) and after creating publishSpy, dispatch
a click event on the target element (found via
rootDom.querySelector('#global_bind_target')) with bubbles set to false, and
assert that mtsBinding.publishEvent (publishSpy) was not called; keep the
existing bubbling dispatch and unregister assertions unchanged. This ensures the
global-bind requires bubbling to trigger.
🪄 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: db88821f-ab39-40d2-a330-3e678e9216eb
📒 Files selected for processing (11)
.changeset/feat-global-bind-support.md.changeset/red-dragons-tickle.mdpackages/web-platform/web-core-e2e/tests/reactlynx.spec.tspackages/web-platform/web-core-e2e/tests/reactlynx/basic-global-bind/index.jsxpackages/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/main_thread/client/element_apis/event_apis.rspackages/web-platform/web-core/src/main_thread/client/main_thread_context.rspackages/web-platform/web-core/tests/element-apis.spec.ts
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
Outdated
Show resolved
Hide resolved
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
Outdated
Show resolved
Hide resolved
Merging this PR will degrade performance by 8.07%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | transform 1000 view elements |
43.6 ms | 47.4 ms | -8.07% |
Comparing PupilTong:p/hw/support-global-bind (0601e37) with main (045ca2f)
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React Example#7173 Bundle Size — 236.82KiB (0%).0601e37(current) vs 045ca2f main#7170(baseline) Bundle metrics
|
| Current #7173 |
Baseline #7170 |
|
|---|---|---|
 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 #7173 |
Baseline #7170 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.06KiB |
91.06KiB |
Bundle analysis report Branch PupilTong:p/hw/support-global-bi... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#306 Bundle Size — 206.11KiB (0%).0601e37(current) vs 045ca2f main#303(baseline) Bundle metrics
|
| Current #306 |
Baseline #303 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
0% |
46.02% |
| 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 #306 |
Baseline #303 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
94.88KiB |
94.88KiB |
Bundle analysis report Branch PupilTong:p/hw/support-global-bi... Project dashboard
Generated by RelativeCI Documentation Report issue
React External#291 Bundle Size — 590.13KiB (0%).0601e37(current) vs 045ca2f main#288(baseline) Bundle metrics
|
| Current #291 |
Baseline #288 |
|
|---|---|---|
| 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/support-global-bi... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8748 Bundle Size — 743.45KiB (+1.81%).0601e37(current) vs 045ca2f main#8745(baseline) Bundle metrics
|
| Current #8748 |
Baseline #8745 |
|
|---|---|---|
 Initial JS |
43.82KiB(+0.44%) |
43.63KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
28.41% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
149 |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.96%(+0.78%) |
34.69% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 2 changes 2 regressions
| Current #8748 |
Baseline #8745 |
|
|---|---|---|
| Other |
398.15KiB (+3.27%) |
385.55KiB |
| JS |
343.13KiB (+0.18%) |
342.53KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch PupilTong:p/hw/support-global-bi... Project dashboard
Generated by RelativeCI Documentation Report issue
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/main_thread/client/element_apis/event_apis.rs`:
- Around line 165-171: The event lookup fails because the event_types vector
contains "global-bind" while handlers are stored as "global-bindevent" (after
React -> global-bindEvent -> lowercased in add_cross_thread_event); update the
event_types entry (or normalize keys) so get_events queries the same lowercased
stored key: change "global-bind" to "global-bindevent" in the event_types list
used by get_events (or apply the same transformation used in
add_cross_thread_event before calling get_framework_cross_thread_event_handler)
so get_framework_cross_thread_event_handler receives the exact key used when
handlers were registered.
🪄 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: ba258de5-f43b-43bf-83da-306a32211a1e
📒 Files selected for processing (1)
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
Outdated
Show resolved
Hide resolved
PupilTong
force-pushed
the
p/hw/support-global-bind
branch
from
a54089a to
806b09d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs (1)
344-389:⚠️ Potential issue | 🔴 CriticalDrop the
global_bind_eventsborrow before calling back into JS.The loop borrows
self.global_bind_eventsviaglobal_bind_idswhile callingpublish_event/publish_mts_event, which can synchronously re-enter wasm. If a handler registers or unregisters a global-bind listener during dispatch, it will cause aRefCellborrow panic.Snapshot the IDs into a
Vec<usize>first so the borrow is released before the first JS callback.🐛 Proposed fix
- if let Some(global_bind_ids) = self.global_bind_events.get(&event_name_lowercase) { - for unique_id in global_bind_ids { - let binding = match self.get_element_data_by_unique_id(*unique_id) { + let global_bind_ids: Vec<usize> = self + .global_bind_events + .get(&event_name_lowercase) + .map(|ids| ids.iter().copied().collect()) + .unwrap_or_default(); + + for unique_id in global_bind_ids { + let binding = match self.get_element_data_by_unique_id(unique_id) { Some(b) => b, None => continue, }; let current_target_element_data = binding.borrow(); let bind_handler = current_target_element_data .get_framework_cross_thread_event_handler(&event_name_lowercase, "global-bindevent"); if let Some(handler) = bind_handler { // ... rest of the code with *unique_id changed to unique_id } // ... - } } - }🤖 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 344 - 389, The loop currently holds a borrow of self.global_bind_events via global_bind_ids while invoking callbacks that can re-enter JS; to fix, snapshot the IDs into a Vec<usize> (e.g., let global_bind_ids_vec = self.global_bind_events.get(&event_name_lowercase).cloned().unwrap_or_default().into_iter().collect::<Vec<_>>();) and iterate over that Vec instead so the RefCell borrow is dropped before calling self.mts_binding.publish_event and self.mts_binding.publish_mts_event; ensure you still use get_element_data_by_unique_id, get_framework_cross_thread_event_handler, and get_framework_run_worklet_event_handler against the snapped IDs and preserve the existing parent_component_unique_id/component_id logic.
🤖 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/element_apis/event_apis.rs`:
- Around line 110-116: The add_run_worklet_event path currently adds unique_id
to self.global_bind_events when event_type == "global-bindevent" and
event_handler_identifier.is_some(), but it never removes it when
event_handler_identifier.is_none(); update add_run_worklet_event to mirror the
removal logic used in add_cross_thread_event: when event_type ==
"global-bindevent" and event_handler_identifier.is_none(), remove unique_id from
the set at self.global_bind_events.entry(event_name.clone()), and if the set
becomes empty, remove the key entirely so entries are cleaned up.
---
Duplicate comments:
In
`@packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs`:
- Around line 344-389: The loop currently holds a borrow of
self.global_bind_events via global_bind_ids while invoking callbacks that can
re-enter JS; to fix, snapshot the IDs into a Vec<usize> (e.g., let
global_bind_ids_vec =
self.global_bind_events.get(&event_name_lowercase).cloned().unwrap_or_default().into_iter().collect::<Vec<_>>();)
and iterate over that Vec instead so the RefCell borrow is dropped before
calling self.mts_binding.publish_event and self.mts_binding.publish_mts_event;
ensure you still use get_element_data_by_unique_id,
get_framework_cross_thread_event_handler, and
get_framework_run_worklet_event_handler against the snapped IDs and preserve the
existing parent_component_unique_id/component_id logic.
🪄 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: 81076f96-41ef-4bf7-9a7e-b687f133234b
📒 Files selected for processing (12)
.changeset/feat-global-bind-support.md.changeset/red-dragons-tickle.mdpackages/web-platform/web-core-e2e/tests/reactlynx.spec.tspackages/web-platform/web-core-e2e/tests/reactlynx/basic-global-bind/index.jsxpackages/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/constants.rspackages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rspackages/web-platform/web-core/src/main_thread/client/main_thread_context.rspackages/web-platform/web-core/tests/element-apis.spec.ts
✅ Files skipped from review due to trivial changes (5)
- .changeset/red-dragons-tickle.md
- .changeset/feat-global-bind-support.md
- packages/web-platform/web-core/binary/client_legacy/client_bg.wasm.d.ts
- packages/web-platform/web-core-e2e/tests/reactlynx.spec.ts
- packages/web-platform/web-core/tests/element-apis.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web-platform/web-core/binary/client/client_bg.wasm.d.ts
- packages/web-platform/web-core/binary/client/client.d.ts
- packages/web-platform/web-core/binary/client_legacy/client.d.ts
- packages/web-platform/web-core/src/main_thread/client/main_thread_context.rs
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
Outdated
Show resolved
Hide resolved
…mprove readability and safety
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs (1)
154-156: Clean up emptyglobal_bind_eventsbuckets after removal.After removing
unique_id, empty sets are left in the map. It’s harmless functionally, but cleaning empty keys avoids unnecessary map growth/lookups over time.♻️ Suggested cleanup
- if let Some(ids) = self.global_bind_events.get_mut(event_name) { - ids.remove(&unique_id); - } + let should_remove_key = if let Some(ids) = self.global_bind_events.get_mut(event_name) { + ids.remove(&unique_id); + ids.is_empty() + } else { + false + }; + if should_remove_key { + self.global_bind_events.remove(event_name); + }🤖 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 154 - 156, After removing unique_id from the set in self.global_bind_events (the block that checks if let Some(ids) = self.global_bind_events.get_mut(event_name)), also check whether ids.is_empty() and if so remove the key from the map (e.g., call self.global_bind_events.remove(event_name) or use the entry API) so empty HashSet buckets aren’t left behind; update the cleanup code in the same scope that handles event_name and unique_id.
🤖 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/element_apis/event_apis.rs`:
- Around line 334-401: The PR added dispatch_global_bind_event and
global_bind_events bookkeeping but lacks JS-side tests; add coverage in
tests/element-apis.spec.ts to register, dispatch and unregister global-bind
handlers for both cross-thread and run-worklet paths. Specifically, write tests
that: (1) register a global-bind handler from the JS side, assert that
dispatch_global_bind_event (triggered via the public dispatch API) invokes the
handler that maps to the Rust function dispatch_global_bind_event and that the
handler path uses get_framework_cross_thread_event_handler and calls
publish_event with correct args; (2) register a run-worklet global-bind handler,
dispatch and assert publish_mts_event is invoked (the run-worklet path uses
get_framework_run_worklet_event_handler and publish_mts_event); and (3)
unregister handlers and assert subsequent dispatches do not call either
publish_event or publish_mts_event. Reference the Rust symbols
dispatch_global_bind_event, global_bind_events,
get_framework_cross_thread_event_handler,
get_framework_run_worklet_event_handler, publish_event and publish_mts_event so
the tests exercise both cross-thread and run-worklet global-bind flows.
---
Nitpick comments:
In
`@packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs`:
- Around line 154-156: After removing unique_id from the set in
self.global_bind_events (the block that checks if let Some(ids) =
self.global_bind_events.get_mut(event_name)), also check whether ids.is_empty()
and if so remove the key from the map (e.g., call
self.global_bind_events.remove(event_name) or use the entry API) so empty
HashSet buckets aren’t left behind; update the cleanup code in the same scope
that handles event_name and unique_id.
🪄 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: 863c86c7-0571-4d20-a4bb-b088bdd75040
📒 Files selected for processing (1)
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs
Show resolved
Hide resolved
Sherry-hue
approved these changes
Apr 10, 2026
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.
… communication
Summary by CodeRabbit
New Features
Tests
Documentation
Checklist