No actionable comments were generated in the recent review. 🎉

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 8eadd574-6118-400e-997a-f3d638e7610e

Reviewing files that changed from the base of the PR and between 6221ca8 and b7f17b5.

• NativeScript/runtime/Messaging.cpp
• NativeScript/runtime/Worker.mm
• NativeScript/runtime/js/broadcast-channel.js
• NativeScript/runtime/js/message-channel.js
• NativeScript/runtime/js/message-event.js
• NativeScript/runtime/js/node-worker-threads.js
• NativeScript/runtime/js/worker-events.js
• TestRunner/app/tests/MessagingTests.js
• TestRunner/app/tests/messaging/describeDataWorker.js
• TestRunner/app/tests/messaging/parentPortDescribeWorker.js

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

Walkthrough

The change adds a message-event factory that preserves undefined payloads, updates messaging relays to use it, normalizes deserialization-error data to null, and adds cross-runtime regression tests for falsy values.

Changes

Messaging data preservation

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to b7f17

The messaging update preserves delivered undefined payloads while retaining constructor defaults and explicit messageerror null handling. No merge-blocking risk is identified.

❌ Failed checks (1 warning)

Explanation

Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 9 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

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.

• X
• Mastodon
• Reddit
• LinkedIn

A rabbit carried messages light,
Undefined stayed undefined in flight.
Null marked errors where values fell,
Ports and workers rang the bell.
Tests watched each payload hop,
And every tiny value did not stop.

Comment @coderabbitai help to get the list of available commands.

@adrian-niculescu thank you! If you can, please open the PR stacked against the android PRs so we can fix it there too

@edusperoni done: NativeScript/android#2063 is stacked on NativeScript/android#2043 and ports this fix together with #489 (the worker messageerror carrying the deserialization error).

While working on it I found a few more problems in the new messaging code. Each has its own Android PR stacked on NativeScript/android#2043, with the iOS fix next to it:

• fix(runtime): keep clone-only message reads read-only, and see an uncloneable brand created mid-clone android#2064 and fix(runtime): keep clone-only message reads read-only, and see an uncloneable brand created mid-clone #490: a clone-only message read writes shared state, and an object marked uncloneable during the clone is still cloned.
• fix(worker): route worker_threads errors like the web surface, and fire once listeners once android#2065 and fix(worker): route worker_threads errors like the web surface, and fire once listeners once #491: a throwing parentPort listener skips the worker error chain, an error handled by worker.on("error") is also reported as unhandled, and a once listener can fire twice.
• fix(worker): throw when an environment-data key cannot be converted to a string android#2066 and fix(worker): throw when an environment-data key cannot be converted to a string #492: an environment-data key whose toString() throws falls back to the "" entry.

There is also an open question on environment-data key identity, on the Android PR: NativeScript/android#2043 (comment)