From 3963e385b71e81670ef91b7146b791b2550b9d76 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Fri, 5 Sep 2025 10:21:45 -0500 Subject: [PATCH] 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 --- .../glob/__tests__/list-files.spec.ts | 58 ++++--------------- src/services/glob/list-files.ts | 49 +++------------- 2 files changed, 17 insertions(+), 90 deletions(-) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index f2f6171fba..b9ff523cff 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -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 }) }) }) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 5423165a30..a65da35aa6 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -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 { /** * 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, - limitHint?: number, + limit: number, ): Promise { 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 =