fix: prevent panic in event dispatching when element data is missing …#2508
Conversation
…and add regression tests
🦋 Changeset detectedLatest commit: c5f070c 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 |
📝 WalkthroughWalkthroughThis PR fixes a panic in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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/element_apis/event_apis.rs (1)
167-179:⚠️ Potential issue | 🔴 CriticalHandle missing element data gracefully in
get_eventandget_eventslike other event APIs do.Lines 167 and 178 still use
.unwrap()which will panic if element data is missing. The recent fix (commit c5f070c) addressed this panic risk indispatch_event_by_path(returningfalse) andcommon_event_handler(usingmatchwith graceful handling). However,get_eventandget_eventswere excluded from that fix.Since these methods are called from JS via
__GetEventand__GetEventswith uniqueIds extracted from DOM elements, stale IDs from removed elements can cause panics. Other similar public methods incomponent_apis.rsanddataset_apis.rsreturnResult<T, JsError>instead. Consider updatingget_eventandget_eventsto follow the same pattern—either returningResult<T, JsError>or using graceful error handling consistent withdispatch_event_by_path.🤖 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 167 - 179, get_event and get_events currently call self.get_element_data_by_unique_id(...).unwrap() which can panic for stale/missing elements; change them to handle missing element data gracefully like dispatch_event_by_path and common_event_handler: replace the unwrap() usage in get_event and get_events with a match or early-return that converts the missing element case into a JS-friendly error (e.g., return Result<JsValue, JsError> for get_event and Result<Vec<EventInfo>, JsError> for get_events) or return a sensible default/error value, ensuring you use the same error creation/JsError helper used elsewhere so JS callers (__GetEvent/__GetEvents) receive a non-panicking failure instead of a panic.
🧹 Nitpick comments (1)
packages/web-platform/web-core/tests/element-apis.spec.ts (1)
82-117: Deadeventlocals in both new tests.
eventis constructed viadocument.createEvent(...)/initEvent(...)but never used — the tests passeventObjecttocommon_event_handler. Dropping the unused setup keeps the regression tests focused.♻️ Proposed cleanup
test('#commonEventHandler should not crash on invalid uniqueId', () => { - // We send an event with a bubblePath containing a non-existent uniqueId const invalidUniqueId = 999999; - const event = document.createEvent('Event') as any; - event.initEvent('touchstart', true, true); - - // Create cross thread event manually for the test const eventObject = { type: 'touchstart', detail: {} } as any; ... test('#commonEventHandler should not crash on empty path', () => { - // We send an event with a bubblePath containing a non-existent uniqueId - const event = document.createEvent('Event') as any; - event.initEvent('touchstart', true, true); - - // Create cross thread event manually for the test const eventObject = { type: 'touchstart', detail: {} } as any;Also consider strengthening the assertion beyond
not.toThrow()— e.g. spy onmtsBinding.publishEventand assert it is not called for the invalid/empty cases, so a future regression that silently dispatches to the wrong element would still fail.🤖 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 82 - 117, Remove the dead local `event` setup in both tests "#commonEventHandler should not crash on invalid uniqueId" and "#commonEventHandler should not crash on empty path" (the `document.createEvent(...)` and `initEvent(...)` lines) since the tests call mtsBinding.wasmContext!.common_event_handler with `eventObject`; then optionally strengthen the assertions by spying on `mtsBinding.publishEvent` (or equivalent) and asserting it was not called for the invalidUniqueId and empty Uint32Array cases to ensure no silent dispatch happens.
🤖 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 212-218: The code currently uses
bubble_unique_id_path.first().cloned().unwrap_or_default() which maps an empty
path to target_unique_id = 0 and then looks up get_element_data_by_unique_id(0);
change this to explicitly handle an empty bubble_unique_id_path by returning
false (or early exit) before computing target_unique_id so you don't
accidentally resolve to a real element with id 0; update the logic around
bubble_unique_id_path, target_unique_id, and the match on
get_element_data_by_unique_id to check bubble_unique_id_path.is_empty() first
and bail out before calling get_element_data_by_unique_id.
---
Outside diff comments:
In
`@packages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rs`:
- Around line 167-179: get_event and get_events currently call
self.get_element_data_by_unique_id(...).unwrap() which can panic for
stale/missing elements; change them to handle missing element data gracefully
like dispatch_event_by_path and common_event_handler: replace the unwrap() usage
in get_event and get_events with a match or early-return that converts the
missing element case into a JS-friendly error (e.g., return Result<JsValue,
JsError> for get_event and Result<Vec<EventInfo>, JsError> for get_events) or
return a sensible default/error value, ensuring you use the same error
creation/JsError helper used elsewhere so JS callers (__GetEvent/__GetEvents)
receive a non-panicking failure instead of a panic.
---
Nitpick comments:
In `@packages/web-platform/web-core/tests/element-apis.spec.ts`:
- Around line 82-117: Remove the dead local `event` setup in both tests
"#commonEventHandler should not crash on invalid uniqueId" and
"#commonEventHandler should not crash on empty path" (the
`document.createEvent(...)` and `initEvent(...)` lines) since the tests call
mtsBinding.wasmContext!.common_event_handler with `eventObject`; then optionally
strengthen the assertions by spying on `mtsBinding.publishEvent` (or equivalent)
and asserting it was not called for the invalidUniqueId and empty Uint32Array
cases to ensure no silent dispatch happens.
🪄 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: 5301e0a0-d179-4fbb-a4fe-79d382e779c1
📒 Files selected for processing (3)
.changeset/fix-event-apis-panic.mdpackages/web-platform/web-core/src/main_thread/client/element_apis/event_apis.rspackages/web-platform/web-core/tests/element-apis.spec.ts
Merging this PR will improve performance by 23.84%
Performance Changes
Comparing 
Footnotes
|
React External#633 Bundle Size — 674.83KiB (0%).c5f070c(current) vs ea5e30e main#622(baseline) Bundle metrics
|
| Current #633 |
Baseline #622 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
0% |
98.35% |
| 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/fix-empty-bubble-... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7515 Bundle Size — 224.41KiB (0%).c5f070c(current) vs ea5e30e main#7504(baseline) Bundle metrics
|
| Current #7515 |
Baseline #7504 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.51% |
44.51% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7515 |
Baseline #7504 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
78.65KiB |
78.65KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-empty-bubble-... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#647 Bundle Size — 195.57KiB (0%).c5f070c(current) vs ea5e30e main#637(baseline) Bundle metrics
|
| Current #647 |
Baseline #637 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44% |
44% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #647 |
Baseline #637 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
84.34KiB |
84.34KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-empty-bubble-... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9087 Bundle Size — 899.79KiB (+0.12%).c5f070c(current) vs ea5e30e main#9077(baseline) Bundle metrics
|
| Current #9087 |
Baseline #9077 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
43.7% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229 |
229 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.2%(-0.07%) |
27.22% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 2 changes 
1 regression 
1 improvement
| Current #9087 |
Baseline #9077 |
|
|---|---|---|
| JS |
495.65KiB (+0.24%) |
494.47KiB |
| Other |
401.92KiB (-0.02%) |
402.02KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-empty-bubble-... Project dashboard
Generated by RelativeCI Documentation Report issue
2 participants
…and add regression tests
Summary by CodeRabbit
Checklist