fix: respect tsconfig exclude patterns in project ownership checks#46
fix: respect tsconfig exclude patterns in project ownership checks#46EladBezalel merged 3 commits intomainfrom
Conversation
Domino's file discovery includes all .ts/.tsx/.js/.jsx files regardless of tsconfig exclude patterns, causing false positives when transitive type dependency chains reach a project solely through excluded files (e.g. stories, specs). Traf avoids this by using addSourceFilesFromTsConfig which respects each project's tsconfig include/exclude. Add tsconfig exclude filtering to ProjectIndex so that files excluded by a project's tsconfig don't count toward marking that project as affected. All files are still parsed and included in the import index for cross-file reference accuracy — only the ownership check is filtered. Closes #45 Made-with: Cursor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughProjectIndex now loads per-project tsconfig excludes (via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tsconfig.rs">
<violation number="1" location="src/tsconfig.rs:47">
P2: Inherited excludes are matched against the child tsconfig directory, which misinterprets parent-defined relative exclude paths.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /// `extends` chain. Returns `None` if the tsconfig doesn't exist, can't be | ||
| /// parsed, or has no exclude patterns. | ||
| pub fn parse(tsconfig_path: &Path, cwd: &Path) -> Option<Self> { | ||
| let base_dir = tsconfig_path.parent()?.to_path_buf(); |
There was a problem hiding this comment.
P2: Inherited excludes are matched against the child tsconfig directory, which misinterprets parent-defined relative exclude paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tsconfig.rs, line 47:
<comment>Inherited excludes are matched against the child tsconfig directory, which misinterprets parent-defined relative exclude paths.</comment>
<file context>
@@ -0,0 +1,372 @@
+ /// `extends` chain. Returns `None` if the tsconfig doesn't exist, can't be
+ /// parsed, or has no exclude patterns.
+ pub fn parse(tsconfig_path: &Path, cwd: &Path) -> Option<Self> {
+ let base_dir = tsconfig_path.parent()?.to_path_buf();
+
+ let excludes = collect_excludes(tsconfig_path);
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tsconfig.rs (1)
131-140: Note: npm package extends are intentionally not resolved.The function only handles relative paths (
.or/prefixes). Extends like"extends": "@nx/js/tsconfig-lib"are skipped, which means exclude patterns won't be inherited from npm packages. This is a reasonable simplification given the feature's scope of filtering local project files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tsconfig.rs` around lines 131 - 140, The resolve_extends function currently only resolves relative/absolute specifiers and returns None for package-style extends (e.g., "@nx/js/tsconfig-lib"); update the code by adding a clear doc comment above fn resolve_extends explaining that npm package "extends" are intentionally not resolved (so exclude patterns from npm packages are not inherited), why this simplification was chosen, and optionally add a TODO comment or a unit test reference to track future support; leave the function logic as-is (checking starts_with '.' or '/') and ensure the comment references resolve_extends by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tsconfig.rs`:
- Around line 131-140: The resolve_extends function currently only resolves
relative/absolute specifiers and returns None for package-style extends (e.g.,
"@nx/js/tsconfig-lib"); update the code by adding a clear doc comment above fn
resolve_extends explaining that npm package "extends" are intentionally not
resolved (so exclude patterns from npm packages are not inherited), why this
simplification was chosen, and optionally add a TODO comment or a unit test
reference to track future support; leave the function logic as-is (checking
starts_with '.' or '/') and ensure the comment references resolve_extends by
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fca7d52e-b0d5-4e40-8bf7-5ad621cba60d
📒 Files selected for processing (6)
src/core.rssrc/lib.rssrc/main.rssrc/tsconfig.rssrc/utils.rstests/integration_test.rs
The 1.0.1 version bump added @front-ops/domino-* optionalDependencies to package.json but didn't regenerate yarn.lock, causing yarn install --frozen-lockfile to fail in CI. Also update CI.yml to run yarn install before committing version bumps so the lockfile stays in sync on future releases. Made-with: Cursor
Add doc comment to resolve_extends explaining the intentional omission of npm package-style extends resolution and cross-referencing the equivalent logic in resolve_options.rs. Made-with: Cursor
1 participant
Summary
excludepatterns (withextendschain support) to determine which files are excluded from the project's production source setProjectIndex::get_package_names_by_pathso that excluded files (e.g.*.stories.tsx,*.spec.ts) don't count toward marking a project as affectedProblem
Domino uses
WalkDirto collect all.ts/.tsx/.js/.jsxfiles regardless of tsconfig excludes. When a transitive type dependency chain reaches a project solely through an excluded file (like a.stories.tsx), domino incorrectly marks that project as affected. Traf avoids this by usingaddSourceFilesFromTsConfigwhich respects each project's tsconfiginclude/excludepatterns.Key design decision
The filtering happens at the project ownership level, not at file parsing. All files still get parsed and indexed for import graph accuracy. When a reference chain reaches a file and we ask "which project owns this?", excluded files are filtered out.
Interaction with PR #22
PR #22 adds include-pattern matching for direct test file changes. When rebasing #22 on top of this, note that
ProjectIndex::get_package_names_by_pathnow filters by tsconfig excludes. Include-pattern matching for direct changes should use an unfiltered lookup to avoid suppressing detection of changed test files that are excluded from tsconfig. Aget_package_names_by_path_unfilteredmethod may be needed.Test plan
cargo clippy --all-targets --all-features— cleancargo fmt --all -- --check— cleancargo test --lib— 169 tests pass (7 new tsconfig + utils tests)Closes #45
Made with Cursor
Summary by CodeRabbit
New Features
Tests
Chores