feat(core): Improve .gitignore handling with globby v16#964
Conversation
|
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. WalkthroughUpgrades Changes
Sequence DiagramsequenceDiagram
participant caller as Caller
participant search as searchFiles<br/>listFiles<br/>listDirectories
participant helper as createBaseGlobbyOptions
participant globby as globby
caller->>search: Request with config
search->>helper: Call to get base options
helper->>helper: Normalize ignore patterns<br/>Handle git worktree<br/>Build options object
helper-->>search: Return base options
search->>globby: Call with base options +<br/>patterns
globby-->>search: File list / Error
search-->>caller: Return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
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 significantly enhances file and directory search capabilities by upgrading the 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
|
Deploying repomix with
|
| Latest commit: |
b99d083
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fde9e2af.repomix.pages.dev |
| Branch Preview URL: | https://feat-improve-ignore.repomix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
==========================================
+ Coverage 89.68% 90.31% +0.62%
==========================================
Files 111 111
Lines 7843 7813 -30
Branches 1504 1506 +2
==========================================
+ Hits 7034 7056 +22
+ Misses 809 757 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 effectively upgrades globby to v16 and refactors the file searching logic to correctly handle .gitignore files, aligning with standard Git behavior. The introduction of the createBaseGlobbyOptions helper function is a good step towards improving maintainability by centralizing common options. The accompanying tests are comprehensive and cover the new functionality and refactoring well. I have one suggestion to further improve maintainability by extracting duplicated logic for preparing ignore patterns.
Code Review SummaryGreat work on this upgrade! The implementation is solid with excellent test coverage. I've identified a few areas for consideration: ✅ Strengths
🔍 Detailed AnalysisCode QualityPositive:
Potential Issues:
Test CoverageExcellent additions:
Note: Tests use .runIf(!isWindows) for gitignore tests, which is appropriate given platform differences Performance Considerations
Security Concerns✅ No security issues identified. The upgrade:
🎯 Premortem Analysis: Potential Failure Scenarios1. Performance Degradation on Large ReposRisk: Medium
2. Behavioral Breaking Change for UsersRisk: Medium
3. Platform-Specific IssuesRisk: Low
4. Integration Issues with Other Globby OptionsRisk: Low
5. Git Worktree Edge CasesRisk: Low
📝 Recommendations
✨ VerdictLGTM with minor suggestions. This is a well-executed upgrade with thoughtful refactoring and comprehensive testing. The behavioral change is properly documented and the code quality is high. Reviewed following CLAUDE.md guidelines |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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
♻️ Duplicate comments (1)
src/core/file/fileSearch.ts (1)
148-176: Consider extracting shared ignore‑context preparation to reduce duplication
searchFiles,listDirectories, andlistFilesall duplicate the same sequence:getIgnorePatterns/getIgnoreFilePatterns,normalizeGlobPattern, and git worktree adjustment logic. Pulling this into a small helper (e.g.,prepareIgnoreContext(rootDir, config)) would keep future changes to ignore behavior in one place and make the globby call sites even thinner.Also applies to: 383-405, 423-445
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/core/file/fileSearch.ts(7 hunks)tests/core/file/fileSearch.test.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/file/fileSearch.ts (1)
src/config/configSchema.ts (1)
RepomixConfigMerged(157-157)
🪛 GitHub Actions: CI
tests/core/file/fileSearch.test.ts
[error] 730-730: TS18048: 'options' is possibly 'undefined'.
🪛 GitHub Check: Lint TypeScript
tests/core/file/fileSearch.test.ts
[failure] 731-731:
'options' is possibly 'undefined'.
[failure] 730-730:
'options' is possibly 'undefined'.
⏰ 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 (4)
package.json (1)
83-91: globby v16 dependency bump looks consistent with the implementationThe upgrade to
"globby": "^16.0.0"matches the new typedGlobbyOptionsusage and gitignore-related behavior infileSearch.ts; nothing blocking here from a manifest perspective.tests/core/file/fileSearch.test.ts (2)
12-14: Tests correctly reflect new gitignore vs ignoreFiles splitThe updated
getIgnoreFilePatternstests and comments align with the implementation:.gitignoreis no longer part ofignoreFilesand is governed purely by thegitignoreoption, while.ignoreand.repomixignoreare controlled by config flags. This gives good coverage of the new ignore‑file behavior.If you’d like, you can also add a small assertion in one test that
gitignoretoggling actually flips thegitignoreflag in the globby options (similar to later tests), to more tightly couple these tests to the runtime behavior.Also applies to: 58-71, 73-85, 87-100
287-289: Parent .gitignore behavior and gitignore flag coverage look solidThe new tests that (1) assert
gitignore: trueis present in the globby options and (2) simulate a parent.gitignorefile filtering nested files give good regression protection for the globby v16 change around gitignore handling. TherunIf(!isWindows)guard is also appropriate given path‑separator differences.It’s worth re‑running these tests against an actual repo with nested
.gitignorefiles to confirm that the mocked behavior matches globby v16’s real behavior on your target platforms.Also applies to: 343-398
src/core/file/fileSearch.ts (1)
4-5: Verify performance implications ofgitignore: trueconditional usageVerification against globby v16 docs shows that the recommended approach for combining
.gitignorewith custom ignore files is to useignoreFileswith.gitignoreexplicitly listed (e.g.,ignoreFiles: ['.gitignore', '.ignore', ...]) rather thangitignore: true, which can incur scanning overhead.Your implementation uses
gitignore: config.ignore.useGitignoreconditionally alongsideignoreFiles: [...ignoreFilePatterns]. The code comment explains this is intentional—to properly respect parent directory.gitignorefiles the way Git does. This is a legitimate design choice but comes with a performance tradeoff when the gitignore option is enabled.If performance is a concern and Git behavior compatibility is not a hard requirement, consider whether switching to explicit
ignoreFiles: ['.gitignore', '**/.ignore', '**/.repomixignore'](if globby supports it) would be acceptable. Otherwise, document this tradeoff clearly so future maintainers understand the choice.
This commit addresses three suggestions from AI code review bots on PR #964: 1. Remove unnecessary array spreads in createBaseGlobbyOptions - Removed defensive copying of ignorePatterns and ignoreFilePatterns - Arrays are already created fresh in calling functions, making spreads redundant - Minor performance optimization by avoiding unnecessary array allocations 2. Extract prepareIgnoreContext helper function - Centralized duplicate ignore pattern preparation logic - Eliminated code duplication across searchFiles, listDirectories, and listFiles - The new helper handles: * Getting ignore patterns and ignore file patterns * Normalizing patterns for consistent trailing slash handling * Git worktree special case handling - Improves maintainability and ensures consistency across all globby calls 3. Add explanatory comment to v16 behavior test - Documented why v16's behavior is superior (matches Git's standard behavior) - Clarifies that v16 respects parent directory .gitignore files - Helps future maintainers understand the intentional breaking change All 856 tests pass with no regressions.
Code Review - Incremental FeedbackThis review focuses on new issues not previously covered by other reviewers. Critical Issue: TypeScript Lint ErrorMust fix before merge: The CI is failing due to a TypeScript error in tests/core/file/fileSearch.test.ts:730-731. See my inline comment for the fix - options needs a type guard since call[1] might be undefined. Premortem Analysis: Additional Failure ScenariosBuilding on previous reviews, here are NEW risks not yet discussed: 1. Globby v16 ignoreFiles Behavior Change (Risk: High) In v15, all ignore files went through ignoreFiles. In v16, you split them - .gitignore via gitignore:true, others via ignoreFiles. This could cause edge cases where parent directory .ignore files might not be respected the same way as parent .gitignore files. Mitigation: Add integration tests with nested .ignore and .repomixignore files to verify they work like .gitignore. 2. Node.js Version Compatibility (Risk: Medium) Globby v16 requires Node.js >=20 per package-lock.json. If Repomix doesn't already require Node 20+, this needs to be a MAJOR version bump (v2.0.0), not minor. Action: Verify package.json engines field and update release notes accordingly. 3. is-path-inside vs path-type Dependency Swap (Risk: Low) Globby v16 replaced path-type with is-path-inside. Could affect symlink handling and Windows junction points. Mitigation: Manual testing with symlinks recommended. 4. Git Worktree + Parent .gitignore Interaction (Risk: Low-Medium) The worktree handling code modifies patterns when .git is a worktree reference. With v16's parent .gitignore support, interactions need validation. Mitigation: Add test case for worktrees with parent directory .gitignore. Test Coverage GapsMissing tests for:
Positive Changes
Action Items Before Merge
Overall: Strong implementation with one blocking lint error. The refactoring significantly improves maintainability. |
|
✅ Node.js Version Check: Confirmed that package.json already requires Node >=20.0.0, so the globby v16 upgrade is compatible. No action needed on this front. |
- Upgrade globby from v15 to v16 - Use gitignore option to respect parent directory .gitignore files - This matches Git's standard behavior where parent .gitignore patterns apply to subdirectories - Move .gitignore handling from ignoreFiles to gitignore option - Update tests to reflect the new behavior This change improves compatibility with Git and provides more accurate file filtering when running Repomix in subdirectories.
- Add createBaseGlobbyOptions function to centralize common globby options - Use Omit<GlobbyOptions, 'onlyFiles' | 'onlyDirectories'> for type safety - Update all 4 globby calls to use the shared helper - This prevents configuration drift and makes future updates easier No behavior changes - all tests pass.
- Add test for parent directory .gitignore pattern handling (v16 behavior) - Add tests for createBaseGlobbyOptions consistency across all functions - Verify gitignore option is passed correctly to all globby calls - Ensure no regression from v15 to v16 upgrade These tests prove that: 1. Parent .gitignore files are respected with globby v16 2. All 4 globby calls (searchFiles files/dirs, listDirectories, listFiles) use consistent base options 3. gitignore configuration is applied uniformly across all functions All 856 tests pass, confirming no regression from the changes.
- Add website/server/dist/ to .gitignore for secretlint - Fix TypeScript type errors in fileSearch.test.ts - Format imports in fileSearch.ts (biome)
This commit addresses three suggestions from AI code review bots on PR #964: 1. Remove unnecessary array spreads in createBaseGlobbyOptions - Removed defensive copying of ignorePatterns and ignoreFilePatterns - Arrays are already created fresh in calling functions, making spreads redundant - Minor performance optimization by avoiding unnecessary array allocations 2. Extract prepareIgnoreContext helper function - Centralized duplicate ignore pattern preparation logic - Eliminated code duplication across searchFiles, listDirectories, and listFiles - The new helper handles: * Getting ignore patterns and ignore file patterns * Normalizing patterns for consistent trailing slash handling * Git worktree special case handling - Improves maintainability and ensures consistency across all globby calls 3. Add explanatory comment to v16 behavior test - Documented why v16's behavior is superior (matches Git's standard behavior) - Clarifies that v16 respects parent directory .gitignore files - Helps future maintainers understand the intentional breaking change All 856 tests pass with no regressions.
Fix TS18048 errors in createBaseGlobbyOptions consistency tests by adding expect(options).toBeDefined() and if (!options) continue guards. This ensures type safety and prevents undefined access to globby call options. All three tests now properly guard against potentially undefined options: - should use consistent base options across all globby calls - should respect gitignore config consistently across all functions - should apply custom ignore patterns consistently across all functions This addresses the coderabbitai feedback on PR #964.
yamadashy
force-pushed
the
feat/improve-ignore
branch
from
2554572 to
dd25bec
Compare
Code Review - PR #964I've reviewed this PR and identified some new issues not mentioned in previous reviews: 🐛 Issues Found
✅ Positive ObservationsClick to expandCode Quality:
Test Coverage:
Documentation:
🎯 Premortem AnalysisPotential failure scenarios not covered by previous reviews1. Monorepo Performance Impact
2. Symbolic Link Edge Cases
3. Case-Sensitive Filesystem Differences
4. Git Submodule Interactions
📋 Action ItemsMust Fix:
Nice to Have:
📊 Overall AssessmentVerdict: Approve with required fixes ✅ The core implementation is solid and well-tested. The globby v16 upgrade is properly handled with good backward compatibility considerations. Once the TypeScript linting error is fixed and the package.json change is addressed, this PR will be ready to merge. The behavioral change (respecting parent .gitignore files) is well-documented and appropriate for a minor version bump. Review conducted following CLAUDE.md guidelines |
Add comprehensive tests for parent directory ignore file handling to address PR #964 review feedback (Risk: High concern from claude[bot]). Added three new test cases: 1. Parent directory .ignore file handling - Verifies .ignore files in parent directories are respected - Tests with useDotIgnore: true configuration - Ensures patterns apply to nested subdirectories 2. Parent directory .repomixignore file handling - Verifies .repomixignore files in parent directories are respected - Tests default configuration (.repomixignore always enabled) - Ensures patterns apply to nested subdirectories 3. Git worktree + parent .gitignore interaction - Verifies worktree environments handle parent .gitignore correctly - Combines worktree detection with parent .gitignore pattern application - Tests that .git file (not directory) is properly handled in worktree - Ensures gitignore: true option enables parent .gitignore handling All tests follow the same pattern as existing "should respect parent directory .gitignore patterns (v16 behavior)" test, providing consistent coverage for .gitignore, .ignore, and .repomixignore files. These tests ensure that globby v16's parent directory ignore file handling works correctly for all supported ignore file types, not just .gitignore.
Code Review - Additional AnalysisExcellent work on the globby v16 upgrade! The refactoring is well-executed with comprehensive test coverage. Since other reviewers have covered many aspects, I'll focus on new observations: ✅ Strengths (Beyond What's Already Mentioned)
🔍 New Observations & Suggestions1. Documentation Enhancement OpportunityLocation: The comment explaining why .gitignore is handled differently is excellent, but consider adding a brief note about when this behavior changed (globby v16) to help future maintainers understand the evolution: // .gitignore files are handled by globby's gitignore option (not ignoreFiles)
// to properly respect parent directory .gitignore files, matching Git's behavior.
// This changed in globby v16 - previously .gitignore was treated like other ignore files.2. Potential Test Reliability IssueLocation: Test file lines 343-402, 404-462, 464-522 Risk Level: Low The parent directory ignore file tests use mocked Suggestion: Consider adding at least one integration test (perhaps in a separate suite) that uses the real globby library with actual temporary files and .gitignore to verify end-to-end behavior. This would catch any misunderstandings about how globby v16's gitignore option actually works. 3. Minor Code Clarity EnhancementLocation: The worktree handling logic uses array mutation ( const adjustedIgnorePatterns = isWorktree
? normalizedIgnorePatterns.map(p => p === '.git/**' ? '.git' : p)
: [...normalizedIgnorePatterns];This is purely stylistic - the current implementation is fine. 4. Coverage Gap: Multiple .gitignore FilesLocation: Test suite Risk Level: Medium The tests verify parent .gitignore behavior, but don't test scenarios where multiple .gitignore files exist at different levels (e.g., root/.gitignore and root/subdir/.gitignore with potentially conflicting patterns). Git has specific precedence rules for this - closer .gitignore files take precedence. Recommendation: Add a test case verifying that when a parent .gitignore ignores 5. Breaking Change CommunicationLocation: PR Description The PR description is excellent, but when this is released, ensure the release notes include:
🎯 Premortem: Additional Failure Scenarios6. Symbolic Link + .gitignore InteractionRisk: Low-Medium Mitigation:
7. .git/info/exclude CompatibilityRisk: Low Investigation needed: Verify whether globby v16's gitignore option already processes .git/info/exclude. If so, the current code might be redundant (though harmless). File: 8. Case-Sensitive Filesystem Edge CasesRisk: Low Note: This is a globby/Git behavior, not specific to this PR, but worth documenting if users report issues. 📊 Code Quality Metrics
🎬 Final VerdictLGTM - This is a high-quality PR with excellent refactoring and test coverage. The suggestions above are minor enhancements and edge cases to consider, not blockers. The code is ready to merge. The most important follow-ups are:
Estimated risk of production issues: Very Low (< 5%) Review completed following CLAUDE.md guidelines. Previous feedback from CodeRabbit and Gemini bots was considered to avoid duplication. |
…yamadashy#959) Add comprehensive test coverage to verify that .ignore files work correctly when customPatterns is defined in the configuration. The issue reported that .ignore files were ignored when customPatterns was set, but investigation shows this was resolved by the globby v16 upgrade in PR yamadashy#964. Changes: - Add config merge tests verifying useDotIgnore is preserved when customPatterns is defined (tests/config/configLoad.test.ts) - Add integration tests using real file system to verify .ignore, .repomixignore, and customPatterns are all merged correctly (tests/core/file/fileSearch.ignore.integration.test.ts) - Add clarifying comment in getIgnoreFilePatterns documenting that ignore file patterns are applied independently of customPatterns
Summary
This PR updates globby from v15 to v16 and improves .gitignore handling to match Git's standard behavior.
Key Changes
gitignore: trueoption to respect parent directory .gitignore files, matching Git's standard behaviorcreateBaseGlobbyOptionshelper function to centralize common globby options across all 4 calls (searchFiles files/dirs, listDirectories, listFiles)Breaking Change Context
While globby v16 introduced a breaking change in the
gitignoreoption behavior, this change aligns Repomix with Git's standard behavior (respecting parent directory .gitignore files). This is why we're releasing this as a MINOR version (v1.10.0) rather than PATCH.Benefits
Checklist
npm run test(856 tests passed)npm run lint(all checks passed)