fix: prevent invalid uniqueId of -1 from being added to bubblePath in…#2493
Conversation
🦋 Changeset detectedLatest commit: b361331 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 introduces a patch-level fix for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web-platform/web-core/tests/element-apis.spec.ts (1)
71-79: Assert the valid ancestor remains inbubblePath.This test would still pass if
#commonEventHandlerreturned an empty path. Please also assert thatnode1’s valid unique ID is preserved.Proposed test tightening
expect(mtsBinding.wasmContext!.common_event_handler).toHaveBeenCalled(); const calls = (mtsBinding.wasmContext!.common_event_handler as any).mock.calls; - const bubblePath = calls[0][1] as Uint32Array; + const bubblePath = Array.from(calls[0][1] as Uint32Array); + const invalidUniqueIdInUint32Array = 0xffffffff; - // Check that -1 (4294967295 in Uint32Array) is NOT in the bubblePath - for (let i = 0; i < bubblePath.length; i++) { - expect(bubblePath[i] !== 4294967295).toBe(true); - } + expect(bubblePath).toEqual([mtsGlobalThis.__GetElementUniqueID(node1)]); + expect(bubblePath).not.toContain(invalidUniqueIdInUint32Array);🤖 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 71 - 79, The test currently only asserts that -1 isn't present in the bubblePath returned from mtsBinding.wasmContext!.common_event_handler but would still pass if the path were empty; update the assertion to also verify that node1's valid unique ID is included in the bubblePath. After retrieving bubblePath (from (mtsBinding.wasmContext!.common_event_handler as any).mock.calls[0][1]), compute or read node1's unique ID (the same identifier used when creating node1) and assert that bubblePath contains that ID (e.g., convert bubblePath to a normal array and expect it to contain node1's ID) so the test ensures the ancestor is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web-platform/web-core/tests/element-apis.spec.ts`:
- Around line 71-79: The test currently only asserts that -1 isn't present in
the bubblePath returned from mtsBinding.wasmContext!.common_event_handler but
would still pass if the path were empty; update the assertion to also verify
that node1's valid unique ID is included in the bubblePath. After retrieving
bubblePath (from (mtsBinding.wasmContext!.common_event_handler as
any).mock.calls[0][1]), compute or read node1's unique ID (the same identifier
used when creating node1) and assert that bubblePath contains that ID (e.g.,
convert bubblePath to a normal array and expect it to contain node1's ID) so the
test ensures the ancestor is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48258bf1-107a-4d4e-ad95-5046f713296a
📒 Files selected for processing (3)
.changeset/fix-event-handler-unique-id.mdpackages/web-platform/web-core/tests/element-apis.spec.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/WASMJSBinding.ts
Merging this PR will degrade performance by 16.16%
Performance Changes
Comparing Footnotes
|
React External#574 Bundle Size — 583.27KiB (0%).b361331(current) vs 02be891 main#571(baseline) Bundle metrics
|
| Current #574 |
Baseline #571 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
17 |
17 |
|
5 |
5 |
|
8.59% |
8.59% |
|
0 |
0 |
|
0 |
0 |
Bundle analysis report Branch PupilTong:p/hw/fix-event-with-in... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9031 Bundle Size — 898.16KiB (~+0.01%).b361331(current) vs 02be891 main#9028(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch PupilTong:p/hw/fix-event-with-in... Project dashboard Generated by RelativeCI Documentation Report issue |
React Example#7456 Bundle Size — 224.41KiB (0%).b361331(current) vs 02be891 main#7453(baseline) Bundle metrics
|
| Current #7456 |
Baseline #7453 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
179 |
179 |
|
69 |
69 |
|
44.5% |
44.5% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #7456 |
Baseline #7453 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
78.65KiB |
78.65KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-event-with-in... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#589 Bundle Size — 195.57KiB (0%).b361331(current) vs 02be891 main#586(baseline) Bundle metrics
|
| Current #589 |
Baseline #586 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
173 |
173 |
|
66 |
66 |
|
43.99% |
43.99% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #589 |
Baseline #586 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
84.34KiB |
84.34KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-event-with-in... Project dashboard
Generated by RelativeCI Documentation Report issue
… WASMJSBinding
Summary by CodeRabbit
Checklist