Codex/ios cjk input fix#55
Conversation
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a diff-driven iOS IME input pipeline in a new utils module, integrates an iOS-only text-controller listener and state into RemotePage with keyboard-event gating, and adds extensive unit tests for multi-IME composing and sentinel edge cases. ChangesiOS soft-keyboard input handling
Sequence DiagramsequenceDiagram
participant TextCtrl as iOS Text Controller
participant Listener as _handleIOSTextControllerChanged
participant Diff as diffIOSSoftKeyboardInput
participant Handler as _handleIOSSoftKeyboardInput
participant Input as inputModel
TextCtrl->>Listener: onChange(text, composingRange, selection)
Listener->>Diff: diff(previousValue, currentValue, composingRange, previousControllerState)
Diff-->>Listener: IOSSoftKeyboardInputResult(nextValue, nextComposingValue, actions)
Listener->>Handler: forward diff result
Handler->>Input: apply actions (inputKey / inputChar / sessionInputString)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@flutter/lib/mobile/utils/ios_soft_keyboard_input.dart`:
- Around line 716-727: The recursive function _isPinyinSyllableSequencePrefix
can explode on pathological inputs; add protection by memoizing results (use a
Map<String,bool> keyed by token) or by adding a depth/length cutoff before
recursing to avoid exponential exploration. Update
_isPinyinSyllableSequencePrefix to check the memo cache (and store results)
and/or bail out when token length or recursion depth exceeds a safe threshold;
keep using _isCompletePinyinSyllable and _isPinyinSyllablePrefixWithFinal for
boundary checks so behavior is preserved for normal short IME inputs.
In `@flutter/test/ios_soft_keyboard_input_test.dart`:
- Around line 1316-1341: Extract the hardcoded sentinel prefix length 3 into a
named constant (e.g., const int kSentinelPrefixLength = 3) and use that constant
inside the _IOSSoftKeyboardInputSession.diff method (replace
sentinelPrefixLength: 3) and in the other test sites that currently use the
literal 3; update any related comments to mention the constant so the
relationship to the '111' sentinel prefix is explicit and maintainable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29b034b4-c1fd-4cdc-84e2-dbc49ebdc601
📒 Files selected for processing (3)
flutter/lib/mobile/pages/remote_page.dartflutter/lib/mobile/utils/ios_soft_keyboard_input.dartflutter/test/ios_soft_keyboard_input_test.dart
Signed-off-by: fufesou <linlong1266@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flutter/lib/mobile/utils/ios_soft_keyboard_input.dart (1)
657-663: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse explicit size calculation for supplementary character handling.
_commonPrefixLengthByRunes(line 795) usesRuneIterator.currentSizeto correctly handle surrogate pairs, but this function relies onString.fromCharCode(rune).length. While this works in practice, using an explicit calculation is clearer and consistent with the approach elsewhere in this file.♻️ Suggested change for consistency
Iterable<int> _runeBoundaryOffsets(String value) sync* { var offset = 0; for (final rune in value.runes) { - offset += String.fromCharCode(rune).length; + offset += rune <= 0xFFFF ? 1 : 2; if (offset < value.length) yield offset; } }🤖 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 `@flutter/lib/mobile/utils/ios_soft_keyboard_input.dart` around lines 657 - 663, Replace the implicit length calculation using String.fromCharCode(rune).length in _runeBoundaryOffsets with an explicit RuneIterator.currentSize-based calculation to correctly handle surrogate pairs and match _commonPrefixLengthByRunes; iterate with a RuneIterator over value (calling moveNext()), add iterator.currentSize to offset for each rune, and yield offset when offset < value.length.
🤖 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.
Outside diff comments:
In `@flutter/lib/mobile/utils/ios_soft_keyboard_input.dart`:
- Around line 657-663: Replace the implicit length calculation using
String.fromCharCode(rune).length in _runeBoundaryOffsets with an explicit
RuneIterator.currentSize-based calculation to correctly handle surrogate pairs
and match _commonPrefixLengthByRunes; iterate with a RuneIterator over value
(calling moveNext()), add iterator.currentSize to offset for each rune, and
yield offset when offset < value.length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb61f699-0d0d-4395-80c0-bfc9002e653d
📒 Files selected for processing (3)
flutter/lib/mobile/pages/remote_page.dartflutter/lib/mobile/utils/ios_soft_keyboard_input.dartflutter/test/ios_soft_keyboard_input_test.dart
💤 Files with no reviewable changes (2)
- flutter/test/ios_soft_keyboard_input_test.dart
- flutter/lib/mobile/pages/remote_page.dart
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: fufesou <linlong1266@gmail.com>
Fixes iOS soft keyboard handling for CJK IMEs in remote sessions.
The previous iOS text diff logic treated any active composing range as text that should not be sent. Some iOS IMEs keep a composing range briefly after the user confirms a candidate, so committed Chinese text could be held until another key, such as space, causes another text change.
Changes
ios_soft_keyboard_input.dart.sessionInputString, including Japanese kanji/kana and Korean hangul once composition is committed.Testing
flutter test --no-pub test/ios_soft_keyboard_input_test.dart test/input_modifier_utils_test.dartKnown limitation:
Known issues
Summary by CodeRabbit
Bug Fixes
Refactor
Tests