fix: report all projects when multiple share the same sourceRoot#39
Conversation
…ntops-dev#38) The file-to-project mapping used `.find()` which returns only the first match, causing non-deterministic results when multiple Nx projects share a sourceRoot (e.g., variant builds like MV2 vs MV3). Replace with `.filter()` to return all matching projects at every call site. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaced single-project ownership resolution with a ProjectIndex that returns all projects whose Changes
Sequence Diagram(s)sequenceDiagram
participant FileChange
participant ProjectIndex
participant Causality
rect rgba(255, 0, 0, 0.5)
Note over FileChange,Causality: Old Behavior (single-owner)
FileChange->>ProjectIndex: query (old single-match) for file
ProjectIndex-->>FileChange: returns one project or none
FileChange->>Causality: record affect for single project
end
rect rgba(0, 255, 0, 0.5)
Note over FileChange,Causality: New Behavior (multi-owner)
FileChange->>ProjectIndex: get_package_names_by_path(file_path)
ProjectIndex-->>FileChange: returns Vec of matching projects
loop for each project in Vec
FileChange->>Causality: record affect / record ImportedSymbol cause for project
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In `@tests/integration_test.rs`:
- Around line 2186-2191: The test is missing local git user config before
committing; call git_in to set user.email and user.name on the test repo (e.g.,
git_in(&root, &["config", "user.email", "test@example.com"]) and git_in(&root,
&["config", "user.name", "Test User"])) before the git_in(&root, &["add", "."])
/ git_in(&root, &["commit", "-m", "initial"]) lines so commits won't fail in CI
without global git config; insert these calls near the existing git_in(...)
sequence in tests/integration_test.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a67a1099-f354-4bdb-b6a2-27881071ef5a
📒 Files selected for processing (3)
src/core.rssrc/utils.rstests/integration_test.rs
![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 3 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="tests/integration_test.rs">
<violation number="1" location="tests/integration_test.rs:2188">
P2: Missing `git config user.email` and `git config user.name` before `git commit`. Other TempDir-based tests in this file set these after `git init`; without them, `git commit` will fail in CI environments that lack global git config.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Add user.email and user.name config after git init to match other TempDir-based tests and prevent failures in CI environments without global git config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📦 Preview Release AvailableA preview release has been published for commit 62fd6e9. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-39-62fd6e9/front-ops-domino-0.7.1.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-39-62fd6e9/front-ops-domino-0.7.1.tgz affectedDetails |
EladBezalel
left a comment
EladBezalel
left a comment
There was a problem hiding this comment.
Review: fix: report all projects when multiple share the same sourceRoot
LGTM — clean, correct fix with good test coverage.
What I checked
-
utils.rs: The.find()→.filter()change is the right fix. Signature change fromOption<String>toVec<String>is clean and the new unit test for shared sourceRoot covers the key scenario well. -
core.rs: All 5 call sites correctly updated to iterate over all matching projects. The change fromfor symbol in symbols→for symbol in &symbols+.clone()is necessary sincesymbolsmust survive across multiplepkgiterations — that's fine. -
process_changed_symbol: The nested iterationfor src_proj in &source_projectsinsidefor pkg in &ref_packagesproduces a cartesian product ofImportedSymbolcauses when both source and reference files are in multiple projects. This is technically correct (each combination is a valid causal chain), but worth noting it can produce verbose reports — e.g., if a source file is in [A, B] and a reference file is in [C, D], you get 4 cause entries. Acceptable for correctness, but if report noise becomes an issue down the road, could be worth deduplicating. -
Integration test: Solid — reproduces the exact issue #38 scenario with two variant-build projects sharing one source directory. Git user config properly set (fixed in the follow-up commit).
One minor suggestion
The get_package_names_by_path function now always scans all projects (no short-circuit). For most monorepos this is fine since project count is small, but if you ever want to optimize, an index from sourceRoot → Vec<project_name> built once at startup would make lookups O(1) instead of O(n_projects). Not needed now — just a thought for the future.
Verdict
Correct fix, good tests, clean implementation. Ship it. 🚢
Replace per-call O(n_projects) scanning in get_package_names_by_path with a ProjectIndex that groups project names by sourceRoot at startup, reducing repeated allocations and comparisons during analysis. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/core.rs (2)
96-127: Resolve changed-line symbols once per line in report mode.
analyzer.find_node_at_line(...)now runs once per changed line per owning package. For shared roots, that repeats the same lookup for identical input. Cache the symbols per line before thepkgloop and reuse them when writing direct-change causes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core.rs` around lines 96 - 127, The loop calls analyzer.find_node_at_line(file_path, line, 0) repeatedly for each owning package; to fix, when generate_report is true compute and cache the symbols per changed line once (e.g., build a HashMap<usize, Vec<Symbol>> from changed_file.changed_lines using analyzer.find_node_at_line) before iterating owning_packages, then inside the pkg loop reuse that cached_symbols[line] to push AffectCause::DirectChange into project_causes; reference changed_file.changed_lines, analyzer.find_node_at_line, owning_packages/project_index.get_package_names_by_path, and project_causes/affected_packages for locating where to apply the change.
464-466: Skip source-owner lookup when no report is being built.
source_projectsis only consumed in theproject_causesbranch. In the normalfind_affectedpath this still does a prefix scan and clones names on every recursive symbol walk.♻️ Suggested guard
- let source_projects = project_index.get_package_names_by_path(file_path); + let source_projects = if state.project_causes.is_some() { + project_index.get_package_names_by_path(file_path) + } else { + Vec::new() + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core.rs` around lines 464 - 466, The code eagerly computes source_projects via project_index.get_package_names_by_path(file_path) even when no report is requested; move this call behind the branch that handles project_causes so it's only executed when project_causes is true (i.e., in the project_causes code path), and keep the find_affected path unchanged to avoid the prefix scan and cloning on every recursive symbol walk; update references to source_projects so they are only used after it's initialized within the project_causes branch.src/utils.rs (1)
170-209: Use generic fixtures in this shared-root test.These names and paths read like repo-specific examples. Please swap them for neutral placeholders so the test stays compliant with the repo’s example-redaction rule.
As per coding guidelines, "Remove references to external repositories, obfuscate repository names, project names, and file paths from examples using generic names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.rs` around lines 170 - 209, Update the test_project_index_shared_source_root fixture to replace repo-specific names and paths with neutral placeholders: change project names like "app-desktop", "app-desktop-mv3", and "other-project" to generic names such as "project-a", "project-b", "project-c", and replace source_root paths like "projects/app-desktop/src" and "projects/other/src" with generic paths such as "packages/project-a/src", "packages/project-b/src", and "packages/project-c/src"; ensure assertions still use the new names and that calls to ProjectIndex::new and get_package_names_by_path continue to reference the updated placeholder paths so the test logic (shared vs unique source_root matches and empty result) remains identical.
🤖 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/core.rs`:
- Around line 96-127: The loop calls analyzer.find_node_at_line(file_path, line,
0) repeatedly for each owning package; to fix, when generate_report is true
compute and cache the symbols per changed line once (e.g., build a
HashMap<usize, Vec<Symbol>> from changed_file.changed_lines using
analyzer.find_node_at_line) before iterating owning_packages, then inside the
pkg loop reuse that cached_symbols[line] to push AffectCause::DirectChange into
project_causes; reference changed_file.changed_lines,
analyzer.find_node_at_line,
owning_packages/project_index.get_package_names_by_path, and
project_causes/affected_packages for locating where to apply the change.
- Around line 464-466: The code eagerly computes source_projects via
project_index.get_package_names_by_path(file_path) even when no report is
requested; move this call behind the branch that handles project_causes so it's
only executed when project_causes is true (i.e., in the project_causes code
path), and keep the find_affected path unchanged to avoid the prefix scan and
cloning on every recursive symbol walk; update references to source_projects so
they are only used after it's initialized within the project_causes branch.
In `@src/utils.rs`:
- Around line 170-209: Update the test_project_index_shared_source_root fixture
to replace repo-specific names and paths with neutral placeholders: change
project names like "app-desktop", "app-desktop-mv3", and "other-project" to
generic names such as "project-a", "project-b", "project-c", and replace
source_root paths like "projects/app-desktop/src" and "projects/other/src" with
generic paths such as "packages/project-a/src", "packages/project-b/src", and
"packages/project-c/src"; ensure assertions still use the new names and that
calls to ProjectIndex::new and get_package_names_by_path continue to reference
the updated placeholder paths so the test logic (shared vs unique source_root
matches and empty result) remains identical.
2 participants
Summary
Fixes #38
get_package_name_by_path(.find()→ first match only) withget_package_names_by_path(.filter()→ all matches) so that every project whosesourceRootcontains a changed file is reported as affectedcore.rsto iterate over all matching projects for source files, asset files, asset references, and cross-file symbol referencesTest plan
cargo test --lib— 82 unit tests pass including newtest_get_package_names_by_path_shared_source_rootcargo test --test integration_test --no-default-features test_shared_source_root— new integration test passes, bothapp-desktopandapp-desktop-mv3are correctly reportedcargo clippy --all-targets --no-default-features— no warningscargo fmt --check— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests