Limitation correctness dx#8
Conversation
|
Important Review skippedToo many files! This PR contains 156 files, which is 6 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (156)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Nitro ecosystem to version 0.5.1, introducing support for untyped hybrid object references (AnyNativeObject), heterogeneous maps (NitroAnyMap), positional records (@NitroTuple), custom FFI codecs (@NitroCustomType), and non-contiguous enums. It also improves nullable primitive transport using packed C-ABI structs, adds advanced stream backpressure modes, and introduces a C++ implementation starter generator. The code review identified critical issues with the newly added String batch streams, where checking for 'String' directly fails to match nullable 'String?' in the Dart, Kotlin, and JNI emitters, and the Swift emitter lacks the actual implementation for String batches entirely. Additionally, a redundant conditional block was noted in the C++ implementation generator.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final batchBase = stream.itemType.name.replaceFirst('?', ''); | ||
| final isBatchRecord = mapper.recordNames.contains(batchBase); | ||
| final isBatchVariant = mapper.variantNames.contains(batchBase); | ||
| if (stream.itemType.name == 'String') { |
There was a problem hiding this comment.
Checking stream.itemType.name == 'String' will evaluate to false if the stream item type is nullable (i.e., String?), causing nullable String batch streams to fall through to the numeric batch path and fail to compile. Use stream.itemType.baseName == 'String' instead to correctly support both nullable and non-nullable String batch streams.
| if (stream.itemType.name == 'String') { | |
| if (stream.itemType.baseName == 'String') { |
|
|
||
| if (stream.isBatch) { | ||
| _emitBatchCollect(writer, stream); | ||
| if (stream.isBatch && stream.itemType.name == 'String') { |
There was a problem hiding this comment.
Checking stream.itemType.name == 'String' will evaluate to false if the stream item type is nullable (i.e., String?), causing nullable String batch streams to fall through to the numeric batch path and fail to compile. Use stream.itemType.baseName == 'String' instead.
| if (stream.isBatch && stream.itemType.name == 'String') { | |
| if (stream.isBatch && stream.itemType.baseName == 'String') { |
| @@ -787,115 +962,259 @@ void _emitJniStreamBridges( | |||
| writer.line('}'); | |||
| writer.blankLine(); | |||
There was a problem hiding this comment.
Checking stream.itemType.name == 'String' will evaluate to false if the stream item type is nullable (i.e., String?), causing nullable String batch streams to fall through to the numeric batch path and fail to compile. Use stream.itemType.baseName == 'String' instead.
if (stream.itemType.baseName == 'String') {| if (stream.isBatch) { | ||
| // Batch mode: native emits Int64List batches [count, item0, item1, ...] | ||
| // Dart unpacks each batch into individual stream items via asyncExpand. | ||
| if (stream.isBatch && itemType == 'String') { |
There was a problem hiding this comment.
Checking itemType == 'String' will evaluate to false if the stream item type is nullable (i.e., String?), causing nullable String batch streams to fall through to the numeric batch path and fail to compile. Use stream.itemType.baseName == 'String' instead.
| if (stream.isBatch && itemType == 'String') { | |
| if (stream.isBatch && stream.itemType.baseName == 'String') { |
| final isVariantBatch = spec.isVariantName(itemBase); | ||
|
|
There was a problem hiding this comment.
Although String has been added as a supported batch stream item type in spec_validator.dart (E005 relaxation), swift_stream_emitter.dart does not implement any handling for String or String? batches. This will cause a Swift compilation error because it falls through to the numeric/enum batch path and attempts to append a String to a [Int64] buffer (_buf.append(item)). Please implement String batch support in Swift as well.
| if (spec.streams.isNotEmpty) { | ||
| w.writeln('#include <stdexcept>'); | ||
| } else { | ||
| w.writeln('#include <stdexcept>'); | ||
| } |
There was a problem hiding this comment.
This conditional block is redundant because both the if and else branches execute the exact same statement (w.writeln('#include <stdexcept>');). You can simplify this by removing the conditional check entirely.
w.writeln('#include <stdexcept>');1 participant
No description provided.