fix(core): apply root fallback for source files outside sourceRoot#65
fix(core): apply root fallback for source files outside sourceRoot#65
Conversation
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>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 46 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes add a fallback mechanism for source files that the semantic analyzer couldn't process. When a file is missing from the analyzer, the code now performs a root-based package ownership lookup via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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/integration_test.rs (1)
3310-3316: Tighten the regression by asserting unrelated projects stay unaffected.This test proves the fallback includes
lib-a; adding a negative assertion forlib-bwould also protect the “owning project only” behavior.Test assertion enhancement
assert!( affected.contains(&"lib-a".to_string()), "lib-a should be affected (jest.config.js at project root, outside sourceRoot). Got: {:?}", affected ); + assert!( + !affected.contains(&"lib-b".to_string()), + "lib-b should NOT be affected by lib-a's root-level config change. Got: {:?}", + affected + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_test.rs` around lines 3310 - 3316, The test currently asserts that "lib-a" is in the affected set via repo.get_affected() but lacks a negative assertion; add an assertion that affected does NOT contain "lib-b" to ensure unrelated projects remain unaffected — use the same pattern as the existing assert! (checking affected.contains(&"lib-b".to_string()) is false or using assert!(!affected.contains(...))) and include a helpful failure message like "lib-b should NOT be affected" to tighten the regression.
🤖 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/integration_test.rs`:
- Around line 3310-3316: The test currently asserts that "lib-a" is in the
affected set via repo.get_affected() but lacks a negative assertion; add an
assertion that affected does NOT contain "lib-b" to ensure unrelated projects
remain unaffected — use the same pattern as the existing assert! (checking
affected.contains(&"lib-b".to_string()) is false or using
assert!(!affected.contains(...))) and include a helpful failure message like
"lib-b should NOT be affected" to tighten the regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccd57f6a-6f12-4780-ae85-a24fd3c18698
📒 Files selected for processing (2)
src/core.rstests/integration_test.rs
|
This PR fixes source-typed config files outside sourceRoot — but it doesn't address the The issue: in Nx monorepos the root workspace project has This PR extends the root fallback to source files too (Step 6a), which would make the Suggested fix — in if project.root != project.source_root
&& !project.root.as_os_str().is_empty()
&& project.root != Path::new(".")
{This should be addressed before merging, otherwise the workspace regression from #61 gets amplified. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
EladBezalel
left a comment
EladBezalel
left a comment
There was a problem hiding this comment.
The fix for the intended case (jest.config.js at project root) is correct, but it amplifies the root: "." over-matching regression from #61 — which is currently breaking loco CI.
Before this PR, unanalyzed source files were skipped. Now they go through get_package_names_by_path, which returns the root workspace project for ANY file. Every PR that changes a source-typed file outside sourceRoot will mark workspace as affected.
Must fix before merge: guard against root: "." in ProjectIndex::new() (in src/utils.rs), or in the fallback branch itself. See inline comments.
📦 Preview Release AvailableA preview release has been published for commit b1cf76f. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-65-b1cf76f/front-ops-domino-1.2.4.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-65-b1cf76f/front-ops-domino-1.2.4.tgz affectedDetails |
…ct cause helper; tighten tests Address PR #65 review: - src/utils.rs: skip projects with `root == ""` or `root == "."` when building `root_entries`. The Nx loader produces `root = ""` via `strip_prefix(cwd)` for workspace-root projects; an empty path is a prefix of every path in the repo and would over-attribute every fallback lookup to the workspace project. (`.` is a no-op under `Path::starts_with` component semantics but guarded for loaders that preserve it literally / paths that arrive with a `./` prefix.) Adds two unit tests covering both cases. - src/core.rs: extract `record_direct_change_causes` helper; reuse it from both the new source root-fallback path (Step 6a) and the asset path (Step 6b) which had identical inlined blocks. - tests/integration_test.rs: tighten `test_source_file_outside_sourceroot_affects_owning_project` to an exact-match assertion (previous `.contains(...)` passed even if every project leaked through). Add `test_workspace_root_project_not_over_attributed` — a root-level project with a nested project next to it; changing the nested project's `jest.config.js` must affect only the nested project. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EladBezalel
left a comment
EladBezalel
left a comment
There was a problem hiding this comment.
All review comments addressed. The root: "." / "" guard in ProjectIndex::new() fixes the loco CI regression, the extracted helper deduplicates the report logic, and the test coverage is solid (unit + integration for both root forms + exact-match assertions).
LGTM.
2 participants
Summary
Follow-up to #61. That PR added a
root_entriesfallback onProjectIndexso files living at project root but outsidesourceRoot(e.g.jest.config.js,webpack.config.ts,project.json) could still be resolved to their owning project. The fallback was wired into the asset path (Step 6b) but not the source path (Step 6a) — meaning source-typed config files (.js/.ts/.jsx/.tsxat project root) were still silently skipped.Why the existing fix missed it
For a project where
sourceRoot ≠ root(e.g.sourceRoot = "apps/my-app/src",root = "apps/my-app"):utils::is_source_file("jest.config.js")returnstrue(has.jsextension) → goes intosource_files, notasset_filessourceRoot→jest.config.jsis never parsed → not inanalyzer.filesif !analyzer.files.contains_key(file_path) { continue; }— fires beforeget_package_names_by_pathis ever calledroot_entrieslookup table exists but nothing on the source path uses itObserved on a real Nx monorepo with 1685 projects:
Fix
In Step 6a, when a changed source file isn't in
analyzer.files, fall back toproject_index.get_package_names_by_path(file_path)and mark owning projects as affected — same treatment the asset path already uses. RecordAffectCause::DirectChangewhen reports are enabled. The existingroot_entriesindex now does its job end-to-end for source-typed files too.After the fix, same input:
Scope
analyzer.files— i.e. the exact path that previously didcontinue;. No hot-path changes.get_package_names_by_pathcall (linear scan over unique roots). In a 1685-project monorepo, that's microseconds — negligible next to Oxc parsing 36k files.Test plan
test_source_file_outside_sourceroot_affects_owning_project— fails without the fix (Got: []), passes with itcargo test --no-default-features --lib— all 189 unit tests passTempNxRepo-based integration tests pass, includingtest_named_inputs_negation_with_root_differs_from_source_root(guards theroot != sourceRootgeometry).tsconfig at project root (webpack.config.ts) → owning project via new fallback.jsonasset at project root (project.json) → unchanged asset pathjest.config.jsin a different project (apps/management) →management+ 5 dependents via implicit deps🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests