Fix glob translation: leading/embedded **/ matches zero or more dirs#4
Conversation
`globToRegExp` in suppress.ts translated `**` to `.*` unconditionally, so a
gitignore-style `**/Button.tsx` became `^.*/Button.tsx$` — which requires a
slash and therefore failed to match a top-level `Button.tsx`. Likewise
`src/**` did not match `src` itself.
Translate the `**` variants with gitignore semantics:
- leading/embedded `**/` -> `(?:.*/)?` (zero or more path segments), so
`**/Button.tsx` matches both `Button.tsx` and `a/b/Button.tsx`.
- trailing `/**` -> `(?:/.*)?`, so `src/**` matches `src` and `src/a.tsx`.
- bare `**` -> `.*`; single `*` -> `[^/]*` (unchanged).
Tokenise via control-char sentinels before escaping regex specials so each
variant is translated exactly once with no collisions.
Adds tests for top-level, nested, embedded, and single-segment matches.
Finding #15 from /review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds 36 lines to ChangesGlob suppression path-matching tests
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 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)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/comment-core.test.ts`:
- Around line 86-117: Add a new test case after the existing pattern tests that
explicitly tests bare ** glob semantics, which is not currently covered. Create
a test with a description like 'bare ** matches files and directories at any
nesting level' and test patterns such as path:**Button.tsx or path:** using the
same structure as the other tests (parseSuppressions followed by multiple
isSuppressed assertions with different file paths to verify the expected
matching behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ec5f1a2-50af-46b0-b1de-3ce0eabdaf19
⛔ Files ignored due to path filters (2)
dist/cli/index.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
src/comment/suppress.tstests/comment-core.test.ts
| it('leading **/ matches a top-level file and any nested depth', () => { | ||
| const s = parseSuppressions('path:**/Button.tsx'); | ||
| expect(isSuppressed(at('Button.tsx'), s)).toBe(true); // top-level, no slash | ||
| expect(isSuppressed(at('a/Button.tsx'), s)).toBe(true); // one dir deep | ||
| expect(isSuppressed(at('a/b/Button.tsx'), s)).toBe(true); // many dirs deep | ||
| expect(isSuppressed(at('a/Card.tsx'), s)).toBe(false); // different file | ||
| expect(isSuppressed(at('a/NotButton.tsx'), s)).toBe(false); // * stays within a segment | ||
| }); | ||
|
|
||
| it('trailing /** matches the directory itself and everything under it', () => { | ||
| const s = parseSuppressions('src/**'); | ||
| expect(isSuppressed(at('src'), s)).toBe(true); // the directory itself | ||
| expect(isSuppressed(at('src/a.tsx'), s)).toBe(true); // a file under it | ||
| expect(isSuppressed(at('src/legacy/b.tsx'), s)).toBe(true); // nested under it | ||
| expect(isSuppressed(at('source/a.tsx'), s)).toBe(false); // not a prefix-name match | ||
| expect(isSuppressed(at('lib/a.tsx'), s)).toBe(false); // unrelated dir | ||
| }); | ||
|
|
||
| it('embedded **/ collapses to zero or more middle segments', () => { | ||
| const s = parseSuppressions('path:src/**/Button.tsx'); | ||
| expect(isSuppressed(at('src/Button.tsx'), s)).toBe(true); // zero middle dirs | ||
| expect(isSuppressed(at('src/ui/Button.tsx'), s)).toBe(true); // one middle dir | ||
| expect(isSuppressed(at('src/ui/forms/Button.tsx'), s)).toBe(true); // several middle dirs | ||
| expect(isSuppressed(at('lib/Button.tsx'), s)).toBe(false); // wrong root | ||
| }); | ||
|
|
||
| it('single * stays within one path segment', () => { | ||
| const s = parseSuppressions('path:src/*.tsx'); | ||
| expect(isSuppressed(at('src/Button.tsx'), s)).toBe(true); | ||
| expect(isSuppressed(at('src/legacy/Button.tsx'), s)).toBe(false); // * does not cross / | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add an explicit regression test for bare ** glob semantics.
These tests cover **/, /**, embedded **/, and *, but not bare ** (which has distinct translation logic). Adding one case (e.g., path:**Button.tsx or path:**) would lock down the remaining branch and prevent silent regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/comment-core.test.ts` around lines 86 - 117, Add a new test case after
the existing pattern tests that explicitly tests bare ** glob semantics, which
is not currently covered. Create a test with a description like 'bare ** matches
files and directories at any nesting level' and test patterns such as
path:**Button.tsx or path:** using the same structure as the other tests
(parseSuppressions followed by multiple isSuppressed assertions with different
file paths to verify the expected matching behavior).
…ing-dirs # Conflicts: # dist/cli/index.js # dist/index.js # src/comment/suppress.ts
1 participant
Finding #15 (from
/review)globToRegExpinsrc/comment/suppress.tstranslated**to.*unconditionally. A gitignore-style
path:**/Button.tsxbecame^.*/Button.tsx$, which requires a leading slash and therefore does notmatch a top-level
Button.tsx. Likewisesrc/**did not matchsrcitself.Fix
Translate the
**variants with gitignore semantics:**/(?:.*/)?**/Button.tsxmatchesButton.tsxanda/b/Button.tsx/**(?:/.*)?src/**matchessrcandsrc/a.tsx**.*/(unchanged)*[^/]*The glob is tokenised with control-char sentinels before escaping regex
specials, so each variant is translated exactly once with no collisions.
Tests
Added cases to
tests/comment-core.test.tscovering:**/matching top-level and nested files (and*not crossing/)/**matching the directory itself + nested, without prefix-name false positives**/collapsing to zero-or-more middle segments*staying within one path segmentnpm run typecheck && npm test && npm run build:allall pass (121 tests); rebuiltdist/is included.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes