mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-10-06 02:47:56 +00:00
refactor: simplify list-files implementation while keeping stack overflow fix
- Keep queue-based iterative approach to prevent stack overflow on deep directory structures - Remove complex limitHint parameter and its associated logic - Add simple early termination when enough directories are collected (2x limit as buffer) - Simplify test cases to match the cleaner implementation - Maintain all core functionality with less code complexity
This commit is contained in:
parent
2293eb838c
commit
3963e385b7
2 changed files with 17 additions and 90 deletions
|
|
@ -66,8 +66,8 @@ describe("listFiles performance optimization", () => {
|
|||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe("listFilteredDirectories with limitHint", () => {
|
||||
it("should stop scanning when limit is reached in recursive mode", async () => {
|
||||
describe("limit behavior in directory scanning", () => {
|
||||
it("should respect limit in recursive mode", async () => {
|
||||
const testPath = "/test/project"
|
||||
const limit = 5
|
||||
|
||||
|
|
@ -119,19 +119,16 @@ describe("listFiles performance optimization", () => {
|
|||
|
||||
const [results, limitReached] = await listFilesPromise
|
||||
|
||||
// Should have stopped early due to limit
|
||||
// Should respect the limit
|
||||
expect(limitReached).toBe(true)
|
||||
expect(results.length).toBeLessThanOrEqual(limit)
|
||||
|
||||
// Verify that readdir was not called excessively
|
||||
// With optimization, it should stop after processing first-level directories
|
||||
// and maybe a few subdirectories before hitting the limit
|
||||
// Should have scanned at least the root directory
|
||||
const callCount = mockReaddir.mock.calls.length
|
||||
expect(callCount).toBeLessThanOrEqual(7) // Root + 3 first-level + maybe a few subdirs
|
||||
expect(callCount).toBeGreaterThan(0) // Should have at least scanned root
|
||||
expect(callCount).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it("should ensure first-level directories are included even with limit", async () => {
|
||||
it("should include first-level directories when limit is reached", async () => {
|
||||
const testPath = "/test/project"
|
||||
const limit = 3
|
||||
|
||||
|
|
@ -164,7 +161,7 @@ describe("listFiles performance optimization", () => {
|
|||
|
||||
const [results, limitReached] = await listFilesPromise
|
||||
|
||||
// Even with limit of 3, all first-level directories should be prioritized
|
||||
// Check if we have first-level directories in results
|
||||
const firstLevelDirs = results.filter((r) => {
|
||||
const relativePath = path.relative(testPath, r.replace(/\/$/, ""))
|
||||
return !relativePath.includes(path.sep)
|
||||
|
|
@ -178,7 +175,7 @@ describe("listFiles performance optimization", () => {
|
|||
expect(firstLevelDirs.length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it("should not apply limit in non-recursive mode", async () => {
|
||||
it("should apply limit to final results in non-recursive mode", async () => {
|
||||
const testPath = "/test/project"
|
||||
const limit = 2
|
||||
|
||||
|
|
@ -277,42 +274,8 @@ describe("listFiles performance optimization", () => {
|
|||
})
|
||||
})
|
||||
|
||||
describe("performance characteristics", () => {
|
||||
it("should calculate appropriate limit hint based on files already collected", async () => {
|
||||
const testPath = "/test/project"
|
||||
const limit = 50
|
||||
|
||||
// Mock many files returned by ripgrep
|
||||
const mockReaddir = vi.mocked(fs.promises.readdir)
|
||||
// Root level returns one directory
|
||||
mockReaddir.mockResolvedValueOnce([
|
||||
{ name: "dir1", isDirectory: () => true, isSymbolicLink: () => false },
|
||||
] as any)
|
||||
// dir1 has no subdirectories to avoid infinite deep traversal in mocks
|
||||
mockReaddir.mockResolvedValueOnce([] as any)
|
||||
|
||||
const listFilesPromise = listFiles(testPath, true, limit)
|
||||
|
||||
// Simulate ripgrep returning many files (45)
|
||||
const fileList = Array.from({ length: 45 }, (_, i) => `file${i}.txt`).join("\n")
|
||||
setTimeout(() => {
|
||||
mockProcess.stdout.emit("data", fileList)
|
||||
mockProcess.emit("close", 0)
|
||||
}, 10)
|
||||
|
||||
const [results, limitReached] = await listFilesPromise
|
||||
|
||||
// With 45 files, remainingCapacity should be small (around 5),
|
||||
// so directory scanning should be minimal.
|
||||
expect(results.length).toBeLessThanOrEqual(limit)
|
||||
|
||||
// Should have minimal directory scanning since files filled most of the limit
|
||||
const callCount = mockReaddir.mock.calls.length
|
||||
expect(callCount).toBeLessThanOrEqual(3) // Root + at most one child
|
||||
expect(callCount).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it("should handle ignored directories correctly with limit", async () => {
|
||||
describe("ignored directories", () => {
|
||||
it("should handle ignored directories correctly", async () => {
|
||||
const testPath = "/test/project"
|
||||
const limit = 10
|
||||
|
||||
|
|
@ -349,7 +312,6 @@ describe("listFiles performance optimization", () => {
|
|||
// Should not include ignored directories
|
||||
expect(results).not.toContain(expect.stringMatching(/node_modules/))
|
||||
expect(results).not.toContain(expect.stringMatching(/\.git/))
|
||||
// It's acceptable if no non-ignored directories are present due to limitHint early exit and ignore rules
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -50,7 +50,6 @@ export async function listFiles(dirPath: string, recursive: boolean, limit: numb
|
|||
// For non-recursive, use the existing approach
|
||||
const files = await listFilesWithRipgrep(rgPath, dirPath, false, limit)
|
||||
const ignoreInstance = await createIgnoreInstance(dirPath)
|
||||
// Pass limit hint to avoid unnecessary directory scanning
|
||||
const directories = await listFilteredDirectories(dirPath, false, ignoreInstance, limit)
|
||||
return formatAndCombineResults(files, directories, limit)
|
||||
}
|
||||
|
|
@ -58,10 +57,7 @@ export async function listFiles(dirPath: string, recursive: boolean, limit: numb
|
|||
// For recursive mode, use the original approach but ensure first-level directories are included
|
||||
const files = await listFilesWithRipgrep(rgPath, dirPath, true, limit)
|
||||
const ignoreInstance = await createIgnoreInstance(dirPath)
|
||||
// Calculate limit hint: account for files already collected.
|
||||
// Always allow at least 1 to ensure we still process the first level completely (the function prioritizes first level).
|
||||
const remainingCapacity = Math.max(1, limit - files.length)
|
||||
const directories = await listFilteredDirectories(dirPath, true, ignoreInstance, remainingCapacity)
|
||||
const directories = await listFilteredDirectories(dirPath, true, ignoreInstance, limit)
|
||||
|
||||
// Combine and check if we hit the limit
|
||||
const [results, limitReached] = formatAndCombineResults(files, directories, limit)
|
||||
|
|
@ -383,13 +379,13 @@ async function findGitignoreFiles(startPath: string): Promise<string[]> {
|
|||
|
||||
/**
|
||||
* List directories with appropriate filtering
|
||||
* @param limitHint - Optional hint for early termination when enough directories are collected
|
||||
* @param limit - Maximum number of results to guide early termination
|
||||
*/
|
||||
async function listFilteredDirectories(
|
||||
dirPath: string,
|
||||
recursive: boolean,
|
||||
ignoreInstance: ReturnType<typeof ignore>,
|
||||
limitHint?: number,
|
||||
limit: number,
|
||||
): Promise<string[]> {
|
||||
const absolutePath = path.resolve(dirPath)
|
||||
const directories: string[] = []
|
||||
|
|
@ -416,24 +412,11 @@ async function listFilteredDirectories(
|
|||
const queue: QueueItem[] = [{ path: absolutePath, context: initialContext }]
|
||||
let head = 0
|
||||
|
||||
// Track first-level directories separately to ensure they're prioritized
|
||||
const firstLevelDirs: string[] = []
|
||||
const baseLevelDepth = absolutePath.split(path.sep).filter((p) => p.length > 0).length
|
||||
|
||||
while (head < queue.length) {
|
||||
// Early termination based on limit hint, preserving first-level visibility semantics
|
||||
if (limitHint && directories.length >= limitHint) {
|
||||
let hasFirstLevelInQueue = false
|
||||
for (let i = head; i < queue.length; i++) {
|
||||
const depth = queue[i].path.split(path.sep).filter((p) => p.length > 0).length
|
||||
if (depth === baseLevelDepth) {
|
||||
hasFirstLevelInQueue = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if (!hasFirstLevelInQueue && firstLevelDirs.length > 0) {
|
||||
break
|
||||
}
|
||||
// Simple early termination: stop scanning if we've already collected enough directories
|
||||
// This prevents excessive scanning in large codebases while keeping the logic simple
|
||||
if (directories.length >= limit * 2) {
|
||||
break
|
||||
}
|
||||
|
||||
const item = queue[head++]
|
||||
|
|
@ -463,15 +446,6 @@ async function listFilteredDirectories(
|
|||
// Add the directory to our results (with trailing slash)
|
||||
// fullDirPath is already absolute since it's built with path.join from absolutePath
|
||||
const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/`
|
||||
|
||||
// Determine if this is a first-level directory
|
||||
const currentDepth = fullDirPath.split(path.sep).filter((p) => p.length > 0).length
|
||||
const isFirstLevel = currentDepth === baseLevelDepth + 1
|
||||
|
||||
if (isFirstLevel) {
|
||||
firstLevelDirs.push(formattedPath)
|
||||
}
|
||||
|
||||
directories.push(formattedPath)
|
||||
}
|
||||
|
||||
|
|
@ -500,15 +474,6 @@ async function listFilteredDirectories(
|
|||
!context.insideExplicitHiddenTarget
|
||||
)
|
||||
if (shouldRecurse) {
|
||||
// Respect limit hint: only continue recursing for first-level when limit reached
|
||||
if (limitHint && directories.length >= limitHint) {
|
||||
const currentDepth = fullDirPath.split(path.sep).filter((p) => p.length > 0).length
|
||||
const isFirstLevel = currentDepth === baseLevelDepth + 1
|
||||
if (!isFirstLevel) {
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
// If we're entering a hidden directory that's the target, or we're already inside one,
|
||||
// mark that we're inside an explicitly targeted hidden directory
|
||||
const newInsideExplicitHiddenTarget =
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue