Terminal emulator core added (v3 Sprint)#91
Merged
Conversation
📝 WalkthroughWalkthroughAdds a public ChangesTerminal emulator
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant REPL
participant Terminal
participant Parser
participant Buffer
REPL->>Terminal: prompt()
Terminal-->>REPL: styled prompt
REPL->>Terminal: write(input)
Terminal->>Parser: parse input bytes
Parser->>Buffer: dispatch text and CSI events
Buffer-->>Terminal: update screen and cursor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/terminal_emulator.rs (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name implies cursor motion is tested, but it only exercises newlines.
The test
handles_newlines_and_cursor_motionwrites standard text and\n, but does not verify any actual horizontal cursor motion (like\b,\t, or CSIH/C/Dsequences). Consider expanding this test with cursor motion assertions, or renaming it to accurately reflect its current contents.🤖 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 `@tests/terminal_emulator.rs` around lines 11 - 19, Rename handles_newlines_and_cursor_motion to accurately describe that it only tests newline handling, unless you expand it to assert actual horizontal cursor motion. Keep the existing newline screen assertions unchanged if choosing the rename.
🤖 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 `@src/terminal.rs`:
- Around line 42-63: Refactor the terminal screen storage and writing logic
around write_char into a fixed character-cell grid so output is placed at
cursor_col rather than appended. Update advance_tab and cursor movement handling
to preserve/render space-filled gaps, and ensure writes overwrite existing cells
while padding rows with spaces when the cursor moves beyond current content;
keep scrolling and screen rendering consistent with the grid representation.
- Around line 155-176: Update clear_to_end and clear_line so erase operations
preserve both cursor_row and cursor_col. For the current row, remove content
from cursor_col through the line’s end rather than clearing the entire row, and
ensure clear_to_end applies the appropriate clearing to subsequent rows. Use a
character-cell-safe representation or bounds handling so truncation never panics
when cursor_col intersects a multi-byte character.
- Around line 185-190: Update parse_cursor_location so splitting on ';'
preserves empty parameters instead of filtering them out. Parse each positional
value independently, treating an empty or missing row/column as the default
value 1 before saturating subtraction, so sequences such as ";10" produce row 1
and column 10.
- Around line 71-78: Update the CSI parsing loop around chars.next() so only
bytes in the final-byte range 0x40..=0x7E set final_byte and terminate parsing;
continue collecting parameter and private-marker bytes in 0x20..=0x3F, including
'?' and ';', so sequences such as CSI ? 25 h remain intact.
---
Nitpick comments:
In `@tests/terminal_emulator.rs`:
- Around line 11-19: Rename handles_newlines_and_cursor_motion to accurately
describe that it only tests newline handling, unless you expand it to assert
actual horizontal cursor motion. Keep the existing newline screen assertions
unchanged if choosing the rename.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ed20d46-b78c-4cd7-946a-c95c12f0f8f8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/lib.rssrc/terminal.rstests/terminal_emulator.rs
exlier
commented
Jul 19, 2026
exlier
left a comment
exlier
left a comment
Contributor
Author
There was a problem hiding this comment.
TruShell has been upgraded
The shell now uses the requested crates to build a stronger terminal foundation:
vtefor VT/ANSI escape parsingnu-ansi-termfor a richer, colorized prompt
What changed
- Added the new dependencies in
Cargo.toml - Implemented a terminal buffer and escape-sequence handling in terminal.rs
- Switched the REPL prompt in main.rs to a styled cyan prompt
- Added regression coverage in terminal_emulator.rs
Verification
I verified the change by running:
cargo test --tests
Contributor
Author
|
What Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/terminal.rs (2)
71-85: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix line wrapping logic.
Because
cursor_colis aggressively clamped toself.width - 1after every character, the wrap conditionself.cursor_col + width > self.widthis mathematically impossible to reach. Characters at the end of a line will indefinitely overwrite the last cell without ever triggering a newline.To fix this, allow
cursor_colto increment up toself.width(representing the delayed-wrap state), and trigger the newline before writing the next character. Theif self.cursor_row >= self.heightcheck here is also redundant becausenewline()already handles scrolling.🐛 Proposed fix
_ => { let width = 1usize; - if self.cursor_col + width > self.width { + if self.cursor_col >= self.width { self.newline(); } - if self.cursor_row >= self.height { - self.scroll_up(); - } - let row = &mut self.rows[self.cursor_row]; row[self.cursor_col] = ch; - self.cursor_col = (self.cursor_col + width).min(self.width.saturating_sub(1)); + self.cursor_col += width; }🤖 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/terminal.rs` around lines 71 - 85, Update the character-writing branch in the terminal rendering logic to preserve a delayed-wrap state: allow cursor_col to advance through self.width instead of clamping to self.width - 1, so the existing wrap check triggers newline before writing the next character. Remove the redundant cursor_row bounds check and rely on newline() to handle scrolling, while preserving row writes for in-bounds cursor positions.
96-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the character under the cursor on backspace.
Standard terminal emulators treat the backspace character (
\bor0x08) strictly as a cursor movement command. It moves the cursor to the left but does not erase the character. Destructive backspace is typically handled by programs using the sequence\b \b. Overwriting the cell with a space breaks standard terminal expectations.🐛 Proposed fix
fn backspace(&mut self) { if self.cursor_col > 0 { self.cursor_col -= 1; - if let Some(row) = self.rows.get_mut(self.cursor_row) { - row[self.cursor_col] = ' '; - } } }🤖 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/terminal.rs` around lines 96 - 103, Update Terminal::backspace so it only moves cursor_col left when the cursor is not at the beginning, and remove the row mutation that replaces the underlying character with a space. Preserve the character under the cursor while retaining the existing boundary behavior.
🧹 Nitpick comments (1)
src/terminal.rs (1)
196-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the closure to fix a Clippy warning.
♻️ Proposed fix
let params: Vec<i64> = params .iter() - .flat_map(|param| param.iter().copied().map(|value| i64::from(value))) + .flat_map(|param| param.iter().copied().map(i64::from)) .collect();🤖 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/terminal.rs` around lines 196 - 200, In csi_dispatch, simplify the nested flat_map closure used to convert Params values into Vec<i64> so it satisfies Clippy, preserving the existing flattening and i64 conversion behavior.Source: Linters/SAST tools
🤖 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 `@src/terminal.rs`:
- Around line 111-119: Update clear_screen to erase all cell contents without
modifying cursor_row or cursor_col; remove the cursor reset so the cursor
remains exactly at its position before the clear.
- Around line 218-234: Update the cursor movement handlers for commands A, B, C,
and D to normalize an explicit zero parameter to an amount of 1, while retaining
1 as the default when the parameter is omitted. Preserve the existing saturating
and boundary-clamping behavior in each movement branch.
---
Outside diff comments:
In `@src/terminal.rs`:
- Around line 71-85: Update the character-writing branch in the terminal
rendering logic to preserve a delayed-wrap state: allow cursor_col to advance
through self.width instead of clamping to self.width - 1, so the existing wrap
check triggers newline before writing the next character. Remove the redundant
cursor_row bounds check and rely on newline() to handle scrolling, while
preserving row writes for in-bounds cursor positions.
- Around line 96-103: Update Terminal::backspace so it only moves cursor_col
left when the cursor is not at the beginning, and remove the row mutation that
replaces the underlying character with a space. Preserve the character under the
cursor while retaining the existing boundary behavior.
---
Nitpick comments:
In `@src/terminal.rs`:
- Around line 196-200: In csi_dispatch, simplify the nested flat_map closure
used to convert Params values into Vec<i64> so it satisfies Clippy, preserving
the existing flattening and i64 conversion behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4670c25e-ba8f-460c-be5c-a5ff720d4687
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/main.rssrc/terminal.rstests/terminal_emulator.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current implementation stays lightweight and testable while leaving a clear path to a more complete emulator later.
Crates used for easing future development:
vte: strong choice for ANSI/VT escape parsingnu-ansi-term: useful if we want richer prompt/colors laterCloses #81
Summary by CodeRabbit