feat: Improve @-command file path completion with fzf integration #5650
Conversation
This commit enhances the file path completion for the `@` command by integrating the `fzf` command-line tool for faster and more efficient filtering. Changes introduced: * **Leveraged `fzf` for file filtering**: When `fzf` is installed and the search pattern is a literal string (not a glob), the `FileSearch` class now pipes the list of all files to `fzf` for fuzzy matching, significantly improving performance. * **Added `fzfFilter.ts` module**: A new module was created to encapsulate the logic for `fzf` availability detection and filtering. * **Included corresponding tests**: Tests were added to verify the `fzf` integration.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @DevMassive, 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!
I've implemented a significant enhancement to the @ command's file path completion by integrating the fzf command-line tool. This change aims to drastically improve the performance and user experience for file path suggestions, especially for large codebases, by leveraging fzf's highly optimized fuzzy matching capabilities. The system is designed to be robust, gracefully falling back to the existing completion logic if fzf is not installed or if the search pattern involves glob characters.
Highlights
- fzf Integration: The
@command's file path completion now integratesfzffor faster and more efficient fuzzy matching. This integration is conditional, activating only whenfzfis installed and the search pattern is a literal string (not a glob). - Conditional Logic and Fallback: The system intelligently checks for
fzfavailability upon initialization and dynamically decides whether to usefzfor the existing filtering mechanism. It falls back to the traditional filter for glob patterns or iffzfis not found on the system. - New
fzfFilterModule: A new module,fzfFilter.ts, has been introduced to encapsulate allfzf-related logic, including checking for its availability (checkCanUseFzf) and performing the fuzzy filtering (filterByFzf) by piping data tofzf's stdin. - Enhanced Test Coverage: New test cases have been added to
fileSearch.test.tsto specifically verify the correct behavior of thefzfintegration, ensuring it's used when available and bypassed when not or when glob patterns are present.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a performance enhancement by integrating fzf for file path filtering. The implementation is well-structured, with the fzf logic correctly encapsulated in a new fzfFilter.ts module and corresponding tests added.
My main concern, which is critical, is a command injection vulnerability in fzfFilter.ts where user input is passed directly to child_process.exec. I've provided a detailed comment and a suggested fix to address this security risk by using execFile and improving error handling.
Once the security issue is resolved, this will be a solid contribution.
| const stdout = await new Promise<string>((resolve) => { | ||
| const { stdin } = exec(`fzf --filter "${pattern}"`, {}, (_err, stdout) => { | ||
| resolve(stdout); | ||
| }); | ||
|
|
||
| stdin?.write(allPaths.join('\n')); | ||
| stdin?.end(); | ||
| }); |
There was a problem hiding this comment.
This implementation has a critical security vulnerability and lacks proper error handling.
-
Command Injection: The
patternis directly concatenated into the command string forexec. A malicious pattern (e.g.,foo"; rm -rf /) could lead to arbitrary command execution. You should useexecFileorspawnto pass arguments safely, which avoids shell interpretation. -
Error Handling: The
_errobject from theexeccallback is ignored. Iffzffails for any reason (e.g., exits with an error code), the error is suppressed, and the function will likely return an empty array, hiding the underlying issue. The promise should be rejected on error. Note thatfzf's exit code1means "no matches", which should be handled as a non-error case.
Please also update the import at the top of the file from exec to execFile.
const stdout = await new Promise<string>((resolve, reject) => {
// Use execFile to prevent command injection vulnerabilities.
const { stdin } = require('child_process').execFile('fzf', ['--filter', pattern], (err, stdout) => {
// fzf exits with code 1 if there are no matches. This is not an error for us.
if (err && err.code !== 1) {
return reject(err);
}
resolve(stdout);
});
// The stdin stream should be handled to pass the file paths to fzf.
stdin?.write(allPaths.join('\n'));
stdin?.end();
});Replaced `child_process.exec` with `child_process.execFile` for executing fzf commands. This change enhances security by preventing shell injection and improves error handling by correctly interpreting fzf's exit code 1 (no matches) as a non-error condition. google-gemini#5650 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fzf integration for faster file path completion. The changes are generally good, but I've identified a critical robustness issue in the new fzfFilter.ts module related to handling the fzf child process, which could lead to unhandled promise rejections and crash the application. Additionally, there's a correctness issue in fileSearch.ts where glob pattern detection is too simplistic, potentially leading to incorrect filtering behavior. I've provided suggestions to address both of these issues.
| const stdout = await new Promise<string>((resolve, reject) => { | ||
| const { stdin } = execFile('fzf', ['--filter', pattern], (err, stdout) => { | ||
| // fzf exits with code 1 if there are no matches. This is not an error for us. | ||
| if (err && err.code !== 1) { | ||
| return reject(err); | ||
| } | ||
| resolve(stdout); | ||
| }); | ||
|
|
||
| stdin?.write(allPaths.join('\n')); | ||
| stdin?.end(); | ||
| }); | ||
|
|
||
| return stdout.split('\n').filter(Boolean); |
There was a problem hiding this comment.
The current implementation of filterByFzf has two potential issues that could affect robustness and performance:
- Unhandled
stdinerrors: Thestdin.write()call can throw an error (e.g.,EPIPE) if thefzfprocess closes itsstdinstream unexpectedly (for example, if it finds a match and exits early). This would be an unhandled exception within thePromiseconstructor, leading to an unhandled promise rejection that could crash the Node.js process. - High memory usage:
allPaths.join('\n')creates a single large string containing all file paths. For projects with a very large number of files, this can consume a significant amount of memory.
A more robust approach is to stream the paths to fzf's stdin and handle potential errors on the stream. This can be achieved by refactoring the function to return the promise directly instead of using await inside.
return new Promise<string[]>((resolve, reject) => {
const child = execFile('fzf', ['--filter', pattern], (err, stdout) => {
// fzf exits with code 1 if there are no matches. This is not an error for us.
if (err && err.code !== 1) {
return reject(err);
}
resolve(stdout.split('\n').filter(Boolean));
});
if (!child.stdin) {
return reject(new Error('Could not get stdin for fzf process.'));
}
child.stdin.on('error', (err) => {
// Ignore EPIPE errors, which can happen if fzf exits before we're done writing.
});
for (const p of allPaths) {
child.stdin.write(p + '\n');
}
child.stdin.end();
});There was a problem hiding this comment.
Seeking human feedback.
| filteredCandidates = await (pattern.includes('*') || !this.canUseFzf | ||
| ? filter(candidates, pattern, options.signal) | ||
| : filterByFzf(this.allFiles, pattern)); |
There was a problem hiding this comment.
The check pattern.includes('*') to determine if a pattern is a glob is not fully reliable. It will fail to detect other glob characters like ?, [...], (...), etc. This can lead to incorrect behavior where a glob pattern is passed to fzf, which does not interpret glob syntax and will treat it as a literal string for fuzzy matching.
For a more robust way to detect if a pattern is a glob, you can use picomatch.scan().
| filteredCandidates = await (pattern.includes('*') || !this.canUseFzf | |
| ? filter(candidates, pattern, options.signal) | |
| : filterByFzf(this.allFiles, pattern)); | |
| const isGlob = picomatch.scan(pattern).isGlob; | |
| filteredCandidates = await (isGlob || !this.canUseFzf | |
| ? filter(candidates, pattern, options.signal) | |
| : filterByFzf(this.allFiles, pattern)); |
There was a problem hiding this comment.
Seeking human feedback.
|
We just landed significantly faster |
|
@jacob314 This PR focuses on a different aspect: enabling fuzzy file search (e.g. typing |
|
I would prefer we support fuzzy matching without requiring running a shell command. I would suggest we integrate fuzzy matching using a node library if possible building on the changes that were just landed.
What do you think of using the fzf-for-js instead so we don't have to add a binary dependency which will make this trickier to get right with sandboxing and the need to support windows? Would the performance of that approach be adequate? |
Replaces the reliance on the external `fzf` binary with the `fzf` npm package for file filtering. This change simplifies the implementation by removing `child_process` calls and improves cross-platform compatibility by eliminating the need for an external binary. google-gemini#5650 (comment)
| 'src/index.js', | ||
| 'src/components/Button.tsx', |
There was a problem hiding this comment.
The order of results has slightly changed due to fuzzy sorting by fzf-for-js.
|
@jacob314 Replaced the CLI tool fzf with fzf-for-js. Note:
|
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
…with fzf integration (google-gemini#5650)
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
) Co-authored-by: Jacob Richman <[email protected]>
) Co-authored-by: Jacob Richman <[email protected]>
) Co-authored-by: Jacob Richman <[email protected]>
) Co-authored-by: Jacob Richman <[email protected]>
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
…ogle-gemini#5650) Co-authored-by: Jacob Richman <[email protected]>
TLDR
This commit enables fuzzy file path completion for the @ command using fzf-for-js.
Dive Deeper
The @ command now supports fuzzy matching for file paths using the fzf-for-js library. When the input is a literal string (not a glob), the FileSearch class uses fzf-for-js to perform fuzzy filtering over the file list.
Note: The implementation is synchronous and does not reuse the FZF instance across calls, for simplicity. See async finder and usage notes for possible future enhancements.
Reviewer Test Plan
Verify fuzzy matching
Run the CLI and use the
@command with a literal string (e.g.,@h/uat). Confirm that fuzzy matching is working as expected.Verify glob patterns
Use the
@command with glob patterns (e.g.,@src/**/*.ts) and confirm that fuzzy matching is not applied.Testing Matrix
Linked issues / bugs
Resolves #1870