feat: add --ignore-tsconfig-excludes flag for test targets#58
feat: add --ignore-tsconfig-excludes flag for test targets#58
Conversation
When set, domino skips tsconfig exclude patterns (e.g., *.spec.ts, *.stories.tsx) during affected detection. This is useful for test targets where test-file imports should also be traced. Without this flag, projects whose only dependency on a changed library is through test files are not detected as affected — the test files are excluded by tsconfig.lib.json and domino never sees the imports. Usage: domino affected --ignore-tsconfig-excludes # for test targets domino affected # for build/lint targets
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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)
2975-3072: Consider adding a test forignore_tsconfig_excludes: true.All tests set
ignore_tsconfig_excludes: false, which verifies the default behavior is preserved. However, there's no test that exercises thetruecase to confirm that tsconfig excludes are actually bypassed when the flag is enabled.A test case like the following would validate the feature works as intended:
💡 Suggested test case
#[test] fn test_ignore_tsconfig_excludes_includes_spec_files() { // Similar setup to test_tsconfig_exclude_prevents_false_positive_via_stories // but with ignore_tsconfig_excludes: true // // When ignore_tsconfig_excludes is true, a change to shared-types // SHOULD mark ui-widgets as affected even if the only link is // through an excluded .stories.tsx file // ... setup code ... let config = TrueAffectedConfig { // ... ignore_tsconfig_excludes: true, // <-- key difference }; // Assert ui-widgets IS affected (opposite of the false case) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_test.rs` around lines 2975 - 3072, Add a new integration test that mirrors the existing tsconfig-exclude test but sets TrueAffectedConfig.ignore_tsconfig_excludes: true; use the TempNxRepo helper (TempNxRepo::new, change_and_commit, and get_affected) to create the workspace, apply the same file changes that previously produced a false-positive-exclusion via a .stories.tsx file, call get_affected (which builds TrueAffectedConfig with the changed flag) and assert the previously-unaffected project (e.g., "ui-widgets") now appears in the returned affected_projects; ensure the test name clearly indicates it verifies ignore_tsconfig_excludes=true behavior.
🤖 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 2975-3072: Add a new integration test that mirrors the existing
tsconfig-exclude test but sets TrueAffectedConfig.ignore_tsconfig_excludes:
true; use the TempNxRepo helper (TempNxRepo::new, change_and_commit, and
get_affected) to create the workspace, apply the same file changes that
previously produced a false-positive-exclusion via a .stories.tsx file, call
get_affected (which builds TrueAffectedConfig with the changed flag) and assert
the previously-unaffected project (e.g., "ui-widgets") now appears in the
returned affected_projects; ensure the test name clearly indicates it verifies
ignore_tsconfig_excludes=true behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b1adfc9-54e1-4d2f-9427-bd62687dbddd
📒 Files selected for processing (6)
src/cli.rssrc/core.rssrc/lib.rssrc/types.rssrc/utils.rstests/integration_test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration_test.rs (1)
2981-3039: Missing path canonicalization inTempNxRepo::root().Other test helpers in this file canonicalize the temp directory path to resolve symlinks (e.g.,
/var→/private/varon macOS). Without canonicalization, path comparisons in the core logic may fail if paths are compared using canonical forms.Proposed fix
struct TempNxRepo { dir: TempDir, + root: std::path::PathBuf, } impl TempNxRepo { fn new(nx_json: &str) -> Self { let dir = TempDir::new().unwrap(); - let root = dir.path(); + let root = dir + .path() + .canonicalize() + .expect("Failed to canonicalize temp dir"); // Init git - git_in(root, &["init", "-q"]); - git_in(root, &["config", "user.email", "test@example.com"]); + git_in(&root, &["init", "-q"]); + git_in(&root, &["config", "user.email", "test@example.com"]); // ... rest of initialization using &root ... - Self { dir } + Self { dir, root } } fn root(&self) -> &std::path::Path { - self.dir.path() + &self.root }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_test.rs` around lines 2981 - 3039, TempNxRepo::root currently returns the TempDir path without canonicalizing, which can cause mismatched comparisons on platforms with symlinked temp dirs; change the method signature of root(&self) to return a std::path::PathBuf (instead of &Path) and inside call self.dir.path().canonicalize().unwrap() (or propagate the error if preferred) so callers get the canonicalized absolute path; update any call sites expecting &Path to accept PathBuf or call .as_path() after the change.
🤖 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 3054-3071: The tests expect namedInputs support but the code never
parses or propagates them; add a named_inputs field to TrueAffectedConfig,
modify discover_projects to parse namedInputs from nx.json into the Project
metadata, pass named_inputs through find_affected (and into
find_affected_internal) and update the affected-calculation logic to honor
namedInputs when computing affected projects; alternatively if namedInputs are
intentionally unsupported, remove or adjust the tests that set namedInputs.
Ensure you reference and update TrueAffectedConfig, discover_projects,
find_affected, and find_affected_internal so the nx.json namedInputs are read,
stored, and applied during affected computation.
---
Nitpick comments:
In `@tests/integration_test.rs`:
- Around line 2981-3039: TempNxRepo::root currently returns the TempDir path
without canonicalizing, which can cause mismatched comparisons on platforms with
symlinked temp dirs; change the method signature of root(&self) to return a
std::path::PathBuf (instead of &Path) and inside call
self.dir.path().canonicalize().unwrap() (or propagate the error if preferred) so
callers get the canonicalized absolute path; update any call sites expecting
&Path to accept PathBuf or call .as_path() after the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa3333ae-dbc5-4786-9e31-a36dc567ee47
📒 Files selected for processing (3)
README.mddocs/index.htmltests/integration_test.rs
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/index.html
| fn get_affected(&self) -> Vec<String> { | ||
| let projects = domino::workspace::discover_projects(self.root()).unwrap(); | ||
| let config = TrueAffectedConfig { | ||
| cwd: self.root().to_path_buf(), | ||
| base: "main".to_string(), | ||
| root_ts_config: None, | ||
| projects, | ||
| include: vec![], | ||
| ignored_paths: vec![], | ||
| lockfile_strategy: LockfileStrategy::None, | ||
| ignore_tsconfig_excludes: false, | ||
| }; | ||
|
|
||
| let profiler = Arc::new(Profiler::new(false)); | ||
| find_affected(config, profiler) | ||
| .expect("find_affected failed") | ||
| .affected_projects | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify namedInputs infrastructure exists in the codebase
echo "=== Checking if TrueAffectedConfig has named_inputs field ==="
rg -n 'named_inputs' src/types.rs || echo "No named_inputs field in TrueAffectedConfig"
echo ""
echo "=== Checking if nx.json namedInputs parsing exists ==="
rg -n 'namedInputs|named_inputs' src/ --type rust -C 3
echo ""
echo "=== Checking discover_projects for namedInputs handling ==="
rg -n 'namedInputs|named_inputs' src/workspace/ --type rust -C 3Repository: frontops-dev/domino
Length of output: 285
🏁 Script executed:
# First, let's see the actual test code around lines 3054-3071
head -n 3100 tests/integration_test.rs | tail -n 50Repository: frontops-dev/domino
Length of output: 1405
🏁 Script executed:
# Search for namedInputs in nx.json test fixtures or test setup
rg -n 'namedInputs' tests/ -C 5Repository: frontops-dev/domino
Length of output: 5241
🏁 Script executed:
# Check the find_affected function signature and implementation
rg -n 'fn find_affected' src/ -A 10Repository: frontops-dev/domino
Length of output: 2538
🏁 Script executed:
# Look for any handling of inputs in the find_affected or related functions
rg -n 'inputs|namedInputs' src/affected.rs -C 3Repository: frontops-dev/domino
Length of output: 118
🏁 Script executed:
# Search for nx.json parsing and namedInputs handling
rg -n 'nx.json|namedInputs' src/ --type rust -B 2 -A 2Repository: frontops-dev/domino
Length of output: 534
🏁 Script executed:
# Check discover_projects implementation
rg -n 'fn discover_projects|pub fn discover_projects' src/ -A 20Repository: frontops-dev/domino
Length of output: 1647
🏁 Script executed:
# Search in workspace directory for nx.json reading
find src -name '*.rs' -type f | head -20Repository: frontops-dev/domino
Length of output: 416
🏁 Script executed:
# Check the find_affected_internal implementation in detail
sed -n '40,200p' src/core.rsRepository: frontops-dev/domino
Length of output: 5875
🏁 Script executed:
# Check nx::get_projects implementation
rg -n 'fn get_projects|pub fn get_projects' src/workspace/nx.rs -A 30Repository: frontops-dev/domino
Length of output: 1120
🏁 Script executed:
# Search for any namedInputs reference or parsing logic
rg -n 'named' src/ --type rust -iRepository: frontops-dev/domino
Length of output: 1944
🏁 Script executed:
# Check more of find_affected_internal to see if namedInputs are handled later
sed -n '200,400p' src/core.rsRepository: frontops-dev/domino
Length of output: 7579
🏁 Script executed:
# Check if the namedInputs tests are marked #[ignore]
sed -n '3074,3100p' tests/integration_test.rsRepository: frontops-dev/domino
Length of output: 815
🏁 Script executed:
# Do a comprehensive search for any namedInputs parsing or handling across entire codebase
rg 'namedInputs' . --type rustRepository: frontops-dev/domino
Length of output: 620
🏁 Script executed:
# Check the end of find_affected_internal
tail -n 100 src/core.rs | head -n 80Repository: frontops-dev/domino
Length of output: 2254
namedInputs infrastructure does not exist — tests will fail without implementation.
These tests configure namedInputs in nx.json but the codebase has no infrastructure to process them. Specifically:
TrueAffectedConfighas nonamed_inputsfieldfind_affected()andfind_affected_internal()never read or applynamedInputsdiscover_projects()does not parsenamedInputsfromnx.json
The tests create temporary Nx repos with namedInputs config, but get_affected() builds TrueAffectedConfig without any way to pass this information through. When these tests run, they will not test the intended behavior because namedInputs are never processed.
Either implement namedInputs support throughout the pipeline (reading from nx.json, storing in TrueAffectedConfig, and applying in find_affected_internal()), or remove these tests if namedInputs support is not planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration_test.rs` around lines 3054 - 3071, The tests expect
namedInputs support but the code never parses or propagates them; add a
named_inputs field to TrueAffectedConfig, modify discover_projects to parse
namedInputs from nx.json into the Project metadata, pass named_inputs through
find_affected (and into find_affected_internal) and update the
affected-calculation logic to honor namedInputs when computing affected
projects; alternatively if namedInputs are intentionally unsupported, remove or
adjust the tests that set namedInputs. Ensure you reference and update
TrueAffectedConfig, discover_projects, find_affected, and find_affected_internal
so the nx.json namedInputs are read, stored, and applied during affected
computation.
📦 Preview Release AvailableA preview release has been published for commit d890031. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-58-d890031/front-ops-domino-1.0.3.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-58-d890031/front-ops-domino-1.0.3.tgz affectedDetails |
1 participant
Summary
--ignore-tsconfig-excludesCLI flag andignoreTsconfigExcludesN-API option*.spec.ts,*.stories.tsx) during affected detectionMotivation
Domino uses
tsconfig.lib.jsonexclude patterns to skip test/story files when determining affected projects. This is correct for build targets — you don't want to rebuild a library just because a test file changed.However, for test targets, this causes false negatives: if a project's only dependency on a changed library is through test files (e.g.,
.spec.tsimportingApplicationIdsfrom a shared lib), domino misses the project entirely.Real example: zeroabstraction/apps#28357 — domino found 10 affected projects, Nx found 1050. The
island-feature-policy-common:testtarget was missed because it only imports frompolicy-application-parametersin test files.Usage
Test plan
false)Summary by CodeRabbit
New Features
Tests
Documentation