Conversation
This PR adds support for .ignore files, which are used by tools like ripgrep and the silver searcher. This allows users to maintain a single .ignore file that works across multiple tools instead of maintaining separate ignore files. Changes: - Add ignore.useDotIgnore config option (default: true) - Add --no-dot-ignore CLI flag to disable .ignore file usage - Update ignore file priority: .repomixignore > .ignore > .gitignore > default patterns - Add comprehensive tests for .ignore file handling - Update documentation to reflect new .ignore file support The .ignore file is enabled by default but can be disabled via configuration or CLI flag, maintaining backward compatibility. Resolves #937
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds support for Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Layer
participant Config as Config Schema
participant Search as File Search
participant Patterns as Ignore Patterns
CLI->>Config: User sets --no-dot-ignore or useDotIgnore: false
Config->>Search: Config object with useDotIgnore flag
Search->>Patterns: getIgnoreFilePatterns()
alt useDotIgnore === true
Patterns->>Patterns: Add **/.repomixignore (first)
Patterns->>Patterns: Add **/.ignore (conditional)
Patterns->>Patterns: Add **/.gitignore / .git/info/exclude
else useDotIgnore === false
Patterns->>Patterns: Add **/.repomixignore (first)
Patterns->>Patterns: Add **/.gitignore / .git/info/exclude
end
Patterns-->>Search: Ordered ignore file patterns
Search-->>CLI: Filtered file list
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying repomix with
|
| Latest commit: |
e003ec6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9636e7ac.repomix.pages.dev |
| Branch Preview URL: | https://feat-ignore.repomix.pages.dev |
Summary of ChangesHello @yamadashy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the file exclusion mechanism by integrating support for '.ignore' files, which are commonly used by tools like ripgrep and The Silver Searcher. This change aims to streamline the user experience by enabling a single, unified ignore file across multiple tools, thereby reducing maintenance effort and improving consistency in file exclusion. It introduces new configuration options and CLI flags for granular control, while maintaining backward compatibility with existing '.gitignore' and '.repomixignore' setups. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for .ignore files to Repomix's file filtering mechanism. The .ignore file format is compatible with tools like ripgrep and the silver searcher, allowing users to maintain a single ignore file across multiple tools.
- Added
useDotIgnoreconfiguration option (defaults totrue) - Updated ignore file priority order to include
.ignorefiles - Added CLI option
--no-dot-ignoreto disable.ignorefile support
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| website/client/src/public/schemas/latest/schema.json | Added useDotIgnore boolean property to the JSON schema |
| website/client/src/public/schemas/1.8.0/schema.json | Added useDotIgnore boolean property to the version-specific schema |
| tests/core/file/fileSearch.test.ts | Updated tests to include useDotIgnore in mock configs and added test for .ignore file exclusion |
| tests/config/configSchema.test.ts | Updated expected default config to include useDotIgnore: true |
| src/core/file/fileSearch.ts | Modified getIgnoreFilePatterns to conditionally add .ignore file pattern |
| src/config/configSchema.ts | Added useDotIgnore property to config schema with default value true |
| src/cli/types.ts | Added dotIgnore optional property to CLI options interface |
| src/cli/cliRun.ts | Added --no-dot-ignore CLI option |
| src/cli/actions/defaultAction.ts | Added logic to handle dotIgnore CLI option |
| README.md | Updated documentation to mention .ignore file support and priority order |
Comments suppressed due to low confidence (6)
tests/core/file/fileSearch.test.ts:155
- The test 'should return default patterns when useDefaultPatterns is true' is missing the
useDotIgnoreproperty in the mock config. SincecreateMockConfiguses defaults fromdefaultConfigwhereuseDotIgnoreistrue, this test should explicitly setuseDotIgnore: truefor consistency with other tests in the file and to make the test's intent clear.
const mockConfig = createMockConfig({
ignore: {
useGitignore: true,
useDefaultPatterns: true,
customPatterns: [],
},
});
tests/core/file/fileSearch.test.ts:170
- The test 'should include custom patterns' is missing the
useDotIgnoreproperty in the mock config. This should be explicitly set to maintain consistency with updated tests and make the test's configuration clear.
const mockConfig = createMockConfig({
ignore: {
useGitignore: true,
useDefaultPatterns: false,
customPatterns: ['*.custom', 'temp/'],
},
});
tests/core/file/fileSearch.test.ts:184
- The test 'should combine default and custom patterns' is missing the
useDotIgnoreproperty in the mock config. This should be explicitly set for consistency with the updated test patterns in this file.
const mockConfig = createMockConfig({
ignore: {
useGitignore: true,
useDefaultPatterns: true,
customPatterns: ['*.custom', 'temp/'],
},
});
tests/core/file/fileSearch.test.ts:200
- The test 'should include patterns from .git/info/exclude when useGitignore is true' is missing the
useDotIgnoreproperty in the mock config. This should be explicitly set for consistency with the updated test suite.
const mockConfig = createMockConfig({
ignore: {
useGitignore: true,
useDefaultPatterns: false,
customPatterns: [],
},
});
tests/core/file/fileSearch.test.ts:271
- The test 'should call globby with correct parameters' is missing the
useDotIgnoreproperty in the mock config. Since this test verifies theignoreFilesparameter passed to globby, it should explicitly setuseDotIgnoreand update the expected array on line 283 to include**/.ignorefor completeness.
const mockConfig = createMockConfig({
include: ['**/*.js'],
ignore: {
useGitignore: true,
useDefaultPatterns: false,
customPatterns: ['*.custom'],
},
});
tests/core/file/fileSearch.test.ts:300
- The test 'Honor .gitignore files in subdirectories' is missing the
useDotIgnoreproperty in the mock config. This should be explicitly set for consistency with the updated test patterns throughout the file.
const mockConfig = createMockConfig({
include: ['**/*.js'],
ignore: {
useGitignore: true,
useDefaultPatterns: false,
customPatterns: [],
},
});
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for .ignore files, which is a great feature for improving compatibility with tools like ripgrep. The changes are well-structured across documentation, configuration, and implementation. I've found a potential issue with the priority order of ignore files in the core logic, which also affects the tests. My review includes suggestions to correct this to align with the documented behavior. Otherwise, the implementation is solid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
937-937: LGTM! Accurate MCP tool documentation.The
ignorePatternsparameter descriptions correctly explain that patterns supplement.gitignore,.ignore, and built-in exclusions. The documentation is clear and helpful for MCP tool users.Note: Static analysis flagged a minor markdown indentation preference on these lines. If you'd like to address the linter warning, adjust the list indentation from 4 spaces to 2 spaces:
- `includePatterns`: (Optional) Specify files to include using fast-glob patterns. Multiple patterns can be comma-separated (e.g., "**/*.{js,ts}", "src/**,docs/**"). Only matching files will be processed. - - `ignorePatterns`: (Optional) Specify additional files to exclude using fast-glob patterns. Multiple patterns can be comma-separated (e.g., "test/**,*.spec.js", "node_modules/**,dist/**"). These patterns supplement .gitignore, .ignore, and built-in exclusions. + - `ignorePatterns`: (Optional) Specify additional files to exclude using fast-glob patterns. Multiple patterns can be comma-separated (e.g., "test/**,*.spec.js", "node_modules/**,dist/**"). These patterns supplement .gitignore, .ignore, and built-in exclusions.Also applies to: 955-955
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(8 hunks)src/cli/actions/defaultAction.ts(1 hunks)src/cli/cliRun.ts(1 hunks)src/cli/types.ts(1 hunks)src/config/configSchema.ts(2 hunks)src/core/file/fileSearch.ts(1 hunks)tests/config/configSchema.test.ts(2 hunks)tests/core/file/fileSearch.test.ts(1 hunks)website/client/src/public/schemas/1.8.0/schema.json(1 hunks)website/client/src/public/schemas/latest/schema.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-27T08:22:19.918Z
Learnt from: boralg
Repo: yamadashy/repomix PR: 318
File: src/config/defaultIgnore.ts:136-138
Timestamp: 2025-01-27T08:22:19.918Z
Learning: In Rust projects, `cargo-timing*.html` files are generated within the `target/` directory, so they're automatically covered by the `**/target/**` ignore pattern.
Applied to files:
README.md
🧬 Code graph analysis (1)
tests/core/file/fileSearch.test.ts (2)
tests/testing/testUtils.ts (1)
createMockConfig(15-45)src/core/file/fileSearch.ts (1)
getIgnoreFilePatterns(295-309)
🪛 markdownlint-cli2 (0.18.1)
README.md
937-937: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
955-955: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
src/core/file/fileSearch.ts (1)
295-309: LGTM! Clean implementation of.ignorefile support.The conditional inclusion of
**/.ignorefollows the existing pattern for.gitignoreand maintains the correct priority order. The logic is clear and straightforward.website/client/src/public/schemas/1.8.0/schema.json (1)
126-128: LGTM! Schema addition is correct.The
useDotIgnoreproperty is properly defined as a boolean type and positioned logically alongside other ignore configuration options.website/client/src/public/schemas/latest/schema.json (1)
126-128: LGTM! Consistent with versioned schema.The schema addition matches the 1.8.0 version, maintaining proper schema continuity.
src/config/configSchema.ts (2)
59-59: LGTM! Base schema addition is consistent.The optional
useDotIgnorefield follows the same pattern asuseGitignore.
115-115: LGTM! Default value is appropriate.Setting
useDotIgnoreto defaulttrueenables the feature by default, which aligns with the PR objective of reducing maintenance by sharing ignore files across tools.tests/config/configSchema.test.ts (2)
129-129: LGTM! Test correctly includes the new field.The addition of
useDotIgnore: trueensures the default schema validation includes the new ignore configuration option.
234-234: LGTM! Merged schema test updated consistently.The test correctly validates that
useDotIgnoreis properly handled in the merged configuration schema.src/cli/cliRun.ts (1)
148-148: LGTM! CLI option is well-placed and consistent.The
--no-dot-ignoreflag follows the same pattern as--no-gitignoreand is properly positioned in the File Selection Options group with a clear description.src/cli/types.ts (1)
35-35: LGTM! Type definition is clean and consistent.The
dotIgnoreoptional boolean is properly defined and positioned alongside the relatedgitignoreoption.src/cli/actions/defaultAction.ts (1)
130-133: LGTM! CLI override handling is consistent and correct.The
dotIgnorehandling exactly mirrors thegitignorepattern above it (lines 126-129), only applying the override when explicitly set to false. This preserves config file values while allowing CLI overrides, which is the correct behavior for Commander.js--no-*flags.README.md (6)
86-86: LGTM! Clear feature documentation.The Git-Aware feature description accurately reflects the new
.ignorefile support alongside existing.gitignoreand.repomixignorefiles.
498-498: LGTM! Accurate JSON format example.The notes field correctly documents that files may be excluded based on
.ignorerules in addition to.gitignoreand.repomixignore.
640-640: LGTM! Well-documented CLI flag.The
--no-dot-ignoreflag is clearly documented alongside the related--no-gitignoreflag, making it easy for users to discover and understand.
1212-1212: LGTM! Clear configuration documentation.The
ignore.useDotIgnoreconfiguration option is well-documented with a clear description and correct default value (true).
1328-1328: LGTM! Comprehensive feature explanation.The
.ignorefile documentation is thorough and explains:
- What the feature does
- Which tools respect it (ripgrep, silver searcher)
- The benefit (reduced maintenance burden)
- How to configure it (both config file and CLI)
This provides excellent context for users adopting the feature.
1337-1344: LGTM! Accurate priority order documentation.The updated priority order correctly reflects the new
.ignorefile support:
- Custom patterns (highest priority)
.repomixignore(Repomix-specific).ignore(shared with ripgrep/ag, when enabled).gitignoreand.git/info/exclude(when enabled)- Default patterns (lowest priority)
This order makes logical sense: user overrides → tool-specific → tool-shared → VCS → defaults.
tests/core/file/fileSearch.test.ts (3)
56-68: LGTM! Comprehensive test for both flags enabled.The test correctly verifies that when both
useGitignoreanduseDotIgnorearetrue, all three ignore file patterns are returned in the expected priority order:.repomixignore→.ignore→.gitignore.
70-82: LGTM! Correct test for selective flag usage.The test correctly verifies that when
useDotIgnoreistruebutuseGitignoreisfalse, the result includes.repomixignoreand.ignorebut excludes.gitignore. This validates the independent control of each ignore source.
84-96: LGTM! Essential test for the new flag.This new test correctly verifies that when
useDotIgnoreisfalse, the.ignorefile patterns are excluded from the result, even whenuseGitignoreistrue. This ensures users can opt out of.ignoresupport if needed.The test completes the coverage matrix for the new feature and validates the backward compatibility path for users who don't want this behavior.
Fixed the priority order of ignore files to match the intended behavior: - .gitignore (lowest priority) - .ignore (medium priority) - .repomixignore (highest priority) The previous implementation had .repomixignore at the lowest priority, which was incorrect. Repomix-specific ignore rules should take precedence over generic ignore files. This ensures that: 1. .repomixignore can override .ignore and .gitignore rules 2. .ignore can override .gitignore rules 3. The priority order documented in README is correctly implemented
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #938 +/- ##
==========================================
+ Coverage 89.52% 89.62% +0.10%
==========================================
Files 111 111
Lines 7830 7839 +9
Branches 1499 1501 +2
==========================================
+ Hits 7010 7026 +16
+ Misses 820 813 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ignoreFilePatterns.push('**/.ignore'); | ||
| } | ||
|
|
||
| ignoreFilePatterns.push('**/.repomixignore'); |
There was a problem hiding this comment.
Critical Issue: Incorrect Priority Order
The current implementation adds .repomixignore last to the ignoreFilePatterns array, which means it has the lowest priority when globby processes the ignore files. However, according to the README documentation (line 1338-1342), the intended priority order is:
- Custom patterns (highest)
.repomixignore.ignore.gitignore(lowest)
Why this is a problem:
When globby processes ignoreFiles, it typically applies them in order, with later files overriding earlier ones in terms of priority. By placing .repomixignore last, it ends up with the highest priority among the ignore files, which is correct. However, the ordering here is confusing.
Actually, I need to verify this: Looking at globby's behavior, ignore files are typically additive (patterns accumulate), not override-based. The priority should actually be controlled by the order patterns are added to the main ignore array in getIgnorePatterns, not the ignoreFiles array.
The Real Issue:
The priority ordering is actually handled correctly here for globby's ignoreFiles option (which just tells globby which files to read patterns from). The actual priority is determined by the order patterns are added in getIgnorePatterns() function (lines 311-357).
However, there's still a documentation inconsistency: The code adds .repomixignore last (line 306), but this doesn't reflect the documented priority since globby reads all ignore files and merges their patterns.
Recommendation:
For clarity and to match the documented priority, consider reordering to:
// Add in reverse priority order (highest priority last for clarity)
if (config.ignore.useGitignore) {
ignoreFilePatterns.push('**/.gitignore');
}
if (config.ignore.useDotIgnore) {
ignoreFilePatterns.push('**/.ignore');
}
ignoreFilePatterns.push('**/.repomixignore');This way, the code visually reflects the intended priority order even though globby treats all ignoreFiles equally by reading and merging their patterns.
| cliConfig.ignore = { ...cliConfig.ignore, useGitignore: options.gitignore }; | ||
| } | ||
| // Only apply dotIgnore setting if explicitly set to false | ||
| if (options.dotIgnore === false) { |
There was a problem hiding this comment.
Good: Consistent with existing patterns
This implementation correctly mirrors the handling of gitignore and defaultPatterns options. The pattern of only applying when explicitly set to false allows config file values to take precedence unless overridden via CLI.
Minor suggestion: Consider adding a comment explaining this behavior for future maintainers:
// Only apply dotIgnore setting if explicitly set to false
// This allows config file values to take precedence when the flag is not specified
if (options.dotIgnore === false) {
cliConfig.ignore = { ...cliConfig.ignore, useDotIgnore: options.dotIgnore };
}
| }); | ||
| const filePatterns = await getIgnoreFilePatterns(mockConfig); | ||
| expect(filePatterns).toEqual(['**/.gitignore', '**/.repomixignore']); | ||
| expect(filePatterns).toEqual(['**/.gitignore', '**/.ignore', '**/.repomixignore']); |
There was a problem hiding this comment.
Test Coverage Issue: Order Matters
While this test verifies that all three ignore file patterns are included, it should also verify the order to ensure the priority is correct.
Consider updating to:
expect(filePatterns).toEqual(['**/.gitignore', '**/.ignore', '**/.repomixignore']);
// Or better yet, add a comment explaining why order matters:
// Order should reflect priority: .gitignore (lowest) -> .ignore -> .repomixignore (highest)However, I need to verify if globby's ignoreFiles option respects order or just merges all patterns. If it merges, order doesn't matter for this array, and the test is fine as-is.
| @@ -1323,6 +1325,7 @@ Repomix offers multiple methods to set ignore patterns for excluding specific fi | |||
| process: | |||
|
|
|||
There was a problem hiding this comment.
Documentation Enhancement Suggestion
Great addition of .ignore file support documentation! Consider adding a note about compatibility/precedence when files conflict:
- **.ignore**: You can use a `.ignore` file in your project root, following the same format as `.gitignore`. This file is respected by tools like ripgrep and the silver searcher, reducing the need to maintain multiple ignore files. Patterns in `.ignore` take precedence over `.gitignore` but are overridden by `.repomixignore`. This behavior can be controlled with the `ignore.useDotIgnore` setting or the `--no-dot-ignore` cli option.This clarifies what happens when the same pattern appears in multiple files.
Code Review SummaryGreat work on implementing .ignore file support! This is a valuable feature that improves compatibility with popular tools like ripgrep and The Silver Searcher. The implementation follows the project's conventions well and is mostly solid. Strengths
Key FindingsMedium Priority - Priority Order Clarity (src/core/file/fileSearch.ts:295-309): The current implementation adds ignore files but doesn't clearly reflect the documented priority order. While globby may treat all ignoreFiles equally by merging their patterns, the code should be reordered for clarity to match the documentation. Low Priority - Enhanced Documentation: Consider clarifying what happens when patterns conflict across different ignore files. Low Priority - Test Coverage: Consider adding integration tests for conflicting patterns across multiple ignore files. Premortem Analysis
RecommendationsBefore Merge:
Future Enhancements:
Overall AssessmentVerdict: Approve with minor suggestions This is a well-implemented feature that adds real value. The code quality is good, tests are comprehensive, and documentation is thorough. The priority order issue is primarily about code clarity rather than a functional bug. The feature successfully achieves the goal stated in issue 937: allowing users to maintain a single .ignore file across multiple tools. Great work! |
Updated English and Japanese documentation to include: - ignore.useDotIgnore configuration option - --no-dot-ignore CLI flag - .ignore file explanation and priority order - Benefits of using .ignore file with ripgrep and ag tools Other languages will be updated separately to maintain consistency across all locales.
Code Review SummaryThis PR successfully adds .ignore file support to Repomix, which is a valuable feature for users of tools like ripgrep and the silver searcher. The implementation is mostly well-structured with proper configuration options, CLI flags, comprehensive tests, and documentation updates. However, I've identified one critical issue that needs to be addressed before merging. Critical Issue: Ignore File Priority Order MismatchThe implementation order in src/core/file/fileSearch.ts does not match the documented priority order in the README and documentation. Documented Priority (README.md:1339-1343):
Actual Implementation (fileSearch.ts:298-306): This is backwards from the documented priority. Impact:
Action Required: Please verify how globby processes the ignoreFiles array order and ensure the implementation matches the documented priority. The order in the array matters for pattern precedence. Positive AspectsCode Quality:
Documentation:
Premortem AnalysisPotential Failure Scenarios:
Minor Suggestions
Checklist Before Merge
ConclusionThis is a well-implemented feature that addresses a real user need (issue #937). The code follows project conventions and has good coverage. The only blocker is resolving the priority order discrepancy. Once the critical issue is addressed, this PR will be ready to merge. Great work! 👍 |
| } | ||
|
|
||
| if (config.ignore.useDotIgnore) { | ||
| ignoreFilePatterns.push('**/.ignore'); |
There was a problem hiding this comment.
The array order here determines how globby processes ignore files. Please verify that this order matches the documented priority in README.md:
Documented priority (highest to lowest):
.repomixignore.ignore.gitignore
Current implementation adds:
.gitignore(if enabled).ignore(if enabled) ← this line.repomixignore
If globby processes ignoreFiles such that later entries override earlier ones, then .repomixignore being last is correct. However, if earlier entries take precedence, this order needs to be reversed.
Please verify and add a comment explaining the order, e.g.:
// Order matters: globby processes these files in order,
// with later files overriding earlier onesUpdated documentation for all supported languages to include: - ignore.useDotIgnore configuration option - --no-dot-ignore CLI flag - .ignore file explanation and priority order - Benefits of using .ignore file with ripgrep and ag tools Also updated test files to include useDotIgnore in mock configs to satisfy TypeScript type checking after adding the new configuration option. Languages updated: - zh-cn (Simplified Chinese) - zh-tw (Traditional Chinese) - ko (Korean) - vi (Vietnamese) - id (Indonesian) - hi (Hindi) - fr (French) - de (German) - es (Spanish) - pt-br (Brazilian Portuguese)
Added explanatory comment in getIgnoreFilePatterns function to document that files added later in the array have higher priority, following the behavior of tools like ripgrep and fd.
Code Review - PR #938: Add .ignore file supportI've completed a thorough review of this PR. The implementation is well-structured and follows best practices, with comprehensive test coverage and documentation updates across all languages. Below is my feedback focusing on areas not already covered by other reviewers. ✅ Strengths1. Excellent Implementation Quality
2. Strong Test CoverageThe test suite covers:
All test mock configs have been properly updated to include 3. Clear User CommunicationDocumentation clearly explains:
🔍 Areas for Improvement1. Missing Integration Test Coverage (Medium Priority)DetailsWhile unit tests are comprehensive, there's no integration test demonstrating the actual Suggestion: Add an integration test in
This would catch issues like:
Example structure: test('should respect .ignore file patterns', async () => {
// Create temp dir with .ignore file containing "*.log"
// Create test.log and test.js files
// Run pack
// Verify test.log is excluded, test.js is included
});2. Documentation: Priority Order Clarity (Low Priority)DetailsThe documented priority order states:
However, in While globby likely processes these in reverse order (last wins), this could be confusing for future maintainers. Suggestion: Add a code comment in export const getIgnoreFilePatterns = async (config: RepomixConfigMerged): Promise<string[]> => {
const ignoreFilePatterns: string[] = [];
// Note: These are added in reverse priority order because globby processes
// them last-to-first, giving later files higher precedence
if (config.ignore.useGitignore) {
ignoreFilePatterns.push('**/.gitignore'); // Lowest priority
}
if (config.ignore.useDotIgnore) {
ignoreFilePatterns.push('**/.ignore'); // Medium priority
}
ignoreFilePatterns.push('**/.repomixignore'); // Highest priority
return ignoreFilePatterns;
};3. Edge Case: Both
|
Code Review SummaryI've completed a thorough review of this PR. Overall, this is a well-implemented feature that adds ✅ Strengths
🔍 Key Review Findings1. Priority Order Documentation vs Implementation MismatchIssue: There's a discrepancy between the documented priority order and the actual implementation. Documentation says (README.md, line 1340-1342): Implementation does (src/core/file/fileSearch.ts:295-313): // Comment says: Priority order (low to high): .gitignore -> .ignore -> .repomixignore
// Array order: [.gitignore, .ignore, .repomixignore]Analysis: The code comment at line 298-301 correctly explains that "files added later in the array have higher priority." This means the actual priority is:
This contradicts the README documentation which states .repomixignore should come before .ignore. Impact: This could cause unexpected behavior where .ignore patterns override .repomixignore patterns, which is counter-intuitive since .repomixignore is tool-specific and should have the highest priority. Recommendation: Either:
I believe the documentation intent is correct - tool-specific files should have priority over general-purpose files. 2. Missing Integration TestObservation: While unit tests are comprehensive, there's no integration test demonstrating actual .ignore file behavior in a real directory structure. Suggestion: Consider adding a test in
This would catch issues like the priority order problem mentioned above. 3. Schema Version ConsiderationQuestion: The schema files updated are:
Should this change introduce a new schema version (e.g., 1.9.0) since it adds a new configuration field? This depends on your versioning policy for schemas. 🎯 Premortem Analysis: Potential Failure ScenariosClick to expand
📝 Minor Observations
🚀 RecommendationsMust address before merge:
Nice to have:
Overall Assessment: This is high-quality work that's almost ready to merge. The priority order issue needs resolution, but otherwise the implementation is solid. Great job maintaining consistency with existing patterns! 👍 |
Correction to Previous ReviewI apologize for the confusion in my previous comment. After re-reading the code more carefully, I need to correct my analysis: ✅ Priority Order is Actually Correct!The implementation in How it works:
Documentation states (README line 1340-1342):
Wait, there IS still a mismatch! According to the docs, the order should be:
But the code gives:
Actually, the numbering in the docs can be confusing. Let me clarify:
So:
The implementation IS correct! ✅Please disregard my previous concern about the priority order - there is no bug here. My apologies for the confusion! The PR is well-implemented and ready to merge after addressing the minor suggestions (integration tests, schema versioning clarification). |
Final Review Summary - Corrected Analysis ✅After careful review and re-analysis, I can confirm this PR is well-implemented and ready to merge. Priority Order Analysis - RESOLVED ✅I initially flagged a potential priority mismatch, but after deeper analysis, the implementation is correct: Code behavior (
Documentation (README:1340-1342):
Conclusion: Documentation and implementation are aligned. The tool-specific Security Review ✅
Performance Review ✅
Test Coverage ✅
Suggestions for Future Enhancement (Optional)
Recommendation: ✅ APPROVE AND MERGE This is clean, well-tested code that follows project conventions perfectly. The feature addresses issue #937 completely and maintains full backward compatibility. Great work @yamadashy! 🎉 |
Corrected the comment in getIgnoreFilePatterns to accurately reflect how globby's ignoreFiles option works: - Priority is determined by directory hierarchy (deeper directories have higher priority) - Multiple ignore files in the same directory are merged together - The order in the ignoreFiles array does not affect priority This fixes the misleading comment that suggested array order determines priority, when in fact globby uses fast-glob to find all matching files and merges them regardless of the array order.
Code Review - PR #938: Add .ignore file supportSummaryThis PR successfully adds support for ✅ StrengthsImplementation Quality
Best Practices Followed
🔍 Code AnalysisPriority Order Verification (src/core/file/fileSearch.ts:295-316) CLI Integration (src/cli/actions/defaultAction.ts:130-133) 🎯 Premortem AnalysisPotential Failure Scenarios & Mitigations1. User Confusion: Ignore File PriorityRisk: Users may not understand how
2. Migration Impact: Existing .ignore FilesRisk: Users who already have
3. Performance: Multiple Ignore File ProcessingRisk: Reading three ignore files instead of two could impact performance 4. Edge Case: Conflicting PatternsRisk:
5. Integration Risk: MCP ServerRisk: MCP server documentation needs 🚀 Suggestions for EnhancementNon-blocking improvements
🔒 Security Assessment
📊 Performance Considerations
✅ Final VerdictThis is a high-quality PR that:
Recommendation: ✅ Approve and merge after addressing any inline comments from other reviewers. The implementation is production-ready and the premortem analysis reveals only minor, low-probability risks that are already well-mitigated. Great work on this feature! The consistency with existing patterns and attention to documentation detail is exemplary. 🎉 |
Updated priority order documentation in README.md and all website configuration guides (12 languages) to accurately reflect how globby handles ignore files: - Merged items 2-4 into a single "Ignore files" category - Clarified that directory hierarchy determines priority (deeper directories win) - Explained that files in the same directory are merged in no particular order - This reflects globby's actual behavior where fast-glob returns files in filesystem order The previous documentation incorrectly implied a fixed priority order between .repomixignore, .ignore, and .gitignore files, when in fact they are all merged together at the same level with priority determined by directory depth only.
Code Review SummaryI've reviewed this PR and overall it's a solid implementation that adds .ignore file support to Repomix. The feature is well-implemented with good test coverage and comprehensive documentation across all supported languages. However, I've identified some important issues that should be addressed before merging, particularly around test coverage completeness and potential edge cases. Critical Issues1. Incomplete Test Coverage for useDotIgnore PropertySeveral test files are missing the useDotIgnore property in their mock configurations. While these tests may still pass due to default values, this creates implicit dependencies on defaults and makes the tests less maintainable. Files affected:
Risk: If the default value for useDotIgnore changes in the future, these tests won't explicitly verify the intended behavior. Recommendation: Add useDotIgnore: true explicitly to all mock configs in test files for consistency and clarity. Code Quality ObservationsPositive Aspects ✅
Areas for Improvement 🔧
Security & Performance✅ No security concerns identified RecommendationsMust Fix Before Merge 🔴
Should Consider 🟡
Nice to Have 🟢
ConclusionThis is a well-implemented feature that adds genuine value. The code quality is high, and the documentation is excellent. With the minor test improvements suggested above, this will be ready to merge. Estimated Risk Level: Low Great work @yamadashy! 🎉 |
Premortem Analysis: Potential Failure ScenariosAs requested, here's a detailed analysis of potential failure scenarios and suggested mitigations: 1. Conflicting Patterns Across Ignore FilesScenario: User has contradictory patterns in .ignore, .gitignore, and .repomixignore in the same directory Example:
Risk: Unexpected file inclusion/exclusion due to pattern merging behavior Likelihood: Medium - users transitioning from other tools may have different patterns Mitigation:
2. Migration Confusion for Existing UsersScenario: Users who already have .ignore files for other tools may be surprised by Repomix suddenly respecting them after upgrade Example: A user has .ignore file for ripgrep that excludes test files. After upgrading Repomix, their test files are suddenly not included in the pack, breaking their workflow. Risk: Files that were previously included may now be excluded, potentially breaking automated workflows Likelihood: Medium-High for repos already using .ignore files Mitigation:
3. Nested Directory Priority ComplexityScenario: Deep directory structure with multiple .ignore files at different levels Risk: Users may not understand which patterns take precedence, leading to confusion about why files are excluded Likelihood: Low-Medium - most projects don't have deeply nested ignore files Mitigation:
4. CLI Flag Interaction Edge CasesScenario: User specifies --no-gitignore and --no-dot-ignore but has .repomixignore Risk: Unclear whether all ignore files are disabled or just specific ones Likelihood: Low - most users use defaults Mitigation:
5. Deployment/CI Pipeline ImpactScenario: CI/CD pipeline relies on specific files being included in Repomix output. After upgrade, .ignore file causes those files to be excluded. Risk: Automated pipelines break after Repomix upgrade Likelihood: Low - but high impact if it occurs Mitigation:
Summary of Action ItemsCritical (Must Address)
High Priority (Should Address)
Medium Priority (Nice to Have)
Overall Risk Assessment: Low-Medium The implementation is solid and follows good patterns. The main risks are around user migration and documentation. With proper release notes and the suggested test improvements, the risk becomes very low. |
Detailed Code Review: File-by-File Analysissrc/core/file/fileSearch.ts:295-316✅ Excellent implementation The code follows the existing pattern perfectly and includes helpful comments explaining the priority behavior. The comment on lines 298-303 is particularly valuable for future maintainers. Minor suggestion: Consider adding a code example in the comment showing actual file paths to make it even clearer: src/cli/actions/defaultAction.ts:130-133✅ Consistent with existing patterns The implementation correctly mirrors the gitignore flag handling. Good use of explicit false check to avoid overriding with undefined. No issues found. src/config/configSchema.ts:59,115✅ Proper schema definition The useDotIgnore property is correctly added to both the base schema (optional) and default schema (with default value true). No issues found. tests/core/file/fileSearch.test.tsWhile the PR adds good test coverage for the new .ignore functionality (lines 56-96), several existing tests were not updated to include the new useDotIgnore property in their mock configs. Tests missing useDotIgnore:
Impact: These tests will pass because useDotIgnore defaults to true, but they're relying on implicit behavior rather than explicit configuration. This makes the tests fragile and harder to understand. Recommendation: Add useDotIgnore: true explicitly to all mock configs for consistency and maintainability. README.md & Documentation Files✅ Excellent documentation coverage The documentation has been comprehensively updated across:
Particularly good:
Minor suggestions:
Schema Files (website/client/src/public/schemas/)✅ Correctly updated Both latest and version-specific (1.8.0) schema files have been updated with the useDotIgnore property. No issues found. Code Style & Best Practices Compliance✅ Follows CLAUDE.md Guidelines
✅ TypeScript Best Practices
✅ Airbnb Style Guide ComplianceNo style violations detected in the implementation. Test Coverage AnalysisCoverage added:
Coverage gaps:
Performance AnalysisImpact: Negligible The changes add:
These operations are O(1) and occur once per run during initialization. No performance concerns. ConclusionThis is a high-quality implementation that demonstrates good engineering practices. The main area for improvement is test coverage completeness. Once the test coverage gaps are addressed, this PR will be in excellent shape for merging. Recommendation: Approve with minor test improvements |
Added unit tests for buildCliConfig function to cover --no-gitignore, --no-dot-ignore, and --no-default-patterns flags. These tests ensure proper handling of ignore configuration options and improve test coverage to meet the 80% threshold. This addresses the Codecov coverage gap where src/cli/actions/defaultAction.ts lines 131-132 were not covered by tests.
Code Review SummaryI've reviewed PR #938 which adds ✅ Strengths
🔍 Code Quality ObservationsConfiguration SchemaThe schema changes in
Code reference: CLI IntegrationThe CLI handling in if (options.dotIgnore === false) {
cliConfig.ignore = { ...cliConfig.ignore, useDotIgnore: options.dotIgnore };
}This ensures the config file value is respected when the flag isn't used. Code reference:
|
This PR adds support for
.ignorefiles, which are used by tools like ripgrep and the silver searcher. This allows users to maintain a single.ignorefile that works across multiple tools instead of maintaining separate ignore files.Changes
ignore.useDotIgnoreconfiguration option (default:true)--no-dot-ignoreCLI flag to disable.ignorefile usage.repomixignore.ignore(new).gitignore.ignorefile handling.ignorefile supportBenefits
.ignorefile that works with ripgrep, ag, and repomix.gitignoreand.repomixignorefiles continue to workignore.useDotIgnore: falseor--no-dot-ignoreflagChecklist
npm run testnpm run lintResolves #937