Repository navigation
fix(progress): block keyboard input while a Rust spinner is visible - #3373
jong-kyung wants to merge 2 commits into
Conversation
Add Progress::counter to vp_shared so global install and update share the same progress row as other Rust spinners.
Typing during a Rust spinner echoed into the progress row, and each Enter pushed a copy of the row down. Switch stdin to Node's raw mode while a new row is visible, as clack's block() does, but keep ISIG so Ctrl+C, Ctrl+\, and Ctrl+Z still signal the whole process group. Pending input is discarded on release so it never reaches the shell. SIGINT, SIGQUIT, SIGTERM, Ctrl+C on Windows, and panics restore the saved terminal state before the process ends. A background job leaves the terminal alone, because changing or flushing its controlling terminal would stop it with SIGTTOU. Closes voidzero-dev#3359
jong-kyung
added
test: e2e
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a895deda3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !block(libc::STDIN_FILENO) { | ||
| return None; | ||
| } | ||
| BLOCKED.store(true, Ordering::SeqCst); | ||
| Some(Self { previous: install_handlers() }) |
There was a problem hiding this comment.
On Unix, a SIGTERM arriving after block() changes termios but before install_handlers() registers its handler still takes the default termination path. Neither Drop nor the panic hook runs on that path, so the terminal is left with echo, canonical input, and carriage-return translation disabled. This is reproducible by signaling a PTY-backed process during guard acquisition. The signal test only raises signals after acquisition completes, so it misses this window. Arm restoration safely before modifying the terminal, or otherwise synchronize this transition with signal delivery.
Useful? React with 👍 / 👎.
Summary
Pressing Enter during a Rust spinner, such as
Installing global packagesinvp install -g, pushed a copy of the spinner row down with each key press. The local CLI already avoids this because clack'sblock()suppresses input while its spinner runs, so this PR gives the Rust spinners the same behavior.Closes #3359
Root cause
The terminal stays in its normal line-editing mode while the spinner is visible, so it echoes typed keys. Each echoed newline moves the cursor down, and indicatif then redraws the spinner on the new line.
Changes
While a new progress row is visible,
vp_shared::progressnow switches stdin to raw input and restores it when the row is cleared. This follows clack'sblock()with two differences:The terminal is restored on every exit path:
The global package spinners for
vp install -gandvp outdated -gpreviously built their own indicatif bars, which bypassed the shared progress module. They now use a newProgress::counter, so the input block applies to them as well. The counter keeps the existing(n/total)display and now also shows the elapsed time, like the other Rust spinners. This is why two snapshots gained<duration>.