fix: detect affected projects for files inside root but outside sourceRoot#61
fix: detect affected projects for files inside root but outside sourceRoot#61
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProjectIndex now indexes both project Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils.rs (1)
93-123:⚠️ Potential issue | 🟠 MajorRoot fallback is skipped for nested projects once any ancestor
source_rootmatches.
matched_source_rootis global, so one prefix match suppresses all root fallback checks. That still misses nested layouts likeapps/parent+apps/parent/child: a file such asapps/parent/child/project.jsonwill match the parentsource_root, set the flag, and never be attributed to the child project even though it is insidechild.root. This breaks the stated “any file inside a project root marks that project affected” rule for overlapping roots.Suggested fix
pub fn get_package_names_by_path(&self, file_path: &Path) -> Vec<String> { let mut result = Vec::new(); - let mut matched_source_root = false; + let mut matched_projects = rustc_hash::FxHashSet::default(); // Primary: match against sourceRoot (with tsconfig exclude filtering) for (root, names) in &self.entries { if file_path.starts_with(root) { - matched_source_root = true; for name in names { + matched_projects.insert(name.clone()); if let Some(excl) = self.excludes.get(name) { if excl.is_excluded(file_path) { debug!( "File {:?} excluded by tsconfig for project '{}'", file_path, name @@ } } } - if !matched_source_root { - for (root, names) in &self.root_entries { - if file_path.starts_with(root) { - for name in names { - result.push(name.clone()); - } - } + // Fallback per project root, not globally. + for (root, names) in &self.root_entries { + if file_path.starts_with(root) { + for name in names { + if !matched_projects.contains(name) { + result.push(name.clone()); + } + } } } result }Please also add a regression test for a nested parent/child project layout so this case stays covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.rs` around lines 93 - 123, The current global matched_source_root bool suppresses all root fallback when any sourceRoot prefix matched, which prevents nested child project roots from being considered; fix this by collecting the actual matched source root paths (e.g., Vec<PathBuf> matched_source_roots) while iterating self.entries and only suppress a given fallback root from self.root_entries if one of the matched_source_roots legitimately should shadow that specific root (i.e., a matched source_root equals or is a child of that fallback root in a way that truly covers it), otherwise allow the fallback root to be attributed; update the loop over self.root_entries to consult matched_source_roots (instead of a single bool) and add a regression test that exercises a parent/child layout like apps/parent and apps/parent/child so project.json inside the child is attributed to the child project.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils.rs`:
- Around line 93-123: The current global matched_source_root bool suppresses all
root fallback when any sourceRoot prefix matched, which prevents nested child
project roots from being considered; fix this by collecting the actual matched
source root paths (e.g., Vec<PathBuf> matched_source_roots) while iterating
self.entries and only suppress a given fallback root from self.root_entries if
one of the matched_source_roots legitimately should shadow that specific root
(i.e., a matched source_root equals or is a child of that fallback root in a way
that truly covers it), otherwise allow the fallback root to be attributed;
update the loop over self.root_entries to consult matched_source_roots (instead
of a single bool) and add a regression test that exercises a parent/child layout
like apps/parent and apps/parent/child so project.json inside the child is
attributed to the child project.
📦 Preview Release AvailableA preview release has been published for commit 852901c. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-61-852901c/front-ops-domino-1.2.2.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-61-852901c/front-ops-domino-1.2.2.tgz affectedDetails |
|
Fixed @coderabbitai comment |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
+1 on this |
EladBezalel
left a comment
There was a problem hiding this comment.
Clean fix — the fallback design is correct and zero-cost for the common case. A few suggestions below, nothing blocking.
…eRoot ProjectIndex only matched files against sourceRoot, so config files like project.json, jest.config.js, and tsconfig.json that live in a project's root directory but outside its sourceRoot were never attributed to their owning project. Add root_entries fallback to ProjectIndex that activates only when no sourceRoot matched. tsconfig excludes are intentionally not applied to the root fallback since config files should always mark a project as affected. Closes #60 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace global matched_source_root boolean with per-project tracking so that nested child projects get attributed via root fallback even when a parent's sourceRoot is a prefix match. Add regression test for the nested parent/child layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Early-return before allocating hashset when root_entries is empty (common case for non-Nx workspaces where root == sourceRoot) - Use FxHashSet<&str> instead of FxHashSet<String> to avoid per-call string clones - Add test asserting root-level spec file bypasses tsconfig excludes (documents the design choice) - Add negative attribution test for nested layout: file inside parent only must not attribute to child Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c82ad61 to
852901c
Compare
Source-typed config files at project root (e.g. jest.config.js, webpack.config.ts when sourceRoot is "<proj>/src") are classified as source by extension but never walked by the analyzer, which only descends sourceRoot. Step 6a used to silently `continue` on such files, so the root fallback added in #61 never got a chance to run for them — changes to project-level config files went undetected. When a changed source file isn't in `analyzer.files`, fall back to `ProjectIndex::get_package_names_by_path` and mark the owning projects affected, mirroring how Step 6b handles assets. The new `root_entries` index from #61 now actually covers this case end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
ProjectIndexonly matched files againstsourceRoot, so config files (project.json,jest.config.js,tsconfig.json) inside a project'srootbut outside itssourceRootwere never attributed to their owning projectroot_entriesfallback toProjectIndexthat activates only when nosourceRootmatched — tsconfig excludes are intentionally not applied to the root fallback since config files should always mark a project as affectedsourceRoot(existing fast path unchanged); the fallback loop only runs for changed files outside allsourceRootpathsCloses #60
Test plan
test_project_index_root_fallback— verifies config files in root (outside sourceRoot) are detectedtest_project_index_root_fallback_with_tsconfig_excludes— verifies tsconfig excludes still apply to sourceRoot matches but don't block root fallback🤖 Generated with Claude Code
Summary by CodeRabbit