Audit vim line

[navicore]

No description provided.

[claude]

Code Review: Audit vim line

Overview

This PR executes a well-documented modularisation of crates/vim-line/src/vim.rs (originally 1379 lines), splitting it into:

• vim/motions.rs — pure cursor-motion functions (290 lines)
• vim/edits.rs — stateful edit operations (173 lines)
• vim/tests.rs — extracted test suite (381 lines)
• docs/design/done/VIM_LINE_SPLIT.md — design rationale

The PR implements passes 1 & 2 of a three-pass plan and deliberately defers per-mode handler extraction to a future PR. The overall direction is good.

---

What Works Well

• Excellent module docs. Each new file opens with a clear purpose statement. Future contributors will immediately know where to look.
• Pure motions module. Every function in motions.rs has the signature fn(cursor, text) -> usize — zero side effects. This is great for testability and portability.
• dispatch_motion() consolidation. Normal mode and Visual mode previously duplicated the same motion match arms. The new helper eliminates that repetition cleanly.
• Proper visibility stratification. pub(in crate::vim) on struct fields, pub(super) on impl methods — limits blast radius correctly.
• All 21 tests preserved verbatim. No coverage was lost. Unicode boundary tests, empty-buffer edge cases, and bracket matching all still run.

---

Concerns

1. Verify public API breakage (action required)

Three items changed visibility and may break downstream crates:

Item	Was	Now
Mode enum	pub	pub(crate)
Operator enum	pub	pub(crate)
VimLineEditor::mode()	pub fn	#[cfg(test)] fn

The design doc says these were never part of the external contract, but please verify against crates/vim-line/src/lib.rs to confirm none of these are re-exported. Also run cargo build on the downstream crates (repl, etc.) to catch any usage outside this crate.

2. Struct fields are now visible to all sibling modules (pub(in crate::vim))

Making cursor, mode, visual_anchor, and yank_buffer visible to edits.rs and motions.rs is a pragmatic choice, but it means any code added to those files can directly mutate cursor instead of going through the motion methods. The current code is well-behaved, but the constraint is only social. If this is expected to stay, a brief comment would help; otherwise, accessor methods would make the invariant enforced by the compiler.

3. find_matching_forward / find_matching_backward elevated from method to free function

These were previously private methods on VimLineEditor. Now they're pub(super) free functions in motions.rs, making them callable from edits.rs and tests.rs. This isn't wrong, but it widens the "internal public surface" slightly. Worth noting so they don't accidentally get repurposed.

4. Missing edge-case test for mismatched brackets

The tests cover % on a matching pair and % on a non-bracket character, but not the case where % is pressed on an opening bracket that has no match (e.g., fn foo( // no closing paren). The function returns cursor in that case, which is correct vim behaviour — but a test would lock that in.

5. apply_operator_line silently recalculates line boundaries

In edits.rs, apply_operator_line recalculates line_start/line_end inline rather than delegating to the motion helpers. The reason (Normal-mode cursor semantics vs. full byte-range semantics) is valid, but it's non-obvious. A one-line comment would save the next reader from wondering why the helpers weren't used.

---

Minor Observations

• paste_after in edits.rs clamps the post-paste cursor with .min(text.len() + to_insert.len()). The reasoning is subtle (text hasn't grown yet at the point of calculation). Correct, but a brief comment would help.
• move_line_end_insert is only called from Insert mode context. The name implies this — no issue, just noting it's safe as-is.
• The design doc accurately records the line counts and constraints. Good practice.

---

Summary

Area	Verdict
Refactor scope	✅ Matches plan; well-scoped
Code quality	✅ Clean separation of concerns, good docs
Correctness	✅ No regressions detected
Public API	⚠️ Needs verification against lib.rs + downstream build
Test coverage	✅ All tests preserved; minor gap on mismatched %
Performance	✅ Neutral — no new allocations
Security	✅ UTF-8 boundary checks preserved

The core refactor is solid. The main ask is to verify the visibility changes don't silently break anything outside this crate. Once that's confirmed, this looks ready to merge.