Fix SharedWriter indentation bug and improve readline type safety #440

nazmulidris · 2025-12-05T23:54:27Z

Summary

Fix SharedWriter indentation bug where subsequent log lines had an extra leading space (MoveRight(0) → MoveRight(1) issue)
Make readline code type-safe using bounds_check module with SegIndex/SegLength types
Switch LineState.line from String to GCStringOwned for pre-computed grapheme metadata

Changes

Bug Fix: SharedWriter Indentation (#439)

The ANSI/CSI cursor movement commands treat 0 as 1, causing MoveRight(0) to shift cursor by 1 position. Added a guard to skip cursor movement when distance is zero.

Type Safety: bounds_check Module

Replaced raw usize cursor positions with type-safe SegIndex and SegLength wrappers that provide:

Compile-time distinction between indices and lengths
Built-in bounds checking via ArrayBoundsCheck and CursorBoundsCheck traits
Overflow detection with ArrayOverflowResult

Performance: GCStringOwned Migration

Changed LineState.line from String to GCStringOwned:

O(1) segment count instead of O(n) grapheme parsing
O(1) display width via pre-computed metadata
Direct segment access with byte offsets and widths

Test plan

All 287 unit tests pass
All doctests pass
Documentation builds without warnings
Clippy clean

Fixes #439

…eRight(1)) Most terminals interpret cursor movement CSI commands with a parameter of 0 as 1 (per ANSI/VT-100 spec). This caused move_from_beginning to emit an unwanted MoveRight(1) when line_remaining_len was 0, resulting in log output gradually accumulating leading spaces. Fix: Guard MoveRight call to skip when value is 0. Add regression test using CsiSequence generators to verify: - to=0: Stay at origin, no movement emitted (was emitting MoveRight(1)) - to=240: Emits CursorDown(2) only, no CursorForward (tests boundary case) - to=5: Normal case, emits CursorForward(5) correctly Also add task/rl_async_update.md documenting Phase 2 modernization plan including a separate off-by-one bug in move_from_beginning boundary cases. Closes #439

Don't use usize and u16, and don't compare untyped values. Use bounds_check mod to ensure type safety is enforced and it is clear what is an index type (what is 1 based and 0 based) and what is a length type. 1) Add type-safe SegIndex for grapheme cursor positioning. Add Sub/SubAssign arithmetic ops and Display impl to SegIndex for proper type-safe grapheme index operations. Key changes: - SegIndex: Add Sub<SegIndex>, SubAssign<SegIndex>, Sub<SegLength>, SubAssign<SegLength>, and Display implementations with comprehensive tests - line_state: Migrate line_cursor_grapheme to SegIndex with CursorBoundsCheck (allows cursor at EOL position == length) - line_state: Migrate move_to_beginning/move_from_beginning params from u16 to ColWidth for type-safe display offset handling - line_state: Use NumericValue::is_zero() pattern instead of raw == 0 comparisons in handlers (handle_ctrl_u, handle_ctrl_w, etc.) 2) The CursorBoundsCheck trait is essential for text cursor positioning where index == length is valid (end of line), unlike array bounds where index must be < length. 3) Refactor line_state.rs monolith into modular structure: - core.rs: LineState struct, LineStateLiveness enum, early_return_if_paused! macro, and core tests - cursor.rs: Cursor movement, grapheme operations, move_to/from_beginning methods, and cursor tests - event_handlers.rs: Ctrl+key, Alt+key, and regular key handlers (handle_ctrl_d, handle_alt_b, etc.) with comprehensive tests - output.rs: Data printing, prompt management, and output tests - render.rs: Terminal rendering, clear/flush operations, and render tests - mod.rs: Private modules with public re-exports following codebase patterns 4) Update recommended extensions to add R3BL VSCode extension pack

Copilot

Pull request overview

This PR fixes a critical SharedWriter indentation bug and modernizes the readline implementation with type-safe coordinates. The bug caused MoveRight(0) to be interpreted as MoveRight(1) due to ANSI/CSI cursor movement commands treating 0 as 1. The migration introduces the bounds_check module with SegIndex/SegLength types and switches from String to GCStringOwned for pre-computed grapheme metadata, enabling O(1) segment operations.

Fixed off-by-one cursor positioning bug by adding zero-distance guard
Migrated to type-safe SegIndex/SegLength types with compile-time bounds checking
Replaced LineState.line: String with GCStringOwned for O(1) grapheme operations

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`tui/src/readline_async/readline_async_impl/readline.rs`	Updated test assertions to use `.as_str()` for `GCStringOwned`
`tui/src/readline_async/readline_async_impl/line_state/render.rs`	New module: rendering logic with zero-distance cursor movement guard
`tui/src/readline_async/readline_async_impl/line_state/output.rs`	New module: output handling and prompt management
`tui/src/readline_async/readline_async_impl/line_state/mod.rs`	New module structure with documentation on type-safe coordinates
`tui/src/readline_async/readline_async_impl/line_state/event_handlers.rs`	New module: keyboard event handlers using `GCStringOwned` segment access
`tui/src/readline_async/readline_async_impl/line_state/cursor.rs`	New module: cursor movement with zero-distance guard and type-safe coordinates
`tui/src/readline_async/readline_async_impl/line_state/core.rs`	New module: `LineState` struct with `GCStringOwned` and type-safe fields
`tui/src/readline_async/readline_async_impl/line_state.rs`	Removed entire file (replaced by modular structure)
`tui/src/core/graphemes/unicode_segment/seg_index.rs`	Added `Sub`, `SubAssign` implementations and `Display` trait for `SegIndex`
`task/rl_async_update.md`	New documentation of implementation plan and bug details
`.vscode/settings.json`	Added "codepaths" to spell checker dictionary
`.vscode/extensions.json`	Added R3BL VSCode extension recommendations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T23:55:24Z

tui/src/readline_async/readline_async_impl/line_state/cursor.rs

+    ///
+    /// The `pos` parameter is a display offset (column width) from the start of the line.
+    pub(crate) fn line_height(&self, pos: ColWidth) -> u16 {
+        pos.as_u16() / self.term_size.col_width.as_u16()


Division should account for exact boundary positions. When pos equals self.term_size.col_width * n for any positive integer n, the cursor is at the beginning of row n, but this calculation returns n. For example, position 80 on an 80-column terminal should be row 1 (one full row down), but 80 / 80 = 1 is correct. However, the comment on line 21 says 'Position 80 on 80-col terminal is Row 1, Col 0: 80/80 = 1 row' which matches the calculation. Review if this is the intended behavior or if it should use (pos.as_u16() + self.term_size.col_width.as_u16() - 1) / self.term_size.col_width.as_u16() for a proper ceiling division when dealing with 1-based positioning.

Copilot · 2025-12-05T23:55:24Z

tui/src/readline_async/readline_async_impl/line_state/event_handlers.rs

+    // If cursor is in middle or end, deletes from start to cursor.
+    if !line_state.line_cursor_grapheme.is_zero() {
+        // Get byte offset at cursor using segment metadata.
+        let cursor_byte_pos = get_byte_offset_at_seg_index(&line_state.line, line_state.line_cursor_grapheme.as_usize());


[nitpick] Line exceeds recommended 100 character limit (127 characters). Consider breaking this into multiple lines for better readability.

Suggested change

let cursor_byte_pos = get_byte_offset_at_seg_index(&line_state.line, line_state.line_cursor_grapheme.as_usize());

let cursor_byte_pos = get_byte_offset_at_seg_index(

&line_state.line,

line_state.line_cursor_grapheme.as_usize(),

);

Copilot · 2025-12-05T23:55:25Z

tui/src/readline_async/readline_async_impl/line_state/cursor.rs

+    }
+
+    #[test]
+    fn test_move_from_beginning_no_spurious_move_right_regression() {


[nitpick] Test function name is overly long (56 characters). Consider shortening to test_move_from_beginning_boundary_cases or test_move_from_beginning_no_spurious_movement while keeping the detailed explanation in a doc comment.

Suggested change

fn test_move_from_beginning_no_spurious_move_right_regression() {

/// Regression test: Ensure that `move_from_beginning` does not emit any

/// spurious rightward movement (CursorForward ANSI sequences) when moving

/// to the beginning of the line, especially for edge cases such as `to=0`

/// and exact multiples of the terminal width. This test checks that no

/// unnecessary ANSI sequences are produced.

fn test_move_from_beginning_no_spurious_movement() {

- New relative movement types to replace the use of u16 and problems w/ CSI interpretation of move by "0" to mean move by "1" - Use test-cases to make tests more readable

Make cursor movement in ANSI/CSI sequences type-safe by using TermRowDelta/TermColDelta (NonZeroU16 wrappers) instead of raw u16. This prevents the CSI zero bug where terminals interpret CSI 0 C as CSI 1 C (move 1 column instead of 0). Zero deltas are now unrepresentable at the type level. Changes: - CsiSequence cursor movement variants now require TermRowDelta/TermColDelta - Add fallible construction: new() returns Option, filtering zero values - Add ONE constant for the common "move by 1" case - Update all call sites to use if-let pattern for conditional emission - Update tests and conformance data for new type signatures

Add clearer semantics for index arithmetic and dimensional operations. Navigation vs Measurement distinction: - Index - Index = Index (navigation): Moving backward in position space with saturating subtraction to prevent underflow - index.distance_from(other) -> Length (measurement): Calculating the span between two positions Dimensional operators for Length types: - Length / Length -> u16 (dimensionless count) - Length % Length -> u16 (dimensionless remainder) - Length / u16 -> Length (scaling) These operators enable cleaner terminal wrapping calculations like `pos / term_width` for row count and `pos % term_width` for column. Documentation updates to check-bounds-safety skill and CLAUDE.md.

Implement three categories of "formalization": 1. Already using type, just removing extraction (e.g., ansi_sequence_generator.rs:96): - Before: term_col.as_u16() → extracted raw value to pass to enum - After: term_col → pass the type directly 2. Magic numbers → Named constants (e.g., output.rs): - Before: CursorHorizontalAbsolute(1) → hardcoded literal - After: CursorHorizontalAbsolute(TermCol::ONE) → semantic constant 3. Magic numbers → Enum variants (e.g., ansi_sequence_generator.rs:129): - Before: EraseDisplay(ED_ERASE_ALL) → u16 constant (value 2) - After: EraseDisplay(EraseDisplayMode::EntireScreen) → self-documenting enum Also refactor AnsiSequenceGenerator to use inner modules for logical grouping (replacing comment banners with formal mod blocks).

Remove redundant CHA(1) escape sequence after newline-terminated data segments in print_data_and_flush. This eliminates the [LF][CHA(1)][CHA(1)] pattern that caused visual artifacts (extra blank line before prompt) on some terminal emulators. The fix conditionally skips the CHA(1) write in the data loop for segments ending with newline, since the subsequent CHA(1) before render_and_flush handles cursor positioning. Closes #442

nazmulidris added 2 commits December 5, 2025 10:09

Copilot AI review requested due to automatic review settings December 5, 2025 23:54

nazmulidris self-assigned this Dec 5, 2025

Copilot AI reviewed Dec 5, 2025

View reviewed changes

nazmulidris mentioned this pull request Dec 5, 2025

r3bl_terminal_async SharedWriter Indentation Bug #439

Closed

nazmulidris force-pushed the terminal_async_bug_fix branch 2 times, most recently from 3ba257a to c5cb232 Compare December 6, 2025 00:08

[readline_async] Switch line_state data structures to GCStringOwned

5970d64

nazmulidris force-pushed the terminal_async_bug_fix branch from c5cb232 to 5970d64 Compare December 6, 2025 00:11

nazmulidris added 5 commits December 6, 2025 14:28

[readline_async] Introduce TermRowDelta and TermColDelta

6548352

- New relative movement types to replace the use of u16 and problems w/ CSI interpretation of move by "0" to mean move by "1" - Use test-cases to make tests more readable

nazmulidris merged commit 5d1282f into main Dec 7, 2025

nazmulidris deleted the terminal_async_bug_fix branch December 7, 2025 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix SharedWriter indentation bug and improve readline type safety #440

Fix SharedWriter indentation bug and improve readline type safety #440

Uh oh!

nazmulidris commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-    fn test_move_from_beginning_no_spurious_move_right_regression() {
+    /// Regression test: Ensure that `move_from_beginning` does not emit any
+    /// spurious rightward movement (CursorForward ANSI sequences) when moving
+    /// to the beginning of the line, especially for edge cases such as `to=0`
+    /// and exact multiples of the terminal width. This test checks that no
+    /// unnecessary ANSI sequences are produced.
+    fn test_move_from_beginning_no_spurious_movement() {

Uh oh!

Fix SharedWriter indentation bug and improve readline type safety #440

Fix SharedWriter indentation bug and improve readline type safety #440

Uh oh!

Conversation

nazmulidris commented Dec 5, 2025

Summary

Changes

Bug Fix: SharedWriter Indentation (#439)

Type Safety: bounds_check Module

Performance: GCStringOwned Migration

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant