Skip to content

Commit 07610dc

Browse files
committed
fix: ensure first-level directories are always included when file limit is reached
- Modified list-files to check if limit was reached and ensure all first-level directories are included - Added helper functions to get first-level directories and adjust results - Updated tests to properly verify the new behavior - Removed the breadth-first implementation in favor of a simpler approach that modifies results after the fact
1 parent 8cc571d commit 07610dc

File tree

2 files changed

+123
-61
lines changed

2 files changed

+123
-61
lines changed

src/services/glob/__tests__/list-files.spec.ts

Lines changed: 32 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -124,34 +124,26 @@ describe("list-files symlink support", () => {
124124

125125
mockSpawn.mockReturnValue(mockProcess as any)
126126

127-
// Mock readdir to return some test entries
128-
vi.mocked(fs.promises.readdir).mockResolvedValue([
129-
{ name: "test-file.txt", isDirectory: () => false, isSymbolicLink: () => false, isFile: () => true } as any,
130-
{ name: "test-dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
131-
])
132-
133127
// Call listFiles with recursive=true
134128
await listFiles("/test/dir", true, 100)
135129

136-
// For breadth-first implementation, ripgrep is called for filtering files
137-
// It should still include the --follow flag
138-
expect(mockSpawn).toHaveBeenCalled()
139-
const calls = mockSpawn.mock.calls
140-
141-
// Find a call that includes --follow flag
142-
const hasFollowFlag = calls.some((call) => {
143-
const [, args] = call
144-
return args && args.includes("--follow")
145-
})
130+
// Verify that spawn was called with --follow flag (the critical fix)
131+
const [rgPath, args] = mockSpawn.mock.calls[0]
132+
expect(rgPath).toBe("/mock/path/to/rg")
133+
expect(args).toContain("--files")
134+
expect(args).toContain("--hidden")
135+
expect(args).toContain("--follow") // This should be present in recursive mode too
146136

147-
expect(hasFollowFlag).toBe(true)
137+
// Platform-agnostic path check - verify the last argument is the resolved path
138+
const expectedPath = path.resolve("/test/dir")
139+
expect(args[args.length - 1]).toBe(expectedPath)
148140
})
149141

150142
it("should ensure first-level directories are included when limit is reached", async () => {
151143
// Mock fs.promises.readdir to simulate a directory structure
152144
const mockReaddir = vi.mocked(fs.promises.readdir)
153145

154-
// Root directory with many items
146+
// Root directory with first-level directories
155147
mockReaddir.mockResolvedValueOnce([
156148
{ name: "a_dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
157149
{ name: "b_dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
@@ -160,33 +152,28 @@ describe("list-files symlink support", () => {
160152
{ name: "file2.txt", isDirectory: () => false, isSymbolicLink: () => false, isFile: () => true } as any,
161153
])
162154

163-
// a_dir contents (many files to trigger limit)
164-
const manyFiles = Array.from(
165-
{ length: 50 },
166-
(_, i) =>
167-
({
168-
name: `file_a_${i}.txt`,
169-
isDirectory: () => false,
170-
isSymbolicLink: () => false,
171-
isFile: () => true,
172-
}) as any,
173-
)
174-
mockReaddir.mockResolvedValueOnce(manyFiles)
175-
176-
// b_dir and c_dir won't be read due to limit
177-
178-
// Mock ripgrep to approve all files
155+
// Mock ripgrep to return many files (simulating hitting the limit)
179156
const mockSpawn = vi.mocked(childProcess.spawn)
180157
const mockProcess = {
181158
stdout: {
182159
on: vi.fn((event, callback) => {
183160
if (event === "data") {
184-
// Return many file names
185-
const fileNames =
186-
Array.from({ length: 52 }, (_, i) =>
187-
i < 2 ? `file${i + 1}.txt` : `file_a_${i - 2}.txt`,
188-
).join("\n") + "\n"
189-
setTimeout(() => callback(fileNames), 10)
161+
// Return many file paths to trigger the limit
162+
const paths =
163+
[
164+
"/test/dir/a_dir/",
165+
"/test/dir/a_dir/subdir1/",
166+
"/test/dir/a_dir/subdir1/file1.txt",
167+
"/test/dir/a_dir/subdir1/file2.txt",
168+
"/test/dir/a_dir/subdir2/",
169+
"/test/dir/a_dir/subdir2/file3.txt",
170+
"/test/dir/a_dir/file4.txt",
171+
"/test/dir/a_dir/file5.txt",
172+
"/test/dir/file1.txt",
173+
"/test/dir/file2.txt",
174+
// Note: b_dir and c_dir are missing from ripgrep output
175+
].join("\n") + "\n"
176+
setTimeout(() => callback(paths), 10)
190177
}
191178
}),
192179
},
@@ -202,6 +189,9 @@ describe("list-files symlink support", () => {
202189
}
203190
mockSpawn.mockReturnValue(mockProcess as any)
204191

192+
// Mock fs.promises.access to simulate .gitignore doesn't exist
193+
vi.mocked(fs.promises.access).mockRejectedValue(new Error("File not found"))
194+
205195
// Call listFiles with recursive=true and a small limit
206196
const [results, limitReached] = await listFiles("/test/dir", true, 10)
207197

@@ -212,8 +202,8 @@ describe("list-files symlink support", () => {
212202
// Count directories in results
213203
const directories = results.filter((r) => r.endsWith("/"))
214204

215-
// With breadth-first, we should have at least the 3 first-level directories
216-
// even if we hit the limit while processing subdirectories
205+
// We should have at least the 3 first-level directories
206+
// even if ripgrep didn't return all of them
217207
expect(directories.length).toBeGreaterThanOrEqual(3)
218208

219209
// Verify all first-level directories are included

src/services/glob/list-files.ts

Lines changed: 91 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -32,33 +32,105 @@ export async function listFiles(dirPath: string, recursive: boolean, limit: numb
3232
// Get ripgrep path
3333
const rgPath = await getRipgrepPath()
3434

35-
// Get files using ripgrep
36-
const files = await listFilesWithRipgrep(rgPath, dirPath, recursive, limit)
35+
if (!recursive) {
36+
// For non-recursive, use the existing approach
37+
const files = await listFilesWithRipgrep(rgPath, dirPath, false, limit)
38+
const ignoreInstance = await createIgnoreInstance(dirPath)
39+
const directories = await listFilteredDirectories(dirPath, false, ignoreInstance)
40+
return formatAndCombineResults(files, directories, limit)
41+
}
3742

38-
// Get directories with proper filtering using ignore library
43+
// For recursive mode, use the original approach but ensure first-level directories are included
44+
const files = await listFilesWithRipgrep(rgPath, dirPath, true, limit)
3945
const ignoreInstance = await createIgnoreInstance(dirPath)
40-
const directories = await listFilteredDirectories(dirPath, recursive, ignoreInstance)
46+
const directories = await listFilteredDirectories(dirPath, true, ignoreInstance)
47+
48+
// Combine and check if we hit the limit
49+
const [results, limitReached] = formatAndCombineResults(files, directories, limit)
50+
51+
// If we hit the limit, ensure all first-level directories are included
52+
if (limitReached) {
53+
const firstLevelDirs = await getFirstLevelDirectories(dirPath, ignoreInstance)
54+
return ensureFirstLevelDirectoriesIncluded(results, firstLevelDirs, limit)
55+
}
4156

42-
let allFiles = files
43-
let allDirectories = directories
44-
const limitReached = files.length + directories.length >= limit
57+
return [results, limitReached]
58+
}
4559

46-
// If limit is reached in recursive mode, fetch one more layer non-recursively and merge results
47-
if (recursive && limitReached) {
48-
// Get files and directories in one layer (non-recursive)
49-
const filesNonRecursive = await listFilesWithRipgrep(rgPath, dirPath, false, limit)
50-
const directoriesNonRecursive = await listFilteredDirectories(dirPath, false, [])
60+
/**
61+
* Get only the first-level directories in a path
62+
*/
63+
async function getFirstLevelDirectories(dirPath: string, ignoreInstance: ReturnType<typeof ignore>): Promise<string[]> {
64+
const absolutePath = path.resolve(dirPath)
65+
const directories: string[] = []
5166

52-
// Merge recursive and non-recursive results, deduplicate
53-
const filesSet = new Set([...files, ...filesNonRecursive])
54-
const directoriesSet = new Set([...directories, ...directoriesNonRecursive])
67+
try {
68+
const entries = await fs.promises.readdir(absolutePath, { withFileTypes: true })
5569

56-
allFiles = Array.from(filesSet)
57-
allDirectories = Array.from(directoriesSet)
70+
for (const entry of entries) {
71+
if (entry.isDirectory() && !entry.isSymbolicLink()) {
72+
const fullDirPath = path.join(absolutePath, entry.name)
73+
if (shouldIncludeDirectory(entry.name, fullDirPath, dirPath, ignoreInstance)) {
74+
const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/`
75+
directories.push(formattedPath)
76+
}
77+
}
78+
}
79+
} catch (err) {
80+
console.warn(`Could not read directory ${absolutePath}: ${err}`)
5881
}
5982

60-
// Combine and format the results
61-
return formatAndCombineResults(allFiles, allDirectories, limit)
83+
return directories
84+
}
85+
86+
/**
87+
* Ensure all first-level directories are included in the results
88+
*/
89+
function ensureFirstLevelDirectoriesIncluded(
90+
results: string[],
91+
firstLevelDirs: string[],
92+
limit: number,
93+
): [string[], boolean] {
94+
// Create a set of existing paths for quick lookup
95+
const existingPaths = new Set(results)
96+
97+
// Find missing first-level directories
98+
const missingDirs = firstLevelDirs.filter((dir) => !existingPaths.has(dir))
99+
100+
if (missingDirs.length === 0) {
101+
// All first-level directories are already included
102+
return [results, true]
103+
}
104+
105+
// We need to make room for the missing directories
106+
// Remove items from the end (which are likely deeper in the tree)
107+
const itemsToRemove = Math.min(missingDirs.length, results.length)
108+
const adjustedResults = results.slice(0, results.length - itemsToRemove)
109+
110+
// Add the missing directories at the beginning (after any existing first-level dirs)
111+
// First, separate existing results into first-level and others
112+
const resultPaths = adjustedResults.map((r) => path.resolve(r))
113+
const basePath = path.resolve(firstLevelDirs[0]).split(path.sep).slice(0, -1).join(path.sep)
114+
115+
const firstLevelResults: string[] = []
116+
const otherResults: string[] = []
117+
118+
for (let i = 0; i < adjustedResults.length; i++) {
119+
const resolvedPath = resultPaths[i]
120+
const relativePath = path.relative(basePath, resolvedPath)
121+
const depth = relativePath.split(path.sep).length
122+
123+
if (depth === 1) {
124+
firstLevelResults.push(adjustedResults[i])
125+
} else {
126+
otherResults.push(adjustedResults[i])
127+
}
128+
}
129+
130+
// Combine: existing first-level dirs + missing first-level dirs + other results
131+
const finalResults = [...firstLevelResults, ...missingDirs, ...otherResults].slice(0, limit)
132+
133+
return [finalResults, true]
62134
}
63135

64136
/**

0 commit comments

Comments
 (0)