Allow for passing in multiple directories #338
Conversation
📝 WalkthroughWalkthroughThe changes update several functions and tests across the codebase to handle multiple directories instead of a single directory string. The Sequence Diagram(s)sequenceDiagram
participant CLI
participant CLI_Run
participant DefaultAction
participant Packager
participant OutputGen
CLI->>CLI_Run: Pass directories array, cwd, and options
CLI_Run->>DefaultAction: executeAction(directories[], cwd, options)
DefaultAction->>Packager: runDefaultAction(directories[], cwd, options)
Packager->>Packager: Resolve and search files for each directory
Packager->>OutputGen: generateOutput(directories[], config, ...)
OutputGen-->>Packager: Return processed output
Packager-->>DefaultAction: Return aggregated result
DefaultAction-->>CLI_Run: Return final result
CLI_Run-->>CLI: Display output
Possibly related PRs
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/cli/actions/defaultAction.test.ts (1)
60-71: Add test cases for multiple directories functionality.While the test has been updated to use an array, it only tests with a single directory. Consider adding the following test cases to validate the new multi-directory functionality:
- Test with multiple valid directories
- Test with an empty array
- Test with invalid directory paths
Example test case:
it('should handle multiple directories successfully', async () => { const options: CliOptions = { output: 'custom-output.txt', verbose: true, }; await runDefaultAction(['path/to/foo', 'path/to/bar'], process.cwd(), options); expect(configLoader.loadFileConfig).toHaveBeenCalled(); expect(configLoader.mergeConfigs).toHaveBeenCalled(); expect(packager.pack).toHaveBeenCalled(); }); it('should handle empty directory array', async () => { const options: CliOptions = {}; await expect(runDefaultAction([], process.cwd(), options)) .rejects .toThrow('At least one directory must be specified'); }); it('should handle invalid directory paths', async () => { const options: CliOptions = {}; await expect(runDefaultAction(['non/existent/path'], process.cwd(), options)) .rejects .toThrow('Directory not found'); });
🧹 Nitpick comments (7)
src/core/packager.ts (1)
47-51: Simplify array flattening usingArray.prototype.flat()The current code uses
reducewithconcatto flatten the array of collected files. This can be simplified by usingflat(), improving readability and efficiency.Apply this diff to simplify the code:
const rawFiles = ( await Promise.all( filePathsByDir.map(({ rootDir, filePaths }) => deps.collectFiles(filePaths, rootDir, progressCallback) ) ) - ).reduce((acc: RawFile[], curr: RawFile[]) => acc.concat(...curr), []); + ).flat();tests/core/output/outputStyles/xmlStyle.test.ts (1)
26-26: Add test cases for multiple directoriesTo fully validate the updated functionality of handling multiple directories, consider adding test cases where
generateOutputreceives multiple directory paths. This will ensure the function behaves as expected with various inputs.tests/core/output/outputStyles/plainStyle.test.ts (1)
26-26: Consider adding test cases for multiple directoriesSince
generateOutputnow handles multiple directories, enhancing the test to include scenarios with multiple directory inputs will ensure comprehensive test coverage and verify correct functionality across different cases.tests/core/output/outputGenerate.test.ts (1)
25-25: LGTM! Consider adding test cases for multiple directories.The changes correctly update the
generateOutputfunction calls to accept an array of directories. However, all test cases only test with a single directory ([process.cwd()]).Consider adding test cases that verify the behavior with multiple directories to ensure the new functionality works as expected. For example:
const output = await generateOutput([process.cwd(), 'path/to/second/dir'], mockConfig, mockProcessedFiles, []);Also applies to: 51-51, 86-86
tests/core/packager.test.ts (1)
72-72: LGTM! Consider expanding test coverage.The changes correctly update the test to handle arrays of directories. However, like the previous file, the test only covers the single directory case.
Consider adding test cases that verify:
- Multiple directories being packed together
- Edge cases like empty array of directories
- Duplicate directories in the array
Also applies to: 84-84
src/cli/cliRun.ts (1)
46-46: LGTM! Consider updating the CLI description.The changes correctly implement support for multiple directories. The default value of
['.']maintains backward compatibility.Consider updating the program description to explicitly mention the support for multiple directories. For example:
- .description('Repomix - Pack your repository into a single AI-friendly file') + .description('Repomix - Pack one or more directories into a single AI-friendly file')Also applies to: 76-76, 84-84, 105-105
src/cli/actions/defaultAction.ts (1)
107-182: Improve code consistency in buildCliConfig.The refactoring of the buildCliConfig function improves code consistency by using the spread operator uniformly. This makes the code more maintainable and easier to read.
Consider extracting the repeated pattern into a helper function to reduce duplication:
+const updateOutput = (cliConfig: RepomixConfigCli, key: string, value: unknown): void => { + cliConfig.output = { + ...cliConfig.output, + [key]: value, + }; +}; const buildCliConfig = (options: CliOptions): RepomixConfigCli => { const cliConfig: RepomixConfigCli = {}; // ... other code ... if (options.topFilesLen !== undefined) { - cliConfig.output = { - ...cliConfig.output, - topFilesLength: options.topFilesLen, - }; + updateOutput(cliConfig, 'topFilesLength', options.topFilesLen); } // Apply similar changes to other output updates return repomixConfigCliSchema.parse(cliConfig); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/cli/actions/defaultAction.ts(4 hunks)src/cli/actions/remoteAction.ts(1 hunks)src/cli/cliRun.ts(3 hunks)src/core/output/outputGenerate.ts(4 hunks)src/core/packager.ts(4 hunks)tests/cli/actions/defaultAction.test.ts(22 hunks)tests/cli/cliRun.test.ts(11 hunks)tests/core/output/outputGenerate.test.ts(3 hunks)tests/core/output/outputStyles/plainStyle.test.ts(1 hunks)tests/core/output/outputStyles/xmlStyle.test.ts(1 hunks)tests/core/packager.test.ts(2 hunks)tests/integration-tests/packager.test.ts(2 hunks)
🔇 Additional comments (13)
src/core/packager.ts (1)
24-24: Update function parameter to accept multiple directoriesChanging the
packfunction'srootDirparameter torootDirs: string[]appropriately enables processing multiple directories. This change is correctly reflected in the function signature and facilitates the intended functionality.tests/core/output/outputStyles/xmlStyle.test.ts (1)
26-26: Update test to pass multiple directories togenerateOutputChanging the argument to
[process.cwd()]correctly aligns with the updatedgenerateOutputfunction signature that now expects an array of directories.tests/core/output/outputStyles/plainStyle.test.ts (1)
26-26: Update test to pass multiple directories togenerateOutputChanging the argument to
[process.cwd()]correctly reflects the updated function signature ofgenerateOutputthat now accepts an array of directories.src/cli/actions/remoteAction.ts (1)
50-50: LGTM! The change maintains compatibility with the new interface.The update correctly wraps
tempDirPathin an array to match the newrunDefaultActionsignature while maintaining the existing remote repository processing logic.tests/integration-tests/packager.test.ts (1)
77-77: LGTM! Test updated to support multiple directories.The test has been correctly updated to pass the input directory as an array, aligning with the new multi-directory support feature.
src/cli/actions/defaultAction.ts (3)
23-27: LGTM! Function signature updated to support multiple directories.The runDefaultAction function now accepts an array of directories, aligning with the PR's objective to support multiple input directories.
46-46: LGTM! Path resolution handles multiple directories.The code efficiently maps over the directories array to resolve all paths in parallel.
54-54: LGTM! Pack function call updated for multiple directories.The pack function is now called with the array of target paths, maintaining consistency with the multi-directory support.
src/core/output/outputGenerate.ts (3)
7-7: LGTM! Improved type safety with explicit type import.Added FileSearchResult type import to enhance type safety.
114-116: LGTM! Function signatures updated for multiple directories.Both generateOutput and buildOutputGeneratorContext functions now accept an array of root directories, aligning with the multi-directory support feature.
Also applies to: 134-136
154-162: LGTM! Efficient parallel directory search implementation.The code efficiently searches multiple directories in parallel using Promise.all and reduces the results into a single FileSearchResult. This implementation maintains good performance while supporting multiple directories.
tests/cli/cliRun.test.ts (1)
128-130: LGTM! Tests updated to support multiple directories.All test cases have been consistently updated to pass directory arguments as arrays, maintaining comprehensive test coverage while supporting the new multi-directory feature.
Also applies to: 134-136, 140-141, 147-148, 154-156, 165-169, 177-181, 191-195, 203-207, 215-219, 227-231, 239-243, 251-257, 265-271, 279-285
tests/cli/actions/defaultAction.test.ts (1)
73-459: LGTM!The feature-specific tests have been consistently updated to use the new array syntax while maintaining their focus on verifying the configuration options. Since these tests are primarily concerned with validating feature behavior rather than directory handling, using a single directory is acceptable.
| const filePathsByDir = await Promise.all( | ||
| rootDirs.map(async (rootDir) => ({ | ||
| rootDir, | ||
| filePaths: (await deps.searchFiles(rootDir, config)).filePaths, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
Handle potential errors during file search across multiple directories
Using Promise.all assumes that all searchFiles operations will succeed. If searchFiles fails for any directory, it will reject the entire Promise.all, potentially causing unhandled exceptions. Consider implementing error handling or using Promise.allSettled to gracefully handle failures in individual directory searches and ensure robustness.
Apply this diff to handle errors using Promise.allSettled:
- const filePathsByDir = await Promise.all(
+ const filePathsByDirResults = await Promise.allSettled(
rootDirs.map(async (rootDir) => ({
rootDir,
filePaths: (await deps.searchFiles(rootDir, config)).filePaths,
})),
);
+
+ const filePathsByDir = filePathsByDirResults
+ .filter(result => result.status === 'fulfilled')
+ .map(result => result.value);
+
+ // Optionally handle rejected promises
+ const rejectedDirs = filePathsByDirResults
+ .filter(result => result.status === 'rejected')
+ .map((result, index) => rootDirs[index]);
+
+ if (rejectedDirs.length > 0) {
+ progressCallback(`Warning: Failed to search files in directories: ${rejectedDirs.join(', ')}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
rchatrath7
force-pushed
the
rc/multiple-dir-paths
branch
from
2e57844 to
e6834d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/packager.ts (1)
47-51: Simplify the file collection logic.The nested Promise.all with reduce makes the code harder to read. Consider extracting the reduction logic into a separate function for better readability.
- const rawFiles = ( - await Promise.all( - filePathsByDir.map(({ rootDir, filePaths }) => deps.collectFiles(filePaths, rootDir, progressCallback)), - ) - ).reduce((acc: RawFile[], curr: RawFile[]) => acc.concat(...curr), []); + const collectFilesFromDir = async ({ rootDir, filePaths }: { rootDir: string; filePaths: string[] }) => + deps.collectFiles(filePaths, rootDir, progressCallback); + + const rawFilesArrays = await Promise.all(filePathsByDir.map(collectFilesFromDir)); + const rawFiles = rawFilesArrays.flat();src/core/output/outputGenerate.ts (1)
154-161: Simplify the empty directory search reduction.The nested reduction logic is complex and could be simplified for better readability.
- emptyDirPaths = (await Promise.all(rootDirs.map((rootDir) => searchFiles(rootDir, config)))).reduce( - (acc: FileSearchResult, curr: FileSearchResult) => - ({ - filePaths: [...acc.filePaths, ...curr.filePaths], - emptyDirPaths: [...acc.emptyDirPaths, ...curr.emptyDirPaths], - }) as FileSearchResult, - { filePaths: [], emptyDirPaths: [] }, - ).emptyDirPaths; + const searchResults = await Promise.all(rootDirs.map((rootDir) => searchFiles(rootDir, config))); + emptyDirPaths = searchResults.flatMap(result => result.emptyDirPaths);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/cli/actions/defaultAction.ts(4 hunks)src/cli/actions/remoteAction.ts(1 hunks)src/cli/cliRun.ts(3 hunks)src/core/output/outputGenerate.ts(4 hunks)src/core/packager.ts(4 hunks)tests/cli/actions/defaultAction.test.ts(22 hunks)tests/cli/cliRun.test.ts(11 hunks)tests/core/output/outputGenerate.test.ts(3 hunks)tests/core/output/outputStyles/plainStyle.test.ts(1 hunks)tests/core/output/outputStyles/xmlStyle.test.ts(1 hunks)tests/core/packager.test.ts(2 hunks)tests/integration-tests/packager.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/core/output/outputStyles/xmlStyle.test.ts
- tests/core/output/outputStyles/plainStyle.test.ts
- tests/core/output/outputGenerate.test.ts
- tests/integration-tests/packager.test.ts
- src/cli/actions/remoteAction.ts
- tests/cli/actions/defaultAction.test.ts
- tests/cli/cliRun.test.ts
- tests/core/packager.test.ts
🔇 Additional comments (3)
src/core/packager.ts (1)
39-44: Handle potential errors during file search across multiple directories.Using
Promise.allassumes that allsearchFilesoperations will succeed. IfsearchFilesfails for any directory, it will reject the entirePromise.all, potentially causing unhandled exceptions.src/cli/cliRun.ts (1)
46-46: LGTM! Good use of variadic arguments.The change to use variadic arguments with
[directories...]and the default value['.']is a clean way to handle multiple directory inputs while maintaining backward compatibility.Also applies to: 76-76
src/cli/actions/defaultAction.ts (1)
107-182: LGTM! Consistent use of spread operator.The refactoring to consistently use the spread operator for config building improves code readability and maintainability.
| @@ -43,15 +43,15 @@ export const runDefaultAction = async ( | |||
|
|
|||
| logger.trace('Merged config:', config); | |||
|
|
|||
| const targetPath = path.resolve(directory); | |||
| const targetPaths = directories.map((directory) => path.resolve(directory)); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating directory existence.
The path resolution doesn't verify if the directories exist. Consider adding validation to provide better error messages.
- const targetPaths = directories.map((directory) => path.resolve(directory));
+ const targetPaths = await Promise.all(
+ directories.map(async (directory) => {
+ const resolvedPath = path.resolve(directory);
+ try {
+ await fs.access(resolvedPath);
+ return resolvedPath;
+ } catch {
+ throw new Error(`Directory not found: ${directory}`);
+ }
+ })
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const targetPaths = directories.map((directory) => path.resolve(directory)); | |
| const targetPaths = await Promise.all( | |
| directories.map(async (directory) => { | |
| const resolvedPath = path.resolve(directory); | |
| try { | |
| await fs.access(resolvedPath); | |
| return resolvedPath; | |
| } catch { | |
| throw new Error(`Directory not found: ${directory}`); | |
| } | |
| }) | |
| ); |
On large projects, I will have files that are contextually related but in separate sub directories that I would like to bundle together. However, due to token limits, I don't necessarily want to find the closest common parent directory and bundle the entire thing. What I really wanted was an option to be able to pass in multiple directories as arguments and have repomix bundle them all together automatically, something like
This PR adds support for having the
directoryargument be an arbitrary number of directories instead of a single string and collects all of that into one output.yields
Checklist
npm run testnpm run lint