Skip to content

Commit 5557d77

Browse files
qdaxbdaniel-lxs
qdaxb
and
daniel-lxs
authored
list-files must include at least the first-level directory contents (RooCodeInc#5303)
Co-authored-by: Daniel Riccio <[email protected]>
1 parent b753774 commit 5557d77

File tree

2 files changed

+297
-8
lines changed

2 files changed

+297
-8
lines changed

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

Lines changed: 201 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
import { describe, it, expect, vi } from "vitest"
1+
import { vi, describe, it, expect, beforeEach } from "vitest"
2+
import * as path from "path"
23
import { listFiles } from "../list-files"
4+
import * as childProcess from "child_process"
35

46
vi.mock("../list-files", async () => {
57
const actual = await vi.importActual("../list-files")
@@ -16,3 +18,201 @@ describe("listFiles", () => {
1618
expect(result).toEqual([[], false])
1719
})
1820
})
21+
22+
// Mock ripgrep to avoid filesystem dependencies
23+
vi.mock("../../ripgrep", () => ({
24+
getBinPath: vi.fn().mockResolvedValue("/mock/path/to/rg"),
25+
}))
26+
27+
// Mock vscode
28+
vi.mock("vscode", () => ({
29+
env: {
30+
appRoot: "/mock/app/root",
31+
},
32+
}))
33+
34+
// Mock filesystem operations
35+
vi.mock("fs", () => ({
36+
promises: {
37+
access: vi.fn().mockRejectedValue(new Error("Not found")),
38+
readFile: vi.fn().mockResolvedValue(""),
39+
readdir: vi.fn().mockResolvedValue([]),
40+
},
41+
}))
42+
43+
// Import fs to set up mocks
44+
import * as fs from "fs"
45+
46+
vi.mock("child_process", () => ({
47+
spawn: vi.fn(),
48+
}))
49+
50+
vi.mock("../../path", () => ({
51+
arePathsEqual: vi.fn().mockReturnValue(false),
52+
}))
53+
54+
describe("list-files symlink support", () => {
55+
beforeEach(() => {
56+
vi.clearAllMocks()
57+
})
58+
59+
it("should include --follow flag in ripgrep arguments", async () => {
60+
const mockSpawn = vi.mocked(childProcess.spawn)
61+
const mockProcess = {
62+
stdout: {
63+
on: vi.fn((event, callback) => {
64+
if (event === "data") {
65+
// Simulate some output to complete the process
66+
setTimeout(() => callback("test-file.txt\n"), 10)
67+
}
68+
}),
69+
},
70+
stderr: {
71+
on: vi.fn(),
72+
},
73+
on: vi.fn((event, callback) => {
74+
if (event === "close") {
75+
setTimeout(() => callback(0), 20)
76+
}
77+
if (event === "error") {
78+
// No error simulation
79+
}
80+
}),
81+
kill: vi.fn(),
82+
}
83+
84+
mockSpawn.mockReturnValue(mockProcess as any)
85+
86+
// Call listFiles to trigger ripgrep execution
87+
await listFiles("/test/dir", false, 100)
88+
89+
// Verify that spawn was called with --follow flag (the critical fix)
90+
const [rgPath, args] = mockSpawn.mock.calls[0]
91+
expect(rgPath).toBe("/mock/path/to/rg")
92+
expect(args).toContain("--files")
93+
expect(args).toContain("--hidden")
94+
expect(args).toContain("--follow") // This is the critical assertion - the fix should add this flag
95+
96+
// Platform-agnostic path check - verify the last argument is the resolved path
97+
const expectedPath = path.resolve("/test/dir")
98+
expect(args[args.length - 1]).toBe(expectedPath)
99+
})
100+
101+
it("should include --follow flag for recursive listings too", async () => {
102+
const mockSpawn = vi.mocked(childProcess.spawn)
103+
const mockProcess = {
104+
stdout: {
105+
on: vi.fn((event, callback) => {
106+
if (event === "data") {
107+
setTimeout(() => callback("test-file.txt\n"), 10)
108+
}
109+
}),
110+
},
111+
stderr: {
112+
on: vi.fn(),
113+
},
114+
on: vi.fn((event, callback) => {
115+
if (event === "close") {
116+
setTimeout(() => callback(0), 20)
117+
}
118+
if (event === "error") {
119+
// No error simulation
120+
}
121+
}),
122+
kill: vi.fn(),
123+
}
124+
125+
mockSpawn.mockReturnValue(mockProcess as any)
126+
127+
// Call listFiles with recursive=true
128+
await listFiles("/test/dir", true, 100)
129+
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
136+
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)
140+
})
141+
142+
it("should ensure first-level directories are included when limit is reached", async () => {
143+
// Mock fs.promises.readdir to simulate a directory structure
144+
const mockReaddir = vi.mocked(fs.promises.readdir)
145+
146+
// Root directory with first-level directories
147+
mockReaddir.mockResolvedValueOnce([
148+
{ name: "a_dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
149+
{ name: "b_dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
150+
{ name: "c_dir", isDirectory: () => true, isSymbolicLink: () => false, isFile: () => false } as any,
151+
{ name: "file1.txt", isDirectory: () => false, isSymbolicLink: () => false, isFile: () => true } as any,
152+
{ name: "file2.txt", isDirectory: () => false, isSymbolicLink: () => false, isFile: () => true } as any,
153+
])
154+
155+
// Mock ripgrep to return many files (simulating hitting the limit)
156+
const mockSpawn = vi.mocked(childProcess.spawn)
157+
const mockProcess = {
158+
stdout: {
159+
on: vi.fn((event, callback) => {
160+
if (event === "data") {
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)
177+
}
178+
}),
179+
},
180+
stderr: {
181+
on: vi.fn(),
182+
},
183+
on: vi.fn((event, callback) => {
184+
if (event === "close") {
185+
setTimeout(() => callback(0), 20)
186+
}
187+
}),
188+
kill: vi.fn(),
189+
}
190+
mockSpawn.mockReturnValue(mockProcess as any)
191+
192+
// Mock fs.promises.access to simulate .gitignore doesn't exist
193+
vi.mocked(fs.promises.access).mockRejectedValue(new Error("File not found"))
194+
195+
// Call listFiles with recursive=true and a small limit
196+
const [results, limitReached] = await listFiles("/test/dir", true, 10)
197+
198+
// Verify that we got results and hit the limit
199+
expect(results.length).toBe(10)
200+
expect(limitReached).toBe(true)
201+
202+
// Count directories in results
203+
const directories = results.filter((r) => r.endsWith("/"))
204+
205+
// We should have at least the 3 first-level directories
206+
// even if ripgrep didn't return all of them
207+
expect(directories.length).toBeGreaterThanOrEqual(3)
208+
209+
// Verify all first-level directories are included
210+
const hasADir = results.some((r) => r.endsWith("a_dir/"))
211+
const hasBDir = results.some((r) => r.endsWith("b_dir/"))
212+
const hasCDir = results.some((r) => r.endsWith("c_dir/"))
213+
214+
expect(hasADir).toBe(true)
215+
expect(hasBDir).toBe(true)
216+
expect(hasCDir).toBe(true)
217+
})
218+
})

‎src/services/glob/list-files.ts

Lines changed: 96 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +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)
4150

42-
// Combine and format the results
43-
return formatAndCombineResults(files, directories, limit)
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+
}
56+
57+
return [results, limitReached]
58+
}
59+
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[] = []
66+
67+
try {
68+
const entries = await fs.promises.readdir(absolutePath, { withFileTypes: true })
69+
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}`)
81+
}
82+
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]
44134
}
45135

46136
/**
@@ -312,7 +402,6 @@ function isDirectoryExplicitlyIgnored(dirName: string): boolean {
312402
return false
313403
}
314404

315-
316405
/**
317406
* Combine file and directory results and format them properly
318407
*/

0 commit comments

Comments
 (0)