From 48939f64558ea4ae42e2c0bfbaf224f43bb156db Mon Sep 17 00:00:00 2001 From: Nir Hadassi Date: Thu, 23 Apr 2026 12:54:06 +0300 Subject: [PATCH 1/2] fix: skip tsconfig excludes for direct changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A directly changed file always belongs to its project — tsconfig excludes define what TypeScript compiles, not project ownership. Previously, changing a spec or stories file produced "no affected projects" because tsconfig.app.json excludes *.spec.ts. Added `get_owning_packages_by_path` — a path-prefix lookup that skips tsconfig exclude filtering. Used for all direct-change ownership in Steps 6a and 6b. The existing filtered `get_package_names_by_path` is preserved for reference traversal where cascade through excluded files may be undesirable. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core.rs | 12 ++++--- src/utils.rs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/core.rs b/src/core.rs index b395c0c..af8b931 100644 --- a/src/core.rs +++ b/src/core.rs @@ -179,7 +179,7 @@ fn find_affected_internal( "Source file not in analyzer.files, using root fallback: {:?}", file_path ); - let owning_packages = project_index.get_package_names_by_path(file_path); + let owning_packages = project_index.get_owning_packages_by_path(file_path); for pkg in &owning_packages { debug!( "File {:?} belongs to package '{}' (via root fallback)", @@ -218,8 +218,10 @@ fn find_affected_internal( ) .collect(); - // Add all packages that own this file (multiple projects can share the same sourceRoot) - let owning_packages = project_index.get_package_names_by_path(file_path); + // Add all packages that own this file (multiple projects can share the same sourceRoot). + // Uses the unfiltered lookup — a directly changed file always belongs to its project + // regardless of tsconfig excludes (spec files, stories, config files all count). + let owning_packages = project_index.get_owning_packages_by_path(file_path); for pkg in &owning_packages { debug!("File {:?} belongs to package '{}'", file_path, pkg); affected_packages.insert(pkg.clone()); @@ -316,8 +318,8 @@ fn find_affected_internal( for asset_file in &asset_files { let asset_path = &asset_file.file_path; - // Mark all owning projects as affected (multiple projects can share the same sourceRoot) - let owning_packages = project_index.get_package_names_by_path(asset_path); + // Mark all owning projects as affected — uses unfiltered lookup (direct change). + let owning_packages = project_index.get_owning_packages_by_path(asset_path); for pkg in &owning_packages { debug!("Asset {:?} belongs to package '{}'", asset_path, pkg); affected_packages.insert(pkg.clone()); diff --git a/src/utils.rs b/src/utils.rs index 69429f3..89b283c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -161,6 +161,45 @@ impl ProjectIndex { } result } + + /// Like `get_package_names_by_path` but skips tsconfig exclude filtering. + /// + /// Use for **direct changes**: a file that was modified in the diff always + /// belongs to its project regardless of whether tsconfig compiles it (spec + /// files, stories, config files all count). The filtered variant should + /// only be used for reference traversal where cascade through excluded + /// files is undesirable. + pub fn get_owning_packages_by_path(&self, file_path: &Path) -> Vec { + let mut result = Vec::new(); + if self.root_entries.is_empty() { + for (root, names) in &self.entries { + if file_path.starts_with(root) { + result.extend(names.iter().cloned()); + } + } + return result; + } + + let mut seen_via_source_root: FxHashSet<&str> = FxHashSet::default(); + for (root, names) in &self.entries { + if file_path.starts_with(root) { + for name in names { + seen_via_source_root.insert(name.as_str()); + result.push(name.clone()); + } + } + } + for (root, names) in &self.root_entries { + if file_path.starts_with(root) { + for name in names { + if !seen_via_source_root.contains(name.as_str()) { + result.push(name.clone()); + } + } + } + } + result + } } /// Convert line number to byte offset in source text @@ -370,6 +409,56 @@ mod tests { ); } + #[test] + fn test_get_owning_packages_skips_tsconfig_excludes() { + let tmp = tempfile::TempDir::new().unwrap(); + let cwd = tmp.path(); + + let lib_dir = cwd.join("libs/ui-widgets"); + std::fs::create_dir_all(&lib_dir).unwrap(); + std::fs::write( + lib_dir.join("tsconfig.lib.json"), + r#"{ "exclude": ["**/*.spec.ts", "**/*.stories.tsx"] }"#, + ) + .unwrap(); + + let projects = vec![Project { + name: "ui-widgets".to_string(), + root: "libs/ui-widgets".into(), + source_root: "libs/ui-widgets/src".into(), + ts_config: Some(lib_dir.join("tsconfig.lib.json")), + implicit_dependencies: vec![], + targets: vec![], + }]; + + let index = ProjectIndex::new(&projects, cwd); + + // get_package_names_by_path excludes spec/stories (for reference traversal) + assert!( + index + .get_package_names_by_path(Path::new("libs/ui-widgets/src/utils.spec.ts")) + .is_empty(), + "filtered method should exclude spec files" + ); + + // get_owning_packages_by_path includes them (for direct changes) + assert_eq!( + index.get_owning_packages_by_path(Path::new("libs/ui-widgets/src/utils.spec.ts")), + vec!["ui-widgets"], + "direct-change method should include spec files" + ); + assert_eq!( + index.get_owning_packages_by_path(Path::new("libs/ui-widgets/src/Grid.stories.tsx")), + vec!["ui-widgets"], + "direct-change method should include stories files" + ); + assert_eq!( + index.get_owning_packages_by_path(Path::new("libs/ui-widgets/src/index.ts")), + vec!["ui-widgets"], + "normal files work with both methods" + ); + } + #[test] fn test_project_index_root_fallback() { let tmp = tempfile::TempDir::new().unwrap(); From 7c5e1c74b367eee8870ce16431df2f98068d3888 Mon Sep 17 00:00:00 2001 From: Nir Hadassi Date: Thu, 23 Apr 2026 14:28:02 +0300 Subject: [PATCH 2/2] refactor: extract resolve_packages helper; add fast-path + integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #66 review: - Extract private `resolve_packages(file_path, skip_excludes)` shared by both public methods — prevents silent divergence if matching logic evolves. - Add `test_get_owning_packages_fast_path_no_root_entries`: exercises the `root_entries.is_empty()` branch (root == sourceRoot + tsconfig excludes). - Add `test_spec_file_change_affects_owning_project`: full pipeline integration test — creates a project with tsconfig excluding *.spec.ts, commits a spec change, asserts `find_affected` returns the owning project. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/utils.rs | 139 ++++++++++++++++++++++---------------- tests/integration_test.rs | 75 ++++++++++++++++++++ 2 files changed, 154 insertions(+), 60 deletions(-) diff --git a/src/utils.rs b/src/utils.rs index 89b283c..f755ebd 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -99,6 +99,21 @@ impl ProjectIndex { /// back to root entries for files that live inside a project's root but outside its /// sourceRoot (e.g. config files like project.json, jest.config.js). pub fn get_package_names_by_path(&self, file_path: &Path) -> Vec { + self.resolve_packages(file_path, false) + } + + /// Like `get_package_names_by_path` but skips tsconfig exclude filtering. + /// + /// Use for **direct changes**: a file that was modified in the diff always + /// belongs to its project regardless of whether tsconfig compiles it (spec + /// files, stories, config files all count). The filtered variant should + /// only be used for reference traversal where cascade through excluded + /// files is undesirable. + pub fn get_owning_packages_by_path(&self, file_path: &Path) -> Vec { + self.resolve_packages(file_path, true) + } + + fn resolve_packages(&self, file_path: &Path, skip_excludes: bool) -> Vec { let mut result = Vec::new(); // Fast path: no root entries means every project has root == sourceRoot, // so there's no fallback to run — skip the hashset allocation entirely. @@ -107,13 +122,15 @@ impl ProjectIndex { for (root, names) in &self.entries { if file_path.starts_with(root) { for name in names { - if let Some(excl) = self.excludes.get(name) { - if excl.is_excluded(file_path) { - debug!( - "File {:?} excluded by tsconfig for project '{}'", - file_path, name - ); - continue; + if !skip_excludes { + if let Some(excl) = self.excludes.get(name) { + if excl.is_excluded(file_path) { + debug!( + "File {:?} excluded by tsconfig for project '{}'", + file_path, name + ); + continue; + } } } result.push(name.clone()); @@ -124,21 +141,21 @@ impl ProjectIndex { } // Track which projects were already considered via sourceRoot (even if excluded - // by tsconfig) so that the root fallback doesn't re-add them. Borrow &str from - // self.entries — no allocation needed. + // by tsconfig) so that the root fallback doesn't re-add them. let mut seen_via_source_root: FxHashSet<&str> = FxHashSet::default(); - // Primary: match against sourceRoot (with tsconfig exclude filtering) for (root, names) in &self.entries { if file_path.starts_with(root) { for name in names { seen_via_source_root.insert(name.as_str()); - if let Some(excl) = self.excludes.get(name) { - if excl.is_excluded(file_path) { - debug!( - "File {:?} excluded by tsconfig for project '{}'", - file_path, name - ); - continue; + if !skip_excludes { + if let Some(excl) = self.excludes.get(name) { + if excl.is_excluded(file_path) { + debug!( + "File {:?} excluded by tsconfig for project '{}'", + file_path, name + ); + continue; + } } } result.push(name.clone()); @@ -146,49 +163,8 @@ impl ProjectIndex { } } // Fallback: match against project root for projects not already matched via - // sourceRoot. This handles files inside a project's root but outside its - // sourceRoot (e.g. config files). Also handles nested projects where the - // parent's sourceRoot is a prefix but the child was never checked. - // tsconfig excludes are not applied here — config files should always count. - for (root, names) in &self.root_entries { - if file_path.starts_with(root) { - for name in names { - if !seen_via_source_root.contains(name.as_str()) { - result.push(name.clone()); - } - } - } - } - result - } - - /// Like `get_package_names_by_path` but skips tsconfig exclude filtering. - /// - /// Use for **direct changes**: a file that was modified in the diff always - /// belongs to its project regardless of whether tsconfig compiles it (spec - /// files, stories, config files all count). The filtered variant should - /// only be used for reference traversal where cascade through excluded - /// files is undesirable. - pub fn get_owning_packages_by_path(&self, file_path: &Path) -> Vec { - let mut result = Vec::new(); - if self.root_entries.is_empty() { - for (root, names) in &self.entries { - if file_path.starts_with(root) { - result.extend(names.iter().cloned()); - } - } - return result; - } - - let mut seen_via_source_root: FxHashSet<&str> = FxHashSet::default(); - for (root, names) in &self.entries { - if file_path.starts_with(root) { - for name in names { - seen_via_source_root.insert(name.as_str()); - result.push(name.clone()); - } - } - } + // sourceRoot. tsconfig excludes are not applied here — config files should + // always count. for (root, names) in &self.root_entries { if file_path.starts_with(root) { for name in names { @@ -459,6 +435,49 @@ mod tests { ); } + #[test] + fn test_get_owning_packages_fast_path_no_root_entries() { + // When root == sourceRoot, root_entries is empty and resolve_packages + // takes the fast path (no hashset allocation). This test exercises that + // branch with tsconfig excludes active. + let tmp = tempfile::TempDir::new().unwrap(); + let cwd = tmp.path(); + + let lib_dir = cwd.join("libs/simple"); + std::fs::create_dir_all(&lib_dir).unwrap(); + std::fs::write( + lib_dir.join("tsconfig.lib.json"), + r#"{ "exclude": ["**/*.spec.ts"] }"#, + ) + .unwrap(); + + let projects = vec![Project { + name: "simple".to_string(), + root: "libs/simple".into(), + source_root: "libs/simple".into(), + ts_config: Some(lib_dir.join("tsconfig.lib.json")), + implicit_dependencies: vec![], + targets: vec![], + }]; + + let index = ProjectIndex::new(&projects, cwd); + + // Filtered: spec excluded + assert!( + index + .get_package_names_by_path(Path::new("libs/simple/utils.spec.ts")) + .is_empty(), + "fast path: filtered method should exclude spec files" + ); + + // Unfiltered: spec included + assert_eq!( + index.get_owning_packages_by_path(Path::new("libs/simple/utils.spec.ts")), + vec!["simple"], + "fast path: direct-change method should include spec files" + ); + } + #[test] fn test_project_index_root_fallback() { let tmp = tempfile::TempDir::new().unwrap(); diff --git a/tests/integration_test.rs b/tests/integration_test.rs index d6fd74e..1c41989 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -3397,6 +3397,81 @@ fn test_workspace_root_project_not_over_attributed() { ); } +#[test] +fn test_spec_file_change_affects_owning_project() { + // lib-a has sourceRoot = "libs/lib-a/src" and its tsconfig.lib.json + // excludes *.spec.ts. A direct change to a spec file must still mark + // lib-a as affected — tsconfig excludes define compilation scope, not + // project ownership. + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + + git_in(root, &["init", "-q"]); + git_in(root, &["config", "user.email", "test@example.com"]); + git_in(root, &["config", "user.name", "Test"]); + git_in(root, &["branch", "-M", "main"]); + + fs::write(root.join("nx.json"), r#"{}"#).unwrap(); + + fs::create_dir_all(root.join("libs/lib-a/src")).unwrap(); + fs::write( + root.join("libs/lib-a/project.json"), + r#"{ "name": "lib-a", "sourceRoot": "libs/lib-a/src" }"#, + ) + .unwrap(); + fs::write( + root.join("libs/lib-a/tsconfig.lib.json"), + r#"{ "exclude": ["**/*.spec.ts", "**/*.stories.tsx"] }"#, + ) + .unwrap(); + fs::write( + root.join("libs/lib-a/src/index.ts"), + "export const a = 1;\n", + ) + .unwrap(); + fs::write( + root.join("libs/lib-a/src/utils.spec.ts"), + "import { a } from './index';\n", + ) + .unwrap(); + + git_in(root, &["add", "."]); + git_in(root, &["commit", "-q", "-m", "init"]); + git_in(root, &["checkout", "-q", "-b", "test-branch"]); + + // Change the spec file + fs::write( + root.join("libs/lib-a/src/utils.spec.ts"), + "import { a } from './index';\n// changed\n", + ) + .unwrap(); + git_in(root, &["add", "."]); + git_in(root, &["commit", "-q", "-m", "change spec"]); + + let projects = domino::workspace::discover_projects(root).unwrap(); + let config = TrueAffectedConfig { + cwd: root.to_path_buf(), + base: "main".to_string(), + head: None, + root_ts_config: None, + projects, + include: vec![], + ignored_paths: vec![], + lockfile_strategy: LockfileStrategy::None, + }; + + let profiler = Arc::new(Profiler::new(false)); + let affected = find_affected(config, profiler) + .expect("find_affected failed") + .affected_projects; + + assert_eq!( + affected, + vec!["lib-a".to_string()], + "lib-a should be affected even though the changed spec file is tsconfig-excluded" + ); +} + #[test] fn test_head_flag_commit_to_commit_diff() { let branch = TestBranch::new("test-head-flag");