fix: respect .ignore file when customPatterns is defined#1165
fix: respect .ignore file when customPatterns is defined#1165hztBUAA wants to merge 1 commit intoyamadashy:mainfrom
Conversation
…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 of ChangesHello @hztBUAA, 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 addresses an issue where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
📝 WalkthroughWalkthroughThis PR addresses issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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
The pull request provides excellent regression test coverage for the issue where .ignore files were being ignored when customPatterns was defined. It includes both configuration merging unit tests and comprehensive integration tests using real file system operations. The added documentation in fileSearch.ts clarifies the expected behavior. The changes are well-structured and follow the project's existing patterns. No issues of medium or higher severity were identified.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/file/fileSearch.ignore.integration.test.ts (1)
39-46: Consider usingcreateMockConfigconsistently instead of manual spread override.Test 1 uses
mergeConfigsthen manually overrides the result with spread syntax (Line 44), which bypasses the Zod schema validation thatmergeConfigsapplies. The other tests usecreateMockConfigdirectly. UsingcreateMockConfighere too would be more consistent and avoid the unvalidated override.That said, the intent to show the
mergeConfigspath for the exact#959reproduction scenario is understood.♻️ Proposed refactor using createMockConfig
- // Simulate: config file has { ignore: { customPatterns: ["bin/"] } } - const fileConfig: RepomixConfigFile = { - ignore: { customPatterns: ['bin/'] }, - }; - const config = mergeConfigs(tempDir, fileConfig, {}); - const testConfig = { ...config, ignore: { ...config.ignore, useGitignore: false, useDefaultPatterns: false } }; + // Simulate: config file has { ignore: { customPatterns: ["bin/"] } } + const testConfig = createMockConfig({ + cwd: tempDir, + ignore: { + useGitignore: false, + useDotIgnore: true, + useDefaultPatterns: false, + customPatterns: ['bin/'], + }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/file/fileSearch.ignore.integration.test.ts` around lines 39 - 46, Replace the manual spread override of the merged config with a call to createMockConfig to ensure Zod validation is applied: instead of calling mergeConfigs(tempDir, fileConfig) then constructing testConfig via { ...config, ignore: { ... } }, call createMockConfig(tempDir, fileConfig, { ignore: { useGitignore: false, useDefaultPatterns: false } }) (or equivalent createMockConfig overload) and pass that result to searchFiles; update references to fileConfig, mergeConfigs, testConfig, and searchFiles accordingly so the test uses the validated mock config consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/file/fileSearch.ignore.integration.test.ts`:
- Around line 39-46: Replace the manual spread override of the merged config
with a call to createMockConfig to ensure Zod validation is applied: instead of
calling mergeConfigs(tempDir, fileConfig) then constructing testConfig via {
...config, ignore: { ... } }, call createMockConfig(tempDir, fileConfig, {
ignore: { useGitignore: false, useDefaultPatterns: false } }) (or equivalent
createMockConfig overload) and pass that result to searchFiles; update
references to fileConfig, mergeConfigs, testConfig, and searchFiles accordingly
so the test uses the validated mock config consistently.
|
Thanks for the review and feedback. I am following up on this PR now and will either push the requested changes or reply point-by-point shortly. |
|
Quick follow-up: I am reviewing the feedback and will update this PR shortly. |
1 participant
Summary
Closes #959
The reported issue was that
.ignorefiles were being ignored whencustomPatternswas defined in the configuration. Investigation shows that this was resolved by the globby v16 upgrade in PR #964, which improved.gitignorehandling and moved it to globby's nativegitignoreoption.This PR adds regression test coverage to ensure the fix stays in place:
tests/config/configLoad.test.ts): Verify thatuseDotIgnoredefaults totrueand is preserved when onlycustomPatternsis defined in the file or CLI configtests/core/file/fileSearch.ignore.integration.test.ts): Use real file system operations to verify that.ignore,.repomixignore, andcustomPatternsare all properly merged and applied togethersrc/core/file/fileSearch.ts): Add clarifying comment ingetIgnoreFilePatternsdocumenting that ignore file patterns are applied independently ofcustomPatternsTest scenarios covered
.ignorerespected whencustomPatternsis defined via config file merge.ignorerespected whencustomPatternsis empty.ignoreNOT applied whenuseDotIgnoreis explicitlyfalse.ignore,.repomixignore,customPatterns) merged together.ignorefiles work alongsidecustomPatternscustomPatternsis providedChecklist
npm run testnpm run lint