(FOR CI) console: allow using arrow left/right to edit the line #52
Conversation
WalkthroughAdds UTF‑8‑aware, in‑place console line editing: UTF‑8 decoding and navigation, per‑character byte/width tracking, cursor movement and redraw helpers, insertion/deletion at character boundaries, enhanced escape‑sequence and Windows key handling (arrows, Home/End, Delete, Ctrl‑word), and persistent input history — all internal to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/console.cpp (1)
382-388: Consider using parameterized ANSI escape sequences for efficiency.The current loop emits one escape sequence per column. For large cursor movements, using parameterized sequences like
\033[%dC(move right N columns) and\033[%dD(move left N columns) would reduce output volume.#else - if (delta < 0) { - for (int i = 0; i < -delta; i++) fprintf(out, "\b"); - } else { - for (int i = 0; i < delta; i++) fprintf(out, "\033[C"); + if (delta < 0) { + fprintf(out, "\033[%dD", -delta); + } else { + fprintf(out, "\033[%dC", delta); } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build-cmake-pkg / linux
- GitHub Check: android-build
- GitHub Check: ubuntu-latest-cmake-cuda
- GitHub Check: ubuntu-24-cmake-vulkan
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: macOS-latest-cmake-ios
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-22-cmake-sycl
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: macOS-latest-cmake-visionos
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: ubuntu-24-cmake-webgpu
- GitHub Check: ubuntu-24-wasm-webgpu
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: server-windows
🔇 Additional comments (8)
common/console.cpp (8)
4-4: LGTM!Required header for the new assertions added later in the code.
340-347: LGTM!The UTF-8 backward navigation is correct. The function properly skips continuation bytes (10xxxxxx pattern) to find the previous character's start position.
349-356: LGTM!Correct forward navigation through UTF-8 sequences, properly skipping continuation bytes.
401-407: LGTM!The position tracking state and invariant assertions are well-designed. The assertions will catch any desynchronization between byte position, character position, and the widths array during development.
427-464: LGTM with a minor note.The arrow key handling is correct. The left/right navigation properly maintains synchronization between
byte_pos,char_pos, and the visual cursor.Note: ESC followed by a non-
[and non-ESC character (e.g., Alt+letter on some terminals) is silently consumed. This is acceptable given the current scope of arrow key support.
465-493: LGTM!The backspace handling correctly manages UTF-8 character deletion at any position. The redraw-and-clear strategy properly handles variable-width characters (including CJK wide chars) by clearing the exact space freed by deletion.
494-531: LGTM!The character insertion logic correctly handles both end-of-line and mid-line insertion with proper UTF-8 byte management and display width tracking. The defensive clamping of negative widths to zero is a reasonable safeguard against non-printable character edge cases.
391-571: Verify cross-platform behavior of cursor handling inreadline_advanced.The
readline_advancedfunction uses intentionally different implementations for Windows and POSIX systems (cursor movement via console API vs. ANSI escape sequences, width estimation via fixed value vs.wcwidth()). These platform-specific approaches should be tested on Linux, macOS, and Windows with:
- Wide character (CJK) input and deletion
- Cursor movement across multi-byte UTF-8 characters
- Behavior at line wrap boundaries
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
common/console.cpp (2)
377-408: Consider optimizing line-wrapping arithmetic.The while loops (lines 388-395) work correctly but could be more efficient with division and modulo operations for larger deltas:
int newY = newCursorPosition.Y + (newCursorPosition.X + delta) / width; int newX = (newCursorPosition.X + delta) % width; if (newX < 0) { newX += width; newY--; }However, for typical navigation (delta = ±1), the current approach is fine.
484-501: Consider extracting duplicated arrow key logic.The left/right arrow handling logic is duplicated between escape sequences (lines 450-463) and Windows key constants (lines 485-498). While this works correctly, extracting the logic into helper functions would reduce duplication:
static void handle_left_arrow(size_t& char_pos, size_t& byte_pos, const std::string& line, const std::vector<int>& widths) { if (char_pos > 0) { int w = widths[char_pos - 1]; move_cursor(-w); char_pos--; byte_pos = prev_utf8_char_pos(line, byte_pos); } }However, the duplication is minimal and the current approach is clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: ggml-ci-x64-cpu-low-perf
- GitHub Check: ubuntu-22-cmake-musa
- GitHub Check: android-ndk-build (arm64-cpu, -D ANDROID_ABI=arm64-v8a -D ANDROID_PLATFORM=android-31 -D CMAKE_TO...
- GitHub Check: ubuntu-22-cmake-sycl-fp16
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: ubuntu-24-cmake-webgpu
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: ubuntu-22-cmake-sycl
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-24-wasm-webgpu
- GitHub Check: ubuntu-24-cmake-vulkan
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: server-windows
🔇 Additional comments (6)
common/console.cpp (6)
4-4: LGTM! Well-designed approach to special key handling.Using private-use Unicode values (0xE000–0xE003) for special keys is a clean solution that avoids conflicts with real input characters and allows uniform handling across platforms.
Also applies to: 39-47
359-375: LGTM! Correct UTF-8 boundary detection.The helpers correctly identify UTF-8 continuation bytes (
(byte & 0xC0) == 0x80) and handle boundary conditions properly.
420-427: LGTM! Good use of assertions to maintain invariants.The assertions correctly validate that
byte_pos >= char_pos(multi-byte UTF-8) andchar_pos <= widths.size()(per-character tracking).
446-483: LGTM! Escape sequence handling is correct.The code properly handles ANSI escape sequences for arrow keys (ESC [ D/C for left/right) and correctly discards unknown sequences.
502-530: LGTM! Backspace handling correctly maintains UTF-8 boundaries.The deletion logic properly:
- Computes UTF-8 character boundaries
- Updates both line content and display widths
- Redraws affected text
- Restores cursor position
531-569: LGTM! Insertion logic correctly handles both end and middle positions.The code properly:
- Converts codepoint to UTF-8
- Inserts at the correct byte position
- Maintains the widths vector
- Redraws affected text
- Updates cursor and positions correctly
The negative width clamping (
if (real_w < 0) real_w = 0) appropriately handles control characters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
common/console.cpp (1)
39-48: Windows arrow key mapping looks good; remove redundantwc == 0check (already flagged previously).Using private-use
KEY_ARROW_*codes and mappingwVirtualKeyCodewhenUnicodeChar == 0is a clean way to surface arrows as logical characters. However, the secondif (wc == 0) { continue; }after the newswitchis now dead/unreachable: allwc == 0paths already eitherreturnaKEY_ARROW_*orcontinuevia thedefaultcase in theswitch.You can safely delete that second
if (wc == 0)block to simplify control flow:- if (wc == 0) { - switch (record.Event.KeyEvent.wVirtualKeyCode) { - case VK_LEFT: return KEY_ARROW_LEFT; - case VK_RIGHT: return KEY_ARROW_RIGHT; - case VK_UP: return KEY_ARROW_UP; - case VK_DOWN: return KEY_ARROW_DOWN; - default: continue; - } - } - if (wc == 0) { - continue; - } + if (wc == 0) { + switch (record.Event.KeyEvent.wVirtualKeyCode) { + case VK_LEFT: return KEY_ARROW_LEFT; + case VK_RIGHT: return KEY_ARROW_RIGHT; + case VK_UP: return KEY_ARROW_UP; + case VK_DOWN: return KEY_ARROW_DOWN; + default: continue; + } + }Also applies to: 191-202, 486-503
🧹 Nitpick comments (3)
common/console.cpp (3)
361-377: UTF‑8 prev/next helpers are correct for valid UTF‑8; optional minor robustness tweak.
prev_utf8_char_pos/next_utf8_char_poscorrectly walk backward/forward over continuation bytes using(byte & 0xC0) == 0x80, assumingposalways lands on a character boundary (which yourbyte_posinvariants maintain). For extra clarity/robustness, you could cast tounsigned charwhen masking to avoid any confusion on platforms wherecharis signed:- while (pos > 0 && (line[pos] & 0xC0) == 0x80) { + while (pos > 0 && (static_cast<unsigned char>(line[pos]) & 0xC0) == 0x80) { pos--; }(and similarly in
next_utf8_char_pos).This is purely cosmetic; the current logic is fine given valid UTF‑8 input and your invariants.
379-409:move_cursorimplementation looks sound; consider clamping on Windows as a safety net.The cross‑platform
move_cursor(delta)abstraction is nice, and the Windows branch correctly wraps horizontally using the console buffer width while the POSIX branch uses\band\033[C.One small defensive improvement: on Windows, if
deltais large and the current cursor is near the top of the buffer,newYcould become negative before callingSetConsoleCursorPosition. ClampingnewYto0(and optionally tobufferInfo.dwSize.Y - 1) would guard against odd console states without complicating the logic much:- while (newX < 0) { - newX += width; - newY--; - } + while (newX < 0) { + newX += width; + newY--; + } + if (newY < 0) { + newY = 0; + }Not strictly required, but it makes the helper safer under unusual cursor positions.
422-424: UTF‑8‑aware editing withbyte_pos/char_posis well‑structured; be aware of combining‑mark edge cases.The new
byte_pos/char_posstate, assertions, and use ofwidthsplusprev_utf8_char_pos/next_utf8_char_posgive you a clean model for left/right navigation, insertion in the middle of the line, and backspace, including multi‑byte UTF‑8 and multi‑column glyphs. The redraw logic for both deletion and insertion (re‑emitting the “tail” and moving the cursor back withmove_cursor) is consistent and looks correct.One subtle edge case: characters with display width 0 (e.g., some combining marks where
wcwidthreturns 0) will getwidths[i] == 0. In those cases, left/right/backspace will updatechar_pos/byte_pos, butmove_cursor(±w)becomes a no‑op, so the logical cursor index can drift relative to the physical console cursor when stepping over such code points. For most use cases this is probably acceptable, but if you ever want perfect behavior with combining sequences/grapheme clusters, you’d likely need to treat them as part of the previous cluster rather than independent positions.Given the added complexity that would entail, I’d keep the current behavior as‑is unless you explicitly target those edge cases; just worth being aware of the limitation.
Also, per the repo guidelines, it would be good to sanity‑check this path on at least one Linux/macOS terminal and a Windows console (arrow left/right, backspace, insertion in the middle, and some simple UTF‑8 like CJK characters).
Also applies to: 427-429, 448-478, 486-503, 505-532, 534-570
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: windows-latest-cmake-sycl
- GitHub Check: windows-latest-cmake (llvm-arm64-opencl-adreno, arm64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN...
- GitHub Check: windows-latest-cmake (openblas-x64, x64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN_FILE=cmake/x6...
- GitHub Check: windows-msys2 (CLANG64, clang-x86_64, Release)
- GitHub Check: windows-latest-cmake (vulkan-x64, x64, -DCMAKE_BUILD_TYPE=Release -DGGML_NATIVE=OFF -DLLAMA_BUILD...
- GitHub Check: windows-latest-cmake (llvm-arm64, arm64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN_FILE=cmake/ar...
- GitHub Check: windows-msys2 (UCRT64, ucrt-x86_64, Release)
- GitHub Check: windows-latest-cmake (cpu-x64 (static), x64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN_FILE=cmak...
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: windows-2022-cmake-cuda (12.4)
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: server-windows
🔇 Additional comments (1)
common/console.cpp (1)
4-4: Adding<cassert>for invariants is appropriate.Including
<cassert>to enforce internal invariants (e.g., onchar_pos/byte_pos) is reasonable here; it will help catch any future cursor/byte index mismatches during development without affecting release builds whenNDEBUGis set.
ngxson
force-pushed
the
xsn/cli_arrow_left_right
branch
from
cd05610 to
77ca259
Compare
Xsn/cli arrow left right
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
common/console.cpp (4)
4-5: New headers, key constants, and history vector look fine; consider bounding history size
- Adding
<cassert>and<cstddef>matches the new use ofassertandsize_t.- Using private-use Unicode values (0xE000–0xE005) for Windows-only key tokens is a reasonable way to keep them out of normal input space.
- A single static
std::vector<std::string> history;is fine for this console, but it will grow unbounded over a long-lived process.If you care about long sessions, you might cap history (e.g., keep last N entries) or offer a way to clear it; otherwise this is acceptable as-is.
Also applies to: 40-52, 62-63
344-370: UTF‑8 helpers work for well‑formed input; add minimal validation for malformed sequences
decode_utf8correctly decodes 1–4 byte sequences and falls back to U+FFFD when the leading byte doesn’t match any known pattern, andprev_utf8_char_pos/next_utf8_char_posare standard “walk over continuation bytes” helpers that assume valid UTF‑8.One small robustness gap is that
decode_utf8doesn’t currently check that continuation bytes are in the10xxxxxxrange; malformed sequences like a 2‑byte lead followed by an ASCII byte will be decoded as if they were valid. If you want to keep behavior predictable on bad input, you can cheaply validate continuation bytes before constructing the codepoint, e.g.:- if ((c & 0xE0u) == 0xC0u && pos + 1 < input.size()) { - advance = 2; - return ((c & 0x1Fu) << 6) | (static_cast<unsigned char>(input[pos + 1]) & 0x3Fu); - } + if ((c & 0xE0u) == 0xC0u && pos + 1 < input.size()) { + unsigned char c1 = static_cast<unsigned char>(input[pos + 1]); + if ((c1 & 0xC0u) != 0x80u) { + advance = 1; + return 0xFFFD; + } + advance = 2; + return ((c & 0x1Fu) << 6) | (c1 & 0x3Fu); + } @@ - if ((c & 0xF0u) == 0xE0u && pos + 2 < input.size()) { - advance = 3; - return ((c & 0x0Fu) << 12) | - ((static_cast<unsigned char>(input[pos + 1]) & 0x3Fu) << 6) | - (static_cast<unsigned char>(input[pos + 2]) & 0x3Fu); - } + if ((c & 0xF0u) == 0xE0u && pos + 2 < input.size()) { + unsigned char c1 = static_cast<unsigned char>(input[pos + 1]); + unsigned char c2 = static_cast<unsigned char>(input[pos + 2]); + if ((c1 & 0xC0u) != 0x80u || (c2 & 0xC0u) != 0x80u) { + advance = 1; + return 0xFFFD; + } + advance = 3; + return ((c & 0x0Fu) << 12) | + ((c1 & 0x3Fu) << 6) | + (c2 & 0x3Fu); + } @@ - if ((c & 0xF8u) == 0xF0u && pos + 3 < input.size()) { - advance = 4; - return ((c & 0x07u) << 18) | - ((static_cast<unsigned char>(input[pos + 1]) & 0x3Fu) << 12) | - ((static_cast<unsigned char>(input[pos + 2]) & 0x3Fu) << 6) | - (static_cast<unsigned char>(input[pos + 3]) & 0x3Fu); - } + if ((c & 0xF8u) == 0xF0u && pos + 3 < input.size()) { + unsigned char c1 = static_cast<unsigned char>(input[pos + 1]); + unsigned char c2 = static_cast<unsigned char>(input[pos + 2]); + unsigned char c3 = static_cast<unsigned char>(input[pos + 3]); + if ((c1 & 0xC0u) != 0x80u || (c2 & 0xC0u) != 0x80u || (c3 & 0xC0u) != 0x80u) { + advance = 1; + return 0xFFFD; + } + advance = 4; + return ((c & 0x07u) << 18) | + ((c1 & 0x3Fu) << 12) | + ((c2 & 0x3Fu) << 6) | + (c3 & 0x3Fu); + }
prev_utf8_char_posandnext_utf8_char_posthen continue to behave well on the strings you build viaappend_utf8.Also applies to: 393-409
415-472: Cursor helpers overall look good; consider clamping Windows coordinates inmove_cursor
clear_current_line,set_line_contents, andmove_to_line_start/move_to_line_endnicely centralize the display-width accounting and keepchar_pos/byte_posconsistent with what’s on screen.In
move_cursor(int delta)on Windows, for large negative or positive deltas near the top or bottom of the buffer,newYcan become < 0 or ≥bufferInfo.dwSize.Y, which makesSetConsoleCursorPositionreceive out-of-range coordinates. Because your deltas are in practice bounded by the current line width, this is unlikely to matter, but adding a clamp is cheap insurance:if (hConsole != NULL) { CONSOLE_SCREEN_BUFFER_INFO bufferInfo; GetConsoleScreenBufferInfo(hConsole, &bufferInfo); COORD newCursorPosition = bufferInfo.dwCursorPosition; int width = bufferInfo.dwSize.X; + int height = bufferInfo.dwSize.Y; int newX = newCursorPosition.X + delta; int newY = newCursorPosition.Y; @@ while (newX < 0) { newX += width; newY--; } - newCursorPosition.X = newX; - newCursorPosition.Y = newY; + if (newY < 0) { + newY = 0; + newX = 0; + } else if (newY >= height) { + newY = height - 1; + newX = width - 1; + } + + newCursorPosition.X = static_cast<SHORT>(newX); + newCursorPosition.Y = static_cast<SHORT>(newY); SetConsoleCursorPosition(hConsole, newCursorPosition); }
761-767: History persistence works; think about multiline prompts and empty/whitespace entriesThe new history persistence at the end of
readline_advancedworks as intended: you trim a trailing newline, then push the resulting string for non‑EOF inputs.Two minor considerations:
- For multiline input, you currently push an entry each time
readline_advancedreturns, even whenhas_moreis true. If you’d prefer history to contain only “complete” prompts, you could gate this onhas_more == false.- If you don’t want purely whitespace lines in history, you could trim and skip when the result is empty.
Both are optional UX tweaks; the current behavior is acceptable if it matches how you expect the console to be used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
🧬 Code graph analysis (1)
common/console.cpp (3)
tools/run/run.cpp (1)
line(1183-1183)vendor/nlohmann/json.hpp (1)
move(6987-6990)tools/server/public_legacy/json-schema-to-grammar.mjs (1)
digits(145-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: windows-latest-cmake-sycl
- GitHub Check: ggml-ci-x64-cpu-high-perf
- GitHub Check: ios-xcode-build
- GitHub Check: windows-msys2 (CLANG64, clang-x86_64, Release)
- GitHub Check: macOS-latest-cmake-visionos
- GitHub Check: ubuntu-22-cmake-musa
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-22-cmake-sycl-fp16
- GitHub Check: ubuntu-24-wasm-webgpu
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: ubuntu-22-cmake-sycl
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: server-windows
🔇 Additional comments (2)
common/console.cpp (2)
619-656: Windows virtual‑key handling integrates cleanly with the navigation and history logicThe Windows‑specific handling of
KEY_ARROW_LEFT/RIGHT,KEY_HOME/END, andKEY_ARROW_UP/DOWNmirrors the CSI behavior on POSIX:
- Left/right correctly move by the prior/next character width and adjust both
char_posandbyte_posusingprev_utf8_char_pos/next_utf8_char_pos.- Home/End reuse
move_to_line_start/move_to_line_end, so cursor semantics stay consistent.- Up/Down reuse the same history navigation logic as the CSI
'A'/'B'path, including restoring the “empty current line” when moving past the latest history entry.This is a nice way to keep the higher‑level editing logic platform‑agnostic.
657-723: Backspace and insertion logic correctly handle UTF‑8 and screen reflowThe new backspace and insertion code in
readline_advancedlooks sound:
- Backspace:
- Only acts when
char_pos > 0, useswidths[char_pos - 1]for visual movement, and usesprev_utf8_char_pos/byte_posto find the byte range to erase.- Updates
lineandwidthsconsistently, then redraws the tail of the line and clears the trailing character cell(s) that may remain on screen.- Insertion:
- Builds a UTF‑8 sequence via
append_utf8, estimates its width, and either appends at the end or inserts into the middle.- After inserting in the middle, it prints the new codepoint, redraws the tail based on
widths, then moves the cursor back over the tail sochar_pos/byte_posstay aligned with the logical cursor.- Uses
put_codepointin both paths so Windows gets accurate width measurements.This gives you correct behavior for multi‑byte UTF‑8 characters, including insertion and deletion in the middle of the line.
|
Last commit add CTRL + left/right arrow -> word jump |
Nice, thanks! I'll push it as a dedicated PR once the current PR is merged |
- Add KEY_CTRL_ARROW_LEFT and KEY_CTRL_ARROW_RIGHT codes - Windows: detect CTRL modifier via dwControlKeyState - Linux: parse ANSI sequences with modifier (1;5D/C) - Implement move_word_left/right with space-skipping logic - Refactor escape sequence parsing to accumulate params
- Windows: VK_DELETE detection - Linux: ESC[3~ sequence parsing - Forward character deletion with UTF-8 support
- Edit any history line during UP/DOWN navigation, edits persist - Pressing Enter appends edited version as new history entry - Original line stay untouched in their positions
…to xsn/cli_arrow_left_right
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
common/console.cpp (3)
4-7: UTF‑8 decoding and char-boundary helpers are sound; consider small robustness tweaks
- The added headers (
<cassert>,<cstddef>,<cctype>,<cwctype>) match the new usages (asserts,size_t,std::isdigit,std::iswspace), so dependency surface is consistent.decode_utf8and theprev_utf8_char_pos/next_utf8_char_poshelpers correctly walk byte boundaries assuming well‑formed UTF‑8 and never returnadvance == 0, so loops that consume byadvancecannot hang.- If you ever expect malformed UTF‑8 (e.g., pasted binary), you might want to:
- Assert
pos < input.size()indecode_utf8, and- Optionally validate continuation bytes (
(b & 0xC0) == 0x80) before treating them as part of a multi‑byte sequence, falling back to0xFFFDwhen that fails.
These aren’t correctness blockers given current controlled sources ofline, but they would make the helpers more defensive if input sources broaden later.Also applies to: 352-417
419-451: Cursor movement and redraw utilities look consistent; tail‑redraw logic is duplicated
delete_at_cursor,clear_current_line,set_line_contents,move_to_line_start/end, andmove_cursormaintainchar_pos,byte_pos, andwidthsinvariants correctly and only move the cursor by the summed widths they themselves printed, so they shouldn’t walk off the visible line under normal usage.- The “redraw tail then move cursor back” pattern in
delete_at_cursoris nearly identical to what the backspace handler does later inreadline_advanced, so there’s some duplication of a fairly intricate sequence (recompute byte positions, print tail codepoints with stored widths, computetail_width, thenmove_cursor(-(tail_width + w))).- Extracting that common “erase one char and redraw tail” behavior into a shared helper (parameterized by whether deletion is backward or forward) would reduce the chance of future drift between backspace and Delete handling while keeping the invariants in one place.
Also applies to: 453-508, 627-658
42-56: Windows key mapping to private char32_t codes is clean; consider tightening the abstraction
- Mapping Windows virtual key codes (arrows, Home/End, Delete, and Ctrl+Arrow variants) to reserved private‑use
char32_tvalues ingetchar32()is a neat way to unify handling with the POSIX escape‑sequence path;readline_advancedjust switches on these logical keys.- The implementation correctly detects Ctrl via
dwControlKeyState & (LEFT_CTRL_PRESSED | RIGHT_CTRL_PRESSED)and distinguishes plain vs. Ctrl+Left/Right.- As a minor improvement, you could wrap these special key codes in a
enum class(or at least a namedenum) instead of rawchar32_tconstants, which would make it harder to accidentally treat them as real Unicode code points elsewhere and would self‑document the intent at call sites.Also applies to: 187-214, 799-852
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: WebUI Check
- GitHub Check: ubuntu-latest-cmake-cuda
- GitHub Check: macOS-latest-cmake-visionos
- GitHub Check: windows-latest-cmake-hip
- GitHub Check: windows-2022-cmake-cuda (12.4)
- GitHub Check: windows-latest-cmake-sycl
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: ubuntu-24-wasm-webgpu
- GitHub Check: ubuntu-22-cmake-hip
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: ubuntu-24-cmake-vulkan
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: server-windows
🔇 Additional comments (3)
common/console.cpp (3)
510-541: Word navigation behavior is reasonable and locale‑aware
has_ctrl_modifiercorrectly recognizes Ctrl‑modified CSI sequences likeESC [ 1 ; 5 D/ESC [ 1 ; 5 Cby treating any;‑separated parameter with value5as “Ctrl pressed”.is_space_codepointusesstd::iswspaceon the decoded code point, so word splitting respects the active locale (after the earliersetlocale(LC_ALL, "")) and handles non‑ASCII space characters.move_word_left/move_word_rightimplement intuitive behavior:
- Left: skip spaces leftwards, then skip a word of non‑spaces, landing at the word start.
- Right: skip spaces rightwards, then a word, then trailing spaces, landing at the start of the next word (or end of line).
Given the invariants onchar_pos,byte_pos, andwidths, the loops respect bounds and don’t overrunline.Also applies to: 542-625
705-791: CSI parsing now fully consumes escape sequences and fixes the Ctrl+Arrow residue issue
- The new
ESC [handling that accumulates all intermediate bytes intoparamsuntil the final byte (A–Z,a–z,~, orWEOF) ensures the entire CSI sequence is consumed before acting, so there are no leftover bytes to be misinterpreted as normal input.- Using
has_ctrl_modifier(params)for the'C'/'D'cases cleanly distinguishes plain arrows from Ctrl+Left/Right (e.g.,ESC [ 1 ; 5 D), and maps them to character vs. word navigation, respectively.- The tilde sequences for Home/End/Delete (
1~/7~/4~/8~/3~) are handled via thedigitsextraction fromparams, with unknown numeric sequences simply ignored after being fully read, which is a safe default.
This redesign addresses the earlier issue where parts of extended CSI sequences (like";5D") could leak into the input stream and show up as spurious characters.
854-922: Backspace and insertion paths preserve UTF‑8 and width invariants
- The backspace branch walks to the previous UTF‑8 character boundary via
prev_utf8_char_pos, erases the correct byte span, updateswidths, redraws the tail with stored widths, and usesmove_cursor(-(tail_width + w))to restore the cursor — this matches the forward‑delete logic indelete_at_cursor.- The insertion path correctly handles both appending and mid‑line insertion:
append_utf8encodesinput_charinto UTF‑8 bytes.- You measure and store
real_wfromput_codepoint(clamped at 0), sowidthsstays non‑negative.- For mid‑line inserts, you print the inserted codepoint, then reprint the tail and move the cursor back by
tail_width, updatingbyte_pos/char_posafterwards.- The asserts at the top of the loop (
char_pos <= byte_posandchar_pos <= widths.size()) are consistent with these updates and should catch any future regressions in editing paths early in debug builds.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-cmake-pkg / linux
- GitHub Check: ubuntu-22-cmake-musa
- GitHub Check: ubuntu-24-cmake-vulkan
- GitHub Check: ubuntu-cpu-cmake (x64, ubuntu-22.04)
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: windows-latest-cmake (llvm-arm64-opencl-adreno, arm64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN...
- GitHub Check: windows-msys2 (CLANG64, clang-x86_64, Release)
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: macOS-latest-cmake-tvos
- GitHub Check: windows-msys2 (UCRT64, ucrt-x86_64, Release)
- GitHub Check: windows-latest-cmake (vulkan-x64, x64, -DCMAKE_BUILD_TYPE=Release -DGGML_NATIVE=OFF -DLLAMA_BUILD...
- GitHub Check: ubuntu-24-cmake-webgpu
- GitHub Check: windows-latest-cmake (cpu-x64 (static), x64, -G "Ninja Multi-Config" -D CMAKE_TOOLCHAIN_FILE=cmak...
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: server-windows
🔇 Additional comments (11)
common/console.cpp (11)
4-8: LGTM!The added headers are appropriate for the UTF-8 and line-editing functionality. The Windows-specific key constants are well-placed in the private-use Unicode area (U+E000-U+F8FF) to avoid conflicts with legitimate input characters.
Also applies to: 43-62
200-213: LGTM!The Windows-specific key handling correctly detects the Ctrl modifier and maps virtual key codes to the private key constants. The logic is clean and handles all the necessary navigation keys.
400-416: LGTM!The UTF-8 character boundary navigation functions correctly identify continuation bytes (pattern
10xxxxxx) and handle edge cases at string boundaries.
424-507: LGTM!The line editing helper functions are well-structured and correctly handle:
- UTF-8 character boundaries during deletion
- Width tracking for proper cursor positioning
- Redrawing affected portions of the line
- Clearing and replacing line contents
579-624: Verify the word-right navigation logic.The
move_word_rightfunction has three sequential loops:
- Skip current whitespace (588-597)
- Skip the next word (599-608)
- Skip trailing whitespace again (610-619)
This means pressing Ctrl+Right will skip over the next word entirely and position at the start of the word after next. For example, with cursor at the start of "hello world test", Ctrl+Right would jump to the start of "test", skipping "world" entirely.
Typically, word-right navigation (bash/readline style) positions at the end of the current word or the start of the next word, not the start of the word-after-next. Can you confirm this three-loop behavior is intentional?
If this should match typical bash behavior, consider simplifying to:
static void move_word_right(size_t & char_pos, size_t & byte_pos, const std::vector<int> & widths, const std::string & line) { if (char_pos >= widths.size()) { return; } size_t new_char_pos = char_pos; size_t new_byte_pos = byte_pos; int move_width = 0; - while (new_char_pos < widths.size()) { + // Skip the current word if we're in one + size_t advance = 0; + char32_t cp = decode_utf8(line, new_byte_pos, advance); + bool in_word = !is_space_codepoint(cp); + + if (in_word) { + while (new_char_pos < widths.size()) { + size_t advance = 0; + char32_t cp = decode_utf8(line, new_byte_pos, advance); + if (is_space_codepoint(cp)) { + break; + } + move_width += widths[new_char_pos]; + new_char_pos++; + new_byte_pos += advance; + } + } + + // Skip whitespace to reach next word + while (new_char_pos < widths.size()) { size_t advance = 0; char32_t cp = decode_utf8(line, new_byte_pos, advance); - if (!is_space_codepoint(cp)) { + if (!is_space_codepoint(cp)) { break; } move_width += widths[new_char_pos]; new_char_pos++; new_byte_pos += advance; } - - while (new_char_pos < widths.size()) { - size_t advance = 0; - char32_t cp = decode_utf8(line, new_byte_pos, advance); - if (is_space_codepoint(cp)) { - break; - } - move_width += widths[new_char_pos]; - new_char_pos++; - new_byte_pos += advance; - } - - while (new_char_pos < widths.size()) { - size_t advance = 0; - char32_t cp = decode_utf8(line, new_byte_pos, advance); - if (!is_space_codepoint(cp)) { - break; - } - move_width += widths[new_char_pos]; - new_char_pos++; - new_byte_pos += advance; - } move_cursor(move_width); char_pos = new_char_pos; byte_pos = new_byte_pos; }
626-711: LGTM!The cursor movement function properly handles platform differences (Windows console wrapping vs POSIX ANSI codes). The history structure implements bash-style editing where modifications persist during navigation, as confirmed in previous review discussions.
728-748: LGTM!The position invariant assertions (char_pos ≤ byte_pos ≤ widths.size()) are excellent defensive checks. The history navigation lambdas provide clean abstractions over the underlying history operations.
768-841: LGTM!The escape sequence handling comprehensively parses CSI sequences with parameter support, Ctrl modifier detection, and proper handling of arrow keys, Home/End, Delete, and history navigation. Previous review discussions confirmed the escape sequence parsing issues were addressed.
877-944: LGTM!The backspace and character insertion logic correctly handles:
- UTF-8 multi-byte characters
- Per-character width tracking
- Tail redrawing for mid-line edits
- Position synchronization (byte_pos and char_pos)
981-988: LGTM!The history management correctly strips the trailing newline before storage and avoids adding empty entries. The TODO comment about multiline history support is a reasonable future enhancement note.
842-876: Compilation error: undefined functionsync_history_line().Line 867 calls
sync_history_line(), but this function is not defined anywhere in the code. This will cause a compilation failure.Looking at the context, this appears to be unnecessary since the history is managed elsewhere. You can simply remove this call:
} else if (input_char == KEY_DELETE) { delete_at_cursor(line, widths, char_pos, byte_pos); - sync_history_line(); } else if (input_char == KEY_ARROW_UP || input_char == KEY_ARROW_DOWN) {The history synchronization happens automatically when you press Enter (lines 981-988), so explicit syncing during editing is not needed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/console.cpp (1)
351-395: Consider adding semantic UTF-8 validation.The decoder now correctly validates continuation byte patterns (
10xxxxxx), which addresses the previous review feedback. However, to be fully UTF-8 compliant, consider adding:
- Overlong encoding rejection: Disallow code points encoded in more bytes than necessary (e.g.,
0xC0 0x80for U+0000 instead of0x00).- Surrogate half rejection: Reject U+D800–U+DFFF, which are invalid in UTF-8.
- Range validation: Reject code points > U+10FFFF.
These checks would prevent accepting technically malformed UTF-8 sequences that could cause issues downstream.
Example validation additions:
// After decoding 2-byte sequence: char32_t cp = ((c & 0x1Fu) << 6) | (c1 & 0x3Fu); if (cp < 0x80) { // overlong advance = 1; return 0xFFFD; } // After decoding 3-byte sequence: char32_t cp = ((c & 0x0Fu) << 12) | ((c1 & 0x3Fu) << 6) | (c2 & 0x3Fu); if (cp < 0x800 || (cp >= 0xD800 && cp <= 0xDFFF)) { // overlong or surrogate advance = 1; return 0xFFFD; } // After decoding 4-byte sequence: char32_t cp = ((c & 0x07u) << 18) | ((c1 & 0x3Fu) << 12) | ((c2 & 0x3Fu) << 6) | (c3 & 0x3Fu); if (cp < 0x10000 || cp > 0x10FFFF) { // overlong or out of range advance = 1; return 0xFFFD; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/console.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,h,hpp}: Always format C++ code usinggit clang-formatbefore committing, following .clang-format configuration (4-space indentation, 120 column limit, braces on same line for functions, pointer/reference alignment in middle)
Minimize external dependencies; avoid adding new external dependencies unless absolutely necessary
Files:
common/console.cpp
**/*.{cpp,h,hpp,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure cross-platform compatibility by testing code changes on Linux, macOS, and Windows when possible
Files:
common/console.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: ngxson
Repo: ngxson/llama.cpp PR: 14
File: examples/lookahead/lookahead.cpp:97-98
Timestamp: 2025-03-13T22:56:47.417Z
Learning: ngxson prefers to prioritize simplification in some cases and defer adding error handling to a later time, as indicated by their response to a suggestion about adding error checks for llama_decode_ext calls.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: WebUI Check
- GitHub Check: build-linux-cross / debian-13-loongarch64-vulkan-cross
- GitHub Check: macOS-latest-cmake-tvos
- GitHub Check: ubuntu-24-cmake-vulkan
- GitHub Check: ubuntu-24-cmake-webgpu
- GitHub Check: ubuntu-24-cmake-vulkan-deb
- GitHub Check: ubuntu-cpu-cmake (arm64, ubuntu-22.04-arm)
- GitHub Check: macOS-latest-cmake-ios
- GitHub Check: ubuntu-latest-cmake-rpc
- GitHub Check: ubuntu-24-wasm-webgpu
- GitHub Check: macOS-latest-cmake-arm64-webgpu
- GitHub Check: ubuntu-latest-cmake-sanitizer (THREAD, Debug)
- GitHub Check: ubuntu-latest-cmake-sanitizer (UNDEFINED, Debug)
- GitHub Check: ubuntu-latest-llguidance
- GitHub Check: macOS-latest-cmake-arm64
- GitHub Check: ubuntu-latest-cmake-sanitizer (ADDRESS, Debug)
- GitHub Check: ubuntu-22-cmake-hip
- GitHub Check: macOS-latest-cmake-x64
- GitHub Check: server-windows
🔇 Additional comments (20)
common/console.cpp (20)
4-8: LGTM!The new standard library includes are appropriate for the UTF-8 handling, character classification, and type safety added in this PR.
43-58: LGTM!Using private-use Unicode values (U+E000–U+E008) to represent Windows special keys is a clean approach that unifies key handling across platforms without risking collisions with actual input.
200-212: LGTM!The Windows key handling correctly maps virtual key codes to the private-use constants and properly detects Ctrl modifiers for word-wise navigation.
418-434: LGTM!The UTF-8 character position navigation correctly identifies character boundaries by scanning for continuation bytes. The boundary checks prevent out-of-bounds access.
442-468: LGTM!The
delete_at_cursorfunction correctly handles UTF-8 multi-byte characters, updates the widths vector, redraws the affected tail, and repositions the cursor.
470-481: LGTM!The
clear_current_linehelper correctly calculates display width and clears the line. The defensive handling of negative widths is a nice touch.
483-505: LGTM!The
set_line_contentsfunction correctly replaces the entire line, rebuilds the widths vector with actual display widths, and positions the cursor at the end. This is essential for history navigation.
507-525: LGTM!The
move_to_line_startandmove_to_line_endfunctions correctly calculate cursor movement from the widths vector and update both character and byte positions.
527-553: LGTM!The
has_ctrl_modifierfunction correctly parses semicolon-separated CSI parameters to detect the Ctrl modifier (value 5), which is standard for sequences likeESC[1;5D(Ctrl+Left).
555-557: LGTM!The
is_space_codepointwrapper provides a clear interface for whitespace detection using the standard library.
559-595: LGTM!The
move_word_leftfunction implements standard readline-style word navigation: skip trailing whitespace, then move backward through the word. The UTF-8 handling is correct.
597-642: LGTM!The
move_word_rightfunction correctly implements bash-style forward word navigation with three phases: skip current whitespace, skip the word, and skip trailing whitespace to position at the start of the next word.
644-675: LGTM!The
move_cursorfunction correctly handles platform-specific cursor movement with proper line-wrapping logic for Windows and standard escape sequences for POSIX.
677-729: LGTM!The
history_tstructure implements clean history navigation with proper state management. The bash-style behavior (edits persist during navigation) was confirmed as intentional in previous review discussions.
746-766: LGTM!The history navigation lambdas cleanly encapsulate the logic for moving through history and updating the displayed line. The state checks ensure correct behavior.
786-893: LGTM!The escape sequence handling is comprehensive and correct. Both POSIX (via CSI sequences) and Windows (via private key constants) support the same navigation features including Ctrl+Arrow for word-wise movement, which addresses previous review feedback.
894-922: LGTM!The backspace handling correctly deletes the previous UTF-8 character, updates the widths vector, redraws the affected tail, and repositions the cursor.
923-961: LGTM!The character insertion logic correctly handles both end-of-line and mid-line insertion, properly encoding UTF-8, updating the widths vector, and redrawing affected portions of the line.
998-1005: LGTM!The history storage correctly adds non-empty lines after removing the trailing newline. The TODO for multiline history support is noted for future enhancement.
746-747: LGTM!The loop invariant assertions are excellent defensive programming that help catch internal logic errors. They correctly express the relationships:
char_pos ≤ byte_pos(since UTF-8 characters can span multiple bytes) andchar_pos ≤ widths.size()(since char_pos indexes into widths).
…pport)
Make sure to read the contributing guidelines before submitting a PR
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.