Walkthrough

Replaces UTF-8 intermediate conversions in the V8-to-Objective-C string bridge with direct UTF-16 access. ToNSString and ToUtf16String now use v8::String::Value to read the UTF-16 buffer, propagating the change to ArgConverter and DictionaryAdapter call sites. Three new tests verify lone surrogates and embedded NULs are preserved.

Changes

UTF-16 Bridging Conversion

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 Hop hop through the bridge of code,
No UTF-8 detour on this road!
Lone surrogates? NUL bytes too?
UTF-16 carries them through!
The rabbit reads bits pure and true 🎉

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

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

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

@coderabbitai thanks, but I'm going to skip this suggestion, it would actually break the test.

Completing the round trip with ns.toString() would fail: the reverse NSString to JS direction still goes through UTF-8 (ToV8String(NSString*) uses [value UTF8String]), so it turns the lone surrogate back into U+FFFD. expect(ns.toString()).toBe("\uD834") and charCodeAt(0) === 0xD834 would both fail. I confirmed that by running the TestRunner suite locally.

That reverse path is intentionally not touched for surrogates in this PR. The reverse-direction edits here only fix NUL truncation (passing an explicit byte length instead of folding through a C string), not surrogate fidelity, so there is no faithful NSString to JS path to round-trip a lone surrogate through. That is exactly why the test observes the NSString with a numeric native query (lengthOfBytesUsingEncoding: is 0 for a faithfully kept lone surrogate, 3 once it has become U+FFFD) instead of converting back to a JS string.

The bidirectional NUL case is already covered by the existing Marshals NSString with null character test in NSStringTests.js.

@adrian-niculescu, thanks for the thorough explanation — that makes perfect sense. The ToV8String(NSString*) path still uses [value UTF8String] which mangles lone surrogates back to U+FFFD, so a round-trip assertion via ns.toString() would indeed be a false expectation at this stage. Using lengthOfBytesUsingEncoding(NSUTF8StringEncoding) === 0 as a numeric native proxy is a clean way to confirm the surrogate was kept intact without attempting the broken reverse path. I'll withdraw my suggestion.

(╯°□°）╯ 🐇 ← gently backing away from that suggestion

Learnt from: adrian-niculescu
URL: https://github.com/NativeScript/ios/pull/392

Timestamp: 2026-06-13T22:37:15.839Z
Learning: In NativeScript iOS (NativeScript/ios), the `ToV8String(NSString*)` function in `Helpers.mm` still goes through `[value UTF8String]`, which means lone (unpaired) UTF-16 surrogates get corrupted to U+FFFD on the NSString→JS direction. PR `#392` intentionally fixes only the JS→NSString direction (surrogate fidelity) and the NUL-truncation issue on both directions, but does NOT fix NSString→JS surrogate fidelity. Therefore, a test for a lone surrogate round-trip through NSString cannot assert `ns.toString() === "\uD834"` — it uses `ns.lengthOfBytesUsingEncoding(NSUTF8StringEncoding) === 0` as a native numeric proxy instead (faithfully stored lone surrogate yields 0 UTF-8 bytes; a corrupted U+FFFD would yield 3).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧹 Nitpick comments (1)

TestRunner/app/tests/ApiTests.js (1)> 15-31: ⚡ Quick win

Consider adding a lone low surrogate test case for completeness.
The current test verifies a lone high surrogate (U+D834, range 0xD800-0xDBFF). Adding a similar test for a lone low surrogate (e.g., "\uDC00", range 0xDC00-0xDFFF) would strengthen coverage of the UTF-16 fidelity fix, since both surrogate ranges should be preserved but have different bit patterns.

📝 Optional additional test case

it("preserves a lone low surrogate when bridging a JS string to NSString", function () {
    var ns = NSString.stringWithString("\uDC00");
    expect(ns.length).toBe(1);
    
    var buffer = interop.alloc(interop.sizeof(interop.types.uint16));
    ns.getCharactersRange(buffer, NSMakeRange(0, 1));
    var codeUnit = new interop.Reference(interop.types.uint16, buffer).value;
    interop.free(buffer);
    
    expect(codeUnit).toBe(0xDC00);
});

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@TestRunner/app/tests/ApiTests.js` around lines 15 - 31, The test suite
currently only covers high surrogates (U+D834) but lacks coverage for low
surrogates in the UTF-16 bridging behavior. Add a new test case after the
existing "preserves a lone surrogate when bridging a JS string to NSString" test
that follows the same pattern but uses a low surrogate code unit (U+DC00 instead
of U+D834) to verify that lone low surrogates are also preserved correctly when
bridging JS strings to NSString. The new test should verify that the bridged
NSString preserves the low surrogate value as 0xDC00.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@TestRunner/app/tests/ApiTests.js`:
- Around line 15-31: The test suite currently only covers high surrogates
(U+D834) but lacks coverage for low surrogates in the UTF-16 bridging behavior.
Add a new test case after the existing "preserves a lone surrogate when bridging
a JS string to NSString" test that follows the same pattern but uses a low
surrogate code unit (U+DC00 instead of U+D834) to verify that lone low
surrogates are also preserved correctly when bridging JS strings to NSString.
The new test should verify that the bridged NSString preserves the low surrogate
value as 0xDC00.

ℹ️ Review info

@coderabbitai, re the low surrogate nitpick: added a lone low surrogate case (U+DC00) next to the high surrogate one in 8cc59bf, and renamed the original so the pair reads clearly. Both read the bridged code unit back as a number via getCharacters:range: instead of a UTF-8 conversion, since converting a lone surrogate to UTF-8 hangs the test runner on CI's iOS 17.5 simulator.

@adrian-niculescu, great — both high (\uD834) and low (\uDC00) surrogate cases are now covered, and the renaming makes the pair read cleanly together. The getCharacters:range: / numeric approach is exactly right given the CI constraint you surfaced.

Good to know about the iOS 17.5 simulator hang on UTF-8 lone surrogate conversion — I'll record that for future reference.

Learnt from: adrian-niculescu
URL: https://github.com/NativeScript/ios/pull/392

Timestamp: 2026-06-13T23:57:12.035Z
Learning: In NativeScript iOS (NativeScript/ios), attempting to convert a lone (unpaired) UTF-16 surrogate to UTF-8 (e.g., via `[NSString UTF8String]` or `lengthOfBytesUsingEncoding:NSUTF8StringEncoding` in a way that triggers actual string conversion) can **hang the test runner** on the iOS 17.5 simulator in CI. Tests that verify lone surrogate preservation (high surrogates U+D800–U+DBFF or low surrogates U+DC00–U+DFFF) must read the code unit back as a numeric value via `getCharacters:range:` into a `uint16` buffer rather than attempting any UTF-8 string conversion. This was discovered while implementing tests for PR `#392`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

I'm curious, should we not just use utf16 everywhere we can then? Also seems like we're doing extra steps there: first convert to std::utf16string by passing a buffer + length and then pass the copied buffer + length to NSString. I feel we could have a tns::ToNSString which just reads the utf16 buffer directly.

I believe most of the utf8 usages we have in this repo come from the misconception that v8 wasn't using utf16 internally or that the utf8 string conversion would be faster somehow (I know I myself already fixed a bunch of utf8 issues with null character in the middle of the string)

I'm curious, should we not just use utf16 everywhere we can then? Also seems like we're doing extra steps there: first convert to std::utf16string by passing a buffer + length and then pass the copied buffer + length to NSString. I feel we could have a tns::ToNSString which just reads the utf16 buffer directly.

I believe most of the utf8 usages we have in this repo come from the misconception that v8 wasn't using utf16 internally or that the utf8 string conversion would be faster somehow (I know I myself already fixed a bunch of utf8 issues with null character in the middle of the string)

Indeed, V8 strings are UTF-16 the whole way down, so these UTF-8 round-trips are just leftover from the old assumption. Same root cause as the embedded-NUL cases you mentioned.

And you're right that we don't need a new helper. tns::ToNSString(isolate, value) already exists, I'd just stopped calling it because its body still went through String::Utf8Value. I made it read the two-byte buffer and hand it straight to stringWithCharacters:length:, then pointed the call sites back at it. That drops the intermediate std::u16string copy and keeps the conversion in one place. Pushed.

On utf16 everywhere: agreed. The spot this doesn't close yet is the reverse direction. ToV8String(NSString*) still goes through [value UTF8String], so it's NUL-safe but not surrogate-safe, and there's already a commented-out NewFromTwoByte path sitting in there for it. Happy to finish that so the bridge is symmetric, reading with getCharacters:range: rather than the commented getBytes:...NSUTF16StringEncoding (which can write a BOM). A wider UTF-8 sweep across the repo is probably its own PR.

Thanks for taking a look at this!