Repository navigation
fix(prompts): end an interrupted spinner with the signal status - #3432
jong-kyung wants to merge 6 commits into
Conversation
clack's block() calls process.exit(0) on Ctrl+C and Escape, so an interrupted command reported success. Replace it with a local input block that stops the spinner and sends SIGINT to the foreground process group, as the terminal does outside raw mode. Windows has no signals, so the process exits with 130 there. Escape and other keys are ignored, as the Rust progress row does. A key pressed just before the spinner stops no longer leaves a listener on stdin. Refs voidzero-dev#3429
An active spinner listened for SIGINT and SIGTERM, stopped itself, and returned, so the command kept running after a termination request. Raise the signal again after stopping, but only when no other listener owns it: none was registered before the spinner started and none was added while it spun. An emit without the signal name only stops the spinner. Windows exits with the matching status instead. Closes voidzero-dev#3429
|
@codex review |
Codex Review Summary: ✅ CompletedThis 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: 19eee2b1a4
ℹ️ 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".
The Ctrl+C and Escape tests expected process.kill, which only the POSIX branch calls. On Windows the spinner exits with 130 instead, so the tests failed there. Pin a POSIX platform unless a test asks for Windows.
Each active spinner blocked input, so two spinners both received a single Ctrl+C and each sent SIGINT to the process group. A shutdown handler then saw two interrupts for one key press. Let only one spinner block input at a time, as the Rust progress row does. The others receive the forwarded signal through their signal listeners.
The Ctrl+C test waited for the worker's PID to disappear. Where nothing reaps orphans, as in some Linux containers, the worker stays a zombie after the group is interrupted, so the test timed out although the worker had ended. Read the process state on Linux and accept a zombie.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
A spinner remembered that another listener had been registered for a signal and never forgot it. After that listener was removed, a signal only stopped the spinner and the command kept running. Check the registered listeners when the signal arrives instead. Node removes a one-shot listener just before calling it, so a removal during the current delivery still counts as another owner, and its cleanup is not cut short.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
jong-kyung
added
test: e2e
Closes #3429. Supersedes #3345.
Summary
The TypeScript spinner reported an interrupt as success. This PR makes it end by the signal, as the Rust progress row does, so the shell reports 130 or 143.
Changes
block(), which callsprocess.exit(0). It stops the spinnerand sends
SIGINTto the foreground process group.SIGINTandSIGTERM: the spinner raises the signal again after it stops, but only when no other listenerowns the signal.