cargo build -p vim-line (no features) — clean; core stays dependency/IO-free
cargo build -p vim-line --features history -p seq-repl — clean
cargo test -p vim-line --features history — 46 + 1 pass
cargo test -p seq-repl — 82 pass
cargo clippy --features history --all-targets -- -D warnings — clean
Purity checkpoint: grep std::fs|std::io crates/vim-line/src → empty ✓
Persistence round-trip: save_history writes store.entries(), and load_history seeds the store via add_entry→store.push at startup — no restart regression ✓
Only seq-repl consumes vim-line, so enabling the history feature can't disturb another consumer ✓

Design / correctness

The two-collection split in ReplState is the right call and is documented well: history = rendered transcript (dups/failures preserved for display), store = deduped navigation ring. Moving the k/j-at-boundary logic into vim-line (and deleting the host's handle_normal_jk shim + text_utils::floor_char_boundary) is a real simplification, and the boundary helpers are correct:

Char-boundary risk checked: is_on_first_line/is_on_last_line slice with the raw byte cursor, which is safe because vim.rs:115 normalizes the incoming cursor to a char boundary at the public entry point, and every existing motion already relies on that invariant. The new helpers are even more defensive (.min(text.len())). Not a regression.
prev never returns Recall::Draft, so history_up matching only Entry is correct; history_down correctly handles Entry/Draft/None. Draft-stash semantics are covered by tests.
Visual-mode j/k staying motion-only, and arrows being mode-independent history nav, are both explicitly tested.

Minor observations (non-blocking)

save_history content semantics changed — from "last 1000 transcript lines (with duplicates)" to "deduped store.entries()". Intended and documented; the on-disk format (newline-delimited) is unchanged, so old history files still load. Flagging it as a user-visible change (the saved file is now deduped).
Two seeding paths exist — Store::load(iter) is provided for hosts, but seqr seeds via per-line add_entry/push (because it must build the transcript in parallel). Justified; Store::load is currently exercised only by tests here and will earn its keep when plgr migrates. Fine to keep as published API.
Reserved Action variants (HistorySearch/HistoryAccept/HistoryCancel) aren't emitted yet — the exhaustive no-op match arm in app.rs is the right way to reserve the vocabulary without dead-code churn.

Nicely done — the design doc, the purity discipline, and the migration-as-proof framing are all solid.

## Review: Approve ✅ Clean, well-factored change with strong docs and test coverage. No blocking issues. ### Verified (on `vim-line2`) - `cargo build -p vim-line` (no features) — clean; core stays dependency/IO-free - `cargo build -p vim-line --features history -p seq-repl` — clean - `cargo test -p vim-line --features history` — 46 + 1 pass - `cargo test -p seq-repl` — 82 pass - `cargo clippy --features history --all-targets -- -D warnings` — clean - Purity checkpoint: `grep std::fs|std::io crates/vim-line/src` → empty ✓ - Persistence round-trip: `save_history` writes `store.entries()`, and `load_history` seeds the store via `add_entry`→`store.push` at startup — no restart regression ✓ - Only `seq-repl` consumes vim-line, so enabling the `history` feature can't disturb another consumer ✓ ### Design / correctness The two-collection split in `ReplState` is the right call and is documented well: `history` = rendered transcript (dups/failures preserved for display), `store` = deduped navigation ring. Moving the `k`/`j`-at-boundary logic into vim-line (and deleting the host's `handle_normal_jk` shim + `text_utils::floor_char_boundary`) is a real simplification, and the boundary helpers are correct: - Char-boundary risk checked: `is_on_first_line`/`is_on_last_line` slice with the raw byte `cursor`, which is safe because `vim.rs:115` normalizes the incoming cursor to a char boundary at the public entry point, and every existing motion already relies on that invariant. The new helpers are even more defensive (`.min(text.len())`). Not a regression. - `prev` never returns `Recall::Draft`, so `history_up` matching only `Entry` is correct; `history_down` correctly handles `Entry`/`Draft`/`None`. Draft-stash semantics are covered by tests. - Visual-mode `j`/`k` staying motion-only, and arrows being mode-independent history nav, are both explicitly tested. ### Minor observations (non-blocking) 1. **`save_history` content semantics changed** — from "last 1000 transcript lines (with duplicates)" to "deduped `store.entries()`". Intended and documented; the on-disk format (newline-delimited) is unchanged, so old history files still load. Flagging it as a user-visible change (the saved file is now deduped). 2. **Two seeding paths exist** — `Store::load(iter)` is provided for hosts, but seqr seeds via per-line `add_entry`/`push` (because it must build the transcript in parallel). Justified; `Store::load` is currently exercised only by tests here and will earn its keep when `plgr` migrates. Fine to keep as published API. 3. **Reserved `Action` variants** (`HistorySearch`/`HistoryAccept`/`HistoryCancel`) aren't emitted yet — the exhaustive no-op match arm in `app.rs` is the right way to reserve the vocabulary without dead-code churn. Nicely done — the design doc, the purity discipline, and the migration-as-proof framing are all solid.

navicore added 1 commit

2026-06-16 03:33:42 +00:00

doc

CI - Linux / CI - Linux x86_64 (pull_request) Successful in 8m12s

Details

c39a462c23

navicore merged commit 575ba903f8 into main

2026-06-16 03:42:50 +00:00

navicore deleted branch vim-line2

2026-06-16 03:42:50 +00:00

navicore referenced this pull request from a commit

2026-06-16 03:42:51 +00:00