fix(tui): add readdir fallback for @-mention in non-git directories (#266)#268
fix(tui): add readdir fallback for @-mention in non-git directories (#266)#268zha-ji-tui wants to merge 9 commits into
Conversation
…oonshotAI#266) When `fd` is not installed and the work dir is not a git repository (or the git cache is empty), the pi-tui inner provider's `getFuzzyFileSuggestions` returns `[]` (no fd → no candidates), so `@` failed to surface any completions. Add a recursive readdir walker (2s TTL, skips node_modules and other heavy dirs) as a fallback in `FileMentionProvider` so `@` works anywhere. Closes MoonshotAI#266
Lower bump from minor to patch (bug fix per skill), rewrite wording as a single user-facing sentence without file/class names.
🦋 Changeset detectedLatest commit: 5b813e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f3cc3674a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return null; | ||
| } | ||
| files.sort(); | ||
| const capped = files.length > READDIR_MAX_ENTRIES ? files.slice(0, READDIR_MAX_ENTRIES) : files; |
There was a problem hiding this comment.
In a non-git workdir without fd, the new fallback still synchronously recurses and statSyncs every eligible file before this slice is applied. For large directories, typing @ can block the TUI for the full traversal even though READDIR_MAX_ENTRIES suggests the work is capped; the walker should short-circuit once the cap is reached instead of only truncating the completed list.
Useful? React with 👍 / 👎.
| // Hidden entries (dotfiles) require explicit query opt-in | ||
| // (handled in buildFromReadDir, not here — we always skip them | ||
| // at the walk level to keep cost bounded; opt-in re-includes). | ||
| if (entry.name.startsWith('.')) continue; |
There was a problem hiding this comment.
When the user is in a non-git directory without fd and types an explicit hidden-path query such as @.env or @.github, the fallback can never return those files because the walker discards every dot-prefixed entry before buildFromReadDir can apply the existing query.startsWith('.') opt-in. The git-backed path supports this opt-in, so the new fallback still leaves common hidden files unmentionable in the scenario it is meant to cover.
Useful? React with 👍 / 👎.
| const readdirResult = this.buildFromReadDir(atPrefix); | ||
| if (readdirResult !== null) { | ||
| return { items: readdirResult, prefix: atPrefix }; |
There was a problem hiding this comment.
In a git repo without fd, when the tracked/untracked git snapshot has no match for the current query, this raw readdir fallback bypasses the existing GitLsFilesCache filtering (git ls-files --others --exclude-standard) and can offer files that .gitignore intentionally hides, e.g. typing @secret can suggest an ignored secret.env if no tracked path matches. Restrict this fallback to non-git snapshots or preserve the gitignore filtering before returning these candidates.
Useful? React with 👍 / 👎.
| return query.length === 0 | ||
| ? rankForEmptyQuery(candidates, snapshot) | ||
| : rankForQuery(candidates, query, snapshot); |
There was a problem hiding this comment.
In a non-git directory without fd, a query that matches no walked files (for example @does-not-exist) reaches rankForQuery and returns an empty array, but buildFromReadDir still treats that as a successful fallback. That changes the provider result from “no autocomplete” into { items: [] }, which can leave the editor in an empty autocomplete state instead of dismissing the menu like the existing git-backed path does when items.length === 0.
Useful? React with 👍 / 👎.
…#266) Address review feedback on the non-git-directory fallback: - Short-circuit the recursive walk once READDIR_MAX_ENTRIES is reached, so typing @ no longer blocks the TUI on large trees. - Stop dropping dot entries at the walk level so the existing query.startsWith('.') opt-in can surface dotfiles like .env or .github/, matching the git-backed path. - Restrict the fallback to non-git workdirs only, so .gitignored files in a git repo are never exposed via readdir. - Return null from buildFromReadDir when ranking produces no matches, so the menu dismisses instead of showing an empty autocomplete state. Refs PR MoonshotAI#268.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f56582454b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (snapshot === null) { | ||
| // Not in a git repo. Inner's getFuzzyFileSuggestions is a dead | ||
| // end without `fd`, so we own the candidate source here. See | ||
| // issue #266. | ||
| const readdirResult = this.buildFromReadDir(atPrefix); |
There was a problem hiding this comment.
When gitCache.getSnapshot() returns null inside an existing git repo, this branch treats it as non-git and scans the filesystem directly. createGitLsFilesCache can return null for transient git ls-files failures while isGitRepo() remains true, so in that scenario ignored files can be suggested because the raw readdir path bypasses --exclude-standard; gate this fallback on !gitCache.isGitRepo() rather than snapshot === null alone.
Useful? React with 👍 / 👎.
| } catch { | ||
| return; | ||
| } | ||
| for (const entry of entries) { |
There was a problem hiding this comment.
Fresh evidence: the added cap check runs only before entering walkDir, so a single large directory still iterates and statSyncs every file in this loop before the final slice is applied. In a non-git workdir without fd, typing @ in a directory with thousands of files can still block the TUI far past READDIR_MAX_ENTRIES; stop the loop once files.length >= READDIR_MAX_ENTRIES before recursing or statting more entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed both P2s from the latest review:
- isGitRepo() gate + snapshot-null handling
- walkDir per-entry break
Tests: added a transient-failure case, all 22 in file-mention-provider.test.ts pass.
- Gate the readdir fallback on `gitCache.isGitRepo()` instead of `snapshot === null`, so a transient `git ls-files` failure inside a real git repo does not expose `.gitignore`d files. The previous gate conflated "not a git repo" with "git ls-files failed transiently". - Break out of the per-entry `walkDir` loop once `READDIR_MAX_ENTRIES` is reached, so a single large directory does not `statSync` every remaining file after the cap is filled. The cap was previously only checked on entry to `walkDir`. - Handle the snapshot-null case after the `isGitRepo()` gate to avoid NPE when `getSnapshot()` returns null transiently inside a real git repo. - Add a transient-failure test (isGitRepo=true, getSnapshot=null) and extend `stubGitCache` to decouple `isGitRepo` from snapshot presence. Refs PR MoonshotAI#268 codex review (P2 MoonshotAI#5, MoonshotAI#6).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdbd382366
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (entry.isDirectory()) { | ||
| if (SKIP_DIRS.has(entry.name)) continue; | ||
| const absChild = join(absDir, entry.name); | ||
| const relChild = relDir === '' ? entry.name : `${relDir}/${entry.name}`; | ||
| this.walkDir(absChild, relChild, depth + 1, files, mtimeByPath); |
There was a problem hiding this comment.
In a non-git directory without fd, the walker now descends into dot directories before the default @ path filters hidden segments out. If an early hidden tree such as .config or .cargo contains READDIR_MAX_ENTRIES files, it fills the cap here and stops the walk, then buildFromReadDir removes all those hidden candidates and visible files like README.md are never suggested. Consider not counting hidden entries against the default snapshot, or prioritizing visible paths before recursing into opt-in hidden trees.
Useful? React with 👍 / 👎.
Process non-dot entries first inside walkDir so a hidden subtree like `.config/` cannot exhaust READDIR_MAX_ENTRIES with hidden paths and push a visible file out of the snapshot. Hidden paths are still collected, just behind visible ones, so opt-in queries (`@.env`, `@.github/`) continue to work. Add a regression test that creates 1 visible file plus 1000 hidden files in `.config/` and verifies the visible file is still surfaced in the default `@` menu. Refs PR MoonshotAI#268 codex review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da0d7f18d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (entry.isDirectory()) { | ||
| if (SKIP_DIRS.has(entry.name)) continue; | ||
| const absChild = join(absDir, entry.name); | ||
| const relChild = relDir === '' ? entry.name : `${relDir}/${entry.name}`; | ||
| this.walkDir(absChild, relChild, depth + 1, files, mtimeByPath); |
There was a problem hiding this comment.
In a non-git directory without fd, this depth-first recursion can fill READDIR_MAX_ENTRIES from the first visible subdirectory before later siblings in the current directory are examined. For example, if big/ contains 1000 files and is visited before root-level README.md or .env, the cap is reached inside big/ and those sibling files are never added, so @README/@.env cannot be suggested even though they are in the working directory. Consider collecting current-directory files before recursing, or otherwise reserving/prioritizing sibling entries.
Useful? React with 👍 / 👎.
…cket When the cap fills inside a large subdirectory, sibling files in the same parent directory were left unmentioned because the walker recursed into the first subdirectory depth-first. Add a secondary sort key in walkDir's toSorted comparator: within the same visibility bucket, files sort before directories, so the current directory's files are captured before any subdirectory recursion. Add a regression test that creates a README.md alongside a `big/` subdirectory with 1000 files and verifies README.md is still surfaced via a targeted `@README` query. The targeted query (rather than a bare `@`) is required because rankForEmptyQuery returns only the top 15 by mtime, and README.md has the oldest mtime in the fixture. Refs PR MoonshotAI#268 codex review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9eb8754c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const aHidden = a.name.startsWith('.') ? 1 : 0; | ||
| const bHidden = b.name.startsWith('.') ? 1 : 0; | ||
| if (aHidden !== bHidden) return aHidden - bHidden; |
There was a problem hiding this comment.
Fresh evidence after the prior cap fixes: this sorter still puts root-level hidden files like .env behind every visible directory. In a non-git directory without fd, if an early visible directory contains READDIR_MAX_ENTRIES files, the loop recurses into it and breaks at the cap before reaching .env, so an explicit @.env query cannot suggest the file even though hidden paths are meant to be opt-in. Consider processing current-directory files before directories regardless of hiddenness, or otherwise reserving hidden root files for explicit hidden queries.
Useful? React with 👍 / 👎.
| const absChild = join(absDir, entry.name); | ||
| const relChild = relDir === '' ? entry.name : `${relDir}/${entry.name}`; | ||
| this.walkDir(absChild, relChild, depth + 1, files, mtimeByPath); | ||
| } else if (entry.isFile()) { |
There was a problem hiding this comment.
In a non-git directory without fd, symlink entries are silently skipped because Dirent.isFile() is false for symbolic links. That means an existing mentionable path such as @config.json where config.json is a symlink to a real file never appears in autocomplete, even though the fallback is meant to list files from the working directory. Consider detecting symlinks and following them when they resolve to files, while still avoiding recursive symlink directory traversal.
Useful? React with 👍 / 👎.
…ymlinks Split walkDir's iteration into two phases. Phase 1 collects files in the current directory (regular files and symlinks that resolve to files) before any subdirectory recursion. Phase 2 recurses into subdirectories. This guarantees that a single large sibling subdirectory cannot fill READDIR_MAX_ENTRIES and leave current-directory files — including opt-in hidden files like root-level `.env` — unmentioned. Also fix symlink handling: `Dirent.isFile()` is false for symlinks, so a `real.ts` linked as `link.ts` was silently dropped. The walker now follows symlinks and records the entry if the resolved target is a file. Symlinks to directories are not recursed into (cycle risk) and not added to the snapshot. Add two regression tests: - Root-level `.env` surfaces via `@.env` even when a sibling subdirectory has 1000 files. - `link.ts` (a symlink to `real.ts`) surfaces via `@link` (skipped on Windows when symlink creation requires developer mode). Refs PR MoonshotAI#268 codex review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48823a294
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const entry of ordered) { | ||
| if (files.length >= READDIR_MAX_ENTRIES) break; | ||
| if (entry.name === '.' || entry.name === '..') continue; | ||
| if (!entry.isDirectory()) continue; | ||
| if (SKIP_DIRS.has(entry.name)) continue; | ||
| const absChild = join(absDir, entry.name); | ||
| const relChild = relDir === '' ? entry.name : `${relDir}/${entry.name}`; | ||
| this.walkDir(absChild, relChild, depth + 1, files, mtimeByPath); |
There was a problem hiding this comment.
In a non-git workdir without fd that contains many subdirectories but fewer than READDIR_MAX_ENTRIES files, this recursion keeps descending into every directory because the stop condition only checks files.length. Since this runs synchronously from getSuggestions when the user types @, a directory-heavy tree can still block the TUI even though the fallback is meant to be capped; count visited directories/entries too, or add a separate traversal budget.
Useful? React with 👍 / 👎.
| for (const entry of ordered) { | ||
| if (files.length >= READDIR_MAX_ENTRIES) break; | ||
| if (entry.name === '.' || entry.name === '..') continue; | ||
| if (!entry.isFile() && !entry.isSymbolicLink()) continue; |
There was a problem hiding this comment.
In a non-git workdir without fd, Phase 1 collects hidden files in the current directory before Phase 2 recurses into visible directories, so 1000 root dotfiles can fill READDIR_MAX_ENTRIES and prevent src/keep.ts from ever entering the snapshot; the later default filter then removes those hidden candidates and @src has no match. Consider deferring hidden files unless the query opts into them, or otherwise reserving capacity for visible directories before counting hidden files.
Useful? React with 👍 / 👎.
Restructure walkDir into four priority-ordered phases: visible files at the current level, recurse into visible subdirectories, hidden files at the current level, recurse into hidden subdirectories. This handles the conflicting requirements that 1000 root dotfiles should not starve a visible subdirectory like `src/` (the directory-heavy-tree case), while a single root-level `.env` should still be reachable via `@.env` even when a sibling visible subdirectory has many files. Add a `dirsVisited` counter on the walker (reset per walk, capped at READDIR_MAX_ENTRIES) so a directory-heavy tree — 10 000 empty subdirs, say — does not synchronously recurse into every one. Both the file cap and the dir cap are checked at the top of each walkDir invocation and at the top of each phase loop. Update the existing 'surfaces a root-level hidden file' test to use 999 files (so the cap leaves exactly one slot for `.env` in phase 3). Add a new test 'captures a visible subdirectory before processing many root dotfiles' for the directory-heavy-tree case. Refs PR MoonshotAI#268 codex review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4950e9da73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const entry of entries) { | ||
| if (files.length >= READDIR_MAX_ENTRIES) break; | ||
| if (entry.name.startsWith('.')) continue; | ||
| collectFile(entry); |
There was a problem hiding this comment.
In a non-git workdir without fd that contains many subdirectories but few files, this first pass still scans every Dirent in the directory before the dirsVisited cap is checked in phase 2. Since collectFile just returns for directories, a root with thousands of folders can still synchronously iterate the whole listing on each cache refresh when the user types @, so the traversal budget does not actually bound this common directory-heavy case; consider applying an entry/iteration budget before the four passes or using an incremental directory read.
Useful? React with 👍 / 👎.
…dir walker Phase 2 and Phase 4 already check dirsVisited >= READDIR_MAX_ENTRIES before iterating entries, but Phase 1 (visible files) and Phase 3 (hidden files) did not. In a directory-heavy tree with few files, Phase 1 would scan every Dirent before the cap was checked in Phase 2, making the traversal budget ineffective for that common case. Refs PR MoonshotAI#268 codex review (commit 4950e9d).
1 participant
Summary
Fix
@file-mention completion failing in non-git directories. Thepi-tui inner provider's
getFuzzyFileSuggestionsreturns[]whenfdis not installed, andFileMentionProviderwas previouslydelegating to it without a fallback. This adds a recursive readdir
walker (2s TTL, skips
node_modulesand other heavy dirs) so@works anywhere.
Closes #266
Related Issue
Resolves #266
Problem
The user-facing symptom in #266 is "@ can't surface file
completion". The maintainer's initial root-cause hypothesis in the
issue thread attributed it to fullwidth
＠(U+FF20) from theChinese IME not matching the halfwidth
@(U+0040) trigger. Afterreading the code I believe that hypothesis is a red herring — the
real cause is on the candidate-source side, not the trigger side.
Trigger is fine. pi-tui's
CombinedAutocompleteProvider.extractAtPrefix(
autocomplete.js:331-342) usestext[tokenStart] === "@"— truehalfwidth. The kimi
FileMentionProviderre-implements the samecheck (
file-mention-provider.ts:133-143) and both correctly handlehalfwidth
@. (The fullwidth hypothesis is plausible but I couldnot reproduce it being necessary; either way, fixing the candidate
source also fixes the user-visible behavior.)
Candidate source is broken without
fdor git. pi-tui'sgetFuzzyFileSuggestions(autocomplete.js:576-579) returns[]when
this.fdPathis null, andgetSuggestions(autocomplete.js:198-199)turns that empty array into
null, which causes the menu to notappear at all. The previous
FileMentionProvideronly had a gitsnapshot fallback — when both
fdand git were unavailable, thefallback was a dead end.
Trigger matrix (before this PR):
fdinstalledAfter this PR, the last two rows become ✅ via the readdir fallback.
What changed
ReadDirWalkerinternal class inapps/kimi-code/src/tui/components/editor/file-mention-provider.tsreaddirSyncof the work dirGitLsFilesCache.TTL_MS).git,node_modules,dist,build,.next,.turbo,.parcel-cache,.cache,__pycache__,.venv,target,.idea,.vscode)level — same default behavior as the git-snapshot path
FileMentionProvider.getSuggestionsnow delegates to the walkerwhen the git snapshot is null or empty. Ranking uses the existing
rankForEmptyQuery/rankForQueryhelpers, which accept anysnapshot with
{ files, mtimeByPath, recencyOrder }— the walker'soutput is structurally compatible.
FileMentionProvider.applyCompletionis unchanged — stilldelegates to pi-tui's
inner.applyCompletionfor@insertionsemantics.
apps/kimi-code/test/tui/components/editor/file-mention-provider.test.ts:node_modules/distnot surfaced.git/: hidden entries not surfaced@queries within 2sdoes not appear until the cache expires
Verification
Automated:
pnpm typecheck(whole monorepo): passpnpm lint: 0 errors (11 pre-existing warnings, none inchanged files)
pnpm test(apps/kimi-code): 111 files / 1019 tests pass /2 skipped (3 new tests added)
pnpm changeset status: pending changeset acceptedManual TUI (in
~/test-mention/, a non-git dir):@→ 7 candidates surface (README.md, config.json, main.ts,docs/intro.md, docs/setup.md, src/index.ts, src/utils.ts)
@re→ README.md and src/index.ts (fuzzy hit, by design)node_modules/foo/skip-me.tsdoes not appear.hidden.mddoes not appearNotes for reviewer
on a fix approach, because the maintainer's initial root-cause
hypothesis in the issue (fullwidth
@) and my proposedcandidate-source fix are on different sides of the same symptom.
Happy to revise the approach if the maintainers prefer a
fullwidth-tolerant trigger instead / in addition.
maintainers should weigh against the "Discuss first" guideline
in CONTRIBUTING.md.
required (behavior is internal to the TUI input box).
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.