docs(voice): terminal shortcut matrix + STT helper diagnostics (#2116)#2151
docs(voice): terminal shortcut matrix + STT helper diagnostics (#2116)#2151donglovejava wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces voice input setup diagnostics and comprehensive documentation regarding terminal compatibility and key chord safety. The reviewer's feedback suggests utilizing the unused _cwd parameter in diagnose_voice_setup to set the current working directory for the probe command, ensuring relative paths or workspace-local executables are correctly resolved.
| pub(crate) async fn diagnose_voice_setup( | ||
| command_line: &str, | ||
| _cwd: &Path, | ||
| ) -> Vec<String> { |
There was a problem hiding this comment.
The _cwd parameter is currently unused. To ensure that diagnostic checks are run in the correct context (especially when the voice command uses relative paths or workspace-specific scripts), we should use cwd and set it as the current directory for the probe command.
| pub(crate) async fn diagnose_voice_setup( | |
| command_line: &str, | |
| _cwd: &Path, | |
| ) -> Vec<String> { | |
| pub(crate) async fn diagnose_voice_setup( | |
| command_line: &str, | |
| cwd: &Path, | |
| ) -> Vec<String> { |
| let probe = tokio::process::Command::new(&program) | ||
| .arg("--version") | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .status() | ||
| .await; |
There was a problem hiding this comment.
Set the current working directory to cwd for the probe command to ensure relative paths or workspace-local executables are correctly resolved during diagnostics, matching the behavior of run_configured_voice_command.
let probe = tokio::process::Command::new(&program)
.current_dir(cwd)
.arg("--version")
.stdout(Stdio::null())
.stderr(Stdio::null())
.status()
.await;![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let consumed: &[&str] = &["Ctrl-K", "Ctrl-W", "Ctrl-U", "Ctrl-T", "Ctrl-O", "Ctrl-Y"]; | ||
| !consumed.contains(&chord) | ||
| } |
There was a problem hiding this comment.
Ctrl-T in consumed list but absent from doc comment
The doc comment above chord_likely_reaches_tui lists the consumed chords as Ctrl-K, Ctrl-W, Ctrl-U, Ctrl-O, Ctrl-Y — but the implementation also includes "Ctrl-T". Ctrl-T is the readline transpose-chars sequence and is intercepted by many terminals before the TUI sees it. As written, a developer reading the doc comment would believe chord_likely_reaches_tui("Ctrl-T") returns true (safe), when in fact it returns false. The voice_shortcut_candidates test matrix also does not exercise Ctrl-T, so this mismatch is not caught by the tests.
| let probe = tokio::process::Command::new(&program) | ||
| .arg("--version") | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .status() | ||
| .await; | ||
| match probe { | ||
| Ok(status) if !status.success() => { | ||
| issues.push(format!( | ||
| "Executable `{program}` exists but returned code {status} on --version. It may need a different invocation." | ||
| )); | ||
| } | ||
| Err(e) => { | ||
| issues.push(format!( | ||
| "Cannot spawn `{program}`: {e}. Check that it is installed and on PATH." | ||
| )); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
--version probe produces false-positive warnings for valid STT helpers
Many common STT helpers (e.g., whisper, openai-whisper, whisper.cpp binaries) do not implement --version and exit with a non-zero code when called with that flag. The Ok(status) if !status.success() branch will fire and push "It may need a different invocation" even when the binary is correctly installed and fully functional. Users who configure a working tool would receive a misleading diagnostic telling them something is wrong. A more reliable probe would use which/std::env::var("PATH")-based lookup, or simply attempt Command::new(&program).arg("--help").status() and treat an Err (spawn failure) as the only definitive "not found" signal.
| /// Chords consumed by the terminal (before the TUI sees them): | ||
| /// Ctrl-K — kill-line (macOS Terminal, iTerm2, Windows Terminal, tmux) | ||
| /// Ctrl-W — unix-word-rubout | ||
| /// Ctrl-U — kill-line-from-start | ||
| /// Ctrl-O — operated-on / execute | ||
| /// Ctrl-Y — yank |
There was a problem hiding this comment.
Ctrl-T, creating a contradiction with the implementation. Add it so the documentation matches the code.
| /// Chords consumed by the terminal (before the TUI sees them): | |
| /// Ctrl-K — kill-line (macOS Terminal, iTerm2, Windows Terminal, tmux) | |
| /// Ctrl-W — unix-word-rubout | |
| /// Ctrl-U — kill-line-from-start | |
| /// Ctrl-O — operated-on / execute | |
| /// Ctrl-Y — yank | |
| /// Chords consumed by the terminal (before the TUI sees them): | |
| /// Ctrl-K — kill-line (macOS Terminal, iTerm2, Windows Terminal, tmux) | |
| /// Ctrl-W — unix-word-rubout | |
| /// Ctrl-U — kill-line-from-start | |
| /// Ctrl-T — transpose-chars | |
| /// Ctrl-O — operated-on / execute | |
| /// Ctrl-Y — yank |
| ("Ctrl-K", false), | ||
| ("Ctrl-W", false), | ||
| ("Ctrl-U", false), | ||
| ("Ctrl-O", false), |
There was a problem hiding this comment.
voice_shortcut_candidates test does not exercise Ctrl-T, so the discrepancy between the doc comment and the consumed list goes undetected. Adding it prevents future regressions.
| ("Ctrl-K", false), | |
| ("Ctrl-W", false), | |
| ("Ctrl-U", false), | |
| ("Ctrl-O", false), | |
| ("Ctrl-K", false), | |
| ("Ctrl-W", false), | |
| ("Ctrl-U", false), | |
| ("Ctrl-T", false), | |
| ("Ctrl-O", false), |
| 2. **Non-zero exit** — the helper exits 0 on success. Non-zero indicates | ||
| recording failure (mic permission, network error, unsupported codec). |
There was a problem hiding this comment.
The heading "Non-zero exit" implies this item is about what to do when non-zero exit occurs, but the body text says "the helper exits 0 on success" — which is the normal/expected case. As written, readers may interpret this item as documenting that non-zero exit is the success condition. A clearer heading like "Exit code must be 0 on success" would remove the ambiguity.
2 participants
Summary
Validate terminal-safe voice shortcut and STT helper setup for the voice input feature shipping in v0.8.45/v0.8.46.
Completed
voice_input.rs: Addeddiagnose_voice_setup()async function that checks:voice_input_commandconfig--versionas probe)voice_input.rs: Addedterminal_detection_testsmodule with:chord_likely_reaches_tui()heuristic function documenting known-consumed chordsdocs/VOICE_INPUT_TERMINALS.md: Terminal compatibility matrix documenting Ctrl-K behavior across 13 terminal emulators + STT helper setup checklist + recommended safe chordsdocs/KEYBINDINGS.md: Added Voice input section documenting the Ctrl-K caveat with cross-reference to the new terminal matrix docNot completed (needs manual verification)
Files changed
Greptile Summary
This PR adds voice-input diagnostics and terminal-shortcut documentation for the v0.8.45 voice feature. No existing behaviour is changed — all new code is either
asyncdiagnostic helpers or#[cfg(test)]-gated functions.voice_input.rs: Addsdiagnose_voice_setup()(probes the configured STT binary) andchord_likely_reaches_tui()with a shortcut-candidate test matrix;Ctrl-Tappears in the consumed-chord array but is omitted from the doc comment, and the--versionprobe can emit false-positive warnings for helpers that don't implement that flag.docs/VOICE_INPUT_TERMINALS.md: New terminal compatibility matrix across 13 emulators with an STT setup checklist; checklist item 2's heading ("Non-zero exit") is worded opposite to its body text.docs/KEYBINDINGS.md: Adds a Voice input section explaining the Ctrl+K caveat and cross-referencing the new matrix doc; content is accurate.Confidence Score: 3/5
Safe to merge if the two defects in voice_input.rs are fixed first — neither affects production paths today, but both would surface as user-visible problems once diagnose_voice_setup is called.
The doc comment for
chord_likely_reaches_tuiomitsCtrl-Twhile the implementation treats it as consumed, creating a silent contradiction that the test matrix does not catch. Separately, the--versionprobe will push a misleading warning for any STT helper that exits non-zero on that flag (a common case for tools like whisper), causing correctly configured setups to appear broken. Both defects are in the new code added by this PR and would affect real users as soon as the diagnostic surface is exposed.crates/tui/src/tui/voice_input.rs — the consumed-chord list mismatch and the --version probe logic both need attention before this ships.
Important Files Changed
diagnose_voice_setupasync diagnostic andchord_likely_reaches_tuitest helper; doc comment for the consumed-chord list omitsCtrl-Twhich is present in the implementation, and the--versionprobe produces false-positive warnings for STT helpers that don't support that flag.DEEPSEEK_VOICE_INPUT_COMMAND) matches the actual setting insettings.rs.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[diagnose_voice_setup called] --> B{command_line empty?} B -- Yes --> C[push: voice_input_command not configured] C --> Z[return issues] B -- No --> D[parse_voice_command] D -- parse error --> E[push: Failed to parse voice command] E --> Z D -- Ok: program, _args --> F[spawn: program --version] F -- Err: spawn failed --> G[push: Cannot spawn program] G --> Z F -- Ok: status != 0 --> H[push: returned code N on --version ⚠️ false-positive risk] H --> Z F -- Ok: status == 0 --> I[no issues] I --> ZReviews (1): Last reviewed commit: "docs(voice): terminal shortcut matrix + ..." | Re-trigger Greptile