fix(multiselect): stop menu from drifting up on each keypress#156
Conversation
…keypress `reposition_and_write` was moving the cursor up by `last_line_count` to return to the top of the previous render, but the redraw loop emits no trailing newline after the last line — so the cursor sits at the *end* of row N-1, not at row N. Moving up by N landed one row above the prior top, and every keypress shifted the menu up by another row (visible in `mise up -i` and any other multi-iteration `MultiSelect` redraw). Fix by moving up `last_line_count - 1`, matching the invariant the pre-#129 code expressed via `height = lines().count() - 1`. The shrink branch had the same off-by-one in mirrored form; restructure it so the cursor lands on the last line of the new render, preserving the same invariant for the next iteration. Adds two regression tests that drive the redraw path through a `Term::read_write_pair` whose writer captures bytes into a shared `Vec`, replays the bytes through a `vt100` emulator, and asserts that the cursor row is stable across iterations (drift case) and repositions correctly when the render shrinks. Both tests fail without the fix — the drift trace `[26, 25, 24, 23, 22, 21]` is the smoking gun. Drive-by clippy fixes across the workspace (collapsible match in select, needless `vec!` in spinner test, needless `return`/borrow in examples and the non-TTY integration test) so `cargo clippy -- -D warnings` passes again on stable.
There was a problem hiding this comment.
Code Review
This pull request addresses a cursor drift issue in the MultiSelect component by fixing an off-by-one error in the reposition_and_write method. It also refactors several example files to simplify result handling, updates Select to use match guards for enter key processing, and introduces comprehensive regression tests for terminal rendering using the vt100 crate. Feedback suggests further simplifying the logic for clearing extra lines when a render shrinks by utilizing clear_to_end_of_screen instead of a manual loop.
| let extra = self.last_line_count - line_count; | ||
| for _ in 0..extra { | ||
| writeln!(self.term)?; | ||
| self.term.move_cursor_left(usize::MAX)?; | ||
| self.term.clear_line()?; | ||
| if i < (self.last_line_count - line_count - 1) { | ||
| writeln!(self.term)?; | ||
| } | ||
| } | ||
| self.term | ||
| .move_cursor_up(self.last_line_count - line_count)?; | ||
| // Leave the cursor on the last line of the new render so the | ||
| // invariant above (cursor at end of row N-1) still holds. | ||
| self.term.move_cursor_up(extra)?; |
There was a problem hiding this comment.
The logic for clearing extra lines when the render shrinks can be simplified by using clear_to_end_of_screen(). Since the cursor is already at the end of the last line of the new render, clearing to the end of the screen will remove all leftover lines from the previous render without needing a loop or manual cursor repositioning. This approach is more efficient and consistent with other parts of the library, such as handle_stop_filtering.
| let extra = self.last_line_count - line_count; | |
| for _ in 0..extra { | |
| writeln!(self.term)?; | |
| self.term.move_cursor_left(usize::MAX)?; | |
| self.term.clear_line()?; | |
| if i < (self.last_line_count - line_count - 1) { | |
| writeln!(self.term)?; | |
| } | |
| } | |
| self.term | |
| .move_cursor_up(self.last_line_count - line_count)?; | |
| // Leave the cursor on the last line of the new render so the | |
| // invariant above (cursor at end of row N-1) still holds. | |
| self.term.move_cursor_up(extra)?; | |
| self.term.clear_to_end_of_screen()?; |
Greptile Summary
Confidence Score: 5/5Safe to merge — the fix is narrowly scoped, the invariant is clearly stated and verified by two targeted regression tests, and no other code paths are affected. No P0 or P1 issues found. The off-by-one correction is correct and well-explained. The shrink branch restructuring preserves the same cursor invariant. Clippy/example cleanups are straightforward. The vt100-backed tests provide genuine regression coverage for the specific bug reported. No files require special attention. Reviews (2): Last reviewed commit: "ci: add test-ci aggregator job for branc..." | Re-trigger Greptile |
Mirrors the pattern in jdx/mise — a single one-minute job that needs the matrix `test` job and re-emits its result. Lets branch protection gate on one check (`test-ci`) instead of all three matrix instances (ubuntu/macos/windows), so adding or removing platforms doesn't require rule updates. Uses `if: !cancelled()` rather than `always()` so workflow cancellation doesn't spin up a wasted runner.
Summary
mise up -ishifts the multiselect menu up by one row on every keypress (jdx/mise discussion #9612). Bisects to make multiselect dialogue rendering more efficient #129, which rewrotereposition_and_writeto skip a full clear/redraw in favor of a cursor-up + line-by-line rewrite.last_line_countto return to the top of the previous frame, but the rewrite loop emits no trailing newline after the last line — so the cursor sits at the end of row N-1, not at row N. Moving up by N lands one row above the prior top, and the menu drifts up one row per iteration.last_line_count - 1, restoring the invariant the pre-make multiselect dialogue rendering more efficient #129 code expressed viaheight = lines().count() - 1. The shrink branch had the same off-by-one in mirrored form; restructured so the cursor lands on the last line of the new render and the same invariant holds for the next iteration.Tests
src/multiselect.rs. They drivereposition_and_writethrough aTerm::read_write_pairwhose writer captures escape codes into a sharedVec, replay the bytes through avt100emulator, and assert on the resulting screen state.reposition_does_not_drift_upward— runs 6 redraw iterations and asserts the cursor row is stable. Without the fix, the cursor row trace is[26, 25, 24, 23, 22, 21](1 row up per iter); with the fix it's constant.reposition_handles_shrinking_render— verifies the cursor lands on the last line of the new (shorter) render and stays stable on the iteration after the shrink.#[cfg(unix)]becauseTerm::read_write_pairitself is#[cfg(unix)]in the console crate. The fix itself is platform-agnostic.Drive-bys
cargo clippy -- -D warningsregressions had crept in (collapsible match inSelect,vec!in aSpinnertest, needlessreturn/borrow across examples and the non-TTY integration test). Fixed alongside so the lint job stays green.console0.16.1 → 0.16.3 viacargo update -p console(already allowed by the"0.16"requirement). Affects only the resolved version, no API changes used bydemand.vt100 = "0.16"as a dev-dependency for the new tests.Test plan
cargo test --lib --tests(matches CI)cargo clippy --all-targets --all-features -- -D warningscargo fmt --checkmise up -iafter this lands in ademandrelease🤖 Generated with Claude Code
Note
Medium Risk
Touches interactive terminal redraw logic in
MultiSelect, where small off-by-one mistakes can cause visible UI regressions across terminals. Risk is mitigated by adding targeted unix-only vt100 regression tests covering repeated redraws and shrinking output.Overview
Fixes a
MultiSelectredraw regression where repeated keypresses could shift the menu upward by one row due to an off-by-one in cursor repositioning, and adjusts the shrink-output cleanup to preserve the same cursor invariant.Adds unix-only vt100-backed regression tests (via new
vt100dev-dependency) to assert redraw stability across iterations and after the rendered output shrinks. Includes small clippy/ergonomics cleanups in examples/tests and refactorsSelect’s enter handling, plus a CI workflowtest-cigate job to fail the workflow if any matrixtestjob fails or is skipped.Reviewed by Cursor Bugbot for commit 0270219. Bugbot is set up for automated code reviews on this repo. Configure here.