feat(cli): include /dir add directories in @ autocomplete suggestions#19246
Conversation
Summary of ChangesHello @jasmeetsb, 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 significantly enhances the Highlights
Changelog
Activity
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. 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
|
![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 effectively extends the @ autocomplete functionality to include files from all workspace directories, not just the current working directory. The implementation correctly handles adding and removing directories during a session by listening to onDirectoriesChanged and re-initializing the file searchers. The use of an initEpoch and AbortController makes the asynchronous logic robust against race conditions. The new tests are comprehensive and validate the multi-directory support. I've identified a couple of areas in the initialize function where the code seems to be written for an incremental update strategy, while the overall design uses a full-reset approach. This leads to redundant code and potential performance issues, which have been highlighted in the comments with reference to best practices for object initialization. My comments suggest removing this redundant logic to improve clarity and align the code with its actual behavior.
| const initPromises: Array<Promise<void>> = []; | ||
|
|
||
| for (const dir of directories) { | ||
| if (fileSearchMap.current.has(dir)) continue; |
There was a problem hiding this comment.
This check appears to be redundant. The resetFileSearchState function, which is called before initialize is triggered (e.g., from onDirectoriesChanged or changes to cwd/config), clears the fileSearchMap. Therefore, this condition will always be false. Removing this check would simplify the code and make its behavior clearer, reflecting the full re-initialization strategy that is currently implemented. This also has performance implications, as it suggests an incremental update that isn't happening, potentially masking the inefficiency of re-initializing all searchers on every change.
References
- When initializing an object that requires fetching remote settings, prefer to gather all settings first, then perform a single initialization. Avoid creating and initializing a temporary object just to fetch settings, as this can lead to resource leaks and unnecessary overhead. This aligns with the principle of performing a single, clean initialization after gathering all necessary settings, avoiding redundant logic that suggests an incremental update strategy and can lead to unnecessary overhead and confusion.
There was a problem hiding this comment.
The check is not redundant. There are two distinct reset paths, both of which exist in the original upstream code (not introduced by this PR):
- Hard reset (resetFileSearchState()) - clears the map. Triggered by cwd/config changes and onDirectoriesChanged.
- Soft reset (dispatch({ type: 'RESET' }) alone, lines 248/253) - resets reducer state to IDLE but preserves the search cache. Triggered when the user exits @ mode (clears the @ prefix or disables completion).
The original single-FileSearch code had the same pattern: the [cwd, config] effect dispatched RESET but the soft-reset paths (disabled/pattern-null) also only dispatched RESET neither path nulled out the fileSearch ref. The ref was reused on re-entry, avoiding a redundant filesystem crawl.
Our has(dir) check preserves this existing caching behavior for the multi-directory case. After a soft reset, the map retains cached FileSearch instances. When the user re-enters @ mode, has(dir) skips re-initialization for already-indexed directories. This is the same cache optimization the original code relied on.
| // Prune searchers for directories removed between getDirectories() and init completion | ||
| const dirSet = new Set(directories); | ||
| for (const key of fileSearchMap.current.keys()) { | ||
| if (!dirSet.has(key)) { | ||
| fileSearchMap.current.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pruning logic is unnecessary given the current implementation. The initEpoch mechanism correctly prevents stale FileSearch instances from being added to the map, and resetFileSearchState clears the map before a new initialization begins. At this point in the code, fileSearchMap.current will only contain instances created during this initialize call, so dirSet.has(key) will always be true. Removing this block simplifies the code and removes confusion about how stale searchers are handled.
References
- When initializing an object that requires fetching remote settings, prefer to gather all settings first, then perform a single initialization. Avoid creating and initializing a temporary object just to fetch settings, as this can lead to resource leaks and unnecessary overhead. This aligns with the principle of performing a single, clean initialization after gathering all necessary settings, avoiding redundant logic that suggests an incremental update strategy and can lead to unnecessary overhead and confusion.
There was a problem hiding this comment.
Agreed. Removed in the latest push.
gemini-cli
bot
added
priority/p2
Files from directories added with `/dir add` were not appearing in @ autocomplete suggestions because useAtCompletion only indexed the current working directory. Replace the single FileSearch instance with a Map of instances (one per workspace directory), listen to onDirectoriesChanged for mid-session updates, and show non-cwd results as absolute paths. Fixes google-gemini#18579
jasmeetsb
force-pushed
the
feature/at-tag-added-dirs
branch
from
e170397 to
f39dbdc
Compare
2 participants
Files from directories added with
/dir addwere not appearing in @ autocomplete suggestions becauseuseAtCompletiononly indexed the current working directory. Replace the singleFileSearchinstance with aMapof instances (one per workspace directory), listen toonDirectoriesChangedfor mid-session updates, and show non-cwd results as absolute paths.Fixes #18579
Summary
@autocomplete only searched the current working directory. After/dir add ../other-project/, files from that directory never appeared in suggestions. This PR makesuseAtCompletionindex all workspace directories and merge results.Details
FileSearchref withMap<string, FileSearch>keyed by directory pathinitEpochref to guard against stale async completions after directory changesonDirectoriesChangedlistener to re-initialize search when directories are added/removed mid-sessionRelated Issues
Fixes #18579
Testing
useAtCompletiontests pass (17 existing + 3 new)npm run preflight) passed: 648 test files, 10,233 tests passed across all packagesHow to Validate
npm run build && npm start@followed by a filename from your cwd — suggestions should appear as before/dir add /some/other/directory(use a real directory with files)@followed by a filename that exists in the added directory — it should now appear with its absolute pathPre-Merge Checklist