multiline input

[navicore]

Summary of changes:

#177

1. crates/repl/src/app.rs:
   - Added Shift+Enter handler that detects SHIFT or ALT modifiers (macOS terminals report Shift+Enter as ALT)
   - Also handles Char('\n') for terminals that send that instead
   - Multiline input rendering with .... continuation prompt
2. crates/repl/src/ui/repl_pane.rs:
   - Rewrote build_lines() to split input by newlines and render each line separately
   - Cursor position correctly tracked across multiple lines
   - History entries also handle multiline display
   - Enabled text wrapping with proper scroll calculation
3. crates/repl/src/main.rs:
   - Added keyboard enhancement flags for better modifier detection

The REPL now supports:

• Shift+Enter to insert newlines for multiline input
• Text wrapping for long lines
• Continuation prompt (.... ) for multiline entries

[claude]

PR Review: Multiline Input Support

Thank you for implementing multiline input support for the REPL! This is a valuable feature that enhances the user experience. Here's my detailed review:

✅ Strengths

1. Well-documented implementation: Clear comments explaining terminal behavior differences (Shift+Enter as SHIFT/ALT modifiers)
2. Comprehensive builtin completions: Excellent expansion of the completion list covering all major language features
3. Good test coverage: Two new tests for tab completion functionality
4. Cross-platform consideration: Handling macOS Terminal/iTerm quirks with ALT modifier detection

---

🐛 Potential Bugs

Critical: Cursor Position Logic Issue (crates/repl/src/ui/repl_pane.rs:293-309)

The cursor position calculation has a subtle bug when the cursor is at the end of input:

for (i, line_text) in input_lines.iter().enumerate() {
    let line_end = pos + line_text.len();
    if self.state.cursor <= line_end {  // ⚠️ Problem here
        line_idx = i;
        col = self.state.cursor - pos;
        break;
    }
    pos = line_end + 1;
    line_idx = i + 1;  // This executes after the last line
}

Issue: When cursor is past all lines (e.g., at position after the final newline), line_idx can become input_lines.len(), causing potential out-of-bounds access at line 308: input_lines.get(line_idx).

Test case that would fail:

• Input: "foo\n" (cursor at position 4, after the newline)
• Expected: cursor on line 1, column 0
• Actual: line_idx = 1, but input_lines.len() = 1 (only one non-empty line after split)

Suggested fix:

let mut line_idx = 0;
let mut col = self.state.cursor;
let mut pos = 0;
for (i, line_text) in input_lines.iter().enumerate() {
    let line_end = pos + line_text.len();
    if self.state.cursor <= line_end {
        line_idx = i;
        col = self.state.cursor - pos;
        break;
    }
    pos = line_end + 1;
}
// Clamp line_idx to valid range
line_idx = line_idx.min(input_lines.len().saturating_sub(1));

---

⚠️ Code Quality & Best Practices

1. Magic String Duplication (crates/repl/src/ui/repl_pane.rs:257, 266)

let continuation_prompt = ".... ";  // Also appears in prompt logic

Recommendation: Define as a module constant or struct field to avoid inconsistencies:

const CONTINUATION_PROMPT: &str = ".... ";

2. Complex Boolean Expression (crates/repl/src/app.rs:350-353)

let is_modified_enter = key.code == KeyCode::Enter
    && (key.modifiers.contains(KeyModifiers::SHIFT)
        || key.modifiers.contains(KeyModifiers::ALT));
let is_newline_char = key.code == KeyCode::Char('\n');

Good: Clear variable names and comments explaining terminal differences

3. Potential Integer Overflow (crates/repl/src/ui/repl_pane.rs:373-377)

let wrapped_height: u16 = lines
    .iter()
    .map(|line| {
        let line_width: usize = line.spans.iter().map(|s| s.content.chars().count()).sum();
        line_width.max(1).div_ceil(width) as u16  // ⚠️ Potential overflow
    })
    .sum();

Issue: If terminal is very narrow or content is very long, as u16 cast could overflow.
Suggestion: Use saturating arithmetic:

.map(|line| line_width.max(1).div_ceil(width).min(u16::MAX as usize) as u16)
.fold(0u16, |acc, h| acc.saturating_add(h))

---

🔒 Security Considerations

No significant security concerns identified. The changes handle user input safely:

• No unsafe code blocks
• No command injection vectors
• Proper string boundary checks (e.g., floor_char_boundary usage)

---

⚡ Performance Considerations

String Splitting on Every Render (crates/repl/src/ui/repl_pane.rs:290)

let input_lines: Vec<&str> = self.state.input.split('\n').collect();

Impact: This happens on every render. For large multiline inputs, this could be noticeable.
Suggestion: Consider caching split lines in ReplState and invalidating on input change.

Character Counting in Scroll Calculation (crates/repl/src/ui/repl_pane.rs:374)

let line_width: usize = line.spans.iter().map(|s| s.content.chars().count()).sum();

Impact: .chars().count() iterates through the string. For very long lines, this adds up.
Minor optimization: Most UI libraries provide width calculations; verify if ratatui offers a helper.

---

🧪 Test Coverage

Good: Added two unit tests for tab completion
Missing: No tests for the core feature of this PR (multiline input!)

Recommended test cases:

1. Basic multiline insertion:

   • Enter insert mode, type "foo", press Shift+Enter, type "bar"
   • Assert input is "foo\nbar"
   • Assert cursor position is correct

2. Cursor tracking across lines:

   • Create multiline input, move cursor with h/l keys
   • Verify cursor position after crossing newline boundaries

3. Multiline rendering:

   • Create ReplPane with multiline input
   • Verify continuation prompt appears on subsequent lines
   • Verify cursor renders on correct line

4. Edge cases:

   • Empty lines between content ("foo\n\nbar")
   • Cursor at end of line vs. beginning of next line
   • Single newline at end of input

---

📝 Documentation

Missing: No updates to user-facing documentation about Shift+Enter functionality. Consider updating:

• README.md section on REPL (lines 86-112)
• Any REPL user guide

---

🎯 Summary

Overall Assessment: This is a solid implementation with good cross-platform awareness. The main concerns are:

1. Critical: Cursor position bug that could cause panics
2. Important: Missing test coverage for the primary feature
3. Minor: Performance optimizations for rendering

Recommendation: Request changes to fix the cursor bug and add multiline-specific tests before merging.

---

Code Quality: ⭐⭐⭐⭐ (4/5)
Test Coverage: ⭐⭐⭐ (3/5)
Documentation: ⭐⭐ (2/5)
Overall: ⭐⭐⭐⭐ (4/5) - Good work with room for improvement!

[claude]

Code Review - PR #184: Multiline Input Support

Summary

This PR successfully implements multiline input support for the REPL with Shift+Enter functionality, along with comprehensive test coverage and documentation updates. The implementation addresses issue #177 and includes important bug fixes discovered during development.

---

✅ Strengths

1. Excellent Bug Fixes

• Critical fix in repl_pane.rs:310: Cursor position is properly clamped to prevent out-of-bounds access with trailing newlines:

  line_idx = line_idx.min(input_lines.len().saturating_sub(1));

• Overflow protection in repl_pane.rs:381: Using saturating_add() prevents potential overflow in wrapped height calculation
• Both of these demonstrate defensive programming and attention to edge cases

2. Cross-Platform Compatibility

The implementation handles terminal inconsistencies gracefully (app.rs:346-353):

• SHIFT modifier detection
• ALT modifier (macOS terminals report Shift+Enter as ALT)
• Direct Char('\n') handling
• Keyboard enhancement flags with proper fallback (main.rs:82-85)

This is well-documented and shows strong awareness of real-world terminal behavior.

3. Test Coverage

Four new multiline tests added:

• test_multiline_input_rendering - validates multiline rendering
• test_cursor_position_trailing_newline - critical edge case
• test_cursor_position_empty_lines - handles empty lines in multiline
• test_multiline_history_entry - ensures history works with multiline

The tests target the exact bugs that were fixed, which is excellent practice.

4. Code Organization

• CONTINUATION_PROMPT constant (repl_pane.rs:18) eliminates magic string duplication
• Clear separation between multiline handling and single-line logic
• Good inline comments explaining terminal behavior differences

---

🔍 Issues & Concerns

1. Performance: Redundant String Splitting (Medium Priority)

In repl_pane.rs:292-315, the cursor position calculation splits the input string and iterates through lines:

let input_lines: Vec<&str> = self.state.input.split('\n').collect();
// ... cursor calculation loop ...
for (i, line_text) in input_lines.iter().enumerate() { // Line 299

Then immediately after at line 318, we iterate through input_lines again to render:

for (i, line_text) in input_lines.iter().enumerate() { // Line 318

Issue: For every render (which happens frequently in a TUI), we're splitting the input string and iterating twice.

Impact:

• Minor for typical REPL input (< 100 lines)
• Could become noticeable with very large multiline inputs
• The split itself allocates a new Vec<&str> on every render

Recommendation: Consider combining the cursor calculation and rendering into a single pass, or cache the split result if self.state.input hasn't changed.

2. Potential Panic in Cursor Position Calculation (Low-Medium Priority)

In repl_pane.rs:311:

let col = col.min(input_lines.get(line_idx).map_or(0, |l| l.len()));

While the line index is clamped at line 310, there's a subtle logic issue:

The loop at line 299-308 sets line_idx = i + 1 when the cursor is past a line. If the cursor is at the very end of the input (after the last character), line_idx could become input_lines.len(), which would then get clamped to input_lines.len() - 1.

However, the col variable at this point would be from the previous iteration and wouldn't accurately represent the column in the clamped line.

Example scenario:

• Input: "foo\nbar" (cursor at position 7, after "bar")
• After loop: line_idx = 2, col = 7 - 4 = 3
• After clamp: line_idx = 1 (the "bar" line)
• col = 3.min(3) = 3 ✓ (happens to work)

But with trailing newline:

• Input: "foo\n" (cursor at position 4, after newline)
• After loop: line_idx = 2, col = 4 - 4 = 0
• After clamp: line_idx = 1 (empty string after split)
• col = 0.min(0) = 0 ✓ (works, but the logic is confusing)

Recommendation: Recalculate col after the clamp, or restructure the logic to be more explicit about the cursor being at the end of input.

3. Expanded Builtin Completions List (Minor)

The builtin_completions() function (app.rs:1319-1528) grew from ~40 to ~200 lines.

Observations:

• This appears to be a separate enhancement (not mentioned in PR description)
• The expansion is good for completeness
• However, it's hard-coded and will need manual maintenance

Questions:

• Should this be generated from the actual language builtins?
• Is there a risk of the completion list getting out of sync with actual available functions?

Recommendation: Consider adding a comment or TODO about potentially auto-generating this from the compiler's builtin definitions in the future.

4. Missing Edge Case Test (Low Priority)

Test coverage gap: No test for cursor navigation within multiline input (e.g., moving cursor with arrow keys between lines).

While the rendering is tested, it would be valuable to verify that:

• Cursor correctly updates when moving between lines
• Boundary conditions (first char of line, last char of line) work correctly

---

🔒 Security Considerations

✅ No security concerns identified. The code:

• Properly handles user input without injection risks
• Uses safe Rust with bounds checking
• No unsafe blocks introduced

---

🎯 Performance Considerations

• ⚠️ Minor performance concern with double iteration (see Issue Add Claude Code GitHub Workflow #1)
• The wrapped height calculation could be expensive for very long history with many wrapped lines
• Consider profiling with large history buffers (1000+ entries)

---

📝 Code Quality

Overall: Good

Positives:

• Clear variable names (cursor_line, cursor_col, line_idx)
• Helpful comments explaining non-obvious behavior
• Consistent style with existing codebase
• Proper error handling with saturating_* operations

Minor style notes:

• Line 310 is quite long; consider breaking it up for readability
• The cursor calculation logic in build_lines() is complex - consider extracting to a helper method calculate_cursor_position()

---

📚 Documentation

✅ Excellent: README updated with clear Editing section documenting Shift+Enter, Tab, and F1/F2/F3 keys

---

🧪 Test Coverage

✅ Good: 4 new tests covering the multiline functionality and edge cases

Suggestions:

1. Add a test for very long multiline input (e.g., 100+ lines) to verify performance
2. Add a test for cursor movement between lines (if that's implemented)
3. Consider property-based testing for cursor position calculations

---

🎯 Recommendations

High Priority

None - the critical bugs are already fixed!

Medium Priority

1. Optimize build_lines(): Combine the two iterations over input_lines into a single pass
2. Clarify cursor calculation: Restructure the cursor position logic to be more explicit about edge cases

Low Priority

1. Add a helper method to extract cursor calculation logic
2. Add cursor navigation tests
3. Add a TODO comment about auto-generating builtin completions

---

✨ Final Assessment

Overall: LGTM with minor suggestions

This is a well-implemented feature with:

• ✅ Clear implementation of the requested functionality
• ✅ Good test coverage targeting critical edge cases
• ✅ Proactive bug fixes (trailing newline, overflow)
• ✅ Cross-platform terminal compatibility
• ✅ Updated documentation

The performance and logic clarity concerns are minor and don't block merging. They could be addressed in a follow-up PR if desired.

Great work on the defensive programming and comprehensive testing! 🎉