🐛 fix: handle non-ASCII character input - #23
Conversation
📝 WalkthroughWalkthroughUnicodeの表示幅API、特殊キーコード、コードポイント単位の編集処理、端末セル幅による描画処理を追加・変更した。ANSIシーケンス、ワイド文字、絵文字、サロゲートペアのテストも追加した。 ChangesUnicode端末動作
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer as render.clj
participant NativeBridge as native.clj
participant UnicodeCore as ecro_core
Renderer->>NativeBridge: テキストの表示幅を要求
NativeBridge->>UnicodeCore: UTF-8バイト列を渡す
UnicodeCore-->>NativeBridge: 端末セル幅を返す
NativeBridge-->>Renderer: 表示幅を返す
Renderer->>Renderer: ANSIと書記素境界を維持して描画幅を調整
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee4e0a905
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (buffer/insert-text (:current-buffer editor-state) | ||
| (key-code->string key-code))) |
There was a problem hiding this comment.
When the terminal supplies a valid code point above U+FFFF, such as an emoji, key-code->string produces a two-unit UTF-16 string and insert-text leaves the point after both units. Existing cursor and deletion operations in ecro.buffer advance or remove only one UTF-16 unit, so pressing Backspace immediately afterward leaves an unpaired surrogate and corrupts the buffer contents. Supplementary input must either be rejected or made atomic across cursor, deletion, and region operations.
Useful? React with 👍 / 👎.
| (defn- function-key-code? | ||
| [key-code] | ||
| (<= 2000 key-code 2255)) |
There was a problem hiding this comment.
For characters whose code points fall from 2000 through 2255, this predicate silently treats ordinary input as a function key and discards it; for example, U+0800 (Samaritan Letter Alaf) arrives as integer 2048. The Rust adapter's encode_key_event uses the same integer space for KeyCode::Char(c) and KeyCode::F(n), so these cases cannot be distinguished here; function keys need disjoint sentinel values above U+10FFFF or an explicit event-kind field.
Useful? React with 👍 / 👎.
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03269abb56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (if (< point (count text)) | ||
| (let [deleted-char (get text point) | ||
| new-buf (assoc buf :text (str (subs text 0 point) (subs text (inc point))))] | ||
| (let [end (next-character-offset text point) |
There was a problem hiding this comment.
When a supplementary character starts a destination line, C-n/C-p can still put point between its UTF-16 units: for example, moving from column 1 in "a\n😀x" computes the destination as the second line's start plus 1. At that point this call treats the low surrogate as a standalone character, so forward deletion removes only that unit and corrupts the buffer. Fresh evidence for the previously reported supplementary-character issue is the unchanged UTF-16 column arithmetic in core/next-line and core/previous-line, which the new boundary helpers do not normalize; vertical movement must also select a code-point boundary.
Useful? React with 👍 / 👎.
|
|
||
| (defn- wide-code-point? | ||
| [code-point] | ||
| (or (Character/isEmojiPresentation code-point) |
There was a problem hiding this comment.
This checks each code point's default presentation independently, so an emoji-style sequence such as ❤️ is measured as one cell: U+2764 has text presentation by default, while U+FE0F is assigned zero width. Terminals render the selected emoji presentation as two cells, causing the cursor to be placed one cell left and causing truncation or padding to use the wrong boundary. Width calculation needs to account for variation selectors or operate on emoji grapheme sequences.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@rust/ecro-core/src/lib.rs`:
- Around line 152-204: Add Rust tests for ecro_display_width and
ecro_prefix_utf16_length_for_width covering Japanese display widths,
supplementary Unicode characters, grapheme-aligned truncation, invalid UTF-8,
negative lengths, and null pointers, including each function’s documented -1
error result. Follow the project’s existing test conventions, write the tests
before implementation changes, and run lint after the tests pass.
In `@src/ecro/render.clj`:
- Around line 71-98: Update truncate-to-width so the loop’s normal end-of-input
path appends "\033[0m" when sgr-active? is true, including exact-width inputs
such as "\033[31mabc"; preserve the existing truncation reset behavior. Add a
render test covering this exact-width trailing-SGR case in the existing render
test suite.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: e0d9b06b-23ed-4701-a1a3-69667c672c9d
📒 Files selected for processing (14)
rust/ecro-core/Cargo.tomlrust/ecro-core/ecro_core.hrust/ecro-core/src/lib.rssrc/ecro/buffer.cljsrc/ecro/core.cljsrc/ecro/key.cljsrc/ecro/native.cljsrc/ecro/render.cljsrc/ecro/undo.cljtest/ecro/buffer_test.cljtest/ecro/core_test.cljtest/ecro/key_test.cljtest/ecro/main_test.cljtest/ecro/render_test.clj
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: clojure
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{clj,cljs,cljc}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{clj,cljs,cljc}: Clojure code must hold all editor meaning and core editor state.
Runcljstyle checkfor Clojure linting.
Files:
test/ecro/core_test.cljtest/ecro/buffer_test.cljsrc/ecro/undo.cljtest/ecro/main_test.cljsrc/ecro/native.cljsrc/ecro/core.cljtest/ecro/key_test.cljsrc/ecro/buffer.cljtest/ecro/render_test.cljsrc/ecro/key.cljsrc/ecro/render.clj
**/*.{clj,cljs,cljc,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{clj,cljs,cljc,rs}: The FFI boundary must use a polling event queue rather than callbacks.
Follow test-driven development: write the test first, implement the minimum needed to pass, refactor afterward, and run lint before declaring completion.
Files:
test/ecro/core_test.cljtest/ecro/buffer_test.cljsrc/ecro/undo.cljtest/ecro/main_test.cljsrc/ecro/native.cljsrc/ecro/core.cljtest/ecro/key_test.cljsrc/ecro/buffer.cljtest/ecro/render_test.cljsrc/ecro/key.cljsrc/ecro/render.cljrust/ecro-core/src/lib.rs
test/ecro/**/*.{clj,cljs,cljc}
📄 CodeRabbit inference engine (AGENTS.md)
Use
clojure.testfor Clojure tests.
Files:
test/ecro/core_test.cljtest/ecro/buffer_test.cljtest/ecro/main_test.cljtest/ecro/key_test.cljtest/ecro/render_test.clj
**/*.clj
📄 CodeRabbit inference engine (AGENTS.md)
Use SCI-evaluated Clojure for the documented
~/.ecro/init.cljconfiguration file.
Files:
test/ecro/core_test.cljtest/ecro/buffer_test.cljsrc/ecro/undo.cljtest/ecro/main_test.cljsrc/ecro/native.cljsrc/ecro/core.cljtest/ecro/key_test.cljsrc/ecro/buffer.cljtest/ecro/render_test.cljsrc/ecro/key.cljsrc/ecro/render.clj
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust terminal-adapter code must not hold editor state; it must remain a stateless terminal adapter.
Files:
rust/ecro-core/src/lib.rs
rust/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
cargo testfor Rust tests,cargo clippy -- -D warningsfor linting, andcargo fmt --checkfor formatting.
Files:
rust/ecro-core/src/lib.rs
🔇 Additional comments (5)
rust/ecro-core/Cargo.toml (1)
11-12: LGTM!rust/ecro-core/ecro_core.h (1)
32-48: LGTM!src/ecro/native.clj (1)
7-9: LGTM!Also applies to: 44-45, 112-129
src/ecro/render.clj (1)
13-68: LGTM!Also applies to: 114-150, 194-195, 240-240
test/ecro/render_test.clj (1)
44-47: LGTM!Also applies to: 68-78, 92-107
| #[unsafe(no_mangle)] | ||
| /// Return the terminal-cell width of a UTF-8 byte slice. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// `text` must point to `length` readable bytes. The bytes must remain valid for the duration of | ||
| /// the call. | ||
| pub unsafe extern "C" fn ecro_display_width(text: *const u8, length: i32) -> i32 { | ||
| if text.is_null() || length < 0 { | ||
| return -1; | ||
| } | ||
|
|
||
| let bytes = unsafe { std::slice::from_raw_parts(text, length as usize) }; | ||
| match std::str::from_utf8(bytes) { | ||
| Ok(text) => i32::try_from(text.width()).unwrap_or(-1), | ||
| Err(_) => -1, | ||
| } | ||
| } | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| /// Return the UTF-16 length of the longest grapheme-aligned prefix within `maximum_width` cells. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// `text` must point to `length` readable bytes. The bytes must remain valid for the duration of | ||
| /// the call. | ||
| pub unsafe extern "C" fn ecro_prefix_utf16_length_for_width( | ||
| text: *const u8, | ||
| length: i32, | ||
| maximum_width: i32, | ||
| ) -> i32 { | ||
| if text.is_null() || length < 0 || maximum_width < 0 { | ||
| return -1; | ||
| } | ||
|
|
||
| let bytes = unsafe { std::slice::from_raw_parts(text, length as usize) }; | ||
| let Ok(text) = std::str::from_utf8(bytes) else { | ||
| return -1; | ||
| }; | ||
|
|
||
| let mut current_width = 0; | ||
| let mut utf16_length = 0; | ||
| for grapheme in text.graphemes(true) { | ||
| let grapheme_width = grapheme.width(); | ||
| if current_width + grapheme_width > maximum_width as usize { | ||
| break; | ||
| } | ||
| current_width += grapheme_width; | ||
| utf16_length += grapheme.encode_utf16().count(); | ||
| } | ||
|
|
||
| i32::try_from(utf16_length).unwrap_or(-1) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
新しい C ABI の境界ケースを Rust テストで固定してください。
ecro_display_width と ecro_prefix_utf16_length_for_width を呼び出すテストがありません。日本語の幅、補助文字、書記素境界、無効 UTF-8、負の長さ、null ポインタの戻り値をテストしてください。これにより Clojure 側の描画契約を固定できます。
As per coding guidelines, “Follow test-driven development: write the test first, implement the minimum needed to pass, refactor afterward, and run lint before declaring completion.”
🤖 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 `@rust/ecro-core/src/lib.rs` around lines 152 - 204, Add Rust tests for
ecro_display_width and ecro_prefix_utf16_length_for_width covering Japanese
display widths, supplementary Unicode characters, grapheme-aligned truncation,
invalid UTF-8, negative lengths, and null pointers, including each function’s
documented -1 error result. Follow the project’s existing test conventions,
write the tests before implementation changes, and run lint after the tests
pass.
Source: Coding guidelines
| (defn- truncate-to-width | ||
| [^String text width] | ||
| (let [result (StringBuilder.)] | ||
| (loop [offset 0 | ||
| current-width 0 | ||
| sgr-active? false] | ||
| (if (< offset (count text)) | ||
| (if-let [ansi-end (ansi-csi-end text offset)] | ||
| (do | ||
| (.append result (subs text offset ansi-end)) | ||
| (recur ansi-end | ||
| current-width | ||
| (sgr-active-after text offset ansi-end sgr-active?))) | ||
| (let [segment-end (plain-segment-end text offset false) | ||
| segment (subs text offset segment-end) | ||
| segment-width (terminal-width segment) | ||
| next-width (+ current-width segment-width)] | ||
| (if (<= next-width width) | ||
| (do | ||
| (.append result segment) | ||
| (recur segment-end next-width sgr-active?)) | ||
| (do | ||
| (let [prefix-length (terminal-prefix-length segment (- width current-width))] | ||
| (.append result (subs segment 0 prefix-length))) | ||
| (when sgr-active? | ||
| (.append result "\033[0m")) | ||
| (str result))))) | ||
| (str result))))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
末尾の有効な SGR をリセットしてください。
入力が幅をちょうど満たし、その末尾に有効な SGR がある場合、Line 98 はリセットを出力しません。
例えば、幅 3 の "\033[31mabc" は "\033[31mabc" のまま返ります。後続の描画に色または属性が継続します。
ループ終端でも sgr-active? が真の場合は "\033[0m" を追加してください。test/ecro/render_test.clj にこの境界ケースを追加してください。
修正案
- (str result)))))
+ (do
+ (when sgr-active?
+ (.append result "\033[0m"))
+ (str result))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn- truncate-to-width | |
| [^String text width] | |
| (let [result (StringBuilder.)] | |
| (loop [offset 0 | |
| current-width 0 | |
| sgr-active? false] | |
| (if (< offset (count text)) | |
| (if-let [ansi-end (ansi-csi-end text offset)] | |
| (do | |
| (.append result (subs text offset ansi-end)) | |
| (recur ansi-end | |
| current-width | |
| (sgr-active-after text offset ansi-end sgr-active?))) | |
| (let [segment-end (plain-segment-end text offset false) | |
| segment (subs text offset segment-end) | |
| segment-width (terminal-width segment) | |
| next-width (+ current-width segment-width)] | |
| (if (<= next-width width) | |
| (do | |
| (.append result segment) | |
| (recur segment-end next-width sgr-active?)) | |
| (do | |
| (let [prefix-length (terminal-prefix-length segment (- width current-width))] | |
| (.append result (subs segment 0 prefix-length))) | |
| (when sgr-active? | |
| (.append result "\033[0m")) | |
| (str result))))) | |
| (str result))))) | |
| (defn- truncate-to-width | |
| [^String text width] | |
| (let [result (StringBuilder.)] | |
| (loop [offset 0 | |
| current-width 0 | |
| sgr-active? false] | |
| (if (< offset (count text)) | |
| (if-let [ansi-end (ansi-csi-end text offset)] | |
| (do | |
| (.append result (subs text offset ansi-end)) | |
| (recur ansi-end | |
| current-width | |
| (sgr-active-after text offset ansi-end sgr-active?))) | |
| (let [segment-end (plain-segment-end text offset false) | |
| segment (subs text offset segment-end) | |
| segment-width (terminal-width segment) | |
| next-width (+ current-width segment-width)] | |
| (if (<= next-width width) | |
| (do | |
| (.append result segment) | |
| (recur segment-end next-width sgr-active?)) | |
| (do | |
| (let [prefix-length (terminal-prefix-length segment (- width current-width))] | |
| (.append result (subs segment 0 prefix-length))) | |
| (when sgr-active? | |
| (.append result "\033[0m")) | |
| (str result))))) | |
| (do | |
| (when sgr-active? | |
| (.append result "\033[0m")) | |
| (str result)))))) |
🤖 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 `@src/ecro/render.clj` around lines 71 - 98, Update truncate-to-width so the
loop’s normal end-of-input path appends "\033[0m" when sgr-active? is true,
including exact-width inputs such as "\033[31mabc"; preserve the existing
truncation reset behavior. Add a render test covering this exact-width
trailing-SGR case in the existing render test suite.
1 participant
日本語のレンダーが正常に文字幅をレンダーできていなかった問題を修正
Summary by CodeRabbit
新機能
改善