fix(zsh): remove trailing space from completions and add directory slash#556
fix(zsh): remove trailing space from completions and add directory slash#556
Conversation
Use _describe with -S '' instead of _arguments to prevent zsh from adding unwanted trailing spaces after completions. This fixes partial completions like `node@` and directory paths like `/opt/homebrew/`. Also append trailing `/` to directory entries in complete_path. Closes #67 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Zsh shell completion experience by addressing two key areas: the removal of extraneous trailing spaces after completions and the consistent addition of trailing slashes for directory suggestions. These changes enhance the accuracy and user-friendliness of Zsh completions, particularly for path and command arguments, and are supported by updated internal logic and new integration tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
==========================================
+ Coverage 72.40% 72.74% +0.34%
==========================================
Files 48 48
Lines 6830 6983 +153
Branches 6830 6983 +153
==========================================
+ Hits 4945 5080 +135
+ Misses 1242 1233 -9
- Partials 643 670 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 correctly addresses issues with zsh completions by switching to _describe to remove trailing spaces and adds logic to append a trailing slash to directory completions. The changes are accompanied by new tests that verify the fixes. I have one suggestion to improve the performance of path completion.
cli/src/cli/complete_word.rs
Outdated
| .map(|de| de.path()) | ||
| .filter(|p| filter(p)) | ||
| .map(|p| { | ||
| p.strip_prefix(base) | ||
| let mut s = p | ||
| .strip_prefix(base) | ||
| .unwrap_or(&p) | ||
| .to_string_lossy() | ||
| .to_string() | ||
| .to_string(); | ||
| if p.is_dir() { | ||
| s.push('/'); | ||
| } | ||
| s | ||
| }) |
There was a problem hiding this comment.
For better performance, we can avoid extra stat syscalls by using DirEntry::file_type(). This information is often available without a separate syscall after read_dir, which can make path completions noticeably faster, especially in large directories. The current implementation converts DirEntry to Path and then calls is_dir(), which issues a syscall for every entry. By preserving the DirEntry, we can optimize this.
| .map(|de| de.path()) | |
| .filter(|p| filter(p)) | |
| .map(|p| { | |
| p.strip_prefix(base) | |
| let mut s = p | |
| .strip_prefix(base) | |
| .unwrap_or(&p) | |
| .to_string_lossy() | |
| .to_string() | |
| .to_string(); | |
| if p.is_dir() { | |
| s.push('/'); | |
| } | |
| s | |
| }) | |
| .filter(|de| filter(&de.path())) | |
| .map(|de| { | |
| let p = de.path(); | |
| let mut s = p | |
| .strip_prefix(base) | |
| .unwrap_or(&p) | |
| .to_string_lossy() | |
| .to_string(); | |
| if de.file_type().map(|ft| ft.is_dir()).unwrap_or_else(|_| p.is_dir()) { | |
| s.push('/'); | |
| } | |
| s | |
| }) |
Greptile SummaryThis PR fixes two related zsh completion UX issues: trailing spaces appended after completions (e.g.,
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant ZSH as Zsh Completion
participant Script as _usage function
participant UsageBin as usage binary
participant Describe as _describe
ZSH->>Script: Tab pressed in shell
Script->>UsageBin: usage --usage-spec > spec_file
Script->>UsageBin: complete-word --shell zsh -f spec_file -- words[@]
Note over UsageBin: Formats output as val:desc
Note over UsageBin: Appends / to directory paths
UsageBin-->>Script: "sub:A subcommand"
UsageBin-->>Script: "dir/: "
UsageBin-->>Script: "file.txt"
Note over Script: Reads lines into completions array
Script->>Describe: _describe completions completions -S ""
Note over Describe: -S suppresses trailing space
Describe-->>ZSH: Rendered completions
Reviews (2): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Remove `--` before `-S ''` in _describe call so the option is correctly forwarded to compadd - Use DirEntry::file_type() instead of Path::is_dir() to avoid extra stat syscalls in complete_path - Move colon escaping inside the descriptions branch only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| .filter(|de| filter(&de.path())) | ||
| .map(|de| { | ||
| let p = de.path(); | ||
| let is_dir = de | ||
| .file_type() | ||
| .map(|ft| ft.is_dir()) | ||
| .unwrap_or_else(|_| p.is_dir()); | ||
| let mut s = p | ||
| .strip_prefix(base) | ||
| .unwrap_or(&p) | ||
| .to_string_lossy() | ||
| .to_string() | ||
| .to_string(); | ||
| if is_dir { | ||
| s.push('/'); | ||
| } | ||
| s | ||
| }) | ||
| .sorted() | ||
| .collect() |
There was a problem hiding this comment.
de.path() called twice per entry
de.path() allocates a new PathBuf on each call. In the current chain it is called once in .filter(|de| filter(&de.path())) and again inside .map() via let p = de.path(). For large directories this doubles the allocation count unnecessarily.
Consider folding the filter into the map to call de.path() only once:
| .filter(|de| filter(&de.path())) | |
| .map(|de| { | |
| let p = de.path(); | |
| let is_dir = de | |
| .file_type() | |
| .map(|ft| ft.is_dir()) | |
| .unwrap_or_else(|_| p.is_dir()); | |
| let mut s = p | |
| .strip_prefix(base) | |
| .unwrap_or(&p) | |
| .to_string_lossy() | |
| .to_string() | |
| .to_string(); | |
| if is_dir { | |
| s.push('/'); | |
| } | |
| s | |
| }) | |
| .sorted() | |
| .collect() | |
| .filter_map(|de| { | |
| let p = de.path(); | |
| if !filter(&p) { | |
| return None; | |
| } | |
| let is_dir = de | |
| .file_type() | |
| .map(|ft| ft.is_dir()) | |
| .unwrap_or_else(|_| p.is_dir()); | |
| let mut s = p | |
| .strip_prefix(base) | |
| .unwrap_or(&p) | |
| .to_string_lossy() | |
| .to_string(); | |
| if is_dir { | |
| s.push('/'); | |
| } | |
| Some(s) | |
| }) |
### 🚀 Features - **(cli)** render all doc-related fields in --help output by [@jdx](https://github.com/jdx) in [#554](#554) - **(cli)** support reading spec from stdin via --file - by [@jdx](https://github.com/jdx) in [#555](#555) ### 🐛 Bug Fixes - **(zsh)** remove trailing space from completions and add directory slash by [@jdx](https://github.com/jdx) in [#556](#556) - use field assignment for non-exhaustive Spec in benchmarks by [@jdx](https://github.com/jdx) in [#552](#552) ### 📦️ Dependency Updates - update apple-actions/import-codesign-certs digest to fe74d46 by [@renovate[bot]](https://github.com/renovate[bot]) in [#550](#550) - update codecov/codecov-action digest to 1af5884 by [@renovate[bot]](https://github.com/renovate[bot]) in [#551](#551) - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#547](#547)
1 participant
Summary
_describewith-S ''instead of_argumentsin zsh completion template to prevent unwanted trailing spaces after completions (e.g.,node@no longer gets a space appended)/to directory entries incomplete_pathso directory completions show/opt/homebrew/instead of/opt/homebrewcomplete-wordoutput format from'val'\:'desc'toval:descto match_describeexpectationsCloses #67
Test plan
test_zsh_complete_word_output_format— verifies newname:descriptionoutput formattest_complete_path_adds_trailing_slash_for_directories— verifies dirs get/suffix, files don't🤖 Generated with Claude Code
Note
Medium Risk
Changes zsh completion generation and
complete-wordoutput formatting, which can break shell completion behavior if the new format is not accepted across zsh setups. Also tweaks path completion output (adds/for directories), affecting user-visible CLI suggestions.Overview
Fixes zsh completions to avoid unwanted trailing spaces by switching the generated zsh completion scripts from
_argumentscommand substitution to collecting candidates and feeding them into_describewith-S ''.Updates
usage complete-word --shell zshto emitvalue:description(with:escaped) to match_describeexpectations, and enhances path completion to append/to directory candidates. Adds integration tests covering the new zsh output format and directory-slash behavior, and updates zsh completion snapshots accordingly.Written by Cursor Bugbot for commit fc5ec7a. This will update automatically on new commits. Configure here.