From b69dea4823977ca8f45a9cd87a08898dc5ec2ad1 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 13 Apr 2026 13:04:25 +0200 Subject: [PATCH] refactor(pi-natives-cache/search): restructured node-modules scan cache - Added skip_node_modules to cache keys and scan calls, preventing stale cache reuse across policies. - Updated build_walker to filter .git and optionally prune node_modules during traversal. - Threaded skip-node-modules decisions through ast, glob, grep, and fff scan paths for consistency. - Adjusted get_or_scan and force_rescan to honor the same skip-node_modules policy. - Added tests verifying node_modules and .git behavior in walker, collect_entries, and force_rescan. --- crates/pi-natives/src/ast.rs | 5 +- crates/pi-natives/src/fff.rs | 6 +- crates/pi-natives/src/fs_cache.rs | 136 ++++++++++++++++++++++++++---- crates/pi-natives/src/glob.rs | 6 +- crates/pi-natives/src/grep.rs | 6 +- 5 files changed, 132 insertions(+), 27 deletions(-) diff --git a/crates/pi-natives/src/ast.rs b/crates/pi-natives/src/ast.rs index d56d9bc9e..8ff731eab 100644 --- a/crates/pi-natives/src/ast.rs +++ b/crates/pi-natives/src/ast.rs @@ -369,7 +369,8 @@ fn collect_candidates( let glob_set = glob_util::try_compile_glob(glob, false)?; let mentions_node_modules = glob.is_some_and(|value| value.contains("node_modules")); - let scan = fs_cache::get_or_scan(&search_path, true, true, ct)?; + let skip_node_modules = !mentions_node_modules; + let scan = fs_cache::get_or_scan(&search_path, true, true, skip_node_modules, ct)?; let mut files = collect_from_entries( &search_path, &scan.entries, @@ -379,7 +380,7 @@ fn collect_candidates( )?; if files.is_empty() && scan.cache_age_ms >= fs_cache::empty_recheck_ms() { - let fresh = fs_cache::force_rescan(&search_path, true, true, true, ct)?; + let fresh = fs_cache::force_rescan(&search_path, true, true, skip_node_modules, true, ct)?; files = collect_from_entries(&search_path, &fresh, glob_set.as_ref(), mentions_node_modules, ct)?; } diff --git a/crates/pi-natives/src/fff.rs b/crates/pi-natives/src/fff.rs index d584609f8..75ac44ee4 100644 --- a/crates/pi-natives/src/fff.rs +++ b/crates/pi-natives/src/fff.rs @@ -280,7 +280,7 @@ fn fuzzy_find_sync( let query = config.query.trim().to_string(); let use_cache = config.cache.unwrap_or(false); if use_cache { - let scan = fs_cache::get_or_scan(&root, include_hidden, respect_gitignore, &ct)?; + let scan = fs_cache::get_or_scan(&root, include_hidden, respect_gitignore, true, &ct)?; let mut results = run_fff_search( &root, &scan.entries, @@ -295,7 +295,7 @@ fn fuzzy_find_sync( && !query.is_empty() && scan.cache_age_ms >= fs_cache::empty_recheck_ms() { - let fresh = fs_cache::force_rescan(&root, include_hidden, respect_gitignore, true, &ct)?; + let fresh = fs_cache::force_rescan(&root, include_hidden, respect_gitignore, true, true, &ct)?; results = run_fff_search( &root, &fresh, @@ -310,7 +310,7 @@ fn fuzzy_find_sync( return Ok(results); } - let fresh = fs_cache::force_rescan(&root, include_hidden, respect_gitignore, false, &ct)?; + let fresh = fs_cache::force_rescan(&root, include_hidden, respect_gitignore, true, false, &ct)?; run_fff_search(&root, &fresh, &query, max_results, include_hidden, respect_gitignore, db, &ct) } diff --git a/crates/pi-natives/src/fs_cache.rs b/crates/pi-natives/src/fs_cache.rs index 5ba7393ad..10d0b6a3f 100644 --- a/crates/pi-natives/src/fs_cache.rs +++ b/crates/pi-natives/src/fs_cache.rs @@ -84,9 +84,10 @@ pub fn max_cache_entries() -> usize { #[derive(Clone, Debug, Eq, Hash, PartialEq)] struct CacheKey { - root: PathBuf, - include_hidden: bool, - use_gitignore: bool, + root: PathBuf, + include_hidden: bool, + use_gitignore: bool, + skip_node_modules: bool, } #[derive(Clone)] @@ -199,12 +200,34 @@ pub fn classify_file_type(path: &Path) -> Option<(FileType, Option)> { /// Builds a deterministic filesystem walker configured for visibility and /// ignore rules. -pub fn build_walker(root: &Path, include_hidden: bool, use_gitignore: bool) -> WalkBuilder { +/// +/// When `skip_node_modules` is true, `node_modules` directories are pruned at +/// traversal time (not just filtered post-scan). `.git` is always skipped. +pub fn build_walker( + root: &Path, + include_hidden: bool, + use_gitignore: bool, + skip_node_modules: bool, +) -> WalkBuilder { let mut builder = WalkBuilder::new(root); builder .hidden(!include_hidden) .follow_links(false) - .sort_by_file_path(|a, b| a.cmp(b)); + .sort_by_file_path(|a, b| a.cmp(b)) + // filter_entry controls whether to yield an entry AND whether to descend + // into a directory. Returning false for a directory skips the entire subtree. + .filter_entry(move |entry| { + let name = entry.file_name().to_str().unwrap_or_default(); + // Always skip .git + if name == ".git" { + return false; + } + // Skip node_modules when skip_node_modules is true + if skip_node_modules && name == "node_modules" { + return false; + } + true + }); if use_gitignore { // Honor repository and global ignore files for repo-like behavior. @@ -229,16 +252,14 @@ pub fn build_walker(root: &Path, include_hidden: bool, use_gitignore: bool) -> W /// Scans filesystem entries and records normalized relative paths with file /// metadata. -/// -/// Always stores `node_modules` entries; caller-side filtering handles -/// exclusion. fn collect_entries( root: &Path, include_hidden: bool, use_gitignore: bool, + skip_node_modules: bool, ct: &task::CancelToken, ) -> Result> { - let builder = build_walker(root, include_hidden, use_gitignore); + let builder = build_walker(root, include_hidden, use_gitignore, skip_node_modules); let mut entries = Vec::new(); for entry in builder.build() { @@ -246,10 +267,6 @@ fn collect_entries( let Ok(entry) = entry else { continue }; let path = entry.path(); - if should_skip_path(path, true) { - // The cache always stores node_modules; caller-side filtering is applied later. - continue; - } let relative = normalize_relative_path(root, path); if relative.is_empty() { @@ -281,16 +298,17 @@ pub fn get_or_scan( root: &Path, include_hidden: bool, use_gitignore: bool, + skip_node_modules: bool, ct: &task::CancelToken, ) -> Result { let ttl = *CACHE_TTL_MS; if ttl == 0 { // Caching disabled – always scan fresh. - let entries = collect_entries(root, include_hidden, use_gitignore, ct)?; + let entries = collect_entries(root, include_hidden, use_gitignore, skip_node_modules, ct)?; return Ok(ScanResult { entries, cache_age_ms: 0 }); } - let key = CacheKey { root: root.to_path_buf(), include_hidden, use_gitignore }; + let key = CacheKey { root: root.to_path_buf(), include_hidden, use_gitignore, skip_node_modules }; let now = Instant::now(); if let Some(entry) = FS_CACHE.get(&key) { @@ -305,7 +323,7 @@ pub fn get_or_scan( FS_CACHE.remove(&key); } - let entries = collect_entries(root, include_hidden, use_gitignore, ct)?; + let entries = collect_entries(root, include_hidden, use_gitignore, skip_node_modules, ct)?; FS_CACHE.insert(key, CacheEntry { created_at: now, entries: entries.clone() }); evict_oldest(); Ok(ScanResult { entries, cache_age_ms: 0 }) @@ -320,13 +338,14 @@ pub fn force_rescan( root: &Path, include_hidden: bool, use_gitignore: bool, + skip_node_modules: bool, store: bool, ct: &task::CancelToken, ) -> Result> { - let key = CacheKey { root: root.to_path_buf(), include_hidden, use_gitignore }; + let key = CacheKey { root: root.to_path_buf(), include_hidden, use_gitignore, skip_node_modules }; FS_CACHE.remove(&key); - let entries = collect_entries(root, include_hidden, use_gitignore, ct)?; + let entries = collect_entries(root, include_hidden, use_gitignore, skip_node_modules, ct)?; if store { let now = Instant::now(); FS_CACHE.insert(key, CacheEntry { created_at: now, entries: entries.clone() }); @@ -449,4 +468,85 @@ mod tests { assert_eq!(classify_file_type(&fifo), None); } + + #[test] + fn build_walker_skips_git_and_node_modules() { + let root = TempDirGuard::new(); + fs::create_dir_all(root.path().join(".git/objects")).unwrap(); + fs::write(root.path().join(".git/objects/a.txt"), "git obj").unwrap(); + fs::create_dir_all(root.path().join("node_modules/pkg")).unwrap(); + fs::write(root.path().join("node_modules/pkg/index.js"), "nm").unwrap(); + fs::write(root.path().join("real.txt"), "ok").unwrap(); + + // skip_node_modules: true -> should only see real.txt + let walker = super::build_walker(root.path(), true, false, true); + let paths: Vec = walker + .build() + .filter_map(|e| e.ok()) + .filter(|e| e.path() != root.path()) + .map(|e| e.path().strip_prefix(root.path()).unwrap().to_string_lossy().into_owned()) + .collect(); + assert!( + !paths.iter().any(|p| p.contains("node_modules") || p.contains(".git")), + "expected no .git or node_modules entries, got: {paths:?}" + ); + assert!(paths.iter().any(|p| p == "real.txt"), "expected real.txt, got: {paths:?}"); + + // skip_node_modules: false -> should see node_modules but not .git + let walker = super::build_walker(root.path(), true, false, false); + let paths: Vec = walker + .build() + .filter_map(|e| e.ok()) + .filter(|e| e.path() != root.path()) + .map(|e| e.path().strip_prefix(root.path()).unwrap().to_string_lossy().into_owned()) + .collect(); + assert!( + !paths.iter().any(|p| p.contains(".git")), + "expected no .git entries, got: {paths:?}" + ); + assert!( + paths.iter().any(|p| p.contains("node_modules")), + "expected node_modules entries, got: {paths:?}" + ); + } + + #[test] + fn collect_entries_skips_node_modules() { + let root = TempDirGuard::new(); + fs::create_dir_all(root.path().join("node_modules/pkg")).unwrap(); + fs::write(root.path().join("node_modules/pkg/index.js"), "nm").unwrap(); + fs::write(root.path().join("real.txt"), "ok").unwrap(); + + let ct = crate::task::CancelToken::default(); + let entries = super::collect_entries(root.path(), true, false, true, &ct).unwrap(); + let paths: Vec<&str> = entries.iter().map(|e| e.path.as_str()).collect(); + assert!( + !paths.iter().any(|p| p.contains("node_modules")), + "expected no node_modules entries, got: {paths:?}" + ); + assert!(paths.iter().any(|p| p == &"real.txt"), "expected real.txt, got: {paths:?}"); + } + + #[test] + fn force_rescan_respects_skip_node_modules() { + let root = TempDirGuard::new(); + // Create a nested node_modules with many files + for i in 0..100 { + let pkg_dir = root.path().join(format!("node_modules/pkg-{i}")); + fs::create_dir_all(&pkg_dir).unwrap(); + fs::write(pkg_dir.join("index.js"), "x").unwrap(); + } + fs::write(root.path().join("app.js"), "ok").unwrap(); + + let ct = crate::task::CancelToken::default(); + + // With skip: should only get app.js + let entries = super::force_rescan(root.path(), true, false, true, false, &ct).unwrap(); + assert_eq!(entries.len(), 1, "skip=true got: {}", entries.len()); + assert_eq!(entries[0].path, "app.js"); + + // Without skip: should get app.js + 100 node_modules files + directories + let entries = super::force_rescan(root.path(), true, false, false, false, &ct).unwrap(); + assert!(entries.len() > 100, "skip=false got: {}", entries.len()); + } } diff --git a/crates/pi-natives/src/glob.rs b/crates/pi-natives/src/glob.rs index 092419e77..3c18142cd 100644 --- a/crates/pi-natives/src/glob.rs +++ b/crates/pi-natives/src/glob.rs @@ -239,8 +239,9 @@ fn run_glob( { collect_files_from_picker(&config.root, &glob_set, &config, db, on_match, &ct)? } else if config.use_cache { + let skip_node_modules = !config.mentions_node_modules; let scan = - fs_cache::get_or_scan(&config.root, config.include_hidden, config.use_gitignore, &ct)?; + fs_cache::get_or_scan(&config.root, config.include_hidden, config.use_gitignore, skip_node_modules, &ct)?; let mut matches = filter_entries(&scan.entries, &glob_set, &config, on_match, &ct)?; // Empty-result recheck: if we got zero matches from a cached scan that's old // enough, force a rescan and try once more before returning empty. @@ -249,6 +250,7 @@ fn run_glob( &config.root, config.include_hidden, config.use_gitignore, + skip_node_modules, true, &ct, )?; @@ -256,10 +258,12 @@ fn run_glob( } matches } else { + let skip_node_modules = !config.mentions_node_modules; let fresh = fs_cache::force_rescan( &config.root, config.include_hidden, config.use_gitignore, + skip_node_modules, false, &ct, )?; diff --git a/crates/pi-natives/src/grep.rs b/crates/pi-natives/src/grep.rs index d676b55e0..da2240cae 100644 --- a/crates/pi-natives/src/grep.rs +++ b/crates/pi-natives/src/grep.rs @@ -1509,17 +1509,17 @@ fn grep_sync( &ct, )? } else if use_cache { - let scan = fs_cache::get_or_scan(&search_path, include_hidden, use_gitignore, &ct)?; + let scan = fs_cache::get_or_scan(&search_path, include_hidden, use_gitignore, true, &ct)?; let mut entries = collect_files(&search_path, &scan.entries, glob_set.as_ref(), type_filter.as_ref()); if entries.is_empty() && scan.cache_age_ms >= fs_cache::empty_recheck_ms() { let fresh = - fs_cache::force_rescan(&search_path, include_hidden, use_gitignore, true, &ct)?; + fs_cache::force_rescan(&search_path, include_hidden, use_gitignore, true, true, &ct)?; entries = collect_files(&search_path, &fresh, glob_set.as_ref(), type_filter.as_ref()); } entries } else { - let fresh = fs_cache::force_rescan(&search_path, include_hidden, use_gitignore, false, &ct)?; + let fresh = fs_cache::force_rescan(&search_path, include_hidden, use_gitignore, true, false, &ct)?; collect_files(&search_path, &fresh, glob_set.as_ref(), type_filter.as_ref()) }; // Check cancellation before heavy work