-
Notifications
You must be signed in to change notification settings - Fork 11
fix: improve non-TTY support, input parsing, and ambiguous key handling for confirm dialog #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jdx
wants to merge
13
commits into
main
Choose a base branch
from
fix/non-tty-check
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When stderr is not a TTY (e.g., in CI environments), attempting to render interactive prompts with terminal control sequences generates massive noise from cursor movement, colors, and other escape codes. This change updates Input and Confirm to: 1. Check if both stdin AND stderr are terminals before entering interactive mode 2. Fall back to simple text prompts (without terminal control) when not in a TTY 3. Write prompts to stderr and read responses from stdin This fixes the issue where programs using demand (like Bitwarden CLI) would flood CI logs with hundreds of lines of escape sequences when trying to prompt for passwords in non-interactive environments. The simple fallback prompts still work correctly, they just don't use any terminal control sequences that would create noise in logs.
Created a new tty module with shared helpers: - is_tty(): Check if both stdin and stderr are terminals - write_prompt(): Write simple text prompts to stderr - read_line(): Read and strip line endings from stdin This eliminates ~30 lines of duplicated code between input.rs and confirm.rs, making the non-TTY handling more maintainable and consistent across different prompt types.
Replaced unwrap() with unwrap_or() to provide default characters:
- Confirm: 'y' for affirmative, 'n' for negative
- DialogButton: 'x' for empty labels
This fixes panics when users set empty strings via the public API:
- Confirm::new("title").affirmative("")
- DialogButton::new("")
The fix applies to all code paths (TTY and non-TTY).
8c55274 to
e2b9279
Compare
e2b9279 to
6a2b50c
Compare
Previously, the confirm dialog would accept hardcoded "y"/"yes"/"n"/"no" responses regardless of custom labels. This created confusing behavior where users could enter "yes" even when the prompt displayed custom labels like "Confirm/Cancel" with "c/c" as the expected characters. Now the input parsing only accepts responses that match the custom labels: - First character of the label (e.g., "c" for "Confirm") - Partial matches (e.g., "conf" for "Confirm") - Full label text (e.g., "confirm" for "Confirm") Added comprehensive tests including new example and integration tests to verify the fix works correctly in non-TTY mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When affirmative and negative labels start with the same character (e.g., "Confirm" and "Cancel"), the previous implementation would: 1. Show ambiguous prompt hints like [c/c] 2. Always resolve to affirmative when user typed the shared character 3. Make it impossible to select the negative option in non-TTY mode Changes: - Added calculate_hints() method to find minimum unique prefixes - When first characters conflict, finds shortest unique prefix (e.g., "co"/"ca") - Non-TTY mode now requires unique prefix to disambiguate - TTY mode disables single-key shortcuts when ambiguous (arrow keys + enter only) - Shows clear "Ambiguous input" error with required prefixes - Updated prompts to display unique prefixes: [co/ca] instead of [c/c] Added comprehensive tests: - Unit tests for hint calculation with/without conflicts - Integration tests for ambiguous input handling - Tests for unique prefix acceptance (co/ca) - Verification that prompt displays correct hints 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated test expectations to match actual rendered output: - Added trailing spaces after negative button (from button padding) - Added extra newline for tests without description - Fixed lifetime issues by storing rendered string before calling without_ansi - Removed unused indoc import 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Benefits: - Easier to maintain: snapshots are stored inline with readable formatting - Clear diffs: changes to rendered output are immediately visible - Auto-update: cargo insta accept automatically updates snapshots - Better readability: raw strings preserve whitespace and special characters Changes: - Added insta = "1" as dev dependency - Converted test_render tests to use assert_snapshot! macro - Snapshots now stored inline with @r"..." syntax - Trailing spaces and formatting automatically captured 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
47a2582 to
d2e977b
Compare
Owner
Author
|
Dang, I think I need to make hk work on windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR significantly improves the robustness of confirm and input dialogs in non-interactive environments, fixes multiple input parsing issues, and enhances the developer experience with better testing tools.
Problems Solved
1. Terminal Noise in Non-TTY Environments
When stderr is not a TTY (e.g., in CI), rendering interactive prompts with terminal control sequences generates massive noise from cursor movement, colors, and escape codes. This was causing issues in fnox CI where Bitwarden CLI (which uses demand) would flood logs with hundreds of lines of escape sequences.
2. Input Parsing Ignores Custom Labels
The confirm dialog accepted hardcoded "y"/"yes"/"n"/"no" regardless of custom affirmative/negative labels, creating confusing behavior where users could enter "yes" even when the prompt displayed "Confirm/Cancel [c/c]".
3. Ambiguous Key Suggestions
When affirmative and negative labels share the same starting character (e.g., "Confirm" and "Cancel"), the dialog would:
[c/c]4. Panics on Empty Label Strings
When custom affirmative/negative strings were empty, the code would panic trying to get the first character.
Solutions
TTY Detection & Fallback (
src/tty.rs)Input::runandConfirm::runto check if both stdin AND stderr are terminals before entering interactive modeInput Parsing Improvements (
src/confirm.rs)Custom Label Support:
Ambiguous Key Resolution:
calculate_hints()method to detect when first characters conflict[co/ca]instead of[c/c]Panic Prevention:
unwrap_or('y')andunwrap_or('n')fallbacks when getting first characters from labelsTest Infrastructure Improvements
Integration Tests (
tests/non_tty_input.rs):Unit Tests (
src/confirm.rs):Developer Experience:
instainline snapshotsexamples/confirm_custom.rsdemonstrating custom labelscargo insta acceptTooling & CI
hk.pklconfiguration for consistent formatting/lintinghk check --allmise.tomlconfiguration for hkinstadev dependency for snapshot testingTesting
All existing tests continue to pass plus:
Files Changed
New Files:
src/tty.rs- TTY detection and fallback prompt helpersexamples/confirm_custom.rs- Custom label example (Confirm/Cancel)hk.pkl- hk configurationModified Files:
src/confirm.rs- TTY detection, custom label parsing, ambiguous key handling, panic prevention, tests (199 lines added)src/input.rs- TTY detection integration (29 lines)tests/non_tty_input.rs- Comprehensive integration tests (272 lines, 187 net added)Cargo.toml- Addedinsta = "1"dev dependency.github/workflows/test.yml- Use hk for CI checksmise.toml- Add hk configurationsrc/lib.rs- Export tty moduleBreaking Changes
None. All changes are backwards compatible. Existing code using default labels ("Yes"/"No") continues to work exactly as before.
Note
Adds non-TTY prompt fallbacks and refined confirm input parsing (with unique-prefix hints) plus new tests, example, and CI/tooling updates.
src/tty.rswithis_tty,write_prompt,read_line; use inInput::runandConfirm::runto avoid escape codes and read stdin in non-interactive environments.calculate_hints()to derive shortcuts and minimum unique prefixes; prevent panics on empty labels.{affirmative}/{negative}/enterorenterwhen ambiguous.tests/non_tty_input.rsfor non-TTY behavior, validation, Windows line endings, confirm custom-label parsing, ambiguity, and prompt visibility.instasnapshots and add unit tests for hint calculation and custom-label rendering.examples/confirm_custom.rsdemonstrating custom labels.hk.pklandmise.toml; update workflow to usejdx/mise-action@v3andhk check --all.instatodev-dependencies.Written by Cursor Bugbot for commit d741e8c. This will update automatically on new commits. Configure here.